From b6c79f1f7179e41f9a9d1496c0187786be53b96f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Mon, 22 Jun 2026 19:46:26 +0200 Subject: [PATCH] [client] Trim verbose comments in mgmt cache async resolve --- client/internal/dns/mgmt/export_test.go | 8 ++-- client/internal/dns/mgmt/mgmt.go | 59 ++++++++----------------- client/internal/dns/server.go | 7 +-- 3 files changed, 24 insertions(+), 50 deletions(-) diff --git a/client/internal/dns/mgmt/export_test.go b/client/internal/dns/mgmt/export_test.go index 676b777ba..fcc5175f9 100644 --- a/client/internal/dns/mgmt/export_test.go +++ b/client/internal/dns/mgmt/export_test.go @@ -2,17 +2,15 @@ package mgmt import "time" -// pendingCount returns the number of domains whose initial resolve is still in -// flight. Test-only: lets tests wait for background resolves kicked off by -// UpdateFromServerDomains. +// pendingCount returns how many initial resolves are still in flight. Test-only. func (m *Resolver) pendingCount() int { m.mutex.RLock() defer m.mutex.RUnlock() return len(m.pending) } -// waitForPendingResolves blocks until all background initial resolves have -// settled, or the timeout elapses. Returns true if all settled. Test-only. +// waitForPendingResolves blocks until all pending resolves settle or the +// timeout elapses, returning true if all settled. Test-only. func (m *Resolver) waitForPendingResolves(timeout time.Duration) bool { deadline := time.Now().Add(timeout) for m.pendingCount() > 0 { diff --git a/client/internal/dns/mgmt/mgmt.go b/client/internal/dns/mgmt/mgmt.go index 6964a9bcb..23f1fb28f 100644 --- a/client/internal/dns/mgmt/mgmt.go +++ b/client/internal/dns/mgmt/mgmt.go @@ -50,11 +50,8 @@ type cachedRecord struct { consecFailures int } -// pendingEntry marks a domain whose initial resolution was kicked off by a -// network-map update but has not completed yet. It lets ServeDNS wait on the -// in-flight resolve (instead of falling through to the upstream, which may be -// the dead DNS path the cache exists to bypass) and lets handler registration -// claim the name before any record exists. +// pendingEntry marks a domain whose initial resolve is in flight, so ServeDNS +// can wait on it instead of falling through to upstream. type pendingEntry struct{} // Resolver caches critical NetBird infrastructure domains. @@ -65,18 +62,15 @@ type Resolver struct { serverDomains *dnsconfig.ServerDomains mutex sync.RWMutex - // pending holds domains whose initial resolution is in flight, keyed by - // the punycode FQDN (trailing dot). A ServeDNS miss for a pending domain - // waits on the resolve via resolveGroup instead of going to upstream. + // pending holds domains whose initial resolve is in flight, keyed by + // punycode FQDN (trailing dot). pending map[string]pendingEntry chain ChainResolver chainMaxPriority int refreshGroup singleflight.Group - // resolveGroup dedups initial (cold-cache) resolves kicked off by - // UpdateFromServerDomains and joined by ServeDNS waiters. Kept separate - // from refreshGroup so a stale-refresh and an initial-resolve for the same - // name don't collapse into one flight with mismatched semantics. + // resolveGroup dedups initial (cold-cache) resolves; kept separate from + // refreshGroup so initial and stale-refresh flights don't collapse. resolveGroup singleflight.Group // refreshing tracks questions whose refresh is running via the OS @@ -145,11 +139,8 @@ func (m *Resolver) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { m.mutex.RUnlock() if !found { - // A network-map update registered this domain but its initial resolve - // has not landed yet. Wait on the in-flight resolve rather than falling - // through to upstream, which may be the dead DNS path the cache exists - // to bypass (e.g. relay hostnames resolved while the DNS-carrying peer - // has no handshake). Bounded by dnsTimeout inside awaitPendingResolve. + // Registered but not resolved yet: wait on the in-flight resolve + // rather than falling through to (possibly dead) upstream. if isPending && m.awaitPendingResolve(question.Name) { m.mutex.RLock() cached, found = m.records[question] @@ -498,11 +489,9 @@ func (m *Resolver) RemoveDomain(d domain.Domain) error { return nil } -// RequestedDomains returns the cacheable infrastructure domains for the given -// server config (signal, relay, STUN, TURN; flow excluded). Used to register -// the cache handler for these names immediately, before resolution completes, -// so ServeDNS can intercept their lookups (and wait on a pending resolve) -// instead of letting them fall through to upstream. +// RequestedDomains returns the cacheable infrastructure domains (signal, relay, +// STUN, TURN; flow excluded) so the cache handler can be registered for them +// before resolution completes. func (m *Resolver) RequestedDomains(serverDomains dnsconfig.ServerDomains) domain.List { return m.extractDomainsFromServerDomains(serverDomains) } @@ -530,15 +519,9 @@ func (m *Resolver) GetCachedDomains() domain.List { // It merges new domains with existing ones, replacing entire domain types when updated. // Empty updates are ignored to prevent clearing infrastructure domains during partial updates. // UpdateFromServerDomains records the requested domains and kicks off their -// resolution in the background. It does NOT block on DNS: resolution is handed -// to resolveGroup and the actual waiting, if any, happens later on the -// ServeDNS path (when the relay/signal client looks the name up), not here. -// This keeps it off the engine sync lock, which a network-map update holds -// while calling this. -// -// The ctx is intentionally unused for resolution — background resolves use -// context.Background() with their own dnsTimeout so a fast-returning sync -// cannot cancel an in-flight resolve. +// resolution in the background, returning without blocking on DNS so it stays +// off the engine sync lock held by the caller. ctx is unused: background +// resolves use context.Background() so a fast-returning sync can't cancel them. func (m *Resolver) UpdateFromServerDomains(_ context.Context, serverDomains dnsconfig.ServerDomains) (domain.List, error) { newDomains := m.extractDomainsFromServerDomains(serverDomains) var removedDomains domain.List @@ -562,9 +545,8 @@ func (m *Resolver) UpdateFromServerDomains(_ context.Context, serverDomains dnsc return removedDomains, nil } -// kickoffResolve marks each domain pending and starts its resolution in the -// background via resolveGroup. Returns immediately; callers must not block on -// the result. A domain that already has a fresh cache entry is skipped. +// kickoffResolve marks each domain pending and starts a background resolve, +// skipping ones already fresh or in flight. Returns immediately. func (m *Resolver) kickoffResolve(domains domain.List) { for _, d := range domains { dnsName := strings.ToLower(dns.Fqdn(d.PunycodeString())) @@ -586,8 +568,7 @@ func (m *Resolver) kickoffResolve(domains domain.List) { } // scheduleInitialResolve runs AddDomain in the background, deduped per domain -// by resolveGroup. The pending marker is cleared once resolution finishes -// (success or failure) so ServeDNS stops waiting and a later update can retry. +// by resolveGroup, clearing the pending marker when it finishes. func (m *Resolver) scheduleInitialResolve(d domain.Domain, dnsName string) { key := "initial|" + dnsName _ = m.resolveGroup.DoChan(key, func() (any, error) { @@ -619,10 +600,8 @@ func (m *Resolver) clearPending(dnsName string) { m.mutex.Unlock() } -// awaitPendingResolve waits for an in-flight initial resolve of dnsName to -// finish, bounded by dnsTimeout. It joins the existing resolveGroup flight so -// concurrent ServeDNS waiters share one resolution. Returns true if a record -// became available. +// awaitPendingResolve joins the in-flight resolve for dnsName (bounded by +// dnsTimeout) and reports whether a record became available. func (m *Resolver) awaitPendingResolve(dnsName string) bool { key := "initial|" + dnsName d, err := domain.FromString(strings.TrimSuffix(dnsName, ".")) diff --git a/client/internal/dns/server.go b/client/internal/dns/server.go index 927484ae7..994ca076d 100644 --- a/client/internal/dns/server.go +++ b/client/internal/dns/server.go @@ -622,11 +622,8 @@ func (s *DefaultServer) UpdateServerConfig(domains dnsconfig.ServerDomains) erro s.deregisterHandler(removedDomains.ToPunycodeList(), PriorityMgmtCache) } - // Register the handler for the requested domains, not just the already - // resolved ones: UpdateFromServerDomains now resolves in the background, - // so the cache may still be empty here. Claiming the names up front lets - // the resolver's ServeDNS wait on the pending resolve instead of the - // lookup falling through to (possibly dead) upstream. + // Register for the requested domains, not just resolved ones: resolution + // now runs in the background, so the cache may still be empty here. newDomains := s.mgmtCacheResolver.RequestedDomains(domains) if len(newDomains) > 0 { s.registerHandler(newDomains.ToPunycodeList(), s.mgmtCacheResolver, PriorityMgmtCache)