[client] Reconcile external nft accept rules on external manager reload#5912
[client] Reconcile external nft accept rules on external manager reload#5912lixmal merged 1 commit intoproto-ipv6-overlayfrom
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: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/firewall/nftables/external_chain_monitor_integration_linux_test.go (1)
45-53: Nit: drop_ = chain.
AddChainis called for its side effect of queueing the chain into the conn; the returned value isn't used. You can simply ignore it without the assignment:🧹 Proposed cleanup
- chain := conn.AddChain(&nftables.Chain{ + conn.AddChain(&nftables.Chain{ Name: "filter_INPUT", Table: table, Hooknum: nftables.ChainHookInput, Priority: nftables.ChainPriorityFilter, Type: nftables.ChainTypeFilter, }) - _ = chain require.NoError(t, conn.Flush(), "create external test chain")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/nftables/external_chain_monitor_integration_linux_test.go` around lines 45 - 53, The variable `chain` is unused after calling `conn.AddChain(...)`; remove the assignment and call `conn.AddChain(&nftables.Chain{...})` for the side effect only (delete the `_ = chain` line and change `chain := conn.AddChain(...)` to a plain call), leaving the subsequent `require.NoError(t, conn.Flush(), "create external test chain")` intact; references: AddChain, chain, conn.Flush.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/firewall/nftables/external_chain_monitor_integration_linux_test.go`:
- Around line 45-53: The variable `chain` is unused after calling
`conn.AddChain(...)`; remove the assignment and call
`conn.AddChain(&nftables.Chain{...})` for the side effect only (delete the `_ =
chain` line and change `chain := conn.AddChain(...)` to a plain call), leaving
the subsequent `require.NoError(t, conn.Flush(), "create external test chain")`
intact; references: AddChain, chain, conn.Flush.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7eb749ba-d28d-46df-a74f-76a24659f670
📒 Files selected for processing (5)
client/firewall/nftables/external_chain_monitor_integration_linux_test.goclient/firewall/nftables/external_chain_monitor_linux.goclient/firewall/nftables/external_chain_monitor_linux_test.goclient/firewall/nftables/manager_linux.goclient/firewall/nftables/router_linux.go
0ce5c3d to
27bd60b
Compare
|



Describe your changes
External nftables managers can flush or recreate their tables and chains at runtime (e.g. on
firewall-cmd --reload), which wipes the netbird passthrough accept rules and breaks new flows overwt0. This adds a netlink monitor that re-inserts the rules when relevant tables/chains appear.UserDatatag lookup; bail on lookup error so blind re-inserts don't accumulate duplicatesIssue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Internal firewall reconciliation behavior, no user-facing API change.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Tests