From 7e1b35037a7a1f588d365ac4f0ab95a42f3bd480 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Mon, 27 Jul 2026 22:03:20 +0200 Subject: [PATCH] [client] Keep exit-node exclusivity on select-all and partial errors Selecting all routes wipes every explicit selection, so exit nodes fall back to management's auto-apply flags, which may mark several at once. Reconcile immediately after select-all so at most one stays active instead of waiting for the next network map. A partial selection failure (e.g. an unknown ID in the request) still selects the valid routes, so run the sibling exit-node deselection regardless of the error and report both failures together. --- client/internal/routemanager/selection.go | 26 +++++++++++--- .../internal/routemanager/selection_test.go | 36 +++++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/client/internal/routemanager/selection.go b/client/internal/routemanager/selection.go index 8320c2ac5..6d5feec79 100644 --- a/client/internal/routemanager/selection.go +++ b/client/internal/routemanager/selection.go @@ -4,9 +4,11 @@ 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" ) @@ -48,11 +50,24 @@ func (m *DefaultManager) deselectRoutes(ids []route.NetID) error { } // SelectAllRoutes selects every available route and applies the selection. +// Exit nodes stay mutually exclusive: at most one remains active. func (m *DefaultManager) SelectAllRoutes() { - m.routeSelector.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() @@ -66,8 +81,11 @@ func (m *DefaultManager) selectRoutes(ids []route.NetID, appendRoute bool) error 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 { - return fmt.Errorf("select routes: %w", err) + merr = multierror.Append(merr, fmt.Errorf("select routes: %w", err)) } // Exit nodes are mutually exclusive: if this selection activates an @@ -76,12 +94,12 @@ func (m *DefaultManager) selectRoutes(ids []route.NetID, appendRoute bool) error if requestActivatesExitNode(routes, routesMap) { if others := otherExitNodeIDs(routesMap, routes); len(others) > 0 { if err := m.routeSelector.DeselectRoutes(others, allIDs); err != nil { - return fmt.Errorf("deselect sibling exit nodes: %w", err) + merr = multierror.Append(merr, fmt.Errorf("deselect sibling exit nodes: %w", err)) } } } - return nil + return nberrors.FormatErrorOrNil(merr) } func isExitNodeRoutes(routes []*route.Route) bool { diff --git a/client/internal/routemanager/selection_test.go b/client/internal/routemanager/selection_test.go index f76ecce80..3dd592315 100644 --- a/client/internal/routemanager/selection_test.go +++ b/client/internal/routemanager/selection_test.go @@ -58,6 +58,42 @@ func TestSelectRoutes_ExitNodeExclusivity(t *testing.T) { assert.True(t, m.routeSelector.IsSelected("lan"), "non-exit route selection is untouched") } +func TestSelectRoutes_PartialErrorStillEnforcesExclusivity(t *testing.T) { + m := newSelectionTestManager() + + require.NoError(t, m.selectRoutes([]route.NetID{"exitA"}, true)) + + // 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. + err := m.selectRoutes([]route.NetID{"exitB", "missing"}, 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()