[ios] Ensure route settlement on iOS before handling DNS responses#5360
[ios] Ensure route settlement on iOS before handling DNS responses#5360
Conversation
…nt bypassing the tunnel.
📝 WalkthroughWalkthroughAdds a platform-specific delay before replacing IPs in DNS responses: after updating domain prefixes the handler calls Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
client/internal/routemanager/dnsinterceptor/handler.go (1)
354-358: The delay fires even when no routes actually changed.
waitForRouteSettlementis called whenevernewPrefixesis non-empty, butupdateDomainPrefixesmay determine that all prefixes already exist (i.e.,toAddis empty), meaning no route changes were triggered. On iOS, this adds an unnecessary 500ms penalty to every repeated DNS lookup for an already-resolved domain.Consider having
updateDomainPrefixesreturn a boolean indicating whether routes actually changed, and only sleep when they did:♻️ Suggested approach
- if err := d.updateDomainPrefixes(resolvedDomain, originalDomain, newPrefixes, logger); err != nil { + changed, err := d.updateDomainPrefixes(resolvedDomain, originalDomain, newPrefixes, logger) + if err != nil { logger.Errorf("failed to update domain prefixes: %v", err) } - // Allow time for route changes to be applied before sending - // the DNS response (relevant on iOS where setTunnelNetworkSettings - // is asynchronous). - waitForRouteSettlement(logger) + if changed { + // Allow time for route changes to be applied before sending + // the DNS response (relevant on iOS where setTunnelNetworkSettings + // is asynchronous). + waitForRouteSettlement(logger) + }And in
updateDomainPrefixes, returnlen(toAdd) > 0 || len(toRemove) > 0as thechangedindicator (adjusting the signature to(bool, error)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/routemanager/dnsinterceptor/handler.go` around lines 354 - 358, The current code always calls waitForRouteSettlement(logger) whenever newPrefixes is non-empty, causing an unnecessary 500ms delay even if updateDomainPrefixes made no changes; modify updateDomainPrefixes to return (bool, error) where the bool is changed := (len(toAdd) > 0 || len(toRemove) > 0), then in the caller check that changed is true before calling waitForRouteSettlement(logger) so the sleep only runs when routes were actually modified.client/internal/routemanager/dnsinterceptor/handler_ios.go (1)
11-19: Pragmatic workaround — consider documenting the 500ms rationale.The fixed 500ms delay is a reasonable heuristic, but it's worth noting that under heavy load or on older devices,
setTunnelNetworkSettingsmay take longer, while on fast paths it wastes time. If there's empirical data or Apple documentation backing this value, a brief comment on the constant would help future maintainers understand the trade-off.Otherwise, the implementation is clean and well-commented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/routemanager/dnsinterceptor/handler_ios.go` around lines 11 - 19, Add documentation explaining why routeSettleDelay is 500ms and any assumptions or evidence backing it: update the comment above the constant routeSettleDelay and the waitForRouteSettlement function to note whether the value is empirical or conservative, mention if it targets specific iOS versions/devices or observed latency ranges, and indicate that it can be tuned or converted to a dynamic/backoff strategy later; reference the routeSettleDelay constant and waitForRouteSettlement(logger *log.Entry) so maintainers know where to change or experiment with the value.
🤖 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/dnsinterceptor/handler_ios.go`:
- Around line 11-19: Add documentation explaining why routeSettleDelay is 500ms
and any assumptions or evidence backing it: update the comment above the
constant routeSettleDelay and the waitForRouteSettlement function to note
whether the value is empirical or conservative, mention if it targets specific
iOS versions/devices or observed latency ranges, and indicate that it can be
tuned or converted to a dynamic/backoff strategy later; reference the
routeSettleDelay constant and waitForRouteSettlement(logger *log.Entry) so
maintainers know where to change or experiment with the value.
In `@client/internal/routemanager/dnsinterceptor/handler.go`:
- Around line 354-358: The current code always calls
waitForRouteSettlement(logger) whenever newPrefixes is non-empty, causing an
unnecessary 500ms delay even if updateDomainPrefixes made no changes; modify
updateDomainPrefixes to return (bool, error) where the bool is changed :=
(len(toAdd) > 0 || len(toRemove) > 0), then in the caller check that changed is
true before calling waitForRouteSettlement(logger) so the sleep only runs when
routes were actually modified.
- Switch iOS route settlement log level from Debug to Trace for finer control. - Add clarifying comments for `waitForRouteSettlement` on non-iOS platforms.
|
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/dnsinterceptor/handler_nonios.go`:
- Around line 7-12: The no-op waitForRouteSettlement in handler_nonios.go is
used on Android due to the !ios build tag, but Android's VpnService.establish()
applies routes asynchronously like iOS; either add an Android-specific
implementation or verify it's unnecessary. Create handler_android.go that
implements waitForRouteSettlement with the same short sleep/delay logic as
handler_ios.go (so the unconditional call from handler.go still waits), or if
you confirm Android timing is safe, document that and remove the
Android-specific sleep path by updating waitForRouteSettlement behavior
accordingly; reference the waitForRouteSettlement function, handler_ios.go
implementation, and the call site in handler.go for locating the change.
| func waitForRouteSettlement(_ *log.Entry) { | ||
| // No-op on non-iOS platforms: route changes are applied synchronously by | ||
| // the kernel, so no settlement delay is needed before the DNS response | ||
| // reaches the application. The delay is only required on iOS where | ||
| // setTunnelNetworkSettings applies routes asynchronously. | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for handler_*.go files in dnsinterceptor
echo "=== Handler files in dnsinterceptor ==="
fd -g 'handler_*.go' client/internal/routemanager/dnsinterceptor/
# Check for android references in dnsinterceptor
echo -e "\n=== Android references in dnsinterceptor ==="
rg -i 'android' client/internal/routemanager/dnsinterceptor/
# Check handler.go for build constraints and waitForRouteSettlement calls
echo -e "\n=== Build constraints and relevant code in handler.go ==="
rg -n '//go:build|//\+build|waitForRouteSettlement' client/internal/routemanager/dnsinterceptor/handler.go
# Check build tags in handler_nonios.go
echo -e "\n=== Build tags in handler_nonios.go ==="
head -10 client/internal/routemanager/dnsinterceptor/handler_nonios.go
# Look for Android target support at repo level
echo -e "\n=== Android GOOS references in root/build files ==="
fd -g 'Makefile|*.mk|*.sh|go.mod|go.sum' | head -5 | xargs rg -l 'android' 2>/dev/null || echo "No android refs in common build files"
# Check if there's any reference to Android vpn service or Android-specific route handling elsewhere
echo -e "\n=== Android VpnService or Android route handling in codebase ==="
rg -i 'vpnservice|android.*route|route.*android' client/ | head -20Repository: netbirdio/netbird
Length of output: 6892
🏁 Script executed:
#!/bin/bash
# Check the iOS implementation
echo "=== handler_ios.go implementation ==="
cat -n client/internal/routemanager/dnsinterceptor/handler_ios.go
# Check context around line 357 in handler.go to see if there's Android gating
echo -e "\n=== Context around waitForRouteSettlement call in handler.go (line 357) ==="
sed -n '350,365p' client/internal/routemanager/dnsinterceptor/handler.go
# Check if there's any condition checking for android before calling waitForRouteSettlement
echo -e "\n=== Full context for waitForRouteSettlement usage ==="
rg -B5 -A5 'waitForRouteSettlement' client/internal/routemanager/dnsinterceptor/handler.go
# Check if there's Android-specific route settlement elsewhere
echo -e "\n=== Android route settlement or similar in Android-specific files ==="
rg -i 'settlement|delay.*route' client/android/ client/internal/routemanager/systemops/systemops_android.go client/internal/routemanager/notifier/notifier_android.go 2>/dev/nullRepository: netbirdio/netbird
Length of output: 1784
Android also executes handler_nonios.go's no-op — add Android-specific route settlement delay or verify it's not needed.
Android is a supported platform in NetBird, and because no handler_android.go exists, the //go:build !ios constraint on handler_nonios.go causes Android to use the no-op implementation. The unconditional call to waitForRouteSettlement at line 357 in handler.go will execute the no-op on Android despite Android's VpnService.establish() also applying tunnel settings asynchronously—the same async behavior that necessitates the 500ms settlement delay on iOS. Without this delay on Android, the DNS-bypass bug this PR fixes on iOS can occur on Android as well.
Either create handler_android.go with the same sleep implementation as handler_ios.go, or confirm Android's route handling does not exhibit the same async timing issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/internal/routemanager/dnsinterceptor/handler_nonios.go` around lines 7
- 12, The no-op waitForRouteSettlement in handler_nonios.go is used on Android
due to the !ios build tag, but Android's VpnService.establish() applies routes
asynchronously like iOS; either add an Android-specific implementation or verify
it's unnecessary. Create handler_android.go that implements
waitForRouteSettlement with the same short sleep/delay logic as handler_ios.go
(so the unconditional call from handler.go still waits), or if you confirm
Android timing is safe, document that and remove the Android-specific sleep path
by updating waitForRouteSettlement behavior accordingly; reference the
waitForRouteSettlement function, handler_ios.go implementation, and the call
site in handler.go for locating the change.
2 participants
Ensure route settlement on iOS before handling DNS responses to prevent bypassing the tunnel.
Describe your changes
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