wip: tighten watcher assertions, document loopback nameservers

2026-09-03 22:53:50 +00:00
parent 240c1bd393
commit 15f148f066
3 changed files with 277 additions and 25 deletions
@@ -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,
@@ -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`
@@ -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,16 +588,33 @@ 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) {
 	t.Parallel()

@@ -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) {