[client] Add GetSelectedClientRoutes to route manager and update DNS route check#5802
Merged
[client] Add GetSelectedClientRoutes to route manager and update DNS route check#5802
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change makes the engine's DNS route checker use only selected client routes (via Changes
Sequence Diagram(s)sequenceDiagram
participant Engine as Engine
participant RM as RouteManager
participant RS as RouteSelector
participant DNS as DNSChecker
Engine->>RM: GetSelectedClientRoutes()
RM->>RM: lock & clone clientRoutes
RM->>RS: FilterSelectedExitNodes(cloned clientRoutes)
RS-->>RM: filtered routes (selected only)
RM-->>Engine: return selected client routes
Engine->>DNS: SetRouteChecker(using selected routes)
DNS-->>Engine: DNS queries checked against selected routes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lixmal
previously approved these changes
Apr 5, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/internal/routemanager/mock.go (1)
72-78: Consider adding a dedicatedGetSelectedClientRoutesFuncfor better test isolation.Both
GetClientRoutesandGetSelectedClientRoutesuse the sameGetClientRoutesFunc, making it impossible to test scenarios where they should return different values (e.g., testing behavior when some routes are deselected).♻️ Suggested improvement
type MockManager struct { ClassifyRoutesFunc func(routes []*route.Route) (map[route.ID]*route.Route, route.HAMap) UpdateRoutesFunc func(updateSerial uint64, serverRoutes map[route.ID]*route.Route, clientRoutes route.HAMap, useNewDNSRoute bool) error TriggerSelectionFunc func(haMap route.HAMap) GetRouteSelectorFunc func() *routeselector.RouteSelector GetClientRoutesFunc func() route.HAMap + GetSelectedClientRoutesFunc func() route.HAMap GetClientRoutesWithNetIDFunc func() map[route.NetID][]*route.Route StopFunc func(manager *statemanager.Manager) }// GetSelectedClientRoutes mock implementation of GetSelectedClientRoutes from Manager interface func (m *MockManager) GetSelectedClientRoutes() route.HAMap { - if m.GetClientRoutesFunc != nil { - return m.GetClientRoutesFunc() + if m.GetSelectedClientRoutesFunc != nil { + return m.GetSelectedClientRoutesFunc() } - return nil + // Fall back to GetClientRoutesFunc for backward compatibility + if m.GetClientRoutesFunc != nil { + return m.GetClientRoutesFunc() + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/routemanager/mock.go` around lines 72 - 78, The mock currently shares GetClientRoutesFunc for both behaviors which prevents testing different return values; add a new field GetSelectedClientRoutesFunc to MockManager and update the GetSelectedClientRoutes method to call GetSelectedClientRoutesFunc when non-nil (falling back to GetClientRoutesFunc or nil if desired), ensuring GetClientRoutes continues to call GetClientRoutesFunc so tests can independently stub selected vs all routes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/internal/routemanager/mock.go`:
- Around line 72-78: The mock currently shares GetClientRoutesFunc for both
behaviors which prevents testing different return values; add a new field
GetSelectedClientRoutesFunc to MockManager and update the
GetSelectedClientRoutes method to call GetSelectedClientRoutesFunc when non-nil
(falling back to GetClientRoutesFunc or nil if desired), ensuring
GetClientRoutes continues to call GetClientRoutesFunc so tests can independently
stub selected vs all routes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3f51b6a-3cf7-4881-a818-672bc09a42a8
📒 Files selected for processing (3)
client/internal/engine.goclient/internal/routemanager/manager.goclient/internal/routemanager/mock.go
|
lixmal
approved these changes
Apr 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
queries
Problem
When an exit node (0.0.0.0/0) was deselected via the iOS UI, DNS resolution stopped working. All queries went through the tunnel but couldn't resolve.
Root cause: The DNS upstream resolver in engine.go called GetClientRoutes() to determine if an upstream DNS server (e.g., 8.8.8.8) should be reached through the WireGuard
tunnel. GetClientRoutes() returns all routes from the management server, including deselected ones. Since the deselected exit node's 0.0.0.0/0 matched every IP, the resolver
bound DNS sockets to the utun interface via IP_BOUND_IF — but with no WireGuard peer to forward through, every query failed.
Fix
Added GetSelectedClientRoutes() to the route manager interface, which filters routes through FilterSelectedExitNodes before returning them. The DNS route checker in engine.go
now uses this method instead of GetClientRoutes().
Verified by logs
Before fix (after deselecting exit node):
routeChecker: ip=8.8.8.8 matched by route network=0.0.0.0/0 (netID=Exit Node)
DNS exchange: upstream=8.8.8.8:53 needsPrivate=true ← bound to utun, no peer, fails
After fix:
routeChecker: ip=8.8.8.8 not matched by any selected client route
DNS exchange: upstream=8.8.8.8:53 needsPrivate=false ← regular socket, resolves
Test plan
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
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/__
Summary by CodeRabbit
Bug Fixes
Chores