From 2ef457be957cd24e4b329b894635b389c62eb27c Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Tue, 28 Jul 2026 17:39:37 +0200 Subject: [PATCH] [client] Unify route selection in the route manager (#6928) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Describe your changes Move route select/deselect handling from the daemon server into exported routemanager methods (SelectRoutes, DeselectRoutes, SelectAllRoutes, DeselectAllRoutes) so every consumer shares one implementation: v4/v6 exit-pair expansion, exit-node mutual exclusion, and selection triggering. Previously the exit-node exclusivity lived only in the daemon's SelectNetworks RPC, so the Android and iOS bindings could leave two exit nodes selected until the next network map reconciliation. Both bindings now call the shared manager methods and enforce exclusivity at toggle time, matching the desktop behavior. ## Issue ticket number and link ## 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) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > 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/__ --- View with [code]smith Autofix with [code]smith Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled. ## Summary by CodeRabbit * **New Features** * Route selection/deselection is now handled through shared route-manager APIs for both individual routes and “all routes”. * Exit-node selections automatically enforce mutual exclusivity while keeping non-exit routes unaffected. * **Bug Fixes** * Unknown or unavailable route IDs now return errors, and exclusivity is preserved even when some route IDs fail. * **Tests** * Added route-selection tests covering exclusivity (including IPv4/IPv6), select-all behavior, partial errors, and invalid IDs. * **Refactor / Chores** * Simplified Android, iOS, and server routing flows to delegate to the shared manager; updated mocks and removed redundant routing command logic/dependencies. --- client/android/client.go | 12 +- client/android/route_command.go | 70 --------- client/internal/routemanager/manager.go | 13 +- client/internal/routemanager/mock.go | 26 ++++ client/internal/routemanager/selection.go | 138 ++++++++++++++++++ .../internal/routemanager/selection_test.go | 129 ++++++++++++++++ client/ios/NetBirdSDK/client.go | 42 ++---- client/server/network.go | 74 +--------- client/server/network_exitnode_test.go | 26 ---- 9 files changed, 324 insertions(+), 206 deletions(-) delete mode 100644 client/android/route_command.go create mode 100644 client/internal/routemanager/selection.go create mode 100644 client/internal/routemanager/selection_test.go delete mode 100644 client/server/network_exitnode_test.go diff --git a/client/android/client.go b/client/android/client.go index 2266ff53d..2ce627f10 100644 --- a/client/android/client.go +++ b/client/android/client.go @@ -439,10 +439,6 @@ func (c *Client) RemoveConnectionListener() { c.recorder.RemoveConnectionListener() } -func (c *Client) toggleRoute(command routeCommand) error { - return command.toggleRoute() -} - func (c *Client) getRouteManager() (routemanager.Manager, error) { client := c.getConnectClient() if client == nil { @@ -462,22 +458,22 @@ func (c *Client) getRouteManager() (routemanager.Manager, error) { return manager, nil } -func (c *Client) SelectRoute(route string) error { +func (c *Client) SelectRoute(id string) error { manager, err := c.getRouteManager() if err != nil { return err } - return c.toggleRoute(selectRouteCommand{route: route, manager: manager}) + return manager.SelectRoutes([]route.NetID{route.NetID(id)}, true) } -func (c *Client) DeselectRoute(route string) error { +func (c *Client) DeselectRoute(id string) error { manager, err := c.getRouteManager() if err != nil { return err } - return c.toggleRoute(deselectRouteCommand{route: route, manager: manager}) + return manager.DeselectRoutes([]route.NetID{route.NetID(id)}) } // getNetworkDomainsFromRoute extracts domains from a route and enriches each domain diff --git a/client/android/route_command.go b/client/android/route_command.go deleted file mode 100644 index 5e7357335..000000000 --- a/client/android/route_command.go +++ /dev/null @@ -1,70 +0,0 @@ -//go:build android - -package android - -import ( - "fmt" - - log "github.com/sirupsen/logrus" - "golang.org/x/exp/maps" - - "github.com/netbirdio/netbird/client/internal/routemanager" - "github.com/netbirdio/netbird/route" -) - -func executeRouteToggle(id string, manager routemanager.Manager, - operationName string, - routeOperation func(routes []route.NetID, allRoutes []route.NetID) error) error { - netID := route.NetID(id) - routes := []route.NetID{netID} - - routesMap := manager.GetClientRoutesWithNetID() - routes = route.ExpandV6ExitPairs(routes, routesMap) - - log.Debugf("%s with ids: %v", operationName, routes) - - if err := routeOperation(routes, maps.Keys(routesMap)); err != nil { - log.Debugf("error when %s: %s", operationName, err) - return fmt.Errorf("error %s: %w", operationName, err) - } - - manager.TriggerSelection(manager.GetClientRoutes()) - - return nil -} - -type routeCommand interface { - toggleRoute() error -} - -type selectRouteCommand struct { - route string - manager routemanager.Manager -} - -func (s selectRouteCommand) toggleRoute() error { - routeSelector := s.manager.GetRouteSelector() - if routeSelector == nil { - return fmt.Errorf("no route selector available") - } - - routeOperation := func(routes []route.NetID, allRoutes []route.NetID) error { - return routeSelector.SelectRoutes(routes, true, allRoutes) - } - - return executeRouteToggle(s.route, s.manager, "selecting route", routeOperation) -} - -type deselectRouteCommand struct { - route string - manager routemanager.Manager -} - -func (d deselectRouteCommand) toggleRoute() error { - routeSelector := d.manager.GetRouteSelector() - if routeSelector == nil { - return fmt.Errorf("no route selector available") - } - - return executeRouteToggle(d.route, d.manager, "deselecting route", routeSelector.DeselectRoutes) -} diff --git a/client/internal/routemanager/manager.go b/client/internal/routemanager/manager.go index 7a818b539..2ab7e2a85 100644 --- a/client/internal/routemanager/manager.go +++ b/client/internal/routemanager/manager.go @@ -52,6 +52,10 @@ type Manager interface { UpdateRoutes(updateSerial uint64, serverRoutes map[route.ID]*route.Route, clientRoutes route.HAMap, useNewDNSRoute bool) error ClassifyRoutes(newRoutes []*route.Route) (map[route.ID]*route.Route, route.HAMap) TriggerSelection(route.HAMap) + SelectRoutes(ids []route.NetID, appendRoute bool) error + DeselectRoutes(ids []route.NetID) error + SelectAllRoutes() + DeselectAllRoutes() GetRouteSelector() *routeselector.RouteSelector GetClientRoutes() route.HAMap GetSelectedClientRoutes() route.HAMap @@ -800,7 +804,7 @@ func (m *DefaultManager) collectExitNodeInfo(clientRoutes route.HAMap) exitNodeI var info exitNodeInfo for haID, routes := range clientRoutes { - if !m.isExitNodeRoute(routes) { + if !isExitNodeRoutes(routes) { continue } @@ -820,13 +824,6 @@ func (m *DefaultManager) collectExitNodeInfo(clientRoutes route.HAMap) exitNodeI return info } -func (m *DefaultManager) isExitNodeRoute(routes []*route.Route) bool { - if len(routes) == 0 { - return false - } - return route.IsV4DefaultRoute(routes[0].Network) || route.IsV6DefaultRoute(routes[0].Network) -} - func (m *DefaultManager) categorizeUserSelection(netID route.NetID, info *exitNodeInfo) { if m.routeSelector.IsSelected(netID) { info.userSelected = append(info.userSelected, netID) diff --git a/client/internal/routemanager/mock.go b/client/internal/routemanager/mock.go index c1620b24c..cf761091d 100644 --- a/client/internal/routemanager/mock.go +++ b/client/internal/routemanager/mock.go @@ -16,6 +16,8 @@ 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) + SelectRoutesFunc func(ids []route.NetID, appendRoute bool) error + DeselectRoutesFunc func(ids []route.NetID) error GetRouteSelectorFunc func() *routeselector.RouteSelector GetClientRoutesFunc func() route.HAMap GetSelectedClientRoutesFunc func() route.HAMap @@ -55,6 +57,30 @@ func (m *MockManager) TriggerSelection(networks route.HAMap) { } } +// SelectRoutes mock implementation of SelectRoutes from Manager interface +func (m *MockManager) SelectRoutes(ids []route.NetID, appendRoute bool) error { + if m.SelectRoutesFunc != nil { + return m.SelectRoutesFunc(ids, appendRoute) + } + return nil +} + +// DeselectRoutes mock implementation of DeselectRoutes from Manager interface +func (m *MockManager) DeselectRoutes(ids []route.NetID) error { + if m.DeselectRoutesFunc != nil { + return m.DeselectRoutesFunc(ids) + } + return nil +} + +// SelectAllRoutes mock implementation of SelectAllRoutes from Manager interface +func (m *MockManager) SelectAllRoutes() { +} + +// DeselectAllRoutes mock implementation of DeselectAllRoutes from Manager interface +func (m *MockManager) DeselectAllRoutes() { +} + // GetRouteSelector mock implementation of GetRouteSelector from Manager interface func (m *MockManager) GetRouteSelector() *routeselector.RouteSelector { if m.GetRouteSelectorFunc != nil { diff --git a/client/internal/routemanager/selection.go b/client/internal/routemanager/selection.go new file mode 100644 index 000000000..6d5feec79 --- /dev/null +++ b/client/internal/routemanager/selection.go @@ -0,0 +1,138 @@ +package routemanager + +import ( + "fmt" + "slices" + + "github.com/hashicorp/go-multierror" + log "github.com/sirupsen/logrus" + "golang.org/x/exp/maps" + + nberrors "github.com/netbirdio/netbird/client/errors" + "github.com/netbirdio/netbird/route" +) + +// SelectRoutes selects the routes with the given network IDs and applies the +// new selection. V4/v6 exit-node pairs are expanded automatically. Exit nodes +// are mutually exclusive: if the selection activates an exit node, every other +// available exit node is deselected so two can't be active at once. With +// appendRoute=false the previous selection is replaced instead of extended. +func (m *DefaultManager) SelectRoutes(ids []route.NetID, appendRoute bool) error { + if err := m.selectRoutes(ids, appendRoute); err != nil { + return err + } + m.TriggerSelection(m.GetClientRoutes()) + return nil +} + +// DeselectRoutes removes the routes with the given network IDs from the +// selection and applies the change. V4/v6 exit-node pairs are expanded +// automatically. +func (m *DefaultManager) DeselectRoutes(ids []route.NetID) error { + if err := m.deselectRoutes(ids); err != nil { + return err + } + m.TriggerSelection(m.GetClientRoutes()) + return nil +} + +func (m *DefaultManager) deselectRoutes(ids []route.NetID) error { + routesMap := m.GetClientRoutesWithNetID() + routes := route.ExpandV6ExitPairs(slices.Clone(ids), routesMap) + + log.Debugf("deselecting routes with ids: %v", routes) + + if err := m.routeSelector.DeselectRoutes(routes, maps.Keys(routesMap)); err != nil { + return fmt.Errorf("deselect routes: %w", err) + } + + return nil +} + +// SelectAllRoutes selects every available route and applies the selection. +// Exit nodes stay mutually exclusive: at most one remains active. +func (m *DefaultManager) SelectAllRoutes() { + m.selectAllRoutes() + m.TriggerSelection(m.GetClientRoutes()) +} + +func (m *DefaultManager) selectAllRoutes() { + m.routeSelector.SelectAllRoutes() + + // Select-all wipes every explicit selection, so exit nodes fall back to + // management's auto-apply flags — which may mark several at once. + // Reconcile immediately so at most one exit node stays active instead of + // waiting for the next network map to enforce it. + m.mux.Lock() + defer m.mux.Unlock() + m.updateRouteSelectorFromManagement(m.clientRoutes) +} + +// DeselectAllRoutes deselects every route and applies the change. +func (m *DefaultManager) DeselectAllRoutes() { + m.routeSelector.DeselectAllRoutes() + m.TriggerSelection(m.GetClientRoutes()) +} + +func (m *DefaultManager) selectRoutes(ids []route.NetID, appendRoute bool) error { + routesMap := m.GetClientRoutesWithNetID() + routes := route.ExpandV6ExitPairs(slices.Clone(ids), routesMap) + allIDs := maps.Keys(routesMap) + + log.Debugf("selecting routes with ids: %v", routes) + + // A partial failure (e.g. an unknown ID in the request) still selects the + // valid routes, so exclusivity below must run regardless of the error. + var merr *multierror.Error + if err := m.routeSelector.SelectRoutes(routes, appendRoute, allIDs); err != nil { + merr = multierror.Append(merr, 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 := m.routeSelector.DeselectRoutes(others, allIDs); err != nil { + merr = multierror.Append(merr, fmt.Errorf("deselect sibling exit nodes: %w", err)) + } + } + } + + return nberrors.FormatErrorOrNil(merr) +} + +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/internal/routemanager/selection_test.go b/client/internal/routemanager/selection_test.go new file mode 100644 index 000000000..6066b5661 --- /dev/null +++ b/client/internal/routemanager/selection_test.go @@ -0,0 +1,129 @@ +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 v6ExitRoute(netID, peer string) *route.Route { + return &route.Route{ + NetID: route.NetID(netID), + Network: netip.MustParsePrefix("::/0"), + Peer: peer, + } +} + +func newSelectionTestManager() *DefaultManager { + return &DefaultManager{ + routeSelector: routeselector.NewRouteSelector(), + clientRoutes: route.HAMap{ + "exitA|0.0.0.0/0": {exitRoute("exitA", "p1", true)}, + "exitA-v6|::/0": {v6ExitRoute("exitA-v6", "p1")}, + "exitB|0.0.0.0/0": {exitRoute("exitB", "p2", true)}, + "lan|192.168.1.0/24": {{NetID: "lan", Network: netip.MustParsePrefix("192.168.1.0/24"), Peer: "p3"}}, + }, + } +} + +func TestSelectRoutes_ExitNodeExclusivity(t *testing.T) { + m := newSelectionTestManager() + + // Selecting an exit node selects its v6 pair and deselects the sibling. + require.NoError(t, m.selectRoutes([]route.NetID{"exitA"}, true)) + assert.True(t, m.routeSelector.IsSelected("exitA"), "exitA should be selected") + assert.True(t, m.routeSelector.IsSelected("exitA-v6"), "the v6 pair follows its v4 base") + assert.False(t, m.routeSelector.IsSelected("exitB"), "the sibling exit node must be deselected") + + // Switching to the sibling deselects the previous exit node and its v6 pair. + require.NoError(t, m.selectRoutes([]route.NetID{"exitB"}, true)) + assert.True(t, m.routeSelector.IsSelected("exitB"), "exitB should now be selected") + assert.False(t, m.routeSelector.IsSelected("exitA"), "the previous exit node must be deselected") + assert.False(t, m.routeSelector.IsSelected("exitA-v6"), "the previous exit node's v6 pair must be deselected") + assert.True(t, m.routeSelector.IsSelected("lan"), "non-exit route selection is untouched") + + // Selecting a non-exit route leaves the active exit node alone. + require.NoError(t, m.selectRoutes([]route.NetID{"lan"}, true)) + assert.True(t, m.routeSelector.IsSelected("exitB"), "selecting a non-exit route keeps the exit node") + + // Deselecting the active exit node turns every exit node off. + require.NoError(t, m.deselectRoutes([]route.NetID{"exitB"})) + assert.False(t, m.routeSelector.IsSelected("exitB"), "exitB should be deselected") + assert.False(t, m.routeSelector.IsSelected("exitA"), "exitA stays deselected") + assert.True(t, m.routeSelector.IsSelected("lan"), "non-exit route selection is untouched") +} + +func TestSelectRoutes_PartialErrorStillEnforcesExclusivity(t *testing.T) { + // The unknown ID must be reported, but the valid exit node in the same + // request is still selected — so its sibling must still be deselected. + // Both orderings are covered: processing must continue past the invalid + // ID wherever it sits in the request. + requests := map[string][]route.NetID{ + "invalid id first": {"missing", "exitB"}, + "invalid id last": {"exitB", "missing"}, + } + + for name, ids := range requests { + t.Run(name, func(t *testing.T) { + m := newSelectionTestManager() + + require.NoError(t, m.selectRoutes([]route.NetID{"exitA"}, true)) + + err := m.selectRoutes(ids, true) + assert.Error(t, err, "unknown id must be reported") + assert.True(t, m.routeSelector.IsSelected("exitB"), "valid exit node from the request is selected") + assert.False(t, m.routeSelector.IsSelected("exitA"), "sibling exit node must be deselected despite the error") + assert.False(t, m.routeSelector.IsSelected("exitA-v6"), "sibling's v6 pair must be deselected too") + }) + } +} + +func TestSelectAllRoutes_KeepsSingleExitNode(t *testing.T) { + // Both exit nodes are marked for auto-apply by management + // (SkipAutoApply=false), the state where select-all could turn on two at + // once without the immediate reconciliation. + m := &DefaultManager{ + routeSelector: routeselector.NewRouteSelector(), + clientRoutes: 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"}}, + }, + } + + require.NoError(t, m.selectRoutes([]route.NetID{"exitB"}, true)) + + m.selectAllRoutes() + + assert.True(t, m.routeSelector.IsSelected("lan"), "non-exit routes are all selected") + assert.True(t, m.routeSelector.IsSelected("exitA"), "the deterministic management pick stays active") + assert.False(t, m.routeSelector.IsSelected("exitB"), "select-all must not leave a second exit node active") +} + +func TestSelectRoutes_UnknownRoute(t *testing.T) { + m := newSelectionTestManager() + + assert.Error(t, m.selectRoutes([]route.NetID{"missing"}, true), "selecting an unavailable route must fail") + assert.Error(t, m.deselectRoutes([]route.NetID{"missing"}), "deselecting an unavailable route must fail") +} + +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/ios/NetBirdSDK/client.go b/client/ios/NetBirdSDK/client.go index a2f123900..9289a3910 100644 --- a/client/ios/NetBirdSDK/client.go +++ b/client/ios/NetBirdSDK/client.go @@ -13,7 +13,6 @@ import ( "time" log "github.com/sirupsen/logrus" - "golang.org/x/exp/maps" "github.com/netbirdio/netbird/client/internal" "github.com/netbirdio/netbird/client/internal/auth" @@ -637,23 +636,18 @@ func (c *Client) SelectRoute(id string) error { } routeManager := engine.GetRouteManager() - routeSelector := routeManager.GetRouteSelector() if id == "All" { log.Debugf("select all routes") - routeSelector.SelectAllRoutes() - } else { - log.Debugf("select route with id: %s", id) - routes := toNetIDs([]string{id}) - routesMap := routeManager.GetClientRoutesWithNetID() - routes = route.ExpandV6ExitPairs(routes, routesMap) - if err := routeSelector.SelectRoutes(routes, true, maps.Keys(routesMap)); err != nil { - log.Debugf("error when selecting routes: %s", err) - return fmt.Errorf("select routes: %w", err) - } + routeManager.SelectAllRoutes() + return nil } - routeManager.TriggerSelection(routeManager.GetClientRoutes()) - return nil + log.Debugf("select route with id: %s", id) + if err := routeManager.SelectRoutes(toNetIDs([]string{id}), true); err != nil { + log.Debugf("error when selecting routes: %s", err) + return err + } + return nil } func (c *Client) DeselectRoute(id string) error { @@ -667,21 +661,17 @@ func (c *Client) DeselectRoute(id string) error { } routeManager := engine.GetRouteManager() - routeSelector := routeManager.GetRouteSelector() if id == "All" { log.Debugf("deselect all routes") - routeSelector.DeselectAllRoutes() - } else { - log.Debugf("deselect route with id: %s", id) - routes := toNetIDs([]string{id}) - routesMap := routeManager.GetClientRoutesWithNetID() - routes = route.ExpandV6ExitPairs(routes, routesMap) - if err := routeSelector.DeselectRoutes(routes, maps.Keys(routesMap)); err != nil { - log.Debugf("error when deselecting routes: %s", err) - return fmt.Errorf("deselect routes: %w", err) - } + routeManager.DeselectAllRoutes() + return nil + } + + log.Debugf("deselect route with id: %s", id) + if err := routeManager.DeselectRoutes(toNetIDs([]string{id})); err != nil { + log.Debugf("error when deselecting routes: %s", err) + return err } - routeManager.TriggerSelection(routeManager.GetClientRoutes()) return nil } diff --git a/client/server/network.go b/client/server/network.go index c38715256..c390b8180 100644 --- a/client/server/network.go +++ b/client/server/network.go @@ -8,7 +8,6 @@ import ( "sort" "strings" - "golang.org/x/exp/maps" "google.golang.org/grpc/codes" gstatus "google.golang.org/grpc/status" @@ -161,30 +160,11 @@ func (s *Server) SelectNetworks(_ context.Context, req *proto.SelectNetworksRequ return nil, fmt.Errorf("no route manager") } - routeSelector := routeManager.GetRouteSelector() if req.GetAll() { - routeSelector.SelectAllRoutes() - } else { - routes := toNetIDs(req.GetNetworkIDs()) - routesMap := routeManager.GetClientRoutesWithNetID() - routes = route.ExpandV6ExitPairs(routes, routesMap) - netIdRoutes := maps.Keys(routesMap) - 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.SelectAllRoutes() + } else if err := routeManager.SelectRoutes(toNetIDs(req.GetNetworkIDs()), req.GetAppend()); err != nil { + return nil, err } - routeManager.TriggerSelection(routeManager.GetClientRoutes()) s.statusRecorder.PublishEvent( proto.SystemEvent_INFO, @@ -224,19 +204,11 @@ func (s *Server) DeselectNetworks(_ context.Context, req *proto.SelectNetworksRe return nil, fmt.Errorf("no route manager") } - routeSelector := routeManager.GetRouteSelector() if req.GetAll() { - routeSelector.DeselectAllRoutes() - } else { - routes := toNetIDs(req.GetNetworkIDs()) - routesMap := routeManager.GetClientRoutesWithNetID() - routes = route.ExpandV6ExitPairs(routes, routesMap) - netIdRoutes := maps.Keys(routesMap) - if err := routeSelector.DeselectRoutes(routes, netIdRoutes); err != nil { - return nil, fmt.Errorf("deselect routes: %w", err) - } + routeManager.DeselectAllRoutes() + } else if err := routeManager.DeselectRoutes(toNetIDs(req.GetNetworkIDs())); err != nil { + return nil, err } - routeManager.TriggerSelection(routeManager.GetClientRoutes()) s.statusRecorder.PublishEvent( proto.SystemEvent_INFO, @@ -261,37 +233,3 @@ 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 deleted file mode 100644 index 1c0ba0ecb..000000000 --- a/client/server/network_exitnode_test.go +++ /dev/null @@ -1,26 +0,0 @@ -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") -}