[v3.31] Fix rendering of NatPortRange in nftables mode#11741
Conversation
…ort-range Fix rendering of NatPortRange in nftables mode (cherry picked from commit 3856883)
nelljerram
added
release-note-required
There was a problem hiding this comment.
Pull request overview
Fixes nftables NAT port-range rendering for MASQUERADE/SNAT so Felix can successfully program nft rules when natPortRange is set.
Changes:
- Update nftables
MasqActionrendering to emitmasquerade to :<min>-<max>(leading:required by nft syntax). - Extend iptables/nftables unit tests to cover
MasqActionwithToPorts. - Add an FV test intended to validate NAT port-range rendering.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| felix/nftables/actions.go | Fix nftables masquerade syntax when a port range is configured. |
| felix/nftables/actions_test.go | Add unit coverage for nftables MasqAction port-range rendering. |
| felix/iptables/actions_test.go | Add unit coverage for iptables MasqAction port-range rendering. |
| felix/fv/nat_outgoing_test.go | Add FV test for NAT port-range rendering across iptables/nft modes. |
| 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"), |
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("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"), | |
| Entry("MasqAction with ports", environment.Features{}, MasqAction{ToPorts: "32768-65535"}, "masquerade to :32768-65535"), | |
| Entry("MasqAction fully random", environment.Features{MASQFullyRandom: true}, MasqAction{}, "masquerade fully-random"), | |
| Entry("MasqAction fully random with ports", environment.Features{MASQFullyRandom: true}, MasqAction{ToPorts: "32768-65535"}, "masquerade to :32768-65535 fully-random"), |
There was a problem hiding this comment.
Correct but it seems conventional in this file.
| 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"), |
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").
| opts.ExtraEnvVars = map[string]string{ | ||
| "FELIX_NATPortRange": "32768:65535", | ||
| } | ||
| tc, client = infrastructure.StartSingleNodeTopology(opts, infra) | ||
|
|
There was a problem hiding this comment.
This new FV test starts topology containers (StartSingleNodeTopology) and a datastore infra but never stops them. Please add an AfterEach (like the earlier NATOutgoing test in this file) to call tc.Stop() and infra.Stop() so the test suite doesn't leak containers/resources and become flaky.
| 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")) |
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").
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.
|
Removing "merge-when-ready" label due to new commits |
Cherry-pick history
Fixes Providing a natPortRange when using Nftables leads to a parsing issue when applied #11738
Release Note