Fix rendering of NatPortRange in nftables mode#11736
Conversation
marvin-tigera
added
release-note-required
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the nftables implementation where the MasqAction was generating incorrect syntax when a port range was specified. The missing colon before the port range has been corrected and corresponding test cases have been added.
Changes:
- Fixed nftables MasqAction to include colon before port range (changed
"masquerade to %s"to"masquerade to :%s") - Added test cases for MasqAction with ToPorts in both nftables and iptables test suites
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| felix/nftables/actions.go | Fixed format string to include missing colon before port range in MasqAction.ToFragment() |
| felix/nftables/actions_test.go | Added test case to verify MasqAction renders correctly with ToPorts parameter |
| felix/iptables/actions_test.go | Added test case to verify MasqAction renders correctly with ToPorts parameter (for consistency) |
| 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"), |
There was a problem hiding this comment.
The test entry name "MasqAction" is duplicated on lines 39, 40, and 41. While this follows an existing pattern in this file, Ginkgo test entries should have unique names for better test identification and reporting. Consider using more descriptive names like "MasqAction with ToPorts", "MasqAction fully random", etc.
| 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"), |
There was a problem hiding this comment.
The test entry name "MasqAction" is duplicated on lines 39, 40, and 41. While this follows an existing pattern in this file, Ginkgo test entries should have unique names for better test identification and reporting. Consider using more descriptive names like "MasqAction with ToPorts", "MasqAction fully random", etc.
| Entry("MasqAction", environment.Features{}, MasqAction{ToPorts: "32768-65535"}, "masquerade to :32768-65535"), | ||
| Entry("MasqAction", environment.Features{MASQFullyRandom: true}, MasqAction{}, "masquerade fully-random"), |
There was a problem hiding this comment.
The MasqAction implementation supports both ToPorts and MASQFullyRandom features simultaneously (line 332 in actions.go appends fullyRand even when ToPorts is set), but there is no test case covering this combination. Consider adding a test case like: Entry("MasqAction with ToPorts and fully random", environment.Features{MASQFullyRandom: true}, MasqAction{ToPorts: "32768-65535"}, "masquerade to :32768-65535 fully-random")
| 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"), |
There was a problem hiding this comment.
The MasqAction implementation supports both ToPorts and MASQFullyRandom features simultaneously (line 313 in actions.go appends fullyRand even when ToPorts is set), but there is no test case covering this combination. Consider adding a test case like: Entry("MasqAction with ToPorts and fully random", environment.Features{MASQFullyRandom: true}, MasqAction{ToPorts: "32768-65535"}, "--jump MASQUERADE --to-ports 32768-65535 --random-fully")
nelljerram
added
docs-not-required
fasaxc
left a comment
fasaxc
left a comment
There was a problem hiding this comment.
Looks like we missed an FV test for this, otherwise it would have been failing but I suppose we've got a UT now
nelljerram
force-pushed
the
nftables-nat-port-range
branch
from
1ed3d91 to
344b8b1
Compare
|
I'm sorry but I failed delete the branch after merging the pull request. |
…ort-range Fix rendering of NatPortRange in nftables mode (cherry picked from commit 3856883)
…ort-range Fix rendering of NatPortRange in nftables mode (cherry picked from commit 3856883)
Fixes #11738
Release Note