diff --git a/README.md b/README.md index ffbb132..7c5f5c3 100644 --- a/README.md +++ b/README.md @@ -80,7 +80,8 @@ notification endpoint set, changes show only on the dashboard; see different addresses than on the previous check. A nameserver added or removed gets only the NS change notification. When the lookup of a nameserver's addresses fails or finds none, its previous addresses are - kept and nothing is sent. + kept and nothing is sent. The lookup fails when no nameserver it asks + answers every one of its queries, for A, AAAA and CNAME. ### DNS Hostname Monitoring (Subdomains) @@ -91,6 +92,18 @@ notification endpoint set, changes show only on the dashboard; see its last two labels (a name under `co.uk`, or in a delegated subdomain). - Queries **each** authoritative nameserver independently for **all** record types: A, AAAA, CNAME, MX, TXT, SRV, CAA, NS. +- Each record type is a query of its own. When a nameserver answers some types + but the query for another gets no usable reply (no reply after two tries, an + error reply such as SERVFAIL, a referral, or a reply too large for UDP whose + retry over TCP fails), the failure is logged with the reason, and the type is + listed in the nameserver's `failedTypes` and keeps the records saved for the + nameserver by the previous check. On that check those records are not compared + with the other nameservers', so no record change or inconsistency is reported + for the type; on the next check they are compared with the nameserver's answer + as usual. When there are none to keep, because the nameserver was new or + failing on the previous check, the type is also listed in `unknownTypes` and + left out of every comparison until it answers. A nameserver none of whose + queries got a usable reply has failed (see NS query failure below). - Stores results **per nameserver**. The state for a hostname is not a merged view — it is a map from nameserver to record set. - DNS names inside record values (CNAME, MX, SRV and NS targets) are stored in @@ -119,8 +132,9 @@ notification endpoint set, changes show only on the dashboard; see 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 reported on the check where it - answers. If a pair agrees again and later disagrees, the alert is sent - again. + answers. So is a pair that differs in a record type whose query to either + nameserver failed on the previous check. If a pair agrees again and later + disagrees, the alert is sent again. - **CNAME address change**: The addresses at the end of a name's CNAME chain differ from those of the previous check. They are found when its nameservers answer with a CNAME and no address; a name that answers with @@ -519,7 +533,7 @@ reachability: | Status | Meaning | | ------- | -------------------------------------------------------- | -| `ok` | Query succeeded, records are current | +| `ok` | Query succeeded, records are current except as below | | `error` | Query failed (timeout, SERVFAIL, REFUSED, network error) | A nameserver that answers NXDOMAIN or with no records has status `ok` and empty @@ -528,6 +542,12 @@ nameservers, has status `error`, empty `records`, and the reason in `error`. A certificate entry whose TLS connection or handshake failed likewise has status `error`, the reason in `error`, and the certificate fields left empty or zero. +A nameserver with status `ok` whose query for one record type failed lists that +type in `failedTypes` and holds its records from the previous check, which may +not be current. When there were none to keep, the type is also listed in +`unknownTypes`, and `records` holds nothing for it. Both lists are left out when +empty. + `nameserverAddresses` lists, by nameserver, the sorted addresses its name resolves to. A state file without it loads, and the next check fills it in without a notification. @@ -535,10 +555,10 @@ without a notification. `cnameAddresses` lists the sorted addresses at the end of the chain of every CNAME target a hostname's nameservers gave, found when they answered with a CNAME and no address; it is empty when they answered with an address. When a -chain cannot be followed, or none of the name's nameservers answered, the -previous check's list is kept, or `null` when no earlier check saved one. A -state file without it loads, and the first check after that saves it without a -notification. +chain cannot be followed, or none of the name's nameservers answered its queries +for A, AAAA and CNAME, the previous check's list is kept, or `null` when no +earlier check saved one. A state file without it loads, and the first check +after that saves it without a notification. A port entry in the older format, with one `hostname` instead of the `hostnames` list, loads as a list of that one name. diff --git a/TODO.md b/TODO.md index 41521ae..58022e1 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,8 @@ trial run of the finished image: https://git.eeqj.de/sneak/dnswatcher/issues/149 # Completed Steps +- 2026-10-02: a record type whose query to a nameserver fails keeps its previous + records and alerts nothing; the other types are still saved (closes #231). - 2026-10-02: Record Change and Inconsistency notifications list only the record types that differ, each with its values as plain text (closes #219). - 2026-10-02: the startup notification no longer says every notification diff --git a/internal/resolver/errors.go b/internal/resolver/errors.go index 5e019c0..83b091a 100644 --- a/internal/resolver/errors.go +++ b/internal/resolver/errors.go @@ -22,6 +22,12 @@ var ( "reply is an error or a referral that leads no closer", ) + // ErrTruncated is the reason given for a reply too large for UDP + // whose retry over TCP failed. + ErrTruncated = errors.New( + "reply truncated and its retry over TCP failed", + ) + // ErrIntercepted is returned when every root server refused a // query. Root servers refuse no query, so the refusals came from // something on the network answering in their place. diff --git a/internal/resolver/export_test.go b/internal/resolver/export_test.go index 17c57d7..2cb7592 100644 --- a/internal/resolver/export_test.go +++ b/internal/resolver/export_test.go @@ -2,10 +2,21 @@ package resolver import ( "context" + "log/slog" + "time" "github.com/miekg/dns" ) +// NewWithFailingTCP returns a Resolver whose TCP client gives up before +// it can connect, so the retry over TCP of every truncated reply fails. +func NewWithFailingTCP(log *slog.Logger) *Resolver { + r := NewFromLogger(log) + r.tcp = &tcpClient{timeout: time.Nanosecond} + + return r +} + // ExtractRecordValue exports extractRecordValue for testing. func ExtractRecordValue(rr dns.RR) string { return extractRecordValue(rr) diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index 1f06a24..6a27423 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -89,6 +89,9 @@ func (r *Resolver) tryExchange( return resp, err } +// retryTCP returns the reply to msg over TCP when resp, its reply over +// UDP, is truncated. When that fails it returns resp, still truncated, +// which holds only the records that fit. func (r *Resolver) retryTCP( ctx context.Context, msg *dns.Msg, @@ -638,8 +641,12 @@ type queryState struct { gotReferral bool netErr error hasRecords bool + answered bool } +// queryEachType asks the nameserver at nsIP about hostname once for each +// record type in qtypes, and lists in resp.FailedTypes the types whose +// query got no usable reply, logging each with the reason. func (r *Resolver) queryEachType( ctx context.Context, nsIP string, @@ -654,7 +661,30 @@ func (r *Resolver) queryEachType( break } - r.querySingleType(ctx, nsIP, hostname, qtype, resp, &state) + err := r.querySingleType(ctx, nsIP, hostname, qtype, resp, &state) + if err == nil { + state.answered = true + + continue + } + + rtype := dns.TypeToString[qtype] + resp.FailedTypes = append(resp.FailedTypes, rtype) + + r.log.Warn( + "record type query failed", + "hostname", hostname, + "nameserver", resp.Nameserver, + "type", rtype, + "error", err, + ) + } + + // The reply about another type can carry the name's CNAME. When the + // query for CNAME itself failed, that is left out too, so Records + // holds nothing for a failed type. + for _, rtype := range resp.FailedTypes { + delete(resp.Records, rtype) } for k := range resp.Records { @@ -664,6 +694,9 @@ func (r *Resolver) queryEachType( return state } +// querySingleType asks the nameserver at nsIP about hostname's records +// of type qtype. It returns nil when the nameserver answered: with +// records, with none, or with NXDOMAIN; otherwise it returns why not. func (r *Resolver) querySingleType( ctx context.Context, nsIP string, @@ -671,7 +704,7 @@ func (r *Resolver) querySingleType( qtype uint16, resp *NameserverResponse, state *queryState, -) { +) error { msg, err := r.queryDNS(ctx, nsIP, hostname, qtype) if err != nil { switch { @@ -683,19 +716,19 @@ func (r *Resolver) querySingleType( state.netErr = err } - return + return err } if msg.Rcode == dns.RcodeNameError { state.gotNXDomain = true - return + return nil } if msg.Rcode == dns.RcodeServerFailure { state.gotSERVFAIL = true - return + return fmt.Errorf("server returned SERVFAIL: %w", ErrUnusableReply) } // A reply with no answer that lists other nameservers, from a server @@ -708,10 +741,20 @@ func (r *Resolver) querySingleType( len(extractNSSet(msg.Ns)) > 0 { state.gotReferral = true - return + return fmt.Errorf("server returned a referral: %w", ErrUnusableReply) + } + + // A reply still truncated is one whose TCP retry failed, and holds + // only the records that fit. + if msg.Truncated { + state.netErr = ErrTruncated + + return ErrTruncated } collectAnswerRecords(msg, resp, state) + + return nil } func collectAnswerRecords( @@ -743,23 +786,26 @@ func isTimeout(err error) bool { return false } +// classifyResponse sets the nameserver's status. One that answered no +// record type has failed, and Error says why; one that answered some has +// the status of those answers. func classifyResponse(resp *NameserverResponse, state queryState) { switch { case state.gotNXDomain && !state.hasRecords: resp.Status = StatusNXDomain - case state.gotTimeout && !state.hasRecords: + case state.gotTimeout && !state.answered: resp.Status = StatusTimeout resp.Error = "all queries timed out" - case state.gotSERVFAIL && !state.hasRecords: + case state.gotSERVFAIL && !state.answered: resp.Status = StatusError resp.Error = "server returned SERVFAIL" - case state.gotRefused && !state.hasRecords: + case state.gotRefused && !state.answered: resp.Status = StatusError resp.Error = "server returned REFUSED" - case state.netErr != nil && !state.hasRecords: + case state.netErr != nil && !state.answered: resp.Status = StatusError resp.Error = "network error: " + state.netErr.Error() - case state.gotReferral && !state.hasRecords: + case state.gotReferral && !state.answered: resp.Status = StatusError resp.Error = "server returned a referral" case !state.hasRecords && !state.gotNXDomain: @@ -920,9 +966,11 @@ func (r *Resolver) resolveIPWithCNAME( } // collectIPs returns the addresses in the nameservers' answers and the -// first CNAME target among them. It returns ErrNoNameserverAnswered when -// every nameserver timed out, failed or returned a referral: that is not -// a name with no addresses. +// first CNAME target among them. A nameserver whose query for one of the +// types failed gave only part of the addresses, and is left out. It +// returns ErrNoNameserverAnswered when every nameserver timed out, +// failed, returned a referral or was left out: that is not a name with +// no addresses. func collectIPs( results map[string]*NameserverResponse, ) ([]string, string, error) { @@ -935,7 +983,8 @@ func collectIPs( answered := false for _, resp := range results { - if resp.Status == StatusTimeout || resp.Status == StatusError { + if resp.Status == StatusTimeout || resp.Status == StatusError || + len(resp.FailedTypes) > 0 { continue } diff --git a/internal/resolver/iterative_internal_test.go b/internal/resolver/iterative_internal_test.go new file mode 100644 index 0000000..880d18b --- /dev/null +++ b/internal/resolver/iterative_internal_test.go @@ -0,0 +1,45 @@ +package resolver + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestClassifyResponse sets a nameserver's status from the results of +// its queries, built here. One that answered some record types, even +// with no records, has not failed when its query for another type timed +// out; one whose every query timed out has. +func TestClassifyResponse(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + results queryState + wantStatus string + wantError string + }{ + { + "some types answered with no records, another timed out", + queryState{answered: true, gotTimeout: true}, + StatusNoData, "", + }, + { + "every query timed out", + queryState{gotTimeout: true}, + StatusTimeout, "all queries timed out", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + resp := &NameserverResponse{Status: StatusOK} + classifyResponse(resp, tt.results) + + assert.Equal(t, tt.wantStatus, resp.Status) + assert.Equal(t, tt.wantError, resp.Error) + }) + } +} diff --git a/internal/resolver/iterative_test.go b/internal/resolver/iterative_test.go index 4f539ae..00f71eb 100644 --- a/internal/resolver/iterative_test.go +++ b/internal/resolver/iterative_test.go @@ -43,6 +43,25 @@ func TestCollectIPs_FailedIsNoAnswer(t *testing.T) { assert.Empty(t, ips) } +// TestCollectIPs_FailedTypeIsNoAnswer checks that a nameserver whose +// query for one of the types failed is no answer: its addresses are +// only part of them. +func TestCollectIPs_FailedTypeIsNoAnswer(t *testing.T) { + t.Parallel() + + ips, _, err := resolver.CollectIPs( + map[string]*resolver.NameserverResponse{ + nsExample1: { + Records: map[string][]string{"A": {"192.0.2.1"}}, + FailedTypes: []string{"AAAA"}, + Status: resolver.StatusOK, + }, + }, + ) + require.ErrorIs(t, err, resolver.ErrNoNameserverAnswered) + assert.Empty(t, ips) +} + const ( // exampleCom is the zone most cases of TestUsableReply and // TestNSSetFrom are about, and wwwExampleCom a name in it. diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go index 83b3f47..1bdfdb9 100644 --- a/internal/resolver/resolver.go +++ b/internal/resolver/resolver.go @@ -31,11 +31,15 @@ type Params struct { } // NameserverResponse holds one nameserver's response for a query. +// FailedTypes lists the record types whose query got no usable reply, +// and Records holds nothing for them: their records are not known. When +// no record type got one, Status and Error say the nameserver failed. type NameserverResponse struct { - Nameserver string - Records map[string][]string - Status string - Error string + Nameserver string + Records map[string][]string + FailedTypes []string + Status string + Error string } // Resolver performs iterative DNS resolution from root servers. diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index a07c718..51829f8 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -1,6 +1,7 @@ package resolver_test import ( + "bytes" "context" "fmt" "log/slog" @@ -245,6 +246,30 @@ func TestQueryNameserver_TXT(t *testing.T) { ) } +// TestQueryNameserver_TruncatedReplyWhoseTCPRetryFails asks a google.com +// nameserver about google.com with a resolver whose retries over TCP +// fail. google.com's TXT records do not fit in a reply over UDP, so TXT +// is reported as failed, holding none of the records that fit, and +// logged with the reason, while the nameserver, which answered the other +// types, is ok. +func TestQueryNameserver_TruncatedReplyWhoseTCPRetryFails(t *testing.T) { + t.Parallel() + + ns := findOneNSForDomain(t, newTestResolver(t), "google.com") + + var logs bytes.Buffer + + r := resolver.NewWithFailingTCP(slog.New(slog.NewTextHandler(&logs, nil))) + resp := liveQueryNameserver(t, r, ns, "google.com") + + assert.Equal(t, resolver.StatusOK, resp.Status) + assert.Contains(t, resp.FailedTypes, "TXT") + assert.NotContains(t, resp.Records, "TXT") + assert.Contains(t, logs.String(), + "hostname=google.com. nameserver="+ns+" type=TXT error=", + ) +} + func TestQueryNameserver_NXDomain(t *testing.T) { t.Parallel() diff --git a/internal/state/state.go b/internal/state/state.go index 47ea4c6..dc266ac 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -45,11 +45,18 @@ type DomainState struct { } // NameserverRecordState holds one NS's response for a hostname. +// FailedTypes lists the record types whose query to the nameserver +// failed on this check: Records holds for them the records saved by the +// previous check, which are kept. UnknownTypes lists those of them whose +// records the previous check did not know either, as when the +// nameserver was new or failing then: Records holds nothing for them. type NameserverRecordState struct { - Records map[string][]string `json:"records"` - Status string `json:"status"` - Error string `json:"error,omitempty"` - LastChecked time.Time `json:"lastChecked"` + Records map[string][]string `json:"records"` + FailedTypes []string `json:"failedTypes,omitempty"` + UnknownTypes []string `json:"unknownTypes,omitempty"` + Status string `json:"status"` + Error string `json:"error,omitempty"` + LastChecked time.Time `json:"lastChecked"` } // HostnameState holds per-nameserver monitoring state for a hostname. diff --git a/internal/watcher/cname_test.go b/internal/watcher/cname_test.go index b3c549f..9f492f1 100644 --- a/internal/watcher/cname_test.go +++ b/internal/watcher/cname_test.go @@ -202,6 +202,42 @@ func TestCNAMEWhoseNameserversAllFailedKeepsPrevious(t *testing.T) { } } +// TestCNAMEWhoseAddressQueryFailedKeepsPrevious checks a name whose +// nameserver answered, but whose query for A, AAAA or CNAME failed with +// nothing kept for it. That is not an answer with no address: the +// addresses the previous check saved from following its CNAME are kept, +// and nothing is looked up, the watcher having no resolver. +func TestCNAMEWhoseAddressQueryFailedKeepsPrevious(t *testing.T) { + t.Parallel() + + for _, rtype := range []string{"A", "AAAA", "CNAME"} { + t.Run(rtype, func(t *testing.T) { + t.Parallel() + + w := watcher.NewForTest(nil, nil, nil, nil, nil, nil) + + current := saved(map[string]*state.NameserverRecordState{ + nsA: { + Records: map[string][]string{}, + FailedTypes: []string{rtype}, + UnknownTypes: []string{rtype}, + Status: "ok", + }, + }) + prev := cnameState(oldIP) + + w.ResolveCNAMEAddresses(t.Context(), host, current, prev) + + if !slices.Equal(current.CNAMEAddresses, prev.CNAMEAddresses) { + t.Errorf( + "saved %v, want %v", + current.CNAMEAddresses, prev.CNAMEAddresses, + ) + } + }) + } +} + // cnameTo builds the records of a nameserver that answered with a CNAME // to target and no address. func cnameTo(target string) map[string][]string { diff --git a/internal/watcher/export_test.go b/internal/watcher/export_test.go index 218a566..4641313 100644 --- a/internal/watcher/export_test.go +++ b/internal/watcher/export_test.go @@ -103,7 +103,8 @@ func (w *Watcher) RunTLSChecks(ctx context.Context) { // BuildHostnameState exports buildHostnameState for testing. func BuildHostnameState( results map[string]*resolver.NameserverResponse, + prev *state.HostnameState, now time.Time, ) *state.HostnameState { - return buildHostnameState(results, now) + return buildHostnameState(results, prev, now) } diff --git a/internal/watcher/failedtype_test.go b/internal/watcher/failedtype_test.go new file mode 100644 index 0000000..832ebf2 --- /dev/null +++ b/internal/watcher/failedtype_test.go @@ -0,0 +1,364 @@ +package watcher_test + +import ( + "maps" + "slices" + "testing" + "time" + + "sneak.berlin/go/dnswatcher/internal/resolver" + "sneak.berlin/go/dnswatcher/internal/state" + "sneak.berlin/go/dnswatcher/internal/watcher" +) + +const ( + // txt is the record type whose query fails in these tests. + txt = "TXT" + spf1 = "v=spf1 -all" + spf2 = "v=spf1 include:example.net -all" +) + +// response is a nameserver's response with these records, whose queries +// for failedTypes failed. +func response( + records map[string][]string, + failedTypes ...string, +) *resolver.NameserverResponse { + return &resolver.NameserverResponse{ + Records: records, + FailedTypes: failedTypes, + Status: resolver.StatusOK, + } +} + +// savedChecks saves the state of each check in turn from the +// nameservers' responses, each from the state the check before saved. +func savedChecks( + checks ...map[string]*resolver.NameserverResponse, +) []*state.HostnameState { + states := make([]*state.HostnameState, 0, len(checks)) + + var prev *state.HostnameState + + for _, results := range checks { + prev = watcher.BuildHostnameState(results, prev, time.Now()) + states = append(states, prev) + } + + return states +} + +// TestFailedTypeKeepsPreviousRecords saves a check in which nsA's query +// for TXT failed, after previous checks of several kinds. TXT is always +// saved in FailedTypes, and in UnknownTypes when there was nothing to +// keep. +func TestFailedTypeKeepsPreviousRecords(t *testing.T) { + t.Parallel() + + aOnly := map[string][]string{"A": {ip1}} + withTXT := map[string][]string{"A": {ip1}, txt: {spf1}} + txtKept := &state.NameserverRecordState{ + Records: withTXT, FailedTypes: []string{txt}, Status: "ok", + } + txtNotKnown := &state.NameserverRecordState{ + Records: aOnly, + FailedTypes: []string{txt}, + UnknownTypes: []string{txt}, + Status: "ok", + } + + tests := []struct { + name string + prev *state.HostnameState + wantRecords map[string][]string + wantUnknown []string + }{ + { + "previous TXT records are kept", + saved(map[string]*state.NameserverRecordState{nsA: answered(withTXT)}), + withTXT, nil, + }, + { + "previous check had no TXT records", + saved(map[string]*state.NameserverRecordState{nsA: answered(aOnly)}), + aOnly, nil, + }, + { + "TXT failed on the previous check, which kept its records", + saved(map[string]*state.NameserverRecordState{nsA: txtKept}), + withTXT, nil, + }, + {"first check", nil, aOnly, []string{txt}}, + { + "nameserver new on this check", + saved(map[string]*state.NameserverRecordState{nsB: answered(withTXT)}), + aOnly, []string{txt}, + }, + { + "nameserver failed on the previous check", + saved(map[string]*state.NameserverRecordState{nsA: failed()}), + aOnly, []string{txt}, + }, + { + "TXT failed on the previous check with nothing to keep", + saved(map[string]*state.NameserverRecordState{nsA: txtNotKnown}), + aOnly, []string{txt}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + hs := watcher.BuildHostnameState( + map[string]*resolver.NameserverResponse{ + nsA: response(map[string][]string{"A": {ip1}}, txt), + }, + tt.prev, time.Now(), + ) + + got := hs.RecordsByNameserver[nsA] + if got.Status != "ok" || + !maps.EqualFunc(got.Records, tt.wantRecords, slices.Equal) || + !slices.Equal(got.FailedTypes, []string{txt}) || + !slices.Equal(got.UnknownTypes, tt.wantUnknown) { + t.Errorf( + "saved status %q, records %v, failed types %v, "+ + "unknown types %v; want ok, %v, [%s], %v", + got.Status, got.Records, got.FailedTypes, + got.UnknownTypes, tt.wantRecords, txt, tt.wantUnknown, + ) + } + }) + } +} + +// TestFailedTypeAlerts saves the checks of each case in turn from the +// nameservers' responses, the first being the state loaded at startup, +// and counts the alerts sent. nsB's TXT query fails on one check, and +// nothing changes. +func TestFailedTypeAlerts(t *testing.T) { + t.Parallel() + + records := map[string][]string{"A": {ip1}, txt: {spf1}} + aOnly := map[string][]string{"A": {ip1}} + + bothAnswer := map[string]*resolver.NameserverResponse{ + nsA: response(records), nsB: response(records), + } + bTXTFails := map[string]*resolver.NameserverResponse{ + nsA: response(records), nsB: response(aOnly, txt), + } + onlyA := map[string]*resolver.NameserverResponse{ + nsA: response(records), + } + bFails := map[string]*resolver.NameserverResponse{ + nsA: response(records), + nsB: { + Records: map[string][]string{}, + Status: resolver.StatusTimeout, + Error: "all queries timed out", + }, + } + + tests := []struct { + name string + checks []map[string]*resolver.NameserverResponse + want alertCounts + }{ + { + "type failing at one nameserver alerts nothing, nor its next answer", + []map[string]*resolver.NameserverResponse{ + bothAnswer, bTXTFails, bothAnswer, + }, + alertCounts{}, + }, + { + "type failing on the first check alerts nothing on the next", + []map[string]*resolver.NameserverResponse{bTXTFails, bothAnswer}, + alertCounts{}, + }, + { + "type failing at a nameserver new on that check alerts nothing", + []map[string]*resolver.NameserverResponse{ + onlyA, bTXTFails, bothAnswer, + }, + alertCounts{}, + }, + { + "type failing at a recovering nameserver alerts the recovery", + []map[string]*resolver.NameserverResponse{ + bFails, bTXTFails, bothAnswer, + }, + alertCounts{recoveries: 1}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + states := savedChecks(tt.checks...) + + got := countAlerts(t, states[0], states[1:]) + if got != tt.want { + t.Errorf("sent %+v, want %+v", got, tt.want) + } + }) + } +} + +// TestFailedTypeComparedOnceItAnswers saves the checks of each case in +// turn as TestFailedTypeAlerts does. nsB's TXT query fails on one check, +// and the TXT record changes: the change is sent as a Record Change for +// each nameserver on the check where it answers it, and an Inconsistency +// only when nsB still answers the old record. +func TestFailedTypeComparedOnceItAnswers(t *testing.T) { + t.Parallel() + + records := map[string][]string{"A": {ip1}, txt: {spf1}} + changed := map[string][]string{"A": {ip1}, txt: {spf2}} + aOnly := map[string][]string{"A": {ip1}} + + bothAnswer := map[string]*resolver.NameserverResponse{ + nsA: response(records), nsB: response(records), + } + bTXTFails := map[string]*resolver.NameserverResponse{ + nsA: response(records), nsB: response(aOnly, txt), + } + bothChange := map[string]*resolver.NameserverResponse{ + nsA: response(changed), nsB: response(changed), + } + aChangesBTXTFails := map[string]*resolver.NameserverResponse{ + nsA: response(changed), nsB: response(aOnly, txt), + } + bStillOld := map[string]*resolver.NameserverResponse{ + nsA: response(changed), nsB: response(records), + } + + tests := []struct { + name string + checks []map[string]*resolver.NameserverResponse + want alertCounts + }{ + { + "change made while the type failed", + []map[string]*resolver.NameserverResponse{ + bothAnswer, bTXTFails, bothChange, + }, + alertCounts{recordChanges: 2}, + }, + { + "change seen at one nameserver while the other's type failed", + []map[string]*resolver.NameserverResponse{ + bothAnswer, aChangesBTXTFails, bothChange, + }, + alertCounts{recordChanges: 2}, + }, + { + "old record answered after the type failed", + []map[string]*resolver.NameserverResponse{ + bothAnswer, aChangesBTXTFails, bStillOld, + }, + alertCounts{recordChanges: 1, inconsistencies: 1}, + }, + { + "change after the type failed on the first check and answered", + []map[string]*resolver.NameserverResponse{ + bTXTFails, bothAnswer, bothChange, + }, + alertCounts{recordChanges: 2}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + states := savedChecks(tt.checks...) + + got := countAlerts(t, states[0], states[1:]) + if got != tt.want { + t.Errorf("sent %+v, want %+v", got, tt.want) + } + }) + } +} + +// TestFailedTypeLeftOutOfMessages checks that a Record Change and an +// Inconsistency name only the record types they compared. nsB's TXT +// records are not known on the first check, and on the second either +// answered or still not known; nsB's A record changes, so both alerts +// are sent and name the A record alone. +func TestFailedTypeLeftOutOfMessages(t *testing.T) { + t.Parallel() + + withTXT := map[string][]string{"A": {ip1}, txt: {spf1}} + txtNotKnown := func(address string) *state.NameserverRecordState { + return &state.NameserverRecordState{ + Records: map[string][]string{"A": {address}}, + FailedTypes: []string{txt}, + UnknownTypes: []string{txt}, + Status: "ok", + } + } + + before := saved(map[string]*state.NameserverRecordState{ + nsA: answered(withTXT), nsB: txtNotKnown(ip1), + }) + + tests := []struct { + name string + after *state.HostnameState + }{ + { + "TXT answers", + saved(map[string]*state.NameserverRecordState{ + nsA: answered(withTXT), + nsB: answered(map[string][]string{"A": {ip2}, txt: {spf1}}), + }), + }, + { + "TXT still not known", + saved(map[string]*state.NameserverRecordState{ + nsA: answered(withTXT), nsB: txtNotKnown(ip2), + }), + }, + } + + want := map[string]string{ + "Record Change: " + host: "Hostname: " + host + + "\nNameserver: " + nsB + "\nType: A\nOld: " + ip1 + "\nNew: " + ip2, + "Inconsistency: " + host: "Hostname: " + host + + "\nType: A\n" + nsA + ": " + ip1 + "\n" + nsB + ": " + ip2, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // The hostname change detection uses only the notifier. + notifier := &mockNotifier{} + w := watcher.NewForTest(nil, nil, nil, nil, nil, notifier) + + w.DetectHostnameChanges(t.Context(), host, before, tt.after) + + notifications := notifier.getNotifications() + if len(notifications) != len(want) { + t.Fatalf( + "sent %d notifications, want %d: %v", + len(notifications), len(want), notifications, + ) + } + + for _, n := range notifications { + if n.Message != want[n.Title] { + t.Errorf( + "%s message:\n%s\nwant:\n%s", + n.Title, n.Message, want[n.Title], + ) + } + } + }) + } +} diff --git a/internal/watcher/nsfailure_test.go b/internal/watcher/nsfailure_test.go index afae908..e9147c1 100644 --- a/internal/watcher/nsfailure_test.go +++ b/internal/watcher/nsfailure_test.go @@ -201,7 +201,7 @@ func TestNameserverThatNeverAnswers(t *testing.T) { } hs := watcher.BuildHostnameState( - map[string]*resolver.NameserverResponse{nsA: resp}, time.Now(), + map[string]*resolver.NameserverResponse{nsA: resp}, nil, time.Now(), ) got := hs.RecordsByNameserver[nsA] @@ -256,7 +256,7 @@ func TestNameserverThatAnswersNXDOMAIN(t *testing.T) { } hs := watcher.BuildHostnameState( - map[string]*resolver.NameserverResponse{ns: resp}, time.Now(), + map[string]*resolver.NameserverResponse{ns: resp}, nil, time.Now(), ) got := hs.RecordsByNameserver[ns] @@ -320,7 +320,7 @@ func TestNameserverThatRefuses(t *testing.T) { } hs := watcher.BuildHostnameState( - map[string]*resolver.NameserverResponse{ns: resp}, time.Now(), + map[string]*resolver.NameserverResponse{ns: resp}, nil, time.Now(), ) got := hs.RecordsByNameserver[ns] diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 001ee95..4f01b2e 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log/slog" + "maps" "slices" "sort" "strings" @@ -381,8 +382,10 @@ func (w *Watcher) checkHostname( return } + prev, _ := w.state.GetHostnameState(hostname) + w.updateHostnameState( - ctx, hostname, buildHostnameState(results, time.Now().UTC()), + ctx, hostname, buildHostnameState(results, prev, time.Now().UTC()), ) } @@ -412,8 +415,9 @@ func (w *Watcher) updateHostnameState( // the addresses found for all of them are saved, so nameservers that // disagree on the target do not change the result from check to check. // The addresses saved in prev, which may be nil, are kept when none of -// the name's nameservers answered, and when a target cannot be -// followed, as when no nameserver of a zone in its chain answers. +// the name's nameservers answered its queries for A, AAAA and CNAME, +// and when a target cannot be followed, as when no nameserver of a zone +// in its chain answers. func (w *Watcher) resolveCNAMEAddresses( ctx context.Context, hostname string, @@ -431,7 +435,10 @@ func (w *Watcher) resolveCNAMEAddresses( targets := make(map[string]bool) for _, nsState := range current.RecordsByNameserver { - if nsState.Status != statusOK { + if nsState.Status != statusOK || + slices.Contains(nsState.FailedTypes, "A") || + slices.Contains(nsState.FailedTypes, "AAAA") || + slices.Contains(nsState.FailedTypes, "CNAME") { continue } @@ -476,11 +483,13 @@ func (w *Watcher) resolveCNAMEAddresses( } // 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. +// that answered, even with NXDOMAIN or no records, is saved as ok, with +// the record types whose query failed; one that timed out or failed is +// saved as error with the reason, and its empty record set is not an +// answer. prev is the hostname's state from the previous check, or nil. func buildHostnameState( results map[string]*resolver.NameserverResponse, + prev *state.HostnameState, now time.Time, ) *state.HostnameState { hs := &state.HostnameState{ @@ -492,7 +501,7 @@ func buildHostnameState( for ns, resp := range results { nsState := &state.NameserverRecordState{ - Records: resp.Records, + Records: maps.Clone(resp.Records), Status: statusOK, LastChecked: now, } @@ -501,6 +510,15 @@ func buildHostnameState( resp.Status == resolver.StatusError { nsState.Status = statusError nsState.Error = resp.Error + } else { + nsState.FailedTypes = resp.FailedTypes + + var prevNS *state.NameserverRecordState + if prev != nil { + prevNS = prev.RecordsByNameserver[ns] + } + + keepFailedTypes(nsState, prevNS) } hs.RecordsByNameserver[ns] = nsState @@ -509,6 +527,27 @@ func buildHostnameState( return hs } +// keepFailedTypes copies into nsState, for each record type in its +// FailedTypes, the records prevNS, the nameserver's state from the +// previous check, holds for that type, which may be none. When prevNS +// does not know them either, because the nameserver was new or failing +// then or the type was in its UnknownTypes, the type goes in +// nsState.UnknownTypes instead. +func keepFailedTypes(nsState, prevNS *state.NameserverRecordState) { + for _, rtype := range nsState.FailedTypes { + if prevNS == nil || prevNS.Status != statusOK || + slices.Contains(prevNS.UnknownTypes, rtype) { + nsState.UnknownTypes = append(nsState.UnknownTypes, rtype) + + continue + } + + if records, ok := prevNS.Records[rtype]; ok { + nsState.Records[rtype] = records + } + } +} + func (w *Watcher) detectHostnameChanges( ctx context.Context, hostname string, @@ -553,7 +592,10 @@ func (w *Watcher) detectCNAMEAddressChanges( // 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. +// failed on either check has no records to compare. The records kept +// for a record type whose query failed are compared too, but not those +// of a type in UnknownTypes on either check, which the message leaves +// out as well. func (w *Watcher) detectRecordChanges( ctx context.Context, hostname string, @@ -565,7 +607,11 @@ func (w *Watcher) detectRecordChanges( continue } - if recordsEqual(prevNS.Records, cur.Records) { + unknown := slices.Concat(prevNS.UnknownTypes, cur.UnknownTypes) + oldRecords := withoutTypes(prevNS.Records, unknown) + newRecords := withoutTypes(cur.Records, unknown) + + if recordsEqual(oldRecords, newRecords) { continue } @@ -573,8 +619,8 @@ func (w *Watcher) detectRecordChanges( "Hostname: %s\nNameserver: %s\n%s", hostname, ns, recordDifferences( - "Old", prevNS.Records, - "New", cur.Records, + "Old", oldRecords, + "New", newRecords, ), ) @@ -661,13 +707,19 @@ func (w *Watcher) detectInconsistencies( ) { for _, pair := range newlyDisagreeingPairs(prev, current) { ns1, ns2 := pair[0], pair[1] + state1 := current.RecordsByNameserver[ns1] + state2 := current.RecordsByNameserver[ns2] + + // The record types left out of the comparison are left out of + // the message too. + failed := slices.Concat(state1.FailedTypes, state2.FailedTypes) msg := fmt.Sprintf( "Hostname: %s\n%s", hostname, recordDifferences( - ns1, current.RecordsByNameserver[ns1].Records, - ns2, current.RecordsByNameserver[ns2].Records, + ns1, withoutTypes(state1.Records, failed), + ns2, withoutTypes(state2.Records, failed), ), ) @@ -685,7 +737,9 @@ func (w *Watcher) detectInconsistencies( // 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. +// failed in current has no records to compare and is in no pair. In +// both checks, a record type whose query failed at either nameserver is +// not compared. func newlyDisagreeingPairs( prev, current *state.HostnameState, ) [][2]string { @@ -702,9 +756,9 @@ func newlyDisagreeingPairs( for i, ns1 := range nameservers { for _, ns2 := range nameservers[i+1:] { - if recordsEqual( - current.RecordsByNameserver[ns1].Records, - current.RecordsByNameserver[ns2].Records, + if nameserversAgree( + current.RecordsByNameserver[ns1], + current.RecordsByNameserver[ns2], ) { continue } @@ -714,7 +768,7 @@ func newlyDisagreeingPairs( if ok1 && ok2 && prev1.Status == statusOK && prev2.Status == statusOK && - !recordsEqual(prev1.Records, prev2.Records) { + !nameserversAgree(prev1, prev2) { continue } @@ -1201,6 +1255,33 @@ func toSet(items []string) map[string]bool { return set } +// nameserversAgree reports whether two nameservers' states from the same +// check hold the same records, leaving out the record types either lists +// in FailedTypes: the records held for those are kept from an earlier +// check, or not known. +func nameserversAgree(a, b *state.NameserverRecordState) bool { + failed := slices.Concat(a.FailedTypes, b.FailedTypes) + + return recordsEqual( + withoutTypes(a.Records, failed), withoutTypes(b.Records, failed), + ) +} + +// withoutTypes returns a copy of records without the record types in +// types. +func withoutTypes( + records map[string][]string, + types []string, +) map[string][]string { + records = maps.Clone(records) + + for _, rtype := range types { + delete(records, rtype) + } + + return records +} + func recordsEqual( a, b map[string][]string, ) bool {