diff --git a/TODO.md b/TODO.md index 49e335e..27c8b3f 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,8 @@ trial run of the finished image: https://git.eeqj.de/sneak/dnswatcher/issues/149 # Completed Steps +- 2026-10-02: watcher tests send far fewer queries and a live attempt may take + 18s; nameserver addresses are asked only for A, AAAA, CNAME (closes #214). - 2026-10-02: the resolver tries root servers, and every other server list it walks, in a random order each time, not always from the top (closes #138). - 2026-10-02: a name listed more than once in `DNSWATCHER_TARGETS`, in any diff --git a/internal/livednstest/livednstest.go b/internal/livednstest/livednstest.go index 338f3aa..7ce20cc 100644 --- a/internal/livednstest/livednstest.go +++ b/internal/livednstest/livednstest.go @@ -36,11 +36,18 @@ const ( // before the test fails. attempts = 3 - // AttemptTimeout bounds one attempt. Worst case for an operation - // is attempts * AttemptTimeout plus the backoff — about 26 - // seconds, well inside the 90-second `go test -timeout` backstop - // even when several operations exhaust their attempts. - AttemptTimeout = 8 * time.Second + // AttemptTimeout bounds one attempt. It must fit the longest + // operation, a watcher check, which sends over a hundred queries one + // after another and on a slow build host takes several times as long + // as the few seconds it takes on a fast one. An operation whose + // every attempt fails takes attempts * AttemptTimeout plus the + // backoff, about 56 seconds, after it waits for one of the + // Concurrency slots that every live operation in the test binary + // shares. So when live DNS does not answer at all, a test binary + // with more live operations than slots runs into the 90-second + // `go test -timeout` backstop instead of each test failing on its + // own. + AttemptTimeout = 18 * time.Second // backoffBase is the delay after the first failed attempt; it is // multiplied by backoffFactor each time. diff --git a/internal/resolver/export_test.go b/internal/resolver/export_test.go index 868174c..8840dd9 100644 --- a/internal/resolver/export_test.go +++ b/internal/resolver/export_test.go @@ -34,7 +34,7 @@ func (r *Resolver) QueryEachNS( nameservers []string, hostname string, ) (map[string]*NameserverResponse, error) { - return r.queryEachNS(ctx, nameservers, hostname) + return r.queryEachNS(ctx, nameservers, hostname, recordTypes()) } // ResolveNSIPs exports resolveNSIPs for testing. diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index 0c526e5..629e043 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -533,12 +533,39 @@ func (r *Resolver) FindAuthoritativeNameservers( return nil, ErrNoNameservers } +// recordTypes returns the record types a nameserver is asked for when a +// name is checked. +func recordTypes() []uint16 { + return []uint16{ + dns.TypeA, dns.TypeAAAA, dns.TypeCNAME, + dns.TypeMX, dns.TypeTXT, dns.TypeSRV, + dns.TypeCAA, dns.TypeNS, + } +} + +// addressTypes returns the record types ResolveIPAddresses asks for, +// the only ones it reads. +func addressTypes() []uint16 { + return []uint16{dns.TypeA, dns.TypeAAAA, dns.TypeCNAME} +} + // QueryNameserver queries a specific nameserver for all record // types and builds a NameserverResponse. func (r *Resolver) QueryNameserver( ctx context.Context, nsHostname string, hostname string, +) (*NameserverResponse, error) { + return r.queryNameserver(ctx, nsHostname, hostname, recordTypes()) +} + +// queryNameserver queries a specific nameserver for the record types +// in qtypes and builds a NameserverResponse. +func (r *Resolver) queryNameserver( + ctx context.Context, + nsHostname string, + hostname string, + qtypes []uint16, ) (*NameserverResponse, error) { if checkCtx(ctx) != nil { return nil, ErrContextCanceled @@ -551,7 +578,7 @@ func (r *Resolver) QueryNameserver( hostname = dns.Fqdn(hostname) - return r.queryAllTypes(ctx, nsHostname, nsIPs[0], hostname) + return r.queryTypes(ctx, nsHostname, nsIPs[0], hostname, qtypes) } // QueryNameserverIP queries a nameserver by its IP address directly, @@ -568,14 +595,15 @@ func (r *Resolver) QueryNameserverIP( hostname = dns.Fqdn(hostname) - return r.queryAllTypes(ctx, nsHostname, nsIP, hostname) + return r.queryTypes(ctx, nsHostname, nsIP, hostname, recordTypes()) } -func (r *Resolver) queryAllTypes( +func (r *Resolver) queryTypes( ctx context.Context, nsHostname string, nsIP string, hostname string, + qtypes []uint16, ) (*NameserverResponse, error) { resp := &NameserverResponse{ Nameserver: nsHostname, @@ -583,12 +611,6 @@ func (r *Resolver) queryAllTypes( Status: StatusOK, } - qtypes := []uint16{ - dns.TypeA, dns.TypeAAAA, dns.TypeCNAME, - dns.TypeMX, dns.TypeTXT, dns.TypeSRV, - dns.TypeCAA, dns.TypeNS, - } - state := r.queryEachType(ctx, nsIP, hostname, qtypes, resp) classifyResponse(resp, state) @@ -779,18 +801,19 @@ func (r *Resolver) QueryAllNameservers( return nil, err } - return r.queryEachNS(ctx, nameservers, hostname) + return r.queryEachNS(ctx, nameservers, hostname, recordTypes()) } func (r *Resolver) queryEachNS( ctx context.Context, nameservers []string, hostname string, + qtypes []uint16, ) (map[string]*NameserverResponse, error) { results := make(map[string]*NameserverResponse) for _, ns := range nameservers { - resp, err := r.QueryNameserver(ctx, ns, hostname) + resp, err := r.queryNameserver(ctx, ns, hostname, qtypes) // A query the context cut short says nothing about the // nameserver, so it must not be returned as its failure. @@ -835,9 +858,10 @@ func (r *Resolver) LookupAllRecords( } // ResolveIPAddresses resolves a hostname to all IPv4 and IPv6 -// addresses, following CNAME chains up to MaxCNAMEDepth. When no -// nameserver of the name's zone answered, it returns an error rather -// than no addresses. +// addresses, following CNAME chains up to MaxCNAMEDepth. It asks each +// nameserver of the name's zone for its A, AAAA and CNAME records only. +// When no nameserver of the name's zone answered, it returns an error +// rather than no addresses. func (r *Resolver) ResolveIPAddresses( ctx context.Context, hostname string, @@ -858,7 +882,12 @@ func (r *Resolver) resolveIPWithCNAME( return nil, ErrCNAMEDepthExceeded } - results, err := r.QueryAllNameservers(ctx, hostname) + nameservers, err := r.FindAuthoritativeNameservers(ctx, hostname) + if err != nil { + return nil, err + } + + results, err := r.queryEachNS(ctx, nameservers, hostname, addressTypes()) if err != nil { return nil, err } diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index abb6b92..30bdaab 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -562,6 +562,34 @@ func TestResolveIPAddresses_CloudflareDomain(t *testing.T) { assert.NotEmpty(t, ips) } +// TestResolveIPAddresses_NameserverIPv4AndIPv6 looks up the addresses of +// one of cloudflare.com's nameservers, as a domain check does for each +// nameserver. That name has A and AAAA records, so both kinds of address +// come back. +func TestResolveIPAddresses_NameserverIPv4AndIPv6(t *testing.T) { + t.Parallel() + + r := newTestResolver(t) + ns := findOneNSForDomain(t, r, "cloudflare.com") + ips := liveResolveIPs(t, r, ns) + + var ipv4, ipv6 int + + for _, ip := range ips { + parsed := net.ParseIP(ip) + require.NotNil(t, parsed, "should be valid IP: %s", ip) + + if parsed.To4() != nil { + ipv4++ + } else { + ipv6++ + } + } + + assert.Positive(t, ipv4, "no IPv4 address for %s: %v", ns, ips) + assert.Positive(t, ipv6, "no IPv6 address for %s: %v", ns, ips) +} + // ---------------------------------------------------------------- // Context cancellation tests // ---------------------------------------------------------------- diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 9918fc8..b527cb1 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -26,18 +26,22 @@ import ( // The watcher looks these names up in live DNS with the real resolver, // so tests assert on what the watcher does with the answers, never on -// the records these zones publish. testHost's nameservers and addresses -// stay the same from one check to the next, which the tests that check -// it twice rely on, and testSmallDomain's nameservers stay the same -// between a test looking them up and its check. A domain check looks up -// each nameserver's addresses, about a second per nameserver, so the -// tests that check a domain use testSmallDomain, which has two -// nameservers, and check it once. The tests that query testDomain's -// nameservers directly do no domain check. +// the records these zones publish. The nameservers of testHost and +// testSmallDomain stay the same between a test looking them up and its +// check. Every query a check sends is one more that can be lost, so the +// tests keep them few. A check asks each of a name's nameservers about +// every record type, and both names have two. A domain check also looks +// up each nameserver's addresses at every nameserver of the zone that +// nameserver is in: testSmallDomain's nameservers are in zones with two +// nameservers, while a domain whose nameservers are in, say, +// cloudflare.com, which has five, makes each domain check much longer. +// A test checks a domain only when it is about domains, and checks once, +// from saved state it builds, rather than twice. The tests that query +// testDomain's nameservers directly do no domain check. const ( testDomain = "google.com" - testSmallDomain = "example.com" - testHost = "cloudflare.com" + testSmallDomain = "desec.io" + testHost = "example.org" testIssuer = "DigiCert" ) @@ -259,55 +263,48 @@ func checkOnce( // runChecks builds a watcher, lets prepare set up the saved state and // stand-ins it starts from, and runs its checks once against live DNS. -// If change is not nil, change then alters the saved state or stand-ins -// and the checks run a second time. When either check finds no fresh -// address for a name (see checkOnce), the watcher is thrown away and -// all of this runs again on a new one, so a failed attempt leaves -// nothing behind in the saved state, the stand-ins or the notifications. +// When the check finds no fresh address for a name (see checkOnce), the +// watcher is thrown away and all of this runs again on a new one, so a +// failed attempt leaves nothing behind in the saved state, the +// stand-ins or the notifications. func runChecks( t *testing.T, cfg *config.Config, - prepare, change func(deps *testDeps), -) *testDeps { + prepare func(deps *testDeps), +) (*watcher.Watcher, *testDeps) { t.Helper() - var deps *testDeps + var ( + w *watcher.Watcher + deps *testDeps + ) livednstest.Retry(t, "watcher checks", func(ctx context.Context) error { - var w *watcher.Watcher - w, deps = newTestWatcher(t, cfg) if prepare != nil { prepare(deps) } - err := checkOnce(ctx, w, deps) - if err != nil || change == nil { - return err - } - - change(deps) - return checkOnce(ctx, w, deps) }) - return deps + return w, deps } -// lookupNameservers returns the nameservers live DNS lists for domain, +// lookupNameservers returns the nameservers live DNS lists for name, // for a test to save in the state its check starts from. -func lookupNameservers(t *testing.T, domain string) []string { +func lookupNameservers(t *testing.T, name string) []string { t.Helper() res := resolver.NewFromLogger(slog.Default()) var nameservers []string - livednstest.Retry(t, "LookupNS("+domain+")", func(ctx context.Context) error { + livednstest.Retry(t, "LookupNS("+name+")", func(ctx context.Context) error { var err error - nameservers, err = res.LookupNS(ctx, domain) + nameservers, err = res.LookupNS(ctx, name) return err }) @@ -370,7 +367,7 @@ func TestFirstRunBaseline(t *testing.T) { cfg.Domains = []string{testSmallDomain} cfg.Hostnames = []string{testHost} - deps := runChecks(t, cfg, nil, nil) + _, deps := runChecks(t, cfg, nil) assertNoNotifications(t, deps) assertStatePopulated(t, deps) @@ -422,7 +419,7 @@ func TestDomainPortAndTLSChecks(t *testing.T) { cfg := defaultTestConfig(t) cfg.Domains = []string{testSmallDomain} - deps := runChecks(t, cfg, nil, nil) + _, deps := runChecks(t, cfg, nil) snap := deps.state.GetSnapshot() @@ -462,11 +459,11 @@ func TestNSChangeDetection(t *testing.T) { cfg.Domains = []string{testSmallDomain} // The saved state lists nameservers that live DNS does not. - deps := runChecks(t, cfg, func(deps *testDeps) { + _, deps := runChecks(t, cfg, func(deps *testDeps) { deps.state.SetDomainState(testSmallDomain, &state.DomainState{ Nameservers: []string{oldNS1, oldNS2}, }) - }, nil) + }) assertNotified(t, deps, "NS Change: "+testSmallDomain, "warning") @@ -486,7 +483,7 @@ func TestNSAddressChangeDetection(t *testing.T) { // The saved state lists the nameservers live DNS lists, each at an // address live DNS never returns. - deps := runChecks(t, cfg, func(deps *testDeps) { + _, deps := runChecks(t, cfg, func(deps *testDeps) { nsAddresses := make(map[string][]string, len(nameservers)) for _, ns := range nameservers { nsAddresses[ns] = []string{oldIP} @@ -496,7 +493,7 @@ func TestNSAddressChangeDetection(t *testing.T) { Nameservers: nameservers, NameserverAddresses: nsAddresses, }) - }, nil) + }) title := "NS Address Change: " + testSmallDomain ds, _ := deps.state.GetDomainState(testSmallDomain) @@ -542,12 +539,12 @@ func TestNSAddedAndRemovedIsNoAddressChange(t *testing.T) { // The saved state lists oldNS1, which live DNS does not, in place of // the first nameserver live DNS lists, so that the check finds that // one added and oldNS1 removed. Only oldNS1 has addresses saved. - deps := runChecks(t, cfg, func(deps *testDeps) { + _, deps := runChecks(t, cfg, func(deps *testDeps) { deps.state.SetDomainState(testSmallDomain, &state.DomainState{ Nameservers: append([]string{oldNS1}, nameservers[1:]...), NameserverAddresses: map[string][]string{oldNS1: {oldIP}}, }) - }, nil) + }) if n := countNotifications(deps, "NS Change: "+testSmallDomain); n != 1 { t.Errorf("sent %d NS changes, want 1", n) @@ -565,15 +562,17 @@ func TestRecordChangeDetection(t *testing.T) { cfg := defaultTestConfig(t) cfg.Hostnames = []string{testHost} - // Between the checks, save for every nameserver an address live DNS - // never returns. - deps := runChecks(t, cfg, nil, func(deps *testDeps) { - hs, _ := deps.state.GetHostnameState(testHost) - for _, nsState := range hs.RecordsByNameserver { - nsState.Records = map[string][]string{"A": {oldIP}} + nameservers := lookupNameservers(t, testHost) + + // The saved state has every nameserver live DNS lists answering + // with an address live DNS never returns. + _, deps := runChecks(t, cfg, func(deps *testDeps) { + byNameserver := make(map[string]*state.NameserverRecordState) + for _, ns := range nameservers { + byNameserver[ns] = answered(map[string][]string{"A": {oldIP}}) } - deps.state.SetHostnameState(testHost, hs) + deps.state.SetHostnameState(testHost, saved(byNameserver)) }) assertNotified(t, deps, "Record Change: "+testHost, "warning") @@ -585,12 +584,15 @@ func TestPortStateChange(t *testing.T) { cfg := defaultTestConfig(t) cfg.Hostnames = []string{testHost} - // Between the checks, every port closes. - deps := runChecks(t, cfg, nil, func(deps *testDeps) { - deps.portChecker.mu.Lock() - deps.portChecker.closed = true - deps.portChecker.mu.Unlock() - }) + w, deps := runChecks(t, cfg, nil) + + // Every port closes, and the port checks run again. They look + // nothing up. + deps.portChecker.mu.Lock() + deps.portChecker.closed = true + deps.portChecker.mu.Unlock() + + w.CheckAllPorts(t.Context()) hs, _ := deps.state.GetHostnameState(testHost) assertNotified( @@ -610,7 +612,7 @@ func TestTLSExpiryWarning(t *testing.T) { cfg := defaultTestConfig(t) cfg.Hostnames = []string{testHost} - deps := runChecks(t, cfg, expiresInThreeDays, nil) + _, deps := runChecks(t, cfg, expiresInThreeDays) assertNotified(t, deps, "TLS Expiry Warning: "+testHost, "warning") } @@ -782,7 +784,7 @@ func TestDNSRunsBeforePortAndTLSChecks(t *testing.T) { cfg.Hostnames = []string{testHost} // The saved state says the last check found testHost at oldIP. - deps := runChecks(t, cfg, func(deps *testDeps) { + _, deps := runChecks(t, cfg, func(deps *testDeps) { deps.state.SetHostnameState(testHost, &state.HostnameState{ RecordsByNameserver: map[string]*state.NameserverRecordState{ oldNS1: { @@ -791,7 +793,7 @@ func TestDNSRunsBeforePortAndTLSChecks(t *testing.T) { }, }, }) - }, nil) + }) snap := deps.state.GetSnapshot() @@ -922,21 +924,20 @@ func TestNSFailureAndRecovery(t *testing.T) { cfg := defaultTestConfig(t) cfg.Hostnames = []string{testHost} - // Between the checks, save every nameserver the first check found - // as one that did not answer, and add, as answering, one that live - // DNS does not list, which then disappears. - deps := runChecks(t, cfg, nil, func(deps *testDeps) { - hs, _ := deps.state.GetHostnameState(testHost) - for ns := range hs.RecordsByNameserver { - hs.RecordsByNameserver[ns] = failed() + nameservers := lookupNameservers(t, testHost) + + // The saved state has every nameserver live DNS lists as one that + // did not answer, and, as answering, one that live DNS does not + // list, which then disappears. + _, deps := runChecks(t, cfg, func(deps *testDeps) { + byNameserver := map[string]*state.NameserverRecordState{ + oldNS1: answered(map[string][]string{"A": {oldIP}}), + } + for _, ns := range nameservers { + byNameserver[ns] = failed() } - hs.RecordsByNameserver[oldNS1] = &state.NameserverRecordState{ - Records: map[string][]string{"A": {oldIP}}, - Status: "ok", - } - - deps.state.SetHostnameState(testHost, hs) + deps.state.SetHostnameState(testHost, saved(byNameserver)) }) assertNotified(t, deps, "NS Failure: "+testHost, "error")