Skip to content

[client] Fix uspfilter duplicate firewall rules#5269

Merged
lixmal merged 2 commits intomainfrom
fix-uspfilter-dupes
Feb 9, 2026
Merged

[client] Fix uspfilter duplicate firewall rules#5269
lixmal merged 2 commits intomainfrom
fix-uspfilter-dupes

Conversation

@lixmal
Copy link
Copy Markdown
Collaborator

@lixmal lixmal commented Feb 5, 2026

Describe your changes

When the ACL manager receives network map updates, it re-applies firewall rules.
In uspfilter, each call to addRouteFiltering previously generated a new UUID for the rule ID, meaning re-adding the same logical rule created a duplicate entry in the routeRules slice.
Similarly, blockInvalidRouted (called from EnableRouting) accumulated duplicate block rules on each invocation. Over time, this would cause unbounded growth of the route rules slice.

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

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

  • Bug Fixes

    • Prevented firewall rule duplication during repeated network updates.
    • Ensured deny rules are properly cleaned up when filters are removed.
  • Performance

    • Reduced overhead by caching route rules to avoid recreating identical rules.
    • Streamlined state reset to free resources reliably and prevent leaks.
  • Tests

    • Added comprehensive tests validating idempotent rule creation, proper cleanup, and stable lifecycle behavior.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 5, 2026

📝 Walkthrough

Walkthrough

Introduces route rule deduplication via a new routeRulesMap, centralizes teardown/cleanup into resetState(), removes inline per-file cleanup logic, and adds extensive unit tests covering route ACLs and peer rule lifecycle and cleanup.

Changes

Cohort / File(s) Summary
Cleanup centralization
client/firewall/uspfilter/allow_netbird.go, client/firewall/uspfilter/allow_netbird_windows.go
Replaced inline/manual cleanup logic in Close with a call to m.resetState(), removing direct map reinitialization and per-component shutdown code.
Route rule deduplication & state
client/firewall/uspfilter/filter.go
Added routeRulesMap map[nbid.RuleID]*RouteRule to Manager; generate and cache composite rule keys via nbid.GenerateRouteRuleKey in addRouteFiltering, deduplicate rules, update deleteRouteRule to consult and remove from cache, and added resetState() to clear rules and stop trackers/forwarder/logger (mutex-protected).
Route ACL tests
client/firewall/uspfilter/filter_routeacl_test.go
New tests validating idempotent route rule creation, distinct IDs for different destinations, non-destructive updates, single block-rule enforcement, and prevention of rule accumulation across repeated enable routing.
Peer rule lifecycle tests
client/firewall/uspfilter/filter_test.go, client/internal/acl/manager_test.go
Added tests covering deny/allow rule lifecycle, cleanup on repeated add/delete, mixed allow/deny handling for same IP, accumulation prevention, and rule removal behaviors across manager/ACL interactions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • #4015: Modifies the same firewall USP filter manager (client/firewall/uspfilter/filter.go) and touches Manager-internal state and rule management logic.

Suggested reviewers

  • pappz
  • crn4

Poem

🐰 I hopped through maps and route IDs bright,

Caching keys to keep rules tight.
A single reset clears every trace,
Tests hop in to guard the place.
🥕 Cheers from the rabbit for tidy state!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 68.42% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main bug fix: preventing duplicate firewall rules in uspfilter when rules are re-applied.
Description check ✅ Passed The description covers the core problem, explains the bug mechanism, and notes documentation is not needed. However, the issue ticket link is missing and no test information is provided despite the template asking about test creation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-uspfilter-dupes

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Feb 6, 2026

@lixmal lixmal merged commit 391221a into main Feb 9, 2026
38 checks passed
@lixmal lixmal deleted the fix-uspfilter-dupes branch February 9, 2026 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants