iptables: test real world roundtrip of iptables rules #52860
Conversation
howardjohn
added
the
release-notes-none
howardjohn
requested review from
a team,
costinm,
linsun,
stevenctl,
ericvn and
hzxuzhonghu
as code owners
istio-testing
added
the
size/L
|
/retest |
howardjohn
force-pushed
the
iptables/e2e
branch
from
2d2b3c9 to
f32eaf4
Compare
| func validateIptablesClean(t *testing.T) { | ||
| cur := iptablesSave(t) | ||
| if strings.Contains(cur, "ISTIO") { | ||
| t.Fatalf("Istio rules leftover: %v", cur) |
There was a problem hiding this comment.
This will likely not be enough to completely validate that iptables are clean.
istio-clean-iptables is also tasked with cleaning non-jump rules in non-istio chains and may leave some leftovers is something is incorrect
e.g:
https://github.com/istio/istio/blob/master/tools/istio-clean-iptables/pkg/cmd/cleanup.go#L76-L80
For the purpose of #50328 e2e cleanup tests I have exposed VerifyIptablesState() to perform this validation
Edit: Actually not even VerifyIptablesState would be enough as I changed it to ignore non-jump rules in non-istio chains. This was done in order to not introduce false positives if the users tinkered with the iptables by themselves.
istio/tools/istio-iptables/pkg/capture/run.go
Line 815 in 4153713
For the tests there is the need to go deeper and rely directly the parsed output of iptables-restore (obtainable via cfg.ruleBuilder.GetStateFromSave(cmdOutput.string()) )
There was a problem hiding this comment.
maybe we can iterate with improved validation
There was a problem hiding this comment.
These are hard to use directly with this approach since we do not have an iptConfigurator.
I have updated it to use the old and new cleanup approach though
|
/retest |
There was a problem hiding this comment.
e2e seems like an odd choice. maybe just .../pkg/test/cleanup_test.go
howardjohn
force-pushed
the
iptables/e2e
branch
from
4764994 to
9a3116b
Compare
|
/retest |
| validateIptablesClean(t) | ||
| // App, cleanup with the new approach | ||
| assert.NoError(t, runIptables(args...)) | ||
| assert.NoError(t, runIptablesClean(args...)) |
There was a problem hiding this comment.
The --cleanup-only option is already verified in TestIdempotentUnequaledRerun in the file istio-iptables/pkg/capture/run_test.go.
https://github.com/istio/istio/blob/master/tools/istio-iptables/pkg/capture/run_test.go#L384-L393
The difference between the two is that the one in run_test.go uses the cleanup option programmatically by setting the istio-iptables/config instead of relying on the CLI.
Do you prefer to have a verification via CLI as well?
There was a problem hiding this comment.
hmm I guess we probably don't, I missed that part of your PR
There was a problem hiding this comment.
This does test istio-clean-iptables as well though
There was a problem hiding this comment.
yes that's true. The old istio-clean-iptables method has currently zero e2e coverage
This is a building block to test things like #52835 and #50328