diff --git a/client/internal/routemanager/exit_node_selection_test.go b/client/internal/routemanager/exit_node_selection_test.go new file mode 100644 index 000000000..c302dee98 --- /dev/null +++ b/client/internal/routemanager/exit_node_selection_test.go @@ -0,0 +1,191 @@ +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" +) + +func newExitNodeTestManager() *DefaultManager { + return &DefaultManager{routeSelector: routeselector.NewRouteSelector()} +} + +func exitRoute(netID, peer string, skipAutoApply bool) *route.Route { + return &route.Route{ + NetID: route.NetID(netID), + Network: netip.MustParsePrefix("0.0.0.0/0"), + Peer: peer, + SkipAutoApply: skipAutoApply, + } +} + +func TestPickPreferredExitNode(t *testing.T) { + tests := []struct { + name string + info exitNodeInfo + want route.NetID + }{ + { + name: "persisted user selection wins over management", + info: exitNodeInfo{ + allIDs: []route.NetID{"a", "b", "c"}, + userSelected: []route.NetID{"b"}, + selectedByManagement: []route.NetID{"a"}, + }, + want: "b", + }, + { + name: "multiple user-selected self-heal to deterministic min", + info: exitNodeInfo{ + allIDs: []route.NetID{"a", "b", "c"}, + userSelected: []route.NetID{"c", "a"}, + }, + want: "a", + }, + { + name: "explicit opt-out keeps none", + info: exitNodeInfo{ + allIDs: []route.NetID{"a", "b"}, + userDeselected: []route.NetID{"a", "b"}, + }, + want: "", + }, + { + name: "fresh defaults to management auto-apply pick", + info: exitNodeInfo{ + allIDs: []route.NetID{"a", "b", "c"}, + selectedByManagement: []route.NetID{"b"}, + }, + want: "b", + }, + { + name: "no user pick and no management auto-apply selects none", + info: exitNodeInfo{ + allIDs: []route.NetID{"c", "a", "b"}, + }, + want: "", + }, + { + name: "user-deselect does not block a management auto-apply sibling", + info: exitNodeInfo{ + allIDs: []route.NetID{"a", "b"}, + userDeselected: []route.NetID{"a"}, + selectedByManagement: []route.NetID{"b"}, + }, + want: "b", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, pickPreferredExitNode(tt.info), "preferred exit node") + }) + } +} + +func TestEnforceSingleExitNode(t *testing.T) { + m := newExitNodeTestManager() + all := []route.NetID{"a", "b", "c"} + + m.enforceSingleExitNode("b", all) + assert.False(t, m.routeSelector.IsSelected("a"), "a should be deselected") + assert.True(t, m.routeSelector.IsSelected("b"), "b should be the only selected exit node") + assert.False(t, m.routeSelector.IsSelected("c"), "c should be deselected") + + // Switching the preferred node moves the single selection. + m.enforceSingleExitNode("c", all) + assert.False(t, m.routeSelector.IsSelected("a"), "a stays deselected") + assert.False(t, m.routeSelector.IsSelected("b"), "b should now be deselected") + assert.True(t, m.routeSelector.IsSelected("c"), "c should now be selected") + + // Empty preferred turns every exit node off. + m.enforceSingleExitNode("", all) + for _, id := range all { + assert.False(t, m.routeSelector.IsSelected(id), "no exit node should be selected") + } +} + +func TestEnforceSingleExitNode_RespectsDeselectAll(t *testing.T) { + m := newExitNodeTestManager() + m.routeSelector.DeselectAllRoutes() + + m.enforceSingleExitNode("b", []route.NetID{"a", "b"}) + + assert.True(t, m.routeSelector.IsDeselectAllActive(), "global deselect-all must stay in effect") + assert.False(t, m.routeSelector.IsSelected("b"), "no exit node should be forced on while deselect-all is set") +} + +func TestUpdateRouteSelectorFromManagement_FreshSelectsOne(t *testing.T) { + m := newExitNodeTestManager() + routes := route.HAMap{ + "exitA|0.0.0.0/0": {exitRoute("exitA", "p1", false)}, + "exitB|0.0.0.0/0": {exitRoute("exitB", "p2", false)}, + "lan|192.168.1.0/24": {{NetID: "lan", Network: netip.MustParsePrefix("192.168.1.0/24"), Peer: "p3"}}, + "exitC|0.0.0.0/0": {exitRoute("exitC", "p4", false)}, + } + + m.updateRouteSelectorFromManagement(routes) + + // Exactly one exit node (the deterministic first) is selected. + assert.True(t, m.routeSelector.IsSelected("exitA"), "exitA is the deterministic default") + assert.False(t, m.routeSelector.IsSelected("exitB"), "exitB must not also be selected") + assert.False(t, m.routeSelector.IsSelected("exitC"), "exitC must not also be selected") + // Non-exit routes are left at their default-on state. + assert.True(t, m.routeSelector.IsSelected("lan"), "non-exit route selection is untouched") +} + +func TestUpdateRouteSelectorFromManagement_HonorsPersistedPick(t *testing.T) { + m := newExitNodeTestManager() + routes := route.HAMap{ + "exitA|0.0.0.0/0": {exitRoute("exitA", "p1", false)}, + "exitB|0.0.0.0/0": {exitRoute("exitB", "p2", false)}, + } + all := []route.NetID{"exitA", "exitB"} + + // Simulate the state the runtime select path leaves behind: exactly one + // exit node explicitly selected, its sibling deselected. + require.NoError(t, m.routeSelector.SelectRoutes([]route.NetID{"exitB"}, true, all)) + require.NoError(t, m.routeSelector.DeselectRoutes([]route.NetID{"exitA"}, all)) + + m.updateRouteSelectorFromManagement(routes) + + assert.True(t, m.routeSelector.IsSelected("exitB"), "persisted pick must stay selected") + assert.False(t, m.routeSelector.IsSelected("exitA"), "the other exit node stays deselected") +} + +func TestUpdateRouteSelectorFromManagement_OptOutKeepsNone(t *testing.T) { + m := newExitNodeTestManager() + routes := route.HAMap{ + "exitA|0.0.0.0/0": {exitRoute("exitA", "p1", false)}, + "exitB|0.0.0.0/0": {exitRoute("exitB", "p2", false)}, + } + all := []route.NetID{"exitA", "exitB"} + + // User deselected exit nodes and selected none. + require.NoError(t, m.routeSelector.DeselectRoutes(all, all)) + + m.updateRouteSelectorFromManagement(routes) + + assert.False(t, m.routeSelector.IsSelected("exitA"), "opt-out keeps exitA off") + assert.False(t, m.routeSelector.IsSelected("exitB"), "opt-out keeps exitB off") +} + +func TestUpdateRouteSelectorFromManagement_NoAutoApplySelectsNone(t *testing.T) { + m := newExitNodeTestManager() + // SkipAutoApply=true: management offers the exit nodes but doesn't request + // auto-activation, so none should be selected until the user picks one. + routes := route.HAMap{ + "exitA|0.0.0.0/0": {exitRoute("exitA", "p1", true)}, + "exitB|0.0.0.0/0": {exitRoute("exitB", "p2", true)}, + } + + m.updateRouteSelectorFromManagement(routes) + + assert.False(t, m.routeSelector.IsSelected("exitA"), "no auto-apply keeps exitA off") + assert.False(t, m.routeSelector.IsSelected("exitB"), "no auto-apply keeps exitB off") +} diff --git a/client/internal/routemanager/manager.go b/client/internal/routemanager/manager.go index 2325267fd..c4e8031d6 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" @@ -707,15 +708,22 @@ func resolveURLsToIPs(urls []string) []net.IP { return ips } -// updateRouteSelectorFromManagement updates the route selector based on the isSelected status from the management server +// updateRouteSelectorFromManagement reconciles exit-node selection on every +// network map: it keeps at most one exit node selected — the user's persisted +// pick, else whatever management marks for auto-apply (SkipAutoApply=false), +// else none. We never auto-activate an exit node the map doesn't request; it +// stays off until the user picks it. Exit nodes are mutually exclusive, but the +// RouteSelector stores routes with default-on semantics, so without this every +// available exit node would report selected at once. func (m *DefaultManager) updateRouteSelectorFromManagement(clientRoutes route.HAMap) { - exitNodeInfo := m.collectExitNodeInfo(clientRoutes) - if len(exitNodeInfo.allIDs) == 0 { + info := m.collectExitNodeInfo(clientRoutes) + if len(info.allIDs) == 0 { return } - m.updateExitNodeSelections(exitNodeInfo) - m.logExitNodeUpdate(exitNodeInfo) + preferred := pickPreferredExitNode(info) + m.enforceSingleExitNode(preferred, info.allIDs) + m.logExitNodeUpdate(info, preferred) } type exitNodeInfo struct { @@ -725,6 +733,10 @@ type exitNodeInfo struct { userDeselected []route.NetID } +// collectExitNodeInfo categorises the available exit nodes by their persisted +// selection state. It keys on the base (v4) NetID and skips the synthesized +// "-v6" partner, which inherits its base's selection through the RouteSelector +// — counting it separately would double-count the pair. func (m *DefaultManager) collectExitNodeInfo(clientRoutes route.HAMap) exitNodeInfo { var info exitNodeInfo @@ -734,6 +746,9 @@ func (m *DefaultManager) collectExitNodeInfo(clientRoutes route.HAMap) exitNodeI } netID := haID.NetID() + if strings.HasSuffix(string(netID), route.V6ExitSuffix) { + continue + } info.allIDs = append(info.allIDs, netID) if m.routeSelector.HasUserSelectionForRoute(netID) { @@ -770,45 +785,69 @@ func (m *DefaultManager) checkManagementSelection(routes []*route.Route, netID r } } -func (m *DefaultManager) updateExitNodeSelections(info exitNodeInfo) { - routesToDeselect := m.getRoutesToDeselect(info.allIDs) - m.deselectExitNodes(routesToDeselect) - m.selectExitNodesByManagement(info.selectedByManagement, info.allIDs) +// pickPreferredExitNode chooses the single exit node to keep selected. In order: +// - a persisted user selection wins (deterministic if several survive from +// legacy state, so the set self-heals down to one); +// - otherwise activate only what management marks for auto-apply +// (SkipAutoApply=false); the lexicographically first if it marks several. +// +// Returns "" when neither holds — we never force an arbitrary exit node on. A +// route the map doesn't auto-apply stays off until the user selects it. +// info.userDeselected is informational only: an explicit deselect simply keeps +// that route out of both lists above, so it can't be picked. +func pickPreferredExitNode(info exitNodeInfo) route.NetID { + if len(info.userSelected) > 0 { + return minNetID(info.userSelected) + } + if len(info.selectedByManagement) > 0 { + return minNetID(info.selectedByManagement) + } + return "" } -func (m *DefaultManager) getRoutesToDeselect(allIDs []route.NetID) []route.NetID { - var routesToDeselect []route.NetID - for _, netID := range allIDs { - if !m.routeSelector.HasUserSelectionForRoute(netID) { - routesToDeselect = append(routesToDeselect, netID) +// enforceSingleExitNode makes preferred the only selected exit node: every other +// available exit node is deselected and preferred (if any) is selected, without +// disturbing non-exit route selections. A global deselect-all is left untouched +// so the user's "all off" stays in effect. +func (m *DefaultManager) enforceSingleExitNode(preferred route.NetID, allIDs []route.NetID) { + if m.routeSelector.IsDeselectAllActive() { + return + } + + others := make([]route.NetID, 0, len(allIDs)) + for _, id := range allIDs { + if id != preferred { + others = append(others, id) } } - return routesToDeselect -} - -func (m *DefaultManager) deselectExitNodes(routesToDeselect []route.NetID) { - if len(routesToDeselect) == 0 { - return + if len(others) > 0 { + if err := m.routeSelector.DeselectRoutes(others, allIDs); err != nil { + log.Warnf("deselect other exit nodes: %v", err) + } } - - err := m.routeSelector.DeselectRoutes(routesToDeselect, routesToDeselect) - if err != nil { - log.Warnf("Failed to deselect exit nodes: %v", err) + if preferred != "" { + if err := m.routeSelector.SelectRoutes([]route.NetID{preferred}, true, allIDs); err != nil { + log.Warnf("select preferred exit node %q: %v", preferred, err) + } } } -func (m *DefaultManager) selectExitNodesByManagement(selectedByManagement []route.NetID, allIDs []route.NetID) { - if len(selectedByManagement) == 0 { - return - } - - err := m.routeSelector.SelectRoutes(selectedByManagement, true, allIDs) - if err != nil { - log.Warnf("Failed to select exit nodes: %v", err) - } +func (m *DefaultManager) logExitNodeUpdate(info exitNodeInfo, preferred route.NetID) { + log.Debugf("Exit node selection: %d available, preferred=%q (%d user-selected, %d user-deselected, %d management-selected)", + len(info.allIDs), preferred, len(info.userSelected), len(info.userDeselected), len(info.selectedByManagement)) } -func (m *DefaultManager) logExitNodeUpdate(info exitNodeInfo) { - log.Debugf("Updated route selector: %d exit nodes available, %d selected by management, %d user-selected, %d user-deselected", - len(info.allIDs), len(info.selectedByManagement), len(info.userSelected), len(info.userDeselected)) +// minNetID returns the lexicographically smallest NetID, for a deterministic +// default pick that stays stable across restarts. +func minNetID(ids []route.NetID) route.NetID { + if len(ids) == 0 { + return "" + } + best := ids[0] + for _, id := range ids[1:] { + if id < best { + best = id + } + } + return best } diff --git a/client/internal/routeselector/routeselector.go b/client/internal/routeselector/routeselector.go index 2ddc24bf2..9f8064fd3 100644 --- a/client/internal/routeselector/routeselector.go +++ b/client/internal/routeselector/routeselector.go @@ -124,6 +124,16 @@ func (rs *RouteSelector) IsSelected(routeID route.NetID) bool { return rs.isSelectedLocked(routeID) } +// IsDeselectAllActive reports whether the global "deselect all" flag is set, +// i.e. the user disabled every route. Callers enforcing per-route invariants +// (e.g. single exit node) should leave the selection untouched when it is. +func (rs *RouteSelector) IsDeselectAllActive() bool { + rs.mu.RLock() + defer rs.mu.RUnlock() + + return rs.deselectAll +} + // FilterSelected removes unselected routes from the provided map. func (rs *RouteSelector) FilterSelected(routes route.HAMap) route.HAMap { rs.mu.RLock() diff --git a/client/server/network.go b/client/server/network.go index 12cefbd9c..c3afdb742 100644 --- a/client/server/network.go +++ b/client/server/network.go @@ -172,6 +172,17 @@ func (s *Server) SelectNetworks(_ context.Context, req *proto.SelectNetworksRequ if err := routeSelector.SelectRoutes(routes, req.GetAppend(), netIdRoutes); err != nil { return nil, fmt.Errorf("select routes: %w", err) } + + // Exit nodes are mutually exclusive: if this selection activates an + // exit node, deselect every other available exit node so two can't be + // selected at once. Non-exit route selections are left untouched. + if requestActivatesExitNode(routes, routesMap) { + if others := otherExitNodeIDs(routesMap, routes); len(others) > 0 { + if err := routeSelector.DeselectRoutes(others, netIdRoutes); err != nil { + return nil, fmt.Errorf("deselect sibling exit nodes: %w", err) + } + } + } } routeManager.TriggerSelection(routeManager.GetClientRoutes()) @@ -249,3 +260,38 @@ func toNetIDs(routes []string) []route.NetID { } return netIDs } + +func isExitNodeRoutes(routes []*route.Route) bool { + return len(routes) > 0 && (route.IsV4DefaultRoute(routes[0].Network) || route.IsV6DefaultRoute(routes[0].Network)) +} + +// requestActivatesExitNode reports whether any requested NetID maps to an exit +// node (default route) in the current route table. +func requestActivatesExitNode(requested []route.NetID, routesMap map[route.NetID][]*route.Route) bool { + for _, id := range requested { + if isExitNodeRoutes(routesMap[id]) { + return true + } + } + return false +} + +// otherExitNodeIDs returns every available exit-node NetID that is not in the +// requested set — the siblings to deselect so a single exit node stays active. +func otherExitNodeIDs(routesMap map[route.NetID][]*route.Route, requested []route.NetID) []route.NetID { + keep := make(map[route.NetID]struct{}, len(requested)) + for _, id := range requested { + keep[id] = struct{}{} + } + var others []route.NetID + for id, routes := range routesMap { + if !isExitNodeRoutes(routes) { + continue + } + if _, ok := keep[id]; ok { + continue + } + others = append(others, id) + } + return others +} diff --git a/client/server/network_exitnode_test.go b/client/server/network_exitnode_test.go new file mode 100644 index 000000000..1c0ba0ecb --- /dev/null +++ b/client/server/network_exitnode_test.go @@ -0,0 +1,26 @@ +package server + +import ( + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/netbirdio/netbird/route" +) + +func TestExitNodeSelectionHelpers(t *testing.T) { + routesMap := map[route.NetID][]*route.Route{ + "exitA": {{Network: netip.MustParsePrefix("0.0.0.0/0")}}, + "exitB": {{Network: netip.MustParsePrefix("::/0")}}, + "lan": {{Network: netip.MustParsePrefix("192.168.0.0/16")}}, + } + + assert.True(t, requestActivatesExitNode([]route.NetID{"exitA"}, routesMap), "v4 default route is an exit node") + assert.True(t, requestActivatesExitNode([]route.NetID{"exitB"}, routesMap), "v6 default route is an exit node") + assert.False(t, requestActivatesExitNode([]route.NetID{"lan"}, routesMap), "lan route is not an exit node") + assert.False(t, requestActivatesExitNode([]route.NetID{"missing"}, routesMap), "unknown id is not an exit node") + + others := otherExitNodeIDs(routesMap, []route.NetID{"exitB"}) + assert.ElementsMatch(t, []route.NetID{"exitA"}, others, "only the other exit node is a sibling; the lan route is ignored") +} diff --git a/client/ui/frontend/src/modules/networks/NetworksContext.tsx b/client/ui/frontend/src/modules/networks/NetworksContext.tsx index 08308e334..c29eee4d8 100644 --- a/client/ui/frontend/src/modules/networks/NetworksContext.tsx +++ b/client/ui/frontend/src/modules/networks/NetworksContext.tsx @@ -87,8 +87,9 @@ export const NetworksProvider = ({ children }: { children: ReactNode }) => { [refresh], ); - // Exit nodes are mutually exclusive — Select with append=false clears any - // other selection before activating the new one. + // Exit nodes are mutually exclusive, but the daemon enforces that now — + // selecting one deselects the other exit nodes. Append so activating an + // exit node doesn't wipe the user's network-route selections. const toggleExitNode = useCallback( async (id: string, selected: boolean) => { try { @@ -101,7 +102,7 @@ export const NetworksProvider = ({ children }: { children: ReactNode }) => { } else { await NetworksSvc.Select({ networkIds: [id], - append: false, + append: true, all: false, }); }