From f7af852851143d231b693ec94f7fc414940ef356 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Mon, 27 Jul 2026 11:57:59 +0200 Subject: [PATCH] Log the deferral reason when no handler answers and keep one logrus test hook --- client/internal/dns/handler_chain.go | 7 +++++-- client/internal/dns/handler_chain_test.go | 12 +++++++----- .../routemanager/dnsinterceptor/handler_test.go | 13 ++++++++++--- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/client/internal/dns/handler_chain.go b/client/internal/dns/handler_chain.go index c4b8ff938..e324e4c7d 100644 --- a/client/internal/dns/handler_chain.go +++ b/client/internal/dns/handler_chain.go @@ -367,8 +367,11 @@ func (c *HandlerChain) dispatch(w dns.ResponseWriter, r *dns.Msg, maxPriority in resutil.AttachEDE(resp, resutil.EDENetbirdSoftenedNegative, "netbird: name is served locally, no fallthrough resolver for this query type") } - logger.Tracef("no handler below the deferring one for domain=%s type=%s, answering NODATA", - qname, dns.TypeToString[question.Qtype]) + // logResponse never runs on this path, so the carried metadata is + // appended here or the reason for the deferral is lost in exactly the + // case that is hardest to diagnose. + logger.Tracef("no handler below the deferring one for domain=%s type=%s, answering NODATA%s", + qname, dns.TypeToString[question.Qtype], carried.format()) } if err := w.WriteMsg(resp); err != nil { logger.Errorf("failed to write DNS response: %v", err) diff --git a/client/internal/dns/handler_chain_test.go b/client/internal/dns/handler_chain_test.go index 7bb1d6d9b..e460370b0 100644 --- a/client/internal/dns/handler_chain_test.go +++ b/client/internal/dns/handler_chain_test.go @@ -1344,6 +1344,12 @@ func responseLineFor(hook *logtest.Hook, qname string) string { // without it an empty answer is indistinguishable from a real "no such record" // in a dig output or a capture taken next to the application. func TestHandlerChain_SoftNegative_IsVisibleToTheClient(t *testing.T) { + // One hook for the whole test: logtest installs it on the standard logger + // and logrus has no way to take it off again, so a hook per subtest would + // leave several behind buffering every later line in the package. + hook := logtest.NewGlobal() + t.Cleanup(hook.Reset) + newChain := func() *nbdns.HandlerChain { chain := nbdns.NewHandlerChain() chain.AddHandler("*.example.com.", &deferringHandler{softNegative: true}, nbdns.PriorityDNSRoute) @@ -1387,9 +1393,6 @@ func TestHandlerChain_SoftNegative_IsVisibleToTheClient(t *testing.T) { t.Run("log fields keep a stable order", func(t *testing.T) { const qname = "_mongodb._tcp.stable.example.com." - hook := logtest.NewGlobal() - t.Cleanup(hook.Reset) - prev := log.GetLevel() log.SetLevel(log.TraceLevel) t.Cleanup(func() { log.SetLevel(prev) }) @@ -1422,8 +1425,7 @@ func TestHandlerChain_SoftNegative_IsVisibleToTheClient(t *testing.T) { t.Run("logged with the reason it was deferred", func(t *testing.T) { const qname = "_mongodb._tcp.reason.example.com." - hook := logtest.NewGlobal() - t.Cleanup(hook.Reset) + hook.Reset() prev := log.GetLevel() log.SetLevel(log.TraceLevel) diff --git a/client/internal/routemanager/dnsinterceptor/handler_test.go b/client/internal/routemanager/dnsinterceptor/handler_test.go index 9f9614e8e..0cccd71ab 100644 --- a/client/internal/routemanager/dnsinterceptor/handler_test.go +++ b/client/internal/routemanager/dnsinterceptor/handler_test.go @@ -135,10 +135,17 @@ func TestDispositionFor(t *testing.T) { t.Run(dns.TypeToString[tt.qtype], func(t *testing.T) { assert.Equal(t, tt.want, dispositionFor(tt.qtype), "disposition for %s", dns.TypeToString[tt.qtype]) - if tt.want != dispositionPeer { - assert.Equal(t, tt.want == dispositionPeerFirst, resutil.SupportedRecordQtype(tt.qtype), - "only types a forwarder resolves may be sent to a peer") + // Address types are resolved by the forwarder's address path, not by + // its record path, so they are deliberately outside + // SupportedRecordQtype and their disposition does not depend on it. + if tt.want == dispositionPeer { + assert.False(t, resutil.SupportedRecordQtype(tt.qtype), + "address types take the forwarder's address path") + return } + + assert.Equal(t, tt.want == dispositionPeerFirst, resutil.SupportedRecordQtype(tt.qtype), + "only types a forwarder resolves may be sent to a peer") }) } }