amadan

carlos / platform Public

steering: fix review findings — shrink guard and plain-record conflicts

FINDING 1: the empty-desired-set refusal guarded the wrong threshold. It
only catches a read that returned NOTHING; a SHORT read — three of five
edges' files listed, a routes query missing a steered host — has a non-empty
want and sailed straight past it, deleting every record for the members that
went missing. For a live steered host that is NXDOMAIN until the next tick.

Adds a proportional shrink guard: a pass may delete at most half the records
it currently manages, or shrinkFloor (2), whichever is larger. Over that, it
logs every intended deletion by (name, setid, value), logs why it refused,
and makes no call at all — the guard runs before the first health check is
created, so a refused pass is zero-WRITE, not merely zero-delete. A genuine
shrink that large is an operator action, exactly like the empty case (of
which this is the degenerate form). The durable half — fail-closed reads, so
a partial listing errors instead of returning a smaller slice — is Task 6's.

FINDING 2: the static-records test enshrined a zone state Route53 forbids.
A name+type cannot hold both a plain record and a routing-policy record set;
the real service rejects the batch, and batches are ATOMIC — so one operator
A record at a steered name would wedge EVERY steering record in the zone, on
every tick, forever.

plainConflicts now finds wanted names the zone already holds a plain A for
and skips those names entirely: no upsert, no delete, one loud log naming
the conflicting record and its value. Never the alternative of deleting the
plain record to make room — no SetIdentifier means operator territory, the
same rule isManaged enforces. The fake learned the real refusal (projected
state validated before commit, so the batch is atomic), and the test now
asserts the skip plus that the OTHER name's record still lands.

Sabotage proof of the wedge: with the skip removed, the flipped test fails
with "change batch 0-2 of 2 ... InvalidChangeBatch", taking the unrelated
edge.example.com. record down with it — exactly the failure described.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev adc5ae33e0a62de2e2bce1ef5668f5875592f6d2 parent 7ca22b1
2 files changed, +381 −45
  • internal/steering/route53.go +184 −34
  • internal/steering/route53_test.go +197 −11
