From a7d85ff3ab88de9154cc1d41e200cfda8dab3bf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Mon, 22 Jun 2026 19:30:59 +0200 Subject: [PATCH] [client] Resolve management cache domains asynchronously MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UpdateFromServerDomains resolved the infrastructure domains (signal, relay, STUN, TURN) synchronously, in a serial loop with a 5s timeout per domain. It runs deep inside handleSync, under the engine syncMsgMux (and the DNS server mutex). When DNS is unhealthy each lookup waits out its full timeout, so the sync lock is held for many seconds — observed as a 16.6s wait for the lock by a signal handler in the field. That starves the signal/p2p processing that shares syncMsgMux, which in turn prevents the handshake that would make DNS healthy again: a self-reinforcing loop. The cache cannot simply be skipped: it is a prerequisite for the relay connection. The relay dials its server by hostname, resolved through the NetBird DNS chain where this resolver sits on top (PriorityMgmtCache); on a miss it falls through to upstream — the dead path the cache exists to bypass. Instead, record intent on sync and resolve on demand: - UpdateFromServerDomains now marks each requested domain pending and kicks off resolution in the background via a dedicated singleflight group (resolveGroup), then returns immediately. No DNS I/O, no timeout, no blocking under the lock. - ServeDNS, on a miss for a pending domain, waits on the in-flight resolve (joining the same singleflight flight, bounded by dnsTimeout) instead of falling through to the dead upstream. The wait now happens in the relay/DNS-serving goroutine, never under syncMsgMux. - UpdateServerConfig registers the cache handler from the requested domains (RequestedDomains) rather than the already-resolved ones, so the resolver owns the names before resolution completes and can intercept the lookup. Background resolves use context.Background() so a fast-returning sync cannot cancel an in-flight resolve. WaitForPendingResolves lets tests (and any warm-cache path) wait for the background work to settle. Cache semantics are otherwise unchanged: TTL, stale-while-revalidate refresh, backoff, and the flow-domain exclusion all stay. --- client/internal/dns/mgmt/mgmt.go | 182 +++++++++++++++++++++++--- client/internal/dns/mgmt/mgmt_test.go | 6 + client/internal/dns/server.go | 7 +- 3 files changed, 178 insertions(+), 17 deletions(-) diff --git a/client/internal/dns/mgmt/mgmt.go b/client/internal/dns/mgmt/mgmt.go index 988e427fb..921664601 100644 --- a/client/internal/dns/mgmt/mgmt.go +++ b/client/internal/dns/mgmt/mgmt.go @@ -50,17 +50,34 @@ 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. +type pendingEntry struct{} + // Resolver caches critical NetBird infrastructure domains. -// records, refreshing, mgmtDomain and serverDomains are all guarded by mutex. +// records, refreshing, pending, mgmtDomain and serverDomains are all guarded by mutex. type Resolver struct { records map[dns.Question]*cachedRecord mgmtDomain *domain.Domain 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 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 singleflight.Group // refreshing tracks questions whose refresh is running via the OS // fallback path. A ServeDNS hit for a question in this map indicates @@ -78,6 +95,7 @@ func NewResolver() *Resolver { return &Resolver{ records: make(map[dns.Question]*cachedRecord), refreshing: make(map[dns.Question]*atomic.Bool), + pending: make(map[string]pendingEntry), cacheTTL: resolveCacheTTL(), } } @@ -117,6 +135,7 @@ func (m *Resolver) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { m.mutex.RLock() cached, found := m.records[question] inflight := m.refreshing[question] + _, isPending := m.pending[question.Name] var shouldRefresh bool if found { stale := time.Since(cached.cachedAt) > m.cacheTTL @@ -126,8 +145,20 @@ func (m *Resolver) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { m.mutex.RUnlock() if !found { - m.continueToNext(w, r) - return + // 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. + if isPending && m.awaitPendingResolve(question.Name) { + m.mutex.RLock() + cached, found = m.records[question] + m.mutex.RUnlock() + } + if !found { + m.continueToNext(w, r) + return + } } if inflight != nil && inflight.CompareAndSwap(false, true) { @@ -467,6 +498,15 @@ 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. +func (m *Resolver) RequestedDomains(serverDomains dnsconfig.ServerDomains) domain.List { + return m.extractDomainsFromServerDomains(serverDomains) +} + // GetCachedDomains returns a list of all cached domains. func (m *Resolver) GetCachedDomains() domain.List { m.mutex.RLock() @@ -489,7 +529,17 @@ func (m *Resolver) GetCachedDomains() domain.List { // UpdateFromServerDomains updates the cache with server domains from network configuration. // 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. -func (m *Resolver) UpdateFromServerDomains(ctx context.Context, serverDomains dnsconfig.ServerDomains) (domain.List, error) { +// 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. +func (m *Resolver) UpdateFromServerDomains(_ context.Context, serverDomains dnsconfig.ServerDomains) (domain.List, error) { newDomains := m.extractDomainsFromServerDomains(serverDomains) var removedDomains domain.List @@ -507,11 +557,122 @@ func (m *Resolver) UpdateFromServerDomains(ctx context.Context, serverDomains dn removedDomains = m.removeStaleDomains(currentDomains, allDomains) } - m.addNewDomains(ctx, newDomains) + m.kickoffResolve(newDomains) 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. +func (m *Resolver) kickoffResolve(domains domain.List) { + for _, d := range domains { + dnsName := strings.ToLower(dns.Fqdn(d.PunycodeString())) + + m.mutex.Lock() + _, hasPending := m.pending[dnsName] + cached := m.hasFreshRecordLocked(dnsName) + if !hasPending && !cached { + m.pending[dnsName] = pendingEntry{} + } + m.mutex.Unlock() + + if hasPending || cached { + continue + } + + m.scheduleInitialResolve(d, dnsName) + } +} + +// 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. +func (m *Resolver) scheduleInitialResolve(d domain.Domain, dnsName string) { + key := "initial|" + dnsName + _ = m.resolveGroup.DoChan(key, func() (any, error) { + defer m.clearPending(dnsName) + if err := m.AddDomain(context.Background(), d); err != nil { + log.Warnf("failed to add/update domain=%s: %v", d.SafeString(), err) + return struct{}{}, err + } + log.Debugf("added/updated management cache domain=%s", d.SafeString()) + return struct{}{}, nil + }) +} + +// hasFreshRecordLocked reports whether a non-stale A or AAAA record exists for +// the name. Caller holds m.mutex. +func (m *Resolver) hasFreshRecordLocked(dnsName string) bool { + for _, qtype := range []uint16{dns.TypeA, dns.TypeAAAA} { + q := dns.Question{Name: dnsName, Qtype: qtype, Qclass: dns.ClassINET} + if c, ok := m.records[q]; ok && time.Since(c.cachedAt) <= m.cacheTTL { + return true + } + } + return false +} + +func (m *Resolver) clearPending(dnsName string) { + m.mutex.Lock() + delete(m.pending, dnsName) + m.mutex.Unlock() +} + +// pendingCount returns the number of domains whose initial resolve is still in +// flight. Primarily useful for tests that need to wait for background resolves +// kicked off by UpdateFromServerDomains. +func (m *Resolver) pendingCount() int { + m.mutex.RLock() + defer m.mutex.RUnlock() + return len(m.pending) +} + +// WaitForPendingResolves blocks until all background initial resolves kicked +// off by UpdateFromServerDomains have settled, or the timeout elapses. It +// returns true if all settled. Intended for tests and shutdown paths that need +// the cache populated synchronously. +func (m *Resolver) WaitForPendingResolves(timeout time.Duration) bool { + deadline := time.Now().Add(timeout) + for m.pendingCount() > 0 { + if time.Now().After(deadline) { + return false + } + time.Sleep(10 * time.Millisecond) + } + return true +} + +// 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. +func (m *Resolver) awaitPendingResolve(dnsName string) bool { + key := "initial|" + dnsName + d, err := domain.FromString(strings.TrimSuffix(dnsName, ".")) + if err != nil { + return false + } + + ch := m.resolveGroup.DoChan(key, func() (any, error) { + defer m.clearPending(dnsName) + if err := m.AddDomain(context.Background(), d); err != nil { + return struct{}{}, err + } + return struct{}{}, nil + }) + + select { + case <-ch: + case <-time.After(dnsTimeout): + return false + } + + m.mutex.RLock() + defer m.mutex.RUnlock() + return m.hasFreshRecordLocked(dnsName) +} + // removeStaleDomains removes cached domains not present in the target domain list. // Management domains are preserved and never removed during server domain updates. func (m *Resolver) removeStaleDomains(currentDomains, newDomains domain.List) domain.List { @@ -577,17 +738,6 @@ func (m *Resolver) isManagementDomain(domain domain.Domain) bool { return m.mgmtDomain != nil && domain == *m.mgmtDomain } -// addNewDomains resolves and caches all domains from the update -func (m *Resolver) addNewDomains(ctx context.Context, newDomains domain.List) { - for _, newDomain := range newDomains { - if err := m.AddDomain(ctx, newDomain); err != nil { - log.Warnf("failed to add/update domain=%s: %v", newDomain.SafeString(), err) - } else { - log.Debugf("added/updated management cache domain=%s", newDomain.SafeString()) - } - } -} - func (m *Resolver) extractDomainsFromServerDomains(serverDomains dnsconfig.ServerDomains) domain.List { var domains domain.List diff --git a/client/internal/dns/mgmt/mgmt_test.go b/client/internal/dns/mgmt/mgmt_test.go index 276cbba0a..95c74a4a9 100644 --- a/client/internal/dns/mgmt/mgmt_test.go +++ b/client/internal/dns/mgmt/mgmt_test.go @@ -329,6 +329,7 @@ func TestResolver_ManagementDomainProtection(t *testing.T) { if err != nil { t.Logf("Server domains update failed: %v", err) } + resolver.WaitForPendingResolves(10 * time.Second) finalDomains := resolver.GetCachedDomains() @@ -366,6 +367,7 @@ func TestResolver_EmptyUpdateDoesNotRemoveDomains(t *testing.T) { if err != nil { t.Skipf("Skipping test due to DNS resolution failure: %v", err) } + resolver.WaitForPendingResolves(10 * time.Second) // Verify domains were added cachedDomains := resolver.GetCachedDomains() @@ -400,6 +402,7 @@ func TestResolver_PartialUpdateReplacesOnlyUpdatedTypes(t *testing.T) { if err != nil { t.Skipf("Skipping test due to DNS resolution failure: %v", err) } + resolver.WaitForPendingResolves(10 * time.Second) assert.Len(t, resolver.GetCachedDomains(), 3) // Update with partial ServerDomains (only signal domain - this should replace signal but preserve stun/turn) @@ -410,6 +413,7 @@ func TestResolver_PartialUpdateReplacesOnlyUpdatedTypes(t *testing.T) { if err != nil { t.Skipf("Skipping test due to DNS resolution failure: %v", err) } + resolver.WaitForPendingResolves(10 * time.Second) // Should remove only the old signal domain assert.Len(t, removedDomains, 1, "Should remove only the old signal domain") @@ -444,6 +448,7 @@ func TestResolver_PartialUpdateAddsNewTypePreservesExisting(t *testing.T) { if err != nil { t.Skipf("Skipping test due to DNS resolution failure: %v", err) } + resolver.WaitForPendingResolves(10 * time.Second) assert.Len(t, resolver.GetCachedDomains(), 3) // Update with partial ServerDomains (only flow domain - flow is intentionally excluded from @@ -455,6 +460,7 @@ func TestResolver_PartialUpdateAddsNewTypePreservesExisting(t *testing.T) { if err != nil { t.Skipf("Skipping test due to DNS resolution failure: %v", err) } + resolver.WaitForPendingResolves(10 * time.Second) assert.Len(t, removedDomains, 0, "Should not remove any domains when only flow domain is provided") diff --git a/client/internal/dns/server.go b/client/internal/dns/server.go index 7556c66cc..927484ae7 100644 --- a/client/internal/dns/server.go +++ b/client/internal/dns/server.go @@ -622,7 +622,12 @@ func (s *DefaultServer) UpdateServerConfig(domains dnsconfig.ServerDomains) erro s.deregisterHandler(removedDomains.ToPunycodeList(), PriorityMgmtCache) } - newDomains := s.mgmtCacheResolver.GetCachedDomains() + // 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. + newDomains := s.mgmtCacheResolver.RequestedDomains(domains) if len(newDomains) > 0 { s.registerHandler(newDomains.ToPunycodeList(), s.mgmtCacheResolver, PriorityMgmtCache) }