Automated backport of #3979: Fix IPv6 protocol conflict in nftables rules#3987
Conversation
Remove RuleActionSelfSNAT from the generic rule action map. IPv6 rules were failing because the generic "snat to ip saddr" was being applied to all IP families. This needs per-family handling. Add tests to verify IPv6 protocol headers work correctly. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis
requested review from
Oats87,
mkolesnik,
skitt,
sridhargaddam,
vthapar and
yboaron
as code owners
|
🤖 Created branch: z_pr3987/tpantelis/automated-backport-of-#3979-upstream-release-0.23 |
WalkthroughThis pull request introduces IPv6 support to nftables packet filtering. Changes include removing Changes
Sequence DiagramsequenceDiagram
participant Client
participant RuleConverter as Rule Converter
participant ProtoHandler as Protocol Handler
participant FamilyHandler as Address Family Handler
participant ActionHandler as Action Handler
participant Renderer as NFTables Renderer
Client->>RuleConverter: toNftRuleSpec(rule, isIPv6=true)
RuleConverter->>ProtoHandler: protoToRuleSpec(protocol, isIPv6=true)
alt IPv6 Protocol Branch
ProtoHandler->>FamilyHandler: Check if UDP/TCP/ICMP
alt ICMP Protocol
FamilyHandler->>FamilyHandler: Map ICMP → icmpv6
FamilyHandler->>Renderer: Emit "ip6 nexthdr icmpv6"
else Other Protocols
FamilyHandler->>Renderer: Emit "ip6 nexthdr <proto>"
end
else IPv4 Protocol Branch
ProtoHandler->>Renderer: Emit "ip protocol <proto>"
end
RuleConverter->>ActionHandler: Handle RuleActionSelfSNAT
alt SelfSNAT Action
ActionHandler->>Renderer: Emit "snat to ip6 saddr" (if IPv6) or "snat to ip saddr" (if IPv4)
else Other Actions
ActionHandler->>Renderer: Emit ruleActionToStr[action]
end
Renderer->>Client: Return serialized rule spec
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/packetfilter/nftables/rule_conversion.go (1)
98-140: Core fix looks correct.The explicit
snat to <prefix> saddremission forRuleActionSelfSNAT(lines 136–140) using the computedsetPrefixcorrectly producessnat to ip saddrfor IPv4 andsnat to ip6 saddrfor IPv6, resolving the protocol conflict that stemmed from a single hard-coded mapping inruleActionToStr.Optional nit:
setPrefixis computed identically here (lines 101–105) and insidesetToRuleSpec(lines 81–85). PassingsetPrefixintosetToRuleSpec(or computing it once at the top and reusing) would eliminate the small duplication — but this is purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/packetfilter/nftables/rule_conversion.go` around lines 98 - 140, The code computes setPrefix twice (once in toNftRuleSpec and again inside setToRuleSpec) which is duplicate; modify to compute setPrefix once in toNftRuleSpec and pass that variable into setToRuleSpec (add a parameter to setToRuleSpec and update its callers) so setPrefix is reused, keeping the existing behavior for IPv4/IPv6 and leaving the explicit RuleActionSelfSNAT handling unchanged.pkg/packetfilter/nftables/nftables_test.go (1)
403-452: Nice, targeted IPv6 coverage.The new suite precisely targets the three IPv6-specific rendering rules changed in
rule_conversion.go(UDP nexthdr, ICMPv6, and per-family SelfSNAT), and correctly usesfakeNft6.Fake.ListRulesto bypass the wrapper that clears theRulefield.One optional thought: consider also adding a symmetric IPv4 assertion for
RuleActionSelfSNAT(expectingsnat to ip saddr) so both family branches intoNftRuleSpecare covered, since this is the regression at the heart of the PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/packetfilter/nftables/nftables_test.go` around lines 403 - 452, Add a symmetric IPv4 assertion for the SelfSNAT rendering: in the IPv4 test suite (where the IPv4 driver/pf is created, analogous to fakeNft6/pf6 and getRuleSpec6), append a test that calls pf.Append with packetfilter.RuleActionSelfSNAT and then asserts the rendered rule (via the fake nft ListRules helper used for IPv4) contains "snat to ip saddr"; reference the same symbols used in the IPv6 tests (RuleActionSelfSNAT, Append, and the fake nft ListRules wrapper) so both branches exercised by toNftRuleSpec are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/packetfilter/nftables/nftables_test.go`:
- Around line 440-443: The negative assertion in the IPv6 ICMP test is checking
the wrong IPv4 substring; update the assertion that references getRuleSpec6() to
assert Not(ContainSubstring("ip protocol icmp")) instead of
Not(ContainSubstring("ip nexthdr icmp")) so it mirrors the IPv4 UDP/ICMP check
(see the UDP assertion near lines 431–432) and actually prevents accidental
IPv4-style formatting being emitted.
---
Nitpick comments:
In `@pkg/packetfilter/nftables/nftables_test.go`:
- Around line 403-452: Add a symmetric IPv4 assertion for the SelfSNAT
rendering: in the IPv4 test suite (where the IPv4 driver/pf is created,
analogous to fakeNft6/pf6 and getRuleSpec6), append a test that calls pf.Append
with packetfilter.RuleActionSelfSNAT and then asserts the rendered rule (via the
fake nft ListRules helper used for IPv4) contains "snat to ip saddr"; reference
the same symbols used in the IPv6 tests (RuleActionSelfSNAT, Append, and the
fake nft ListRules wrapper) so both branches exercised by toNftRuleSpec are
covered.
In `@pkg/packetfilter/nftables/rule_conversion.go`:
- Around line 98-140: The code computes setPrefix twice (once in toNftRuleSpec
and again inside setToRuleSpec) which is duplicate; modify to compute setPrefix
once in toNftRuleSpec and pass that variable into setToRuleSpec (add a parameter
to setToRuleSpec and update its callers) so setPrefix is reused, keeping the
existing behavior for IPv4/IPv6 and leaving the explicit RuleActionSelfSNAT
handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bedb5d9-07d3-4e40-846a-a057ee354bd9
📒 Files selected for processing (4)
.github/workflows/e2e.ymlpkg/packetfilter/nftables/nftables.gopkg/packetfilter/nftables/nftables_test.gopkg/packetfilter/nftables/rule_conversion.go
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
…3979-upstream-release-0.23
|
🤖 Closed branches: [z_pr3987/tpantelis/automated-backport-of-#3979-upstream-release-0.23] |
3 participants
Backport of #3979 on release-0.23.
#3979: Fix IPv6 protocol conflict in nftables rules
For details on the backport process, see the backport requests page.
Summary by CodeRabbit
Release Notes
New Features
Tests