diff --git a/README.md b/README.md index 6650221..ec6a55d 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. - Also watches the domain's own records as a hostname's are watched (see DNS Hostname Monitoring below): its A, AAAA, CNAME, MX, TXT, SRV, CAA and NS records, stored per nameserver. Their changes are notified as a hostname's @@ -97,6 +98,19 @@ 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 the previous check did not know the type's records either, + because the nameserver was new or failing then or the type was already listed + in `unknownTypes`, 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 @@ -125,8 +139,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 @@ -545,7 +560,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 @@ -554,6 +569,13 @@ 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 the previous check did not know the type's records either, +because the nameserver was new or failing then or the type was already listed in +`unknownTypes`, 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. @@ -561,10 +583,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's `hostnames` lists every name that resolves to its address, domains included. A port entry in the older format, with one `hostname` instead diff --git a/TODO.md b/TODO.md index 2d97fbb..2895ca3 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: a Port Change notification lists the port's domains on a `Domains:` line and its hostnames on a `Hostnames:` line (closes #248). - 2026-10-02: the dashboard's Ports table and `/api/v1/status` port entries list diff --git a/internal/resolver/errors.go b/internal/resolver/errors.go index f603217..61ca007 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 07c293b..a0e4103 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 5686622..4b15543 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -8,6 +8,7 @@ import ( "net" "slices" "sort" + "strconv" "strings" "time" @@ -99,6 +100,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, @@ -320,13 +324,20 @@ func (r *Resolver) queryServers( return nil, fmt.Errorf("all servers failed: %w", lastErr) } +// isErrorReply reports whether msg is an error reply: one with any code +// but NOERROR and NXDOMAIN, such as SERVFAIL, NOTIMP or FORMERR. An error +// reply says nothing about the name's records. +func isErrorReply(msg *dns.Msg) bool { + return msg.Rcode != dns.RcodeSuccess && msg.Rcode != dns.RcodeNameError +} + // usableReply reports whether resp, a reply from one of the servers of // zone to a query about name, is usable. An error reply such as SERVFAIL // is not. Nor is a referral, unless it refers the query to a zone below // zone that name is in: a server that refers it back to zone, up or // sideways does not serve zone as it should. func usableReply(resp *dns.Msg, zone string, name string) bool { - if resp.Rcode != dns.RcodeSuccess && resp.Rcode != dns.RcodeNameError { + if isErrorReply(resp) { return false } @@ -714,15 +725,21 @@ func (r *Resolver) queryTypes( } type queryState struct { - gotNXDomain bool - gotSERVFAIL bool - gotRefused bool - gotTimeout bool - gotReferral bool - netErr error - hasRecords bool + gotNXDomain bool + gotErrorReply bool + errorReply string // its code, such as SERVFAIL, or number if unnamed + gotRefused bool + gotTimeout bool + 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 unless ctx was +// cancelled: shutdown cancels it, and a query it cut short did not fail. func (r *Resolver) queryEachType( ctx context.Context, nsIP string, @@ -737,7 +754,34 @@ 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) + + if errors.Is(ctx.Err(), context.Canceled) { + continue + } + + 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 { @@ -747,6 +791,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, @@ -754,7 +801,7 @@ func (r *Resolver) querySingleType( qtype uint16, resp *NameserverResponse, state *queryState, -) { +) error { msg, err := r.queryDNS(ctx, nsIP, hostname, qtype) if err != nil { switch { @@ -766,19 +813,40 @@ func (r *Resolver) querySingleType( state.netErr = err } - return + return err } + return readReply(msg, resp, state) +} + +// readReply adds to resp the records in msg, a nameserver's reply to a +// query about one record type. It returns nil when the nameserver +// answered: with records, with none, or with NXDOMAIN; otherwise it +// returns why not. +func readReply( + msg *dns.Msg, + resp *NameserverResponse, + state *queryState, +) error { if msg.Rcode == dns.RcodeNameError { state.gotNXDomain = true - return + return nil } - if msg.Rcode == dns.RcodeServerFailure { - state.gotSERVFAIL = true + if isErrorReply(msg) { + state.gotErrorReply = true - return + code, named := dns.RcodeToString[msg.Rcode] + if !named { + code = strconv.Itoa(msg.Rcode) + } + + state.errorReply = code + + return fmt.Errorf( + "server returned %s: %w", state.errorReply, ErrUnusableReply, + ) } // A reply with no answer that lists other nameservers, from a server @@ -791,10 +859,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 } // collectAnswerRecords adds the records in msg's answer to resp, each @@ -833,23 +911,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.gotErrorReply && !state.answered: resp.Status = StatusError - resp.Error = "server returned SERVFAIL" - case state.gotRefused && !state.hasRecords: + resp.Error = "server returned " + state.errorReply + 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: @@ -1010,9 +1091,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) { @@ -1025,7 +1108,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..f36097d --- /dev/null +++ b/internal/resolver/iterative_internal_test.go @@ -0,0 +1,131 @@ +package resolver + +import ( + "strconv" + "syscall" + "testing" + + "github.com/miekg/dns" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// 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 got +// no usable reply, whatever the reason; one whose every query got none +// 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, "", + }, + { + "some types answered with no records, another got SERVFAIL", + queryState{ + answered: true, gotErrorReply: true, errorReply: "SERVFAIL", + }, + StatusNoData, "", + }, + { + "some types answered with no records, another was refused", + queryState{answered: true, gotRefused: true}, + StatusNoData, "", + }, + { + "some types answered with no records, another got a network error", + queryState{answered: true, netErr: syscall.ECONNREFUSED}, + StatusNoData, "", + }, + { + "some types answered with no records, another's reply was " + + "truncated and its retry over TCP failed", + queryState{answered: true, netErr: ErrTruncated}, + StatusNoData, "", + }, + { + "some types answered with no records, another got a referral", + queryState{answered: true, gotReferral: true}, + StatusNoData, "", + }, + { + "every query timed out", + queryState{gotTimeout: true}, + StatusTimeout, "all queries timed out", + }, + { + "every query got NOTIMP", + queryState{gotErrorReply: true, errorReply: "NOTIMP"}, + StatusError, "server returned NOTIMP", + }, + } + + 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) + }) + } +} + +// TestReadReply checks which replies to a query about one record type, +// built here, are an answer: one with the code NOERROR or NXDOMAIN. A +// reply with any other code is not, and the type's query has failed; a +// nameserver whose only reply it is has failed, and Error gives the +// code, or its number when the code has no name. +func TestReadReply(t *testing.T) { + t.Parallel() + + tests := []struct { + rcode int + wantStatus string + wantError string + }{ + {dns.RcodeSuccess, StatusNoData, ""}, + {dns.RcodeNameError, StatusNXDomain, ""}, + {dns.RcodeServerFailure, StatusError, "server returned SERVFAIL"}, + {dns.RcodeNotImplemented, StatusError, "server returned NOTIMP"}, + {dns.RcodeFormatError, StatusError, "server returned FORMERR"}, + {12, StatusError, "server returned 12"}, // unassigned, no name + } + + for _, tt := range tests { + t.Run(strconv.Itoa(tt.rcode), func(t *testing.T) { + t.Parallel() + + msg := new(dns.Msg) + msg.Authoritative = true + msg.Rcode = tt.rcode + + resp := &NameserverResponse{Records: map[string][]string{}} + + var state queryState + + err := readReply(msg, resp, &state) + classifyResponse(resp, state) + + if tt.wantStatus == StatusError { + require.ErrorIs(t, err, ErrUnusableReply) + } else { + require.NoError(t, err) + } + + 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 ca422cd..836b555 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 ac45b89..94a2fa0 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -1,6 +1,7 @@ package resolver_test import ( + "bytes" "context" "errors" "fmt" @@ -366,6 +367,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() @@ -1001,6 +1026,29 @@ func TestQueryNameserverIP_Timeout(t *testing.T) { assert.NotEmpty(t, resp.Error) } +// TestQueryNameserverIP_CancelledLogsNothing cancels the context while +// a query to 192.0.2.1, where nothing answers, is waiting for a reply, +// as shutdown does. The query was cut short, not failed, so nothing is +// logged. +func TestQueryNameserverIP_CancelledLogsNothing(t *testing.T) { + t.Parallel() + + var logs bytes.Buffer + + r := resolver.NewFromLogger(slog.New(slog.NewTextHandler(&logs, nil))) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + time.AfterFunc(100*time.Millisecond, cancel) + + _, err := r.QueryNameserverIP( + ctx, "unreachable.test.", "192.0.2.1", "example.com", + ) + require.NoError(t, err) + + assert.Empty(t, logs.String()) +} + // TestCollectIPs_NoNameserverAnswered takes the response of a // nameserver at 192.0.2.1, where nothing answers, as // TestQueryNameserverIP_Timeout does. Addresses collected from diff --git a/internal/state/state.go b/internal/state/state.go index 4f5a325..eecca4e 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -46,11 +46,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/state/state_test.go b/internal/state/state_test.go index b2f7391..e511014 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -236,6 +236,61 @@ func TestSaveLoadRoundTrip_CNAMEAddresses(t *testing.T) { } } +// TestSaveLoadRoundTrip_FailedTypes checks that a nameserver's +// failedTypes and unknownTypes survive a save and load. Without +// unknownTypes, a type whose records were not known would load as one +// with no records. +func TestSaveLoadRoundTrip_FailedTypes(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + s := state.NewForTestWithDataDir(dir) + + failed := []string{"TXT", "CAA"} + unknown := []string{"CAA"} + + s.SetHostnameState(testHostname, &state.HostnameState{ + RecordsByNameserver: map[string]*state.NameserverRecordState{ + testNS1: { + Records: map[string][]string{"TXT": {"v=spf1 -all"}}, + FailedTypes: failed, + UnknownTypes: unknown, + Status: "ok", + }, + }, + }) + + err := s.Save() + if err != nil { + t.Fatalf("Save() error: %v", err) + } + + loaded := state.NewForTestWithDataDir(dir) + + err = loaded.Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + + hs, ok := loaded.GetHostnameState(testHostname) + if !ok { + t.Fatal("missing hostname " + testHostname) + } + + ns1 := hs.RecordsByNameserver[testNS1] + if ns1 == nil { + t.Fatal("missing nameserver " + testNS1) + } + + if !reflect.DeepEqual(ns1.FailedTypes, failed) { + t.Errorf("failedTypes: got %#v", ns1.FailedTypes) + } + + if !reflect.DeepEqual(ns1.UnknownTypes, unknown) { + t.Errorf("unknownTypes: got %#v", ns1.UnknownTypes) + } +} + // TestLoadStateFromBeforeCNAMEAddresses loads a state file written // before the addresses at the end of a hostname's CNAME chain were // saved. They load as not known (nil), not as none. 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 e932a2c..e527a2d 100644 --- a/internal/watcher/export_test.go +++ b/internal/watcher/export_test.go @@ -120,7 +120,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 0a09014..5649e92 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "maps" "slices" "sort" "strings" @@ -401,8 +402,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()), ) } @@ -432,8 +435,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, @@ -451,7 +455,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 } @@ -497,11 +504,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{ @@ -513,7 +522,7 @@ func buildHostnameState( for ns, resp := range results { nsState := &state.NameserverRecordState{ - Records: resp.Records, + Records: maps.Clone(resp.Records), Status: statusOK, LastChecked: now, } @@ -522,6 +531,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 @@ -530,6 +548,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, @@ -618,7 +657,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, @@ -630,7 +672,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 } @@ -638,8 +684,8 @@ func (w *Watcher) detectRecordChanges( "%s\nNameserver: %s\n%s", w.nameLine(hostname), ns, recordDifferences( - "Old", prevNS.Records, - "New", cur.Records, + "Old", oldRecords, + "New", newRecords, ), ) @@ -726,13 +772,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( "%s\n%s", w.nameLine(hostname), recordDifferences( - ns1, current.RecordsByNameserver[ns1].Records, - ns2, current.RecordsByNameserver[ns2].Records, + ns1, withoutTypes(state1.Records, failed), + ns2, withoutTypes(state2.Records, failed), ), ) @@ -750,7 +802,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 { @@ -767,9 +821,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 } @@ -779,7 +833,7 @@ func newlyDisagreeingPairs( if ok1 && ok2 && prev1.Status == statusOK && prev2.Status == statusOK && - !recordsEqual(prev1.Records, prev2.Records) { + !nameserversAgree(prev1, prev2) { continue } @@ -1268,6 +1322,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 {