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

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)

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
 	}
 }

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

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)
 

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()

internal/watcher/inconsistency_test.go

@@ -183,3 +183,52 @@ 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 := []string{
+		cname, cname, cname, cname, cname, cname, cname, cname,
+	}
+
+	saved := newTestDeps(t, cfg).state
+	saved.SetHostnameState(host, hostnameState(map[string]map[string][]string{
+		nsA: {"CNAME": repeated}, nsB: {"CNAME": 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: {"CNAME": {cname}}, nsB: {"CNAME": {cname}},
+	})
+
+	// 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)
+	}
+}