wip: restore transport-failure classification coverage

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)

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 /
+// Transport-failure classification (StatusTimeout / StatusError)
-// StatusError for a nameserver that does not answer) is deliberately
+// is covered in transport_test.go, against real nameservers bound on
-// not covered here. Forcing it required the mock DNSClient this
+// loopback.
-// 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.
 
 func TestResolveIPAddresses_ContextCanceled(t *testing.T) {
 	t.Parallel()

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)
+}