middleware: take the client address from the right of X-Forwarded-For (closes #181)

realIP took the first X-Forwarded-For entry, which the client itself
can write, so behind a proxy that appends to the header a client chose
the address dnswatcher logs and the /metrics rate limit counts. It now
walks the entries from the right past trusted proxies, using the
existing trusted-proxy check, and takes the first that is not one; the
leftmost when all are. All X-Forwarded-For header lines are read as one
list, since a proxy may add its own line instead of appending to the
client's. An empty entry where the client address belongs falls back
to the peer address, as an empty first entry did before. X-Real-IP is
unchanged.

Model: opus-5-5

README.md

@@ -71,25 +71,18 @@ 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": a nameserver
+    This is distinct from "responded with no records."
-    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. Its records are not compared with those from
+    responding again.
-    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), or failed on it, and answers differently is
+    after dropping out) and answers differently is reported on the check
-    reported on the check where it answers. If a pair agrees again and
+    where it appears. If a pair agrees again and later disagrees, the
-    later disagrees, the alert is sent again.
+    alert is sent again.
 
 ### TCP Port Monitoring
 
@@ -466,13 +459,9 @@ 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, REFUSED, network error)   |
+| `error`     | Query failed (timeout, SERVFAIL, 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,14 +15,13 @@ on the 1.0 milestone: https://git.eeqj.de/sneak/dnswatcher/milestone/7
 
 # Next Step
 
-nameserver IP address changes: https://git.eeqj.de/sneak/dnswatcher/issues/105
+NS failure and NS recovery notifications:
+https://git.eeqj.de/sneak/dnswatcher/issues/104
 
 # Completed Steps
 
 - 2026-10-01: the client address from `X-Forwarded-For` is the last entry that
   is not a trusted proxy, not the first, which the client sets (closes #181).
-- 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,
@@ -99,6 +98,7 @@ nameserver IP address changes: https://git.eeqj.de/sneak/dnswatcher/issues/105
 
 # 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,21 +1,8 @@
 package resolver
 
-import (
+import "github.com/miekg/dns"
-	"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,9 +504,7 @@ func (r *Resolver) queryAllTypes(
 type queryState struct {
 	gotNXDomain bool
 	gotSERVFAIL bool
-	gotRefused  bool
 	gotTimeout  bool
-	netErr      error
 	hasRecords  bool
 }
 
@@ -544,13 +542,8 @@ func (r *Resolver) querySingleType(
 ) {
 	msg, err := r.queryDNS(ctx, nsIP, hostname, qtype)
 	if err != nil {
-		switch {
+		if isTimeout(err) {
-		case isTimeout(err):
 			state.gotTimeout = true
-		case errors.Is(err, ErrRefused):
-			state.gotRefused = true
-		default:
-			state.netErr = err
 		}
 
 		return
@@ -610,12 +603,6 @@ 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
 	}
@@ -695,14 +682,11 @@ 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,
@@ -730,13 +714,21 @@ 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]*NameserverResponse, error) {
+) (map[string]map[string][]string, error) {
-	return r.QueryAllNameservers(ctx, hostname)
+	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
 }
 
 // ResolveIPAddresses resolves a hostname to all IPv4 and IPv6

internal/resolver/resolver_test.go

@@ -2,7 +2,6 @@ package resolver_test
 
 import (
 	"context"
-	"fmt"
 	"log/slog"
 	"net"
 	"os"
@@ -14,7 +13,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"sneak.berlin/go/dnswatcher/internal/livednstest"
 	"sneak.berlin/go/dnswatcher/internal/resolver"
 )
 
@@ -233,45 +231,6 @@ 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()
 
@@ -559,29 +518,6 @@ 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
 // ----------------------------------------------------------------
@@ -594,7 +530,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 error and
+	// makes the status vary from run to run between nodata 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,7 +6,6 @@ import (
 	"time"
 
 	"sneak.berlin/go/dnswatcher/internal/config"
-	"sneak.berlin/go/dnswatcher/internal/resolver"
 	"sneak.berlin/go/dnswatcher/internal/state"
 )
 
@@ -34,7 +33,8 @@ func NewForTest(
 
 // NewlyDisagreeingPairs exports newlyDisagreeingPairs for testing.
 func NewlyDisagreeingPairs(
-	prev, current *state.HostnameState,
+	prev *state.HostnameState,
+	current map[string]map[string][]string,
 ) [][2]string {
 	return newlyDisagreeingPairs(prev, current)
 }
@@ -43,15 +43,8 @@ func NewlyDisagreeingPairs(
 func (w *Watcher) DetectHostnameChanges(
 	ctx context.Context,
 	hostname string,
-	prev, current *state.HostnameState,
+	prev *state.HostnameState,
+	current map[string]map[string][]string,
 ) {
 	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,9 +105,7 @@ func TestNewlyDisagreeingPairs(t *testing.T) {
 
 			prev := hostnameState(tt.loaded)
 
-			for i, records := range tt.checks {
+			for i, current := range tt.checks {
-				current := hostnameState(records)
-
 				got := watcher.NewlyDisagreeingPairs(prev, current)
 				if !slices.Equal(got, tt.want[i]) {
 					t.Errorf(
@@ -116,7 +114,7 @@ func TestNewlyDisagreeingPairs(t *testing.T) {
 					)
 				}
 
-				prev = current
+				prev = hostnameState(current)
 			}
 		})
 	}
@@ -164,9 +162,8 @@ func TestInconsistencyAlert(t *testing.T) {
 			prev := hostnameState(tt.loaded)
 
 			for range 3 {
-				current := hostnameState(disagree)
+				w.DetectHostnameChanges(t.Context(), host, prev, disagree)
-				w.DetectHostnameChanges(t.Context(), host, prev, current)
+				prev = hostnameState(disagree)
-				prev = current
 			}
 
 			got := 0

internal/watcher/interfaces.go

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

internal/watcher/nsfailure_test.go

@@ -1,333 +0,0 @@
-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,7 +13,6 @@ 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"
 )
@@ -228,7 +227,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.
-	results, err := w.resolver.LookupAllRecords(ctx, domain)
+	records, err := w.resolver.LookupAllRecords(ctx, domain)
 	if err != nil {
 		w.log.Error(
 			"failed to lookup records for domain",
@@ -239,13 +238,12 @@ func (w *Watcher) checkDomain(
 		return
 	}
 
-	newState := buildHostnameState(results, now)
-
 	prevHS, hasPrevHS := w.state.GetHostnameState(domain)
 	if hasPrevHS && !w.firstRun {
-		w.detectHostnameChanges(ctx, domain, prevHS, newState)
+		w.detectHostnameChanges(ctx, domain, prevHS, records)
 	}
 
+	newState := buildHostnameState(records, now)
 	w.state.SetHostnameState(domain, newState)
 }
 
@@ -294,7 +292,7 @@ func (w *Watcher) checkHostname(
 	ctx context.Context,
 	hostname string,
 ) {
-	results, err := w.resolver.LookupAllRecords(ctx, hostname)
+	records, err := w.resolver.LookupAllRecords(ctx, hostname)
 	if err != nil {
 		w.log.Error(
 			"failed to lookup records",
@@ -305,22 +303,19 @@ func (w *Watcher) checkHostname(
 		return
 	}
 
-	newState := buildHostnameState(results, time.Now().UTC())
+	now := time.Now().UTC()
-
 	prev, hasPrev := w.state.GetHostnameState(hostname)
+
 	if hasPrev && !w.firstRun {
-		w.detectHostnameChanges(ctx, hostname, prev, newState)
+		w.detectHostnameChanges(ctx, hostname, prev, records)
 	}
 
+	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(
-	results map[string]*resolver.NameserverResponse,
+	records map[string]map[string][]string,
 	now time.Time,
 ) *state.HostnameState {
 	hs := &state.HostnameState{
@@ -330,20 +325,12 @@ func buildHostnameState(
 		LastChecked: now,
 	}
 
-	for ns, resp := range results {
+	for ns, recs := range records {
-		nsState := &state.NameserverRecordState{
+		hs.RecordsByNameserver[ns] = &state.NameserverRecordState{
-			Records:     resp.Records,
+			Records:     recs,
 			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
@@ -352,29 +339,27 @@ func buildHostnameState(
 func (w *Watcher) detectHostnameChanges(
 	ctx context.Context,
 	hostname string,
-	prev, current *state.HostnameState,
+	prev *state.HostnameState,
+	current map[string]map[string][]string,
 ) {
 	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, current *state.HostnameState,
+	prev *state.HostnameState,
+	current map[string]map[string][]string,
 ) {
-	for ns, cur := range current.RecordsByNameserver {
+	for ns, recs := range current {
 		prevNS, ok := prev.RecordsByNameserver[ns]
-		if !ok || prevNS.Status != statusOK || cur.Status != statusOK {
+		if !ok {
 			continue
 		}
 
-		if recordsEqual(prevNS.Records, cur.Records) {
+		if recordsEqual(prevNS.Records, recs) {
 			continue
 		}
 
@@ -382,7 +367,7 @@ func (w *Watcher) detectRecordChanges(
 			"Hostname: %s\nNameserver: %s\n"+
 				"Old: %v\nNew: %v",
 			hostname, ns,
-			prevNS.Records, cur.Records,
+			prevNS.Records, recs,
 		)
 
 		w.notify.SendNotification(
@@ -397,10 +382,11 @@ func (w *Watcher) detectRecordChanges(
 func (w *Watcher) detectNSDisappearances(
 	ctx context.Context,
 	hostname string,
-	prev, current *state.HostnameState,
+	prev *state.HostnameState,
+	current map[string]map[string][]string,
 ) {
 	for ns, prevNS := range prev.RecordsByNameserver {
-		if _, ok := current.RecordsByNameserver[ns]; ok || prevNS.Status != statusOK {
+		if _, ok := current[ns]; ok || prevNS.Status != statusOK {
 			continue
 		}
 
@@ -416,36 +402,13 @@ func (w *Watcher) detectNSDisappearances(
 			"error",
 		)
 	}
-}
 
-// detectNSFailures notifies when a nameserver that answered on the
+	for ns := range current {
-// 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 {
+		if !ok || prevNS.Status != statusError {
 			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,
@@ -458,13 +421,13 @@ func (w *Watcher) detectNSFailures(
 			"success",
 		)
 	}
-	}
 }
 
 func (w *Watcher) detectInconsistencies(
 	ctx context.Context,
 	hostname string,
-	prev, current *state.HostnameState,
+	prev *state.HostnameState,
+	current map[string]map[string][]string,
 ) {
 	for _, pair := range newlyDisagreeingPairs(prev, current) {
 		ns1, ns2 := pair[0], pair[1]
@@ -472,8 +435,8 @@ func (w *Watcher) detectInconsistencies(
 		msg := fmt.Sprintf(
 			"Hostname: %s\n%s: %v\n%s: %v",
 			hostname,
-			ns1, current.RecordsByNameserver[ns1].Records,
+			ns1, current[ns1],
-			ns2, current.RecordsByNameserver[ns2].Records,
+			ns2, current[ns2],
 		)
 
 		w.notify.SendNotification(
@@ -485,21 +448,18 @@ func (w *Watcher) detectInconsistencies(
 	}
 }
 
-// newlyDisagreeingPairs returns every pair of nameservers that answered
+// newlyDisagreeingPairs returns every pair of nameservers whose records
-// in current and whose records differ there, in sorted order of name,
+// differ in current, in sorted order of name, except pairs where both
-// except pairs where both nameservers answered in prev and already
+// nameservers were in prev and already differed there. A nameserver
-// differed there. A nameserver missing from prev, or that failed there,
+// missing from prev is paired with every nameserver it differs from.
-// 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, current *state.HostnameState,
+	prev *state.HostnameState,
+	current map[string]map[string][]string,
 ) [][2]string {
-	nameservers := make([]string, 0, len(current.RecordsByNameserver))
+	nameservers := make([]string, 0, len(current))
-	for ns, cur := range current.RecordsByNameserver {
+	for ns := range current {
-		if cur.Status == statusOK {
 		nameservers = append(nameservers, ns)
 	}
-	}
 
 	sort.Strings(nameservers)
 
@@ -507,19 +467,14 @@ func newlyDisagreeingPairs(
 
 	for i, ns1 := range nameservers {
 		for _, ns2 := range nameservers[i+1:] {
-			if recordsEqual(
+			if recordsEqual(current[ns1], current[ns2]) {
-				current.RecordsByNameserver[ns1].Records,
-				current.RecordsByNameserver[ns2].Records,
-			) {
 				continue
 			}
 
 			prev1, ok1 := prev.RecordsByNameserver[ns1]
 			prev2, ok2 := prev.RecordsByNameserver[ns2]
 
-			if ok1 && ok2 &&
+			if ok1 && ok2 && !recordsEqual(prev1.Records, prev2.Records) {
-				prev1.Status == statusOK && prev2.Status == statusOK &&
-				!recordsEqual(prev1.Records, prev2.Records) {
 				continue
 			}
 

internal/watcher/watcher_test.go

@@ -687,12 +687,11 @@ func TestNSFailureAndRecovery(t *testing.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
+	// as failed, and add, as answering, one that live DNS does not list.
-	// 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 {
+		for _, nsState := range hs.RecordsByNameserver {
-			hs.RecordsByNameserver[ns] = failed()
+			nsState.Status = "error"
 		}
 
 		hs.RecordsByNameserver[oldNS1] = &state.NameserverRecordState{
@@ -705,10 +704,4 @@ 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)
-	}
 }