[PR #6863] [MERGED] [client] Reconcile routed allowed IPs when a lazy connection goes idle #27326

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

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/6863
Author: @riccardomanfrin
Created: 7/22/2026
Status: Merged
Merged: 7/23/2026
Merged by: @riccardomanfrin

Base: mainHead: fix_lazy_conn_allowed_ips


📝 Commits (2)

  • 521caaa Reconciling of client outbound routes on lazy connection
  • 059d693 Addresses coderabbit issue

📊 Changes

8 files changed (+241 additions, -3 deletions)

View changed files

📝 client/internal/conn_mgr.go (+11 -0)
📝 client/internal/engine.go (+6 -0)
📝 client/internal/lazyconn/manager/manager.go (+37 -3)
📝 client/internal/routemanager/manager.go (+25 -0)
📝 client/internal/routemanager/mock.go (+5 -0)
client/internal/routemanager/reconcile_test.go (+90 -0)
📝 client/internal/routemanager/refcounter/refcounter.go (+20 -0)
client/internal/routemanager/refcounter/refcounter_test.go (+47 -0)

📄 Description

Describe your changes

Under lazy connections, when a routing peer goes idle its WireGuard peer is torn down and re-created with a wake endpoint by
the activity listener, carrying only the overlay /32 (peerCfg.AllowedIPs). The routed subnet prefixes are dropped from
the device on the Connected→Idle transition.

They are meant to be restored by the route watcher, which reacts to the peer's status change and calls recalculateRoutes
AddAllowedIP. Two things prevent that from healing the peer:

  • AddAllowedIP uses update_only, which is a silent no-op (no error) when the peer does not exist. While the peer is
    being torn down and re-armed with its wake endpoint, it is briefly absent, so a re-add that lands in that window is lost.
  • The allowed-IP refcounter only calls its add function on a prefix's 0→1 transition. The routed prefix stays referenced
    across the idle cycle, so once the device entry is gone the refcounter does not re-push it on its own, and nothing retries.

As a result, traffic to the routed subnet is black-holed while the peer is idle. Because the wake endpoint only fires when a
packet matches the peer's AllowedIPs, a packet to the subnet is dropped before reaching the wake endpoint, so it cannot
wake the peer. The peer only recovers when woken by other means (e.g. a ping to its overlay IP).

Approach

This change keeps the existing Connected→Idle transition as-is and reconciles the AllowedIPs afterwards, avoiding any
additional locking on the transition path. The peer is torn down and re-armed with its wake endpoint as today; the routed
prefixes are then re-applied from the route manager's allowed-IP refcounter once the wake endpoint has been (re)armed.

A single add-only method, ReconcilePeerAllowedIPs(peerKey), re-applies every routed prefix currently tracked for the peer
in the refcounter (the authoritative store; it already covers static, dynamic and dnsinterceptor routes). It runs whenever
the peer's wake endpoint is (re)created in the lazy manager — every point where the activity listener builds it with the
overlay /32 only:

  • initial registration (AddPeer, cold start): the route manager may have already pushed the peer's routes before the
    wake endpoint existed, so those AddAllowedIP calls no-op'd; the reconcile installs them on the freshly created wake
    endpoint.
  • the two paths into idle (DeactivatePeer on a remote GOAWAY, onPeerInactivityTimedOut on local inactivity): the
    peer is torn down and re-armed, so the routed prefixes must be re-applied.

In every case the routed prefixes end up on the wake endpoint, so traffic to a routed subnet can wake the peer. Arming the
wake endpoint and reconciling are wrapped in a single armActivityListener helper so the two always happen together.

New helper: refcounter.Counter.KeysMatching(pred) to enumerate a peer's prefixes under the counter lock.

Note on scope: the reconcile restores what the refcounter tracks. All routed AllowedIPs currently go through it, so this
covers the routed-prefix case; it does not attempt to reconcile AllowedIPs installed outside the refcounter. The
Idle→Connected (wake) path does not need this: the peer is not removed there (the listener close leaves it in place and only
the endpoint is updated), so a concurrent AddAllowedIP lands normally.

Testing

Reproduced deterministically in a local dev setup (userspace client, NB_WG_KERNEL_DISABLED=true, B_LAZY_CONN_INACTIVITY_THRESHOLD=1 inactivity threshold 1
min). A temporary 30s sleep in the tear-down → re-arm window widens the race so the route watcher's async AddAllowedIP
reliably lands while the peer is absent and no-ops (the sleep is a test aid, not part of the change):

  • without the reconcile: after the peer goes idle, a ping to any routed IP — both a pre-existing route and one added
    during the window — black-holes; the peer never wakes.
  • with the reconcile: the same ping wakes the peer and passes.

Added unit tests: ReconcilePeerAllowedIPs (re-applies all of a peer's tracked prefixes, scoped to that peer) and
refcounter.Counter.KeysMatching.

Note: netbird status -d is not a reliable signal for this — AddPeerStateRoute records the route regardless of whether
the underlying AddAllowedIP no-op'd, so it reflects the route manager's intent rather than device state. The reliable
signal is functional (ping the subnet from idle).

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 (unit tests for the reconcile + KeysMatching)

Documentation

  • Documentation is not needed for this change (internal client behavior, no API / gRPC / CLI / flag change)

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

Summary by CodeRabbit

  • Bug Fixes
    • Routed IP assignments are automatically reconciled and restored whenever a peer’s lazy wake endpoint is armed or re-armed.
    • Routed allowed IPs are re-applied after inactivity transitions and monitoring re-initialization.
    • If reconciliation can’t be performed, the client safely skips it; if reconciliation encounters issues, failures are logged without stopping connection monitoring.

🔄 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/6863 **Author:** [@riccardomanfrin](https://github.com/riccardomanfrin) **Created:** 7/22/2026 **Status:** ✅ Merged **Merged:** 7/23/2026 **Merged by:** [@riccardomanfrin](https://github.com/riccardomanfrin) **Base:** `main` ← **Head:** `fix_lazy_conn_allowed_ips` --- ### 📝 Commits (2) - [`521caaa`](https://github.com/netbirdio/netbird/commit/521caaacae852b103c97431ac1b5558c214e1561) Reconciling of client outbound routes on lazy connection - [`059d693`](https://github.com/netbirdio/netbird/commit/059d6939f5717e8b6bb255def201cb469c12f0aa) Addresses coderabbit issue ### 📊 Changes **8 files changed** (+241 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `client/internal/conn_mgr.go` (+11 -0) 📝 `client/internal/engine.go` (+6 -0) 📝 `client/internal/lazyconn/manager/manager.go` (+37 -3) 📝 `client/internal/routemanager/manager.go` (+25 -0) 📝 `client/internal/routemanager/mock.go` (+5 -0) ➕ `client/internal/routemanager/reconcile_test.go` (+90 -0) 📝 `client/internal/routemanager/refcounter/refcounter.go` (+20 -0) ➕ `client/internal/routemanager/refcounter/refcounter_test.go` (+47 -0) </details> ### 📄 Description ## Describe your changes Under lazy connections, when a routing peer goes idle its WireGuard peer is torn down and re-created with a wake endpoint by the activity listener, carrying only the overlay /32 (`peerCfg.AllowedIPs`). The routed subnet prefixes are dropped from the device on the Connected→Idle transition. They are meant to be restored by the route watcher, which reacts to the peer's status change and calls `recalculateRoutes` → `AddAllowedIP`. Two things prevent that from healing the peer: - `AddAllowedIP` uses `update_only`, which is a silent no-op (no error) when the peer does not exist. While the peer is being torn down and re-armed with its wake endpoint, it is briefly absent, so a re-add that lands in that window is lost. - The allowed-IP refcounter only calls its add function on a prefix's 0→1 transition. The routed prefix stays referenced across the idle cycle, so once the device entry is gone the refcounter does not re-push it on its own, and nothing retries. As a result, traffic to the routed subnet is black-holed while the peer is idle. Because the wake endpoint only fires when a packet matches the peer's AllowedIPs, a packet to the subnet is dropped before reaching the wake endpoint, so it cannot wake the peer. The peer only recovers when woken by other means (e.g. a ping to its overlay IP). ## Approach This change keeps the existing Connected→Idle transition as-is and reconciles the AllowedIPs afterwards, avoiding any additional locking on the transition path. The peer is torn down and re-armed with its wake endpoint as today; the routed prefixes are then re-applied from the route manager's allowed-IP refcounter once the wake endpoint has been (re)armed. A single add-only method, `ReconcilePeerAllowedIPs(peerKey)`, re-applies every routed prefix currently tracked for the peer in the refcounter (the authoritative store; it already covers static, dynamic and dnsinterceptor routes). It runs whenever the peer's wake endpoint is (re)created in the lazy manager — every point where the activity listener builds it with the overlay /32 only: - **initial registration** (`AddPeer`, cold start): the route manager may have already pushed the peer's routes before the wake endpoint existed, so those `AddAllowedIP` calls no-op'd; the reconcile installs them on the freshly created wake endpoint. - **the two paths into idle** (`DeactivatePeer` on a remote GOAWAY, `onPeerInactivityTimedOut` on local inactivity): the peer is torn down and re-armed, so the routed prefixes must be re-applied. In every case the routed prefixes end up on the wake endpoint, so traffic to a routed subnet can wake the peer. Arming the wake endpoint and reconciling are wrapped in a single `armActivityListener` helper so the two always happen together. New helper: `refcounter.Counter.KeysMatching(pred)` to enumerate a peer's prefixes under the counter lock. Note on scope: the reconcile restores what the refcounter tracks. All routed AllowedIPs currently go through it, so this covers the routed-prefix case; it does not attempt to reconcile AllowedIPs installed outside the refcounter. The Idle→Connected (wake) path does not need this: the peer is not removed there (the listener close leaves it in place and only the endpoint is updated), so a concurrent `AddAllowedIP` lands normally. ## Testing Reproduced deterministically in a local dev setup (userspace client, `NB_WG_KERNEL_DISABLED=true`, `B_LAZY_CONN_INACTIVITY_THRESHOLD=1` inactivity threshold 1 min). A temporary 30s sleep in the tear-down → re-arm window widens the race so the route watcher's async `AddAllowedIP` reliably lands while the peer is absent and no-ops (the sleep is a test aid, not part of the change): - **without the reconcile:** after the peer goes idle, a ping to any routed IP — both a pre-existing route and one added during the window — black-holes; the peer never wakes. - **with the reconcile:** the same ping wakes the peer and passes. Added unit tests: `ReconcilePeerAllowedIPs` (re-applies all of a peer's tracked prefixes, scoped to that peer) and `refcounter.Counter.KeysMatching`. Note: `netbird status -d` is not a reliable signal for this — `AddPeerStateRoute` records the route regardless of whether the underlying `AddAllowedIP` no-op'd, so it reflects the route manager's intent rather than device state. The reliable signal is functional (ping the subnet from idle). ## Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (unit tests for the reconcile + `KeysMatching`) ## Documentation - [x] Documentation is **not needed** for this change (internal client behavior, no API / gRPC / CLI / flag change) <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/netbirdio/codesmith/netbird/pr/6863"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with Codesmith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1787330960&installation_model_id=427504&pr_number=6863&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6863&signature=f3d6a97d7db82e92b3939fdd0f159c5ee74913ff88f4eb82e41e88fcb787aff4"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with Codesmith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>/codesmith</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Routed IP assignments are automatically reconciled and restored whenever a peer’s lazy wake endpoint is armed or re-armed. * Routed allowed IPs are re-applied after inactivity transitions and monitoring re-initialization. * If reconciliation can’t be performed, the client safely skips it; if reconciliation encounters issues, failures are logged without stopping connection monitoring. <!-- 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:08:33 -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#27326