From c42d242a3ba6411c529a4a743f7c22974c18c362 Mon Sep 17 00:00:00 2001 From: Vassiliy Yegorov Date: Fri, 3 Jul 2026 13:12:10 +0700 Subject: [PATCH] =?UTF-8?q?feat(diff):=20prune-guard=20Updates()/Prunes()?= =?UTF-8?q?=20+=20=D1=84=D0=B8=D0=BA=D1=81=D0=B0=D1=86=D0=B8=D1=8F=20?= =?UTF-8?q?=D1=81=D0=B5=D0=BC=D0=B0=D0=BD=D1=82=D0=B8=D0=BA=D0=B8=20dedup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/diff/diff.go | 46 ++++++++++++++ internal/diff/diff_test.go | 119 +++++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 2d1edef..78df864 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -37,6 +37,44 @@ func (c Changeset) Actionable() []RecordDiff { return out } +// Updates returns managed (non-ReadOnly) diffs that add or bring a record in +// line with the template (Kind == Add or Kind == Update). It never contains +// Delete diffs, so it is safe for callers to apply automatically without +// risking data loss from an incomplete or empty template. +func (c Changeset) Updates() []RecordDiff { + var out []RecordDiff + for _, d := range c.Diffs { + if d.ReadOnly { + continue + } + if d.Kind == Add || d.Kind == Update { + out = append(out, d) + } + } + return out +} + +// Prunes returns managed (non-ReadOnly) diffs where a record exists in the +// zone but is absent from the template (Kind == Delete). These are +// potentially destructive: an incomplete or empty template would surface +// every managed zone record here. Callers should require explicit +// confirmation before applying Prunes(), unlike Updates(). +// +// Updates() and Prunes() partition Actionable(): every diff in Actionable() +// appears in exactly one of the two. +func (c Changeset) Prunes() []RecordDiff { + var out []RecordDiff + for _, d := range c.Diffs { + if d.ReadOnly { + continue + } + if d.Kind == Delete { + out = append(out, d) + } + } + return out +} + // Diff compares a template against the actual zone records. // Records present in the zone but absent from the template yield Delete. func Diff(template, actual []model.Record) Changeset { @@ -70,6 +108,14 @@ func Diff(template, actual []model.Record) Changeset { return Changeset{Diffs: diffs} } +// index builds a lookup of records by Key(). If two records in recs share +// the same Key() (e.g. a provider returning a duplicate RRset entry), this +// is a deliberate, documented choice, not an oversight: the LAST record with +// that Key() wins, since later map assignments overwrite earlier ones for +// the same key during the loop below. Diff() only ever sees the winning +// (last) record for a duplicated key; earlier duplicates are silently +// collapsed. See TestIndexDedupLastWriteWins in diff_test.go, which pins +// this behavior down. func index(recs []model.Record) map[string]model.Record { m := make(map[string]model.Record, len(recs)) for _, r := range recs { diff --git a/internal/diff/diff_test.go b/internal/diff/diff_test.go index eac6b43..6a50200 100644 --- a/internal/diff/diff_test.go +++ b/internal/diff/diff_test.go @@ -113,3 +113,122 @@ func TestActionableExcludesInSyncAndReadOnly(t *testing.T) { t.Fatalf("only b.example.com. is actionable, got %+v", act) } } + +// Prune-guard: with an empty/nil template every managed zone record surfaces +// as a Delete. Prunes() must isolate exactly those (potentially destructive) +// entries, while Updates() must stay empty — there is nothing safe to apply. +func TestPrunesEmptyTemplateAllManagedNoneInUpdates(t *testing.T) { + actual := []model.Record{ + {Type: model.A, Name: "a.example.com.", TTL: 300, Values: []string{"1.1.1.1"}}, + {Type: model.A, Name: "b.example.com.", TTL: 300, Values: []string{"2.2.2.2"}}, + {Type: model.NS, Name: "example.com.", TTL: 3600, Values: []string{"ns1.example.com."}}, + } + cs := Diff(nil, actual) + + prunes := cs.Prunes() + if len(prunes) != 2 { + t.Fatalf("expected 2 managed prunes (A records only), got %d: %+v", len(prunes), prunes) + } + for _, d := range prunes { + if d.Kind != Delete { + t.Fatalf("Prunes() must only contain Delete diffs, got %+v", d) + } + if d.ReadOnly { + t.Fatalf("Prunes() must exclude ReadOnly diffs, got %+v", d) + } + if d.Type == model.NS { + t.Fatalf("NS must be excluded from Prunes(), got %+v", d) + } + } + + updates := cs.Updates() + if len(updates) != 0 { + t.Fatalf("expected 0 updates for an empty template, got %d: %+v", len(updates), updates) + } +} + +// Mixed case: Add/Update land in Updates(), Delete lands in Prunes(), and the +// ReadOnly NS diff (here an Add) is excluded from both — mirroring Actionable(). +func TestUpdatesAndPrunesMixedSeparation(t *testing.T) { + tmpl := []model.Record{ + {Type: model.A, Name: "a.example.com.", TTL: 300, Values: []string{"1.1.1.1"}}, // in sync + {Type: model.A, Name: "b.example.com.", TTL: 300, Values: []string{"9.9.9.9"}}, // update + {Type: model.A, Name: "c.example.com.", TTL: 300, Values: []string{"3.3.3.3"}}, // add + {Type: model.NS, Name: "example.com.", TTL: 3600, Values: []string{"ns1.example.com."}}, // read-only add + } + actual := []model.Record{ + {Type: model.A, Name: "a.example.com.", TTL: 300, Values: []string{"1.1.1.1"}}, + {Type: model.A, Name: "b.example.com.", TTL: 300, Values: []string{"2.2.2.2"}}, + {Type: model.A, Name: "d.example.com.", TTL: 300, Values: []string{"4.4.4.4"}}, // delete (extra) + } + cs := Diff(tmpl, actual) + + updates := cs.Updates() + if len(updates) != 2 { + t.Fatalf("expected 2 updates (b update + c add), got %d: %+v", len(updates), updates) + } + for _, d := range updates { + if d.Kind != Add && d.Kind != Update { + t.Fatalf("Updates() must only contain Add/Update diffs, got %+v", d) + } + if d.ReadOnly { + t.Fatalf("Updates() must exclude ReadOnly diffs, got %+v", d) + } + } + + prunes := cs.Prunes() + if len(prunes) != 1 || prunes[0].Name != "d.example.com." { + t.Fatalf("expected exactly 1 prune (d.example.com.), got %+v", prunes) + } + if prunes[0].ReadOnly { + t.Fatalf("prune must not be ReadOnly, got %+v", prunes[0]) + } + + // Updates ∪ Prunes must equal Actionable, with no overlap. + act := cs.Actionable() + if len(act) != len(updates)+len(prunes) { + t.Fatalf("Updates()+Prunes() must partition Actionable(): actionable=%d updates=%d prunes=%d", + len(act), len(updates), len(prunes)) + } +} + +// Dedup semantics: index() is keyed by model.Record.Key(). If the same Key() +// appears twice within a single slice (e.g. actual records fetched from a +// provider with a duplicate RRset entry), the map assignment in the loop +// means the LAST occurrence wins and earlier ones are discarded silently. +// This test fixes that behavior in place so it is a documented, intentional +// choice rather than an accidental artifact of map iteration order. +func TestIndexDedupLastWriteWins(t *testing.T) { + actual := []model.Record{ + {Type: model.A, Name: "dup.example.com.", TTL: 300, Values: []string{"1.1.1.1"}}, + {Type: model.A, Name: "dup.example.com.", TTL: 300, Values: []string{"9.9.9.9"}}, // same Key(), later wins + } + tmpl := []model.Record{ + {Type: model.A, Name: "dup.example.com.", TTL: 300, Values: []string{"9.9.9.9"}}, + } + cs := Diff(tmpl, actual) + + // Only one diff should exist for the duplicated key (dedup collapsed it), + // and it must reflect the LAST actual record ("9.9.9.9"), matching the + // template value, hence InSync rather than Update. + d := find(cs, "A dup.example.com.") + if d == nil { + t.Fatalf("expected a diff for dup.example.com.") + } + if d.Kind != InSync { + t.Fatalf("expected InSync (last actual record wins in index()), got %+v", d) + } + if d.Actual == nil || len(d.Actual.Values) != 1 || d.Actual.Values[0] != "9.9.9.9" { + t.Fatalf("expected Actual to be the last duplicate (9.9.9.9), got %+v", d.Actual) + } + + count := 0 + for _, dd := range cs.Diffs { + if dd.Type == model.A && dd.Name == "dup.example.com." { + count++ + } + } + if count != 1 { + t.Fatalf("expected exactly 1 diff for the duplicated key, got %d", count) + } +}