diff --git a/client/internal/routemanager/manager.go b/client/internal/routemanager/manager.go index a9a1f5eed..fd499adc5 100644 --- a/client/internal/routemanager/manager.go +++ b/client/internal/routemanager/manager.go @@ -9,6 +9,7 @@ import ( "net/url" "runtime" "slices" + "strings" "sync" "sync/atomic" "time" @@ -700,6 +701,8 @@ func resolveURLsToIPs(urls []string) []net.IP { // updateRouteSelectorFromManagement updates the route selector based on the isSelected status from the management server func (m *DefaultManager) updateRouteSelectorFromManagement(clientRoutes route.HAMap) { + m.mirrorV6ExitPairSelections(clientRoutes) + exitNodeInfo := m.collectExitNodeInfo(clientRoutes) if len(exitNodeInfo.allIDs) == 0 { return @@ -709,6 +712,24 @@ func (m *DefaultManager) updateRouteSelectorFromManagement(clientRoutes route.HA m.logExitNodeUpdate(exitNodeInfo) } +// mirrorV6ExitPairSelections keeps every synthesized "-v6" exit route's selection +// consistent with its v4 base. The v4/v6 exit pair is a single toggle, so the v6 +// entry always follows the base: deselecting the v4 exit node also drops its ::/0 +// pair, and any stale (orphaned) explicit selection on the v6 entry is reset. This +// runs before selection is read so both collectExitNodeInfo and FilterSelectedExitNodes +// see consistent state, including pairs loaded from persisted selector state. +func (m *DefaultManager) mirrorV6ExitPairSelections(clientRoutes route.HAMap) { + routesByNetID := make(map[route.NetID][]*route.Route, len(clientRoutes)) + for haID, routes := range clientRoutes { + routesByNetID[haID.NetID()] = routes + } + + for v6ID := range route.V6ExitMergeSet(routesByNetID) { + baseID := route.NetID(strings.TrimSuffix(string(v6ID), route.V6ExitSuffix)) + m.routeSelector.SyncPairedSelection(baseID, v6ID) + } +} + type exitNodeInfo struct { allIDs []route.NetID selectedByManagement []route.NetID @@ -745,10 +766,7 @@ func (m *DefaultManager) isExitNodeRoute(routes []*route.Route) bool { } func (m *DefaultManager) categorizeUserSelection(netID route.NetID, info *exitNodeInfo) { - // Use the exit-node-aware check so a synthesized "-v6" route inherits the v4 - // base's selection: deselecting the v4 exit node must also drop its ::/0 pair, - // otherwise the v6 default route leaks into the tunnel despite the deselect. - if m.routeSelector.IsSelectedForExitNode(netID) { + if m.routeSelector.IsSelected(netID) { info.userSelected = append(info.userSelected, netID) } else { info.userDeselected = append(info.userDeselected, netID) diff --git a/client/internal/routemanager/manager_v6exit_test.go b/client/internal/routemanager/manager_v6exit_test.go new file mode 100644 index 000000000..15ab99cbd --- /dev/null +++ b/client/internal/routemanager/manager_v6exit_test.go @@ -0,0 +1,47 @@ +package routemanager + +import ( + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/internal/routeselector" + "github.com/netbirdio/netbird/route" +) + +// TestUpdateRouteSelectorFromManagement_MirrorsV6ExitPair reproduces the bug seen +// in netbird-engine.log: persisted selector state has the v4 exit node deselected +// but its synthesized "-v6" pair explicitly selected (orphaned), so the ::/0 route +// leaked onto the tunnel. The management update must mirror the v4 deselect onto the +// v6 pair so FilterSelectedExitNodes drops it. +func TestUpdateRouteSelectorFromManagement_MirrorsV6ExitPair(t *testing.T) { + const ( + v4ID = route.NetID("Exit Node (raspberrypi)") + v6ID = route.NetID("Exit Node (raspberrypi)-v6") + ) + all := []route.NetID{v4ID, v6ID} + + rs := routeselector.NewRouteSelector() + // Orphan the v6 selection: select the pair, then deselect only the v4 base. + require.NoError(t, rs.SelectRoutes([]route.NetID{v4ID, v6ID}, true, all)) + require.NoError(t, rs.DeselectRoutes([]route.NetID{v4ID}, all)) + require.True(t, rs.IsSelected(v6ID), "precondition: orphaned v6 selection survives v4 deselect") + + m := &DefaultManager{routeSelector: rs} + + v4Route := &route.Route{NetID: v4ID, Network: netip.MustParsePrefix("0.0.0.0/0")} + v6Route := &route.Route{NetID: v6ID, Network: netip.MustParsePrefix("::/0")} + clientRoutes := route.HAMap{ + "Exit Node (raspberrypi)|0.0.0.0/0": {v4Route}, + "Exit Node (raspberrypi)-v6|::/0": {v6Route}, + } + + m.updateRouteSelectorFromManagement(clientRoutes) + + assert.False(t, rs.IsSelected(v6ID), "v6 pair must follow the v4 base deselect after the management update") + + filtered := rs.FilterSelectedExitNodes(clientRoutes) + assert.Empty(t, filtered, "deselected v4 exit node must not leak its ::/0 pair onto the tunnel") +} diff --git a/client/internal/routeselector/routeselector.go b/client/internal/routeselector/routeselector.go index 7f211304b..443bb86ae 100644 --- a/client/internal/routeselector/routeselector.go +++ b/client/internal/routeselector/routeselector.go @@ -4,7 +4,6 @@ import ( "encoding/json" "fmt" "slices" - "strings" "sync" "github.com/hashicorp/go-multierror" @@ -55,11 +54,6 @@ func (rs *RouteSelector) SelectRoutes(routes []route.NetID, appendRoute bool, al } delete(rs.deselectedRoutes, route) rs.selectedRoutes[route] = struct{}{} - // Keep the v4/v6 exit pair consistent: clear any orphaned explicit state on - // the "-v6" sibling so it inherits the v4 base via effectiveNetID. Skip when - // the pair is itself part of this batch (callers expand it deliberately when - // it should diverge). - rs.clearPairedV6Locked(route, routes) } rs.deselectAll = false @@ -100,10 +94,6 @@ func (rs *RouteSelector) DeselectRoutes(routes []route.NetID, allRoutes []route. } rs.deselectedRoutes[route] = struct{}{} delete(rs.selectedRoutes, route) - // Keep the v4/v6 exit pair consistent: clear any orphaned explicit selection - // on the "-v6" sibling so it falls back to inheriting the v4 base's state - // (via effectiveNetID) instead of staying stuck as explicitly selected. - rs.clearPairedV6Locked(route, routes) } return errors.FormatErrorOrNil(err) @@ -133,15 +123,31 @@ func (rs *RouteSelector) IsSelected(routeID route.NetID) bool { return rs.isSelectedLocked(routeID) } -// IsSelectedForExitNode checks if an exit-node route is selected, mirroring the -// v4/v6 pair: a synthesized "-v6" entry with no explicit state of its own inherits -// its v4 base's selection, so a deselect on the v4 base also deselects the v6 entry. -// Only call this from exit-node code paths (see effectiveNetID). -func (rs *RouteSelector) IsSelectedForExitNode(routeID route.NetID) bool { - rs.mu.RLock() - defer rs.mu.RUnlock() +// SyncPairedSelection forces pairedID's explicit selection state to match baseID's, +// so a synthesized "-v6" exit route always follows its v4 base: selecting or +// deselecting the v4 exit node governs the ::/0 pair, and any stale (orphaned) +// explicit state on the v6 entry is reset. The v4/v6 exit pair is treated as a single +// toggle, so the v6 entry carries no independent selection of its own. +func (rs *RouteSelector) SyncPairedSelection(baseID, pairedID route.NetID) { + rs.mu.Lock() + defer rs.mu.Unlock() - return rs.isSelectedLocked(rs.effectiveNetID(routeID)) + if rs.deselectAll { + return + } + + _, baseSelected := rs.selectedRoutes[baseID] + _, baseDeselected := rs.deselectedRoutes[baseID] + + delete(rs.selectedRoutes, pairedID) + delete(rs.deselectedRoutes, pairedID) + + switch { + case baseSelected: + rs.selectedRoutes[pairedID] = struct{}{} + case baseDeselected: + rs.deselectedRoutes[pairedID] = struct{}{} + } } // FilterSelected removes unselected routes from the provided map. @@ -163,14 +169,13 @@ func (rs *RouteSelector) FilterSelected(routes route.HAMap) route.HAMap { } // HasUserSelectionForRoute returns true if the user has explicitly selected or deselected this route. -// Intended for exit-node code paths: a v6 exit-node pair (e.g. "MyExit-v6") with no explicit state of -// its own inherits its v4 base's state, so legacy persisted selections that predate v6 pairing -// transparently apply to the synthesized v6 entry. +// The lookup is literal; v4/v6 exit pairs are kept consistent at write time via SyncPairedSelection, +// so a synthesized "-v6" entry carries the same explicit state as its v4 base. func (rs *RouteSelector) HasUserSelectionForRoute(routeID route.NetID) bool { rs.mu.RLock() defer rs.mu.RUnlock() - return rs.hasUserSelectionForRouteLocked(rs.effectiveNetID(routeID)) + return rs.hasUserSelectionForRouteLocked(routeID) } func (rs *RouteSelector) FilterSelectedExitNodes(routes route.HAMap) route.HAMap { @@ -253,24 +258,6 @@ func (rs *RouteSelector) UnmarshalJSON(data []byte) error { return nil } -// effectiveNetID returns the v4 base for a "-v6" exit pair entry that has no explicit -// state of its own, so selections made on the v4 entry govern the v6 entry automatically. -// Only call this from exit-node-specific code paths: applying it to a non-exit "-v6" route -// would make it inherit unrelated v4 state. Must be called with rs.mu held. -func (rs *RouteSelector) effectiveNetID(id route.NetID) route.NetID { - name := string(id) - if !strings.HasSuffix(name, route.V6ExitSuffix) { - return id - } - if _, ok := rs.selectedRoutes[id]; ok { - return id - } - if _, ok := rs.deselectedRoutes[id]; ok { - return id - } - return route.NetID(strings.TrimSuffix(name, route.V6ExitSuffix)) -} - func (rs *RouteSelector) isSelectedLocked(routeID route.NetID) bool { if rs.deselectAll { return false @@ -293,34 +280,14 @@ func (rs *RouteSelector) hasUserSelectionForRouteLocked(routeID route.NetID) boo return selected || deselected } -// clearPairedV6Locked removes any explicit selected/deselected state on the "-v6" -// sibling of a v4 exit-node NetID, so the synthesized v6 entry resolves through -// effectiveNetID to its v4 base. No-op for IDs that already carry the "-v6" suffix, -// or when the sibling is itself part of the current batch (the caller is setting it -// deliberately, e.g. via ExpandV6ExitPairs). Must be called with rs.mu held. -func (rs *RouteSelector) clearPairedV6Locked(id route.NetID, batch []route.NetID) { - if strings.HasSuffix(string(id), route.V6ExitSuffix) { - return - } - v6ID := route.NetID(string(id) + route.V6ExitSuffix) - if slices.Contains(batch, v6ID) { - return - } - delete(rs.selectedRoutes, v6ID) - delete(rs.deselectedRoutes, v6ID) -} - func (rs *RouteSelector) applyExitNodeFilter( id route.HAUniqueID, netID route.NetID, rt []*route.Route, out route.HAMap, ) { - // Exit-node path: apply the v4/v6 pair mirror so a deselect on the v4 base also - // drops the synthesized v6 entry that lacks its own explicit state. - effective := rs.effectiveNetID(netID) - if rs.hasUserSelectionForRouteLocked(effective) { - if rs.isSelectedLocked(effective) { + if rs.hasUserSelectionForRouteLocked(netID) { + if rs.isSelectedLocked(netID) { out[id] = rt } return diff --git a/client/internal/routeselector/routeselector_test.go b/client/internal/routeselector/routeselector_test.go index 190b816b3..c9d6acb4d 100644 --- a/client/internal/routeselector/routeselector_test.go +++ b/client/internal/routeselector/routeselector_test.go @@ -330,63 +330,73 @@ func TestRouteSelector_FilterSelectedExitNodes(t *testing.T) { assert.Len(t, filtered, 0) // No routes should be selected } -// TestRouteSelector_V6ExitPairInherits covers the v4/v6 exit-node pair selection -// mirror. The mirror is scoped to exit-node code paths: HasUserSelectionForRoute -// and FilterSelectedExitNodes resolve a "-v6" entry without explicit state to its -// v4 base, so legacy persisted selections that predate v6 pairing transparently -// apply to the synthesized v6 entry. General lookups (IsSelected, FilterSelected) -// stay literal so unrelated routes named "*-v6" don't inherit unrelated state. -func TestRouteSelector_V6ExitPairInherits(t *testing.T) { +// TestRouteSelector_V6ExitPairSync covers SyncPairedSelection, which keeps a v4 +// exit node and its synthesized "-v6" counterpart consistent. The selector itself +// is literal and never infers a v6 entry's state from its v4 base; callers that know +// the pairing (exit-node code paths) call SyncPairedSelection to force the v6 entry +// to follow the base, treating the pair as a single toggle. +func TestRouteSelector_V6ExitPairSync(t *testing.T) { all := []route.NetID{"exit1", "exit1-v6", "exit2", "exit2-v6", "corp", "corp-v6"} - t.Run("HasUserSelectionForRoute mirrors deselected v4 base", func(t *testing.T) { + t.Run("selector lookups stay literal without sync", func(t *testing.T) { rs := routeselector.NewRouteSelector() require.NoError(t, rs.DeselectRoutes([]route.NetID{"exit1"}, all)) - assert.True(t, rs.HasUserSelectionForRoute("exit1-v6"), "v6 pair sees v4 base's user selection") + // The selector does not pair-resolve: the v6 entry is independent until synced. + assert.False(t, rs.HasUserSelectionForRoute("exit1-v6"), "v6 entry has no state of its own") + assert.True(t, rs.IsSelected("exit1-v6"), "unsynced v6 entry stays selected by default") - // unrelated v6 with no v4 base touched is unaffected - assert.False(t, rs.HasUserSelectionForRoute("exit2-v6")) + // A route literally named "exit1-something" must never pair-resolve either. + assert.False(t, rs.HasUserSelectionForRoute("exit1-something")) }) - t.Run("IsSelected stays literal for non-exit lookups", func(t *testing.T) { - rs := routeselector.NewRouteSelector() - require.NoError(t, rs.DeselectRoutes([]route.NetID{"corp"}, all)) - - // A non-exit route literally named "corp-v6" must not inherit "corp"'s state - // via the mirror; the mirror only applies in exit-node code paths. - assert.False(t, rs.IsSelected("corp")) - assert.True(t, rs.IsSelected("corp-v6"), "non-exit *-v6 routes must not inherit unrelated v4 state") - }) - - t.Run("IsSelectedForExitNode mirrors deselected v4 base", func(t *testing.T) { + t.Run("sync mirrors deselected v4 base onto v6", func(t *testing.T) { rs := routeselector.NewRouteSelector() require.NoError(t, rs.DeselectRoutes([]route.NetID{"exit1"}, all)) - // Regression: deselecting the v4 exit node must also report the synthesized - // "-v6" pair as not selected, otherwise the ::/0 route leaks into the tunnel. - assert.False(t, rs.IsSelectedForExitNode("exit1")) - assert.False(t, rs.IsSelectedForExitNode("exit1-v6"), "v6 pair inherits v4 base deselect") + rs.SyncPairedSelection("exit1", "exit1-v6") - // An exit node with no user selection at all stays selected by default. - assert.True(t, rs.IsSelectedForExitNode("exit2")) - assert.True(t, rs.IsSelectedForExitNode("exit2-v6")) + assert.False(t, rs.IsSelected("exit1")) + assert.False(t, rs.IsSelected("exit1-v6"), "v6 pair follows v4 base deselect") + assert.True(t, rs.HasUserSelectionForRoute("exit1-v6"), "v6 carries explicit deselect after sync") }) - t.Run("IsSelectedForExitNode respects explicit v6 state", func(t *testing.T) { + t.Run("sync mirrors selected v4 base onto v6", func(t *testing.T) { + rs := routeselector.NewRouteSelector() + require.NoError(t, rs.SelectRoutes([]route.NetID{"exit1"}, false, all)) + + rs.SyncPairedSelection("exit1", "exit1-v6") + + assert.True(t, rs.IsSelected("exit1")) + assert.True(t, rs.IsSelected("exit1-v6"), "v6 pair follows v4 base select") + }) + + t.Run("sync clears v6 state when base has no explicit selection", func(t *testing.T) { rs := routeselector.NewRouteSelector() - require.NoError(t, rs.DeselectRoutes([]route.NetID{"exit1"}, all)) require.NoError(t, rs.SelectRoutes([]route.NetID{"exit1-v6"}, true, all)) + require.True(t, rs.HasUserSelectionForRoute("exit1-v6")) - // Explicit selection on the v6 entry overrides the v4 base's deselect. - assert.False(t, rs.IsSelectedForExitNode("exit1")) - assert.True(t, rs.IsSelectedForExitNode("exit1-v6"), "explicit v6 select wins over v4 base") + rs.SyncPairedSelection("exit1", "exit1-v6") + + assert.False(t, rs.HasUserSelectionForRoute("exit1-v6"), + "v6 explicit state is cleared so it follows management like its base") }) - t.Run("explicit v6 state overrides v4 base in filter", func(t *testing.T) { + // Regression for the observed bug (see netbird-engine.log): persisted state has + // the v4 base deselected but the v6 sibling explicitly selected (orphaned). The + // sync must reset the orphan so the ::/0 route does not leak onto the tunnel. + t.Run("sync clears orphaned explicit v6 selection on deselected base", func(t *testing.T) { rs := routeselector.NewRouteSelector() + + // Prior state: both explicitly selected, then only the v4 base deselected, + // leaving the v6 entry as a stale explicit selection. + require.NoError(t, rs.SelectRoutes([]route.NetID{"exit1", "exit1-v6"}, true, all)) require.NoError(t, rs.DeselectRoutes([]route.NetID{"exit1"}, all)) - require.NoError(t, rs.SelectRoutes([]route.NetID{"exit1-v6"}, true, all)) + require.True(t, rs.IsSelected("exit1-v6"), "precondition: orphaned v6 selection") + + rs.SyncPairedSelection("exit1", "exit1-v6") + + assert.False(t, rs.IsSelected("exit1-v6"), "orphaned v6 selection reset to follow v4 deselect") v4Route := &route.Route{NetID: "exit1", Network: netip.MustParsePrefix("0.0.0.0/0")} v6Route := &route.Route{NetID: "exit1-v6", Network: netip.MustParsePrefix("::/0")} @@ -394,23 +404,14 @@ func TestRouteSelector_V6ExitPairInherits(t *testing.T) { "exit1|0.0.0.0/0": {v4Route}, "exit1-v6|::/0": {v6Route}, } - filtered := rs.FilterSelectedExitNodes(routes) - assert.NotContains(t, filtered, route.HAUniqueID("exit1|0.0.0.0/0")) - assert.Contains(t, filtered, route.HAUniqueID("exit1-v6|::/0"), "explicit v6 select wins over v4 base") + assert.Empty(t, filtered, "deselecting v4 base must drop the v6 pair even if it was explicitly selected before") }) - t.Run("non-v6-suffix routes unaffected", func(t *testing.T) { - rs := routeselector.NewRouteSelector() - require.NoError(t, rs.DeselectRoutes([]route.NetID{"exit1"}, all)) - - // A route literally named "exit1-something" must not pair-resolve. - assert.False(t, rs.HasUserSelectionForRoute("exit1-something")) - }) - - t.Run("filter v6 paired with deselected v4 base", func(t *testing.T) { + t.Run("filter drops synced v6 pair of deselected v4 base", func(t *testing.T) { rs := routeselector.NewRouteSelector() require.NoError(t, rs.DeselectRoutes([]route.NetID{"exit1"}, all)) + rs.SyncPairedSelection("exit1", "exit1-v6") v4Route := &route.Route{NetID: "exit1", Network: netip.MustParsePrefix("0.0.0.0/0")} v6Route := &route.Route{NetID: "exit1-v6", Network: netip.MustParsePrefix("::/0")} @@ -423,43 +424,13 @@ func TestRouteSelector_V6ExitPairInherits(t *testing.T) { assert.Empty(t, filtered, "deselecting v4 base must also drop the v6 pair") }) - // Regression for the observed bug: a stale explicit selection on the "-v6" - // sibling (e.g. persisted from an earlier select where the pair was expanded) - // must not survive a later deselect of the v4 base. Without clearing the orphan, - // effectiveNetID sees the v6's own explicit "selected" state and the ::/0 route - // leaks into the tunnel despite the user disabling the exit node. - t.Run("deselect v4 base clears orphaned explicit v6 selection", func(t *testing.T) { + t.Run("deselectAll makes sync a no-op", func(t *testing.T) { rs := routeselector.NewRouteSelector() + rs.DeselectAllRoutes() - // Prior state: both v4 and v6 explicitly selected (pair was expanded once). - require.NoError(t, rs.SelectRoutes([]route.NetID{"exit1", "exit1-v6"}, true, all)) - require.True(t, rs.IsSelected("exit1-v6")) + rs.SyncPairedSelection("exit1", "exit1-v6") - // User later deselects only the v4 base (v6 not expanded into this batch). - require.NoError(t, rs.DeselectRoutes([]route.NetID{"exit1"}, all)) - - // The orphaned explicit v6 selection must be gone, so the v6 inherits the - // v4 deselect via effectiveNetID instead of staying selected. - assert.False(t, rs.IsSelectedForExitNode("exit1-v6"), "v6 must inherit v4 deselect after orphan cleared") - - v4Route := &route.Route{NetID: "exit1", Network: netip.MustParsePrefix("0.0.0.0/0")} - v6Route := &route.Route{NetID: "exit1-v6", Network: netip.MustParsePrefix("::/0")} - routes := route.HAMap{ - "exit1|0.0.0.0/0": {v4Route}, - "exit1-v6|::/0": {v6Route}, - } - filtered := rs.FilterSelectedExitNodes(routes) - assert.Empty(t, filtered, "deselecting v4 base must drop the v6 pair even if it was explicitly selected before") - }) - - // The inverse: an explicit v6 selection made in the SAME batch as the v4 (the - // deliberate ExpandV6ExitPairs case) must be preserved, not wiped by the pair sync. - t.Run("explicit v6 select in same batch is preserved", func(t *testing.T) { - rs := routeselector.NewRouteSelector() - require.NoError(t, rs.SelectRoutes([]route.NetID{"exit1", "exit1-v6"}, true, all)) - - assert.True(t, rs.IsSelectedForExitNode("exit1")) - assert.True(t, rs.IsSelectedForExitNode("exit1-v6"), "v6 selected in the same batch must survive") + assert.False(t, rs.HasUserSelectionForRoute("exit1-v6"), "sync must not write explicit state under deselectAll") }) t.Run("non-exit *-v6 routes pass through FilterSelectedExitNodes", func(t *testing.T) {