diff --git a/internal/steering/route53.go b/internal/steering/route53.go
index 00adefda6..ba22f9604 100644
--- a/internal/steering/route53.go
+++ b/internal/steering/route53.go
@@ -56,6 +56,11 @@ const (
// own 1000-change cap. Smaller batches mean a failure retries less work
// and names a smaller blast radius in the log.
changeBatchLimit = 100
+
+ // shrinkFloor is the number of managed records a pass may always remove,
+ // however small the zone. Without it, the proportional guard below would
+ // make a 2-record zone unable to lose a single record. See shrinkLimit.
+ shrinkFloor = 2
)
// steeringZonePattern is the region-host tofu module's own zone-id shape
@@ -129,9 +134,12 @@ type ownedCheck struct {
// before anything is written, and an empty result means the managed
// record set is empty, which means NOTHING is deletable;
// 2. read the zone;
-// 3. CREATE any missing health check — capacity before records need it;
-// 4. UPSERT wanted records, DELETE managed-but-unwanted ones;
-// 5. GC health checks nothing references any more — after the records that
+// 3. drop names Route53 will not let us write (plainConflicts) and decide
+// what the pass would delete — BOTH REFUSALS RUN BEFORE THE FIRST WRITE,
+// so a refused pass is genuinely zero-write, not merely zero-delete;
+// 4. CREATE any missing health check — capacity before records need it;
+// 5. UPSERT wanted records, DELETE managed-but-unwanted ones;
+// 6. GC health checks nothing references any more — after the records that
// referenced them are gone, never before.
//
// A failure at any step returns loudly and leaves the rest of the pass
@@ -153,6 +161,10 @@ func Converge(ctx context.Context, client route53API, zoneID string, want []desi
// set on that signal would turn a transient read failure into a global
// outage. So: refuse, loudly, and let the next tick decide again.
// A genuine full teardown is an operator action.
+ //
+ // This is the degenerate case of the proportional guard below; both
+ // exist because "delete everything" and "delete most things" are the
+ // same mistake at different sizes.
if len(want) == 0 {
log.Printf("steering: empty desired set for zone %s — refusing to remove any record (a wholesale teardown is an operator action, not a converge outcome)", zoneID)
return nil
@@ -179,7 +191,28 @@ func Converge(ctx context.Context, client route53API, zoneID string, want []desi
return err
}
- // 3. health checks for every wanted EIP.
+ // 3a. names Route53 forbids us from writing at all.
+ conflicted := plainConflicts(want, existing)
+ if len(conflicted) > 0 {
+ want = dropNames(want, conflicted)
+ if len(want) == 0 {
+ log.Printf("steering: every steered name in zone %s collides with a plain record — refusing to change anything", zoneID)
+ return nil
+ }
+ }
+
+ // 3b. the proportional shrink guard.
+ managed, unwanted := partitionManaged(want, existing, ownedIDs, conflicted)
+ if limit := shrinkLimit(managed); len(unwanted) > limit {
+ for _, rr := range unwanted {
+ log.Printf("steering: would delete %s (set %s, value %s)", aws.ToString(rr.Name), aws.ToString(rr.SetIdentifier), firstValue(rr))
+ }
+ log.Printf("steering: refusing to converge zone %s: the pass would delete %d of %d managed records (limit %d) — a shrink that large is far likelier a short read of edges or routes than an intent to withdraw those names, and a genuine teardown that size is an operator action, exactly like the empty-desired-set case; the intended deletions are listed above",
+ zoneID, len(unwanted), managed, limit)
+ return nil
+ }
+
+ // 4. health checks for every wanted EIP.
sni := steeringHostname(want)
for _, eip := range distinctEIPs(want) {
if _, ok := checkByEIP[eip]; ok {
@@ -198,8 +231,8 @@ func Converge(ctx context.Context, client route53API, zoneID string, want []desi
owned = append(owned, ownedCheck{id: id, eip: eip})
}
- // 4. records.
- changes, err := recordChanges(want, existing, checkByEIP, ownedIDs)
+ // 5. records.
+ changes, err := recordChanges(want, existing, checkByEIP, ownedIDs, unwanted)
if err != nil {
return err
}
@@ -207,10 +240,38 @@ func Converge(ctx context.Context, client route53API, zoneID string, want []desi
return err
}
- // 5. GC. Runs only once the record batches have landed.
+ // 6. GC. Runs only once the record batches have landed.
return gcHealthChecks(ctx, client, owned, want, existing, changes)
}
+// shrinkLimit is how many managed records one pass may delete, given how many
+// it currently manages: half of them, but never fewer than shrinkFloor.
+//
+// FINDING 1 (review, 2026-08-22). The empty-desired-set refusal above only
+// catches a read that returned NOTHING. A SHORT read — three of five edges'
+// files listed, or a routes query that came back missing a steered host — has
+// a non-empty want and sails straight past it, and the converger would
+// dutifully delete every record belonging to the members that went missing.
+// For a live steered host that is NXDOMAIN until the next tick: not a slow
+// failover, a hard outage, caused by a transient read rather than by anything
+// an operator did.
+//
+// The durable fix is fail-closed reads upstream (Task 6's half: a partial
+// listing must produce an ERROR, not a smaller slice). This is the half that
+// belongs here — a blast-radius cap that holds even if a caller one day hands
+// us a quietly-truncated set. A pass that would remove most of what it manages
+// stops and describes itself instead.
+//
+// The floor keeps small zones workable: a 2-record zone losing one record is
+// ordinary, and a purely proportional rule would refuse it forever. The floor
+// is small enough that the cap still bites at any size worth protecting.
+func shrinkLimit(managed int) int {
+ if half := managed / 2; half > shrinkFloor {
+ return half
+ }
+ return shrinkFloor
+}
+
// ---------------------------------------------------------------------------
// ownership
// ---------------------------------------------------------------------------
@@ -559,7 +620,111 @@ func sameRRSet(have, want r53types.ResourceRecordSet) bool {
return true
}
-// recordChanges diffs want against the zone.
+// plainConflicts returns the wanted names the zone already holds a PLAIN A
+// record for — one with no SetIdentifier — lowercased with the trailing dot.
+//
+// FINDING 2 (review, 2026-08-22). Route53 will not let a name+type hold BOTH
+// a simple record and a routing-policy record set: submitting a latency A for
+// a name that already has a plain A is rejected with InvalidChangeBatch. And
+// change batches are ATOMIC — the rejection takes the whole batch with it. So
+// a single operator-made A record at a name somebody later steers would wedge
+// every steering record in the zone, on every tick, forever: the pass would
+// fail, retry identically, and fail again, while unrelated names never got
+// their records.
+//
+// The resolution is to skip the conflicting NAME entirely — no upsert, no
+// delete — and say so loudly. Never the alternative of deleting the plain
+// record to make room: it is an operator's record (no SetIdentifier means
+// static/operator territory, the same rule isManaged enforces), and taking a
+// name away from its owner because a member typed it into a steering field
+// is not a call this code gets to make.
+func plainConflicts(want []desiredRecord, existing []r53types.ResourceRecordSet) map[string]bool {
+ wantNames := make(map[string]bool, len(want))
+ for _, rec := range want {
+ wantNames[strings.ToLower(rec.Name)] = true
+ }
+ conflicted := map[string]bool{}
+ for _, rr := range existing {
+ if rr.Type != r53types.RRTypeA || aws.ToString(rr.SetIdentifier) != "" {
+ continue
+ }
+ name := strings.ToLower(aws.ToString(rr.Name))
+ if !wantNames[name] || conflicted[name] {
+ continue
+ }
+ conflicted[name] = true
+ log.Printf("steering: SKIPPING %s entirely: the zone already holds a plain A record there (value %s, TTL %d) and Route53 forbids a name from holding both a plain record and a latency record set — steering this name needs that record removed by its owner first",
+ aws.ToString(rr.Name), firstValue(rr), aws.ToInt64(rr.TTL))
+ }
+ return conflicted
+}
+
+// dropNames removes every desired record whose Name is in the set.
+func dropNames(want []desiredRecord, names map[string]bool) []desiredRecord {
+ out := make([]desiredRecord, 0, len(want))
+ for _, rec := range want {
+ if names[strings.ToLower(rec.Name)] {
+ continue
+ }
+ out = append(out, rec)
+ }
+ return out
+}
+
+// partitionManaged counts the zone's carlos-managed records and collects the
+// ones no desired key wants — the pass's would-be deletions. It is separated
+// from recordChanges so Converge can weigh the deletions BEFORE it creates a
+// single health check, which is what makes the shrink guard's refusal
+// genuinely zero-write.
+//
+// Records at a conflicted name are excluded from both halves: the pass has
+// already promised not to touch that name. That exclusion is a STRUCTURAL
+// BACKSTOP and is deliberately untested — a conflicted name is one holding a
+// plain record, and Route53 forbids a plain record and a latency record set
+// from sharing a name, so a MANAGED record at a conflicted name is a state
+// the service cannot produce. It is kept, in the same spirit as
+// dedupeRecords in steering.go, so that a misread listing fails by leaving
+// records alone rather than by deleting our own.
+func partitionManaged(want []desiredRecord, existing []r53types.ResourceRecordSet, ownedIDs, conflicted map[string]bool) (managed int, unwanted []r53types.ResourceRecordSet) {
+ wanted := make(map[string]bool, len(want))
+ for _, rec := range want {
+ wanted[wantKey(rec)] = true
+ }
+ for _, rr := range existing {
+ if conflicted[strings.ToLower(aws.ToString(rr.Name))] {
+ continue
+ }
+ if !isManaged(rr, ownedIDs) {
+ // Say so when it LOOKS like ours but fails the ownership gate —
+ // a latency A record with a SetIdentifier whose health check we
+ // do not own is either an operator's or an orphan of ours whose
+ // check went missing. Either way we do not touch it, and either
+ // way somebody should know it is there.
+ if rr.Type == r53types.RRTypeA && aws.ToString(rr.SetIdentifier) != "" && rr.Region != "" && rr.AliasTarget == nil {
+ log.Printf("steering: leaving latency record %s (set %s) alone: health check %q is not one this deployment owns",
+ aws.ToString(rr.Name), aws.ToString(rr.SetIdentifier), aws.ToString(rr.HealthCheckId))
+ }
+ continue
+ }
+ managed++
+ if !wanted[recordKey(rr)] {
+ unwanted = append(unwanted, rr)
+ }
+ }
+ return managed, unwanted
+}
+
+// firstValue renders a record set's first value for a log line.
+func firstValue(rr r53types.ResourceRecordSet) string {
+ if len(rr.ResourceRecords) == 0 {
+ return "?"
+ }
+ return aws.ToString(rr.ResourceRecords[0].Value)
+}
+
+// recordChanges diffs want against the zone. The deletions were decided
+// already, by partitionManaged, and are passed in — this function only turns
+// them into changes.
//
// UPSERTs come first in the returned slice and DELETEs last, so that when the
// changes are chunked, capacity is added before any is taken away.
@@ -568,14 +733,17 @@ func sameRRSet(have, want r53types.ResourceRecordSet) bool {
// latency record at exactly the (name, SetIdentifier) we would create) is
// ADOPTED by the UPSERT — CREATE would collide with it and there is no third
// option. It is logged, because taking over a record is the one write here
-// that changes something we did not make.
-func recordChanges(want []desiredRecord, existing []r53types.ResourceRecordSet, checkByEIP map[string]string, ownedIDs map[string]bool) ([]r53types.Change, error) {
+// that changes something we did not make. Note the difference from the
+// plain-record conflict above: adoption replaces a record of the SAME routing
+// policy at the SAME key, which Route53 permits and which is the record we
+// would otherwise have created; the plain-record case is a different policy
+// at the same name, which Route53 refuses outright.
+func recordChanges(want []desiredRecord, existing []r53types.ResourceRecordSet, checkByEIP map[string]string, ownedIDs map[string]bool, unwanted []r53types.ResourceRecordSet) ([]r53types.Change, error) {
byKey := make(map[string]r53types.ResourceRecordSet, len(existing))
for _, rr := range existing {
byKey[recordKey(rr)] = rr
}
- wanted := make(map[string]bool, len(want))
var upserts []r53types.Change
for _, rec := range want {
checkID, ok := checkByEIP[rec.EIP]
@@ -585,10 +753,8 @@ func recordChanges(want []desiredRecord, existing []r53types.ResourceRecordSet,
// record with no health check, which would never withdraw.
return nil, fmt.Errorf("steering: no health check for %s (record %s / %s)", rec.EIP, rec.Name, rec.Box)
}
- key := wantKey(rec)
- wanted[key] = true
desired := desiredRRSet(rec, checkID)
- have, present := byKey[key]
+ have, present := byKey[wantKey(rec)]
if present && sameRRSet(have, desired) {
continue
}
@@ -598,26 +764,10 @@ func recordChanges(want []desiredRecord, existing []r53types.ResourceRecordSet,
upserts = append(upserts, r53types.Change{Action: r53types.ChangeActionUpsert, ResourceRecordSet: &desired})
}
- var deletes []r53types.Change
- for _, rr := range existing {
- key := recordKey(rr)
- if wanted[key] {
- continue
- }
- if isManaged(rr, ownedIDs) {
- set := rr // DELETE must resubmit the record set verbatim
- deletes = append(deletes, r53types.Change{Action: r53types.ChangeActionDelete, ResourceRecordSet: &set})
- continue
- }
- // Not managed. Say so when it LOOKS like ours but fails the
- // ownership gate — a latency A record with a SetIdentifier whose
- // health check we do not own is either an operator's or an orphan
- // of ours whose check went missing. Either way we do not touch it,
- // and either way somebody should know it is there.
- if rr.Type == r53types.RRTypeA && aws.ToString(rr.SetIdentifier) != "" && rr.Region != "" && rr.AliasTarget == nil {
- log.Printf("steering: leaving latency record %s (set %s) alone: health check %q is not one this deployment owns",
- aws.ToString(rr.Name), aws.ToString(rr.SetIdentifier), aws.ToString(rr.HealthCheckId))
- }
+ deletes := make([]r53types.Change, 0, len(unwanted))
+ for _, rr := range unwanted {
+ set := rr // DELETE must resubmit the record set verbatim
+ deletes = append(deletes, r53types.Change{Action: r53types.ChangeActionDelete, ResourceRecordSet: &set})
}
return append(upserts, deletes...), nil
diff --git a/internal/steering/route53_test.go b/internal/steering/route53_test.go
index cb82834a3..7369652d9 100644
--- a/internal/steering/route53_test.go
+++ b/internal/steering/route53_test.go
@@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
+ "reflect"
"sort"
"strings"
"testing"
@@ -24,10 +25,14 @@ import (
//
// It deliberately reproduces the real API's refusals that matter to this
// converger: DELETE of a record that is not present errors (Route53's
-// InvalidChangeBatch), ListTagsForResources refuses more than 10 ids, and
+// InvalidChangeBatch), a name+type may not hold BOTH a plain record and a
+// routing-policy record set (also InvalidChangeBatch, and it takes the whole
+// atomic batch with it), ListTagsForResources refuses more than 10 ids, and
// DeleteHealthCheck of a check still referenced by a record errors
// (HealthCheckInUse). A fake that is more permissive than the service would
-// let a bug through.
+// let a bug through — the plain-vs-policy rule is in here precisely because
+// an earlier version of this file was more permissive and enshrined a zone
+// state Route53 forbids.
type fakeR53 struct {
records []r53types.ResourceRecordSet
checks []r53types.HealthCheck
@@ -107,12 +112,15 @@ func (f *fakeR53) ChangeResourceRecordSets(ctx context.Context, in *route53.Chan
if in.ChangeBatch == nil || len(in.ChangeBatch.Changes) == 0 {
return nil, errors.New("fake: empty change batch")
}
+
+ // A batch is ATOMIC: project it onto a copy, validate the result, and
+ // commit only if the whole thing is legal.
+ projected := append([]r53types.ResourceRecordSet(nil), f.records...)
for _, c := range in.ChangeBatch.Changes {
- f.submitted = append(f.submitted, c)
rr := *c.ResourceRecordSet
key := rrKey(rr)
idx := -1
- for i, have := range f.records {
+ for i, have := range projected {
if rrKey(have) == key {
idx = i
break
@@ -121,19 +129,40 @@ func (f *fakeR53) ChangeResourceRecordSets(ctx context.Context, in *route53.Chan
switch c.Action {
case r53types.ChangeActionUpsert:
if idx >= 0 {
- f.records[idx] = rr
+ projected[idx] = rr
} else {
- f.records = append(f.records, rr)
+ projected = append(projected, rr)
}
case r53types.ChangeActionDelete:
if idx < 0 {
return nil, fmt.Errorf("fake: InvalidChangeBatch: no such record %s", key)
}
- f.records = append(f.records[:idx], f.records[idx+1:]...)
+ projected = append(projected[:idx], projected[idx+1:]...)
default:
return nil, fmt.Errorf("fake: unsupported action %q", c.Action)
}
}
+
+ // Route53's rule: for one (name, type), either a single simple record OR
+ // a set of routing-policy records — never both.
+ type nt struct{ name, typ string }
+ plain, policy := map[nt]bool{}, map[nt]bool{}
+ for _, rr := range projected {
+ k := nt{strings.ToLower(aws.ToString(rr.Name)), string(rr.Type)}
+ if aws.ToString(rr.SetIdentifier) == "" {
+ plain[k] = true
+ } else {
+ policy[k] = true
+ }
+ }
+ for k := range plain {
+ if policy[k] {
+ return nil, fmt.Errorf("fake: InvalidChangeBatch: RRSet with DNS name %s and type %s cannot be both a simple record and a routing-policy record set", k.name, k.typ)
+ }
+ }
+
+ f.submitted = append(f.submitted, in.ChangeBatch.Changes...)
+ f.records = projected
sort.SliceStable(f.records, func(i, j int) bool { return rrKey(f.records[i]) < rrKey(f.records[j]) })
return &route53.ChangeResourceRecordSetsOutput{}, nil
}
@@ -533,6 +562,7 @@ func TestConvergeStaticRecordsUntouched(t *testing.T) {
}
before := recordNames(f.records)
+ buf := captureLog(t)
want := append([]desiredRecord{dublinRec}, hostRecs("app.example.com.", dublinRec)...)
mustConverge(t, f, want)
@@ -555,11 +585,167 @@ func TestConvergeStaticRecordsUntouched(t *testing.T) {
t.Errorf("record %s disappeared", name)
}
}
- // the plain A for the steered name still stands beside the latency ones
- // (different (name,type,SetIdentifier) keys — no collision).
+
+ // app.example.com. already holds a PLAIN A record. Route53 forbids a
+ // name from holding both a simple record and a latency record set, and
+ // change batches are atomic — so writing a latency record there would
+ // not merely fail for that name, it would take every other steering
+ // record in the zone down with it, on every tick, forever. The name is
+ // skipped entirely: no upsert, no delete, and loudly.
+ for _, c := range f.submitted {
+ if strings.EqualFold(aws.ToString(c.ResourceRecordSet.Name), "app.example.com.") {
+ t.Errorf("submitted a change for the plain-record-conflicted name: %s %s", c.Action, rrKey(*c.ResourceRecordSet))
+ }
+ }
+ if !strings.Contains(buf.String(), "SKIPPING app.example.com.") {
+ t.Errorf("the conflict was not named in the log; log = %q", buf.String())
+ }
+ if !strings.Contains(buf.String(), "9.9.9.9") {
+ t.Errorf("the log did not name the conflicting record's value; log = %q", buf.String())
+ }
plain := find(t, f, "app.example.com.", "")
- if aws.ToInt64(plain.TTL) != 300 {
- t.Errorf("the static app.example.com. A record was rewritten: TTL %d", aws.ToInt64(plain.TTL))
+ if aws.ToInt64(plain.TTL) != 300 || aws.ToString(plain.ResourceRecords[0].Value) != "9.9.9.9" {
+ t.Errorf("the operator's app.example.com. A record was changed: TTL %d value %v", aws.ToInt64(plain.TTL), plain.ResourceRecords)
+ }
+
+ // ...and the OTHER name's record still lands. One conflicting name must
+ // not wedge the whole zone.
+ edgeRR := find(t, f, "edge.example.com.", "dublin")
+ if aws.ToString(edgeRR.ResourceRecords[0].Value) != "1.2.3.4" {
+ t.Errorf("the unconflicted name did not converge: %v", edgeRR.ResourceRecords)
+ }
+ if f.nChangeRecords != 1 {
+ t.Errorf("want 1 change batch (the unconflicted name), got %d", f.nChangeRecords)
+ }
+}
+
+// TestFakeRejectsPlainVsPolicyConflict proves the fake's new refusal is real,
+// so the skip above is a fix and not a tautology: submitting the batch the
+// converger used to submit does fail, atomically, taking an unrelated and
+// perfectly legal change in the same batch down with it.
+func TestFakeRejectsPlainVsPolicyConflict(t *testing.T) {
+ f := newFake()
+ f.records = []r53types.ResourceRecordSet{staticA("app.example.com.", "9.9.9.9")}
+
+ latency := r53types.ResourceRecordSet{
+ Name: aws.String("app.example.com."), Type: r53types.RRTypeA,
+ SetIdentifier: aws.String("dublin"), Region: "eu-west-1", TTL: aws.Int64(60),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("1.2.3.4")}},
+ }
+ innocent := r53types.ResourceRecordSet{
+ Name: aws.String("edge.example.com."), Type: r53types.RRTypeA,
+ SetIdentifier: aws.String("dublin"), Region: "eu-west-1", TTL: aws.Int64(60),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("1.2.3.4")}},
+ }
+ _, err := f.ChangeResourceRecordSets(context.Background(), &route53.ChangeResourceRecordSetsInput{
+ HostedZoneId: aws.String(testZone),
+ ChangeBatch: &r53types.ChangeBatch{Changes: []r53types.Change{
+ {Action: r53types.ChangeActionUpsert, ResourceRecordSet: &innocent},
+ {Action: r53types.ChangeActionUpsert, ResourceRecordSet: &latency},
+ }},
+ })
+ if err == nil {
+ t.Fatalf("the fake accepted a plain-plus-latency name — it is more permissive than Route53")
+ }
+ if !strings.Contains(err.Error(), "InvalidChangeBatch") {
+ t.Errorf("wrong error: %v", err)
+ }
+ if len(f.records) != 1 {
+ t.Fatalf("the batch was not atomic: %v", recordNames(f.records))
+ }
+}
+
+// TestConvergeShrinkGuard is FINDING 1: the empty-desired-set refusal only
+// catches a read that returned NOTHING. A SHORT read has a non-empty want and
+// would delete every record for the members that went missing — NXDOMAIN for
+// a live steered host. A pass that would remove most of what it manages must
+// refuse and describe itself instead.
+func TestConvergeShrinkGuard(t *testing.T) {
+ // 2 names x 2 edges = 4 managed records.
+ full := append([]desiredRecord{dublinRec, oregonRec}, hostRecs("app.example.com.", dublinRec, oregonRec)...)
+
+ t.Run("deleting 3 of 4 refuses with zero writes and names them", func(t *testing.T) {
+ f := newFake()
+ mustConverge(t, f, full)
+ before := f.writes()
+ state := recordNames(f.records)
+
+ buf := captureLog(t)
+ // A short read that saw only one edge and only the synthetic name.
+ mustConverge(t, f, []desiredRecord{dublinRec})
+
+ if got := f.writes() - before; got != 0 {
+ t.Fatalf("the refused pass made %d write calls, want 0", got)
+ }
+ if len(f.deletedChecks) != 0 {
+ t.Errorf("the refused pass GC'd checks: %v", f.deletedChecks)
+ }
+ if got := recordNames(f.records); !reflect.DeepEqual(got, state) {
+ t.Fatalf("the zone changed: %v -> %v", state, got)
+ }
+ logged := buf.String()
+ if !strings.Contains(logged, "refusing to converge") {
+ t.Errorf("the refusal was not logged; log = %q", logged)
+ }
+ // all three intended deletions named by (name, setid)
+ for _, want := range []string{
+ "would delete edge.example.com. (set oregon",
+ "would delete app.example.com. (set dublin",
+ "would delete app.example.com. (set oregon",
+ } {
+ if !strings.Contains(logged, want) {
+ t.Errorf("log does not name %q; log = %q", want, logged)
+ }
+ }
+ })
+
+ t.Run("deleting 1 of 4 proceeds", func(t *testing.T) {
+ f := newFake()
+ mustConverge(t, f, full)
+
+ // drop exactly one record: app.example.com. on oregon
+ var want []desiredRecord
+ for _, rec := range full {
+ if rec.Name == "app.example.com." && rec.Box == "oregon" {
+ continue
+ }
+ want = append(want, rec)
+ }
+ mustConverge(t, f, want)
+
+ if len(f.records) != 3 {
+ t.Fatalf("want 3 records left, got %v", recordNames(f.records))
+ }
+ for _, rr := range f.records {
+ if strings.EqualFold(aws.ToString(rr.Name), "app.example.com.") && aws.ToString(rr.SetIdentifier) == "oregon" {
+ t.Errorf("the ordinary deletion did not happen")
+ }
+ }
+ })
+
+ t.Run("the floor lets a small zone shrink", func(t *testing.T) {
+ // 2 managed records; half of 2 is 1, but the floor allows 2.
+ f := newFake()
+ two := []desiredRecord{dublinRec, oregonRec}
+ mustConverge(t, f, two)
+ mustConverge(t, f, append(hostRecs("app.example.com.", dublinRec), dublinRec))
+ if len(f.records) != 2 {
+ t.Fatalf("want 2 records, got %v", recordNames(f.records))
+ }
+ })
+}
+
+// TestShrinkLimit pins the threshold arithmetic on its own — the zone-level
+// test above can only exercise the sizes it happens to build.
+func TestShrinkLimit(t *testing.T) {
+ tests := []struct{ managed, want int }{
+ {0, shrinkFloor}, {1, shrinkFloor}, {2, shrinkFloor}, {4, shrinkFloor},
+ {6, 3}, {10, 5}, {11, 5}, {100, 50},
+ }
+ for _, tt := range tests {
+ if got := shrinkLimit(tt.managed); got != tt.want {
+ t.Errorf("shrinkLimit(%d) = %d, want %d", tt.managed, got, tt.want)
+ }
}
}