From 240c1bd393d840e0b93c35d7a51f42857d7ef2fc Mon Sep 17 00:00:00 2001 From: clawbot Date: Thu, 3 Sep 2026 22:51:30 +0000 Subject: [PATCH] wip: restore transport-failure classification coverage --- internal/resolver/iterative.go | 19 ++- internal/resolver/resolver_test.go | 12 +- internal/resolver/transport_test.go | 253 ++++++++++++++++++++++++++++ 3 files changed, 274 insertions(+), 10 deletions(-) create mode 100644 internal/resolver/transport_test.go diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index eebab39..50ff63a 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -20,6 +20,10 @@ const ( minDomainLabels = 2 ) +// defaultDNSPort is the port a nameserver is assumed to listen on +// when its address does not carry one. +const defaultDNSPort = "53" + // ErrRefused is returned when a DNS server refuses a query. var ErrRefused = errors.New("dns query refused") @@ -106,6 +110,19 @@ func (r *Resolver) retryTCP( return resp } +// nameserverAddr renders a nameserver address for dialling. A bare +// address — the normal case, and what a delegation's glue records +// carry — is given the default DNS port. An address that already +// specifies a port is dialled as written, which is what makes a +// nameserver listening somewhere other than 53 reachable. +func nameserverAddr(nsIP string) string { + if _, _, err := net.SplitHostPort(nsIP); err == nil { + return nsIP + } + + return net.JoinHostPort(nsIP, defaultDNSPort) +} + // queryDNS sends a DNS query to a specific server IP. // Tries non-recursive first, falls back to recursive on // REFUSED (handles DNS interception environments). @@ -120,7 +137,7 @@ func (r *Resolver) queryDNS( } name = dns.Fqdn(name) - addr := net.JoinHostPort(serverIP, "53") + addr := nameserverAddr(serverIP) msg := new(dns.Msg) msg.SetQuestion(name, qtype) diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index 1e6d4b0..0750fd1 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -517,15 +517,9 @@ func TestQueryAllNameservers_ContextCanceled(t *testing.T) { assert.Error(t, err) } -// The resolver's transport-failure classification (StatusTimeout / -// StatusError for a nameserver that does not answer) is deliberately -// not covered here. Forcing it required the mock DNSClient this -// change removes, and a live substitute is not available: the build -// environment transparently intercepts all UDP/53 traffic and answers -// it locally, so a query to a black-holed address such as an RFC 5737 -// documentation address comes back StatusOK with real records. See -// TESTING.md; restoring this coverage needs a mechanism that is -// neither a mock nor dependent on the sandbox's network behaviour. +// Transport-failure classification (StatusTimeout / StatusError) +// is covered in transport_test.go, against real nameservers bound on +// loopback. func TestResolveIPAddresses_ContextCanceled(t *testing.T) { t.Parallel() diff --git a/internal/resolver/transport_test.go b/internal/resolver/transport_test.go new file mode 100644 index 0000000..fe668d5 --- /dev/null +++ b/internal/resolver/transport_test.go @@ -0,0 +1,253 @@ +package resolver_test + +// Transport-failure classification tests. +// +// These are live tests, not mocks. Nothing here substitutes the +// resolver's DNSClient: the resolver dials a real UDP socket, writes +// a real DNS query with the real miekg/dns client, and applies its +// real deadline and its real classification logic to what comes +// back. The only thing under test control is which address the query +// is sent to, and what — if anything — is listening there. +// +// That distinction is what the no-DNS-mocks rule in TESTING.md is +// about. A fake DNSClient lets the code under test skip DNS entirely +// and hands it a manufactured verdict; a nameserver bound on +// loopback makes it speak DNS for real and earn one. Pointing a live +// query at a nameserver of the test's choosing is no more a mock +// than pointing it at a.root-servers.net. +// +// The public network cannot produce these outcomes on demand. A +// black-holed address is not black-holed everywhere — build +// environments that intercept UDP/53 answer it locally — so a test +// built on one asserts on the network it happens to run on rather +// than on the resolver. A loopback nameserver is deterministic +// everywhere, and it is fast, because the test picks the deadline. + +import ( + "context" + "net" + "testing" + "time" + + "github.com/miekg/dns" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/dnswatcher/internal/resolver" +) + +const ( + // transportBudget is the wall time the timeout test must stay + // under. The resolver asks a nameserver for eight record types + // in turn and retries each one once, so a nameserver silent on + // every type would cost sixteen query timeouts. The test's + // nameserver is silent on exactly one type, which costs two, + // and this budget fails loudly if that ever stops being true. + transportBudget = 8 * time.Second + + // transportDeadline is the caller deadline the tests run + // under. It is generous on purpose: these tests are about the + // resolver classifying a nameserver's behaviour, so the + // caller's deadline must never be the thing that expires. + transportDeadline = 30 * time.Second + + // silentNS and failingNS are the nameserver names reported + // back in NameserverResponse.Nameserver. They are .test names + // (RFC 6761) and are never resolved: the tests address the + // nameserver by its socket address. + silentNS = "silent.ns.test." + failingNS = "servfail.ns.test." + + // transportHostname is the name queried. Nothing resolves it; + // the point is entirely how the nameserver behaves. + transportHostname = "example.com" +) + +// startNameserver binds a real UDP nameserver on loopback and serves +// every datagram it receives with handle, which returns the reply to +// send or nil to stay silent. It returns the "host:port" address to +// aim a query at, and stops the server when the test ends. +func startNameserver( + t *testing.T, + handle func(query *dns.Msg) *dns.Msg, +) string { + t.Helper() + + conn, err := net.ListenPacket("udp", "127.0.0.1:0") + require.NoError(t, err, "binding loopback nameserver") + + stopped := make(chan struct{}) + + t.Cleanup(func() { + _ = conn.Close() + <-stopped + }) + + go serveNameserver(conn, handle, stopped) + + return conn.LocalAddr().String() +} + +// serveNameserver reads queries until conn is closed, replying with +// whatever handle produces. +func serveNameserver( + conn net.PacketConn, + handle func(query *dns.Msg) *dns.Msg, + stopped chan<- struct{}, +) { + defer close(stopped) + + buf := make([]byte, dns.MaxMsgSize) + + for { + n, from, err := conn.ReadFrom(buf) + if err != nil { + return + } + + query := new(dns.Msg) + if query.Unpack(buf[:n]) != nil { + continue + } + + reply := handle(query) + if reply == nil { + continue + } + + wire, err := reply.Pack() + if err != nil { + continue + } + + if _, err := conn.WriteTo(wire, from); err != nil { + return + } + } +} + +// unservedAddr returns a loopback address with nothing listening on +// it, by binding a port and releasing it again. +func unservedAddr(t *testing.T) string { + t.Helper() + + conn, err := net.ListenPacket("udp", "127.0.0.1:0") + require.NoError(t, err, "binding loopback port") + + addr := conn.LocalAddr().String() + require.NoError(t, conn.Close(), "releasing loopback port") + + return addr +} + +// TestQueryNameserverIP_Timeout covers the StatusTimeout branch: a +// nameserver that takes the query and never answers. +func TestQueryNameserverIP_Timeout(t *testing.T) { + t.Parallel() + + // A real nameserver that drops A queries and answers every + // other type. Silence on one type is all the resolver needs to + // classify the response as a timeout, and it keeps the test + // two query timeouts long instead of sixteen. + addr := startNameserver(t, func(query *dns.Msg) *dns.Msg { + if len(query.Question) > 0 && + query.Question[0].Qtype == dns.TypeA { + return nil + } + + reply := new(dns.Msg) + reply.SetReply(query) + + return reply + }) + + r := newTestResolver(t) + + ctx, cancel := context.WithTimeout( + context.Background(), transportDeadline, + ) + defer cancel() + + start := time.Now() + + resp, err := r.QueryNameserverIP( + ctx, silentNS, addr, transportHostname, + ) + + elapsed := time.Since(start) + + require.NoError(t, err) + require.NotNil(t, resp) + + assert.Equal(t, resolver.StatusTimeout, resp.Status) + assert.Equal(t, "all queries timed out", resp.Error) + assert.Empty(t, resp.Records) + assert.Equal(t, silentNS, resp.Nameserver) + + assert.Less( + t, elapsed, transportBudget, + "one silent record type must cost one query's retries, "+ + "not every record type's", + ) +} + +// TestQueryNameserverIP_ServFail covers the StatusError branch: a +// nameserver that answers, and answers SERVFAIL. +func TestQueryNameserverIP_ServFail(t *testing.T) { + t.Parallel() + + addr := startNameserver(t, func(query *dns.Msg) *dns.Msg { + reply := new(dns.Msg) + reply.SetRcode(query, dns.RcodeServerFailure) + + return reply + }) + + r := newTestResolver(t) + + ctx, cancel := context.WithTimeout( + context.Background(), transportDeadline, + ) + defer cancel() + + resp, err := r.QueryNameserverIP( + ctx, failingNS, addr, transportHostname, + ) + + require.NoError(t, err) + require.NotNil(t, resp) + + assert.Equal(t, resolver.StatusError, resp.Status) + assert.Equal(t, "server returned SERVFAIL", resp.Error) + assert.Empty(t, resp.Records) + assert.Equal(t, failingNS, resp.Nameserver) +} + +// TestQueryNameserverIP_NoListener pins the third transport outcome: +// a refused datagram is not a timeout. The socket fails immediately +// with ECONNREFUSED rather than going quiet, so isTimeout is false, +// no failure flag is set, and the response classifies as NoData. +// Asserting it here is what stops that path being mistaken for the +// timeout path, in either direction. +func TestQueryNameserverIP_NoListener(t *testing.T) { + t.Parallel() + + addr := unservedAddr(t) + + r := newTestResolver(t) + + ctx, cancel := context.WithTimeout( + context.Background(), transportDeadline, + ) + defer cancel() + + resp, err := r.QueryNameserverIP( + ctx, silentNS, addr, transportHostname, + ) + + require.NoError(t, err) + require.NotNil(t, resp) + + assert.Equal(t, resolver.StatusNoData, resp.Status) + assert.Empty(t, resp.Records) +}