diff --git a/README.md b/README.md index 9b4ade1..0365448 100644 --- a/README.md +++ b/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 @@ -458,10 +465,14 @@ not as a merged view, to enable inconsistency detection. 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) | +| Status | Meaning | +|-------------|------------------------------------------------------------| +| `ok` | Query succeeded, records are current | +| `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`. --- diff --git a/TODO.md b/TODO.md index 2d00cb7..3fae221 100644 --- a/TODO.md +++ b/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: diff --git a/internal/resolver/export_test.go b/internal/resolver/export_test.go index 6f4f9f1..d8e382b 100644 --- a/internal/resolver/export_test.go +++ b/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) +} diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index f89d73d..1e0b494 100644 --- a/internal/resolver/iterative.go +++ b/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 diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index 28544f7..2946519 100644 --- a/internal/resolver/resolver_test.go +++ b/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, diff --git a/internal/watcher/export_test.go b/internal/watcher/export_test.go index c647a6e..5bebe13 100644 --- a/internal/watcher/export_test.go +++ b/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) +} diff --git a/internal/watcher/inconsistency_test.go b/internal/watcher/inconsistency_test.go index 9128426..494ec2b 100644 --- a/internal/watcher/inconsistency_test.go +++ b/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 diff --git a/internal/watcher/interfaces.go b/internal/watcher/interfaces.go index dd68017..41f53bc 100644 --- a/internal/watcher/interfaces.go +++ b/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( diff --git a/internal/watcher/nsfailure_test.go b/internal/watcher/nsfailure_test.go new file mode 100644 index 0000000..91a5580 --- /dev/null +++ b/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, + ) + } +} diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index c3d59f0..2d18f85 100644 --- a/internal/watcher/watcher.go +++ b/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,32 +416,55 @@ 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 } - msg := fmt.Sprintf( - "Hostname: %s\nNameserver: %s recovered", - hostname, ns, - ) + 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 Recovery: "+hostname, - msg, - "success", - ) + 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, + ) + + w.notify.SendNotification( + ctx, + "NS Recovery: "+hostname, + msg, + "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,17 +485,20 @@ 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 = append(nameservers, ns) + 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 } diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 0620f8a..5f23678 100644 --- a/internal/watcher/watcher_test.go +++ b/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) + } }