-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ios] Ensure route settlement on iOS before handling DNS responses #5360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
d1ddef9
Ensure route settlement on iOS before handling DNS responses to preve…
pappz ee844aa
Merge branch 'main' into fix/ios-dns-first-response
mlsmaycon 4163012
add more logs
mlsmaycon 63c34e8
Merge branch 'main' into fix/ios-dns-first-response
mlsmaycon 8c08d49
rollback debug changes
mlsmaycon bbfbc26
rollback changes
mlsmaycon f33020e
[client] Improve logging and add comments for iOS route settlement logic
pappz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
20 changes: 20 additions & 0 deletions
20
client/internal/routemanager/dnsinterceptor/handler_ios.go
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| //go:build ios | ||
|
|
||
| package dnsinterceptor | ||
|
|
||
| import ( | ||
| "time" | ||
|
|
||
| log "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| const routeSettleDelay = 500 * time.Millisecond | ||
|
|
||
| // waitForRouteSettlement introduces a short delay on iOS to allow | ||
| // setTunnelNetworkSettings to apply route changes before the DNS | ||
| // response reaches the application. Without this, the first request | ||
| // to a newly resolved domain may bypass the tunnel. | ||
| func waitForRouteSettlement(logger *log.Entry) { | ||
| logger.Tracef("waiting %v for iOS route settlement", routeSettleDelay) | ||
| time.Sleep(routeSettleDelay) | ||
| } |
12 changes: 12 additions & 0 deletions
12
client/internal/routemanager/dnsinterceptor/handler_nonios.go
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| //go:build !ios | ||
|
|
||
| package dnsinterceptor | ||
|
|
||
| import log "github.com/sirupsen/logrus" | ||
|
|
||
| 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. | ||
| } | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: netbirdio/netbird
Length of output: 6892
🏁 Script executed:
Repository: 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.goexists, the//go:build !iosconstraint onhandler_nonios.gocauses Android to use the no-op implementation. The unconditional call towaitForRouteSettlementat line 357 inhandler.gowill execute the no-op on Android despite Android'sVpnService.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.gowith the same sleep implementation ashandler_ios.go, or confirm Android's route handling does not exhibit the same async timing issue.🤖 Prompt for AI Agents