[client] Passthrough all non-NetBird chains to prevent them from dropping NetBird traffic#4899
[client] Passthrough all non-NetBird chains to prevent them from dropping NetBird traffic#4899
Conversation
lixmal
changed the title
WalkthroughAdds runtime discovery and deterministic handling of external nftables chains, makes nftables accept/remove flows table-aware, introduces helpers for hook/family naming and external-chain detection, and refactors insertion/removal into per-table and per-chain operations with aggregated error handling and expanded table constants. (34 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/firewall/nftables/router_linux.go (2)
1248-1264: Deterministic cleanup is well-designed.The runtime scanning in
removeExternalChainsRulesensures proper cleanup even if chains changed after startup or NetBird crashed. Good approach for resilience.Note: This function logs errors as warnings without aggregating them (line 1259), unlike other removal functions that use multierror. This makes cleanup more resilient but loses error visibility. Consider whether you want to aggregate these errors while still continuing the cleanup loop.
1266-1306: Consider extracting chain filter logic to reduce complexity.SonarCloud flagged this function with cognitive complexity 23 (threshold 20). While the logic is correct, extracting the chain filtering conditions (lines 1280-1299) into a helper method like
isExternalChainRelevant(chain *nftables.Chain) boolwould improve readability and testability.Example refactor:
+func (r *router) isExternalChainRelevant(chain *nftables.Chain) bool { + if r.workTable != nil && chain.Table.Name == r.workTable.Name { + return false + } + if r.filterTable != nil && chain.Table.Name == r.filterTable.Name && chain.Table.Family == r.filterTable.Family { + return false + } + if chain.Type != nftables.ChainTypeFilter { + return false + } + if chain.Hooknum == nil { + return false + } + if *chain.Hooknum != *nftables.ChainHookForward && *chain.Hooknum != *nftables.ChainHookInput { + return false + } + return true +} + func (r *router) findExternalChains() []*nftables.Chain { var chains []*nftables.Chain families := []nftables.TableFamily{nftables.TableFamilyIPv4, nftables.TableFamilyINet} for _, family := range families { allChains, err := r.conn.ListChainsOfTableFamily(family) if err != nil { log.Debugf("list chains for family %d: %v", family, err) continue } for _, chain := range allChains { - if r.workTable != nil && chain.Table.Name == r.workTable.Name { - continue - } - if r.filterTable != nil && chain.Table.Name == r.filterTable.Name && chain.Table.Family == r.filterTable.Family { - continue - } - if chain.Type != nftables.ChainTypeFilter { - continue - } - if chain.Hooknum == nil { - continue - } - if *chain.Hooknum != *nftables.ChainHookForward && *chain.Hooknum != *nftables.ChainHookInput { - continue - } - chains = append(chains, chain) + if r.isExternalChainRelevant(chain) { + chains = append(chains, chain) + } } } return chains }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go(8 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
client/firewall/nftables/router_linux.go
[failure] 1268-1268: Refactor this method to reduce its Cognitive Complexity from 23 to the 20 allowed.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
🔇 Additional comments (6)
client/firewall/nftables/router_linux.go (6)
94-94: LGTM! Appropriate log level change.Downgrading from warning to debug is appropriate here since the absence of a filter table is handled gracefully and doesn't indicate a problem.
186-198: LGTM! Clean helper for readable logging.The nil check prevents panics, and the hook mapping improves log readability. Well done.
946-958: LGTM! Clean refactoring with proper error aggregation.The split into filter table and external chains paths improves code organization, and error handling using multierror is appropriate.
1015-1090: LGTM! Good refactoring for flexibility.Parameterizing the table makes this function more flexible and reusable. The implementation correctly uses the provided table parameter throughout.
1092-1179: Accept rules are only added at startup.The dynamic discovery in
acceptExternalChainsRulesruns during initialization, so external chains created after NetBird starts won't receive accept rules until NetBird restarts. This is likely acceptable since most firewall tools (firewalld, docker) create their chains during system boot, but consider whether periodic re-scanning would be beneficial for your use case.The implementation itself looks correct—rules are properly inserted with UserData tags for later cleanup.
Do you want to add periodic re-scanning of chains, or is startup-only insertion sufficient for your deployment scenarios?
103-105: Cleanup at init ensures idempotent startup.Calling
removeAcceptFilterRules()during initialization (before adding new rules) ensures clean state even after crashes. Good defensive programming.
lixmal
force-pushed
the
nftables-all-chains
branch
2 times, most recently
from
203753e to
daec924
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/firewall/nftables/router_linux.go (1)
983-1001: Fix multierror.Append usage to properly accumulate errorsIn both
acceptFilterRulesIptables()andremoveAcceptFilterRulesIptables(), the first argument tomultierror.Appendis the current iteration'serrinstead of the accumulatormerr, causing earlier errors to be discarded:merr = multierror.Append(err, fmt.Errorf("add iptables forward rule: %v", err))Should be:
merr = multierror.Append(merr, fmt.Errorf("add iptables forward rule: %v", err))This applies to all error handling in both functions. The correct pattern is used elsewhere in the file, so this appears to be an inconsistency. Each iteration overwrites
merrwith a new multierror containing only the current failure, losing previous errors.
🧹 Nitpick comments (3)
client/firewall/nftables/router_linux.go (3)
91-96: Distinguish “filter table not found” from other nftables errors in newRouterRight now all failures from
loadFilterTable()are treated as “filter table not found” and silently ignored. It would be safer to only downgradeerrFilterTableNotFoundwhile surfacing or at least warning on other errors (e.g. netlink permission issues):- r.filterTable, err = r.loadFilterTable() - if err != nil { - log.Debugf("ip filter table not found: %v", err) - } + r.filterTable, err = r.loadFilterTable() + if err != nil { + if errors.Is(err, errFilterTableNotFound) { + log.Debugf("ip filter table not found: %v", err) + } else { + return nil, fmt.Errorf("load filter table: %w", err) + } + }This keeps the “no filter table” case non-fatal without masking genuine nftables failures.
1181-1215: removeAcceptRulesFromChain/removeExternalChainsRules behave as best‑effort cleanup
removeAcceptRulesFromChain()usesUserDataequality on the three NetBird markers and avoids touching other rules, which is exactly what you want.removeExternalChainsRules()then iterates all external chains, logs per‑chain failures as warnings, and only returns a flush error, making this path intentionally best‑effort.If you later need stronger guarantees (e.g. for
Reset()), consider aggregating per‑chain errors viamultierrorinstead of logging them only.
1217-1256: findExternalChains implements the right selection logic; Sonar complexity warning is low‑priorityThe function correctly:
- Scans IPv4 and INet families,
- Excludes the NetBird work table and the configured filter table,
- Restricts to
ChainTypeFilter, and- Keeps only
FORWARD/INPUTchains with non‑nil hooks.That matches the external‑chain contract well. Sonar’s Cognitive Complexity of 23 vs the 20 threshold is mostly from the series of
iffilters inside nested loops. If you want to appease it without changing behavior, you could extract the chain‑filtering logic into a small helper:+func (r *router) isExternalFilterChain(chain *nftables.Chain) bool { + if r.workTable != nil && chain.Table.Name == r.workTable.Name { + return false + } + if r.filterTable != nil && + chain.Table.Name == r.filterTable.Name && + chain.Table.Family == r.filterTable.Family { + return false + } + if chain.Type != nftables.ChainTypeFilter || chain.Hooknum == nil { + return false + } + return *chain.Hooknum == *nftables.ChainHookForward || + *chain.Hooknum == *nftables.ChainHookInput +} + func (r *router) findExternalChains() []*nftables.Chain { var chains []*nftables.Chain @@ - for _, chain := range allChains { - if r.workTable != nil && chain.Table.Name == r.workTable.Name { - continue - } - if r.filterTable != nil && chain.Table.Name == r.filterTable.Name && chain.Table.Family == r.filterTable.Family { - continue - } - if chain.Type != nftables.ChainTypeFilter { - continue - } - if chain.Hooknum == nil { - continue - } - if *chain.Hooknum != *nftables.ChainHookForward && *chain.Hooknum != *nftables.ChainHookInput { - continue - } - chains = append(chains, chain) - } + for _, chain := range allChains { + if r.isExternalFilterChain(chain) { + chains = append(chains, chain) + } + }Functionally it’s equivalent and should satisfy the analyzer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go(8 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
client/firewall/nftables/router_linux.go
[failure] 1219-1219: Refactor this method to reduce its Cognitive Complexity from 23 to the 20 allowed.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: release_ui_darwin
- GitHub Check: release
🔇 Additional comments (7)
client/firewall/nftables/router_linux.go (7)
186-198: hookName helper is nil‑safe and improves logging clarityThe
hookNamehelper cleanly handlesnilhooks and maps known hooks to readable names, with a sane fallback for others. This is a nice improvement for debug logs with no obvious behavioral risk.
947-980: Aggregating errors from filter‑table and external‑chain accept paths looks good
acceptForwardRules()now attempts bothacceptFilterTableRules()andacceptExternalChainsRules()and aggregates failures viamultierror, so one path failing doesn’t prevent the other from being tried. That matches the PR goal of broadening coverage while still surfacing combined errors throughnberrors.FormatErrorOrNil.
960-980: iptables→nftables fallback for filter table is reasonable, but depends on environmentUsing iptables when available and falling back to nftables (
acceptFilterRulesNftables(r.filterTable)) wheniptables.New()fails provides a pragmatic compatibility story. The deferred log that reports which backend was used is also helpful for debugging. Just be aware cleanup must mirror whichever backend actually succeeded, which you handle via the corresponding removal paths below.
1015-1039: acceptFilterRulesNftables correctly parameterizes the table and reuses insert helpersPassing the table into
acceptFilterRulesNftables()and constructingFORWARD/INPUTchains on that table, then delegating toinsertForwardAcceptRules/insertInputAcceptRule, is a clean abstraction. The finalFlush()centralizes error handling while the individualInsertRulecalls remain simple.
1041-1068: acceptExternalChainsRules aligns with the PR objective; behavior seems soundDynamically discovering external
FORWARD/INPUTfilter chains and inserting the same iif/oif/established rules into each (then flushing once) matches the intent to avoid NetBird traffic being dropped by third‑party tables. ThehookNameusage keeps logs readable, andfindExternalChains()already guaranteesHooknumis non‑nil before theswitch.
1070-1128: insertForwardAcceptRules/insertInputAcceptRule give deterministic, removable rulesThese helpers correctly:
- Match by
IIFNAME/OIFNAMEagainst the WireGuard interface,- Tag rules with
UserData(frwacceptiif/frwacceptoif/inputaccept) for deterministic cleanup, and- Use
InsertRuleso NetBird’s accept rules precede existing chain rules.The shared helpers avoid duplication and line up with the removal logic later that looks for these
UserDatamarkers.
1130-1179: Filter‑table + external‑chain cleanup flow is consistent; minor removal behavior note
removeAcceptFilterRules()now orchestrates:
removeFilterTableRules()(iptables‑first, with nftables fallback viaremoveAcceptRulesFromTable), andremoveExternalChainsRules()for non‑NetBird tables.
removeAcceptRulesFromTable()then scopes deletions to theFORWARD/INPUTchains of the given table and only removes rules carrying yourUserDatatags, finishing with a singleFlush(). This gives a coherent add/remove story for both internal and external chains with low risk of collateral deletion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/firewall/nftables/router_linux.go (3)
946-1041: Confirm behavior whenfilterTableis nil so iptables-only setups still get accept rules
acceptFilterTableRulesnow returns immediately whenr.filterTable == nil(Line 961), so you never attempt the iptables path on systems where the nftables"filter"table cannot be discovered. On an iptables‑only / iptables‑legacy system that doesn’t expose a nftables filter table, this would mean no forward/input accept rules are installed at all, even thoughiptables.New()might succeed.If iptables‑only environments are still supported, consider decoupling the iptables flow from nftables detection, e.g.:
- Always try
iptables.New()first.- Only fall back to
acceptFilterRulesNftableswhenr.filterTable != nilandiptables.New()fails.That keeps nftables fallback but avoids skipping iptables in cases where nftables state isn’t available.
1132-1217: Make nftables removal best-effort across all chains (mirror external-chain behavior)
removeAcceptFilterRulesaggregates errors, butremoveAcceptRulesFromTablecurrently:
- Iterates chains and returns immediately on the first error from
removeAcceptRulesFromChain(Lines 1175–1177).- Only flushes if no per-chain error occurred (Lines 1180–1181).
This contrasts with
removeExternalChainsRules, which logs per-chain failures, continues, and then flushes once. For resilience, especially after partial failures, you may want filter‑table cleanup to be similarly best‑effort:
- Iterate all relevant chains, collecting errors into a
*multierror.Error.- Always attempt the final
Flush, appending any flush error.- Return
FormatErrorOrNilat the end.That avoids leaving stale rules in later chains when a single chain deletion fails.
-func (r *router) removeAcceptRulesFromTable(table *nftables.Table) error { - chains, err := r.conn.ListChainsOfTableFamily(table.Family) +func (r *router) removeAcceptRulesFromTable(table *nftables.Table) error { + chains, err := r.conn.ListChainsOfTableFamily(table.Family) if err != nil { return fmt.Errorf("list chains: %v", err) } - for _, chain := range chains { + var merr *multierror.Error + for _, chain := range chains { if chain.Table.Name != table.Name { continue } if chain.Name != chainNameForward && chain.Name != chainNameInput { continue } - if err := r.removeAcceptRulesFromChain(table, chain); err != nil { - return err - } + if err := r.removeAcceptRulesFromChain(table, chain); err != nil { + merr = multierror.Append(merr, err) + } } - return r.conn.Flush() + if err := r.conn.Flush(); err != nil { + merr = multierror.Append(merr, fmt.Errorf("flush: %w", err)) + } + return nberrors.FormatErrorOrNil(merr) }
1221-1258: Consider extracting a small predicate helper to reducefindExternalChainscomplexity (Sonar warning)
findExternalChainshas a series of earlycontinuefilters (work table, filter table, type, hook, families loop), which Sonar flags as cognitive complexity 23 (>20). The logic is fine, but you can satisfy the check and improve readability by pulling the chain filter into a helper, e.g.isExternalFilterHookChain(chain *nftables.Chain) bool, and then doing a singleif !r.isExternalFilterHookChain(chain) { continue }inside the inner loop.That keeps behavior unchanged while addressing the static-analysis concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go(8 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
client/firewall/nftables/router_linux.go
[failure] 1221-1221: Refactor this method to reduce its Cognitive Complexity from 23 to the 20 allowed.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
🔇 Additional comments (2)
client/firewall/nftables/router_linux.go (2)
186-198: hookName helper looks robust for logging and nil-safetyMapping known hooks to human-readable names and guarding against
nilavoids panics when logging external chains; no issues spotted here.
1043-1130: External chain handling and rule helpers are consistent; good reuse of UserData markersThe new
acceptExternalChainsRules+insertForwardAcceptRules/insertInputAcceptRuleflow cleanly:
findExternalChainsscopes to non‑NetBird, filter‑type chains with FORWARD/INPUT hooks, so you don’t touch your own work/filter tables.- Forward/input rules use distinct
UserDatamarkers (frwacceptiif,frwacceptoif,inputaccept), which align with the removal helpers and keep cleanup deterministic.- Using
InsertRuleinstead ofAddRuleis appropriate to get rules near the top of the chain so they can bypass DROP policies.Overall this looks correct and matches the PR goal of protecting NetBird traffic across third‑party chains.
lixmal
force-pushed
the
nftables-all-chains
branch
from
daec924 to
11fe967
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/firewall/nftables/router_linux.go (2)
1171-1192: Consider aggregating errors instead of early return.Currently, if
removeAcceptRulesFromChainfails for one chain, subsequent chains won't be processed. This could leave the system in an inconsistent state. Consider aggregating errors withmultierror.Appendto ensure all chains are processed.func (r *router) removeAcceptRulesFromTable(table *nftables.Table) error { chains, err := r.conn.ListChainsOfTableFamily(table.Family) if err != nil { return fmt.Errorf("list chains: %v", err) } + var merr *multierror.Error + for _, chain := range chains { if chain.Table.Name != table.Name { continue } if chain.Name != chainNameForward && chain.Name != chainNameInput { continue } if err := r.removeAcceptRulesFromChain(table, chain); err != nil { - return err + merr = multierror.Append(merr, err) } } - return r.conn.Flush() + if err := r.conn.Flush(); err != nil { + merr = multierror.Append(merr, fmt.Errorf("flush: %w", err)) + } + + return nberrors.FormatErrorOrNil(merr) }
1212-1228: Consider returning aggregated errors from external chain cleanup.The function logs warnings for errors but doesn't return them. This could hide cleanup failures from the caller. Consider aggregating errors with
multierror.Appendand returning them.func (r *router) removeExternalChainsRules() error { chains := r.findExternalChains() if len(chains) == 0 { return nil } + var merr *multierror.Error + for _, chain := range chains { if err := r.removeAcceptRulesFromChain(chain.Table, chain); err != nil { log.Warnf("remove rules from external chain %s/%s: %v", chain.Table.Name, chain.Name, err) + merr = multierror.Append(merr, fmt.Errorf("remove rules from %s/%s: %w", chain.Table.Name, chain.Name, err)) } } - return r.conn.Flush() + if err := r.conn.Flush(); err != nil { + merr = multierror.Append(merr, fmt.Errorf("flush: %w", err)) + } + + return nberrors.FormatErrorOrNil(merr) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/firewall/nftables/router_linux.go (2)
client/errors/errors.go (1)
FormatErrorOrNil(25-30)client/firewall/nftables/rule_linux.go (1)
Rule(10-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: release_ui_darwin
🔇 Additional comments (15)
client/firewall/nftables/router_linux.go (15)
29-34: LGTM!The additional table name constants are correctly defined and align with standard nftables table names.
204-215: LGTM!The
familyNamehelper correctly maps nftables table families to readable strings with appropriate fallback handling.
963-975: LGTM!The refactoring properly separates filter table handling from external chain handling, with appropriate error aggregation.
977-999: LGTM!The function correctly tries iptables first and falls back to nftables when iptables is unavailable, with appropriate logging.
1001-1020: LGTM!Error messages are appropriately updated and error handling is consistent.
1034-1058: LGTM!The refactoring to accept a table parameter improves reusability, and the chain structures are correctly constructed.
1089-1122: LGTM!The function is correctly parameterized to work with any chain, promoting code reuse for both filter table and external chains.
1124-1141: LGTM!The function is correctly parameterized and follows the same pattern as
insertForwardAcceptRules.
1143-1155: LGTM!The removal logic correctly mirrors the acceptance logic structure, maintaining good symmetry.
1157-1169: LGTM!The removal logic correctly follows the same fallback pattern as the acceptance path.
1194-1210: LGTM!The function correctly identifies and removes rules by UserData, with appropriate error handling.
1230-1252: LGTM!The function correctly discovers external chains from relevant table families, with appropriate error handling for listing failures.
1254-1273: LGTM!The filtering logic correctly identifies external chains by excluding NetBird's own table, iptables-managed tables, and chains that don't match the required criteria.
1283-1298: LGTM!Error messages are appropriately updated and consistent with the refactoring.
1275-1281: No action required —tableNameFilteris properly defined.The constant is declared in
client/firewall/nftables/manager_linux.go(line 27) as a package-level constant and is correctly accessible inrouter_linux.gosince both files are in the same package.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/firewall/nftables/router_linux.go (1)
1060-1092: Consider adding tests for external chain discovery logic.The external chain discovery and filtering logic (
findExternalChains,isExternalChain) is complex and critical for preventing traffic drops. This logic would benefit from unit tests covering:
- Discovery of chains in different table families (IPv4, INet)
- Filtering of NetBird's own workTable
- Filtering of iptables-managed tables
- Edge cases like nil Hooknum, non-filter chain types
Per the PR checklist, no tests were added. While the code logic appears sound, tests would provide confidence for future refactoring and help document expected behavior.
Also applies to: 1235-1286
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: JS / Lint
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
🔇 Additional comments (10)
client/firewall/nftables/router_linux.go (10)
30-34: LGTM! Table constants added for standard nftables tables.The additional table constants (mangle, raw, security) are correctly named and align with standard nftables table names used by iptables-nft and other tools.
963-975: LGTM! Well-structured error aggregation.The refactored method properly separates filter table rules from external chain rules and aggregates errors using multierror, ensuring both operations are attempted.
1034-1058: LGTM! Excellent refactoring for table-agnostic rule insertion.The signature change to accept a table parameter, combined with delegation to the extracted helper methods, enables rule insertion into any table while maintaining clean separation of concerns.
1060-1092: LGTM! Nil check properly addresses previous review concern.The nil check for
chain.Hooknumat lines 1071-1074 prevents the potential panic identified in the previous review. The function correctly handles Forward and Input hooks per the PR objectives.
1094-1127: LGTM! Clean extraction enables table-agnostic rule insertion.The extracted method properly parameterizes the chain and interface, allowing rules to be inserted into any chain while maintaining the correct logic for both input and output interface rules.
1129-1146: LGTM! Consistent extraction pattern.The extracted method follows the same pattern as
insertForwardAcceptRules, providing table-agnostic input rule insertion.
1148-1160: LGTM! Symmetric removal structure.The refactored removal method mirrors the insertion path, ensuring both filter table and external chains are cleaned up with proper error aggregation.
1176-1233: LGTM! Deterministic cleanup with resilient error handling.The removal methods implement deterministic cleanup by discovering chains at removal time, ensuring cleanup works even after restarts or crashes. The pattern of logging warnings for individual failures while continuing cleanup is appropriate for resilience.
1235-1286: Code review approved—all constants properly defined.The chain discovery and filtering logic is sound. The
tableNameFilterconstant referenced at line 1282 is correctly defined inmanager_linux.go(line 27) and accessible within the package. The family-specific filtering appropriately skips iptables-managed tables in IPv4 while processing native nftables in the INet family to avoid conflicts.
190-215: No action required.The
chainNameInputconstant is properly defined as a package-level constant inclient/firewall/nftables/manager_linux.go(line 28), making it accessible to the helper functions inrouter_linux.go. BothhookName()andfamilyName()functions are well-implemented with appropriate error handling and readable output formatting.
2 participants
Describe your changes
Currently, some tools (firewalld, miniupnpd, docker, ...) might create extra ntables tables with chains that have a drop policy. This would prevent NetBird's accept rules from working; hence we need to insert a passthrough rule in all input and forward chains that we find, not just the
ip filtertable fromiptables-nft.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:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.