[client] Make raw table initialization non-fatal in firewall managers#5621
[client] Make raw table initialization non-fatal in firewall managers#5621
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMakes raw-table NOTRACK chain setup non-fatal during firewall Init for both iptables and nftables on Linux. Adds a Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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
🤖 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/iptables/manager_linux.go`:
- Around line 387-390: The cleanupNoTrackChain function's early return on
m.rawSupported prevents init-time recovery called from initNoTrackChain, causing
NewChain to fail if leftover chains exist; remove the "if !m.rawSupported {
return nil }" guard so cleanupNoTrackChain always attempts to remove chains (it
can rely on ChainExists to no-op when nothing to clean), and ensure rawSupported
is still set to false on the appropriate exit paths in initNoTrackChain (keep
setting m.rawSupported = false on failure branches) so state remains correct
after cleanup attempts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 791484ea-d860-495b-9bcc-9e977e226173
📒 Files selected for processing (2)
client/firewall/iptables/manager_linux.goclient/firewall/nftables/manager_linux.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/firewall/iptables/manager_linux.go (1)
387-393:⚠️ Potential issue | 🟠 MajorTreat missing raw table in
cleanupNoTrackChain()as non-fatal when raw support was not initialized.When raw table is unavailable on the system,
ChainExists()returns an error (not false). If initialization never enabled raw support (rawSupported=false), this cleanup error should not be fatal. Currently, the error blocks state deletion inClose(), even though the cleanup was never necessary.💡 Proposed fix
func (m *Manager) cleanupNoTrackChain() error { exists, err := m.ipv4Client.ChainExists(tableRaw, chainNameRaw) if err != nil { + // Raw table may be unavailable; skip cleanup if raw support was never enabled + if !m.rawSupported { + return nil + } return fmt.Errorf("check chain exists: %w", err) } if !exists { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/iptables/manager_linux.go` around lines 387 - 393, In cleanupNoTrackChain(), handle the case where the raw table is absent and raw support was never initialized by checking m.rawSupported (or the equivalent flag) before treating ChainExists errors as fatal: if m.rawSupported is false, ignore the ChainExists error and return nil; otherwise keep the current behavior (return fmt.Errorf(...)). Locate the call to m.ipv4Client.ChainExists(tableRaw, chainNameRaw) in cleanupNoTrackChain and gate the error handling on m.rawSupported (or, alternatively, detect the specific “table missing” error from ipv4Client and convert it to a non-fatal nil when raw support was not enabled).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@client/firewall/iptables/manager_linux.go`:
- Around line 387-393: In cleanupNoTrackChain(), handle the case where the raw
table is absent and raw support was never initialized by checking m.rawSupported
(or the equivalent flag) before treating ChainExists errors as fatal: if
m.rawSupported is false, ignore the ChainExists error and return nil; otherwise
keep the current behavior (return fmt.Errorf(...)). Locate the call to
m.ipv4Client.ChainExists(tableRaw, chainNameRaw) in cleanupNoTrackChain and gate
the error handling on m.rawSupported (or, alternatively, detect the specific
“table missing” error from ipv4Client and convert it to a non-fatal nil when raw
support was not enabled).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a94c4d5b-7ae8-4752-bbe6-8eefaa718050
📒 Files selected for processing (1)
client/firewall/iptables/manager_linux.go
|
2 participants
Describe your changes
Init()iptable_rawor raw priority support now log a warning instead of failing the entire firewall managerrawSupportedguard to iptables soSetupEBPFProxyNoTrackand cleanup are skipped when raw is unavailableThe raw table is only used for eBPF proxy notrack rules, not core firewall functionality. The engine already handles
SetupEBPFProxyNoTrackerrors as non-fatal warnings, so blocking init on it was unnecessarily strict.Issue ticket number and link
Fixes #5551
Checklist
Documentation
Select exactly one:
Summary by CodeRabbit
Bug Fixes
Behavior Change