[client] Fix NetworkAddresses discovery on Android and re-sync on network change#5807
[client] Fix NetworkAddresses discovery on Android and re-sync on network change#5807MichaelUray wants to merge 10 commits intonetbirdio:mainfrom
Conversation
|
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 periodic and on-demand detection of local network-address changes in the Engine with debounced resyncs, platform-specific network interface discovery (Android and non-Android), an Android client callback to trigger immediate resyncs, tests for address equality, and a small .gitignore update. Changes
Sequence DiagramsequenceDiagram
actor Platform as Platform/Timer
participant Engine
participant System
participant MgmtClient as ManagementClient
Platform->>Engine: OnUnderlyingNetworkChanged() or 10s timer
activate Engine
Engine->>Engine: resyncMetaIfNetworkChanged() (debounced)
Engine->>System: GetInfo(ctx) -> include NetworkAddresses
activate System
System-->>Engine: current NetworkAddresses
deactivate System
Engine->>Engine: networkAddressesEqual(cached, current)?
alt changed and not debounced
Engine->>MgmtClient: SyncMeta(info with UpdatedNetworkAddresses)
activate MgmtClient
MgmtClient-->>Engine: response
deactivate MgmtClient
Engine->>Engine: update cached lastNetworkAddresses & timestamp
else unchanged or debounced
Engine-->>Engine: skip sync
end
deactivate Engine
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/internal/engine.go (1)
1040-1086: German comment should be in English.Line 1092 contains a German comment:
// Sort-unabhängiger Vergleich: prüfe ob alle IPs aus a in b vorkommenThis should be in English for consistency with the rest of the codebase.
Proposed fix
func networkAddressesEqual(a, b []system.NetworkAddress) bool { if len(a) != len(b) { return false } - // Sort-unabhängiger Vergleich: prüfe ob alle IPs aus a in b vorkommen + // Order-independent comparison: check if all IPs from a exist in b bSet := make(map[string]struct{}, len(b))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/engine.go` around lines 1040 - 1086, Replace the German inline comment in the networkAddressesEqual function/logic with an English equivalent; specifically change "// Sort-unabhängiger Vergleich: prüfe ob alle IPs aus a in b vorkommen" to something like "// Order-independent comparison: check that all IPs from a are present in b" so the comment matches the rest of the codebase and clearly documents the behavior.
🤖 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/engine.go`:
- Around line 567-569: The network-address watcher goroutine started in the
engine initialization isn't being tracked by the engine's shutdown wait group,
so Stop() can return before it exits; modify the caller that launches
startNetworkAddressWatcher() to call e.shutdownWg.Add(1) immediately before
starting the goroutine and ensure startNetworkAddressWatcher() defers
e.shutdownWg.Done() at its top (mirroring patterns used in
receiveManagementEvents, receiveSignalEvents, and the WG interface monitor) so
the watcher is properly awaited during shutdown.
- Around line 213-216: The fields lastNetworkAddresses and
lastNetworkAddressSync are accessed concurrently and must be protected by the
existing syncMsgMux; update startNetworkAddressWatcher (the periodic goroutine),
ResyncNetworkAddresses (external entry), and any other places that read/write
these fields to acquire syncMsgMux (RLock for readers, Lock for writers) around
all reads and writes, and keep handleSync using syncMsgMux as it already does;
ensure the lock is held during the entire read/compare/update sequences to
eliminate the data race.
---
Nitpick comments:
In `@client/internal/engine.go`:
- Around line 1040-1086: Replace the German inline comment in the
networkAddressesEqual function/logic with an English equivalent; specifically
change "// Sort-unabhängiger Vergleich: prüfe ob alle IPs aus a in b vorkommen"
to something like "// Order-independent comparison: check that all IPs from a
are present in b" so the comment matches the rest of the codebase and clearly
documents the behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09e1d26e-92c6-4658-9d1e-1aec42bc7b48
📒 Files selected for processing (9)
.gitignoreclient/android/client.goclient/internal/engine.goclient/internal/network_address_sync_test.goclient/system/info.goclient/system/info_android.goclient/system/info_test.goclient/system/network_addresses.goclient/system/network_addresses_android.go
eec555c to
70129c0
Compare
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/engine.go`:
- Around line 895-897: The resync path in e.resyncMetaIfNetworkChanged() is
currently using system.GetInfo(e.ctx) which omits Info.Files populated by
GetInfoWithChecks(e.ctx, e.checks); update e.resyncMetaIfNetworkChanged() to
call GetInfoWithChecks(...) to build the SyncMeta payload (including
files/process posture), call SyncMeta with that payload, and only update/commit
lastNetworkAddresses or any snapshot state after SyncMeta returns success;
ensure handleSync() callers rely on this new behavior so a failed SyncMeta
during a network handoff does not advance lastNetworkAddresses and thus will
retry on subsequent polls.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c25c432-bf5c-42a5-b355-bee1b24d0de5
📒 Files selected for processing (9)
.gitignoreclient/android/client.goclient/internal/engine.goclient/internal/network_address_sync_test.goclient/system/info.goclient/system/info_android.goclient/system/info_test.goclient/system/network_addresses.goclient/system/network_addresses_android.go
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- client/system/network_addresses.go
- client/system/network_addresses_android.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/system/info_android.go
- client/system/info_test.go
- client/internal/network_address_sync_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/internal/engine.go (1)
1101-1102: Consider usingslices.Clonefor defensive slice copying.Line 1101 assigns the slice reference directly. While currently safe since
info.NetworkAddressesis freshly allocated innetworkAddresses(), usingslices.Clonewould protect against future implementation changes that might reuse or cache slices.♻️ Proposed defensive fix
if err := e.mgmClient.SyncMeta(info); err != nil { log.Warnf("failed to re-sync meta after network change: %v", err) return } - e.lastNetworkAddresses = current + e.lastNetworkAddresses = slices.Clone(current) e.lastNetworkAddressSync = time.Now()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/engine.go` around lines 1101 - 1102, The assignment e.lastNetworkAddresses = current copies the slice header only; change it to a defensive copy using slices.Clone to avoid sharing the underlying array if networkAddresses() implementation changes. Replace the direct assignment in the method updating e.lastNetworkAddresses with e.lastNetworkAddresses = slices.Clone(current) (and ensure the slices package is imported), so the engine keeps its own independent slice rather than a shared reference.
🤖 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/engine.go`:
- Around line 1101-1102: The assignment e.lastNetworkAddresses = current copies
the slice header only; change it to a defensive copy using slices.Clone to avoid
sharing the underlying array if networkAddresses() implementation changes.
Replace the direct assignment in the method updating e.lastNetworkAddresses with
e.lastNetworkAddresses = slices.Clone(current) (and ensure the slices package is
imported), so the engine keeps its own independent slice rather than a shared
reference.
| ) | ||
|
|
||
| func getNetInterfaces() ([]net.Interface, error) { | ||
| return anet.Interfaces() |
There was a problem hiding this comment.
Did you consider to use stdnet.ExternalIFaceDiscover from mobile_dependencies.go?
There was a problem hiding this comment.
f85fe12 to
9cd9dc4
Compare
Address @pappz review on PR netbirdio#5807: instead of pulling in the github.com/wlynxg/anet third-party package to work around the broken net.Interfaces() on Android 11+, reuse the existing stdnet.ExternalIFaceDiscover hook that the host application (android-client) already provides via mobile_dependencies. How it works: - system/info.go gets a new context key IFaceDiscoverCtxKey carrying an IFaceDiscoverFunc -- a callback that returns the same newline-separated interface description string used by stdnet/discover_mobile.go. - system/network_addresses_android.go parses that description into []net.Interface and a per-call map of addresses, stashed back in the context so getInterfaceAddrs() can return them without a second IFaces() round-trip. When no discoverer is injected (e.g. unit tests on a desktop machine) it falls back to net.Interfaces() so callers never crash. - system/network_addresses.go gains a no-op WithIFaceDiscover so the engine can call it unconditionally on every platform. - internal/engine.go has a small systemCtx() helper that wraps e.ctx with the IFaceDiscover from e.mobileDep.IFaceDiscover, and every system.GetInfo / system.GetInfoWithChecks call now uses e.systemCtx() instead of e.ctx directly. - The wlynxg/anet dependency is no longer referenced from any first party code; go.mod still lists it as an indirect dependency from an unrelated transitive use, which is fine. Note: the existing mobile_dependencies.go IFaces format does not yet include the hardware MAC, so on Android the interfaces parsed via the discoverer have an empty HardwareAddr and are filtered out by the "skip iface without MAC" check that landed in upstream commit bb85eee. Populating the MAC requires a parallel change in the android-client repository to extend the format string. That is the reason the existing posture-check evaluation for Android still relies on the addresses reported through SyncMeta, which works once the host application updates its IFaces() implementation to include the MAC. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/engine.go`:
- Around line 1073-1087: The code calls system.GetInfoWithChecks(e.systemCtx(),
e.checks) (which runs heavy file/process posture checks) before comparing
addresses, causing repeated posture scans even when NetworkAddresses haven't
changed; change the logic to first fetch only the lightweight network info
(e.g., call a method that returns network addresses without running checks or
call system.GetInfo with checks disabled) and compare against
e.lastNetworkAddresses using networkAddressesEqual, and only if addresses differ
then call GetInfoWithChecks (or trigger checkFileAndProcess) and proceed to
SyncMeta to advance lastNetworkAddressSync; reference GetInfoWithChecks,
networkAddressesEqual, e.lastNetworkAddresses, e.systemCtx(), and SyncMeta in
your change.
In `@client/system/network_addresses_android.go`:
- Around line 107-108: The warnings in the Android network parsing code leak raw
interface payloads by interpolating variables line and addr into log.Warnf
calls; change these log statements to avoid printing the raw payloads (either
remove the %q interpolation and log a generic message like "payload redacted" or
replace the payload with a stable short hash/fingerprint) and apply the same
change to the other log site that prints addr so no CIDRs or interface names are
emitted; update the log.Warnf calls that reference line and addr to use redacted
or hashed values instead.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1031b50-f2d4-4857-aa02-3fd3b07f8f8a
📒 Files selected for processing (12)
.gitignoreclient/android/client.goclient/internal/engine.goclient/internal/network_address_sync_test.goclient/system/info.goclient/system/info_android.goclient/system/info_darwin.goclient/system/info_linux.goclient/system/info_test.goclient/system/info_windows.goclient/system/network_addresses.goclient/system/network_addresses_android.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- client/system/info_android.go
- client/internal/network_address_sync_test.go
- client/system/network_addresses.go
- client/system/info_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/android/client.go (1)
164-169: Optional: extract IFaceDiscover context wiring into a shared helper.
RunandRunWithoutLoginnow carry identical injection logic; a helper would reduce drift risk.♻️ Suggested refactor
+func (c *Client) withIFaceDiscover(ctx context.Context) context.Context { + if c.iFaceDiscover == nil { + return ctx + } + return system.WithIFaceDiscover(ctx, func() (string, error) { + return c.iFaceDiscover.IFaces() + }) +}- if c.iFaceDiscover != nil { - ctxWithValues = system.WithIFaceDiscover(ctxWithValues, func() (string, error) { - return c.iFaceDiscover.IFaces() - }) - } + ctxWithValues = c.withIFaceDiscover(ctxWithValues)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/android/client.go` around lines 164 - 169, Extract the repeated IFaceDiscover context wiring in Run and RunWithoutLogin into a small helper (e.g., attachIFaceDiscover(ctx, c)) that checks c.iFaceDiscover and, if non-nil, calls system.WithIFaceDiscover(ctx, func() (string, error) { return c.iFaceDiscover.IFaces() }), then returns the new context; replace the duplicated blocks in both Run and RunWithoutLogin with a call to this helper to centralize behavior and reduce drift.
🤖 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/android/client.go`:
- Around line 164-169: Extract the repeated IFaceDiscover context wiring in Run
and RunWithoutLogin into a small helper (e.g., attachIFaceDiscover(ctx, c)) that
checks c.iFaceDiscover and, if non-nil, calls system.WithIFaceDiscover(ctx,
func() (string, error) { return c.iFaceDiscover.IFaces() }), then returns the
new context; replace the duplicated blocks in both Run and RunWithoutLogin with
a call to this helper to centralize behavior and reduce drift.
9113ec3 to
5b3905c
Compare
… network change Android 10+ restricts MAC addresses and Go's net.Interfaces() is broken on Android 11+ due to SELinux restrictions on netlink sockets. Use wlynxg/anet as drop-in replacement on Android. Remove MAC-based interface filter. Add 10s network address watcher with 30s debounce to re-sync with management server on WiFi/cellular transitions. Fixes netbirdio#3614 netbirdio#2962
- Use GetInfoWithChecks(ctx, e.checks) in resyncMetaIfNetworkChanged so posture-check context (Info.Files) is included in SyncMeta after a network change, matching receivedByNewCheckList() behaviour. - Only advance e.lastNetworkAddresses/e.lastNetworkAddressSync after a successful SyncMeta so a transient management failure during a network handoff is retried on the next polling cycle. - Translate remaining German comment in networkAddressesEqual to English for consistency with the rest of the codebase. - Document that resyncMetaIfNetworkChanged must be called while holding syncMsgMux. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address @pappz review on PR netbirdio#5807: instead of pulling in the github.com/wlynxg/anet third-party package to work around the broken net.Interfaces() on Android 11+, reuse the existing stdnet.ExternalIFaceDiscover hook that the host application (android-client) already provides via mobile_dependencies. How it works: - system/info.go gets a new context key IFaceDiscoverCtxKey carrying an IFaceDiscoverFunc -- a callback that returns the same newline-separated interface description string used by stdnet/discover_mobile.go. - system/network_addresses_android.go parses that description into []net.Interface and a per-call map of addresses, stashed back in the context so getInterfaceAddrs() can return them without a second IFaces() round-trip. When no discoverer is injected (e.g. unit tests on a desktop machine) it falls back to net.Interfaces() so callers never crash. - system/network_addresses.go gains a no-op WithIFaceDiscover so the engine can call it unconditionally on every platform. - internal/engine.go has a small systemCtx() helper that wraps e.ctx with the IFaceDiscover from e.mobileDep.IFaceDiscover, and every system.GetInfo / system.GetInfoWithChecks call now uses e.systemCtx() instead of e.ctx directly. - The wlynxg/anet dependency is no longer referenced from any first party code; go.mod still lists it as an indirect dependency from an unrelated transitive use, which is fine. Note: the existing mobile_dependencies.go IFaces format does not yet include the hardware MAC, so on Android the interfaces parsed via the discoverer have an empty HardwareAddr and are filtered out by the "skip iface without MAC" check that landed in upstream commit bb85eee. Populating the MAC requires a parallel change in the android-client repository to extend the format string. That is the reason the existing posture-check evaluation for Android still relies on the addresses reported through SyncMeta, which works once the host application updates its IFaces() implementation to include the MAC. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…twork addresses system.GetInfo() is called during login (auth.go doMgmLogin) before the Engine starts. Without the discoverer in the context, the login sends empty NetworkAddresses to management, causing posture checks to fail on initial connect because management thinks the peer has no LAN address. The engine's systemCtx() already does this injection, but that only runs after login completes. By injecting into the root context in both Run() and RunWithoutLogin(), every code path that calls system.GetInfo() — including login — can discover Android network interfaces.
5b3905c to
24505d6
Compare
Replace magic number 10*time.Second with a named constant to satisfy SonarCloud code-smell check.
24505d6 to
b4a69c2
Compare
|
Thanks @pappz for the review! I have reworked the PR based on your feedback and the coderabbitai suggestions:
Ready for another look. |
|
@pappz friendly ping — all review feedback has been addressed. Ready for another look whenever you have time. Thanks! |
- Add dedicated networkAddrMu mutex for lastNetworkAddresses/ lastNetworkAddressSync fields (data race protection) - Skip debounce for explicit platform callbacks (force=true) so Android NetworkCallback network changes are never suppressed - Lightweight address-only check (NetworkAddresses()) before running full posture-check scans (GetInfoWithChecks), avoiding unnecessary file/process checks on every 10s watcher tick - Export system.NetworkAddresses() for callers that only need addresses - Redact raw interface names/CIDRs from Android parse warnings to prevent local network topology leaking into logs/support bundles
|
@MichaelUray Thank you for the PR! Network address watcher may be unnecessary The startNetworkAddressWatcher() (10s polling + 30s debounce) and ResyncNetworkAddresses() add significant complexity (new goroutine, two mutexes, debounce logic), but I believe the existing reconnection flow already covers this case. When the network changes (WiFi ↔ cellular), the network monitor triggers ErrResetConnection, which restarts the engine. The restart goes through login/sync, which sends fresh GetInfoWithChecks including up-to-date NetworkAddresses. Can you describe a scenario where the network addresses change but the network monitor doesn't fire? I prefer to remove this logic. The system info part looks correct (I did a quick test). I don’t prefer the context value pattern, but I don’t have a better idea at the moment. I hope that sooner or later the fix for the Android-related network interface syscalls will be merged into the Go standard library. |
|
@pappz You were right — the watcher is unnecessary. I've removed it entirely from the PR. Your comment prompted me to dig deeper into why the network change wasn't triggering an engine restart on Android. The root cause turned out to be on the Java side, not the Go side: Root Cause:
|
Two small follow-up fixes for CI: 1. FreeBSD build broke because info_freebsd.go still called the old networkAddresses() signature. Pass ctx like every other platform. 2. codespell flagged "unparseable" in the Android warning log. Switch to "unparsable" per codespell dictionary.
|
Addressed the three failing checks:
The remaining |
Upstream netbirdio#5900 added networkAddresses() to info_ios.go. Since this PR updates the networkAddresses() signature to require context (for the Android external iface discoverer), the iOS caller needs the same ctx argument.
|
Follow-up after syncing the latest upstream
CI status on the latest push32 of 33 required checks are green (including codespell, all docs checks, Quality Gate, Client (Docker) / Unit, Client Build on iOS / Windows / macOS / Linux, etc.). The one remaining red check is Client / Unit (macOS runner), which timed out after 5m00s inside |
…-addresses Resolved conflicts against upstream netbirdio#5906 (ios mac filter / network_addr.go extraction) and netbirdio#5888 (DebugBundle on Android client): - client/android/client.go: keep both OnUnderlyingNetworkChanged (ours) and DebugBundle (upstream), replaced non-ASCII arrow in comment with plain text. - client/system/info.go: drop duplicated networkAddresses/isDuplicated - they now live in client/system/network_addr.go after upstream extraction. - client/system/network_addr.go: adopt upstream's toNetworkAddress helper but keep ctx-aware signature + skipNoMacFilter so Android continues to use the external iFace discoverer. - client/system/info_ios.go: add exported NetworkAddresses(ctx) shim so the engine call compiles on ios; the iOS body stays context-free (iOS has no external discoverer).
|



Summary
Two issues prevented posture checks from working on Android:
net.Interfaces()broken on Android 11+: SELinux blocksNETLINK_ROUTEsockets (golang/go#40569). Usewlynxg/anet(already an indirect dependency via pion/ice) as drop-in replacement on Android.HardwareAddrfor all interfaces, causing the MAC-based filter to skip everything. Removed the filter since loopback is already filtered by IP and MAC is only metadata not used for posture check matching.SyncMeta.Includes unit tests for
networkAddressesEqual()andnetworkAddresses().Fixes #3614 #2962
Checklist
By submitting this pull request, I confirm that I have read and agree to the terms of the Contributor License Agreement.
Related Issues
Related #3968 — Posture checks peer network range failed on iPhone (Android side)
Related #4657 — iOS Client loses all routes when Posture Checks enabled (Android side)
Related #5810 — Feature Request: Expose peer LAN network addresses via API
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores