mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-04 19:45:14 -04:00
## Describe your changes Move route select/deselect handling from the daemon server into exported routemanager methods (SelectRoutes, DeselectRoutes, SelectAllRoutes, DeselectAllRoutes) so every consumer shares one implementation: v4/v6 exit-pair expansion, exit-node mutual exclusion, and selection triggering. Previously the exit-node exclusivity lived only in the daemon's SelectNetworks RPC, so the Android and iOS bindings could leave two exit nodes selected until the next network map reconciliation. Both bindings now call the shared manager methods and enforce exclusivity at toggle time, matching the desktop behavior. ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: https://github.com/netbirdio/docs/pull/__ <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/netbirdio/codesmith/netbird/pr/6928"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1787772098&installation_model_id=427504&pr_number=6928&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6928&signature=31ad59e1483e1582cd447a8db2fe21e5309230e631cbd0cad0f977cd15fb7b9b"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Route selection/deselection is now handled through shared route-manager APIs for both individual routes and “all routes”. * Exit-node selections automatically enforce mutual exclusivity while keeping non-exit routes unaffected. * **Bug Fixes** * Unknown or unavailable route IDs now return errors, and exclusivity is preserved even when some route IDs fail. * **Tests** * Added route-selection tests covering exclusivity (including IPv4/IPv6), select-all behavior, partial errors, and invalid IDs. * **Refactor / Chores** * Simplified Android, iOS, and server routing flows to delegate to the shared manager; updated mocks and removed redundant routing command logic/dependencies. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
130 lines
5.6 KiB
Go
130 lines
5.6 KiB
Go
package routemanager
|
|
|
|
import (
|
|
"net/netip"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
|
|
"github.com/netbirdio/netbird/client/internal/routeselector"
|
|
"github.com/netbirdio/netbird/route"
|
|
)
|
|
|
|
func v6ExitRoute(netID, peer string) *route.Route {
|
|
return &route.Route{
|
|
NetID: route.NetID(netID),
|
|
Network: netip.MustParsePrefix("::/0"),
|
|
Peer: peer,
|
|
}
|
|
}
|
|
|
|
func newSelectionTestManager() *DefaultManager {
|
|
return &DefaultManager{
|
|
routeSelector: routeselector.NewRouteSelector(),
|
|
clientRoutes: route.HAMap{
|
|
"exitA|0.0.0.0/0": {exitRoute("exitA", "p1", true)},
|
|
"exitA-v6|::/0": {v6ExitRoute("exitA-v6", "p1")},
|
|
"exitB|0.0.0.0/0": {exitRoute("exitB", "p2", true)},
|
|
"lan|192.168.1.0/24": {{NetID: "lan", Network: netip.MustParsePrefix("192.168.1.0/24"), Peer: "p3"}},
|
|
},
|
|
}
|
|
}
|
|
|
|
func TestSelectRoutes_ExitNodeExclusivity(t *testing.T) {
|
|
m := newSelectionTestManager()
|
|
|
|
// Selecting an exit node selects its v6 pair and deselects the sibling.
|
|
require.NoError(t, m.selectRoutes([]route.NetID{"exitA"}, true))
|
|
assert.True(t, m.routeSelector.IsSelected("exitA"), "exitA should be selected")
|
|
assert.True(t, m.routeSelector.IsSelected("exitA-v6"), "the v6 pair follows its v4 base")
|
|
assert.False(t, m.routeSelector.IsSelected("exitB"), "the sibling exit node must be deselected")
|
|
|
|
// Switching to the sibling deselects the previous exit node and its v6 pair.
|
|
require.NoError(t, m.selectRoutes([]route.NetID{"exitB"}, true))
|
|
assert.True(t, m.routeSelector.IsSelected("exitB"), "exitB should now be selected")
|
|
assert.False(t, m.routeSelector.IsSelected("exitA"), "the previous exit node must be deselected")
|
|
assert.False(t, m.routeSelector.IsSelected("exitA-v6"), "the previous exit node's v6 pair must be deselected")
|
|
assert.True(t, m.routeSelector.IsSelected("lan"), "non-exit route selection is untouched")
|
|
|
|
// Selecting a non-exit route leaves the active exit node alone.
|
|
require.NoError(t, m.selectRoutes([]route.NetID{"lan"}, true))
|
|
assert.True(t, m.routeSelector.IsSelected("exitB"), "selecting a non-exit route keeps the exit node")
|
|
|
|
// Deselecting the active exit node turns every exit node off.
|
|
require.NoError(t, m.deselectRoutes([]route.NetID{"exitB"}))
|
|
assert.False(t, m.routeSelector.IsSelected("exitB"), "exitB should be deselected")
|
|
assert.False(t, m.routeSelector.IsSelected("exitA"), "exitA stays deselected")
|
|
assert.True(t, m.routeSelector.IsSelected("lan"), "non-exit route selection is untouched")
|
|
}
|
|
|
|
func TestSelectRoutes_PartialErrorStillEnforcesExclusivity(t *testing.T) {
|
|
// 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.
|
|
// Both orderings are covered: processing must continue past the invalid
|
|
// ID wherever it sits in the request.
|
|
requests := map[string][]route.NetID{
|
|
"invalid id first": {"missing", "exitB"},
|
|
"invalid id last": {"exitB", "missing"},
|
|
}
|
|
|
|
for name, ids := range requests {
|
|
t.Run(name, func(t *testing.T) {
|
|
m := newSelectionTestManager()
|
|
|
|
require.NoError(t, m.selectRoutes([]route.NetID{"exitA"}, true))
|
|
|
|
err := m.selectRoutes(ids, 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()
|
|
|
|
assert.Error(t, m.selectRoutes([]route.NetID{"missing"}, true), "selecting an unavailable route must fail")
|
|
assert.Error(t, m.deselectRoutes([]route.NetID{"missing"}), "deselecting an unavailable route must fail")
|
|
}
|
|
|
|
func TestExitNodeSelectionHelpers(t *testing.T) {
|
|
routesMap := map[route.NetID][]*route.Route{
|
|
"exitA": {{Network: netip.MustParsePrefix("0.0.0.0/0")}},
|
|
"exitB": {{Network: netip.MustParsePrefix("::/0")}},
|
|
"lan": {{Network: netip.MustParsePrefix("192.168.0.0/16")}},
|
|
}
|
|
|
|
assert.True(t, requestActivatesExitNode([]route.NetID{"exitA"}, routesMap), "v4 default route is an exit node")
|
|
assert.True(t, requestActivatesExitNode([]route.NetID{"exitB"}, routesMap), "v6 default route is an exit node")
|
|
assert.False(t, requestActivatesExitNode([]route.NetID{"lan"}, routesMap), "lan route is not an exit node")
|
|
assert.False(t, requestActivatesExitNode([]route.NetID{"missing"}, routesMap), "unknown id is not an exit node")
|
|
|
|
others := otherExitNodeIDs(routesMap, []route.NetID{"exitB"})
|
|
assert.ElementsMatch(t, []route.NetID{"exitA"}, others, "only the other exit node is a sibling; the lan route is ignored")
|
|
}
|