watcher: notify NS query failure and recovery (closes #104)

LookupAllRecords now returns each nameserver's response, so the
watcher saves its status: ok when it answered, NXDOMAIN and no records
included, and error with the reason when it timed out, answered
SERVFAIL or REFUSED, or could not be reached. A nameserver that starts
failing sends NS Failure and one that answers again sends NS Recovery.
A failing nameserver is left out of the record change and
inconsistency comparisons. The resolver used to report REFUSED and
network errors as an answer with no records; they are now errors. A
lookup cut short by its context now returns an error instead of a
failure of the nameserver it was querying.

Model: opus-5-5

README.md

@@ -71,18 +71,25 @@ rejected.
     did on the previous check (additions, removals, value changes).
   - **NS query failure**: A nameserver that previously responded
     becomes unreachable (timeout, SERVFAIL, REFUSED, network error).
-    This is distinct from "responded with no records."
+    This is distinct from "responded with no records": a nameserver
+    that answers NXDOMAIN or with no records has responded. The alert
+    is sent once, on the check where it starts failing. A failing
+    nameserver gives no records, so it is not reported as a record
+    change or compared for inconsistency. A nameserver that is already
+    failing on the first check that sees it is recorded silently.
   - **NS recovery**: A previously-unreachable nameserver starts
-    responding again.
+    responding again. Its records are not compared with those from
+    before it failed, so a change made while it was failing is not
+    reported as a record change.
   - **Inconsistency detected**: Two nameservers return different record
     sets for the same hostname and did not already differ on the previous
     check. Every pair of nameservers is compared. The alert is sent once
     for each such pair, on the check where they start to disagree, and not
     again while they keep disagreeing, including after a restart. A
     nameserver that was not in the previous check (newly added, or back
-    after dropping out) and answers differently is reported on the check
-    where it appears. If a pair agrees again and later disagrees, the
-    alert is sent again.
+    after dropping out), or failed on it, and answers differently is
+    reported on the check where it answers. If a pair agrees again and
+    later disagrees, the alert is sent again.
 
 ### TCP Port Monitoring
 
@@ -459,9 +466,13 @@ The `status` field for each per-nameserver entry and certificate entry
 tracks reachability:
 
 | Status      | Meaning                                                    |
-|-------------|-------------------------------------------------|
+|-------------|------------------------------------------------------------|
 | `ok`        | Query succeeded, records are current                       |
-| `error`     | Query failed (timeout, SERVFAIL, network error)  |
+| `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`.
 
 ---
 

TODO.md

@@ -15,11 +15,12 @@ on the 1.0 milestone: https://git.eeqj.de/sneak/dnswatcher/milestone/7
 
 # Next Step
 
-NS failure and NS recovery notifications:
-https://git.eeqj.de/sneak/dnswatcher/issues/104
+nameserver IP address changes: https://git.eeqj.de/sneak/dnswatcher/issues/105
 
 # Completed Steps
 
