From 4a518b2868c2b17513e9faea7a501cb8a0a883f5 Mon Sep 17 00:00:00 2001 From: Maxim Egorov Date: Fri, 28 Aug 2026 12:45:38 +0200 Subject: [PATCH] [client] Apply the route selection even when some IDs are unknown SelectRoutes and DeselectRoutes returned the error before TriggerSelection, so a request mixing valid and unknown network IDs changed the selector but never reached the routing table. The valid routes read as selected while `ip route` showed nothing. Trigger the selection first and return the error afterwards. The inner selectRoutes already applied the valid part of a partial request, only the outer layer dropped it. --- client/internal/routemanager/selection.go | 25 +++++--- .../internal/routemanager/selection_test.go | 59 +++++++++++++++++++ 2 files changed, 75 insertions(+), 9 deletions(-) diff --git a/client/internal/routemanager/selection.go b/client/internal/routemanager/selection.go index 6d5feec79..b81d51b67 100644 --- a/client/internal/routemanager/selection.go +++ b/client/internal/routemanager/selection.go @@ -17,23 +17,30 @@ import ( // 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. +// A partial failure (e.g. an unknown ID mixed with valid ones) still applies +// the valid IDs to the routing table; the unknown ones are reported in the +// returned error. func (m *DefaultManager) SelectRoutes(ids []route.NetID, appendRoute bool) error { - if err := m.selectRoutes(ids, appendRoute); err != nil { - return err - } + err := m.selectRoutes(ids, appendRoute) + // Apply regardless of err: selectRoutes already selects the valid part of a + // partial request, and skipping this on error would leave those routes + // selected in the selector but never installed in the routing table. m.TriggerSelection(m.GetClientRoutes()) - return nil + return err } // DeselectRoutes removes the routes with the given network IDs from the // selection and applies the change. V4/v6 exit-node pairs are expanded -// automatically. +// automatically. A partial failure (e.g. an unknown ID mixed with valid ones) +// still applies the valid IDs to the routing table; the unknown ones are +// reported in the returned error. func (m *DefaultManager) DeselectRoutes(ids []route.NetID) error { - if err := m.deselectRoutes(ids); err != nil { - return err - } + err := m.deselectRoutes(ids) + // Apply regardless of err: deselectRoutes already deselects the valid part + // of a partial request, and skipping this on error would leave those routes + // installed in the routing table despite being marked deselected. m.TriggerSelection(m.GetClientRoutes()) - return nil + return err } func (m *DefaultManager) deselectRoutes(ids []route.NetID) error { diff --git a/client/internal/routemanager/selection_test.go b/client/internal/routemanager/selection_test.go index 6066b5661..3db6bbfe6 100644 --- a/client/internal/routemanager/selection_test.go +++ b/client/internal/routemanager/selection_test.go @@ -1,12 +1,16 @@ package routemanager import ( + "context" "net/netip" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/netbirdio/netbird/client/internal/peer" + "github.com/netbirdio/netbird/client/internal/routemanager/client" + "github.com/netbirdio/netbird/client/internal/routemanager/notifier" "github.com/netbirdio/netbird/client/internal/routeselector" "github.com/netbirdio/netbird/route" ) @@ -112,6 +116,61 @@ func TestSelectRoutes_UnknownRoute(t *testing.T) { assert.Error(t, m.deselectRoutes([]route.NetID{"missing"}), "deselecting an unavailable route must fail") } +// newPartialFailureTestManager builds a manager whose TriggerSelection exercises the +// real install/remove path (client.HandlerFromRoute plus the route refcounter) without +// touching the system: setupRefCounters(true) swaps in the noop refcounter, and +// clientNetworks is pre-seeded for every route so no watcher goroutine is ever started. +func newPartialFailureTestManager() *DefaultManager { + ctx := context.Background() + + m := &DefaultManager{ + ctx: ctx, + clientRoutes: route.HAMap{ + "lan|192.168.1.0/24": {{NetID: "lan", Network: netip.MustParsePrefix("192.168.1.0/24"), Peer: "p1"}}, + "other|10.1.2.0/24": {{NetID: "other", Network: netip.MustParsePrefix("10.1.2.0/24"), Peer: "p2"}}, + }, + routeSelector: routeselector.NewRouteSelector(), + notifier: notifier.NewNotifier(), + statusRecorder: peer.NewRecorder("https://mgm"), + activeRoutes: make(map[route.HAUniqueID]client.RouteHandler), + clientNetworks: map[route.HAUniqueID]*client.Watcher{ + "lan|192.168.1.0/24": client.NewWatcher(client.WatcherConfig{Context: ctx}), + "other|10.1.2.0/24": client.NewWatcher(client.WatcherConfig{Context: ctx}), + }, + } + m.setupRefCounters(true) + return m +} + +// Regression for the reported symptom: a partial failure returned before +// TriggerSelection ran, so the valid route was marked selected while never +// reaching the routing table (activeRoutes/ip route). +func TestSelectRoutes_PartialFailureStillInstallsValidRoute(t *testing.T) { + m := newPartialFailureTestManager() + + err := m.SelectRoutes([]route.NetID{"missing", "lan"}, false) + + assert.Error(t, err, "the unknown id must still be reported") + assert.Contains(t, m.activeRoutes, route.HAUniqueID("lan|192.168.1.0/24"), "the valid route must be installed despite the error") + assert.NotContains(t, m.activeRoutes, route.HAUniqueID("other|10.1.2.0/24"), "the deselected route must not be installed") +} + +// Mirror of the case above: a partial failure must remove the valid route from +// the routing table, not just mark it deselected in the selector. +func TestDeselectRoutes_PartialFailureStillRemovesValidRoute(t *testing.T) { + m := newPartialFailureTestManager() + + require.NoError(t, m.SelectRoutes([]route.NetID{"lan", "other"}, false)) + require.Contains(t, m.activeRoutes, route.HAUniqueID("lan|192.168.1.0/24")) + require.Contains(t, m.activeRoutes, route.HAUniqueID("other|10.1.2.0/24")) + + err := m.DeselectRoutes([]route.NetID{"missing", "other"}) + + assert.Error(t, err, "the unknown id must still be reported") + assert.NotContains(t, m.activeRoutes, route.HAUniqueID("other|10.1.2.0/24"), "the deselected route must be removed") + assert.Contains(t, m.activeRoutes, route.HAUniqueID("lan|192.168.1.0/24"), "the untouched route stays installed") +} + func TestExitNodeSelectionHelpers(t *testing.T) { routesMap := map[route.NetID][]*route.Route{ "exitA": {{Network: netip.MustParsePrefix("0.0.0.0/0")}},