From 1504b9c64b0032acf4edfbf19d3f2c7cabcebbf8 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 13 Oct 2026 11:27:04 +0000 Subject: [PATCH] watcher: a removed nameserver is an NS Change, not an NS Failure (closes #262) A nameserver that left a domain's delegation was reported twice: as the domain's NS Change, and by the check of the domain's own records as an error-level NS Failure saying it disappeared. The check of a name's records now sends one NS Change at warning level, starting Hostname: or Domain:, listing the nameservers it was asked at before and not now. For a domain it leaves out those of its previous delegation, which the domain's own NS Change lists, so a domain's records are now checked before its new delegation is saved. Nothing is sent for a nameserver that joins. The README says what each case gets. A nameserver that was failing before it left is now listed too; the NS Failure skipped it. Model: opus-5-5 --- README.md | 23 +++++-- TODO.md | 2 + internal/watcher/message_test.go | 103 +++++++++++++++++++++++++++---- internal/watcher/watcher.go | 80 +++++++++++++++--------- internal/watcher/watcher_test.go | 34 +++++++++- 5 files changed, 193 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index 7386c61..2cae056 100644 --- a/README.md +++ b/README.md @@ -99,11 +99,16 @@ notification endpoint set, changes show only on the dashboard; see records, stored per nameserver. Their changes are notified as a hostname's are, as a record change, NS query failure, NS recovery, inconsistency or CNAME address change, in a message that starts `Domain:` where a hostname's starts - `Hostname:`. A domain listed in `DNSWATCHER_SKIP_RECORD_NOTIFICATIONS` gets no - record change or inconsistency notification. A domain with no delegation of - its own has these records asked at the servers of the zone it is in, as a - hostname has. A domain that does not exist has none: they are not asked for, - and those saved by an earlier check are removed without a notification. + `Hostname:`. A nameserver removed from the domain's delegation gets only the + NS change notification above. A domain listed in + `DNSWATCHER_SKIP_RECORD_NOTIFICATIONS` gets no record change or inconsistency + notification. A domain with no delegation of its own has these records asked + at the servers of the zone it is in, as a hostname has. A nameserver the + domain's records were asked at on the previous check but not on this one gets + a hostname's NS change notification (see below), unless it was in the domain's + delegation on the previous check, as when that delegation is withdrawn. A + domain that does not exist has none: they are not asked for, and those saved + by an earlier check are removed without a notification. ### DNS Hostname Monitoring (Subdomains) @@ -156,6 +161,10 @@ notification endpoint set, changes show only on the dashboard; see - **NS recovery**: A previously-unreachable nameserver starts 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. + - **NS change**: A nameserver of the zone the hostname is in on the previous + check is not one now, whether or not it answered. One notification lists + every such nameserver on a `Removed:` line. Nothing is sent for a + nameserver that joins the zone. - **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 @@ -237,7 +246,9 @@ includes: - **DNS record changes**: Which hostname or domain, which nameserver, what record type, old values, new values. -- **DNS NS changes**: Which domain, which nameservers were added/removed. +- **DNS NS changes**: Which domain, which nameservers were added to or removed + from its delegation. For a hostname, or a domain with no delegation of its + own, which nameservers left the zone it is in. - **NS address changes**: Which domain, which nameserver, its old and new addresses. - **CNAME address changes**: Which hostname or domain, the old and new addresses diff --git a/TODO.md b/TODO.md index 368b81f..040cc80 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-13: a nameserver leaving a domain's delegation sends only NS Change; + one leaving a hostname's zone sends NS Change, not NS Failure (closes #262). - 2026-10-13: a bad `PORT`, `DNSWATCHER_TLS_EXPIRY_WARNING` or true/false setting stops startup instead of being read as 0 or false (closes #269). - 2026-10-13: a check saving the state while the dashboard or `/api/v1/status` diff --git a/internal/watcher/message_test.go b/internal/watcher/message_test.go index e446bd2..c9857f1 100644 --- a/internal/watcher/message_test.go +++ b/internal/watcher/message_test.go @@ -5,7 +5,6 @@ import ( "strings" "testing" - "sneak.berlin/go/dnswatcher/internal/config" "sneak.berlin/go/dnswatcher/internal/state" "sneak.berlin/go/dnswatcher/internal/watcher" ) @@ -74,21 +73,19 @@ b.ns.example.net.: 192.0.2.2`, func TestDomainRecordNotificationsNameTheDomain(t *testing.T) { t.Parallel() - notifier := &mockNotifier{} - w := watcher.NewForTest( - &config.Config{Domains: []string{domain}}, - nil, nil, nil, nil, notifier, - ) + cfg := defaultTestConfig(t) + cfg.Domains = []string{domain} + + deps := newTestDeps(t, cfg) + w := watcher.NewForTest(cfg, deps.state, nil, nil, nil, deps.notifier) // nsA's address changes, which also makes it differ from nsC; nsB - // fails; nsC answers again; nsD is gone. - nsD := "d.ns.example.net." + // fails; nsC answers again. w.DetectHostnameChanges(t.Context(), domain, saved(map[string]*state.NameserverRecordState{ nsA: answered(map[string][]string{"A": {ip1}}), nsB: answered(map[string][]string{"A": {ip1}}), nsC: failed(), - nsD: answered(map[string][]string{"A": {ip1}}), }), saved(map[string]*state.NameserverRecordState{ nsA: answered(map[string][]string{"A": {ip2}}), @@ -102,18 +99,17 @@ func TestDomainRecordNotificationsNameTheDomain(t *testing.T) { t.Context(), domain, cnameState(ip1), cnameState(ip2), ) - // NS Failure is sent for nsB failing and for nsD being gone. want := map[string]int{ "Record Change": 1, "Inconsistency": 1, - "NS Failure": 2, + "NS Failure": 1, "NS Recovery": 1, "CNAME Address Change": 1, } sent := make(map[string]int) - for _, n := range notifier.getNotifications() { + for _, n := range deps.notifier.getNotifications() { kind, _, _ := strings.Cut(n.Title, ":") sent[kind]++ @@ -128,6 +124,89 @@ func TestDomainRecordNotificationsNameTheDomain(t *testing.T) { } } +// nsB, which answered, and nsC, which did not, are no longer among the +// nameservers. One NS Change lists both, for the hostname and for the +// configured apex domain with no delegation of its own. Nothing is sent +// for the domain whose delegation on the previous check listed them: +// the check of its delegation sends its NS Change. +func TestRemovedNameserversAreAnNSChange(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + target string + // The domain's nameservers saved by the previous check. + delegation []string + // The NS Change sent, or "" for none. + want string + }{ + { + name: "hostname", + target: host, + want: `Hostname: www.example.net +Removed: b.ns.example.net., c.ns.example.net.`, + }, + { + name: "domain with no delegation of its own", + target: domain, + delegation: []string{}, + want: `Domain: example.net +Removed: b.ns.example.net., c.ns.example.net.`, + }, + { + name: "domain whose delegation listed them", + target: domain, + delegation: []string{nsA, nsB, nsC}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + cfg := defaultTestConfig(t) + cfg.Domains = []string{domain} + + deps := newTestDeps(t, cfg) + w := watcher.NewForTest( + cfg, deps.state, nil, nil, nil, deps.notifier, + ) + + deps.state.SetDomainState(domain, &state.DomainState{ + Nameservers: tt.delegation, + }) + + prev := saved(map[string]*state.NameserverRecordState{ + nsA: answered(map[string][]string{"A": {ip1}}), + nsB: answered(map[string][]string{"A": {ip1}}), + nsC: failed(), + }) + current := saved(map[string]*state.NameserverRecordState{ + nsA: answered(map[string][]string{"A": {ip1}}), + }) + + w.DetectHostnameChanges(t.Context(), tt.target, prev, current) + + got := deps.notifier.getNotifications() + + if tt.want == "" { + if len(got) != 0 { + t.Errorf("sent %v, want nothing", got) + } + + return + } + + title := "NS Change: " + tt.target + if len(got) != 1 || got[0].Title != title || + got[0].Message != tt.want { + t.Errorf("sent %v, want one %q with message:\n%s", + got, title, tt.want) + } + }) + } +} + // The startup notification counts the configured domains and hostnames, // although the state's hostnames also hold the apex domain's own // records. Nothing is looked up: the watcher has no resolver. diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 8c5b17b..b04617c 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -327,25 +327,25 @@ func (w *Watcher) checkDomain( w.detectNSAddressChanges(ctx, domain, prevAddresses, addresses) } + // A domain that does not exist has no records of its own: none are + // asked for, and those saved by an earlier check are removed. + // Otherwise the apex domain's records are also checked and saved as + // a hostname's, so that the port and TLS checks find its addresses. + // Notifications about them name it as a domain (see nameLine). They + // are checked before the domain's new nameservers are saved: + // detectNSDisappearances reads those of the previous check. + if nxdomain { + w.state.DeleteHostnameState(domain) + } else { + w.checkHostname(ctx, domain) + } + w.state.SetDomainState(domain, &state.DomainState{ Nameservers: nameservers, NameserverAddresses: addresses, NXDomain: nxdomain, LastChecked: now, }) - - // A domain that does not exist has no records of its own: none are - // asked for, and those saved by an earlier check are removed. - if nxdomain { - w.state.DeleteHostnameState(domain) - - return - } - - // The apex domain's records are also checked and saved as a - // hostname's, so that the port and TLS checks find its addresses. - // Notifications about them name it as a domain (see nameLine). - w.checkHostname(ctx, domain) } func (w *Watcher) detectNSChanges( @@ -772,28 +772,52 @@ func (w *Watcher) detectRecordChanges( } } +// detectNSDisappearances sends one NS Change listing the nameservers of +// the previous check that are no longer those of the zone hostname is +// in. For a configured apex domain it leaves out those of the domain's +// delegation on the previous check, which detectNSChanges notifies when +// they are removed from it; checkDomain saves the new delegation only +// after this runs. func (w *Watcher) detectNSDisappearances( ctx context.Context, hostname string, prev, current *state.HostnameState, ) { - for ns, prevNS := range prev.RecordsByNameserver { - if _, ok := current.RecordsByNameserver[ns]; ok || prevNS.Status != statusOK { - continue + var delegation []string + + if w.isDomain(hostname) { + if domainState, ok := w.state.GetDomainState(hostname); ok { + delegation = domainState.Nameservers } - - msg := fmt.Sprintf( - "%s\nNameserver: %s disappeared", - w.nameLine(hostname), ns, - ) - - w.notify.SendNotification( - ctx, - "NS Failure: "+hostname, - msg, - "error", - ) } + + var removed []string + + for ns := range prev.RecordsByNameserver { + _, ok := current.RecordsByNameserver[ns] + if !ok && !slices.Contains(delegation, ns) { + removed = append(removed, ns) + } + } + + if len(removed) == 0 { + return + } + + slices.Sort(removed) + + msg := fmt.Sprintf( + "%s\nRemoved: %s", + w.nameLine(hostname), + strings.Join(removed, ", "), + ) + + w.notify.SendNotification( + ctx, + "NS Change: "+hostname, + msg, + "warning", + ) } // detectNSFailures notifies when a nameserver that answered on the diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 32517d7..5a2b744 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -558,6 +558,9 @@ func TestDomainThatDoesNotExist(t *testing.T) { // nameservers and without nxdomain, and its records, asked at the // codeberg.page servers, are saved. Those are testSmallDomain's two // nameservers; github.io, the zone of the README's example, has eight. +// The previous check saved a delegation to oldNS1, now withdrawn, and +// records asked at oldNS1: the NS Change of its delegation removes +// oldNS1, and no other NS Change lists it again. func TestDomainWithNoDelegationOfItsOwn(t *testing.T) { t.Parallel() @@ -573,6 +576,15 @@ func TestDomainWithNoDelegationOfItsOwn(t *testing.T) { w, deps = newTestWatcher(t, cfg) + deps.state.SetDomainState(domain, &state.DomainState{ + Nameservers: []string{oldNS1}, + }) + deps.state.SetHostnameState(domain, saved( + map[string]*state.NameserverRecordState{ + oldNS1: answered(map[string][]string{"A": {oldIP}}), + }, + )) + err := checkOnce(ctx, w, deps) // A domain saved as not existing has no records to wait for; @@ -593,6 +605,22 @@ func TestDomainWithNoDelegationOfItsOwn(t *testing.T) { if _, ok := deps.state.GetHostnameState(domain); !ok { t.Errorf("no records saved for %s", domain) } + + title := "NS Change: " + domain + want := "Domain: " + domain + "\nAdded: \nRemoved: " + oldNS1 + + var changes []notification + + for _, n := range deps.notifier.getNotifications() { + if n.Title == title { + changes = append(changes, n) + } + } + + if len(changes) != 1 || changes[0].Message != want { + t.Errorf("sent %v, want one %q with message:\n%s", + changes, title, want) + } } func TestNSAddressChangeDetection(t *testing.T) { @@ -1049,7 +1077,7 @@ func TestSendTestNotification_Disabled(t *testing.T) { } } -func TestNSFailureAndRecovery(t *testing.T) { +func TestNSChangeAndRecovery(t *testing.T) { t.Parallel() cfg := defaultTestConfig(t) @@ -1059,7 +1087,7 @@ func TestNSFailureAndRecovery(t *testing.T) { // The saved state has every nameserver live DNS lists as one that // did not answer, and, as answering, one that live DNS does not - // list, which then disappears. + // list, which the check then finds removed. _, deps := runChecks(t, cfg, func(deps *testDeps) { byNameserver := map[string]*state.NameserverRecordState{ oldNS1: answered(map[string][]string{"A": {oldIP}}), @@ -1071,7 +1099,7 @@ func TestNSFailureAndRecovery(t *testing.T) { deps.state.SetHostnameState(testHost, saved(byNameserver)) }) - assertNotified(t, deps, "NS Failure: "+testHost, "error") + assertNotified(t, deps, "NS Change: "+testHost, "warning") assertNotified(t, deps, "NS Recovery: "+testHost, "success") // A nameserver that did not answer has no records to compare, so