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()