diff --git a/TESTING.md b/TESTING.md index 573d156..765ead2 100644 --- a/TESTING.md +++ b/TESTING.md @@ -39,6 +39,42 @@ assertions and sensible timeouts, not from mocks. running at once the gates sum instead of holding, and the resolver's per-attempt deadlines start expiring +### Transport failures: loopback nameservers, not mocks + +The resolver classifies a nameserver that stays silent as +`StatusTimeout` and one that answers SERVFAIL as `StatusError`. The +public network cannot be made to produce either on demand — a +black-holed address is only black-holed on some networks, and build +environments that transparently intercept UDP/53 answer it locally — +so a test built on a chosen remote address asserts on the network it +happens to run on rather than on the resolver. + +`internal/resolver/transport_test.go` binds a real nameserver on +`127.0.0.1` instead and points the query at it. +`nameserverAddr` dials an address that already carries a port as +written, so no production behaviour is bypassed to arrange this. + +**This is permitted, and it is not a mock.** The rule above bans +substituting `DNSClient` or any other DNS abstraction, which lets the +code under test skip DNS and hands it a manufactured verdict. A +loopback nameserver does the opposite: the resolver dials a real +socket, writes a real query with the real `miekg/dns` client, and +applies its real deadline and its real classification logic to what +comes back. Choosing which nameserver a live query is sent to is not +faking DNS — the resolver is aimed at a nameserver of the caller's +choosing in production too. + +The distinction to hold on to: **substituting the client is banned; +choosing the server is not.** A test that reaches for a fake +`DNSClient` to force a classification is still forbidden, no matter +how awkward the alternative looks. + +Such a test must stay cheap. The resolver asks for eight record +types and retries each once, so a nameserver silent on every type +costs sixteen query timeouts. `TestQueryNameserverIP_Timeout` is +silent on `A` alone and answers the rest, which is all the resolver +needs to classify the response and keeps the test to two. + ### What NOT to do - **Do not mock `DNSClient`**, the watcher's `DNSResolver` interface, diff --git a/TODO.md b/TODO.md index 0bc4b90..062293f 100644 --- a/TODO.md +++ b/TODO.md @@ -26,6 +26,22 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 2026-09-03: restored the transport-failure classification coverage + the DNS-mock removal had dropped, and tightened two over-tolerant + watcher assertions. `internal/resolver/transport_test.go` covers + `StatusTimeout`, `StatusError` and the connection-refused path by + binding real nameservers on loopback rather than by mocking + `DNSClient` or by aiming a query at a remote address and hoping the + network black-holes it; `queryDNS` now honours a port already + present in a nameserver address, which is what lets a query be + aimed at one. The watcher's `assertStatePopulated` and + `TestDomainPortAndTLSChecks` now assert that port and certificate + state, and the arguments the port and TLS checkers were called + with, match the addresses live DNS returned — previously they + asserted only that those sets were non-empty, which would not have + caught resolving the wrong addresses. `TESTING.md` records why a + loopback nameserver is not a mock. + - 2026-08-10: comment-only corrections to `script/bootstrap`, `script/cibuild`, and `Dockerfile.lint`. The `goimports` pin in `script/bootstrap` was justified by a claim that `script/fmt-check` 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) +} diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 3907fb6..f039422 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "os" + "slices" "sort" "strings" "sync" @@ -53,6 +54,12 @@ const ( longLife = 90 * 24 * time.Hour shortLife = 3 * 24 * time.Hour + // monitoredTestPorts and tlsCheckPort mirror the ports the + // watcher checks. They are duplicated here deliberately: a + // test that imported the production values would agree with + // the watcher no matter which ports it drifted to. + tlsCheckPort = 443 + pollInterval = 100 * time.Millisecond scanTimeout = 25 * time.Second notifyGrace = 500 * time.Millisecond @@ -64,22 +71,33 @@ var errNotFound = errors.New("not found") // --- Test doubles for non-DNS collaborators --- +// portCall records one CheckPort invocation. The arguments matter, +// not just the count: a watcher that dutifully checks ports on the +// wrong addresses produces exactly the same call count as one that +// resolved correctly, so the tests assert what was checked. +type portCall struct { + address string + port int +} + type mockPortChecker struct { mu sync.Mutex openAll bool err error calls int + seen []portCall } func (m *mockPortChecker) CheckPort( _ context.Context, - _ string, - _ int, + address string, + port int, ) (*portcheck.PortResult, error) { m.mu.Lock() defer m.mu.Unlock() m.calls++ + m.seen = append(m.seen, portCall{address: address, port: port}) if m.err != nil { return nil, m.err @@ -102,22 +120,50 @@ func (m *mockPortChecker) callCount() int { return m.calls } +// checkedKeys returns the distinct "address:port" pairs the checker +// was asked about, sorted, in the same form as the state store's +// port keys so the two can be compared directly. +func (m *mockPortChecker) checkedKeys() []string { + m.mu.Lock() + defer m.mu.Unlock() + + keys := make([]string, 0, len(m.seen)) + for _, c := range m.seen { + keys = append( + keys, fmt.Sprintf("%s:%d", c.address, c.port), + ) + } + + return sortedSet(keys) +} + +// tlsCall records one CheckCertificate invocation, for the same +// reason portCall records CheckPort's arguments. +type tlsCall struct { + address string + hostname string +} + type mockTLSChecker struct { mu sync.Mutex cert *tlscheck.CertificateInfo err error calls int + seen []tlsCall } func (m *mockTLSChecker) CheckCertificate( _ context.Context, - _ string, + address string, hostname string, ) (*tlscheck.CertificateInfo, error) { m.mu.Lock() defer m.mu.Unlock() m.calls++ + m.seen = append( + m.seen, tlsCall{address: address, hostname: hostname}, + ) if m.err != nil { return nil, m.err @@ -146,6 +192,23 @@ func (m *mockTLSChecker) callCount() int { return m.calls } +// checkedKeys returns the distinct certificate keys the checker was +// asked about, sorted, in the state store's "address:port:hostname" +// form so the two can be compared directly. +func (m *mockTLSChecker) checkedKeys() []string { + m.mu.Lock() + defer m.mu.Unlock() + + keys := make([]string, 0, len(m.seen)) + for _, c := range m.seen { + keys = append(keys, fmt.Sprintf( + "%s:%d:%s", c.address, tlsCheckPort, c.hostname, + )) + } + + return sortedSet(keys) +} + type notification struct { Title string Message string @@ -367,6 +430,116 @@ func liveIPs(snap state.Snapshot, hostname string) []string { return ips } +// monitoredTestPorts is the set of ports the watcher is expected to +// check on every resolved address. +func monitoredTestPorts() []int { + return []int{80, tlsCheckPort} +} + +// sortedSet deduplicates and sorts, so that call records and state +// keys can be compared as sets. +func sortedSet(values []string) []string { + seen := make(map[string]bool, len(values)) + + out := make([]string, 0, len(values)) + + for _, v := range values { + if seen[v] { + continue + } + + seen[v] = true + + out = append(out, v) + } + + sort.Strings(out) + + return out +} + +// watchedNames is every name the config puts under observation. +// Domains get hostname state too, for their port and TLS checks. +func watchedNames(cfg *config.Config) []string { + names := make([]string, 0, len(cfg.Hostnames)+len(cfg.Domains)) + names = append(names, cfg.Hostnames...) + names = append(names, cfg.Domains...) + + return sortedSet(names) +} + +// stateHostnames is the set of names the run actually recorded. +func stateHostnames(snap state.Snapshot) []string { + names := make([]string, 0, len(snap.Hostnames)) + for name := range snap.Hostnames { + names = append(names, name) + } + + return sortedSet(names) +} + +// expectedPortKeys is every "address:port" the watcher should have +// touched: the monitored ports on every address live DNS resolved +// for every watched name, and nothing else. +func expectedPortKeys( + snap state.Snapshot, + names []string, +) []string { + var keys []string + + for _, name := range names { + for _, ip := range liveIPs(snap, name) { + for _, port := range monitoredTestPorts() { + keys = append( + keys, fmt.Sprintf("%s:%d", ip, port), + ) + } + } + } + + return sortedSet(keys) +} + +// expectedCertKeys is every "address:port:hostname" the watcher +// should have a certificate for, given that every port reports open. +func expectedCertKeys( + snap state.Snapshot, + names []string, +) []string { + var keys []string + + for _, name := range names { + for _, ip := range liveIPs(snap, name) { + keys = append(keys, fmt.Sprintf( + "%s:%d:%s", ip, tlsCheckPort, name, + )) + } + } + + return sortedSet(keys) +} + +// stateKeys sorts a state map's keys for set comparison. +func stateKeys[V any](m map[string]V) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + + return sortedSet(keys) +} + +// assertSameSet fails with both sides named when they differ. +func assertSameSet(t *testing.T, what string, want, got []string) { + t.Helper() + + if !slices.Equal(want, got) { + t.Errorf( + "%s: expected %v, got %v", what, want, got, + ) + } +} + // --- Tests --- func TestFirstRunBaseline(t *testing.T) { @@ -415,14 +588,31 @@ func assertStatePopulated( ) } - // Hostnames includes both explicit hostnames and domains - // (domains now also get hostname state for port/TLS checks). - if len(snap.Hostnames) < 1 { - t.Errorf( - "expected at least 1 hostname in state, got %d", - len(snap.Hostnames), - ) + // Hostname state covers both explicit hostnames and domains + // (domains also get hostname state for port/TLS checks), and + // covers exactly those: a name in state that nothing asked + // for, or a configured name missing from it, is a bug. + names := watchedNames(deps.config) + assertSameSet( + t, "hostnames in state", names, stateHostnames(snap), + ) + + // Every watched name must have resolved to something, or the + // key comparisons below would pass vacuously on empty sets. + for _, name := range names { + if len(liveIPs(snap, name)) == 0 { + t.Errorf("no addresses resolved for %s", name) + } } + + // The addresses the port checker was aimed at must be the + // addresses live DNS returned. Counting calls would not + // notice the watcher checking the wrong hosts. + assertSameSet( + t, "ports checked", + expectedPortKeys(snap, names), + deps.portChecker.checkedKeys(), + ) } func TestDomainPortAndTLSChecks(t *testing.T) { @@ -438,24 +628,34 @@ func TestDomainPortAndTLSChecks(t *testing.T) { snap := deps.state.GetSnapshot() - // The domain resolved via live DNS should have port state - // populated for its real addresses. - if len(snap.Ports) == 0 { - t.Error("expected port state for domain, got none") + names := watchedNames(cfg) + + ips := liveIPs(snap, testDomain) + if len(ips) == 0 { + t.Fatal("live DNS resolved no addresses for " + testDomain) } - // Domain should have certificate state populated. - if len(snap.Certificates) == 0 { - t.Error("expected certificate state for domain, got none") - } + // Port and certificate state must be keyed by the addresses + // live DNS actually returned — every one of them, and no + // others. Asserting only that the maps are non-empty would + // hold just as well if the watcher had resolved the wrong + // name or dropped all but one of its addresses. + wantPorts := expectedPortKeys(snap, names) + assertSameSet(t, "port state", wantPorts, stateKeys(snap.Ports)) + assertSameSet( + t, "ports checked", wantPorts, + deps.portChecker.checkedKeys(), + ) - if deps.portChecker.callCount() == 0 { - t.Error("expected port checker to be called for domain") - } - - if deps.tlsChecker.callCount() == 0 { - t.Error("expected TLS checker to be called for domain") - } + wantCerts := expectedCertKeys(snap, names) + assertSameSet( + t, "certificate state", wantCerts, + stateKeys(snap.Certificates), + ) + assertSameSet( + t, "certificates checked", wantCerts, + deps.tlsChecker.checkedKeys(), + ) } func TestNSChangeDetection(t *testing.T) {