From d820aaa8fced831f23d3a4d17809b956e8fdc706 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Wed, 22 Jul 2026 05:22:28 +0000 Subject: [PATCH] [client] Address review comments on DNS lazy-connection warm-up - Use netip.Addr instead of string IPs in the local resolver's PeerConnectivity and PeerActivator interfaces; extract record addresses as netip.Addr (v4-mapped forms unmapped) and convert to string only at the peer.Status boundary. - Scope warm-up to match-only (non-authoritative) zones so the synthesized private-service records keep warming while plain peer-name lookups in the authoritative peer zone no longer wake every idle peer. - Parse NB_DNS_LAZY_WARMUP_TIMEOUT once in the resolver constructor instead of on every request, and log invalid values. - Stop reusing engine.syncMsgMux on the DNS path: ConnMgr.ActivatePeer is now safe for concurrent use (lazy manager pointer guarded by a dedicated RWMutex; the manager itself is internally synchronized), so network-map processing no longer blocks DNS resolution. - Add SetPeerActivator to the dns.Server interface (no-op on the mock) and drop the *dns.DefaultServer type assertion in the engine. - Drop WireGuard wording from dns/local comments; the layer is transport-agnostic. Tests: authoritative zones never trigger warm-up; warm-up timeout env parsing (valid/invalid/non-positive); extractRecordAddr unmaps v4-mapped record data; dnsPeerActivator skips connected/unknown/ conn-less peers, returns on connect, and releases at the budget when the peer stays idle; ConnMgr.ActivatePeer races the manager lifecycle cleanly under -race. --- client/internal/conn_mgr.go | 28 ++++- client/internal/conn_mgr_test.go | 66 +++++++++++ client/internal/dns/local/local.go | 111 +++++++++++------- client/internal/dns/local/local_test.go | 4 +- client/internal/dns/local/warmup_test.go | 115 ++++++++++++++++-- client/internal/dns/mock_server.go | 6 + client/internal/dns/server.go | 5 +- client/internal/dns_peer_activator.go | 27 +++-- client/internal/dns_peer_activator_test.go | 129 +++++++++++++++++++++ client/internal/engine.go | 15 +-- 10 files changed, 419 insertions(+), 87 deletions(-) create mode 100644 client/internal/dns_peer_activator_test.go diff --git a/client/internal/conn_mgr.go b/client/internal/conn_mgr.go index 77d1e6ca5..754ce37a3 100644 --- a/client/internal/conn_mgr.go +++ b/client/internal/conn_mgr.go @@ -34,6 +34,8 @@ const ( // - Handling connection establishment based on peer signaling // // The implementation is not thread-safe; it is protected by engine.syncMsgMux. +// The only exception is ActivatePeer, which is safe for concurrent use so the +// DNS warm-up path can call it without contending on the engine mutex. type ConnMgr struct { peerStore *peerstore.Store statusRecorder *peer.Status @@ -42,6 +44,10 @@ type ConnMgr struct { rosenpassEnabled bool lazyConnMgr *manager.Manager + // lazyConnMgrMu guards the lazyConnMgr pointer for readers outside the + // engine loop (ActivatePeer). Writers hold it in addition to + // engine.syncMsgMux; all other reads stay under engine.syncMsgMux only. + lazyConnMgrMu sync.RWMutex wg sync.WaitGroup lazyCtx context.Context @@ -238,12 +244,20 @@ func (e *ConnMgr) RemovePeerConn(peerKey string) { conn.Log.Infof("removed peer from lazy conn manager") } +// ActivatePeer wakes an idle lazy connection. Unlike the rest of ConnMgr it is +// safe for concurrent use: the lazy manager pointer is read under lazyConnMgrMu +// and the manager itself is internally synchronized, so callers outside the +// engine loop (DNS warm-up) do not need engine.syncMsgMux. func (e *ConnMgr) ActivatePeer(ctx context.Context, conn *peer.Conn) { - if !e.isStartedWithLazyMgr() { + e.lazyConnMgrMu.RLock() + lazyConnMgr := e.lazyConnMgr + started := lazyConnMgr != nil && e.lazyCtxCancel != nil + e.lazyConnMgrMu.RUnlock() + if !started { return } - if found := e.lazyConnMgr.ActivatePeer(conn.GetKey()); found { + if found := lazyConnMgr.ActivatePeer(conn.GetKey()); found { if err := conn.Open(ctx); err != nil { conn.Log.Errorf("failed to open connection: %v", err) } @@ -268,16 +282,21 @@ func (e *ConnMgr) Close() { e.lazyCtxCancel() e.wg.Wait() + + e.lazyConnMgrMu.Lock() e.lazyConnMgr = nil + e.lazyConnMgrMu.Unlock() } func (e *ConnMgr) initLazyManager(engineCtx context.Context) { cfg := manager.Config{ InactivityThreshold: inactivityThresholdEnv(), } - e.lazyConnMgr = manager.NewManager(cfg, engineCtx, e.peerStore, e.iface) + e.lazyConnMgrMu.Lock() + e.lazyConnMgr = manager.NewManager(cfg, engineCtx, e.peerStore, e.iface) e.lazyCtx, e.lazyCtxCancel = context.WithCancel(engineCtx) + e.lazyConnMgrMu.Unlock() e.wg.Add(1) go func() { @@ -316,7 +335,10 @@ func (e *ConnMgr) closeManager(ctx context.Context) { e.lazyCtxCancel() e.wg.Wait() + + e.lazyConnMgrMu.Lock() e.lazyConnMgr = nil + e.lazyConnMgrMu.Unlock() for _, peerID := range e.peerStore.PeersPubKey() { e.peerStore.PeerConnOpen(ctx, peerID) diff --git a/client/internal/conn_mgr_test.go b/client/internal/conn_mgr_test.go index 5e2c53e35..e027fd4f2 100644 --- a/client/internal/conn_mgr_test.go +++ b/client/internal/conn_mgr_test.go @@ -1,10 +1,21 @@ package internal import ( + "context" + "net" + "net/netip" "os" + "sync" "testing" + "time" + "golang.zx2c4.com/wireguard/wgctrl/wgtypes" + + "github.com/netbirdio/netbird/client/iface/wgaddr" "github.com/netbirdio/netbird/client/internal/lazyconn" + "github.com/netbirdio/netbird/client/internal/peer" + "github.com/netbirdio/netbird/client/internal/peerstore" + "github.com/netbirdio/netbird/monotime" ) func TestResolveLazyForce(t *testing.T) { @@ -38,3 +49,58 @@ func TestResolveLazyForce(t *testing.T) { }) } } + +type mockLazyWGIface struct{} + +func (mockLazyWGIface) RemovePeer(string) error { return nil } +func (mockLazyWGIface) UpdatePeer(string, []netip.Prefix, time.Duration, *net.UDPAddr, *wgtypes.Key) error { + return nil +} +func (mockLazyWGIface) IsUserspaceBind() bool { return false } +func (mockLazyWGIface) Address() wgaddr.Address { return wgaddr.Address{} } +func (mockLazyWGIface) LastActivities() map[string]monotime.Time { return nil } +func (mockLazyWGIface) MTU() uint16 { return 1280 } + +// TestConnMgr_ActivatePeerConcurrentWithLifecycle exercises ActivatePeer from +// non-engine goroutines (the DNS warm-up path) racing the manager lifecycle, +// which stays on the engine loop. Run with -race: it fails if ActivatePeer +// still requires engine.syncMsgMux for safety. +func TestConnMgr_ActivatePeerConcurrentWithLifecycle(t *testing.T) { + t.Setenv(lazyconn.EnvLazyConn, "on") + + status := peer.NewRecorder("https://mgm") + store := peerstore.NewConnStore() + connMgr := NewConnMgr(&EngineConfig{}, status, store, mockLazyWGIface{}) + + conn := newTestPeerConn(t, "peerA") + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + connMgr.Start(ctx) + + done := make(chan struct{}) + var wg sync.WaitGroup + for range 4 { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-done: + return + default: + connMgr.ActivatePeer(ctx, conn) + } + } + }() + } + + // Let the activators spin against the started manager, then tear it down + // underneath them and let them spin against the stopped manager. + time.Sleep(100 * time.Millisecond) + connMgr.Close() + time.Sleep(50 * time.Millisecond) + + close(done) + wg.Wait() +} diff --git a/client/internal/dns/local/local.go b/client/internal/dns/local/local.go index 667bd0cc2..cf6d59d40 100644 --- a/client/internal/dns/local/local.go +++ b/client/internal/dns/local/local.go @@ -37,31 +37,43 @@ type resolver interface { // record is left alone (it points at something outside our mesh, e.g. // a non-peer upstream). type PeerConnectivity interface { - IsConnectedByIP(ip string) (known, connected bool) + IsConnectedByIP(ip netip.Addr) (known, connected bool) } // PeerActivator wakes lazy-connection peers on demand. The local resolver calls // it with the tunnel IPs an answer points at, so a peer that is idle (lazily -// disconnected) begins its WireGuard handshake at DNS-resolution time rather -// than racing the client's first request packet. nil disables warm-up. +// disconnected) starts connecting at DNS-resolution time rather than racing the +// client's first request packet. nil disables warm-up. type PeerActivator interface { - // ActivatePeersByIP triggers wake-up for the peer(s) owning ips and blocks + // ActivatePeersByIP triggers wake-up for the peer(s) owning addrs and blocks // until one is connected or ctx (a short per-query budget) expires. It is a - // fast no-op for unknown or already-connected IPs. - ActivatePeersByIP(ctx context.Context, ips []string) + // fast no-op for unknown or already-connected addresses. + ActivatePeersByIP(ctx context.Context, addrs []netip.Addr) } -const defaultLazyWarmupTimeout = 2 * time.Second +const ( + defaultLazyWarmupTimeout = 2 * time.Second + envLazyWarmupTimeout = "NB_DNS_LAZY_WARMUP_TIMEOUT" +) -// lazyWarmupTimeout is the per-query budget for waking a lazy-connection peer a -// DNS answer points at. Tunable via NB_DNS_LAZY_WARMUP_TIMEOUT (a Go duration). -func lazyWarmupTimeout() time.Duration { - if v := os.Getenv("NB_DNS_LAZY_WARMUP_TIMEOUT"); v != "" { - if d, err := time.ParseDuration(v); err == nil && d > 0 { - return d - } +// lazyWarmupTimeoutFromEnv returns the per-query budget for waking a +// lazy-connection peer a DNS answer points at. Tunable via +// NB_DNS_LAZY_WARMUP_TIMEOUT (a Go duration). Parsed once at construction time. +func lazyWarmupTimeoutFromEnv() time.Duration { + v := os.Getenv(envLazyWarmupTimeout) + if v == "" { + return defaultLazyWarmupTimeout } - return defaultLazyWarmupTimeout + d, err := time.ParseDuration(v) + if err != nil { + log.Warnf("invalid %s value %q, using default %s: %v", envLazyWarmupTimeout, v, defaultLazyWarmupTimeout, err) + return defaultLazyWarmupTimeout + } + if d <= 0 { + log.Warnf("non-positive %s value %q, using default %s", envLazyWarmupTimeout, v, defaultLazyWarmupTimeout) + return defaultLazyWarmupTimeout + } + return d } type Resolver struct { @@ -79,6 +91,9 @@ type Resolver struct { // peerActivator, when non-nil, is called at resolution time to warm the // lazy connection to the peer(s) an answer points at. nil disables warm-up. peerActivator PeerActivator + // warmupTimeout is the per-query budget for the lazy-connection warm-up + // wait, resolved from the environment once at construction time. + warmupTimeout time.Duration ctx context.Context cancel context.CancelFunc @@ -87,11 +102,12 @@ type Resolver struct { func NewResolver() *Resolver { ctx, cancel := context.WithCancel(context.Background()) return &Resolver{ - records: make(map[dns.Question][]dns.RR), - domains: make(map[domain.Domain]struct{}), - zones: make(map[domain.Domain]bool), - ctx: ctx, - cancel: cancel, + records: make(map[dns.Question][]dns.RR), + domains: make(map[domain.Domain]struct{}), + zones: make(map[domain.Domain]bool), + warmupTimeout: lazyWarmupTimeoutFromEnv(), + ctx: ctx, + cancel: cancel, } } @@ -160,7 +176,7 @@ func (d *Resolver) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { result := d.lookupRecords(logger, question) // Warm before filtering: activation flips a lazily-idle target to connected, // which then lets it survive the disconnected-peer filter below. - d.warmLazyPeers(result.records) + d.warmLazyPeers(question, result.records) result.records = d.filterDisconnectedPeerAnswers(logger, question, result.records) replyMessage.Authoritative = !result.hasExternalData replyMessage.Answer = result.records @@ -534,8 +550,8 @@ func (d *Resolver) filterDisconnectedPeerAnswers(logger *log.Entry, question dns kept := make([]dns.RR, 0, len(records)) var dropped int for _, rr := range records { - ip := extractRecordIP(rr) - if ip == "" { + ip, ok := extractRecordAddr(rr) + if !ok { kept = append(kept, rr) continue } @@ -559,47 +575,52 @@ func (d *Resolver) filterDisconnectedPeerAnswers(logger *log.Entry, question dns // warmLazyPeers triggers lazy-connection wake-up for the peers a resolved // answer points at and waits briefly for one to connect, so the caller's first -// request doesn't race the WireGuard handshake. No-op when no activator is -// wired (lazy connections disabled) or the answer carries no peer IPs. -func (d *Resolver) warmLazyPeers(records []dns.RR) { +// request doesn't race the connection establishment. Warm-up is scoped to +// match-only (non-authoritative) zones — the synthesized private-service zones +// and user-created zones whose records point at specific peers. The account's +// peer zone is authoritative, so plain peer-name lookups never trigger warm-up; +// otherwise resolving any peer's name would wake its idle connection, defeating +// laziness mesh-wide. No-op when no activator is wired (lazy connections +// disabled) or the answer carries no peer IPs. +func (d *Resolver) warmLazyPeers(question dns.Question, records []dns.RR) { d.mu.RLock() activator := d.peerActivator + var nonAuth, found bool + if activator != nil { + nonAuth, found = d.findZone(question.Name) + } d.mu.RUnlock() - if activator == nil { + if activator == nil || !found || !nonAuth { return } - var ips []string + var addrs []netip.Addr for _, rr := range records { - if ip := extractRecordIP(rr); ip != "" { - ips = append(ips, ip) + if addr, ok := extractRecordAddr(rr); ok { + addrs = append(addrs, addr) } } - if len(ips) == 0 { + if len(addrs) == 0 { return } - ctx, cancel := context.WithTimeout(d.ctx, lazyWarmupTimeout()) + ctx, cancel := context.WithTimeout(d.ctx, d.warmupTimeout) defer cancel() - activator.ActivatePeersByIP(ctx, ips) + activator.ActivatePeersByIP(ctx, addrs) } -// extractRecordIP returns the dotted-decimal / colon-hex IP carried by -// an A or AAAA record, or "" for any other record type. -func extractRecordIP(rr dns.RR) string { +// extractRecordAddr returns the IP address carried by an A or AAAA record. +// ok is false for any other record type or a record with no address. +func extractRecordAddr(rr dns.RR) (netip.Addr, bool) { switch r := rr.(type) { case *dns.A: - if r.A == nil { - return "" - } - return r.A.String() + addr, ok := netip.AddrFromSlice(r.A) + return addr.Unmap(), ok case *dns.AAAA: - if r.AAAA == nil { - return "" - } - return r.AAAA.String() + addr, ok := netip.AddrFromSlice(r.AAAA) + return addr.Unmap(), ok } - return "" + return netip.Addr{}, false } // Update replaces all zones and their records diff --git a/client/internal/dns/local/local_test.go b/client/internal/dns/local/local_test.go index 9b7dac231..89e896c0a 100644 --- a/client/internal/dns/local/local_test.go +++ b/client/internal/dns/local/local_test.go @@ -37,8 +37,8 @@ type mockPeerConnectivity struct { byIP map[string]struct{ known, connected bool } } -func (m mockPeerConnectivity) IsConnectedByIP(ip string) (known, connected bool) { - v, ok := m.byIP[ip] +func (m mockPeerConnectivity) IsConnectedByIP(ip netip.Addr) (known, connected bool) { + v, ok := m.byIP[ip.String()] if !ok { return false, false } diff --git a/client/internal/dns/local/warmup_test.go b/client/internal/dns/local/warmup_test.go index 5c3408ec4..e0c97ba96 100644 --- a/client/internal/dns/local/warmup_test.go +++ b/client/internal/dns/local/warmup_test.go @@ -2,8 +2,11 @@ package local import ( "context" + "net" + "net/netip" "sync" "testing" + "time" "github.com/miekg/dns" "github.com/stretchr/testify/assert" @@ -13,19 +16,19 @@ import ( nbdns "github.com/netbirdio/netbird/dns" ) -// recordingActivator records the IPs it was asked to warm and returns +// recordingActivator records the addresses it was asked to warm and returns // immediately, so ServeDNS is not blocked by the test. type recordingActivator struct { mu sync.Mutex called bool - ips []string + addrs []netip.Addr } -func (r *recordingActivator) ActivatePeersByIP(_ context.Context, ips []string) { +func (r *recordingActivator) ActivatePeersByIP(_ context.Context, addrs []netip.Addr) { r.mu.Lock() defer r.mu.Unlock() r.called = true - r.ips = append(r.ips, ips...) + r.addrs = append(r.addrs, addrs...) } func serveA(t *testing.T, resolver *Resolver, name string) *dns.Msg { @@ -36,10 +39,21 @@ func serveA(t *testing.T, resolver *Resolver, name string) *dns.Msg { return resp } +// serviceZone registers rec in a match-only (non-authoritative) zone, the shape +// the synthesized private-service zones arrive in. +func serviceZone(t *testing.T, resolver *Resolver, zone string, records ...nbdns.SimpleRecord) { + t.Helper() + resolver.Update([]nbdns.CustomZone{{ + Domain: zone, + Records: records, + NonAuthoritative: true, + }}) +} + func TestLocalResolver_WarmsLazyPeerOnResolve(t *testing.T) { - rec := nbdns.SimpleRecord{Name: "svc.netbird.cloud.", Type: 1, Class: nbdns.DefaultClass, TTL: 300, RData: "100.64.0.7"} + rec := nbdns.SimpleRecord{Name: "svc.proxy.netbird.cloud.", Type: 1, Class: nbdns.DefaultClass, TTL: 300, RData: "100.64.0.7"} resolver := NewResolver() - require.NoError(t, resolver.RegisterRecord(rec)) + serviceZone(t, resolver, "proxy.netbird.cloud", rec) act := &recordingActivator{} resolver.SetPeerActivator(act) @@ -50,15 +64,15 @@ func TestLocalResolver_WarmsLazyPeerOnResolve(t *testing.T) { act.mu.Lock() defer act.mu.Unlock() - assert.True(t, act.called, "activator must be invoked for an overlay A answer") - assert.Contains(t, act.ips, "100.64.0.7", "activator must receive the answer's peer IP") + assert.True(t, act.called, "activator must be invoked for a service-zone A answer") + assert.Contains(t, act.addrs, netip.MustParseAddr("100.64.0.7"), "activator must receive the answer's peer IP") } func TestLocalResolver_NoActivatorNoWarmup(t *testing.T) { // With no activator wired the resolver behaves exactly as before. - rec := nbdns.SimpleRecord{Name: "svc.netbird.cloud.", Type: 1, Class: nbdns.DefaultClass, TTL: 300, RData: "100.64.0.7"} + rec := nbdns.SimpleRecord{Name: "svc.proxy.netbird.cloud.", Type: 1, Class: nbdns.DefaultClass, TTL: 300, RData: "100.64.0.7"} resolver := NewResolver() - require.NoError(t, resolver.RegisterRecord(rec)) + serviceZone(t, resolver, "proxy.netbird.cloud", rec) resp := serveA(t, resolver, rec.Name) require.NotNil(t, resp, "resolver must still answer without an activator") @@ -68,14 +82,91 @@ func TestLocalResolver_NoActivatorNoWarmup(t *testing.T) { func TestLocalResolver_NoWarmupForMissingRecord(t *testing.T) { // A query that resolves to nothing must not invoke the activator (no IPs). resolver := NewResolver() - require.NoError(t, resolver.RegisterRecord(nbdns.SimpleRecord{Name: "svc.netbird.cloud.", Type: 1, Class: nbdns.DefaultClass, TTL: 300, RData: "100.64.0.7"})) + serviceZone(t, resolver, "proxy.netbird.cloud", + nbdns.SimpleRecord{Name: "svc.proxy.netbird.cloud.", Type: 1, Class: nbdns.DefaultClass, TTL: 300, RData: "100.64.0.7"}) act := &recordingActivator{} resolver.SetPeerActivator(act) - serveA(t, resolver, "absent.netbird.cloud.") + serveA(t, resolver, "absent.proxy.netbird.cloud.") act.mu.Lock() defer act.mu.Unlock() assert.False(t, act.called, "activator must not be invoked when there is no answer") } + +func TestLocalResolver_NoWarmupInAuthoritativeZone(t *testing.T) { + // The account's peer zone is authoritative; resolving a peer's name there + // must not wake its lazy connection — warm-up is scoped to match-only + // (non-authoritative) zones such as the synthesized private-service zones. + rec := nbdns.SimpleRecord{Name: "peer.netbird.cloud.", Type: 1, Class: nbdns.DefaultClass, TTL: 300, RData: "100.64.0.9"} + resolver := NewResolver() + resolver.Update([]nbdns.CustomZone{{ + Domain: "netbird.cloud", + Records: []nbdns.SimpleRecord{rec}, + }}) + + act := &recordingActivator{} + resolver.SetPeerActivator(act) + + resp := serveA(t, resolver, rec.Name) + require.NotNil(t, resp, "resolver must answer") + require.NotEmpty(t, resp.Answer, "answer must carry the A record") + + act.mu.Lock() + defer act.mu.Unlock() + assert.False(t, act.called, "activator must not be invoked for authoritative-zone answers") +} + +func TestLazyWarmupTimeoutFromEnv(t *testing.T) { + tests := []struct { + name string + value string + envSet bool + want time.Duration + }{ + {name: "unset uses default", want: defaultLazyWarmupTimeout}, + {name: "valid overrides", value: "5s", envSet: true, want: 5 * time.Second}, + {name: "invalid falls back", value: "not-a-duration", envSet: true, want: defaultLazyWarmupTimeout}, + {name: "negative falls back", value: "-1s", envSet: true, want: defaultLazyWarmupTimeout}, + {name: "zero falls back", value: "0s", envSet: true, want: defaultLazyWarmupTimeout}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.envSet { + t.Setenv(envLazyWarmupTimeout, tt.value) + } + assert.Equal(t, tt.want, lazyWarmupTimeoutFromEnv()) + assert.Equal(t, tt.want, NewResolver().warmupTimeout, "constructor must resolve the timeout once") + }) + } +} + +func TestExtractRecordAddr(t *testing.T) { + t.Run("A record yields unmapped v4", func(t *testing.T) { + // net.ParseIP returns the 16-byte v4-in-v6 form, the same shape + // miekg/dns stores after parsing an A record; the extracted address + // must compare equal to a plain v4 netip.Addr. + addr, ok := extractRecordAddr(&dns.A{A: net.ParseIP("100.64.0.7")}) + require.True(t, ok) + assert.True(t, addr.Is4()) + assert.Equal(t, netip.MustParseAddr("100.64.0.7"), addr) + }) + + t.Run("AAAA record yields v6", func(t *testing.T) { + addr, ok := extractRecordAddr(&dns.AAAA{AAAA: net.ParseIP("fd00::1")}) + require.True(t, ok) + assert.Equal(t, netip.MustParseAddr("fd00::1"), addr) + }) + + t.Run("A record without address", func(t *testing.T) { + _, ok := extractRecordAddr(&dns.A{}) + assert.False(t, ok) + }) + + t.Run("non-address record", func(t *testing.T) { + _, ok := extractRecordAddr(&dns.CNAME{Target: "target.netbird.cloud."}) + assert.False(t, ok) + }) +} diff --git a/client/internal/dns/mock_server.go b/client/internal/dns/mock_server.go index 31fedd9e5..b19862c2f 100644 --- a/client/internal/dns/mock_server.go +++ b/client/internal/dns/mock_server.go @@ -8,6 +8,7 @@ import ( "github.com/miekg/dns" dnsconfig "github.com/netbirdio/netbird/client/internal/dns/config" + "github.com/netbirdio/netbird/client/internal/dns/local" nbdns "github.com/netbirdio/netbird/dns" "github.com/netbirdio/netbird/route" "github.com/netbirdio/netbird/shared/management/domain" @@ -92,6 +93,11 @@ func (m *MockServer) SetFirewall(Firewall) { // Mock implementation - no-op } +// SetPeerActivator mock implementation of SetPeerActivator from Server interface +func (m *MockServer) SetPeerActivator(local.PeerActivator) { + // Mock implementation - no-op +} + // BeginBatch mock implementation of BeginBatch from Server interface func (m *MockServer) BeginBatch() { // Mock implementation - no-op diff --git a/client/internal/dns/server.go b/client/internal/dns/server.go index c4501c3e0..f79454457 100644 --- a/client/internal/dns/server.go +++ b/client/internal/dns/server.go @@ -82,6 +82,7 @@ type Server interface { PopulateManagementDomain(mgmtURL *url.URL) error SetRouteSources(selected, active func() route.HAMap) SetFirewall(Firewall) + SetPeerActivator(local.PeerActivator) } type nsGroupsByDomain struct { @@ -1442,11 +1443,11 @@ type localPeerConnectivity struct { // IsConnectedByIP looks the IP up in the peerstore and surfaces both // the known and connected bits. Used by Resolver.filterDisconnectedPeerAnswers. -func (l localPeerConnectivity) IsConnectedByIP(ip string) (known, connected bool) { +func (l localPeerConnectivity) IsConnectedByIP(ip netip.Addr) (known, connected bool) { if l.status == nil { return false, false } - state, ok := l.status.PeerStateByIP(ip) + state, ok := l.status.PeerStateByIP(ip.String()) if !ok { return false, false } diff --git a/client/internal/dns_peer_activator.go b/client/internal/dns_peer_activator.go index 1bd740ef9..c283d6251 100644 --- a/client/internal/dns_peer_activator.go +++ b/client/internal/dns_peer_activator.go @@ -2,7 +2,7 @@ package internal import ( "context" - "sync" + "net/netip" "time" "github.com/netbirdio/netbird/client/internal/peer" @@ -12,33 +12,33 @@ import ( const dnsActivationPollInterval = 50 * time.Millisecond // dnsPeerActivator wakes lazy-connection peers from the DNS resolution path. It -// implements dns/local.PeerActivator. ConnMgr is not thread-safe (guarded by -// the engine's syncMsgMux) while DNS queries run on their own goroutines, so -// activation runs under that mutex; the connection wait runs without it. +// implements dns/local.PeerActivator. DNS queries run on their own goroutines, +// so it only touches state that is safe for concurrent use — ConnMgr.ActivatePeer, +// peerstore.Store and peer.Status — and never takes the engine's syncMsgMux, +// keeping DNS resolution from contending with network-map processing. type dnsPeerActivator struct { connMgr *ConnMgr peerStore *peerstore.Store status *peer.Status - mu *sync.Mutex // ctx is the engine's long-lived context. The connection dial is tied to it // (not the per-query DNS wait budget) so a handshake that outlasts the wait // still completes in the background rather than being cancelled at the deadline. ctx context.Context } -// ActivatePeersByIP triggers wake-up for the peer(s) owning ips and waits until -// one is connected or ctx (the per-query DNS wait budget) expires. Activation -// itself is tied to the engine's long-lived context so the dial survives a wait -// that times out. Unknown or already-connected IPs are skipped, so the -// steady-state (warm) path adds no latency. -func (a *dnsPeerActivator) ActivatePeersByIP(ctx context.Context, ips []string) { +// ActivatePeersByIP triggers wake-up for the peer(s) owning addrs and waits +// until one is connected or ctx (the per-query DNS wait budget) expires. +// Activation itself is tied to the engine's long-lived context so the dial +// survives a wait that times out. Unknown or already-connected addresses are +// skipped, so the steady-state (warm) path adds no latency. +func (a *dnsPeerActivator) ActivatePeersByIP(ctx context.Context, addrs []netip.Addr) { if a == nil || a.connMgr == nil { return } var pending []string - a.mu.Lock() - for _, ip := range ips { + for _, addr := range addrs { + ip := addr.String() st, ok := a.status.PeerStateByIP(ip) if !ok || st.ConnStatus == peer.StatusConnected { continue @@ -50,7 +50,6 @@ func (a *dnsPeerActivator) ActivatePeersByIP(ctx context.Context, ips []string) a.connMgr.ActivatePeer(a.ctx, conn) pending = append(pending, ip) } - a.mu.Unlock() if len(pending) == 0 { return diff --git a/client/internal/dns_peer_activator_test.go b/client/internal/dns_peer_activator_test.go new file mode 100644 index 000000000..8c3b75e59 --- /dev/null +++ b/client/internal/dns_peer_activator_test.go @@ -0,0 +1,129 @@ +package internal + +import ( + "context" + "net/netip" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/internal/peer" + "github.com/netbirdio/netbird/client/internal/peerstore" +) + +func newTestPeerConn(t *testing.T, key string) *peer.Conn { + t.Helper() + conn, err := peer.NewConn(peer.ConnConfig{ + Key: key, + LocalKey: "local", + WgConfig: peer.WgConfig{ + AllowedIps: []netip.Prefix{netip.MustParsePrefix("100.64.0.1/32")}, + }, + }, peer.ServiceDependencies{}) + require.NoError(t, err) + return conn +} + +func newTestDNSPeerActivator(t *testing.T) (*dnsPeerActivator, *peer.Status, *peerstore.Store) { + t.Helper() + status := peer.NewRecorder("https://mgm") + store := peerstore.NewConnStore() + // ConnMgr without Start: the lazy manager is nil, so ActivatePeer is a + // no-op — these tests exercise the activator's skip/wait logic. + connMgr := NewConnMgr(&EngineConfig{}, status, store, nil) + return &dnsPeerActivator{ + connMgr: connMgr, + peerStore: store, + status: status, + ctx: context.Background(), + }, status, store +} + +func TestDNSPeerActivator_NilSafe(t *testing.T) { + var a *dnsPeerActivator + a.ActivatePeersByIP(context.Background(), []netip.Addr{netip.MustParseAddr("100.64.0.1")}) +} + +// TestDNSPeerActivator_SkipsUnknownAndConnectedPeers verifies the steady-state +// (warm) path adds no latency: already-connected and unknown addresses never +// enter the wait loop. +func TestDNSPeerActivator_SkipsUnknownAndConnectedPeers(t *testing.T) { + a, status, store := newTestDNSPeerActivator(t) + + require.NoError(t, status.AddPeer("peerA", "a.netbird.cloud", "100.64.0.1", "fd00::1")) + require.NoError(t, status.UpdatePeerState(peer.State{PubKey: "peerA", ConnStatus: peer.StatusConnected})) + store.AddPeerConn("peerA", newTestPeerConn(t, "peerA")) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + start := time.Now() + a.ActivatePeersByIP(ctx, []netip.Addr{ + netip.MustParseAddr("100.64.0.1"), // known, connected -> skipped + netip.MustParseAddr("fd00::1"), // known via IPv6, connected -> skipped + netip.MustParseAddr("100.64.0.99"), // unknown -> skipped + }) + require.Less(t, time.Since(start), time.Second, "no pending peer must mean no wait") +} + +// TestDNSPeerActivator_WaitsForPendingPeerToConnect verifies the wait loop +// returns as soon as a pending peer reports connected, well before the +// per-query budget expires. +func TestDNSPeerActivator_WaitsForPendingPeerToConnect(t *testing.T) { + a, status, store := newTestDNSPeerActivator(t) + + require.NoError(t, status.AddPeer("peerA", "a.netbird.cloud", "100.64.0.1", "")) + store.AddPeerConn("peerA", newTestPeerConn(t, "peerA")) + + go func() { + time.Sleep(150 * time.Millisecond) + _ = status.UpdatePeerState(peer.State{PubKey: "peerA", ConnStatus: peer.StatusConnected}) + }() + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + start := time.Now() + a.ActivatePeersByIP(ctx, []netip.Addr{netip.MustParseAddr("100.64.0.1")}) + elapsed := time.Since(start) + + require.GreaterOrEqual(t, elapsed, 100*time.Millisecond, "must wait for the pending peer") + require.Less(t, elapsed, 5*time.Second, "must return on connect, not at the deadline") +} + +// TestDNSPeerActivator_ReturnsAtBudgetWhenPeerStaysIdle verifies a peer that +// never connects releases the DNS response at the per-query budget instead of +// blocking it indefinitely. +func TestDNSPeerActivator_ReturnsAtBudgetWhenPeerStaysIdle(t *testing.T) { + a, status, store := newTestDNSPeerActivator(t) + + require.NoError(t, status.AddPeer("peerA", "a.netbird.cloud", "100.64.0.1", "")) + store.AddPeerConn("peerA", newTestPeerConn(t, "peerA")) + + ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond) + defer cancel() + + start := time.Now() + a.ActivatePeersByIP(ctx, []netip.Addr{netip.MustParseAddr("100.64.0.1")}) + elapsed := time.Since(start) + + require.GreaterOrEqual(t, elapsed, 250*time.Millisecond, "must wait out the budget for a pending peer") + require.Less(t, elapsed, 5*time.Second, "must not block past the budget") +} + +// TestDNSPeerActivator_NoWaitWithoutPeerConn verifies a known-but-idle peer +// with no connection object in the store is not waited on: there is nothing to +// activate, so waiting could only ever time out. +func TestDNSPeerActivator_NoWaitWithoutPeerConn(t *testing.T) { + a, status, _ := newTestDNSPeerActivator(t) + + require.NoError(t, status.AddPeer("peerA", "a.netbird.cloud", "100.64.0.1", "")) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + start := time.Now() + a.ActivatePeersByIP(ctx, []netip.Addr{netip.MustParseAddr("100.64.0.1")}) + require.Less(t, time.Since(start), time.Second, "peer without a conn must not be waited on") +} diff --git a/client/internal/engine.go b/client/internal/engine.go index 7c24d70b2..91d86024b 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -657,15 +657,12 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL) // Wire DNS-time lazy-connection warm-up now that the connection manager // exists (it does not at DNS-server construction time). A DNS answer that // points at an idle peer then wakes it before the client's first request. - if ds, ok := e.dnsServer.(*dns.DefaultServer); ok { - ds.SetPeerActivator(&dnsPeerActivator{ - connMgr: e.connMgr, - peerStore: e.peerStore, - status: e.statusRecorder, - mu: e.syncMsgMux, - ctx: e.ctx, - }) - } + e.dnsServer.SetPeerActivator(&dnsPeerActivator{ + connMgr: e.connMgr, + peerStore: e.peerStore, + status: e.statusRecorder, + ctx: e.ctx, + }) e.srWatcher = guard.NewSRWatcher(e.signal, e.relayManager, e.mobileDep.IFaceDiscover, iceCfg) e.srWatcher.Start(peer.IsForceRelayed())