[client] Replace exclusion routes with scoped default + IP_BOUND_IF on macOS#5918
Merged
[client] Replace exclusion routes with scoped default + IP_BOUND_IF on macOS#5918
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 Darwin advanced-routing support (scoped default routes, route add/delete, bound-interface cache and socket binding), BSD no-op stubs, AF_ROUTE read/write refactor with retries/deadline, build-tag adjustments for net/env and init hooks for Dialer/Listener, and removes routing-separation helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Init as Dialer/Listener init
participant Setup as SetupRouting
participant Route as AF_ROUTE subsystem
participant Cache as BoundInterface Cache
participant Socket as Socket Control
App->>Init: start
Init->>Init: Darwin init() sets Control callback (applyBoundIfToSocket)
Init->>Socket: register Control callback
App->>Setup: SetupRouting(advancedRouting=true)
Setup->>Route: flushScopedDefaults()
Route-->>Setup: residual scoped routes removed
loop per AF (IPv4, IPv6)
Setup->>Route: resolve physical default nexthop
Route-->>Setup: nexthop + iface
Setup->>Route: addScopedDefault(unspec, nexthop) via AF_ROUTE RTM_ADD
Setup->>Cache: SetBoundInterface(af, iface)
end
App->>Socket: create connection
Socket->>Cache: choose bound interface (AF or IPv6 zone)
Socket->>Socket: set IP_BOUND_IF / IPV6_BOUND_IF via Control -> setsockopt
Socket-->>App: socket configured
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)✅ Unit Test PR creation complete.
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
force-pushed
the
macos-ip-bound-if
branch
2 times, most recently
from
04dec53 to
24c8183
Compare
![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
🧹 Nitpick comments (3)
client/internal/routemanager/systemops/systemops_bsd_other.go (1)
5-9: Return an explicit unsupported error instead of silently succeeding to detect misconfiguration, or keep the defensive nil stubs consistent with other platforms (Android, iOS, Windows, JS).These stubs are unreachable in practice—non-Darwin BSDs always have
advancedRouting=false(sourced fromenv_generic.go), sosetupAdvancedRouting()andcleanupAdvancedRouting()are never called. The suggestion to return an error would be consistent with defensive programming practices in the codebase (similar to theAddOutputDNATguard), but conflicts with how Windows, Android, and iOS handle unsupported operations (which return nil). The current approach is correct and consistent; any change should align with platform patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/routemanager/systemops/systemops_bsd_other.go` around lines 5 - 9, These stubs should remain defensive no-ops to match other unsupported platforms (Android/iOS/Windows/JS) rather than returning an error; keep SysOps.setupAdvancedRouting and SysOps.cleanupAdvancedRouting returning nil, but add/clarify a brief comment that they are intentionally no-ops because non-Darwin BSDs set advancedRouting=false (see env_generic.go) and the methods are only present so systemops_unix.go compiles.client/net/net_darwin.go (2)
41-49:afas a bareintis brittle.
SetBoundInterfacetreats anyaf != 6as IPv4. If a caller ever passessyscall.AF_INET6(30 on Darwin) orunix.AF_INET6by mistake, the v6 interface will be silently stored as v4. Today the only caller passes literal4/6, but the signature invites misuse. Consider either a dedicated enum/type, branching onnetip.Addr.Is6(), or at least rejecting unknown values:♻️ Proposed tightening
-func SetBoundInterface(af int, iface *net.Interface) { - boundIfaceMu.Lock() - defer boundIfaceMu.Unlock() - if af == 6 { - boundIface6 = iface - } else { - boundIface4 = iface - } -} +func SetBoundInterface(af int, iface *net.Interface) { + boundIfaceMu.Lock() + defer boundIfaceMu.Unlock() + switch af { + case 4: + boundIface4 = iface + case 6: + boundIface6 = iface + default: + log.Warnf("SetBoundInterface: ignoring unknown address family %d", af) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/net/net_darwin.go` around lines 41 - 49, SetBoundInterface currently accepts a bare int and treats any af != 6 as IPv4, which is brittle; update SetBoundInterface to validate the address family explicitly (referencing SetBoundInterface, boundIface6, boundIface4, boundIfaceMu) by switching on known constants (e.g., syscall.AF_INET / syscall.AF_INET6 or unix.AF_INET / unix.AF_INET6) or by introducing a small addressFamily type/enum, set boundIface6 for the IPv6 constant and boundIface4 for the IPv4 constant, and handle unknown values by returning early and logging or returning an error instead of silently treating them as IPv4.
16-26: Useunix.IP_BOUND_IFandunix.IPV6_BOUND_IFfrom the importedgolang.org/x/sys/unixpackage.The constants are already defined and available in the unix package (as confirmed by
upstream_ios.gousingunix.IP_BOUND_IF). Redeclaring them locally duplicates platform-specific knowledge and creates unnecessary drift risk. Remove the local const block and use the unix package constants directly in the function calls at lines 103 and 110.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/net/net_darwin.go` around lines 16 - 26, Remove the local const block that defines IP_BOUND_IF and IPV6_BOUND_IF and replace all usages with the constants from the unix package (use unix.IP_BOUND_IF and unix.IPV6_BOUND_IF where the code currently refers to IP_BOUND_IF and IPV6_BOUND_IF, e.g. the setsockopt calls around the current uses at lines ~103 and ~110). Ensure the file uses the already-imported "golang.org/x/sys/unix" symbols and delete the duplicate local declarations to avoid platform drift.
🤖 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/internal/routemanager/systemops/systemops_darwin.go`:
- Around line 217-263: The current readRouteResponse can misattribute unrelated
AF_ROUTE broadcasts; change the flow so routeMessageRoundtrip passes the
request's identifying fields (msg.Type and msg.Seq) and the current pid
(os.Getpid()) into readRouteResponse (or add parameters like expectedType,
expectedSeq, expectedPid), then have readRouteResponse loop reading from fd
until it finds a header where hdr.Type == expectedType && hdr.Seq == expectedSeq
&& hdr.Pid == expectedPid; ignore non-matching messages (continue), treat
matching headers with hdr.Errno != 0 as a permanent error and hdr.Errno == 0 as
success, and handle short reads (< unsafe.Sizeof(unix.RtMsghdr{})) by
continuing; also set a receive timeout on the socket (unix.SetsockoptTimeval
with SO_RCVTIMEO) before writes/reads so the loop can't block forever.
---
Nitpick comments:
In `@client/internal/routemanager/systemops/systemops_bsd_other.go`:
- Around line 5-9: These stubs should remain defensive no-ops to match other
unsupported platforms (Android/iOS/Windows/JS) rather than returning an error;
keep SysOps.setupAdvancedRouting and SysOps.cleanupAdvancedRouting returning
nil, but add/clarify a brief comment that they are intentionally no-ops because
non-Darwin BSDs set advancedRouting=false (see env_generic.go) and the methods
are only present so systemops_unix.go compiles.
In `@client/net/net_darwin.go`:
- Around line 41-49: SetBoundInterface currently accepts a bare int and treats
any af != 6 as IPv4, which is brittle; update SetBoundInterface to validate the
address family explicitly (referencing SetBoundInterface, boundIface6,
boundIface4, boundIfaceMu) by switching on known constants (e.g.,
syscall.AF_INET / syscall.AF_INET6 or unix.AF_INET / unix.AF_INET6) or by
introducing a small addressFamily type/enum, set boundIface6 for the IPv6
constant and boundIface4 for the IPv4 constant, and handle unknown values by
returning early and logging or returning an error instead of silently treating
them as IPv4.
- Around line 16-26: Remove the local const block that defines IP_BOUND_IF and
IPV6_BOUND_IF and replace all usages with the constants from the unix package
(use unix.IP_BOUND_IF and unix.IPV6_BOUND_IF where the code currently refers to
IP_BOUND_IF and IPV6_BOUND_IF, e.g. the setsockopt calls around the current uses
at lines ~103 and ~110). Ensure the file uses the already-imported
"golang.org/x/sys/unix" symbols and delete the duplicate local declarations to
avoid platform drift.
🪄 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: 24935800-e9da-4ce2-8821-d0fab5f258c8
📒 Files selected for processing (10)
client/internal/routemanager/systemops/systemops_bsd_other.goclient/internal/routemanager/systemops/systemops_darwin.goclient/internal/routemanager/systemops/systemops_unix.goclient/net/dialer_init_darwin.goclient/net/dialer_init_generic.goclient/net/env_darwin.goclient/net/env_generic.goclient/net/listener_init_darwin.goclient/net/listener_init_generic.goclient/net/net_darwin.go
lixmal
force-pushed
the
macos-ip-bound-if
branch
from
24c8183 to
79e4958
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/internal/routemanager/systemops/systemops_darwin.go`:
- Around line 75-78: The current check in systemops_darwin.go rejects nexthop
entries that have an interface but no gateway IP, breaking
point-to-point/interface-scoped defaults; change the logic in the failing branch
so it only rejects when nexthop.Intf is nil (i.e., allow nexthop with Intf set
and an empty/invalid nexthop.IP), update the error message accordingly, and
ensure the rest of the flow (setupAdvancedRouting and buildRouteMessage) will
treat an iface-only nexthop by emitting RTAX_GATEWAY as a LinkAddr as done in
buildRouteMessage (systemops_unix.go) so the scoped default can be installed.
🪄 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: 25ed117a-92d9-4af3-a6d7-d522de3da2ea
📒 Files selected for processing (10)
client/internal/routemanager/systemops/systemops_bsd_other.goclient/internal/routemanager/systemops/systemops_darwin.goclient/internal/routemanager/systemops/systemops_unix.goclient/net/dialer_init_darwin.goclient/net/dialer_init_generic.goclient/net/env_darwin.goclient/net/env_generic.goclient/net/listener_init_darwin.goclient/net/listener_init_generic.goclient/net/net_darwin.go
✅ Files skipped from review due to trivial changes (4)
- client/net/dialer_init_generic.go
- client/net/listener_init_generic.go
- client/net/env_generic.go
- client/internal/routemanager/systemops/systemops_bsd_other.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/net/dialer_init_darwin.go
- client/net/listener_init_darwin.go
- client/net/env_darwin.go
lixmal
force-pushed
the
macos-ip-bound-if
branch
from
79e4958 to
40e8020
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
client/net/net_darwin.go (2)
116-139: LGTM — optional: avoid recomputing the family suffix.The control callback is correct: it’s gated on
AdvancedRouting(), falls through silently when no interface is cached, and propagates both theRawConn.Controlerror and the innersetsockopterror. The IPv6/IPv4 branch decision here is also consistent withboundInterfaceFor(both usestrings.HasSuffix(network, "6")), and the top-of-file comment correctly documents why IPv6 sockets handle v4-mapped egress and whyIP_BOUND_IFon AF_INET6 would EINVAL.Tiny optional cleanup: compute
isV6 := strings.HasSuffix(network, "6")once and pass it toboundInterfaceFor/the sockopt branch to avoid doing the same suffix check twice. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/net/net_darwin.go` around lines 116 - 139, The code recomputes whether the network is IPv6 twice using strings.HasSuffix(network, "6"); simplify by computing a boolean once (e.g., isV6 := strings.HasSuffix(network, "6")) at the top of applyBoundIfToSocket and reuse it for both the call to boundInterfaceFor(...) (add an overload or pass the boolean if you prefer) and the sockopt branch to call setIPv6BoundIf or setIPv4BoundIf, leaving all existing error handling intact and keeping the same log message and behavior.
36-47: Consider logging or no-oping nil iface explicitly.
SetBoundInterface(af, nil)currently clears the cached slot silently (same effect asClearBoundInterfacesbut per family). That seems usable as an API (e.g., clearing just v6), but it’s undocumented — the doc comment says “records the egress interface.” Either:
- document that
nilclears the slot, or- reject
nilwith a warn log to mirror the default branch.Minor/nit — not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/net/net_darwin.go` around lines 36 - 47, SetBoundInterface currently treats a nil iface as a silent clear; make nil handling explicit by rejecting nil inputs with a warning and no-op to mirror the unsupported-family branch: inside SetBoundInterface, check if iface == nil and call log.Warnf("SetBoundInterface: nil iface for AF %d, no-op", af) then return without modifying boundIface4/boundIface6; reference SetBoundInterface, boundIface4, boundIface6 and the default log.Warnf so reviewers can find the spot to change (optionally update the doc comment on "records the egress interface" to state nil is ignored).
🤖 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/net/net_darwin.go`:
- Around line 116-139: The code recomputes whether the network is IPv6 twice
using strings.HasSuffix(network, "6"); simplify by computing a boolean once
(e.g., isV6 := strings.HasSuffix(network, "6")) at the top of
applyBoundIfToSocket and reuse it for both the call to boundInterfaceFor(...)
(add an overload or pass the boolean if you prefer) and the sockopt branch to
call setIPv6BoundIf or setIPv4BoundIf, leaving all existing error handling
intact and keeping the same log message and behavior.
- Around line 36-47: SetBoundInterface currently treats a nil iface as a silent
clear; make nil handling explicit by rejecting nil inputs with a warning and
no-op to mirror the unsupported-family branch: inside SetBoundInterface, check
if iface == nil and call log.Warnf("SetBoundInterface: nil iface for AF %d,
no-op", af) then return without modifying boundIface4/boundIface6; reference
SetBoundInterface, boundIface4, boundIface6 and the default log.Warnf so
reviewers can find the spot to change (optionally update the doc comment on
"records the egress interface" to state nil is ignored).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63b58c27-e651-421b-a828-4eaf012333da
📒 Files selected for processing (10)
client/internal/routemanager/systemops/systemops_bsd_other.goclient/internal/routemanager/systemops/systemops_darwin.goclient/internal/routemanager/systemops/systemops_unix.goclient/net/dialer_init_darwin.goclient/net/dialer_init_generic.goclient/net/env_darwin.goclient/net/env_generic.goclient/net/listener_init_darwin.goclient/net/listener_init_generic.goclient/net/net_darwin.go
✅ Files skipped from review due to trivial changes (3)
- client/net/listener_init_generic.go
- client/net/dialer_init_generic.go
- client/net/listener_init_darwin.go
🚧 Files skipped from review as they are similar to previous changes (6)
- client/net/env_generic.go
- client/net/dialer_init_darwin.go
- client/internal/routemanager/systemops/systemops_bsd_other.go
- client/internal/routemanager/systemops/systemops_unix.go
- client/net/env_darwin.go
- client/internal/routemanager/systemops/systemops_darwin.go
lixmal
force-pushed
the
macos-ip-bound-if
branch
from
40e8020 to
413fc1d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/net/net_darwin.go (1)
123-149: Defensive check recommended to skip unsupported socket families.
applyBoundIfToSocketassumes any non-*6 network is IPv4 and applies IP socket options. This would fail if invoked with unsupported families likeunix,unixgram, orunixpacket. Although the codebase currently uses only IP networks with these wrappers, adding an explicit guard prevents future breakage.Proposed hardening
func applyBoundIfToSocket(network, address string, c syscall.RawConn) error { if !AdvancedRouting() { return nil } + switch { + case strings.HasSuffix(network, "4"), strings.HasSuffix(network, "6"): + default: + return nil + } + iface := boundInterfaceFor(network, address) if iface == nil { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/net/net_darwin.go` around lines 123 - 149, applyBoundIfToSocket currently treats any non-IPv6 network as IPv4 which will break for non-IP socket families (e.g., "unix", "unixgram", "unixpacket"); update applyBoundIfToSocket to explicitly detect and only handle IPv4/IPv6 families (use the existing isV6Network and add or use an isV4Network check) and return nil immediately for unsupported families, avoiding calling setIPv4BoundIf/setIPv6BoundIf; include a small debug log when skipping unsupported families to aid future debugging.
🤖 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/internal/routemanager/systemops/systemops_unix.go`:
- Around line 197-219: The readRouteResponse loop can spin forever when
unrelated AF_ROUTE broadcasts keep making unix.Read succeed; add an absolute
deadline check to bound the whole filtering operation by computing a deadline
before the for loop (e.g., now + a configurable timeout) and at the top of the
loop return a permanent timeout error if time.Now() is after the deadline;
ensure the check references readRouteResponse, the fd/unix.Read loop and the
wantType/wantSeq/hdr filtering logic so the function still filters responses but
will abort with a clear backoff.Permanent timeout if the overall wait exceeds
the deadline.
---
Nitpick comments:
In `@client/net/net_darwin.go`:
- Around line 123-149: applyBoundIfToSocket currently treats any non-IPv6
network as IPv4 which will break for non-IP socket families (e.g., "unix",
"unixgram", "unixpacket"); update applyBoundIfToSocket to explicitly detect and
only handle IPv4/IPv6 families (use the existing isV6Network and add or use an
isV4Network check) and return nil immediately for unsupported families, avoiding
calling setIPv4BoundIf/setIPv6BoundIf; include a small debug log when skipping
unsupported families to aid future debugging.
🪄 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: 50ee690c-11b2-469a-8410-b33c73b7d0f3
📒 Files selected for processing (10)
client/internal/routemanager/systemops/systemops_bsd_other.goclient/internal/routemanager/systemops/systemops_darwin.goclient/internal/routemanager/systemops/systemops_unix.goclient/net/dialer_init_darwin.goclient/net/dialer_init_generic.goclient/net/env_darwin.goclient/net/env_generic.goclient/net/listener_init_darwin.goclient/net/listener_init_generic.goclient/net/net_darwin.go
✅ Files skipped from review due to trivial changes (4)
- client/net/listener_init_generic.go
- client/net/dialer_init_generic.go
- client/net/listener_init_darwin.go
- client/internal/routemanager/systemops/systemops_bsd_other.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/net/dialer_init_darwin.go
- client/net/env_generic.go
lixmal
force-pushed
the
macos-ip-bound-if
branch
from
413fc1d to
514934e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/net/net_darwin.go (1)
123-150: Use the zone‑derived iface as the source of truth for address family.
boundInterfaceForcan return an interface extracted from an IPv6 link‑local zone (e.g.fe80::1%en0), butapplyBoundIfToSocketdetermines the socket family viaisV6Network(network)(suffix"6"). Per the top‑of‑file note,IP_BOUND_IFon anAF_INET6socket returnsEINVAL; so ifnetworkwere bare"tcp"/"udp"(without the"6"suffix) alongside an IPv6 zoned address, we'd callsetIPv4BoundIfon an IPv6 socket and fail.Go's
net.Dialerandnet.ListenerConfigresolve bare network strings to specific variants (e.g.,"tcp"→"tcp4"or"tcp6") before invoking the Control callback based on the target address family, so this scenario is mitigated in practice. However, derivingisV6from the zone (which unambiguously signals IPv6) rather than the network string suffix makes the logic more explicit and robust.♻️ Proposed refactor
-func boundInterfaceFor(network, address string) *net.Interface { - if iface := zoneInterface(address); iface != nil { - return iface - } - - boundIfaceMu.RLock() - defer boundIfaceMu.RUnlock() - - if isV6Network(network) { - return boundIface6 - } - return boundIface4 -} +// boundInterfaceFor returns the egress interface and whether the selection +// implies an IPv6 socket (either because the address carried an IPv6 zone or +// because the network is an IPv6 variant). +func boundInterfaceFor(network, address string) (*net.Interface, bool) { + if iface := zoneInterface(address); iface != nil { + return iface, true + } + + boundIfaceMu.RLock() + defer boundIfaceMu.RUnlock() + + if isV6Network(network) { + return boundIface6, true + } + return boundIface4, false +} @@ - iface := boundInterfaceFor(network, address) - if iface == nil { - return nil - } - - isV6 := isV6Network(network) + iface, isV6 := boundInterfaceFor(network, address) + if iface == nil { + return nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/net/net_darwin.go` around lines 123 - 150, The code currently determines IPv6 vs IPv4 by isV6Network(network) which can mismatch when boundInterfaceFor(...) returns a zone-derived IPv6 interface; change applyBoundIfToSocket to derive isV6 from the returned iface instead: call iface.Addrs() and treat the socket as IPv6 if any returned net.Addr is an IPNet with IP.To4()==nil (i.e., IPv6), falling back to isV6Network(network) only if iface.Addrs() errors or returns no addresses; then use that isV6 value to choose between setIPv6BoundIf and setIPv4BoundIf (keep existing logging and error flow in applyBoundIfToSocket).
🤖 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/net/env_darwin.go`:
- Around line 26-44: In checkAdvancedRoutingSupport(), make two fixes: (1) when
strconv.ParseBool on envUseLegacyRouting fails, set legacyRouting = true
(fail-safe) and change the warning to state the env var was unparseable and
legacy routing is being assumed, and (2) split the single log that currently
runs for both conditions into two branches so netstack.IsEnabled() and
legacyRouting produce distinct messages (e.g., "advanced routing has been
requested to be disabled" when legacyRouting is true, vs "advanced routing
cannot be enabled because netstack is active" when netstack.IsEnabled() is true)
to make it clear which gate closed; use the checkAdvancedRoutingSupport function
and netstack.IsEnabled()/envUseLegacyRouting identifiers to locate the code.
---
Nitpick comments:
In `@client/net/net_darwin.go`:
- Around line 123-150: The code currently determines IPv6 vs IPv4 by
isV6Network(network) which can mismatch when boundInterfaceFor(...) returns a
zone-derived IPv6 interface; change applyBoundIfToSocket to derive isV6 from the
returned iface instead: call iface.Addrs() and treat the socket as IPv6 if any
returned net.Addr is an IPNet with IP.To4()==nil (i.e., IPv6), falling back to
isV6Network(network) only if iface.Addrs() errors or returns no addresses; then
use that isV6 value to choose between setIPv6BoundIf and setIPv4BoundIf (keep
existing logging and error flow in applyBoundIfToSocket).
🪄 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: 4625fd34-e219-4acb-9c65-cb19f6bb2f39
📒 Files selected for processing (10)
client/internal/routemanager/systemops/systemops_bsd_other.goclient/internal/routemanager/systemops/systemops_darwin.goclient/internal/routemanager/systemops/systemops_unix.goclient/net/dialer_init_darwin.goclient/net/dialer_init_generic.goclient/net/env_darwin.goclient/net/env_generic.goclient/net/listener_init_darwin.goclient/net/listener_init_generic.goclient/net/net_darwin.go
✅ Files skipped from review due to trivial changes (4)
- client/net/dialer_init_generic.go
- client/net/listener_init_generic.go
- client/internal/routemanager/systemops/systemops_bsd_other.go
- client/internal/routemanager/systemops/systemops_unix.go
🚧 Files skipped from review as they are similar to previous changes (4)
- client/net/listener_init_darwin.go
- client/net/env_generic.go
- client/net/dialer_init_darwin.go
- client/internal/routemanager/systemops/systemops_darwin.go
lixmal
force-pushed
the
macos-ip-bound-if
branch
from
36971a4 to
48d78c7
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/internal/routemanager/systemops/systemops_unix.go (1)
151-191: Backoff budget vs. per-attempt deadline interaction.
writeRouteMessagesetsMaxElapsedTime = 1swhilerouteMessageRoundtriphas its own ~1s read deadline (SO_RCVTIMEO=1s+deadline := time.Now().Add(time.Second)inreadRouteResponse). If a single attempt consumes its full read deadline,backoff.Retrywill not retry — the budget is already exhausted by the first attempt. In practice this is fine because the retries are mainly for fast-failingENOBUFS/EAGAINwrites, but if you later want genuine retries after a slow/no-reply attempt, consider making the outerMaxElapsedTimea multiple of the per-attempt read deadline (e.g., 3–5s).Also,
msg.Marshal()is re-run on every retry even thoughmsgis immutable across attempts — you could hoist the marshalling intowriteRouteMessageonce and pass[]byteinto the roundtrip. Minor, purely optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/routemanager/systemops/systemops_unix.go` around lines 151 - 191, The outer backoff MaxElapsedTime (writeRouteMessage) equals the per-attempt read deadline in readRouteResponse, so a single full-duration attempt consumes the whole retry budget; increase MaxElapsedTime to a multiple of the per-attempt read timeout (e.g., 3*time.Second or compute as N*perAttempt) in writeRouteMessage so retries remain possible, and optionally hoist msg.Marshal() out of the retry loop by marshaling once in writeRouteMessage and passing the []byte into routeMessageRoundtrip (update routeMessageRoundtrip signature/uses accordingly) to avoid repeated marshals; ensure references to read timeout (readRouteResponse's time.Second) remain consistent with the chosen MaxElapsedTime.
🤖 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/systemops/systemops_unix.go`:
- Around line 151-191: The outer backoff MaxElapsedTime (writeRouteMessage)
equals the per-attempt read deadline in readRouteResponse, so a single
full-duration attempt consumes the whole retry budget; increase MaxElapsedTime
to a multiple of the per-attempt read timeout (e.g., 3*time.Second or compute as
N*perAttempt) in writeRouteMessage so retries remain possible, and optionally
hoist msg.Marshal() out of the retry loop by marshaling once in
writeRouteMessage and passing the []byte into routeMessageRoundtrip (update
routeMessageRoundtrip signature/uses accordingly) to avoid repeated marshals;
ensure references to read timeout (readRouteResponse's time.Second) remain
consistent with the chosen MaxElapsedTime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50f43633-77a9-4fa5-be9d-93323ef2c351
📒 Files selected for processing (15)
client/iface/udpmux/universal.goclient/internal/routemanager/systemops/systemops_bsd_other.goclient/internal/routemanager/systemops/systemops_darwin.goclient/internal/routemanager/systemops/systemops_generic.goclient/internal/routemanager/systemops/systemops_js.goclient/internal/routemanager/systemops/systemops_linux.goclient/internal/routemanager/systemops/systemops_nonlinux.goclient/internal/routemanager/systemops/systemops_unix.goclient/net/dialer_init_darwin.goclient/net/dialer_init_generic.goclient/net/env_bound_iface.goclient/net/env_generic.goclient/net/listener_init_darwin.goclient/net/listener_init_generic.goclient/net/net_darwin.go
💤 Files with no reviewable changes (3)
- client/internal/routemanager/systemops/systemops_nonlinux.go
- client/internal/routemanager/systemops/systemops_linux.go
- client/internal/routemanager/systemops/systemops_js.go
✅ Files skipped from review due to trivial changes (4)
- client/net/listener_init_generic.go
- client/net/listener_init_darwin.go
- client/iface/udpmux/universal.go
- client/internal/routemanager/systemops/systemops_bsd_other.go
🚧 Files skipped from review as they are similar to previous changes (5)
- client/net/dialer_init_generic.go
- client/net/env_generic.go
- client/net/dialer_init_darwin.go
- client/net/net_darwin.go
- client/internal/routemanager/systemops/systemops_darwin.go
lixmal
force-pushed
the
macos-ip-bound-if
branch
2 times, most recently
from
4c0ba2f to
10f151a
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/internal/routemanager/systemops/systemops_unix.go (1)
229-235: Set the route message sender ID for compliance with golang.org/x/net/route library best practices.The
golang.org/x/net/routepackage documents thatIDshould be set touintptr(os.Getpid())when manipulating routing information. While the current code functions because the kernel automatically reflects the sender's PID in responses, explicitly settingIDaligns with the library's recommended usage.Proposed fix
msg = &route.RouteMessage{ Type: action, Flags: unix.RTF_UP | routeProtoFlag, Version: unix.RTM_VERSION, + ID: uintptr(os.Getpid()), Seq: r.getSeq(), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/routemanager/systemops/systemops_unix.go` around lines 229 - 235, The route message built in SysOps.buildRouteMessage does not set the sender ID on the route.RouteMessage; set msg.ID = uintptr(os.Getpid()) before returning so the message follows golang.org/x/net/route recommendations (import os if needed). Locate buildRouteMessage and add assignment to msg.ID (using os.Getpid()) while keeping other fields (Type, Flags, Version, Seq) unchanged.
🤖 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/systemops/systemops_unix.go`:
- Around line 229-235: The route message built in SysOps.buildRouteMessage does
not set the sender ID on the route.RouteMessage; set msg.ID =
uintptr(os.Getpid()) before returning so the message follows
golang.org/x/net/route recommendations (import os if needed). Locate
buildRouteMessage and add assignment to msg.ID (using os.Getpid()) while keeping
other fields (Type, Flags, Version, Seq) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13e69b68-c450-4ade-9032-053b76c1fada
📒 Files selected for processing (4)
client/internal/routemanager/systemops/systemops_darwin.goclient/internal/routemanager/systemops/systemops_unix.goclient/net/net_darwin.goclient/server/state.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/internal/routemanager/systemops/systemops_darwin.go
lixmal
force-pushed
the
macos-ip-bound-if
branch
2 times, most recently
from
c85494a to
4eb38fc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/internal/routemanager/systemops/systemops_darwin.go (1)
82-99:⚠️ Potential issue | 🟠 MajorAdd IP validation check to reject interface-only nexthops before RTM_ADD.
Darwin's routing socket requires a valid gateway IP when installing
RTF_IFSCOPEscoped default routes. The current code permitsnexthop.IPto be invalid and falls back to emittingRTAX_GATEWAYas aLinkAddr, but this approach is not supported on Darwin for this operation. Fail early and reject interface-only nexthops.Proposed fix
- if nexthop.Intf == nil { - return false, fmt.Errorf("unusable default nexthop for %s (no interface)", unspec) + if nexthop.Intf == nil || !nexthop.IP.IsValid() { + return false, fmt.Errorf("unusable default nexthop for %s (interface: %v, gateway: %s)", unspec, nexthop.Intf, nexthop.IP) } if err := r.addScopedDefault(unspec, nexthop); err != nil { return false, fmt.Errorf("add scoped default on %s: %w", nexthop.Intf.Name, err) } @@ - via := "point-to-point" - if nexthop.IP.IsValid() { - via = nexthop.IP.String() - } + via := nexthop.IP.String() log.Infof("installed scoped default route via %s on %s for %s", via, nexthop.Intf.Name, afOf(unspec)) return true, nil } @@ - if nexthop.IP.IsValid() { - msg.Flags |= unix.RTF_GATEWAY - gw, err := addrToRouteAddr(nexthop.IP.Unmap()) - if err != nil { - return fmt.Errorf("build gateway: %w", err) - } - addrs[unix.RTAX_GATEWAY] = gw - } else { - addrs[unix.RTAX_GATEWAY] = &route.LinkAddr{ - Index: nexthop.Intf.Index, - Name: nexthop.Intf.Name, - } + msg.Flags |= unix.RTF_GATEWAY + gw, err := addrToRouteAddr(nexthop.IP.Unmap()) + if err != nil { + return fmt.Errorf("build gateway: %w", err) } + addrs[unix.RTAX_GATEWAY] = gwAlso applies to: lines 212-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/routemanager/systemops/systemops_darwin.go` around lines 82 - 99, The code currently allows interface-only nexthops and proceeds to call r.addScopedDefault and RTM_ADD, which fails on Darwin; update the logic in the scoped-default install paths (the block using nexthop, r.addScopedDefault, nbnet.SetBoundInterface and afOf) to check nexthop.IP.IsValid() early and return an error (e.g., "interface-only nexthop not supported for scoped default") instead of proceeding when the gateway IP is missing; apply the same validation and early-return change to the other scoped-default block referenced around the second location (the lines handling nexthop and addScopedDefault at 212-224) so both code paths reject interface-only nexthops before attempting RTM_ADD.
🤖 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/internal/routemanager/systemops/systemops_darwin.go`:
- Around line 43-68: The setupAdvancedRouting path must clear stale socket
bindings before rebuilding scoped defaults: after calling
r.flushScopedDefaults() in setupAdvancedRouting, invoke the socket cache reset
(call SetBoundInterface(nil) on the socket manager / the equivalent method on r
that clears previously-bound interfaces) so any old cached interface is removed
before running the installScopedDefaultFor loop; ensure this clearing happens
unconditionally (even if flushScopedDefaults returned an error) and keep the
existing error aggregation and logging behavior.
---
Duplicate comments:
In `@client/internal/routemanager/systemops/systemops_darwin.go`:
- Around line 82-99: The code currently allows interface-only nexthops and
proceeds to call r.addScopedDefault and RTM_ADD, which fails on Darwin; update
the logic in the scoped-default install paths (the block using nexthop,
r.addScopedDefault, nbnet.SetBoundInterface and afOf) to check
nexthop.IP.IsValid() early and return an error (e.g., "interface-only nexthop
not supported for scoped default") instead of proceeding when the gateway IP is
missing; apply the same validation and early-return change to the other
scoped-default block referenced around the second location (the lines handling
nexthop and addScopedDefault at 212-224) so both code paths reject
interface-only nexthops before attempting RTM_ADD.
🪄 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: 36cb8c4f-e8de-4035-8a3e-1cec8937ab37
📒 Files selected for processing (5)
client/internal/routemanager/systemops/systemops_darwin.goclient/internal/routemanager/systemops/systemops_unix.goclient/net/env_mobile.goclient/net/net_darwin.goclient/server/state.go
✅ Files skipped from review due to trivial changes (1)
- client/net/env_mobile.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/internal/routemanager/systemops/systemops_unix.go
lixmal
force-pushed
the
macos-ip-bound-if
branch
from
4eb38fc to
a4d8f23
Compare
lixmal
force-pushed
the
macos-ip-bound-if
branch
from
a4d8f23 to
40e7e14
Compare
|
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
✅ Created PR with unit tests: #5927 |
pappz
approved these changes
Apr 20, 2026
This was referenced Apr 21, 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
Replace per-peer exclusion routes on macOS with a scoped default route plus
IP_BOUND_IFon NetBird sockets.RTF_IFSCOPEdefault per address family pinned to the physical egress, tagged withRTF_PROTO1.IP_BOUND_IF/IPV6_BOUND_IFon sockets fromnbnet.NewDialer/nbnet.NewListener.NB_USE_LEGACY_ROUTING=1falls back to the old per-peer refcounter.Also skip the routed ICE-candidate filter (
IsAddrRouted) on all platforms whenAdvancedRouting()is active. With fwmark (linux),IP_UNICAST_IF(windows) orIP_BOUND_IF(darwin) the WG socket bypasses the main routing table, so a candidate address that overlaps a VPN route still goes out the physical interface and can't form a loop. Collapses the linux-onlyhasSeparateRouting/ErrRoutingIsSeparateindirection into a singleAdvancedRouting()check.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
New Features
Refactor
Chores
Bug Fixes