From 5fab7b7417031a4641061732a939efb6d0fec380 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 wrapping ErrNoNameserverAnswered when every nameserver of the name's zone timed out or failed, instead of no addresses and no error. It reads each nameserver's status as the lookup already sets it; one answer, even NXDOMAIN, is enough for an empty result without an 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 --- TODO.md | 2 ++ internal/resolver/errors.go | 5 +++++ internal/resolver/export_test.go | 7 +++++++ internal/resolver/iterative.go | 29 +++++++++++++++++++++++++---- internal/resolver/iterative_test.go | 16 ++++++++++++++++ internal/resolver/resolver_test.go | 29 +++++++++++++++++++++++++++++ internal/watcher/watcher.go | 7 +++---- 7 files changed, 87 insertions(+), 8 deletions(-) diff --git a/TODO.md b/TODO.md index 928baeb..4278559 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,8 @@ 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: a hostname is queried at the servers of the zone it is in, found by following delegations for the name, not its last two labels (closes #189). - 2026-10-01: each nameserver's addresses are saved with its domain, and a diff --git a/internal/resolver/errors.go b/internal/resolver/errors.go index 3f203d4..cc2cbd2 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 or failed, 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..921870c 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -734,7 +734,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 wrapping +// ErrNoNameserverAnswered rather than no addresses. func (r *Resolver) ResolveIPAddresses( ctx context.Context, hostname string, @@ -760,7 +762,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 +776,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 or failed: 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 +821,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..04a33b9 100644 --- a/internal/resolver/iterative_test.go +++ b/internal/resolver/iterative_test.go @@ -5,10 +5,26 @@ 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) +} + func TestExtractRecordValue_LetterCase(t *testing.T) { t.Parallel() diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index 15a18c5..e8bce6a 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -633,6 +633,35 @@ 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) +} + 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,