amadan

carlos / platform Public

steering: fix round 2 — sibling wedges, adoption log regression, SNI downgrade

FINDING A: F2's cure was incomplete. Two sibling shapes at a wanted name
have the identical failure mode — the write is rejected and takes the whole
atomic batch with it, wedging every steering record in the zone forever:

  - a CNAME (exclusive in DNS: a name holding one holds no other type). A
    member steering a hostname that already CNAMEs inside the steering zone
    is the ordinary custom-domain shape, not an exotic case.
  - a NON-LATENCY policy A set (SetIdentifier set, Region empty —
    weighted/failover/geo/geoproximity/CIDR/multivalue). One routing policy
    per name+type.

plainConflicts becomes conflictedNames, with the three shapes enumerated in
conflictReason, sharing one cure: skip the name, log which shape is in the
way and (for a policy set) WHICH policy. The fake's validator models all
three refusals on the projected post-batch state, so atomicity is modelled
rather than asserted. Left open on the report, deliberately not guessed at:
an ALIAS latency record at exactly one wanted (name, SetIdentifier), which
Route53 also refuses to convert in place — that is a KEY-level conflict, and
dropping the whole NAME for it would refuse names that are steerable.

FINDING B (Low, round-1 regression): partitionManaged lost recordChanges'
wanted[key] guard when the loop was split out, so "leaving latency record X
alone" fired for the very record the same pass ADOPTS — two log lines
claiming opposite fates for one record. Restored the guard. The adoption
test now asserts the "leaving alone" line does NOT appear, and a new test
holds the line up for a foreign record the pass genuinely does not touch, so
the fix cannot silence it everywhere.

Doc/observability: the SNI fallback site now logs its downgrade once. When
the synthetic edge.<apps-domain> name is itself skipped for a conflict,
every check created afterwards silently rides one member's name and
certificate instead of the shared steering hostname — the log names the
fallback host and points at the skip above as the cause.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 39b36ecfe6c41d0639f3ca72b97989a36fe36cbe parent adc5ae3
2 files changed, +399 −73
  • internal/steering/route53.go +118 −29
  • internal/steering/route53_test.go +281 −44