+- 2026-10-01: a nameserver that does not answer is saved as `error` with the
+  reason, and NS failure and NS recovery are notified (closes #104).
 - 2026-10-01: a `DNSWATCHER_DNS_INTERVAL` or `DNSWATCHER_TLS_INTERVAL` that is
   not a positive duration stops startup; empty means the default (closes #177).
 - 2026-10-01: `/metrics` allows each client address 30 requests a minute,
@@ -96,7 +97,6 @@ https://git.eeqj.de/sneak/dnswatcher/issues/104
 
 # Future Steps
 
-- nameserver IP address changes: https://git.eeqj.de/sneak/dnswatcher/issues/105
 - `DNSWATCHER_SENTRY_DSN` does nothing:
   https://git.eeqj.de/sneak/dnswatcher/issues/107
 - trial run of the finished image:

internal/resolver/export_test.go

@@ -1,8 +1,21 @@
 package resolver
 
-import "github.com/miekg/dns"
+import (
+	"context"
+
+	"github.com/miekg/dns"
+)
 
 // ExtractRecordValue exports extractRecordValue for testing.
 func ExtractRecordValue(rr dns.RR) string {
 	return extractRecordValue(rr)
 }
+
+// QueryEachNS exports queryEachNS for testing.
+func (r *Resolver) QueryEachNS(
+	ctx context.Context,
+	nameservers []string,
+	hostname string,
+) (map[string]*NameserverResponse, error) {
+	return r.queryEachNS(ctx, nameservers, hostname)
+}

internal/resolver/iterative.go

@@ -504,7 +504,9 @@ func (r *Resolver) queryAllTypes(
 type queryState struct {
 	gotNXDomain bool
 	gotSERVFAIL bool
+	gotRefused  bool
 	gotTimeout  bool
+	netErr      error
 	hasRecords  bool
 }
 
@@ -542,8 +544,13 @@ func (r *Resolver) querySingleType(
 ) {
 	msg, err := r.queryDNS(ctx, nsIP, hostname, qtype)
 	if err != nil {
-		if isTimeout(err) {
+		switch {
+		case isTimeout(err):
 			state.gotTimeout = true
+		case errors.Is(err, ErrRefused):
+			state.gotRefused = true
+		default:
+			state.netErr = err
 		}
 
 		return
@@ -603,6 +610,12 @@ func classifyResponse(resp *NameserverResponse, state queryState) {
 	case state.gotSERVFAIL && !state.hasRecords:
 		resp.Status = StatusError
 		resp.Error = "server returned SERVFAIL"
+	case state.gotRefused && !state.hasRecords:
+		resp.Status = StatusError
+		resp.Error = "server returned REFUSED"
+	case state.netErr != nil && !state.hasRecords:
+		resp.Status = StatusError
+		resp.Error = "network error: " + state.netErr.Error()
 	case !state.hasRecords && !state.gotNXDomain:
 		resp.Status = StatusNoData
 	}
@@ -682,11 +695,14 @@ func (r *Resolver) queryEachNS(
 	results := make(map[string]*NameserverResponse)
 
 	for _, ns := range nameservers {
+		resp, err := r.QueryNameserver(ctx, ns, hostname)
+
+		// A query the context cut short says nothing about the
+		// nameserver, so it must not be returned as its failure.
 		if checkCtx(ctx) != nil {
 			return nil, ErrContextCanceled
 		}
 
-		resp, err := r.QueryNameserver(ctx, ns, hostname)
 		if err != nil {
 			results[ns] = &NameserverResponse{
 				Nameserver: ns,
@@ -714,21 +730,13 @@ func (r *Resolver) LookupNS(
 
 // LookupAllRecords performs iterative resolution to find all DNS
 // records for the given hostname, keyed by authoritative nameserver.
+// Each nameserver's response carries its status and error with its
+// records.
 func (r *Resolver) LookupAllRecords(
 	ctx context.Context,
 	hostname string,
-) (map[string]map[string][]string, error) {
-	results, err := r.QueryAllNameservers(ctx, hostname)
-	if err != nil {
-		return nil, err
-	}
-
-	out := make(map[string]map[string][]string, len(results))
-	for ns, resp := range results {
-		out[ns] = resp.Records
-	}
-
-	return out, nil
+) (map[string]*NameserverResponse, error) {
+	return r.QueryAllNameservers(ctx, hostname)
 }
 
 // ResolveIPAddresses resolves a hostname to all IPv4 and IPv6

internal/resolver/resolver_test.go

@@ -2,6 +2,7 @@ package resolver_test
 
 import (
 	"context"
+	"fmt"
 	"log/slog"
 	"net"
 	"os"
@@ -13,6 +14,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"sneak.berlin/go/dnswatcher/internal/livednstest"
 	"sneak.berlin/go/dnswatcher/internal/resolver"
 )
 
@@ -231,6 +233,45 @@ func TestQueryNameserver_NXDomain(t *testing.T) {
 	assert.Equal(t, resolver.StatusNXDomain, resp.Status)
 }
 
+// TestQueryNameserver_Refused asks a google.com nameserver about
+// cloudflare.com, a zone it does not serve, which it refuses. Refusing
+// is a failure to answer, not an answer with no records.
+func TestQueryNameserver_Refused(t *testing.T) {
+	t.Parallel()
+
+	r := newTestResolver(t)
+	ns := findOneNSForDomain(t, r, "google.com")
+
+	var resp *resolver.NameserverResponse
+
+	livednstest.Retry(
+		t,
+		"QueryNameserver("+ns+", cloudflare.com)",
+		func(ctx context.Context) error {
+			var err error
+
+			resp, err = r.QueryNameserver(ctx, ns, "cloudflare.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: %s",
+					livednstest.ErrNoAnswer, ns, resp.Error,
+				)
+			}
+
+			return nil
+		},
+	)
+
+	assert.Equal(t, resolver.StatusError, resp.Status)
+	assert.Equal(t, "server returned REFUSED", resp.Error)
+}
+
 func TestQueryNameserver_RecordsSorted(t *testing.T) {
 	t.Parallel()
 
@@ -518,6 +559,29 @@ func TestQueryAllNameservers_ContextCanceled(t *testing.T) {
 	assert.Error(t, err)
 }
 
+// TestQueryEachNS_CanceledDuringQuery cancels the context while a
+// nameserver is being queried, as shutdown does. A lookup cut short
+// says nothing about the nameserver, so it must return an error, not a
+// failed response for it.
+func TestQueryEachNS_CanceledDuringQuery(t *testing.T) {
+	t.Parallel()
+
+	r := newTestResolver(t)
+	ctx, cancel := context.WithCancel(context.Background())
+	t.Cleanup(cancel)
+
+	// Finding the nameserver's address alone starts at the root
+	// servers and takes several round trips, so a cancel a few
+	// milliseconds in lands during the query.
+	time.AfterFunc(5*time.Millisecond, cancel)
+
+	results, err := r.QueryEachNS(
+		ctx, []string{"ns1.google.com."}, "google.com",
+	)
+	require.ErrorIs(t, err, resolver.ErrContextCanceled)
+	assert.Nil(t, results)
+}
+
 // ----------------------------------------------------------------
 // Timeout tests
 // ----------------------------------------------------------------
@@ -530,7 +594,7 @@ func TestQueryNameserverIP_Timeout(t *testing.T) {
 	// Nothing answers at 192.0.2.1, a documentation address. The
 	// resolver tries each query twice, and the first try gives up
 	// after two seconds. A deadline that ends during the first try
-	// makes the status vary from run to run between nodata and
+	// makes the status vary from run to run between error and
 	// timeout, so the deadline must outlast the first try.
 	ctx, cancel := context.WithTimeout(
 		context.Background(), 3*time.Second,

internal/watcher/export_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"sneak.berlin/go/dnswatcher/internal/config"
+	"sneak.berlin/go/dnswatcher/internal/resolver"
 	"sneak.berlin/go/dnswatcher/internal/state"
 )
 
@@ -33,8 +34,7 @@ func NewForTest(
 
 // NewlyDisagreeingPairs exports newlyDisagreeingPairs for testing.
 func NewlyDisagreeingPairs(
-	prev *state.HostnameState,
-	current map[string]map[string][]string,
+	prev, current *state.HostnameState,
 ) [][2]string {
 	return newlyDisagreeingPairs(prev, current)
 }
@@ -43,8 +43,15 @@ func NewlyDisagreeingPairs(
 func (w *Watcher) DetectHostnameChanges(
 	ctx context.Context,
 	hostname string,
-	prev *state.HostnameState,
-	current map[string]map[string][]string,
+	prev, current *state.HostnameState,
 ) {
 	w.detectHostnameChanges(ctx, hostname, prev, current)
 }
+
+// BuildHostnameState exports buildHostnameState for testing.
+func BuildHostnameState(
+	results map[string]*resolver.NameserverResponse,
+	now time.Time,
+) *state.HostnameState {
+	return buildHostnameState(results, now)
+}

internal/watcher/inconsistency_test.go

@@ -105,7 +105,9 @@ func TestNewlyDisagreeingPairs(t *testing.T) {
 
 			prev := hostnameState(tt.loaded)
 
-			for i, current := range tt.checks {
+			for i, records := range tt.checks {
+				current := hostnameState(records)
+
 				got := watcher.NewlyDisagreeingPairs(prev, current)
 				if !slices.Equal(got, tt.want[i]) {
 					t.Errorf(
@@ -114,7 +116,7 @@ func TestNewlyDisagreeingPairs(t *testing.T) {
 					)
 				}
 
-				prev = hostnameState(current)
+				prev = current
 			}
 		})
 	}
@@ -162,8 +164,9 @@ func TestInconsistencyAlert(t *testing.T) {
 			prev := hostnameState(tt.loaded)
 
 			for range 3 {
-				w.DetectHostnameChanges(t.Context(), host, prev, disagree)
-				prev = hostnameState(disagree)
+				current := hostnameState(disagree)
+				w.DetectHostnameChanges(t.Context(), host, prev, current)
+				prev = current
 			}
 
 			got := 0

internal/watcher/interfaces.go

@@ -5,6 +5,7 @@ import (
 	"context"
 
 	"sneak.berlin/go/dnswatcher/internal/portcheck"
+	"sneak.berlin/go/dnswatcher/internal/resolver"
 	"sneak.berlin/go/dnswatcher/internal/tlscheck"
 )
 
@@ -17,11 +18,11 @@ type DNSResolver interface {
 	) ([]string, error)
 
 	// LookupAllRecords queries all record types for a hostname,
-	// returning results keyed by nameserver then record type.
+	// returning each nameserver's response keyed by nameserver.
 	LookupAllRecords(
 		ctx context.Context,
 		hostname string,
-	) (map[string]map[string][]string, error)
+	) (map[string]*resolver.NameserverResponse, error)
 
 	// ResolveIPAddresses resolves a hostname to all IP addresses.
 	ResolveIPAddresses(

internal/watcher/nsfailure_test.go

@@ -0,0 +1,333 @@
+package watcher_test
+
+import (
+	"context"
+	"fmt"
+	"log/slog"
+	"strings"
+	"testing"
+	"time"
+
+	"sneak.berlin/go/dnswatcher/internal/livednstest"
+	"sneak.berlin/go/dnswatcher/internal/resolver"
+	"sneak.berlin/go/dnswatcher/internal/state"
+	"sneak.berlin/go/dnswatcher/internal/watcher"
+)
+
+// answered is what a check saves for a nameserver that answered with
+// these records.
+func answered(records map[string][]string) *state.NameserverRecordState {
+	return &state.NameserverRecordState{Records: records, Status: "ok"}
+}
+
+// failed is what a check saves for a nameserver that did not answer.
+func failed() *state.NameserverRecordState {
+	return &state.NameserverRecordState{
+		Records: map[string][]string{},
+		Status:  "error",
+		Error:   "all queries timed out",
+	}
+}
+
+// saved builds the hostname state a check saves.
+func saved(
+	byNameserver map[string]*state.NameserverRecordState,
+) *state.HostnameState {
+	return &state.HostnameState{RecordsByNameserver: byNameserver}
+}
+
+// alertCounts counts the hostname alerts sent, by kind.
+type alertCounts struct {
+	failures, recoveries, recordChanges, inconsistencies int
+}
+
+// countAlerts runs the hostname change detection from the state loaded
+// at startup through each check in turn, and counts the alerts sent.
+func countAlerts(
+	t *testing.T,
+	loaded *state.HostnameState,
+	checks []*state.HostnameState,
+) alertCounts {
+	t.Helper()
+
+	// The hostname change detection uses only the notifier.
+	notifier := &mockNotifier{}
+	w := watcher.NewForTest(nil, nil, nil, nil, nil, notifier)
+
+	prev := loaded
+
+	for _, current := range checks {
+		w.DetectHostnameChanges(t.Context(), host, prev, current)
+		prev = current
+	}
+
+	var got alertCounts
+
+	for _, n := range notifier.getNotifications() {
+		kind, _, _ := strings.Cut(n.Title, ":")
+
+		switch kind {
+		case "NS Failure":
+			got.failures++
+		case "NS Recovery":
+			got.recoveries++
+		case "Record Change":
+			got.recordChanges++
+		case "Inconsistency":
+			got.inconsistencies++
+		}
+	}
+
+	return got
+}
+
+func TestNSFailureAndRecoveryAlerts(t *testing.T) {
+	t.Parallel()
+
+	records := map[string][]string{"A": {ip1}}
+
+	bothAnswer := saved(map[string]*state.NameserverRecordState{
+		nsA: answered(records), nsB: answered(records),
+	})
+	bFails := saved(map[string]*state.NameserverRecordState{
+		nsA: answered(records), nsB: failed(),
+	})
+	onlyA := saved(map[string]*state.NameserverRecordState{
+		nsA: answered(records),
+	})
+	bAnswersNoRecords := saved(map[string]*state.NameserverRecordState{
+		nsA: answered(records), nsB: answered(map[string][]string{}),
+	})
+	bAnswersDifferently := saved(map[string]*state.NameserverRecordState{
+		nsA: answered(records), nsB: answered(map[string][]string{"A": {ip2}}),
+	})
+
+	// Each case starts from the state loaded at startup and runs the
+	// checks in order.
+	tests := []struct {
+		name   string
+		loaded *state.HostnameState
+		checks []*state.HostnameState
+		want   alertCounts
+	}{
+		{
+			"failure lasting several checks alerts once",
+			bothAnswer, []*state.HostnameState{bFails, bFails, bFails},
+			alertCounts{failures: 1},
+		},
+		{
+			"recovery alerts once",
+			bFails, []*state.HostnameState{bothAnswer, bothAnswer},
+			alertCounts{recoveries: 1},
+		},
+		{
+			"failing again after recovering alerts again",
+			bothAnswer, []*state.HostnameState{bFails, bothAnswer, bFails},
+			alertCounts{failures: 2, recoveries: 1},
+		},
+		{
+			"nameserver failing when first seen does not alert",
+			onlyA, []*state.HostnameState{bFails, bFails},
+			alertCounts{},
+		},
+		{
+			"answer with no records is a record change, not a failure",
+			bothAnswer, []*state.HostnameState{bAnswersNoRecords},
+			alertCounts{recordChanges: 1, inconsistencies: 1},
+		},
+		{
+			"recovered nameserver that answers differently disagrees",
+			bFails, []*state.HostnameState{bAnswersDifferently},
+			alertCounts{recoveries: 1, inconsistencies: 1},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			got := countAlerts(t, tt.loaded, tt.checks)
+			if got != tt.want {
+				t.Errorf("sent %+v, want %+v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestNSFailureAlertNamesHostnameNameserverAndReason(t *testing.T) {
+	t.Parallel()
+
+	records := map[string][]string{"A": {ip1}}
+
+	notifier := &mockNotifier{}
+	w := watcher.NewForTest(nil, nil, nil, nil, nil, notifier)
+
+	w.DetectHostnameChanges(
+		t.Context(), host,
+		saved(map[string]*state.NameserverRecordState{nsA: answered(records)}),
+		saved(map[string]*state.NameserverRecordState{nsA: failed()}),
+	)
+
+	notifications := notifier.getNotifications()
+	if len(notifications) != 1 {
+		t.Fatalf("sent %v, want one NS Failure", notifications)
+	}
+
+	msg := notifications[0].Message
+	if !strings.Contains(msg, host) || !strings.Contains(msg, nsA) ||
+		!strings.Contains(msg, failed().Error) {
+		t.Errorf(
+			"message %q does not name %s, %s and the reason",
+			msg, host, nsA,
+		)
+	}
+}
+
+// TestNameserverThatNeverAnswers asks a nameserver address where
+// nothing answers, 192.0.2.1, and checks what the watcher saves for it.
+// The deadline outlasts the resolver's first two-second try, as in the
+// resolver's timeout test.
+func TestNameserverThatNeverAnswers(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithTimeout(t.Context(), 3*time.Second)
+	t.Cleanup(cancel)
+
+	res := resolver.NewFromLogger(slog.Default())
+
+	resp, err := res.QueryNameserverIP(ctx, nsA, "192.0.2.1", host)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	hs := watcher.BuildHostnameState(
+		map[string]*resolver.NameserverResponse{nsA: resp}, time.Now(),
+	)
+
+	got := hs.RecordsByNameserver[nsA]
+	if got.Status != "error" || got.Error == "" {
+		t.Errorf(
+			"saved status %q, error %q; want status error with a reason",
+			got.Status, got.Error,
+		)
+	}
+}
+
+// TestNameserverThatAnswersNXDOMAIN asks a real nameserver about a name
+// that does not exist and checks what the watcher saves for it: NXDOMAIN
+// is an answer, so the nameserver is saved as ok with no error.
+func TestNameserverThatAnswersNXDOMAIN(t *testing.T) {
+	t.Parallel()
+
+	res := resolver.NewFromLogger(slog.Default())
+	name := "this-surely-does-not-exist-xyz." + testDomain
+
+	var (
+		ns   string
+		resp *resolver.NameserverResponse
+	)
+
+	livednstest.Retry(t, "QueryNameserver("+name+")", func(ctx context.Context) error {
+		nameservers, err := res.LookupNS(ctx, testDomain)
+		if err != nil {
+			return err
+		}
+
+		ns = nameservers[0]
+
+		resp, err = res.QueryNameserver(ctx, ns, name)
+		if err != nil {
+			return err
+		}
+
+		// A timeout or a failure is no answer to check.
+		if resp.Status == resolver.StatusTimeout ||
+			resp.Status == resolver.StatusError {
+			return fmt.Errorf(
+				"%w: %s: %s", livednstest.ErrNoAnswer, ns, resp.Error,
+			)
+		}
+
+		return nil
+	})
+
+	if resp.Status != resolver.StatusNXDomain {
+		t.Fatalf("%s answered %q for %s, want NXDOMAIN", ns, resp.Status, name)
+	}
+
+	hs := watcher.BuildHostnameState(
+		map[string]*resolver.NameserverResponse{ns: resp}, time.Now(),
+	)
+
+	got := hs.RecordsByNameserver[ns]
+	if got.Status != "ok" || got.Error != "" {
+		t.Errorf(
+			"saved status %q, error %q; want status ok with no error",
+			got.Status, got.Error,
+		)
+	}
+}
+
+// TestNameserverThatRefuses asks a google.com nameserver about
+// cloudflare.com, a zone it does not serve, which it refuses, and checks
+// what the watcher saves for it: REFUSED is no answer, so the nameserver
+// is saved as error with the reason.
+func TestNameserverThatRefuses(t *testing.T) {
+	t.Parallel()
+
+	const reason = "server returned REFUSED"
+
+	res := resolver.NewFromLogger(slog.Default())
+
+	var (
+		ns   string
+		resp *resolver.NameserverResponse
+	)
+
+	livednstest.Retry(
+		t,
+		"QueryNameserver(cloudflare.com)",
+		func(ctx context.Context) error {
+			nameservers, err := res.LookupNS(ctx, testDomain)
+			if err != nil {
+				return err
+			}
+
+			ns = nameservers[0]
+
+			resp, err = res.QueryNameserver(ctx, ns, "cloudflare.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: %s", livednstest.ErrNoAnswer, ns, resp.Error,
+				)
+			}
+
+			return nil
+		},
+	)
+
+	if resp.Error != reason {
+		t.Fatalf(
+			"%s answered %q (%s) for cloudflare.com, want REFUSED",
+			ns, resp.Status, resp.Error,
+		)
+	}
+
+	hs := watcher.BuildHostnameState(
+		map[string]*resolver.NameserverResponse{ns: resp}, time.Now(),
+	)
+
+	got := hs.RecordsByNameserver[ns]
+	if got.Status != failed().Status || got.Error != reason {
+		t.Errorf(
+			"saved status %q, error %q; want status %q, error %q",
+			got.Status, got.Error, failed().Status, reason,
+		)
+	}
+}

internal/watcher/watcher.go

@@ -13,6 +13,7 @@ import (
 
 	"sneak.berlin/go/dnswatcher/internal/config"
 	"sneak.berlin/go/dnswatcher/internal/logger"
+	"sneak.berlin/go/dnswatcher/internal/resolver"
 	"sneak.berlin/go/dnswatcher/internal/state"
 	"sneak.berlin/go/dnswatcher/internal/tlscheck"
 )
@@ -227,7 +228,7 @@ func (w *Watcher) checkDomain(
 	// Also look up A/AAAA records for the apex domain so that
 	// port and TLS checks (which read HostnameState) can find
 	// the domain's IP addresses.
-	records, err := w.resolver.LookupAllRecords(ctx, domain)
+	results, err := w.resolver.LookupAllRecords(ctx, domain)
 	if err != nil {
 		w.log.Error(
 			"failed to lookup records for domain",
@@ -238,12 +239,13 @@ func (w *Watcher) checkDomain(
 		return
 	}
 
+	newState := buildHostnameState(results, now)
+
 	prevHS, hasPrevHS := w.state.GetHostnameState(domain)
 	if hasPrevHS && !w.firstRun {
-		w.detectHostnameChanges(ctx, domain, prevHS, records)
+		w.detectHostnameChanges(ctx, domain, prevHS, newState)
 	}
 
-	newState := buildHostnameState(records, now)
 	w.state.SetHostnameState(domain, newState)
 }
 
@@ -292,7 +294,7 @@ func (w *Watcher) checkHostname(
 	ctx context.Context,
 	hostname string,
 ) {
-	records, err := w.resolver.LookupAllRecords(ctx, hostname)
+	results, err := w.resolver.LookupAllRecords(ctx, hostname)
 	if err != nil {
 		w.log.Error(
 			"failed to lookup records",
@@ -303,19 +305,22 @@ func (w *Watcher) checkHostname(
 		return
 	}
 
-	now := time.Now().UTC()
-	prev, hasPrev := w.state.GetHostnameState(hostname)
+	newState := buildHostnameState(results, time.Now().UTC())
 
+	prev, hasPrev := w.state.GetHostnameState(hostname)
 	if hasPrev && !w.firstRun {
-		w.detectHostnameChanges(ctx, hostname, prev, records)
+		w.detectHostnameChanges(ctx, hostname, prev, newState)
 	}
 
-	newState := buildHostnameState(records, now)
 	w.state.SetHostnameState(hostname, newState)
 }
 
+// buildHostnameState saves each nameserver's response. A nameserver
+// that answered, even with NXDOMAIN or no records, is saved as ok; one
+// that timed out or failed is saved as error with the reason, and its
+// empty record set is not an answer.
 func buildHostnameState(
-	records map[string]map[string][]string,
+	results map[string]*resolver.NameserverResponse,
 	now time.Time,
 ) *state.HostnameState {
 	hs := &state.HostnameState{
@@ -325,12 +330,20 @@ func buildHostnameState(
 		LastChecked: now,
 	}
 
-	for ns, recs := range records {
-		hs.RecordsByNameserver[ns] = &state.NameserverRecordState{
-			Records:     recs,
+	for ns, resp := range results {
+		nsState := &state.NameserverRecordState{
+			Records:     resp.Records,
 			Status:      statusOK,
 			LastChecked: now,
 		}
+
+		if resp.Status == resolver.StatusTimeout ||
+			resp.Status == resolver.StatusError {
+			nsState.Status = statusError
+			nsState.Error = resp.Error
+		}
+
+		hs.RecordsByNameserver[ns] = nsState
 	}
 
 	return hs
@@ -339,27 +352,29 @@ func buildHostnameState(
 func (w *Watcher) detectHostnameChanges(
 	ctx context.Context,
 	hostname string,
-	prev *state.HostnameState,
-	current map[string]map[string][]string,
+	prev, current *state.HostnameState,
 ) {
 	w.detectRecordChanges(ctx, hostname, prev, current)
 	w.detectNSDisappearances(ctx, hostname, prev, current)
+	w.detectNSFailures(ctx, hostname, prev, current)
 	w.detectInconsistencies(ctx, hostname, prev, current)
 }
 
+// detectRecordChanges compares each nameserver's records with those of
+// the previous check. Only answers are compared: a nameserver that
+// failed on either check has no records to compare.
 func (w *Watcher) detectRecordChanges(
 	ctx context.Context,
 	hostname string,
-	prev *state.HostnameState,
-	current map[string]map[string][]string,
+	prev, current *state.HostnameState,
 ) {
-	for ns, recs := range current {
+	for ns, cur := range current.RecordsByNameserver {
 		prevNS, ok := prev.RecordsByNameserver[ns]
-		if !ok {
+		if !ok || prevNS.Status != statusOK || cur.Status != statusOK {
 			continue
 		}
 
-		if recordsEqual(prevNS.Records, recs) {
+		if recordsEqual(prevNS.Records, cur.Records) {
 			continue
 		}
 
@@ -367,7 +382,7 @@ func (w *Watcher) detectRecordChanges(
 			"Hostname: %s\nNameserver: %s\n"+
 				"Old: %v\nNew: %v",
 			hostname, ns,
-			prevNS.Records, recs,
+			prevNS.Records, cur.Records,
 		)
 
 		w.notify.SendNotification(
@@ -382,11 +397,10 @@ func (w *Watcher) detectRecordChanges(
 func (w *Watcher) detectNSDisappearances(
 	ctx context.Context,
 	hostname string,
-	prev *state.HostnameState,
-	current map[string]map[string][]string,
+	prev, current *state.HostnameState,
 ) {
 	for ns, prevNS := range prev.RecordsByNameserver {
-		if _, ok := current[ns]; ok || prevNS.Status != statusOK {
+		if _, ok := current.RecordsByNameserver[ns]; ok || prevNS.Status != statusOK {
 			continue
 		}
 
@@ -402,13 +416,36 @@ func (w *Watcher) detectNSDisappearances(
 			"error",
 		)
 	}
+}
 
-	for ns := range current {
+// detectNSFailures notifies when a nameserver that answered on the
+// previous check fails, and when one that failed answers again. A
+// nameserver missing from the previous check is not compared.
+func (w *Watcher) detectNSFailures(
+	ctx context.Context,
+	hostname string,
+	prev, current *state.HostnameState,
+) {
+	for ns, cur := range current.RecordsByNameserver {
 		prevNS, ok := prev.RecordsByNameserver[ns]
-		if !ok || prevNS.Status != statusError {
+		if !ok {
 			continue
 		}
 
+		switch {
+		case prevNS.Status == statusOK && cur.Status == statusError:
+			msg := fmt.Sprintf(
+				"Hostname: %s\nNameserver: %s\nError: %s",
+				hostname, ns, cur.Error,
+			)
+
+			w.notify.SendNotification(
+				ctx,
+				"NS Failure: "+hostname,
+				msg,
+				"error",
+			)
+		case prevNS.Status == statusError && cur.Status == statusOK:
 			msg := fmt.Sprintf(
 				"Hostname: %s\nNameserver: %s recovered",
 				hostname, ns,
@@ -421,13 +458,13 @@ func (w *Watcher) detectNSDisappearances(
 				"success",
 			)
 		}
+	}
 }
 
 func (w *Watcher) detectInconsistencies(
 	ctx context.Context,
 	hostname string,
-	prev *state.HostnameState,
-	current map[string]map[string][]string,
+	prev, current *state.HostnameState,
 ) {
 	for _, pair := range newlyDisagreeingPairs(prev, current) {
 		ns1, ns2 := pair[0], pair[1]
@@ -435,8 +472,8 @@ func (w *Watcher) detectInconsistencies(
 		msg := fmt.Sprintf(
 			"Hostname: %s\n%s: %v\n%s: %v",
 			hostname,
-			ns1, current[ns1],
-			ns2, current[ns2],
+			ns1, current.RecordsByNameserver[ns1].Records,
+			ns2, current.RecordsByNameserver[ns2].Records,
 		)
 
 		w.notify.SendNotification(
@@ -448,18 +485,21 @@ func (w *Watcher) detectInconsistencies(
 	}
 }
 
-// newlyDisagreeingPairs returns every pair of nameservers whose records
-// differ in current, in sorted order of name, except pairs where both
-// nameservers were in prev and already differed there. A nameserver
-// missing from prev is paired with every nameserver it differs from.
+// newlyDisagreeingPairs returns every pair of nameservers that answered
+// in current and whose records differ there, in sorted order of name,
+// except pairs where both nameservers answered in prev and already
+// differed there. A nameserver missing from prev, or that failed there,
+// is paired with every nameserver it differs from. A nameserver that
+// failed in current has no records to compare and is in no pair.
 func newlyDisagreeingPairs(
-	prev *state.HostnameState,
-	current map[string]map[string][]string,
+	prev, current *state.HostnameState,
 ) [][2]string {
-	nameservers := make([]string, 0, len(current))
-	for ns := range current {
+	nameservers := make([]string, 0, len(current.RecordsByNameserver))
+	for ns, cur := range current.RecordsByNameserver {
+		if cur.Status == statusOK {
 			nameservers = append(nameservers, ns)
 		}
+	}
 
 	sort.Strings(nameservers)
 
@@ -467,14 +507,19 @@ func newlyDisagreeingPairs(
 
 	for i, ns1 := range nameservers {
 		for _, ns2 := range nameservers[i+1:] {
-			if recordsEqual(current[ns1], current[ns2]) {
+			if recordsEqual(
+				current.RecordsByNameserver[ns1].Records,
+				current.RecordsByNameserver[ns2].Records,
+			) {
 				continue
 			}
 
 			prev1, ok1 := prev.RecordsByNameserver[ns1]
 			prev2, ok2 := prev.RecordsByNameserver[ns2]
 
-			if ok1 && ok2 && !recordsEqual(prev1.Records, prev2.Records) {
+			if ok1 && ok2 &&
+				prev1.Status == statusOK && prev2.Status == statusOK &&
+				!recordsEqual(prev1.Records, prev2.Records) {
 				continue
 			}
 

internal/watcher/watcher_test.go

@@ -687,11 +687,12 @@ func TestNSFailureAndRecovery(t *testing.T) {
 	cfg.Hostnames = []string{testHost}
 
 	// Between the checks, save every nameserver the first check found
-	// as failed, and add, as answering, one that live DNS does not list.
+	// 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 _, nsState := range hs.RecordsByNameserver {
-			nsState.Status = "error"
+		for ns := range hs.RecordsByNameserver {
+			hs.RecordsByNameserver[ns] = failed()
 		}
 
 		hs.RecordsByNameserver[oldNS1] = &state.NameserverRecordState{
@@ -704,4 +705,10 @@ func TestNSFailureAndRecovery(t *testing.T) {
 
 	assertNotified(t, deps, "NS Failure: "+testHost, "error")
 	assertNotified(t, deps, "NS Recovery: "+testHost, "success")
+
+	// A nameserver that did not answer has no records to compare, so
+	// its recovery is not also a record change.
+	if n := countNotifications(deps, "Record Change: "+testHost); n != 0 {
+		t.Errorf("sent %d record changes on recovery, want 0", n)
+	}
 }