From 6e3795fde18a58df1e525411369d6f5c27dc2596 Mon Sep 17 00:00:00 2001 From: clawbot Date: Thu, 1 Oct 2026 21:28:45 +0000 Subject: [PATCH] resolver: error from ResolveIPAddresses when no nameserver answered (closes #190) ResolveIPAddresses now returns an error, not no addresses, when no nameserver of the name's zone answered. A nameserver with status timeout or error is not an answer; one answer, even NXDOMAIN, is enough for an empty result without an error. When every server of a zone fails, FindAuthoritativeNameservers moves on to the parent name, whose servers only refer the query onward. Such a referral now has status error, so it is no answer either, and a hostname's saved records show it as error. The only caller, the nameserver address lookup, already keeps the previous addresses on an error; its comment no longer says the resolver hides this case. Model: opus-5-5 --- README.md | 4 +- TODO.md | 2 + internal/resolver/errors.go | 5 ++ internal/resolver/export_test.go | 7 +++ internal/resolver/iterative.go | 45 +++++++++++++++-- internal/resolver/iterative_test.go | 32 ++++++++++++ internal/resolver/resolver_test.go | 76 +++++++++++++++++++++++++++++ internal/watcher/watcher.go | 7 ++- 8 files changed, 168 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index c5461fb..b25ef86 100644 --- a/README.md +++ b/README.md @@ -488,8 +488,8 @@ reachability: | `error` | Query failed (timeout, SERVFAIL, REFUSED, network error) | A nameserver that answers NXDOMAIN or with no records has status `ok` and empty -`records`. A nameserver whose query failed has status `error`, empty `records`, -and the reason in `error`. +`records`. A nameserver whose query failed, or that only referred it to other +nameservers, has status `error`, empty `records`, and the reason in `error`. `nameserverAddresses` lists, by nameserver, the sorted addresses its name resolves to. A state file without it loads, and the next check fills it in diff --git a/TODO.md b/TODO.md index a7e12ce..10f64bb 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-01: `ResolveIPAddresses` returns an error, not no addresses, when no + nameserver of the name's zone answered (closes #190). - 2026-10-01: `make fmt` and `make fmt-check` cover Markdown with prettier, run in Docker at the version pinned by `yarn.lock` (closes #119). - 2026-10-01: `make fmt-check` fails on a file `goimports` would change; both diff --git a/internal/resolver/errors.go b/internal/resolver/errors.go index 3f203d4..ab05678 100644 --- a/internal/resolver/errors.go +++ b/internal/resolver/errors.go @@ -10,6 +10,11 @@ var ( "no authoritative nameservers found", ) + // ErrNoNameserverAnswered is returned when every nameserver + // asked about a name timed out, failed or returned a referral, + // so whether the name has addresses is unknown. + ErrNoNameserverAnswered = errors.New("no nameserver answered") + // ErrCNAMEDepthExceeded is returned when a CNAME chain // exceeds MaxCNAMEDepth. ErrCNAMEDepthExceeded = errors.New( diff --git a/internal/resolver/export_test.go b/internal/resolver/export_test.go index d8e382b..9569a3a 100644 --- a/internal/resolver/export_test.go +++ b/internal/resolver/export_test.go @@ -11,6 +11,13 @@ func ExtractRecordValue(rr dns.RR) string { return extractRecordValue(rr) } +// CollectIPs exports collectIPs for testing. +func CollectIPs( + results map[string]*NameserverResponse, +) ([]string, string, error) { + return collectIPs(results) +} + // QueryEachNS exports queryEachNS for testing. func (r *Resolver) QueryEachNS( ctx context.Context, diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index 93f6b9b..a4c88ab 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -516,6 +516,7 @@ type queryState struct { gotSERVFAIL bool gotRefused bool gotTimeout bool + gotReferral bool netErr error hasRecords bool } @@ -578,6 +579,18 @@ func (r *Resolver) querySingleType( return } + // A reply with no answer that lists other nameservers, from a server + // that does not hold the name's zone, is a referral and says nothing + // about the name's records. A parent zone's servers send one when + // every server of the name's own zone failed and + // FindAuthoritativeNameservers moved on to the parent name. + if !msg.Authoritative && len(msg.Answer) == 0 && + len(extractNSSet(msg.Ns)) > 0 { + state.gotReferral = true + + return + } + collectAnswerRecords(msg, resp, state) } @@ -626,6 +639,9 @@ func classifyResponse(resp *NameserverResponse, state queryState) { case state.netErr != nil && !state.hasRecords: resp.Status = StatusError resp.Error = "network error: " + state.netErr.Error() + case state.gotReferral && !state.hasRecords: + resp.Status = StatusError + resp.Error = "server returned a referral" case !state.hasRecords && !state.gotNXDomain: resp.Status = StatusNoData } @@ -734,7 +750,9 @@ func (r *Resolver) LookupAllRecords( } // ResolveIPAddresses resolves a hostname to all IPv4 and IPv6 -// addresses, following CNAME chains up to MaxCNAMEDepth. +// addresses, following CNAME chains up to MaxCNAMEDepth. 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, @@ -760,7 +778,10 @@ func (r *Resolver) resolveIPWithCNAME( return nil, err } - ips, cnameTarget := collectIPs(results) + ips, cnameTarget, err := collectIPs(results) + if err != nil { + return nil, fmt.Errorf("resolving %s: %w", hostname, err) + } if len(ips) == 0 && cnameTarget != "" { return r.resolveIPWithCNAME(ctx, cnameTarget, depth+1) @@ -771,16 +792,28 @@ func (r *Resolver) resolveIPWithCNAME( return ips, nil } +// collectIPs returns the addresses in the nameservers' answers and the +// first CNAME target among them. It returns ErrNoNameserverAnswered when +// every nameserver timed out, failed or returned a referral: that is not +// a name with no addresses. func collectIPs( results map[string]*NameserverResponse, -) ([]string, string) { +) ([]string, string, error) { seen := make(map[string]bool) var ips []string var cnameTarget string + answered := false + for _, resp := range results { + if resp.Status == StatusTimeout || resp.Status == StatusError { + continue + } + + answered = true + if resp.Status == StatusNXDomain { continue } @@ -804,5 +837,9 @@ func collectIPs( } } - return ips, cnameTarget + if !answered { + return nil, "", ErrNoNameserverAnswered + } + + return ips, cnameTarget, nil } diff --git a/internal/resolver/iterative_test.go b/internal/resolver/iterative_test.go index 781e90b..f3440e7 100644 --- a/internal/resolver/iterative_test.go +++ b/internal/resolver/iterative_test.go @@ -5,10 +5,42 @@ import ( "github.com/miekg/dns" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "sneak.berlin/go/dnswatcher/internal/resolver" ) +// TestCollectIPs_OneAnswerIsEnough checks that one nameserver answering +// NXDOMAIN says the name has no addresses, though the other timed out. +func TestCollectIPs_OneAnswerIsEnough(t *testing.T) { + t.Parallel() + + ips, _, err := resolver.CollectIPs( + map[string]*resolver.NameserverResponse{ + "ns1.example.": {Status: resolver.StatusTimeout}, + "ns2.example.": {Status: resolver.StatusNXDomain}, + }, + ) + require.NoError(t, err) + assert.Empty(t, ips) +} + +// TestCollectIPs_FailedIsNoAnswer checks that nameservers that all have +// status error, from a refusal, a server failure, a network error or a +// referral, are no answer rather than a name with no addresses. +func TestCollectIPs_FailedIsNoAnswer(t *testing.T) { + t.Parallel() + + ips, _, err := resolver.CollectIPs( + map[string]*resolver.NameserverResponse{ + "ns1.example.": {Status: resolver.StatusError}, + "ns2.example.": {Status: resolver.StatusError}, + }, + ) + require.ErrorIs(t, err, resolver.ErrNoNameserverAnswered) + assert.Empty(t, ips) +} + func TestExtractRecordValue_LetterCase(t *testing.T) { t.Parallel() diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index 15a18c5..dd585c8 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -633,6 +633,82 @@ func TestQueryNameserverIP_Timeout(t *testing.T) { assert.NotEmpty(t, resp.Error) } +// TestCollectIPs_NoNameserverAnswered takes the response of a +// nameserver at 192.0.2.1, where nothing answers, as +// TestQueryNameserverIP_Timeout does. Addresses collected from +// nameservers that all failed to answer are an error, not none. +func TestCollectIPs_NoNameserverAnswered(t *testing.T) { + t.Parallel() + + r := newTestResolver(t) + + // The deadline outlasts the first try, as in + // TestQueryNameserverIP_Timeout. + ctx, cancel := context.WithTimeout( + context.Background(), 3*time.Second, + ) + t.Cleanup(cancel) + + resp, err := r.QueryNameserverIP( + ctx, "unreachable.test.", "192.0.2.1", + "example.com", + ) + require.NoError(t, err) + + ips, _, err := resolver.CollectIPs( + map[string]*resolver.NameserverResponse{resp.Nameserver: resp}, + ) + require.ErrorIs(t, err, resolver.ErrNoNameserverAnswered) + assert.Empty(t, ips) +} + +// TestCollectIPs_ReferralIsNoAnswer asks a root server about +// example.com, which the root zone does not hold, so it only refers the +// query to the com servers. That reply is no answer, as is a parent +// zone's when every server of the name's own zone failed. +func TestCollectIPs_ReferralIsNoAnswer(t *testing.T) { + t.Parallel() + + r := newTestResolver(t) + + var resp *resolver.NameserverResponse + + livednstest.Retry( + t, + "QueryNameserverIP(a.root-servers.net, example.com)", + func(ctx context.Context) error { + var err error + + resp, err = r.QueryNameserverIP( + ctx, "a.root-servers.net.", "198.41.0.4", + "example.com", + ) + if err != nil { + return err + } + + // A timeout or a network error is no reply at all. + if resp.Status == resolver.StatusTimeout || + strings.HasPrefix(resp.Error, "network error") { + return fmt.Errorf( + "%w: %s", livednstest.ErrNoAnswer, resp.Error, + ) + } + + return nil + }, + ) + + assert.Equal(t, resolver.StatusError, resp.Status) + assert.Equal(t, "server returned a referral", resp.Error) + + ips, _, err := resolver.CollectIPs( + map[string]*resolver.NameserverResponse{resp.Nameserver: resp}, + ) + require.ErrorIs(t, err, resolver.ErrNoNameserverAnswered) + assert.Empty(t, ips) +} + func TestResolveIPAddresses_ContextCanceled(t *testing.T) { t.Parallel() diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 2a2c523..98f0c68 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -321,10 +321,9 @@ func (w *Watcher) detectNSChanges( } // resolveNameserverAddresses returns the sorted addresses each -// nameserver's name resolves to. A nameserver whose lookup fails or -// finds no address keeps its addresses from prev: the resolver finds no -// address, without an error, when every server it asks times out, and -// that is not an address change. +// nameserver's name resolves to. A nameserver whose lookup fails, as it +// does when no nameserver of the name's zone answers, or finds no +// address keeps its addresses from prev and is not an address change. func (w *Watcher) resolveNameserverAddresses( ctx context.Context, nameservers []string, -- 2.54.0