[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.
This commit is contained in:
Zoltán Papp
2026-07-27 22:03:20 +02:00
parent cc0ab3247d
commit 7e1b35037a
2 changed files with 58 additions and 4 deletions

View File

@@ -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 {

View File

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