[v3.31] Fix rendering of NatPortRange in nftables mode #11741
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,3 +109,74 @@ var _ = infrastructure.DatastoreDescribe("NATOutgoing rule rendering test", []ap | |
| } | ||
| }) | ||
| }) | ||
|
|
||
| var _ = infrastructure.DatastoreDescribe("NATPortRange rendering test", []apiconfig.DatastoreType{apiconfig.EtcdV3, apiconfig.Kubernetes}, func(getInfra infrastructure.InfraFactory) { | ||
| var ( | ||
| infra infrastructure.DatastoreInfra | ||
| tc infrastructure.TopologyContainers | ||
| client client.Interface | ||
| dumpedDiags bool | ||
| ) | ||
|
|
||
| BeforeEach(func() { | ||
| var err error | ||
| infra = getInfra() | ||
|
|
||
| dumpedDiags = false | ||
| opts := infrastructure.DefaultTopologyOptions() | ||
| opts.IPIPMode = api.IPIPModeNever | ||
| opts.EnableIPv6 = true | ||
|
|
||
| opts.ExtraEnvVars = map[string]string{ | ||
| "FELIX_NATPortRange": "32768:65535", | ||
| } | ||
| tc, client = infrastructure.StartSingleNodeTopology(opts, infra) | ||
|
|
||
| ctx := context.Background() | ||
| ippool := api.NewIPPool() | ||
| ippool.Name = "nat-pool" | ||
| ippool.Spec.CIDR = "10.244.255.0/24" | ||
| ippool.Spec.NATOutgoing = true | ||
| ippool, err = client.IPPools().Create(ctx, ippool, options.SetOptions{}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| // Utility function to dump diags if the test failed. Should be called in the inner-most | ||
| // AfterEach() to dump diags before the test is torn down. Only the first call for a given | ||
| // test has any effect. | ||
| dumpDiags := func() { | ||
| if !CurrentGinkgoTestDescription().Failed || dumpedDiags { | ||
| return | ||
| } | ||
| if NFTMode() { | ||
| logNFTDiags(tc.Felixes[0]) | ||
| } else { | ||
| iptSave, err := tc.Felixes[0].ExecOutput("iptables-save", "-c") | ||
| if err == nil { | ||
| log.Info("iptables-save:\n" + iptSave) | ||
| } | ||
| } | ||
| dumpedDiags = true | ||
| infra.DumpErrorData() | ||
| } | ||
|
|
||
| AfterEach(func() { | ||
| dumpDiags() | ||
| tc.Stop() | ||
| infra.Stop() | ||
| }) | ||
|
|
||
| It("should have expected rendering", func() { | ||
| if NFTMode() { | ||
| Eventually(func() string { | ||
| output, _ := tc.Felixes[0].ExecOutput("nft", "list", "chain", "ip", "calico", "nat-cali-nat-outgoing") | ||
| return output | ||
| }, 5*time.Second, 100*time.Millisecond).Should(ContainSubstring("32768-65535")) | ||
|
Comment on lines
+170
to
+174
There was a problem hiding this comment. The NFT-mode assertion only checks for the port range substring ("32768-65535"). That would still pass even if the rendered rule is missing the required leading colon (the bug this PR is fixing). Please assert on the full expected fragment for nftables (e.g. include "to :32768-65535" / ":32768-65535").
Member
Author
There was a problem hiding this comment. Yes I could have included the colon in the test. However it's wrong to say that the test would pass before the bug fix: in fact what happened is that nft execution failed with an error, because of the missing colon, and hence those rules did not get programmed at all, so the test still failed even without the colon. |
||
| } else { | ||
| Eventually(func() string { | ||
| output, _ := tc.Felixes[0].ExecOutput("iptables-save", "-t", "nat") | ||
| return output | ||
| }, 5*time.Second, 100*time.Millisecond).Should(ContainSubstring("32768-65535")) | ||
| } | ||
| }) | ||
| }) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,9 @@ var _ = DescribeTable("Actions", | |
| Entry("SNATAction", environment.Features{}, SNATAction{ToAddr: "10.0.0.1"}, "--jump SNAT --to-source 10.0.0.1"), | ||
| Entry("SNATAction fully random", environment.Features{SNATFullyRandom: true}, SNATAction{ToAddr: "10.0.0.1"}, "--jump SNAT --to-source 10.0.0.1 --random-fully"), | ||
| Entry("MasqAction", environment.Features{}, MasqAction{}, "--jump MASQUERADE"), | ||
| Entry("MasqAction", environment.Features{}, MasqAction{ToPorts: "32768-65535"}, "--jump MASQUERADE --to-ports 32768-65535"), | ||
| Entry("MasqAction", environment.Features{MASQFullyRandom: true}, MasqAction{}, "--jump MASQUERADE --random-fully"), | ||
| Entry("MasqAction", environment.Features{MASQFullyRandom: true}, MasqAction{ToPorts: "32768-65535"}, "--jump MASQUERADE --to-ports 32768-65535 --random-fully"), | ||
|
Comment on lines
39
to
+42
There was a problem hiding this comment. These new table entries reuse the same description string ("MasqAction") as existing entries. In Ginkgo Table output, that makes failures hard to attribute to a specific case; please give the new entries distinct names (e.g. indicate "with ports" and/or "fully random"). |
||
| Entry("ClearMarkAction", environment.Features{}, ClearMarkAction{Mark: 0x1000}, "--jump MARK --set-mark 0/0x1000"), | ||
| Entry("SetMarkAction", environment.Features{}, SetMarkAction{Mark: 0x1000}, "--jump MARK --set-mark 0x1000/0x1000"), | ||
| Entry("SetMaskedMarkAction", environment.Features{}, SetMaskedMarkAction{ | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -37,7 +37,9 @@ var _ = DescribeTable("Actions", | |||||||||||||
| Entry("SNATAction", environment.Features{}, SNATAction{ToAddr: "10.0.0.1"}, "snat to 10.0.0.1"), | ||||||||||||||
| Entry("SNATAction fully random", environment.Features{SNATFullyRandom: true}, SNATAction{ToAddr: "10.0.0.1"}, "snat to 10.0.0.1 fully-random"), | ||||||||||||||
| Entry("MasqAction", environment.Features{}, MasqAction{}, "masquerade"), | ||||||||||||||
| Entry("MasqAction", environment.Features{}, MasqAction{ToPorts: "32768-65535"}, "masquerade to :32768-65535"), | ||||||||||||||
| Entry("MasqAction", environment.Features{MASQFullyRandom: true}, MasqAction{}, "masquerade fully-random"), | ||||||||||||||
| Entry("MasqAction", environment.Features{MASQFullyRandom: true}, MasqAction{ToPorts: "32768-65535"}, "masquerade to :32768-65535 fully-random"), | ||||||||||||||
|
Comment on lines
+40
to
+42
There was a problem hiding this comment. These new table entries reuse the same description string ("MasqAction") as existing entries. In Ginkgo Table output, that makes failures hard to attribute to a specific case; please give the new entries distinct names (e.g. indicate "with ports" and/or "fully random").
Suggested change
Member
Author
There was a problem hiding this comment. Correct but it seems conventional in this file. |
||||||||||||||
| Entry("ClearMarkAction", environment.Features{}, ClearMarkAction{Mark: 0x1000}, "meta mark set mark & 0xffffefff"), | ||||||||||||||
| Entry("SetMarkAction", environment.Features{}, SetMarkAction{Mark: 0x1000}, "meta mark set mark or 0x1000"), | ||||||||||||||
| Entry("SetMaskedMarkAction", environment.Features{}, SetMaskedMarkAction{Mark: 0x1000, Mask: 0xf000}, "meta mark set mark & 0xffff0fff ^ 0x1000"), | ||||||||||||||
|
|
||||||||||||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new FV test starts topology containers (
StartSingleNodeTopology) and a datastore infra but never stops them. Please add anAfterEach(like the earlier NATOutgoing test in this file) to calltc.Stop()andinfra.Stop()so the test suite doesn't leak containers/resources and become flaky.