[client] Make mss clamping optional for nftables#4843
Conversation
WalkthroughRefactored nftables router initialization in Changes
Sequence DiagramsequenceDiagram
participant Init as createContainers
participant Helpers as rule helpers
participant Conn as conn (Flush)
rect rgb(245,250,255)
Note over Init,Helpers: Centralized initialization and single Flush
Init->>Helpers: insertReturnTrafficRule (relocated)
Init->>Helpers: addPostroutingRules() -- now void
Init->>Helpers: addMSSClampingRules() -- now void (may Flush internally)
Init->>Helpers: acceptFilterRulesNftables() -- now void (may Flush internally)
Init->>Conn: conn.Flush() -- single consolidated flush
Conn-->>Init: flush result (error propagated)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
lixmal
force-pushed
the
mss-clamping-optional
branch
from
05c5323 to
677e75f
Compare
![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
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)
767-839: Guard MSS calculation against invalid/“disabled” MTU values
r.mtuis auint16, somss := r.mtu - ipTCPHeaderMinSizewill silently underflow ifr.mtu <= ipTCPHeaderMinSize(including the common “disabled” sentinel ofmtu == 0). That yields a very large MSS value, which effectively disables clamping but in a non-obvious way and could be surprising ifmtuis misconfigured.To make MSS clamping explicitly optional and safer, you could:
- Treat
mtu == 0(or generallymtu <= ipTCPHeaderMinSize) as “MSS clamping disabled” and skip adding rules.- Only compute
mssonce you’ve validated the MTU range.For example:
func (r *router) addMSSClampingRules() error { - mss := r.mtu - ipTCPHeaderMinSize + if r.mtu == 0 { + // MSS clamping explicitly disabled for nftables + return nil + } + if r.mtu <= ipTCPHeaderMinSize { + log.Warnf("mtu %d too small for MSS clamping, skipping", r.mtu) + return nil + } + mss := r.mtu - ipTCPHeaderMinSize … - return r.conn.Flush() + return r.conn.Flush() }This makes the “optional MSS clamping” behavior explicit and avoids underflowed MSS values.
🧹 Nitpick comments (3)
client/firewall/nftables/router_linux.go (3)
237-255: Clarify which initialization failures should be treated as fatal
createContainersonly returns an error if theFlushat Line 241 fails; errors fromaddMSSClampingRules,acceptForwardRules, andrefreshRulesMapare just logged and ignored. Giveninit()treats anycreateContainerserror as a hard failure, this effectively makes MSS clamping and accept rules best-effort/optional and allows initialization to complete even ifrefreshRulesMapcannot populater.rules.If that matches your intent (especially for “MSS clamping optional”), this is fine; otherwise consider bubbling up some of these errors or tightening the conditions under which
createContainersreturnsnil. While here, you might also want to wrap theFlusherror with%winstead of%vto preserve the underlying cause:if err := r.conn.Flush(); err != nil { return fmt.Errorf("initialize tables: %w", err) }
695-762: Helper return type change for addPostroutingRules looks fineSwitching
addPostroutingRulesto a void helper that only enqueues rules and relies on the caller’sFlushis consistent with hownftables.Connis typically used, and avoids per-helper flushes. The trailingreturnat Line 762 is redundant in Go and can be dropped to keep the function a bit cleaner:-func (r *router) addPostroutingRules() { +func (r *router) addPostroutingRules() { … - r.conn.AddRule(&nftables.Rule{ + r.conn.AddRule(&nftables.Rule{ … }) - - return }
996-1069: Direct Flush in acceptFilterRulesNftables is OK, consider consistent wrappingHaving
acceptFilterRulesNftablesreturnr.conn.Flush()directly is functionally fine, andacceptForwardRulesalready provides context in its log message when this fails. If you want consistent error formatting with otherFlushcallers in this file, you could optionally wrap it:-func (r *router) acceptFilterRulesNftables() error { +func (r *router) acceptFilterRulesNftables() error { … - return r.conn.Flush() + if err := r.conn.Flush(); err != nil { + return fmt.Errorf(flushError, err) + } + return nil }Not required, but would align this helper with other nftables operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go(7 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). (17)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- 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: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
🔇 Additional comments (1)
client/firewall/nftables/router_linux.go (1)
1193-1205: Improved error wrapping in refreshRulesMap looks goodUsing
%winreturn fmt.Errorf("list rules: %w", err)together with the sharedrefreshRulesMapErrorformat string at call sites gives you both consistent log messages and proper error chaining for callers that want to inspect the root cause. No functional issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/firewall/nftables/router_linux.go (2)
233-253: createContainers now treats MSS clamping, accept rules, and rules‑map refresh as best‑effortThe new flow:
- Creates all chains and postrouting rules (
insertReturnTrafficRule,addPostroutingRules) and then does a singleFlush(). This isolates critical setup and ensures either all core chains/rules are committed or none are.- Calls
addMSSClampingRules()andacceptForwardRules()and only logs on error, so both features become non‑fatal/optional.- Also logs (rather than propagates)
refreshRulesMap()failures.This matches the PR goal of keeping optional features (MSS clamping and accept‑rules plumbing) from breaking core routing, with
addMSSClampingRules/acceptFilterRulesNftablesdoing their own dedicatedFlush. One minor nit: for consistency with the rest of the file’s wrapped errors, you could switchreturn fmt.Errorf("initialize tables: %v", err)to use
%w.- if err := r.conn.Flush(); err != nil { - return fmt.Errorf("initialize tables: %v", err) - } + if err := r.conn.Flush(); err != nil { + return fmt.Errorf("initialize tables: %w", err) + }Also, note the behavioural change:
init()will now succeed even ifacceptForwardRulesorrefreshRulesMapfail. That’s likely acceptable given they are best‑effort, but worth double‑checking against expectations for environments with restrictive host firewalls or pre‑existing nftables state.
691-759: Remove redundantreturnstatement from void functionAll call sites of
addPostroutingRules()in the nftables router are correctly updated—the single invocation at line 235 properly calls it without error handling. The void signature change is clean.Remove the redundant
returnat the end of the function (around line 759):func (r *router) addPostroutingRules() { r.conn.AddRule(&nftables.Rule{ Table: r.workTable, Chain: r.chains[chainNameRoutingNat], Exprs: exprs2, }) - - return }
📜 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). (21)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: JS / Lint
🔇 Additional comments (5)
client/firewall/nftables/router_linux.go (5)
91-95: Filter-table load errors are now fully non‑fatal – confirm this change in failure semantics is desired
newRouternow logs and continues on anyloadFilterTableerror, leavingfilterTablenil and effectively disabling accept‑rules setup (later guarded byr.filterTable == nil). This aligns with treating filter‑table manipulation as optional, but it also means:
- nftables router construction will no longer fail on unexpected errors from
ListTablesOfFamily(e.g. permission issues, corrupted state), so higher‑level fallback logic that relied onnewRouterfailing may no longer trigger.- Existing filter rules from previous runs might remain in place when we can’t read/modify the table.
If that change in behaviour is intentional, the code is fine as‑is. Otherwise, consider distinguishing
errFilterTableNotFound(log+skip) from other errors (still fatal), and/or logging non‑“not found” errors at a higher level.
171-175: Improved error wrapping for ListTablesOfFamilySwitching to
fmt.Errorf("list tables: %w", err)gives better error chaining and consistent formatting with other callers. No issues here.
992-1065: acceptFilterRulesNftables now owns its Flush, aligning with optional best‑effort setupAdding
return r.conn.Flush()at the end ofacceptFilterRulesNftablesmeans:
- nftables‑based accept rules are committed in their own transaction, separate from the critical routing setup flush in
createContainers.- Any nftables error now cleanly surfaces as an
errorfromacceptForwardRules, whichcreateContainerslogs and ignores, keeping these rules best‑effort.This is consistent with the new initialization strategy and doesn’t appear to introduce new failure modes, given that iptables‑based setup was already immediate and independent.
1189-1193: refreshRulesMap error wrapping improvedChanging the error from
"unable to list rules: %v"to"list rules: %w"(viafmt.Errorf) makes the error message cleaner and preserves the original error value for callers. No behavioural issues with this change.
763-836: Verification confirms the change is safe—addMSSClampingRulesis only called after a precedingFlush().The function is invoked once in
createContainers()at line 241, afterr.conn.Flush()has already been called at line 237. The internalFlush()inaddMSSClampingRulestherefore does not disrupt any batched operations. The pattern matches the iptables implementation, where the error is logged but does not abort initialization. No other code path callsaddMSSClampingRules, confirming the new internal flush poses no risk to existing batching contexts.
|
2 participants
Describe your changes
This avoids polluting the critical setup with optional rules that can fail.
Issue ticket number and link
netbirdio/addon-netbird#311
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
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.