From c16b3f300b844696840cd18ad454cb3fc86d1ca9 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 2 Oct 2026 06:01:25 +0000 Subject: [PATCH] resolver: store a name's CNAME once per nameserver (closes #220) For a name with a CNAME, a nameserver answers a query of any type with that CNAME, and the records of every answer were added, so the CNAME was stored once for each of the eight record types asked for. collectAnswerRecords now adds each value once per record type. A state file saved before this holds the repeated values. Load keeps each record value once, so the first check after upgrading sees no record change and notifies nothing for them. Model: opus-5-5 --- TODO.md | 2 ++ internal/resolver/export_test.go | 7 ++++ internal/resolver/iterative.go | 13 +++++-- internal/resolver/iterative_test.go | 32 +++++++++++++++++ internal/state/state.go | 14 ++++++++ internal/state/state_test.go | 47 ++++++++++++++++++++++++ internal/watcher/inconsistency_test.go | 50 ++++++++++++++++++++++++++ 7 files changed, 162 insertions(+), 3 deletions(-) diff --git a/TODO.md b/TODO.md index 927a598..f096568 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 name's CNAME is stored once per nameserver, not once per record + type asked for; a state file with repeats loads each value once (closes #220). - 2026-10-02: a resolver test that reads one record type from a nameserver's answer asks again when that type is missing from it (closes #218). - 2026-10-02: a plain `docker build .` of a clone stamps its tag or short diff --git a/internal/resolver/export_test.go b/internal/resolver/export_test.go index 17c57d7..58b88b1 100644 --- a/internal/resolver/export_test.go +++ b/internal/resolver/export_test.go @@ -11,6 +11,13 @@ func ExtractRecordValue(rr dns.RR) string { return extractRecordValue(rr) } +// CollectAnswerRecords exports collectAnswerRecords for testing. +func CollectAnswerRecords(msg *dns.Msg, resp *NameserverResponse) { + var state queryState + + collectAnswerRecords(msg, resp, &state) +} + // UsableReply exports usableReply for testing. func UsableReply(resp *dns.Msg, zone string, name string) bool { return usableReply(resp, zone, name) diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index 1f06a24..6ffedc5 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -714,6 +714,10 @@ func (r *Resolver) querySingleType( collectAnswerRecords(msg, resp, state) } +// collectAnswerRecords adds the records in msg's answer to resp, each +// value once per record type. For a name with a CNAME, a nameserver +// answers a query of any type with that CNAME, so the same value comes +// in the answer to every type asked for. func collectAnswerRecords( msg *dns.Msg, resp *NameserverResponse, @@ -726,9 +730,12 @@ func collectAnswerRecords( } typeName := dns.TypeToString[rr.Header().Rrtype] - resp.Records[typeName] = append( - resp.Records[typeName], val, - ) + if !slices.Contains(resp.Records[typeName], val) { + resp.Records[typeName] = append( + resp.Records[typeName], val, + ) + } + state.hasRecords = true } } diff --git a/internal/resolver/iterative_test.go b/internal/resolver/iterative_test.go index 4f539ae..ca422cd 100644 --- a/internal/resolver/iterative_test.go +++ b/internal/resolver/iterative_test.go @@ -238,6 +238,38 @@ func TestExtractRecordValue_LetterCase(t *testing.T) { } } +// TestCollectAnswerRecords_CNAMEOnce collects the answers a nameserver +// gives for a name with a CNAME, one for each record type a check asks +// for. Each answer holds the CNAME, which must be stored once. +func TestCollectAnswerRecords_CNAMEOnce(t *testing.T) { + t.Parallel() + + cname := &dns.CNAME{ + Hdr: dns.RR_Header{ + Name: "git.eeqj.de.", Rrtype: dns.TypeCNAME, Class: dns.ClassINET, + }, + Target: "fsn1app1.datavi.be.", + } + + resp := &resolver.NameserverResponse{Records: map[string][]string{}} + + for _, qtype := range []uint16{ + dns.TypeA, dns.TypeAAAA, dns.TypeCNAME, dns.TypeMX, + dns.TypeTXT, dns.TypeSRV, dns.TypeCAA, dns.TypeNS, + } { + msg := new(dns.Msg) + msg.SetQuestion("git.eeqj.de.", qtype) + msg.Answer = []dns.RR{cname} + + resolver.CollectAnswerRecords(msg, resp) + } + + assert.Equal(t, + map[string][]string{"CNAME": {"fsn1app1.datavi.be."}}, + resp.Records, + ) +} + // TestShuffled shuffles the root servers with many seeds. Every order // must hold each root server once, so each is tried before a // resolution fails; each root server must come first for some seed, so diff --git a/internal/state/state.go b/internal/state/state.go index a8af3f9..3f5bb7c 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -8,6 +8,7 @@ import ( "log/slog" "os" "path/filepath" + "slices" "sync" "time" @@ -196,6 +197,19 @@ func (s *State) Load() error { return fmt.Errorf("parsing state file: %w", err) } + // A state file saved before each record value was stored once can + // hold a hostname's CNAME once for every record type asked for. + // Each value is kept once, so the first check does not see a + // record change. + for _, hs := range snapshot.Hostnames { + for _, ns := range hs.RecordsByNameserver { + for recordType, values := range ns.Records { + slices.Sort(values) + ns.Records[recordType] = slices.Compact(values) + } + } + } + s.snapshot = &snapshot s.log.Info("loaded state from disk", "path", path) diff --git a/internal/state/state_test.go b/internal/state/state_test.go index 16f709d..ea8f180 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -188,6 +188,53 @@ func TestLoadStateFromBeforeNameserverAddresses(t *testing.T) { } } +// TestLoadStateWithRepeatedValues loads a state file saved when a +// hostname's CNAME was stored once for every record type asked for. +// Each value must load once. +func TestLoadStateWithRepeatedValues(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + data := []byte(`{ + "version": 1, + "hostnames": { + "www.example.com": { + "recordsByNameserver": { + "ns1.example.com.": { + "records": { + "CNAME": ["a.example.net.", "a.example.net.", "a.example.net."] + }, + "status": "ok" + } + } + } + } + }`) + + err := os.WriteFile(filepath.Join(dir, "state.json"), data, 0o600) + if err != nil { + t.Fatalf("writing state file: %v", err) + } + + s := state.NewForTestWithDataDir(dir) + + err = s.Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + + hs, ok := s.GetHostnameState(testHostname) + if !ok { + t.Fatal("missing hostname " + testHostname) + } + + got := hs.RecordsByNameserver[testNS1].Records["CNAME"] + if !reflect.DeepEqual(got, []string{"a.example.net."}) { + t.Errorf("CNAME: got %v, want it once", got) + } +} + // TestSaveLoadRoundTrip_Hostnames verifies hostname data survives a save/load cycle. func TestSaveLoadRoundTrip_Hostnames(t *testing.T) { t.Parallel() diff --git a/internal/watcher/inconsistency_test.go b/internal/watcher/inconsistency_test.go index 494ec2b..a2f34db 100644 --- a/internal/watcher/inconsistency_test.go +++ b/internal/watcher/inconsistency_test.go @@ -183,3 +183,53 @@ func TestInconsistencyAlert(t *testing.T) { }) } } + +// TestFirstCheckAfterRepeatedValuesLoaded saves a state file holding a +// hostname's CNAME once for every record type asked for, as checks did +// before each value was stored once, and loads it. A check that then +// finds the CNAME once at each nameserver must notify nothing. +func TestFirstCheckAfterRepeatedValuesLoaded(t *testing.T) { + t.Parallel() + + const cname = "c.example.net." + + cfg := defaultTestConfig(t) + repeated := map[string][]string{ + "CNAME": {cname, cname, cname, cname, cname, cname, cname, cname}, + } + once := map[string][]string{"CNAME": {cname}} + + saved := newTestDeps(t, cfg).state + saved.SetHostnameState(host, hostnameState(map[string]map[string][]string{ + nsA: repeated, nsB: repeated, + })) + + err := saved.Save() + if err != nil { + t.Fatalf("saving the state file: %v", err) + } + + deps := newTestDeps(t, cfg) + + err = deps.state.Load() + if err != nil { + t.Fatalf("loading the state file: %v", err) + } + + prev, ok := deps.state.GetHostnameState(host) + if !ok { + t.Fatal("the state file has no state for " + host) + } + + current := hostnameState(map[string]map[string][]string{ + nsA: once, nsB: once, + }) + + // The hostname change detection uses only the notifier. + w := watcher.NewForTest(nil, nil, nil, nil, nil, deps.notifier) + w.DetectHostnameChanges(t.Context(), host, prev, current) + + if got := deps.notifier.getNotifications(); len(got) != 0 { + t.Errorf("sent %v, want no notification", got) + } +}