[PR #5272] [MERGED] [client] Fix stale entries in nftables with no handle #25455

Closed
opened 2026-08-05 07:06:02 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/5272
Author: @lixmal
Created: 2/6/2026
Status: Merged
Merged: 2/12/2026
Merged by: @lixmal

Base: mainHead: fix-nftables-stale-removal


📝 Commits (1)

  • ca101f8 Fix stale entries in nftables with no handle

📊 Changes

2 files changed (+261 additions, -43 deletions)

View changed files

📝 client/firewall/nftables/router_linux.go (+126 -43)
📝 client/firewall/nftables/router_linux_test.go (+135 -0)

📄 Description

Describe your changes

Summary

  • Fix refreshRulesMap() to rebuild the rules map from scratch instead of merging, so stale entries (Handle == 0) from failed flushes are naturally dropped
  • Add Handle == 0 guards to all delete paths (DeleteRouteRule, removeNatRule, removeLegacyRouteRule, DeleteDNATRule, RemoveInboundDNAT) that clean up stale entries instead of returning errors
  • Implement rollbackRules() for AddNatRule flush failures to clean up unflushed entries and decrement set counters
  • Change RemoveNatRule from fail-fast to error accumulation for cleanup resilience

Problem

When conn.Flush() fails in AddNatRule, rules are stored in the map with Handle == 0 (the kernel never assigned handles). The old refreshRulesMap() merged kernel rules into the existing map, so these stale entries persisted
indefinitely. Any subsequent delete operation would fail on Handle == 0, which blocked further operations like re-adding the same NAT rule.

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

    • Improved firewall rule resilience: stale kernel entries are now detected and cleaned without causing errors, reducing failed operations.
    • Better cleanup and rollback for partially-applied NAT/route changes, preventing orphaned state and unnecessary flushes.
    • Enhanced logging for stale/missing rules and aggregated error reporting to reduce noisy failures.
  • Tests

    • Added tests covering stale-rule detection, cleanup, and NAT/route resilience.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/5272 **Author:** [@lixmal](https://github.com/lixmal) **Created:** 2/6/2026 **Status:** ✅ Merged **Merged:** 2/12/2026 **Merged by:** [@lixmal](https://github.com/lixmal) **Base:** `main` ← **Head:** `fix-nftables-stale-removal` --- ### 📝 Commits (1) - [`ca101f8`](https://github.com/netbirdio/netbird/commit/ca101f82cd692a82abd5258b9b34cb7da42d7768) Fix stale entries in nftables with no handle ### 📊 Changes **2 files changed** (+261 additions, -43 deletions) <details> <summary>View changed files</summary> 📝 `client/firewall/nftables/router_linux.go` (+126 -43) 📝 `client/firewall/nftables/router_linux_test.go` (+135 -0) </details> ### 📄 Description ## Describe your changes Summary - Fix refreshRulesMap() to rebuild the rules map from scratch instead of merging, so stale entries (Handle == 0) from failed flushes are naturally dropped - Add Handle == 0 guards to all delete paths (DeleteRouteRule, removeNatRule, removeLegacyRouteRule, DeleteDNATRule, RemoveInboundDNAT) that clean up stale entries instead of returning errors - Implement rollbackRules() for AddNatRule flush failures to clean up unflushed entries and decrement set counters - Change RemoveNatRule from fail-fast to error accumulation for cleanup resilience Problem When conn.Flush() fails in AddNatRule, rules are stored in the map with Handle == 0 (the kernel never assigned handles). The old refreshRulesMap() merged kernel rules into the existing map, so these stale entries persisted indefinitely. Any subsequent delete operation would fail on Handle == 0, which blocked further operations like re-adding the same NAT rule. ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [x] 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](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] 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/__ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved firewall rule resilience: stale kernel entries are now detected and cleaned without causing errors, reducing failed operations. * Better cleanup and rollback for partially-applied NAT/route changes, preventing orphaned state and unnecessary flushes. * Enhanced logging for stale/missing rules and aggregated error reporting to reduce noisy failures. * **Tests** * Added tests covering stale-rule detection, cleanup, and NAT/route resilience. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 07:06:02 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#25455