[client] Add dual-stack nftables manager with IPv6 table support#5707
[client] Add dual-stack nftables manager with IPv6 table support#5707lixmal wants to merge 12 commits intoclient-ipv6-routingfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds IPv6 support by introducing an address-family abstraction and parallel IPv6 router/ACL components; nftables constructs, payload offsets, protocol numbers, and set handling are parameterized by address family and rule operations dispatch to the appropriate family. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Manager as Firewall Manager
participant IPv4 as IPv4 Router/ACL
participant IPv6 as IPv6 Router/ACL
participant nftables
Client->>Manager: AddPeerFiltering(rule)
alt rule is IPv4
Manager->>IPv4: dispatch rule
IPv4->>nftables: create/set IPv4 rules
else rule is IPv6
Manager->>IPv6: dispatch rule
IPv6->>nftables: create/set IPv6 rules
end
Client->>Manager: UpdateSet(mixed prefixes)
Manager->>Manager: split into v4/v6 batches
Manager->>IPv4: update IPv4 routes/sets
Manager->>IPv6: update IPv6 routes/sets
IPv4->>nftables: apply IPv4 expressions
IPv6->>nftables: apply IPv6 expressions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
lixmal
force-pushed
the
client-ipv6-nftables
branch
from
7f7b3ee to
e4857b4
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/nftables/router_linux.go`:
- Around line 182-183: acceptFilterTableRules() and removeFilterTableRules()
currently create an iptables client with iptables.New() (defaulting to IPv4)
which breaks when r.af.tableFamily is nftables.TableFamilyIPv6; update both
functions to conditionally instantiate the iptables client using
iptables.NewWithProtocol(iptables.ProtocolIPv6) when r.af.tableFamily ==
nftables.TableFamilyIPv6 and fall back to iptables.New() (or
iptables.NewWithProtocol(iptables.ProtocolIPv4)) otherwise, then pass that
client into acceptFilterRulesIptables() so the correct IPv4/IPv6 protocol is
used.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b6d8fa5-7ed3-4b3b-a8ed-21af1935e731
📒 Files selected for processing (5)
client/firewall/nftables/acl_linux.goclient/firewall/nftables/addr_family_linux.goclient/firewall/nftables/manager_linux.goclient/firewall/nftables/router_linux.goclient/firewall/nftables/router_linux_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/firewall/nftables/acl_linux.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/nftables/manager_linux_test.go`:
- Around line 409-415: The test currently only checks for ip6tables-save but
later calls seedIp6tables() (which uses ip6tables) and also runs iptables-save
later, so expand the guard: before calling seedIp6tables() add exec.LookPath
checks for "ip6tables" and "iptables-save" (in addition to the existing
"ip6tables-save") and call t.Skipf with a clear message if any are missing;
reference the existing exec.LookPath check and the seedIp6tables function to
locate where to insert the extra checks.
In `@client/firewall/nftables/manager_linux.go`:
- Around line 385-389: The current early return on m.router6.Reset() prevents
cleanupNetbirdTables() and the final flush from running; instead, call
m.router6.Reset() and if it returns an error capture it (e.g., append to an
error variable or wrap it), but do not return immediately — continue to call
cleanupNetbirdTables() and the final flush/finalizer (the same cleanup path
executed when Reset succeeds), then after all teardown steps complete return the
collected error (or nil) so teardown completes even when router6.Reset()
partially fails; reference m.hasIPv6(), m.router6.Reset(),
cleanupNetbirdTables() and the final flush call to locate and update the logic.
- Around line 148-153: When m.initIPv6() fails during Init(), perform a
best-effort rollback of the already-programmed IPv4 state by calling the router
and ACL teardown/close methods before returning: invoke m.router.close() (or
m.router.teardown()/m.router.Destroy() if that’s the actual name) and
m.aclManager.close() (or m.aclManager.teardown()/m.aclManager.Destroy()), log
any errors from those calls but do not overwrite the original IPv6 init error,
then return the wrapped IPv6 error from Init(); this ensures the v4 table and
external accept rules set by m.router.init() and m.aclManager.init() are undone
on failure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4d50d73-c799-4762-845b-d31a38c35fd2
📒 Files selected for processing (3)
client/firewall/nftables/manager_linux.goclient/firewall/nftables/manager_linux_test.goclient/firewall/nftables/router_linux.go
…rrors, expand test guards
|
There was a problem hiding this comment.
Please keep the Go convection. Vars -> struct -> at the end statuc functions
| rules: make(map[string]*nftables.Rule), | ||
| af: familyForAddr(workTable.Family == nftables.TableFamilyIPv4), | ||
| wgIface: wgIface, | ||
| ipFwdState: ipfwdstate.NewIPForwardingState(), |
There was a problem hiding this comment.
newRouter() always creates a fresh ipfwdstate.NewIPForwardingState().
As a result, router and router6 maintain separate counters.
When router6.AddDNATRule() is called, it increments router6.ipFwdState.
However, Manager.DisableRouting() only releases m.router.ipFwdState.
This means the router6 counter is never decremented, so IP forwarding is never fully disabled once a v6 DNAT rule has been added.
|
|
||
| return nil | ||
| // rollbackInit performs best-effort cleanup of already-initialized state when Init fails partway through. | ||
| func (m *Manager) rollbackInit() { |
2 participants
Describe your changes
addrFamilyabstraction encapsulating IPv4/IPv6 header offsets, address lengths, set key types, and ICMP protocol numbersip6 netbirdtable with its own router and ACL manager when the interface has IPv6UpdateSetprefixes by family for dynamic DNS route setsTestNftablesCreateIpSet_IPv6) andcalculateLastIPtests covering both familiesStacked on #5706.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#667
Summary by CodeRabbit
New Features
Tests