mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-30 03:21:29 +02:00
## 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 -->
126 lines
4.2 KiB
Go
126 lines
4.2 KiB
Go
package routemanager
|
|
|
|
import (
|
|
"context"
|
|
|
|
firewall "github.com/netbirdio/netbird/client/firewall/manager"
|
|
"github.com/netbirdio/netbird/client/iface"
|
|
"github.com/netbirdio/netbird/client/internal/listener"
|
|
"github.com/netbirdio/netbird/client/internal/routeselector"
|
|
"github.com/netbirdio/netbird/client/internal/statemanager"
|
|
"github.com/netbirdio/netbird/route"
|
|
)
|
|
|
|
// MockManager is the mock instance of a route manager
|
|
type MockManager struct {
|
|
ClassifyRoutesFunc func(routes []*route.Route) (map[route.ID]*route.Route, route.HAMap)
|
|
UpdateRoutesFunc func(updateSerial uint64, serverRoutes map[route.ID]*route.Route, clientRoutes route.HAMap, useNewDNSRoute bool) error
|
|
TriggerSelectionFunc func(haMap route.HAMap)
|
|
GetRouteSelectorFunc func() *routeselector.RouteSelector
|
|
GetClientRoutesFunc func() route.HAMap
|
|
GetSelectedClientRoutesFunc func() route.HAMap
|
|
GetActiveClientRoutesFunc func() route.HAMap
|
|
GetClientRoutesWithNetIDFunc func() map[route.NetID][]*route.Route
|
|
StopFunc func(manager *statemanager.Manager)
|
|
}
|
|
|
|
func (m *MockManager) Init() error {
|
|
return nil
|
|
}
|
|
|
|
// InitialRouteRange mock implementation of InitialRouteRange from Manager interface
|
|
func (m *MockManager) InitialRouteRange() []string {
|
|
return nil
|
|
}
|
|
|
|
// UpdateRoutes mock implementation of UpdateRoutes from Manager interface
|
|
func (m *MockManager) UpdateRoutes(updateSerial uint64, newRoutes map[route.ID]*route.Route, clientRoutes route.HAMap, useNewDNSRoute bool) error {
|
|
if m.UpdateRoutesFunc != nil {
|
|
return m.UpdateRoutesFunc(updateSerial, newRoutes, clientRoutes, useNewDNSRoute)
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// ClassifyRoutes mock implementation of ClassifyRoutes from Manager interface
|
|
func (m *MockManager) ClassifyRoutes(routes []*route.Route) (map[route.ID]*route.Route, route.HAMap) {
|
|
if m.ClassifyRoutesFunc != nil {
|
|
return m.ClassifyRoutesFunc(routes)
|
|
}
|
|
return nil, nil
|
|
}
|
|
|
|
func (m *MockManager) TriggerSelection(networks route.HAMap) {
|
|
if m.TriggerSelectionFunc != nil {
|
|
m.TriggerSelectionFunc(networks)
|
|
}
|
|
}
|
|
|
|
// GetRouteSelector mock implementation of GetRouteSelector from Manager interface
|
|
func (m *MockManager) GetRouteSelector() *routeselector.RouteSelector {
|
|
if m.GetRouteSelectorFunc != nil {
|
|
return m.GetRouteSelectorFunc()
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// GetClientRoutes mock implementation of GetClientRoutes from the Manager interface
|
|
func (m *MockManager) GetClientRoutes() route.HAMap {
|
|
if m.GetClientRoutesFunc != nil {
|
|
return m.GetClientRoutesFunc()
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// GetSelectedClientRoutes mock implementation of GetSelectedClientRoutes from the Manager interface
|
|
func (m *MockManager) GetSelectedClientRoutes() route.HAMap {
|
|
if m.GetSelectedClientRoutesFunc != nil {
|
|
return m.GetSelectedClientRoutesFunc()
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// GetActiveClientRoutes mock implementation of GetActiveClientRoutes from the Manager interface
|
|
func (m *MockManager) GetActiveClientRoutes() route.HAMap {
|
|
if m.GetActiveClientRoutesFunc != nil {
|
|
return m.GetActiveClientRoutesFunc()
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// GetClientRoutesWithNetID mock implementation of GetClientRoutesWithNetID from Manager interface
|
|
func (m *MockManager) GetClientRoutesWithNetID() map[route.NetID][]*route.Route {
|
|
if m.GetClientRoutesWithNetIDFunc != nil {
|
|
return m.GetClientRoutesWithNetIDFunc()
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// Start mock implementation of Start from Manager interface
|
|
func (m *MockManager) Start(ctx context.Context, iface *iface.WGIface) {
|
|
}
|
|
|
|
// SetRouteChangeListener mock implementation of SetRouteChangeListener from Manager interface
|
|
func (m *MockManager) SetRouteChangeListener(listener listener.NetworkChangeListener) {
|
|
|
|
}
|
|
|
|
func (m *MockManager) SetFirewall(firewall.Manager) error {
|
|
panic("implement me")
|
|
}
|
|
|
|
// SetDNSForwarderPort mock implementation of SetDNSForwarderPort from Manager interface
|
|
func (m *MockManager) SetDNSForwarderPort(port uint16) {
|
|
}
|
|
|
|
// ReconcilePeerAllowedIPs mock implementation of ReconcilePeerAllowedIPs from Manager interface
|
|
func (m *MockManager) ReconcilePeerAllowedIPs(peerKey string) error {
|
|
return nil
|
|
}
|
|
|
|
// Stop mock implementation of Stop from Manager interface
|
|
func (m *MockManager) Stop(stateManager *statemanager.Manager) {
|
|
if m.StopFunc != nil {
|
|
m.StopFunc(stateManager)
|
|
}
|
|
}
|