[client] Ability to disable default route with Exit Node#5309
Open
Arsolitt wants to merge 31 commits intonetbirdio:mainfrom
Open
[client] Ability to disable default route with Exit Node#5309Arsolitt wants to merge 31 commits intonetbirdio:mainfrom
Arsolitt wants to merge 31 commits intonetbirdio:mainfrom
Conversation
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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) ChangesDisableDefaultRoute propagation
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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
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
Arsolitt
force-pushed
the
feature/disable-default-route
branch
from
48fc631 to
3220165
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/internal/routemanager/manager_test.go (1)
210-239: 💤 Low value
clientNetworkWatchersExpected: 1is dead in this test case.Because
clientNetworkWatchersExpectedAllowedis non-zero (2), the test-runner logic at line 487 unconditionally overridesclientNetworkWatchersExpected, making the1value unobserved and the intent of the field ambiguous for this case.Consider either removing the unused field value or documenting why both differ, to keep the test data self-explanatory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/routemanager/manager_test.go` around lines 210 - 239, The test case sets clientNetworkWatchersExpected to 1 but the test-runner logic overrides it when clientNetworkWatchersExpectedAllowed is non-zero, making that field dead; update the test case in manager_test.go by either removing the unused clientNetworkWatchersExpected entry, setting clientNetworkWatchersExpected to match clientNetworkWatchersExpectedAllowed, or add a one-line comment next to the fields explaining why they differ so the intent is explicit (refer to the test case struct literal and the test-runner logic that uses clientNetworkWatchersExpectedAllowed).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/internal/routemanager/manager_test.go`:
- Around line 540-547: The test currently conditionally checks the Get result so
a false negative can silently pass; change the assertion to unconditionally
require that mgr.routeRefCounter.Get(normalPrefix) returns ok==true and then
assert ref.Count == 1 (and also ensure the Increment call's err is
checked/handled) so the test fails if the non-default prefix is not tracked;
refer to routeRefCounter.Increment, normalPrefix, routeRefCounter.Get and the
require.Equal assertion for where to update the test.
---
Nitpick comments:
In `@client/internal/routemanager/manager_test.go`:
- Around line 210-239: The test case sets clientNetworkWatchersExpected to 1 but
the test-runner logic overrides it when clientNetworkWatchersExpectedAllowed is
non-zero, making that field dead; update the test case in manager_test.go by
either removing the unused clientNetworkWatchersExpected entry, setting
clientNetworkWatchersExpected to match clientNetworkWatchersExpectedAllowed, or
add a one-line comment next to the fields explaining why they differ so the
intent is explicit (refer to the test case struct literal and the test-runner
logic that uses clientNetworkWatchersExpectedAllowed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38323ef8-b8e1-495c-9725-70bd65528721
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.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 (6)
- client/internal/engine.go
- client/cmd/up.go
- client/internal/profilemanager/config.go
- client/internal/connect.go
- client/internal/routemanager/manager.go
- client/server/server.go
Collaborator
|
@Arsolitt the intent is clear, but out of curiosity, can you share a bit more around the use case you have now? |
Author
hi, i use sing-box for more flexible routing, it can send traffic to any network interface on host sing-box TUN inbound pushes everything into sing-box tun interface, and from there it routes some of the traffic through an exit node in netbird based on my own rules. as a workaround i can either run an infinite loop in a bash script that keeps deleting default route through netbird, or maintain my own builds for all platforms |
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.
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
The Login handler does not read Disable* fields from LoginRequest - config is applied via SetConfig instead. Remove the dead field to avoid confusion. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Make non-default route assertion unconditional to prevent silently skipping verification when AddVPNRoute fails due to missing network interface. Signed-off-by: Arsolitt <arsolitt@gmail.com>
When ErrIgnore is returned by the add function, the refcounter never stores the key, so decrement early-returns before calling remove. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
…efcounter Use the manager's real routeRefCounter (created by setupRefCounters) instead of replacing it with a test double. This ensures the test actually validates the production wiring in manager.go: if the disableDefaultRoute guard were removed, the default routes would no longer return ErrIgnore and the refcounter presence assertions would catch the regression. Also extend coverage to IPv6 default route (::/0) alongside IPv4 (0.0.0.0/0), matching the dual-check in production code. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
…ilable Signed-off-by: Arsolitt <arsolitt@gmail.com>
…te config Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Move noop check before real counter creation so the disableDefaultRoute closure is only allocated when it will actually be used. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Replace WireGuard-dependent test with spy-based unit test that verifies the refcounter filtering logic for both IPv4 and IPv6 default routes without requiring elevated privileges. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
…oute Signed-off-by: Arsolitt <arsolitt@gmail.com>
TestDisableDefaultRouteWatcherCreation verifies that with DisableDefaultRoute=true, UpdateRoutes creates watchers in clientNetworks for all routes (default + normal) while the default route is excluded from the system routing table via ErrIgnore. Uses noop ref-counters and constructs DefaultManager fields directly (same package), so the test runs without a real WireGuard interface and is safe in unprivileged CI. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
…dateRoutes The behavior is fully covered by dedicated tests that run without elevated privileges: TestDisableDefaultRouteSkipsSystemRoute and TestDisableDefaultRouteWatcherCreation. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Arsolitt
force-pushed
the
feature/disable-default-route
branch
from
626d6a7 to
abf0baf
Compare
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Arsolitt
force-pushed
the
feature/disable-default-route
branch
from
abf0baf to
6bba1b1
Compare
Split the monolithic apply method into focused helper methods grouped by concern: network, routes, server, interface, and credentials. No behavior changes. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Extract repeated field-update patterns into generic helpers to bring all methods under the SonarQube complexity limit of 20. Signed-off-by: Arsolitt <arsolitt@gmail.com>
|
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
3 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