Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iptables: test real world roundtrip of iptables rules #52860

Merged
merged 5 commits into from
Sep 5, 2024
Merged

iptables: test real world roundtrip of iptables rules #52860

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e6d16cc
iptables: test real world roundtrip of iptables rules
howardjohn Aug 26, 2024
477fee5
wip
howardjohn Aug 26, 2024
cea0025
enable dns test
howardjohn Aug 27, 2024
9a3116b
Use new and old cleanup
howardjohn Aug 27, 2024
4761bb3
Fix new test setup
howardjohn Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Use new and old cleanup
  • Loading branch information
@howardjohn
howardjohn committed Aug 27, 2024
commit 9a3116bb8fbb57fae47de0ba9604dd101a000a60
33 changes: 25 additions & 8 deletions tools/istio-iptables/pkg/e2e/e2e_test.go
View file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e2e seems like an odd choice. maybe just .../pkg/test/cleanup_test.go

Original file line number Diff line number Diff line change
@@ -29,24 +29,31 @@ import (
"github.com/howardjohn/unshare-go/userns"

"istio.io/istio/pkg/log"
"istio.io/istio/pkg/slices"
"istio.io/istio/pkg/test/util/assert"
cleancmd "istio.io/istio/tools/istio-clean-iptables/pkg/cmd"
iptablescmd "istio.io/istio/tools/istio-iptables/pkg/cmd"
)

func TestIptablesCleanRoundTrip(t *testing.T) {
setup(t)
// We only have UID 0 in our namespace, so always set it to that
const proxyUID = "--proxy-uid=0"
t.Run("basic", func(t *testing.T) {
assert.NoError(t, runIptables(proxyUID))
assert.NoError(t, runIptablesClean(proxyUID))
roundtrip := func(t *testing.T, args ...string) {
// We only have UID 0 in our namespace, so always set it to that
args = append([]string{"--proxy-uid=0"}, args...)
// Apply, cleanup with the old approach
assert.NoError(t, runIptables(args...))
assert.NoError(t, runIptablesOldClean(args...))
validateIptablesClean(t)
// App, cleanup with the new approach
assert.NoError(t, runIptables(args...))
assert.NoError(t, runIptablesClean(args...))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I guess we probably don't, I missed that part of your PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does test istio-clean-iptables as well though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that's true. The old istio-clean-iptables method has currently zero e2e coverage

validateIptablesClean(t)
}
t.Run("basic", func(t *testing.T) {
roundtrip(t)
})
t.Run("dns", func(t *testing.T) {
assert.NoError(t, runIptables(proxyUID, "--redirect-dns"))
assert.NoError(t, runIptablesClean(proxyUID, "--redirect-dns"))
validateIptablesClean(t)
roundtrip(t, "--redirect-dns")
})
}

@@ -55,6 +62,9 @@ func validateIptablesClean(t *testing.T) {
if strings.Contains(cur, "ISTIO") {
t.Fatalf("Istio rules leftover: %v", cur)
Copy link
Contributor

@leosarra leosarra Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

istio/tools/istio-iptables/pkg/capture/run_test.go

Line 465 in 4153713

residueExists, deltaExists := iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)

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

if !ok || (strings.HasPrefix(chain, "ISTIO_") && len(rules) != len(currentRules)) {

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()) )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can iterate with improved validation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
if strings.Contains(cur, "-A") {
t.Fatalf("Rules: %v", cur)
}
}

var initialized = &sync.Once{}
@@ -81,6 +91,13 @@ func runIptables(args ...string) error {
}

func runIptablesClean(args ...string) error {
c := iptablescmd.GetCommand(log.DefaultOptions())
args = append(slices.Clone(args), "--cleanup-only")
c.SetArgs(args)
return c.Execute()
}

func runIptablesOldClean(args ...string) error {
c := cleancmd.GetCommand(log.DefaultOptions())
c.SetArgs(args)
return c.Execute()