[client] Support embed.Client on Android with netstack mode#5623
Merged
pappz merged 2 commits intonetbirdio:mainfrom Apr 1, 2026
Merged
[client] Support embed.Client on Android with netstack mode#5623pappz merged 2 commits intonetbirdio:mainfrom
pappz merged 2 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an Android-specific startup hook and Android-only runner plus no-op mobile dependency stubs so ConnectClient.Run can delegate to an embed-focused flow on Android without full platform integrations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Android App
participant CC as ConnectClient
participant IF as ExternalIFaceDiscover
participant NL as NetworkChangeListener
participant DNS as dns.ReadyListener
participant Engine as ConnectClient.run
App->>CC: Start / ConnectClient.Run(runningChan, logPath)
alt androidRunOverride != nil
CC->>CC: androidRunOverride(c, runningChan, logPath)
androidRunOverride->>CC: runOnAndroidEmbed(...)
runOnAndroidEmbed->>IF: use provided IFaceDiscover
runOnAndroidEmbed->>NL: use provided NetworkChangeListener
runOnAndroidEmbed->>DNS: use provided DnsReadyListener
runOnAndroidEmbed->>Engine: c.run(MobileDependency{IF,NL,DNS}, runningChan, logPath)
Engine-->>CC: returns error/ok
CC-->>App: return result
else
CC->>Engine: c.run(MobileDependency{}, runningChan, logPath)
Engine-->>CC: returns error/ok
CC-->>App: return result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
tham-le
force-pushed
the
tham/embed-android-support
branch
from
28f6e55 to
d4ff11b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make client/embed usable on Android when running in netstack/userspace mode by avoiding nil-pointer dereferences stemming from empty MobileDependency{} passed through ConnectClient.Run().
Changes:
- Added an Android-only
RunOnAndroidEmbedhelper to passrunningChanplus selected mobile dependencies into the internal connect flow. - Added an
androidRunOverridehook inConnectClient.Run()to allow Android-specific dispatch. - Hardened Android engine code paths with nil checks for
NetworkChangeListener,TunAdapter, andDnsReadyListener, and adjusted DNS-server selection behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| client/internal/engine.go | Adds nil checks for Android/mobile dependencies and adjusts Android DNS/WG interface setup behavior to avoid panics in embed/netstack scenarios. |
| client/internal/connect_android_embed.go | Introduces Android-only RunOnAndroidEmbed helper that forwards a runningChan to the shared connect flow. |
| client/internal/connect.go | Adds androidRunOverride hook to intercept Run() on Android and inject mobile deps for embed use cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/internal/engine.go (1)
1775-1792:⚠️ Potential issue | 🔴 CriticalAndroid DNS fallback currently jumps into the iOS branch.
At Line 1788,
fallthroughsends Android execution todns.NewDefaultServerIos(...). That diverges from the intended non-mobile/default fallback and can break Android embed netstack DNS behavior whenDnsReadyListeneris nil.🐛 Proposed fix
switch runtime.GOOS { case "android": if e.mobileDep.NetworkChangeListener != nil && e.mobileDep.DnsReadyListener != nil { dnsServer := dns.NewDefaultServerPermanentUpstream( e.ctx, e.wgInterface, e.mobileDep.HostDNSAddresses, *dnsConfig, e.mobileDep.NetworkChangeListener, e.statusRecorder, e.config.DisableDNS, ) go e.mobileDep.DnsReadyListener.OnReady() return dnsServer, nil } - fallthrough + dnsServer, err := dns.NewDefaultServer(e.ctx, dns.DefaultServerConfig{ + WgInterface: e.wgInterface, + CustomAddress: e.config.CustomDNSAddress, + StatusRecorder: e.statusRecorder, + StateManager: e.stateManager, + DisableSys: e.config.DisableDNS, + }) + if err != nil { + return nil, err + } + return dnsServer, nil case "ios": dnsServer := dns.NewDefaultServerIos(e.ctx, e.wgInterface, e.mobileDep.DnsManager, e.statusRecorder, e.config.DisableDNS) return dnsServer, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/engine.go` around lines 1775 - 1792, The Android branch incorrectly uses a bare fallthrough which transfers control into the "ios" case and calls dns.NewDefaultServerIos; remove the fallthrough and instead call the intended non-mobile/default DNS fallback (e.g., construct/return the same default server used elsewhere such as dns.NewDefaultServerPermanentUpstream or the package's generic/default server) when e.mobileDep.DnsReadyListener is nil, guarding on e.mobileDep.NetworkChangeListener and e.mobileDep.DnsReadyListener; update the Android branch around the check that references e.mobileDep, DnsReadyListener, NewDefaultServerPermanentUpstream and NewDefaultServerIos so Android never falls into the iOS case.
🤖 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/connect.go`:
- Around line 46-49: The declared hook variable androidRunOverride is never
assigned but is checked in ConnectClient.Run(), so either wire it in an
Android-specific init or remove the unused hook; to fix either add an
Android-only file (build tag // +build android) that defines init() which
assigns androidRunOverride to the Android-specific implementation function
(matching signature func(c *ConnectClient, runningChan chan struct{}, logPath
string) error) so the Run() path can execute, or remove the androidRunOverride
declaration and its conditional check in ConnectClient.Run() to eliminate the
dead code.
---
Outside diff comments:
In `@client/internal/engine.go`:
- Around line 1775-1792: The Android branch incorrectly uses a bare fallthrough
which transfers control into the "ios" case and calls dns.NewDefaultServerIos;
remove the fallthrough and instead call the intended non-mobile/default DNS
fallback (e.g., construct/return the same default server used elsewhere such as
dns.NewDefaultServerPermanentUpstream or the package's generic/default server)
when e.mobileDep.DnsReadyListener is nil, guarding on
e.mobileDep.NetworkChangeListener and e.mobileDep.DnsReadyListener; update the
Android branch around the check that references e.mobileDep, DnsReadyListener,
NewDefaultServerPermanentUpstream and NewDefaultServerIos so Android never falls
into the iOS case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04f8067a-8e3a-4d1f-86be-4bd8f658c470
📒 Files selected for processing (3)
client/internal/connect.goclient/internal/connect_android_embed.goclient/internal/engine.go
tham-le
force-pushed
the
tham/embed-android-support
branch
2 times, most recently
from
06402ca to
e5f0f8e
Compare
pappz
requested changes
Mar 30, 2026
Collaborator
|
@tham-le Instead of adding nil-check if/else branches in engine.go, provide complete no-op stubs in connect_android_default.go including a noopDnsReadyListener. This way, the engine’s existing Android code path (NewDefaultServerPermanentUpstream + DnsReadyListener.OnReady()) works unchanged with embed mode, and no modifications to engine.go Also, logPath is received by androidRunOverride but is not forwarded to RunOnAndroidEmbed — it gets silently dropped. |
tham-le
force-pushed
the
tham/embed-android-support
branch
from
e5f0f8e to
724d454
Compare
embed.Client.Start() calls ConnectClient.Run() which passes an empty
MobileDependency{}. On Android, the engine dereferences nil fields
(IFaceDiscover, NetworkChangeListener, DnsReadyListener) causing panics.
Provide complete no-op stubs so the engine's existing Android code
paths work unchanged — zero modifications to engine.go:
- Add androidRunOverride hook in Run() for Android-specific dispatch
- Add runOnAndroidEmbed() with complete MobileDependency (all stubs)
- Wire default stubs via init() in connect_android_default.go:
noopIFaceDiscover, noopNetworkChangeListener, noopDnsReadyListener
- Forward logPath to c.run()
Tested: embed.Client starts on Android arm64, joins mesh via relay,
discovers peers, localhost proxy works for TCP+UDP forwarding.
Use filepath.Join in test assertions instead of hardcoded POSIX paths so the test passes on Windows where filepath.Join uses backslashes.
tham-le
force-pushed
the
tham/embed-android-support
branch
from
26f2a7c to
3a6d094
Compare
|
pappz
approved these changes
Apr 1, 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
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
embed.Client.Start()crashes on Android with nil pointer dereferences becauseConnectClient.Run()passes an emptyMobileDependency{}. The engine then dereferences nilIFaceDiscover,NetworkChangeListener,DnsReadyListener, andTunAdapter.This PR makes
embed.Clientusable on Android with netstack/userspace mode (NB_USE_NETSTACK_MODE=true), without requiringVpnService,TunAdapter, or root access.Approach
Provide complete no-op stubs for all mobile dependencies so the engine's existing Android code paths work unchanged — zero modifications to engine.go.
On Android builds, an
init()inconnect_android_default.gosetsandroidRunOverridewhich routesRun()throughrunOnAndroidEmbed()with complete stubs:noopIFaceDiscover— returns empty interface list (relay-only, no P2P ICE)noopNetworkChangeListener— ignores network change eventsnoopDnsReadyListener— ignores DNS readiness notificationsThe engine sees a fully populated
MobileDependencyand behaves exactly like the real Android app.Changes (3 files, no engine.go modifications)
connect.go: AddandroidRunOverridehook inRun()— checked before the defaultc.run(MobileDependency{})pathconnect_android_default.go(new): Defaultinit()wires complete no-op stubs viaandroidRunOverride. IncludesnoopIFaceDiscover,noopNetworkChangeListener,noopDnsReadyListener.connect_android_embed.go(new): UnexportedrunOnAndroidEmbed()that accepts all mobile deps +runningChan+logPath, buildsMobileDependency, callsc.run()Testing
Tested on Android arm64 (Pixel 7 Pro, Android 15):
embed.New()+Start()succeedsNon-Breaking
androidRunOverrideis nil,Run()behaves exactly as beforeMobileDependencyIssue ticket number and link
N/A — new feature, no existing issue.
Stack
Checklist
Documentation
Select exactly one:
Internal engine change with no user-facing API modification. The embed package API is unchanged — existing callers work as before. Android support is automatic via build tags.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
N/A
Summary by CodeRabbit
New Features
Tests