[client] Add system DNS fallback for Windows, systemd-resolved, NetworkManager#6000
[client] Add system DNS fallback for Windows, systemd-resolved, NetworkManager#6000lixmal wants to merge 1 commit intodrop-dns-probesfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
lixmal
force-pushed
the
dns-fallback-capture-extension
branch
from
b35f0f3 to
534d193
Compare
lixmal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/dns/server.go (1)
754-785:⚠️ Potential issue | 🟠 MajorResource leak: previous fallback handler is never
Stop()-ed when replaced.Each call to
registerFallbackconstructs a newupstreamResolvervianewUpstreamResolver(s.ctx, ...)and registers it under(RootZone, PriorityFallback).handlerChain.AddHandlerwill replace any prior handler for the same domain/priority, but the displaced handler is dropped without callingStop(), so its goroutines/sockets keep running untils.ctxis cancelled at server shutdown. Compare withupdateMux(line 941), which explicitly callsexisting.handler.Stop()before replacement.This matters most on Android, where
OnUpdatedHostDNSServerinvokesregisterFallbackon every network-change event, so the leak grows with network churn. On other platforms it's bounded by the config-hash gate inapplyHostConfigbut still occurs whenever fallback re-registration happens during the daemon's lifetime. The same omission is present indisableDNS(line 510): the deregister doesn't stop the handler.Track the registered fallback handler on
sand stop the prior one before replacing. Sketch:♻️ Suggested fix
type DefaultServer struct { ... extraDomains map[domain.Domain]int batchMode bool + fallbackHandler handlerWithStop ... }func (s *DefaultServer) registerFallback() { originalNameservers := s.hostManager.getOriginalNameservers() if len(originalNameservers) == 0 { log.Debugf("no fallback upstreams to register; clearing PriorityFallback handler") s.deregisterHandler([]string{nbdns.RootZone}, PriorityFallback) + if s.fallbackHandler != nil { + s.fallbackHandler.Stop() + s.fallbackHandler = nil + } return } @@ handler.addRace(servers) + if s.fallbackHandler != nil { + s.fallbackHandler.Stop() + } + s.fallbackHandler = handler s.registerHandler([]string{nbdns.RootZone}, handler, PriorityFallback) }func (s *DefaultServer) disableDNS() (retErr error) { @@ if len(s.hostManager.getOriginalNameservers()) > 0 { log.Debugf("deregistering fallback handlers") s.deregisterHandler([]string{nbdns.RootZone}, PriorityFallback) } + if s.fallbackHandler != nil { + s.fallbackHandler.Stop() + s.fallbackHandler = nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/server.go` around lines 754 - 785, registerFallback currently creates a new upstreamResolver via newUpstreamResolver and registers it with registerHandler under (nbdns.RootZone, PriorityFallback) without stopping any prior handler, causing goroutine/socket leaks; modify DefaultServer to keep a reference to the currently-registered fallback handler (e.g., s.fallbackHandler *upstreamResolver or generic handler interface), and in registerFallback call Stop() on the existing s.fallbackHandler (or existing.handler.Stop()) before replacing it and assigning the new handler; also apply the same Stop() behavior in disableDNS where deregisterHandler can drop a handler so ensure the removed handler is stopped; locate uses of newUpstreamResolver, registerHandler, deregisterHandler, updateMux (existing.handler.Stop() sample) and disableDNS to mirror that pattern.
🧹 Nitpick comments (5)
client/internal/dns/network_manager_unix.go (2)
252-273: IPv6 NameserverData not consulted — acceptable for the supported NM range, worth a note.
readIPv6ConfigDNSreads only the legacyNameservers(aay). NM addedIP6Config.NameserverDatalater (≈NM 1.50). SincesupportedNetworkManagerVersionConstraintscaps at< 1.45, this is fine today, but the next time someone bumps the supported range past ~1.50 they'll need to mirror the v4 NameserverData/legacy fallback structure here. A short comment now will save that hunt later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/network_manager_unix.go` around lines 252 - 273, Add a short explanatory comment above readIPv6ConfigDNS stating that it intentionally only reads the legacy IP6Config.Nameservers ([]byte) and does not consult IP6Config.NameserverData (introduced around NetworkManager ~1.50) because supportedNetworkManagerVersionConstraints currently caps NM < 1.45; note that if the supported NM range is later bumped past ~1.50 the function should mirror the IPv4 logic that checks NameserverData with a legacy fallback. Reference the function name readIPv6ConfigDNS and the NM property IP6Config.NameserverData in the comment so future maintainers know what to change.
130-149: Same loopback / link-local filtering gap as Windows path.
captureOriginalNameserversfilters only invalid/unspecified addresses. NM-managed devices on hosts with a local stub resolver (systemd-resolved withdns=default, dnsmasq, etc.) will report127.0.0.53/127.0.0.1here, which then get registered as a "fallback upstream" — likely causing loops once Netbird's own resolver is up. IPv6 link-local entries fromNameserversare also unusable becauseaaycarries no scope id.♻️ Suggested filter
for _, addr := range readNetworkManagerDeviceDNS(dev) { addr = addr.Unmap() - if !addr.IsValid() || addr.IsUnspecified() { + if !addr.IsValid() || addr.IsUnspecified() || addr.IsLoopback() || addr.IsLinkLocalUnicast() || addr.IsMulticast() { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/network_manager_unix.go` around lines 130 - 149, The loop collecting NM device nameservers (in the function that iterates devices and calls readNetworkManagerDeviceDNS, skipping n.dbusLinkObject) only filters invalid/unspecified addresses — update that loop to also skip loopback addresses and IPv6 link-local addresses: call addr.IsLoopback() and addr.IsLinkLocalUnicast() (or equivalent checks) after Unmap() and before deduping so 127.0.0.x (including 127.0.0.1/127.0.0.53) and fe80::/10 entries are ignored; keep the existing seen dedupe logic and return behavior unchanged.client/internal/dns/host_ios.go (1)
18-24: Optional: prefernetip.MustParseAddrfor readability.The hand-encoded 16-byte IPv6 literal works (verified
2620:fe::fe→26 20 00 fe 00…00 fe), but it's easy to mis-count bytes when this is touched again.MustParseAddrmakes the intent obvious and matches the comment.♻️ Suggested refactor
func (a iosHostManager) getOriginalNameservers() []netip.Addr { // Quad9 v4+v6: 9.9.9.9, 2620:fe::fe. return []netip.Addr{ - netip.AddrFrom4([4]byte{9, 9, 9, 9}), - netip.AddrFrom16([16]byte{0x26, 0x20, 0x00, 0xfe, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xfe}), + netip.MustParseAddr("9.9.9.9"), + netip.MustParseAddr("2620:fe::fe"), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/host_ios.go` around lines 18 - 24, Replace the hand-encoded address literals in getOriginalNameservers with netip.MustParseAddr to make the intent clearer and avoid byte-count errors; specifically, change the netip.AddrFrom4 and netip.AddrFrom16 usages in iosHostManager.getOriginalNameservers to netip.MustParseAddr("9.9.9.9") and netip.MustParseAddr("2620:fe::fe") respectively, preserving return type and behavior and keeping the netip import.client/internal/dns/host_windows.go (1)
180-194: Consider filtering loopback and link-local addresses.
Unmap+IsUnspecifiedis the only filter. If the user previously had a local stub resolver (e.g.,NameServer=127.0.0.1to a Pi-hole/Stubby/etc., or::1), it gets captured as a "fallback upstream" and may cause resolution loops once Netbird's own listener is bound to a loopback address. Same applies to IPv6 link-localfe80::/10, which is unusable as a generic upstream because the registry value carries no zone/scope id.♻️ Suggested filter
addr = addr.Unmap() - if !addr.IsValid() || addr.IsUnspecified() { + if !addr.IsValid() || addr.IsUnspecified() || addr.IsLoopback() || addr.IsLinkLocalUnicast() || addr.IsMulticast() { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/host_windows.go` around lines 180 - 194, The parsing loop currently only filters with addr.Unmap() and addr.IsUnspecified(); update it to also skip loopback and IPv6 link-local addresses to avoid capturing local stub resolvers or zone-less fe80:: addresses. After addr = addr.Unmap() and before appending, add checks using netip methods (e.g., if addr.IsLoopback() || addr.IsLinkLocalUnicast() { continue }) so parsed addresses that are loopback (::1, 127.0.0.0/8) or link-local (fe80::/10) are excluded before out = append(out, addr).client/internal/dns/systemd_linux.go (1)
96-147: Optional: reuse the manager dbus object across captures.
captureOriginalNameserverscallsgetSystemdLinkPath(manager scope),isSystemdLinkDefaultRoute(link scope), andreadSystemdLinkDNS(link scope) for each interface, each of which opens and closes a fresh dbus connection viagetDbusObject. The constructor already has a manager-scopeobjopen at line 76 that could be threaded throughgetSystemdLinkPathto avoid one connection per interface, and a single per-link connection could serve bothisSystemdLinkDefaultRouteandreadSystemdLinkDNS. This is purely a startup-latency optimization and matches the existing per-call pattern in this file, so feel free to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/systemd_linux.go` around lines 96 - 147, captureOriginalNameservers currently opens a new D-Bus connection for every helper call (getSystemdLinkPath, isSystemdLinkDefaultRoute, readSystemdLinkDNS) via getDbusObject; to fix, thread the already-open manager-scoped D-Bus object (the constructor's obj) into getSystemdLinkPath so it reuses the manager connection, and open one per-link D-Bus connection once and pass that same per-link object into both isSystemdLinkDefaultRoute and readSystemdLinkDNS for that interface; update the signatures of those helper functions to accept a dbus object parameter and use it instead of calling getDbusObject, ensuring you still close per-link connections after processing each link while keeping the manager obj open for the whole captureOriginalNameservers run.
🤖 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/dns/server.go`:
- Around line 526-530: OnUpdatedHostDNSServer currently calls registerFallback
without holding s.mux, exposing s.hostManager to races; fix by taking s.mux
before any access to hostManager — either acquire s.mux at the start of
OnUpdatedHostDNSServer before calling registerFallback, or modify
registerFallback to lock s.mux internally; ensure the lock covers
getOriginalNameservers() and any reads of s.hostManager (references:
DefaultServer, OnUpdatedHostDNSServer, registerFallback, s.mux, hostManager,
getOriginalNameservers) and keep Initialize/enableDNS/disableDNS assignments
consistent under the same mutex.
---
Outside diff comments:
In `@client/internal/dns/server.go`:
- Around line 754-785: registerFallback currently creates a new upstreamResolver
via newUpstreamResolver and registers it with registerHandler under
(nbdns.RootZone, PriorityFallback) without stopping any prior handler, causing
goroutine/socket leaks; modify DefaultServer to keep a reference to the
currently-registered fallback handler (e.g., s.fallbackHandler *upstreamResolver
or generic handler interface), and in registerFallback call Stop() on the
existing s.fallbackHandler (or existing.handler.Stop()) before replacing it and
assigning the new handler; also apply the same Stop() behavior in disableDNS
where deregisterHandler can drop a handler so ensure the removed handler is
stopped; locate uses of newUpstreamResolver, registerHandler, deregisterHandler,
updateMux (existing.handler.Stop() sample) and disableDNS to mirror that
pattern.
---
Nitpick comments:
In `@client/internal/dns/host_ios.go`:
- Around line 18-24: Replace the hand-encoded address literals in
getOriginalNameservers with netip.MustParseAddr to make the intent clearer and
avoid byte-count errors; specifically, change the netip.AddrFrom4 and
netip.AddrFrom16 usages in iosHostManager.getOriginalNameservers to
netip.MustParseAddr("9.9.9.9") and netip.MustParseAddr("2620:fe::fe")
respectively, preserving return type and behavior and keeping the netip import.
In `@client/internal/dns/host_windows.go`:
- Around line 180-194: The parsing loop currently only filters with addr.Unmap()
and addr.IsUnspecified(); update it to also skip loopback and IPv6 link-local
addresses to avoid capturing local stub resolvers or zone-less fe80:: addresses.
After addr = addr.Unmap() and before appending, add checks using netip methods
(e.g., if addr.IsLoopback() || addr.IsLinkLocalUnicast() { continue }) so parsed
addresses that are loopback (::1, 127.0.0.0/8) or link-local (fe80::/10) are
excluded before out = append(out, addr).
In `@client/internal/dns/network_manager_unix.go`:
- Around line 252-273: Add a short explanatory comment above readIPv6ConfigDNS
stating that it intentionally only reads the legacy IP6Config.Nameservers
([]byte) and does not consult IP6Config.NameserverData (introduced around
NetworkManager ~1.50) because supportedNetworkManagerVersionConstraints
currently caps NM < 1.45; note that if the supported NM range is later bumped
past ~1.50 the function should mirror the IPv4 logic that checks NameserverData
with a legacy fallback. Reference the function name readIPv6ConfigDNS and the NM
property IP6Config.NameserverData in the comment so future maintainers know what
to change.
- Around line 130-149: The loop collecting NM device nameservers (in the
function that iterates devices and calls readNetworkManagerDeviceDNS, skipping
n.dbusLinkObject) only filters invalid/unspecified addresses — update that loop
to also skip loopback addresses and IPv6 link-local addresses: call
addr.IsLoopback() and addr.IsLinkLocalUnicast() (or equivalent checks) after
Unmap() and before deduping so 127.0.0.x (including 127.0.0.1/127.0.0.53) and
fe80::/10 entries are ignored; keep the existing seen dedupe logic and return
behavior unchanged.
In `@client/internal/dns/systemd_linux.go`:
- Around line 96-147: captureOriginalNameservers currently opens a new D-Bus
connection for every helper call (getSystemdLinkPath, isSystemdLinkDefaultRoute,
readSystemdLinkDNS) via getDbusObject; to fix, thread the already-open
manager-scoped D-Bus object (the constructor's obj) into getSystemdLinkPath so
it reuses the manager connection, and open one per-link D-Bus connection once
and pass that same per-link object into both isSystemdLinkDefaultRoute and
readSystemdLinkDNS for that interface; update the signatures of those helper
functions to accept a dbus object parameter and use it instead of calling
getDbusObject, ensuring you still close per-link connections after processing
each link while keeping the manager obj open for the whole
captureOriginalNameservers run.
🪄 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: 18c173ae-0b4a-4903-a98b-b626459a7f8e
📒 Files selected for processing (11)
client/internal/connect.goclient/internal/dns/host.goclient/internal/dns/host_android.goclient/internal/dns/host_ios.goclient/internal/dns/host_windows.goclient/internal/dns/network_manager_unix.goclient/internal/dns/server.goclient/internal/dns/server_android.goclient/internal/dns/systemd_linux.goclient/internal/engine.goclient/ios/NetBirdSDK/client.go
💤 Files with no reviewable changes (1)
- client/internal/connect.go
lixmal
force-pushed
the
dns-fallback-capture-extension
branch
4 times, most recently
from
3b5f857 to
b5be35c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (5)
client/internal/dns/network_manager_unix.go (2)
111-121: Snapshot-at-startup is fine, but consider documenting staleness.
origNameserversis captured once during constructor and never refreshed. If the user roams networks (e.g., switches Wi-Fi) after the daemon starts, the fallback list will continue pointing at the old upstreams until the daemon restarts. This matches the PR's documented "snapshot at startup" design and Android'sOnUpdatedHostDNSServeris the only refresh path, but a brief comment on the field (or here) would make the limitation obvious to future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/network_manager_unix.go` around lines 111 - 121, Add a brief comment explaining that origNameservers (set via captureOriginalNameservers in the NetworkManager constructor and assigned to c.origNameservers) is a snapshot taken at startup and is not refreshed if the host network changes; mention that roaming/Wi‑Fi switches will leave the fallback list stale until restart and that Android's OnUpdatedHostDNSServer is the only refresh path, so future maintainers know this is intentional rather than a bug.
256-272: Endianness assumption in IPv4 legacy nameserver decoding may limit portability.
readIPv4LegacyNameserversdecodes the deprecatedNameserversproperty usingbinary.LittleEndian.PutUint32to recover the on-wire bytes. NetworkManager stores IPv4 addresses in network byte order (big-endian), and this approach assumes a little-endian host where the D-Bus-decodeduint32carries the address in a representation thatbinary.LittleEndian.PutUint32can correctly byte-swap back to the original address bytes. On a big-endian platform, this would incorrectly byte-reverse the address.This matches the convention already used in
applyDNSConfig(line 313), and netbird's supported platforms (x86_64, ARM64 Linux/FreeBSD) are all little-endian, so it is not a functional bug. However, consider centralizing the conversion in a small helper or usingbinary.NativeEndianwith a clear comment documenting the endianness assumption, so the rationale is explicit in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/network_manager_unix.go` around lines 256 - 272, The readIPv4LegacyNameservers function assumes little-endian when reconstructing IPv4 bytes from the uint32 D-Bus value; extract the conversion into a small helper (e.g., decodeUint32ToIPv4 or nativeToIPv4) used by readIPv4LegacyNameservers and applyDNSConfig so the endianness handling is centralized, and add a brief comment in that helper explaining the deliberate little-endian assumption (or choice of binary.LittleEndian) and the supported-platform rationale; update both readIPv4LegacyNameservers and applyDNSConfig to call the helper so the behavior and rationale are explicit and maintained in one place.client/internal/dns/systemd_linux.go (1)
112-141: LGTM with minor note: D-Bus calls in capture path lack explicit timeouts.The capture pipeline (
getSystemdLinkPath→isSystemdLinkDefaultRoute→readSystemdLinkDNS) issues N×3 D-Bus calls viaobj.Call/obj.GetPropertywithout context timeouts, while the rest of the file usesCallWithContextwith a 5s timeout forapplyDNSConfigpaths. A wedged systemd-resolved would block constructor (and therefore engine startup) until it returns. This matches the pre-existingobj.Call(systemdDbusGetLinkMethod, ...)at Line 83, so consistency is preserved, but the multiplied call count amplifies the exposure.If you want to harden this without a larger refactor, you could wrap the per-link dbus reads in
CallWithContext/obj.GetPropertyWithContext-style helpers with a short overall budget.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/systemd_linux.go` around lines 112 - 141, captureOriginalNameservers currently performs multiple D-Bus calls per interface via getSystemdLinkPath, isSystemdLinkDefaultRoute and readSystemdLinkDNS using obj.Call/obj.GetProperty without timeouts; update these call sites so each per-link D-Bus interaction uses context-aware variants (e.g., CallWithContext/GetPropertyWithContext) with a short deadline (matching the 5s used for applyDNSConfig) or wrap the trio in a per-link context with a timeout, and add small helper functions if needed to centralize the CallWithContext/GetPropertyWithContext usage to avoid deadlocks when systemd-resolved is unresponsive.client/internal/dns/host.go (1)
143-145: Optional: MakemockHostConfigurator.getOriginalNameserversconfigurable for tests.Other methods on
mockHostConfiguratorare wired through*Funcfields (seeapplyDNSConfigFunc,restoreHostDNSFunc, etc.) so tests can inject custom behavior. The newgetOriginalNameserversreturns a hardcodednil, which prevents tests from exercising fallback registration paths through the mock. Consider mirroring the existing pattern for consistency.♻️ Proposed refactor
type mockHostConfigurator struct { applyDNSConfigFunc func(config HostDNSConfig, stateManager *statemanager.Manager) error restoreHostDNSFunc func() error supportCustomPortFunc func() bool restoreUncleanShutdownDNSFunc func(*netip.Addr) error stringFunc func() string + getOriginalNameserversFunc func() []netip.Addr } @@ func (m *mockHostConfigurator) getOriginalNameservers() []netip.Addr { + if m.getOriginalNameserversFunc != nil { + return m.getOriginalNameserversFunc() + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/host.go` around lines 143 - 145, mockHostConfigurator.getOriginalNameservers currently returns a hardcoded nil which prevents tests from injecting behavior; add a getOriginalNameserversFunc field to mockHostConfigurator and have getOriginalNameservers call that func if non-nil (falling back to returning nil only if the func is nil) so tests can provide custom nameserver lists; update usages of getOriginalNameservers to rely on the new configurable func.client/internal/dns/server.go (1)
770-816: Avoid rebuilding the fallback handler when upstreams are unchanged.
registerFallbackcreates a freshupstreamResolver, registers it, andStop()s the previous handler on every call — including fromapplyHostConfig, which runs on each management config apply. WhengetOriginalNameservers()returns the same set as before (the common case on desktop, where the snapshot is set once at startup), this still tears down and recreates the resolver, briefly racing requests against handler swap and discarding any in-flight upstream health/cache state.Consider tracking the last-registered nameserver set and short-circuiting when it matches:
♻️ Proposed refactor
+ // fallbackNameservers is the upstream set last registered at + // PriorityFallback; used to skip rebuilds when getOriginalNameservers + // returns an unchanged list. + fallbackNameservers []netip.Addr @@ func (s *DefaultServer) registerFallback() { originalNameservers := s.hostManager.getOriginalNameservers() if len(originalNameservers) == 0 { log.Debugf("no fallback upstreams to register; clearing PriorityFallback handler") s.clearFallback() return } + + if s.fallbackHandler != nil && slices.Equal(s.fallbackNameservers, originalNameservers) { + return + } @@ prev := s.fallbackHandler s.fallbackHandler = handler + s.fallbackNameservers = slices.Clone(originalNameservers) s.registerHandler([]string{nbdns.RootZone}, handler, PriorityFallback) if prev != nil { prev.Stop() } } func (s *DefaultServer) clearFallback() { s.deregisterHandler([]string{nbdns.RootZone}, PriorityFallback) if s.fallbackHandler != nil { s.fallbackHandler.Stop() s.fallbackHandler = nil + s.fallbackNameservers = nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/server.go` around lines 770 - 816, registerFallback currently always creates a new upstreamResolver and stops the previous one even when the original nameserver list hasn't changed, causing unnecessary handler swaps and lost state; modify DefaultServer (e.g., add a field like lastFallbackServers or lastFallbackNameservers) to store the canonical set of netip.AddrPort (or the originalNameservers slice) used for the active fallback, then at the start of registerFallback compare the new originalNameservers (or converted AddrPort slice) with that stored value and return early if they are equal, otherwise proceed to create/register the new handler and update the stored value; also handle the empty case by calling clearFallback and clearing the stored value, and ensure the stored value is updated/cleared when replacing or stopping the fallbackHandler (refer to registerFallback, clearFallback, s.fallbackHandler and s.hostManager.getOriginalNameservers).
🤖 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/dns/host.go`:
- Around line 143-145: mockHostConfigurator.getOriginalNameservers currently
returns a hardcoded nil which prevents tests from injecting behavior; add a
getOriginalNameserversFunc field to mockHostConfigurator and have
getOriginalNameservers call that func if non-nil (falling back to returning nil
only if the func is nil) so tests can provide custom nameserver lists; update
usages of getOriginalNameservers to rely on the new configurable func.
In `@client/internal/dns/network_manager_unix.go`:
- Around line 111-121: Add a brief comment explaining that origNameservers (set
via captureOriginalNameservers in the NetworkManager constructor and assigned to
c.origNameservers) is a snapshot taken at startup and is not refreshed if the
host network changes; mention that roaming/Wi‑Fi switches will leave the
fallback list stale until restart and that Android's OnUpdatedHostDNSServer is
the only refresh path, so future maintainers know this is intentional rather
than a bug.
- Around line 256-272: The readIPv4LegacyNameservers function assumes
little-endian when reconstructing IPv4 bytes from the uint32 D-Bus value;
extract the conversion into a small helper (e.g., decodeUint32ToIPv4 or
nativeToIPv4) used by readIPv4LegacyNameservers and applyDNSConfig so the
endianness handling is centralized, and add a brief comment in that helper
explaining the deliberate little-endian assumption (or choice of
binary.LittleEndian) and the supported-platform rationale; update both
readIPv4LegacyNameservers and applyDNSConfig to call the helper so the behavior
and rationale are explicit and maintained in one place.
In `@client/internal/dns/server.go`:
- Around line 770-816: registerFallback currently always creates a new
upstreamResolver and stops the previous one even when the original nameserver
list hasn't changed, causing unnecessary handler swaps and lost state; modify
DefaultServer (e.g., add a field like lastFallbackServers or
lastFallbackNameservers) to store the canonical set of netip.AddrPort (or the
originalNameservers slice) used for the active fallback, then at the start of
registerFallback compare the new originalNameservers (or converted AddrPort
slice) with that stored value and return early if they are equal, otherwise
proceed to create/register the new handler and update the stored value; also
handle the empty case by calling clearFallback and clearing the stored value,
and ensure the stored value is updated/cleared when replacing or stopping the
fallbackHandler (refer to registerFallback, clearFallback, s.fallbackHandler and
s.hostManager.getOriginalNameservers).
In `@client/internal/dns/systemd_linux.go`:
- Around line 112-141: captureOriginalNameservers currently performs multiple
D-Bus calls per interface via getSystemdLinkPath, isSystemdLinkDefaultRoute and
readSystemdLinkDNS using obj.Call/obj.GetProperty without timeouts; update these
call sites so each per-link D-Bus interaction uses context-aware variants (e.g.,
CallWithContext/GetPropertyWithContext) with a short deadline (matching the 5s
used for applyDNSConfig) or wrap the trio in a per-link context with a timeout,
and add small helper functions if needed to centralize the
CallWithContext/GetPropertyWithContext usage to avoid deadlocks when
systemd-resolved is unresponsive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2eb7969b-e64e-44e5-816a-ceb1cffab719
📒 Files selected for processing (12)
client/internal/connect.goclient/internal/dns/host.goclient/internal/dns/host_android.goclient/internal/dns/host_ios.goclient/internal/dns/host_windows.goclient/internal/dns/hosts_dns_holder.goclient/internal/dns/network_manager_unix.goclient/internal/dns/server.goclient/internal/dns/server_android.goclient/internal/dns/systemd_linux.goclient/internal/engine.goclient/ios/NetBirdSDK/client.go
💤 Files with no reviewable changes (1)
- client/internal/connect.go
✅ Files skipped from review due to trivial changes (2)
- client/internal/dns/hosts_dns_holder.go
- client/internal/dns/host_windows.go
🚧 Files skipped from review as they are similar to previous changes (4)
- client/internal/dns/host_ios.go
- client/internal/dns/server_android.go
- client/internal/engine.go
- client/internal/dns/host_android.go
lixmal
force-pushed
the
dns-fallback-capture-extension
branch
from
b5be35c to
733c147
Compare
lixmal
force-pushed
the
dns-fallback-capture-extension
branch
from
733c147 to
ad57a87
Compare
|
1 participant
Describe your changes
Snapshot the OS's DNS resolvers at start-up on the platforms where we didn't yet (Windows, systemd-resolved, NetworkManager) and register them as a low-priority DNS fallback, the same way macOS and the file/resolvconf paths already do. Also move the iOS public-DNS fallback into its host manager so it lives in the same fallback tier as every other platform.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Internal client-side change with no user-facing surface.
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
Release Notes