diff --git a/internal/steering/route53.go b/internal/steering/route53.go
index ba22f9604..5985d5d2c 100644
--- a/internal/steering/route53.go
+++ b/internal/steering/route53.go
@@ -192,11 +192,11 @@ func Converge(ctx context.Context, client route53API, zoneID string, want []desi
}
// 3a. names Route53 forbids us from writing at all.
- conflicted := plainConflicts(want, existing)
+ conflicted := conflictedNames(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)
+ log.Printf("steering: every steered name in zone %s collides with an existing record — refusing to change anything", zoneID)
return nil
}
}
@@ -214,6 +214,7 @@ func Converge(ctx context.Context, client route53API, zoneID string, want []desi
// 4. health checks for every wanted EIP.
sni := steeringHostname(want)
+ loggedSNIFallback := false
for _, eip := range distinctEIPs(want) {
if _, ok := checkByEIP[eip]; ok {
continue
@@ -221,6 +222,22 @@ func Converge(ctx context.Context, client route53API, zoneID string, want []desi
fqdnFor := sni
if fqdnFor == "" {
fqdnFor = fallbackHostname(want, eip)
+ // The shared steering hostname is the RIGHT SNI name because
+ // every edge holds a certificate for it — a check on it tests
+ // the path every visitor takes. Falling back to one member's
+ // hostname is a real downgrade: the check now depends on that
+ // one member's name and certificate, so if the member later
+ // moves or its route is removed, the check starts failing for
+ // reasons that have nothing to do with the edge's health.
+ //
+ // Note the interaction worth seeing in a log: if the synthetic
+ // edge.<apps-domain> name was DROPPED for a record conflict just
+ // above, this is where that shows up — a name-level conflict
+ // silently degrading every health check created afterwards.
+ if !loggedSNIFallback {
+ log.Printf("steering: no edge.<apps-domain> record in the desired set — new health checks fall back to a member hostname (%s) for SNI instead of the shared steering hostname; if that name was skipped for a record conflict above, THAT is the cause, and the check now rides one member's name and certificate", fqdnFor)
+ loggedSNIFallback = true
+ }
}
id, err := createHealthCheck(ctx, client, zoneID, eip, fqdnFor)
if err != nil {
@@ -620,45 +637,110 @@ func sameRRSet(have, want r53types.ResourceRecordSet) bool {
return true
}
-// plainConflicts returns the wanted names the zone already holds a PLAIN A
-// record for — one with no SetIdentifier — lowercased with the trailing dot.
+// conflictedNames returns the wanted names this converger cannot write a
+// latency A record at, lowercased with the trailing dot, because an existing
+// record there makes the write illegal.
+//
+// FINDING 2 (review round 1) + FINDING A (round 2, 2026-08-22). Three record
+// shapes at a wanted name are all rejected by Route53, and change batches are
+// ATOMIC — so the rejection takes the whole batch with it. A single such
+// record 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. conflictReason enumerates the
+// three; they share one failure mode and therefore one cure.
//
-// 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 cure is to skip the conflicting NAME entirely — no upsert, no delete —
+// and say so loudly, naming what is in the way. Never the alternative of
+// deleting the offending record to make room: it is an operator's record
+// (a plain record has no SetIdentifier, which is static/operator territory
+// under the same rule isManaged enforces; a CNAME or a weighted set is
+// somebody's deliberate routing), 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.
//
-// 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 {
+// Scope note: this is a NAME-level refusal, and deliberately so — all three
+// shapes make the whole name unwritable. It is not the right shape for a
+// KEY-level conflict such as an existing ALIAS latency record at exactly one
+// (name, SetIdentifier) we want, which Route53 also refuses to convert in
+// place via UPSERT; that one is left open on the report rather than guessed
+// at here, because dropping the whole name for it would refuse to steer names
+// that are in fact steerable.
+func conflictedNames(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
}
+ reason := conflictReason(rr)
+ if reason == "" {
+ 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))
+ log.Printf("steering: SKIPPING %s entirely: %s — steering this name needs that record changed by its owner first; every other steered name in this zone is unaffected",
+ aws.ToString(rr.Name), reason)
}
return conflicted
}
+// conflictReason names why an existing record makes its name unwritable by
+// this converger, or "" if it does not. Three shapes, all with the SAME
+// failure mode — the write is rejected and takes the whole atomic batch with
+// it — and therefore all with the same cure: skip the name.
+func conflictReason(rr r53types.ResourceRecordSet) string {
+ setID := aws.ToString(rr.SetIdentifier)
+ switch {
+ // A CNAME is EXCLUSIVE in DNS: a name holding one may hold no other
+ // record type at all. A member steering a hostname that already CNAMEs
+ // somewhere in the steering zone is not exotic — it is the ordinary
+ // custom-domain shape the spec itself recommends (CNAME to
+ // edge.<apps-domain>), pointed at a name inside the same zone.
+ case rr.Type == r53types.RRTypeCname:
+ return fmt.Sprintf("the zone already holds a CNAME there (-> %s, TTL %d), and a name holding a CNAME may hold no other record type",
+ firstValue(rr), aws.ToInt64(rr.TTL))
+
+ // A plain (simple) A record and a routing-policy record set cannot share
+ // a name+type.
+ case rr.Type == r53types.RRTypeA && setID == "":
+ return fmt.Sprintf("the zone already holds a plain A record there (value %s, TTL %d), and Route53 forbids a name from holding both a simple record and a routing-policy record set",
+ firstValue(rr), aws.ToInt64(rr.TTL))
+
+ // One routing POLICY per name+type: a weighted, failover, geo, geo-
+ // proximity, CIDR or multivalue A set at this name cannot coexist with a
+ // latency set. Latency records are the one policy we write, and they are
+ // identified by Region being set.
+ case rr.Type == r53types.RRTypeA && setID != "" && rr.Region == "":
+ return fmt.Sprintf("the zone already holds a NON-LATENCY A record set there (set %q, %s), and Route53 allows only one routing policy per name and type",
+ setID, otherPolicyName(rr))
+ }
+ return ""
+}
+
+// otherPolicyName describes which non-latency routing policy a record set
+// uses, for the conflict log — an operator reading it should not have to go
+// look the record up to know what they are being told about.
+func otherPolicyName(rr r53types.ResourceRecordSet) string {
+ switch {
+ case rr.Weight != nil:
+ return "weighted"
+ case rr.Failover != "":
+ return "failover"
+ case rr.GeoLocation != nil:
+ return "geolocation"
+ case rr.GeoProximityLocation != nil:
+ return "geoproximity"
+ case rr.CidrRoutingConfig != nil:
+ return "CIDR"
+ case aws.ToBool(rr.MultiValueAnswer):
+ return "multivalue-answer"
+ }
+ return "a routing policy this converger does not write"
+}
+
// 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))
@@ -679,10 +761,10 @@ func dropNames(want []desiredRecord, names map[string]bool) []desiredRecord {
//
// 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
+// BACKSTOP and is deliberately untested — every conflict shape
+// (conflictReason) is one Route53 forbids from coexisting with a latency
+// record set at that 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) {
@@ -700,7 +782,14 @@ func partitionManaged(want []desiredRecord, existing []r53types.ResourceRecordSe
// 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 {
+ //
+ // NOT when this pass WANTS that key, though: recordChanges is
+ // about to adopt it, so "leaving it alone" would be a lie about
+ // the one record here that does get taken over. (The wanted
+ // check was lost when this loop was split out of recordChanges
+ // in review round 1; round 2 caught the regression.)
+ if rr.Type == r53types.RRTypeA && aws.ToString(rr.SetIdentifier) != "" && rr.Region != "" && rr.AliasTarget == nil &&
+ !wanted[recordKey(rr)] {
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))
}
diff --git a/internal/steering/route53_test.go b/internal/steering/route53_test.go
index 7369652d9..1bbb92a31 100644
--- a/internal/steering/route53_test.go
+++ b/internal/steering/route53_test.go
@@ -67,6 +67,86 @@ func (f *fakeR53) writes() int {
return f.nChangeRecords + f.nCreateCheck + f.nDeleteCheck + f.nChangeTags
}
+// validateZone enforces the three Route53 rules this converger can violate,
+// each of which rejects the WHOLE batch:
+//
+// 1. a name+type holds either one simple record OR a routing-policy record
+// set, never both;
+// 2. a name holding a CNAME holds no other record type;
+// 3. one routing policy per name+type — a weighted set and a latency set
+// cannot share a name.
+//
+// Modelled on the projected post-batch state, so a batch is judged by what it
+// would produce, atomically.
+func validateZone(recs []r53types.ResourceRecordSet) error {
+ type nt struct{ name, typ string }
+ plain, policy := map[nt]bool{}, map[nt]bool{}
+ policies := map[nt]map[string]bool{}
+ names := map[string]map[string]bool{} // name -> set of types
+ for _, rr := range recs {
+ name := strings.ToLower(aws.ToString(rr.Name))
+ k := nt{name, string(rr.Type)}
+ if names[name] == nil {
+ names[name] = map[string]bool{}
+ }
+ names[name][string(rr.Type)] = true
+ if aws.ToString(rr.SetIdentifier) == "" {
+ plain[k] = true
+ continue
+ }
+ policy[k] = true
+ if policies[k] == nil {
+ policies[k] = map[string]bool{}
+ }
+ policies[k][fakePolicyKind(rr)] = true
+ }
+ for k := range plain {
+ if policy[k] {
+ return 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)
+ }
+ }
+ for name, types := range names {
+ if types["CNAME"] && len(types) > 1 {
+ return fmt.Errorf("fake: InvalidChangeBatch: RRSet with DNS name %s has a CNAME, which cannot coexist with any other record type", name)
+ }
+ }
+ for k, kinds := range policies {
+ if len(kinds) > 1 {
+ return fmt.Errorf("fake: InvalidChangeBatch: RRSet with DNS name %s and type %s mixes routing policies (%v) — only one policy per name and type", k.name, k.typ, sortedKeys(kinds))
+ }
+ }
+ return nil
+}
+
+func fakePolicyKind(rr r53types.ResourceRecordSet) string {
+ switch {
+ case rr.Region != "":
+ return "latency"
+ case rr.Weight != nil:
+ return "weighted"
+ case rr.Failover != "":
+ return "failover"
+ case rr.GeoLocation != nil:
+ return "geolocation"
+ case rr.GeoProximityLocation != nil:
+ return "geoproximity"
+ case rr.CidrRoutingConfig != nil:
+ return "cidr"
+ case aws.ToBool(rr.MultiValueAnswer):
+ return "multivalue"
+ }
+ return "unknown"
+}
+
+func sortedKeys(m map[string]bool) []string {
+ out := make([]string, 0, len(m))
+ for k := range m {
+ out = append(out, k)
+ }
+ sort.Strings(out)
+ return out
+}
+
func rrKey(rr r53types.ResourceRecordSet) string {
return strings.ToLower(aws.ToString(rr.Name)) + "|" + string(rr.Type) + "|" + aws.ToString(rr.SetIdentifier)
}
@@ -143,22 +223,8 @@ func (f *fakeR53) ChangeResourceRecordSets(ctx context.Context, in *route53.Chan
}
}
- // 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)
- }
+ if err := validateZone(projected); err != nil {
+ return nil, err
}
f.submitted = append(f.submitted, in.ChangeBatch.Changes...)
@@ -619,39 +685,179 @@ func TestConvergeStaticRecordsUntouched(t *testing.T) {
}
}
-// 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) {
+// TestFakeRejectsConflictingBatches proves the fake's refusals are real, so
+// the skips are fixes and not tautologies: 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. One row per conflict shape.
+func TestFakeRejectsConflictingBatches(t *testing.T) {
+ tests := []struct {
+ name string
+ seeded r53types.ResourceRecordSet
+ wantErr string
+ }{
+ {
+ name: "plain A",
+ seeded: staticA("app.example.com.", "9.9.9.9"),
+ wantErr: "cannot be both a simple record and a routing-policy record set",
+ },
+ {
+ name: "CNAME",
+ seeded: r53types.ResourceRecordSet{
+ Name: aws.String("app.example.com."), Type: r53types.RRTypeCname, TTL: aws.Int64(300),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("elsewhere.example.net.")}},
+ },
+ wantErr: "cannot coexist with any other record type",
+ },
+ {
+ name: "weighted A set",
+ seeded: r53types.ResourceRecordSet{
+ Name: aws.String("app.example.com."), Type: r53types.RRTypeA,
+ SetIdentifier: aws.String("legacy"), Weight: aws.Int64(10), TTL: aws.Int64(60),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("8.8.8.8")}},
+ },
+ wantErr: "mixes routing policies",
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ f := newFake()
+ f.records = []r53types.ResourceRecordSet{tt.seeded}
+
+ 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 the batch — it is more permissive than Route53")
+ }
+ if !strings.Contains(err.Error(), "InvalidChangeBatch") || !strings.Contains(err.Error(), tt.wantErr) {
+ t.Errorf("wrong error: %v", err)
+ }
+ // atomic: the innocent change did NOT land either
+ if len(f.records) != 1 {
+ t.Fatalf("the batch was not atomic: %v", recordNames(f.records))
+ }
+ })
+ }
+}
+
+// TestConvergeSkipsConflictingNames is FINDING A: the plain-A wedge has two
+// siblings with the identical failure mode. Each must be skipped by name, and
+// each must leave the OTHER steered names converging normally.
+func TestConvergeSkipsConflictingNames(t *testing.T) {
+ tests := []struct {
+ name string
+ seeded r53types.ResourceRecordSet
+ wantLogged string
+ }{
+ {
+ name: "a CNAME at a steered name",
+ seeded: r53types.ResourceRecordSet{
+ Name: aws.String("app.example.com."), Type: r53types.RRTypeCname, TTL: aws.Int64(300),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("elsewhere.example.net.")}},
+ },
+ wantLogged: "a name holding a CNAME may hold no other record type",
+ },
+ {
+ name: "a weighted A set at a steered name",
+ seeded: r53types.ResourceRecordSet{
+ Name: aws.String("app.example.com."), Type: r53types.RRTypeA,
+ SetIdentifier: aws.String("legacy"), Weight: aws.Int64(10), TTL: aws.Int64(60),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("8.8.8.8")}},
+ },
+ wantLogged: "only one routing policy per name and type",
+ },
+ {
+ name: "a failover A set at a steered name",
+ seeded: r53types.ResourceRecordSet{
+ Name: aws.String("app.example.com."), Type: r53types.RRTypeA,
+ SetIdentifier: aws.String("primary"), Failover: r53types.ResourceRecordSetFailoverPrimary,
+ TTL: aws.Int64(60),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("8.8.8.8")}},
+ },
+ wantLogged: "only one routing policy per name and type",
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ f := newFake()
+ f.records = []r53types.ResourceRecordSet{tt.seeded}
+ seededKey := rrKey(tt.seeded)
+
+ buf := captureLog(t)
+ want := append([]desiredRecord{dublinRec}, hostRecs("app.example.com.", dublinRec)...)
+ mustConverge(t, f, want)
+
+ // the conflicted name is skipped ENTIRELY — no upsert, no delete
+ for _, c := range f.submitted {
+ if strings.EqualFold(aws.ToString(c.ResourceRecordSet.Name), "app.example.com.") {
+ t.Errorf("submitted a change for the conflicted name: %s %s", c.Action, rrKey(*c.ResourceRecordSet))
+ }
+ }
+ // the operator's record is untouched
+ var found bool
+ for _, rr := range f.records {
+ if rrKey(rr) == seededKey {
+ found = true
+ }
+ }
+ if !found {
+ t.Errorf("the operator's record disappeared; zone = %v", recordNames(f.records))
+ }
+ // the log names the conflict and why
+ logged := buf.String()
+ if !strings.Contains(logged, "SKIPPING app.example.com.") {
+ t.Errorf("the conflict was not named; log = %q", logged)
+ }
+ if !strings.Contains(logged, tt.wantLogged) {
+ t.Errorf("the log did not explain the conflict (%q); log = %q", tt.wantLogged, logged)
+ }
+ // ...and the OTHER name still converges in the same pass
+ 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)
+ }
+ })
+ }
+}
+
+// TestConvergeSNIDowngradeIsLogged: when the synthetic edge.<apps-domain>
+// name is itself dropped for a conflict, every health check created
+// afterwards silently falls back to a member hostname for SNI. That
+// downgrade must be visible.
+func TestConvergeSNIDowngradeIsLogged(t *testing.T) {
f := newFake()
- f.records = []r53types.ResourceRecordSet{staticA("app.example.com.", "9.9.9.9")}
+ f.records = []r53types.ResourceRecordSet{staticA("edge.example.com.", "9.9.9.9")}
+
+ buf := captureLog(t)
+ want := append([]desiredRecord{dublinRec}, hostRecs("app.example.com.", dublinRec)...)
+ mustConverge(t, f, want)
- 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")}},
+ if len(f.createdChecks) != 1 {
+ t.Fatalf("want 1 check, got %d", len(f.createdChecks))
}
- 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 got := aws.ToString(f.createdChecks[0].HealthCheckConfig.FullyQualifiedDomainName); got != "app.example.com" {
+ t.Fatalf("SNI = %q, want the member hostname fallback", got)
}
- if !strings.Contains(err.Error(), "InvalidChangeBatch") {
- t.Errorf("wrong error: %v", err)
+ logged := buf.String()
+ if !strings.Contains(logged, "fall back to a member hostname") {
+ t.Errorf("the SNI downgrade was silent; log = %q", logged)
}
- if len(f.records) != 1 {
- t.Fatalf("the batch was not atomic: %v", recordNames(f.records))
+ if !strings.Contains(logged, "SKIPPING edge.example.com.") {
+ t.Errorf("the cause was not logged alongside it; log = %q", logged)
}
}
@@ -927,6 +1133,37 @@ func TestConvergeAdoptsForeignLatencyRecordUnderOurKey(t *testing.T) {
if !strings.Contains(buf.String(), "adopting") {
t.Errorf("the takeover was silent; log = %q", buf.String())
}
+ // ...and it must NOT also claim to be leaving that same record alone.
+ // The two lines describe opposite fates for one record; only one of them
+ // is true. (Regression: the "leaving alone" log lost its wanted-key
+ // guard when the loop was split out of recordChanges in review round 1.)
+ if strings.Contains(buf.String(), "leaving latency record") {
+ t.Errorf("logged BOTH adoption and 'leaving alone' for the same record; log = %q", buf.String())
+ }
+}
+
+// TestConvergeLeavingAloneLogNamesOnlyUnwantedKeys: the "leaving alone" line
+// is still owed for a foreign latency record this pass genuinely does not
+// touch — the FINDING B fix must not silence it everywhere.
+func TestConvergeLeavingAloneLogNamesOnlyUnwantedKeys(t *testing.T) {
+ f := newFake()
+ f.records = []r53types.ResourceRecordSet{
+ { // a foreign latency record at a name/identifier we do NOT want
+ Name: aws.String("legacy.example.com."), Type: r53types.RRTypeA,
+ SetIdentifier: aws.String("dublin"), Region: "eu-west-1", TTL: aws.Int64(60),
+ HealthCheckId: aws.String("hc-someone-else"),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("7.7.7.7")}},
+ },
+ }
+ buf := captureLog(t)
+ mustConverge(t, f, []desiredRecord{dublinRec})
+
+ if !strings.Contains(buf.String(), "leaving latency record legacy.example.com.") {
+ t.Errorf("the untouched foreign record was not reported; log = %q", buf.String())
+ }
+ if len(f.records) != 2 {
+ t.Errorf("the foreign record was touched; zone = %v", recordNames(f.records))
+ }
}
// TestIsManagedGates pins the deletion boundary GATE BY GATE.