[client] Ability to disable default route with Exit Node#5309
Open
Arsolitt wants to merge 6 commits intonetbirdio:mainfrom
Open
[client] Ability to disable default route with Exit Node#5309Arsolitt wants to merge 6 commits intonetbirdio:mainfrom
Arsolitt wants to merge 6 commits intonetbirdio:mainfrom
Conversation
|
|
Contributor
📝 WalkthroughWalkthroughAdds a new disable-default-route flag and boolean that flow CLI → proto/server → profile config → engine → route manager; when enabled, default IPv4/IPv6 system routes are skipped during add/remove operations. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Server as Server (daemon)
participant Profile as ProfileManager
participant Engine as Engine
participant RouteMgr as RouteManager
CLI->>Server: Send SetConfigRequest (disable_default_route)
Server->>Profile: apply SetConfig (config.DisableDefaultRoute = msg.DisableDefaultRoute)
Profile->>Engine: create EngineConfig (propagate DisableDefaultRoute)
Engine->>RouteMgr: NewManager / ManagerConfig (DisableDefaultRoute)
RouteMgr->>RouteMgr: setupRefCounters: if DisableDefaultRoute -> skip default routes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/proto/daemon.proto`:
- Around line 673-674: The proto adds the new field disable_default_route but it
was not added to the LoginRequest and GetConfigResponse messages nor propagated
in setupLoginRequest, so the value won't persist or be queryable; add an
optional bool disable_default_route field to both LoginRequest and
GetConfigResponse (choosing a unique tag number consistent with nearby fields),
update setupLoginRequest in client/cmd/up.go to copy the local
disable_default_route setting into the LoginRequest like the other disable_*
flags, regenerate protobuf stubs, and ensure server-side code that builds
GetConfigResponse populates disable_default_route from the server config/state.
🧹 Nitpick comments (1)
client/internal/routemanager/manager_test.go (1)
209-228: Test validates watcher creation but not route-skipping behavior.The test only asserts
require.Len(t, routeManager.clientNetworks, expectedWatchers), confirming a watcher is created. It does not verify the core feature: whendisableDefaultRouteis true, the default route is excluded from the system routing table via the route ref counter'sErrIgnorepath (see manager.go lines 190-193).Add assertions that verify the default route is skipped from system route addition, or document that this behavior is covered by integration tests.
Arsolitt
force-pushed
the
feature/disable-default-route
branch
from
9b0c2e3 to
60a55a1
Compare
This was referenced Feb 14, 2026
Add optional bool disable_default_route field (tag 35) to the SetConfigRequest protobuf message and wire it through the daemon server handler to ConfigInput.
Add DisableDefaultRoute field to ConfigInput, Config, EngineConfig structs and wire it through the config update/create logic and engine config mapping.
Register the --disable-default-route flag and implement filtering in the route ref counter to skip adding default route (0.0.0.0/0) to the system routing table while preserving WireGuard allowed IPs.
Update setconfig_test.go to cover the new DisableDefaultRoute field in SetConfigRequest (AllFieldsSaved, verifyAllFieldsCovered, and CLIFlags mapping). Add a test case to TestManagerUpdateRoutes verifying that the default route watcher is still created when the flag is set.
The field was missing from LoginRequest (used during setup key login) and GetConfigResponse (used by UI and status queries), causing the flag to not persist in daemon mode.
Add TestDisableDefaultRouteSkipsSystemRoute that directly asserts the route ref counter behavior: default route (0.0.0.0/0) is not tracked in the ref counter (ErrIgnore path) while non-default routes are tracked normally. Also update the table-driven test case to include a non-default route alongside the default one to verify selective filtering.
Arsolitt
force-pushed
the
feature/disable-default-route
branch
from
f247332 to
48fc631
Compare
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/cmd/system.go`:
- Around line 32-34: Update the flag help text for
upCmd.PersistentFlags().BoolVar that uses disableDefaultRoute and
disableDefaultRouteFlag to explicitly mention both IPv4 and IPv6 default routes
(e.g. "Disable adding the IPv4 and IPv6 default routes (0.0.0.0/0 and ::/0) to
the system routing table while keeping them in WireGuard allowed IPs.") so users
understand the flag affects both families.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
client/cmd/system.goclient/cmd/up.goclient/internal/connect.goclient/internal/engine.goclient/internal/profilemanager/config.goclient/internal/routemanager/manager.goclient/internal/routemanager/manager_test.goclient/proto/daemon.protoclient/server/server.goclient/server/setconfig_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/cmd/up.go
- client/proto/daemon.proto
- client/internal/engine.go
Comment on lines
+32
to
+34
| upCmd.PersistentFlags().BoolVar(&disableDefaultRoute, disableDefaultRouteFlag, false, | ||
| "Disable adding default route (0.0.0.0/0) to the system routing table while keeping it in WireGuard allowed IPs.") | ||
|
|
Contributor
There was a problem hiding this comment.
Help text should mention IPv6 default route too.
The behavior skips both IPv4 and IPv6 defaults, but the flag description only cites 0.0.0.0/0. Consider clarifying to avoid user confusion.
Suggested text update
- upCmd.PersistentFlags().BoolVar(&disableDefaultRoute, disableDefaultRouteFlag, false,
- "Disable adding default route (0.0.0.0/0) to the system routing table while keeping it in WireGuard allowed IPs.")
+ upCmd.PersistentFlags().BoolVar(&disableDefaultRoute, disableDefaultRouteFlag, false,
+ "Disable adding default routes (0.0.0.0/0, ::/0) to the system routing table while keeping them in WireGuard allowed IPs.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| upCmd.PersistentFlags().BoolVar(&disableDefaultRoute, disableDefaultRouteFlag, false, | |
| "Disable adding default route (0.0.0.0/0) to the system routing table while keeping it in WireGuard allowed IPs.") | |
| upCmd.PersistentFlags().BoolVar(&disableDefaultRoute, disableDefaultRouteFlag, false, | |
| "Disable adding default routes (0.0.0.0/0, ::/0) to the system routing table while keeping them in WireGuard allowed IPs.") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/cmd/system.go` around lines 32 - 34, Update the flag help text for
upCmd.PersistentFlags().BoolVar that uses disableDefaultRoute and
disableDefaultRouteFlag to explicitly mention both IPv4 and IPv6 default routes
(e.g. "Disable adding the IPv4 and IPv6 default routes (0.0.0.0/0 and ::/0) to
the system routing table while keeping them in WireGuard allowed IPs.") so users
understand the flag affects both families.
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
This PR introduces the ability to prevent the installation of the default route (0.0.0.0/0) into the system routing table while preserving the WireGuard AllowedIPs configuration by passing
--disable-default-routeCLI arg.This allows enabling an Exit Node on the peer without automatically routing all host traffic through the tunnel, enabling custom routing management outside netbird.
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:
netbirdio/docs#602
Summary by CodeRabbit
New Features
Tests