mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-01 04:21:29 +02:00
[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.
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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")}},
|
||||
|
||||
Reference in New Issue
Block a user