diff --git a/app/operator/config/crd/bases/dns.appthrust.io_zoneunits.yaml b/app/operator/config/crd/bases/dns.appthrust.io_zoneunits.yaml index d7bef08..55811c1 100644 --- a/app/operator/config/crd/bases/dns.appthrust.io_zoneunits.yaml +++ b/app/operator/config/crd/bases/dns.appthrust.io_zoneunits.yaml @@ -499,8 +499,10 @@ spec: recordSetUID: description: |- RecordSetUID identifies the claim incarnation observed by the provider. - Empty or mismatched UIDs cannot authorize cleanup or transfer provider - ownership to a recreated claim, even when name and generation match. + A mismatched UID cannot authorize cleanup or transfer provider ownership + to a recreated claim, even when name and generation match. An empty UID + is a pre-upgrade receipt: it binds to the current claim only when the + provider observes the live record equal to the claim's desired state. minLength: 1 type: string required: diff --git a/deploy/charts/dns-api/crds/dns.appthrust.io_zoneunits.yaml b/deploy/charts/dns-api/crds/dns.appthrust.io_zoneunits.yaml index d7bef08..55811c1 100644 --- a/deploy/charts/dns-api/crds/dns.appthrust.io_zoneunits.yaml +++ b/deploy/charts/dns-api/crds/dns.appthrust.io_zoneunits.yaml @@ -499,8 +499,10 @@ spec: recordSetUID: description: |- RecordSetUID identifies the claim incarnation observed by the provider. - Empty or mismatched UIDs cannot authorize cleanup or transfer provider - ownership to a recreated claim, even when name and generation match. + A mismatched UID cannot authorize cleanup or transfer provider ownership + to a recreated claim, even when name and generation match. An empty UID + is a pre-upgrade receipt: it binds to the current claim only when the + provider observes the live record equal to the claim's desired state. minLength: 1 type: string required: diff --git a/docs/design/core-api.md b/docs/design/core-api.md index 890b1cd..7f3a899 100644 --- a/docs/design/core-api.md +++ b/docs/design/core-api.md @@ -729,7 +729,7 @@ If multiple `RecordSet` claims target the same record identity, core keeps the e - `recordSets`: provider result for accepted owner `RecordSet` claims. - `recordSets[].recordSetNamespace`: owner `RecordSet` namespace matching `spec.recordSets[]`. - `recordSets[].recordSetName`: owner `RecordSet` name matching `spec.recordSets[]`. -- `recordSets[].recordSetUID`: exact nonempty UID of the observed spec item. Missing or different UIDs do not establish ownership or authorize claim finalization. +- `recordSets[].recordSetUID`: exact nonempty UID of the observed spec item. A different UID never establishes ownership or authorizes claim finalization; a missing UID is a pre-upgrade receipt that binds only through the live evidence described under **UID cutover and legacy state**. - `recordSets[].observedGeneration`: owner `RecordSet` generation observed in `ZoneUnit.spec`. - `recordSets[].conditions`: provider acceptance and programming result for that `RecordSet` claim. - `recordSets[].provider.data`: public provider data to project to `RecordSet.status.provider.data`. @@ -746,11 +746,15 @@ Route 53 pending affected-record entries include the claim UID. A legacy or pred **UID cutover and legacy state.** -The UID fields are optional on the wire so existing ledgers remain readable. Explicit empty strings are invalid in the CRD; omitted or mismatched UIDs still fail closed in controllers. There is no namespace/name/generation fallback and no automatic relabeling of old provider state. +The UID fields are optional on the wire so existing ledgers remain readable. Explicit empty strings are invalid in the CRD; a mismatched UID always fails closed in controllers. There is no namespace/name/generation fallback and no relabeling of old provider state by name alone. -This is not a transparent in-place upgrade for existing UID-less ownership receipts. Apply the updated CRDs before adopting the corresponding controllers; Helm does not update existing CRDs from a chart's `crds/` directory during a normal upgrade. Qualify the coordinated controller/schema cutover and recovery of existing claims separately. +A `status.recordSets[]` entry without `recordSetUID` is a pre-upgrade receipt: the provider's own record that it programmed the record for that claim name, without proof of which incarnation. Providers bind such a receipt to the current claim incarnation only from live evidence, in place and without a provider write: the receipt must name exactly the record identity the claim desires (Route 53 hosted zone, record name and type; Cloudflare record IDs covering every same-type record at that name) and the live record must already equal the claim's desired state. The binding then only records which incarnation owns the record; a live record that differs from desired stays `ProviderConflict`, because a pre-upgrade receipt cannot distinguish an ordinary desired-value update from a recreated claim inheriting a stranger's record. The same evidence authorizes deletion for a deleting claim whose retained desired values still match the live record; a minimal cleanup item without values cannot match. A receipt marked `deletionCompleted` never binds. Providers emit a `*LegacyOwnershipBound` event when a receipt is bound. -Already-present external records with unbound ownership remain conflicts. Recovery uses separately authorized, provider-validated explicit `RecordSet.spec.adoption`, or a fresh provider observation establishing absence before normal creation or cleanup. Desired-value equality alone does not adopt a foreign Cloudflare record ID. Do not patch UIDs, provider status, or finalizers to manufacture ownership, and do not enable zone-wide adoption as an automatic fallback. The source correction neither restores already-lost live authority nor authorizes release or deployment. +Apply the updated CRDs before adopting the corresponding controllers; Helm does not update existing CRDs from a chart's `crds/` directory during a normal upgrade, so the CRD delivery owner must apply them with the controller image (Argo CD renders `crds/` and upgrades them unless `skipCrds` is set). Until the CRD carries `recordSetUID`, the field is pruned from `ZoneUnit.spec` and every provider status write fails with a source-changed conflict. + +Providers also prune orphaned ledger entries whose claim namespace/name has no `spec.recordSets[]` item once the entry can no longer matter: `deletionCompleted` is set, or (Route 53) the record it names is absent from the hosted zone, or (Cloudflare) it records no IDs. An orphan that still names a live record is kept deliberately: core may drop a spec item transiently (a rejected claim whose generation changed) and re-add it later, and the retained entry is the only evidence of a leaked record. Cloudflare does not confirm absence per ID, so ID-bearing orphans stay until an operator removes the records. + +Already-present external records with unbound ownership and no matching receipt remain conflicts. Recovery uses separately authorized, provider-validated explicit `RecordSet.spec.adoption`, or a fresh provider observation establishing absence before normal creation or cleanup. Desired-value equality alone does not adopt a foreign Cloudflare record ID. Do not patch UIDs, provider status, or finalizers to manufacture ownership, and do not enable zone-wide adoption as an automatic fallback. The source correction neither restores already-lost live authority nor authorizes release or deployment. Provider-specific status follows a public-data boundary: diff --git a/internal/go/providers/cloudflare/controllers/zoneunit/recordset.go b/internal/go/providers/cloudflare/controllers/zoneunit/recordset.go index 3991f07..71dd6bf 100644 --- a/internal/go/providers/cloudflare/controllers/zoneunit/recordset.go +++ b/internal/go/providers/cloudflare/controllers/zoneunit/recordset.go @@ -73,10 +73,14 @@ type recordSetReconciler struct { } func (r *recordSetReconciler) reconcileZoneUnitRecordSets(ctx context.Context, unit *dnsv1alpha1.ZoneUnit) (ctrl.Result, error) { + if pruned, err := r.pruneOrphanedRecordSetStatuses(ctx, unit); err != nil || pruned { + return ctrl.Result{Requeue: pruned}, err + } statusByRef := map[string]dnsv1alpha1.ZoneUnitRecordSetStatus{} for _, status := range unit.Status.RecordSets { statusByRef[cloudflareRecordSetClaimKey(status.RecordSetNamespace, status.RecordSetName)] = status } + legacy := legacyRecordSetReceipts(unit) var aggregate ctrl.Result for _, item := range unit.Spec.RecordSets { @@ -87,9 +91,13 @@ func (r *recordSetReconciler) reconcileZoneUnitRecordSets(ctx context.Context, u aggregate = mergeCloudflareRecordSetResult(aggregate, ctrl.Result{RequeueAfter: r.requeueAfter()}) continue } - recordSet := cloudflareRecordSetFromZoneUnitItem(unit, item, statusByRef[cloudflareRecordSetClaimKey(item.RecordSetNamespace, item.RecordSetName)]) + claimKey := cloudflareRecordSetClaimKey(item.RecordSetNamespace, item.RecordSetName) + recordSet := cloudflareRecordSetFromZoneUnitItem(unit, item, statusByRef[claimKey]) ctxData, accepted, err := r.acceptRecordSetFromZoneUnit(ctx, &recordSet, unit) ctxData.RecordSetSource = item.DeepCopy() + if receipt, ok := legacy[claimKey]; ok { + ctxData.LegacyReceipt = &receipt + } if err != nil || !accepted { return aggregate, err } @@ -130,8 +138,11 @@ type cloudflareRecordSetContext struct { Version *dnsv1alpha1.ProviderVersion ZoneUnit *dnsv1alpha1.ZoneUnit RecordSetSource *dnsv1alpha1.ZoneUnitRecordSetSpec - ZoneID string - FullName string + // LegacyReceipt is the UID-less pre-upgrade ledger entry for this claim, + // if any. It is never loaded into RecordSet status; see legacyReceiptRecords. + LegacyReceipt *dnsv1alpha1.ZoneUnitRecordSetStatus + ZoneID string + FullName string } func cloudflareRecordSetFromZoneUnitItem(unit *dnsv1alpha1.ZoneUnit, item dnsv1alpha1.ZoneUnitRecordSetSpec, status dnsv1alpha1.ZoneUnitRecordSetStatus) dnsv1alpha1.RecordSet { @@ -369,6 +380,12 @@ func (r *recordSetReconciler) reconcileNormal(ctx context.Context, provider Reco managedIDs := cloudflareStatusRecordIDs(recordSet) if len(managedIDs) == 0 { if len(sameType) > 0 { + if ctxData.LegacyReceipt != nil { + if bound, ok := legacyReceiptRecords(*ctxData.LegacyReceipt, recordSet, ctxData.FullName, current); ok { + r.recordEvent(recordSet, corev1.EventTypeNormal, "CloudflareRecordSetLegacyOwnershipBound", fmt.Sprintf("pre-upgrade Cloudflare ownership receipt was bound to RecordSet %s", recordSet.UID)) + return ctrl.Result{}, r.setReady(ctx, recordSet, bound) + } + } return ctrl.Result{}, r.setProgrammed(ctx, recordSet, metav1.ConditionFalse, "ProviderConflict", "same Cloudflare DNS records already exist") } return r.applyCloudflareRecordSetDiff(ctx, provider, recordSet, ctxData, nil, desired) @@ -444,10 +461,22 @@ func (r *recordSetReconciler) reconcileDeleteWithContext(ctx context.Context, re if err != nil { return r.failProgrammedForProviderError(ctx, recordSet, err) } - if len(observed) > 0 { + if len(observed) == 0 { + return ctrl.Result{}, r.removeFinalizer(ctx, recordSet) + } + bound, err := r.bindLegacyReceiptForDeletion(ctx, recordSet, ctxData, observed) + if err != nil { + return ctrl.Result{}, err + } + if !bound { return ctrl.Result{}, r.setProgrammed(ctx, recordSet, metav1.ConditionFalse, "ProviderOwnershipNotEstablished", "Cloudflare DNS records exist but are not owned by the current RecordSet UID") } - return ctrl.Result{}, r.removeFinalizer(ctx, recordSet) + // patchStatus rebased recordSet.Status onto the bound ledger entry. + statusData, err = cloudflareRecordSetStatusData(recordSet) + if err != nil { + return ctrl.Result{}, r.setProgrammed(ctx, recordSet, metav1.ConditionFalse, "ReconcileError", err.Error()) + } + ids = cloudflareRecordStatusIDs(statusData.Records) } var deletes []CloudflareDNSRecord for _, id := range ids { diff --git a/internal/go/providers/cloudflare/controllers/zoneunit/recordset_ledger.go b/internal/go/providers/cloudflare/controllers/zoneunit/recordset_ledger.go new file mode 100644 index 0000000..d5f6e2a --- /dev/null +++ b/internal/go/providers/cloudflare/controllers/zoneunit/recordset_ledger.go @@ -0,0 +1,142 @@ +package cloudflare + +import ( + "context" + "fmt" + "slices" + + cloudflarev1alpha1 "github.com/appthrust/dns-api/pkg/go/api/cloudflare/v1alpha1" + dnsv1alpha1 "github.com/appthrust/dns-api/pkg/go/api/dns/v1alpha1" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// legacyRecordSetReceipts returns the ZoneUnit.status.recordSets[] entries +// written before recordSetUID existed, keyed by claim namespace/name. +// +// Such an entry is the provider's own receipt that it created the listed +// record IDs for that claim name, but it cannot prove which claim incarnation +// they belong to. Callers must pair it with fresh live evidence before treating +// the records as owned; a completed or ID-less entry carries no receipt. +func legacyRecordSetReceipts(unit *dnsv1alpha1.ZoneUnit) map[string]dnsv1alpha1.ZoneUnitRecordSetStatus { + receipts := map[string]dnsv1alpha1.ZoneUnitRecordSetStatus{} + for _, status := range unit.Status.RecordSets { + if status.RecordSetUID != "" || status.DeletionCompleted { + continue + } + data, err := cloudflareRecordSetStatusDataFromProvider(status.Provider) + if err != nil || len(cloudflareRecordStatusIDs(data.Records)) == 0 { + continue + } + receipts[cloudflareRecordSetClaimKey(status.RecordSetNamespace, status.RecordSetName)] = status + } + return receipts +} + +// legacyReceiptRecords returns the live records a UID-less receipt may bind to +// the current claim incarnation. Every listed record of the claim's type must +// carry an ID from the receipt, and those records must already equal the +// desired state: the migration then changes nothing in Cloudflare, it only +// records which incarnation owns the IDs. A foreign same-type record or a live +// value that differs from desired stays a conflict, because a pre-upgrade +// receipt cannot distinguish an ordinary desired-value update from a recreated +// claim inheriting a stranger's records. +func legacyReceiptRecords(receipt dnsv1alpha1.ZoneUnitRecordSetStatus, recordSet *dnsv1alpha1.RecordSet, fullName string, listed []CloudflareDNSRecord) ([]CloudflareDNSRecord, bool) { + data, err := cloudflareRecordSetStatusDataFromProvider(receipt.Provider) + if err != nil { + return nil, false + } + ids := cloudflareRecordStatusIDs(data.Records) + if len(ids) == 0 { + return nil, false + } + options, err := cloudflareRecordSetOptions(recordSet) + if err != nil || validateCloudflareRecordSetOptions(recordSet, options) != "" { + return nil, false + } + desired, err := desiredCloudflareDNSRecords(recordSet, fullName, options) + if err != nil { + return nil, false + } + sameType := cloudflareRecordsByType(listed, string(recordSet.Spec.Type)) + if len(sameType) == 0 { + return nil, false + } + for _, record := range sameType { + if !slices.Contains(ids, record.ID) { + return nil, false + } + } + if !cloudflareDNSRecordSetEqual(sameType, desired) { + return nil, false + } + return sameType, true +} + +// bindLegacyReceiptForDeletion binds a pre-upgrade receipt to a deleting claim +// when the retained desired values still match the live records. Deletion is +// the one mutation a claim's own receipt must keep authorizing after the UID +// cutover; otherwise every pre-upgrade claim would be stuck in deletion. A +// minimal cleanup item without values cannot match and stays a conflict. +func (r *recordSetReconciler) bindLegacyReceiptForDeletion(ctx context.Context, recordSet *dnsv1alpha1.RecordSet, ctxData cloudflareRecordSetContext, observed []CloudflareDNSRecord) (bool, error) { + if ctxData.LegacyReceipt == nil { + return false, nil + } + bound, ok := legacyReceiptRecords(*ctxData.LegacyReceipt, recordSet, ctxData.FullName, observed) + if !ok { + return false, nil + } + if err := r.patchCloudflareRecordSetStatus(ctx, recordSet, func(data *cloudflarev1alpha1.CloudflareRecordSetStatusData, state *cloudflarev1alpha1.CloudflareRecordSetState) { + data.Records = cloudflareDNSRecordStatuses(bound) + state.Records = cloudflareDNSRecordStates(bound) + }); err != nil { + return false, err + } + r.recordEvent(recordSet, corev1.EventTypeNormal, "CloudflareRecordSetLegacyOwnershipBound", fmt.Sprintf("pre-upgrade Cloudflare ownership receipt was bound to deleting RecordSet %s", recordSet.UID)) + return true, nil +} + +// pruneOrphanedRecordSetStatuses drops status.recordSets[] entries whose claim +// key has no spec item and which can no longer matter: cleanup completed, or +// no record ID is recorded. Without this the ledger grows with every deletion +// and pre-upgrade UID-less entries stay forever. +// +// An orphan that still records IDs is kept on purpose. Core can drop a spec +// item transiently (a claim rejected while its generation changed) and re-add +// it later; pruning that receipt would turn the provider's own records into a +// permanent ProviderConflict. Confirming absence would need one Cloudflare +// read per recorded ID on every reconcile, so the retained entry stays as the +// only evidence of a leaked record. +// +// Returns true after a write so the caller re-observes the ZoneUnit. The +// caller's unit may already trail zone-status writes from the same pass, so +// the patch is based on a fresh observation rather than that snapshot. +func (r *recordSetReconciler) pruneOrphanedRecordSetStatuses(ctx context.Context, unit *dnsv1alpha1.ZoneUnit) (bool, error) { + var observed dnsv1alpha1.ZoneUnit + if err := r.Get(ctx, client.ObjectKeyFromObject(unit), &observed); err != nil { + return false, client.IgnoreNotFound(err) + } + claims := make(map[string]struct{}, len(observed.Spec.RecordSets)) + for _, item := range observed.Spec.RecordSets { + claims[cloudflareRecordSetClaimKey(item.RecordSetNamespace, item.RecordSetName)] = struct{}{} + } + prunable := func(status dnsv1alpha1.ZoneUnitRecordSetStatus) bool { + if _, claimed := claims[cloudflareRecordSetClaimKey(status.RecordSetNamespace, status.RecordSetName)]; claimed { + return false + } + if status.DeletionCompleted { + return true + } + data, err := cloudflareRecordSetStatusDataFromProvider(status.Provider) + return err != nil || len(cloudflareRecordStatusIDs(data.Records)) == 0 + } + if !slices.ContainsFunc(observed.Status.RecordSets, prunable) { + return false, nil + } + base := observed.DeepCopy() + observed.Status.RecordSets = slices.DeleteFunc(observed.Status.RecordSets, prunable) + if err := r.Status().Patch(ctx, &observed, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})); err != nil { + return false, client.IgnoreNotFound(err) + } + return true, nil +} diff --git a/internal/go/providers/cloudflare/controllers/zoneunit/recordset_ledger_test.go b/internal/go/providers/cloudflare/controllers/zoneunit/recordset_ledger_test.go new file mode 100644 index 0000000..2c1e6bc --- /dev/null +++ b/internal/go/providers/cloudflare/controllers/zoneunit/recordset_ledger_test.go @@ -0,0 +1,260 @@ +package cloudflare + +import ( + "testing" + + cloudflarev1alpha1 "github.com/appthrust/dns-api/pkg/go/api/cloudflare/v1alpha1" + dnsv1alpha1 "github.com/appthrust/dns-api/pkg/go/api/dns/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// cloudflareLegacyReceipt is a pre-upgrade ledger entry: no recordSetUID, but +// the provider's own record IDs for the claim name. +func cloudflareLegacyReceipt(t *testing.T, recordSet *dnsv1alpha1.RecordSet, records ...CloudflareDNSRecord) dnsv1alpha1.ZoneUnitRecordSetStatus { + t.Helper() + return dnsv1alpha1.ZoneUnitRecordSetStatus{ + RecordSetNamespace: recordSet.Namespace, + RecordSetName: recordSet.Name, + ObservedGeneration: recordSet.Generation, + Provider: &dnsv1alpha1.ProviderStatus{Data: cloudflareRaw(t, cloudflarev1alpha1.CloudflareRecordSetStatusData{ + Records: cloudflareDNSRecordStatuses(records), + })}, + Conditions: []metav1.Condition{ + {Type: string(dnsv1alpha1.ConditionProgrammed), Status: metav1.ConditionTrue, Reason: "Programmed", ObservedGeneration: recordSet.Generation}, + }, + } +} + +func cloudflareLegacyLiveRecords() []CloudflareDNSRecord { + first := cloudflareLifecycleARecord("023e105f4ecef8ad9ca31a8372d0c010") + second := cloudflareLifecycleARecord("023e105f4ecef8ad9ca31a8372d0c011") + second.Content = "192.0.2.11" + return []CloudflareDNSRecord{first, second} +} + +func TestCloudflareRecordSetBindsLegacyReceiptWhenLiveRecordsMatchDesired(t *testing.T) { + ctx := t.Context() + zone := cloudflareProgrammedZone("app", "apps-example-com") + recordSet := cloudflareARecordSet("app", "www-a") + recordSet.UID = types.UID("42f149e7-107d-48bb-885c-7a6c539678a6") + unit := cloudflareRecordSetLifecycleUnit(recordSet, zone, false) + live := cloudflareLegacyLiveRecords() + unit.Status.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetStatus{cloudflareLegacyReceipt(t, recordSet, live...)} + k8sClient := cloudflareRecordSetLifecycleClient(t, zone, unit) + provider := &fakeCloudflareRecordSetProvider{records: live} + reconciler := &recordSetReconciler{Client: k8sClient, Provider: provider} + + if _, err := reconciler.reconcileZoneUnitRecordSets(ctx, unit); err != nil { + t.Fatalf("Reconcile returned error: %v", err) + } + if len(provider.batches) != 0 || len(provider.patched) != 0 || len(provider.deleted) != 0 { + t.Fatalf("binding a matching legacy receipt mutated Cloudflare: %#v %#v %#v", provider.batches, provider.patched, provider.deleted) + } + var got dnsv1alpha1.ZoneUnit + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(unit), &got); err != nil { + t.Fatal(err) + } + if len(got.Status.RecordSets) != 1 { + t.Fatalf("legacy entry was appended instead of bound in place: %#v", got.Status.RecordSets) + } + status := got.Status.RecordSets[0] + if status.RecordSetUID != recordSet.UID { + t.Fatalf("ledger UID = %q, want current %q", status.RecordSetUID, recordSet.UID) + } + data, err := cloudflareRecordSetStatusDataFromProvider(status.Provider) + if err != nil { + t.Fatal(err) + } + if ids := cloudflareRecordStatusIDs(data.Records); len(ids) != 2 || ids[0] != live[0].ID || ids[1] != live[1].ID { + t.Fatalf("bound record IDs = %#v, want the receipt's live IDs", ids) + } + assertCloudflareCondition(t, status.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionTrue, "Programmed") +} + +func TestCloudflareRecordSetRejectsLegacyReceiptWithoutEvidence(t *testing.T) { + cases := []struct { + name string + receipt func(*testing.T, *dnsv1alpha1.RecordSet, []CloudflareDNSRecord) dnsv1alpha1.ZoneUnitRecordSetStatus + live func() []CloudflareDNSRecord + }{ + { + name: "live record differs from desired", + receipt: func(t *testing.T, recordSet *dnsv1alpha1.RecordSet, live []CloudflareDNSRecord) dnsv1alpha1.ZoneUnitRecordSetStatus { + return cloudflareLegacyReceipt(t, recordSet, live...) + }, + live: func() []CloudflareDNSRecord { + live := cloudflareLegacyLiveRecords() + live[1].Content = "192.0.2.99" + return live + }, + }, + { + name: "a same-type record is not in the receipt", + receipt: func(t *testing.T, recordSet *dnsv1alpha1.RecordSet, live []CloudflareDNSRecord) dnsv1alpha1.ZoneUnitRecordSetStatus { + return cloudflareLegacyReceipt(t, recordSet, live[0]) + }, + live: cloudflareLegacyLiveRecords, + }, + { + name: "receipt already completed deletion", + receipt: func(t *testing.T, recordSet *dnsv1alpha1.RecordSet, live []CloudflareDNSRecord) dnsv1alpha1.ZoneUnitRecordSetStatus { + receipt := cloudflareLegacyReceipt(t, recordSet, live...) + receipt.DeletionCompleted = true + return receipt + }, + live: cloudflareLegacyLiveRecords, + }, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + ctx := t.Context() + zone := cloudflareProgrammedZone("app", "apps-example-com") + recordSet := cloudflareARecordSet("app", "www-a") + recordSet.UID = types.UID("42f149e7-107d-48bb-885c-7a6c539678a6") + unit := cloudflareRecordSetLifecycleUnit(recordSet, zone, false) + live := testCase.live() + unit.Status.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetStatus{testCase.receipt(t, recordSet, live)} + k8sClient := cloudflareRecordSetLifecycleClient(t, zone, unit) + provider := &fakeCloudflareRecordSetProvider{records: live} + reconciler := &recordSetReconciler{Client: k8sClient, Provider: provider} + + if _, err := reconciler.reconcileZoneUnitRecordSets(ctx, unit); err != nil { + t.Fatalf("Reconcile returned error: %v", err) + } + if len(provider.batches) != 0 || len(provider.patched) != 0 || len(provider.deleted) != 0 { + t.Fatalf("unbound legacy receipt authorized a provider mutation: %#v %#v %#v", provider.batches, provider.patched, provider.deleted) + } + var got dnsv1alpha1.ZoneUnit + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(unit), &got); err != nil { + t.Fatal(err) + } + if len(got.Status.RecordSets) != 1 { + t.Fatalf("ledger = %#v, want one entry for the claim", got.Status.RecordSets) + } + status := got.Status.RecordSets[0] + if status.RecordSetUID != recordSet.UID { + t.Fatalf("ledger UID = %q, want current %q", status.RecordSetUID, recordSet.UID) + } + if ids := cloudflareRecordStatusIDs(mustCloudflareStatusData(t, status).Records); len(ids) != 0 { + t.Fatalf("legacy record IDs were rebound without evidence: %#v", ids) + } + assertCloudflareCondition(t, status.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionFalse, "ProviderConflict") + }) + } +} + +func mustCloudflareStatusData(t *testing.T, status dnsv1alpha1.ZoneUnitRecordSetStatus) cloudflarev1alpha1.CloudflareRecordSetStatusData { + t.Helper() + data, err := cloudflareRecordSetStatusDataFromProvider(status.Provider) + if err != nil { + t.Fatal(err) + } + return data +} + +func TestCloudflareRecordSetDeletesRecordsThroughLegacyReceipt(t *testing.T) { + ctx := t.Context() + zone := cloudflareProgrammedZone("app", "apps-example-com") + recordSet := cloudflareARecordSet("app", "www-a") + recordSet.UID = types.UID("42f149e7-107d-48bb-885c-7a6c539678a6") + unit := cloudflareRecordSetLifecycleUnit(recordSet, zone, true) + live := cloudflareLegacyLiveRecords() + unit.Status.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetStatus{cloudflareLegacyReceipt(t, recordSet, live...)} + k8sClient := cloudflareRecordSetLifecycleClient(t, zone, unit) + provider := &fakeCloudflareRecordSetProvider{records: live} + reconciler := &recordSetReconciler{Client: k8sClient, Provider: provider} + + if _, err := reconciler.reconcileZoneUnitRecordSets(ctx, unit); err != nil { + t.Fatalf("Reconcile returned error: %v", err) + } + if len(provider.deleted) != 2 { + t.Fatalf("deleting claim with a matching legacy receipt did not delete its records: %#v", provider.deleted) + } + var got dnsv1alpha1.ZoneUnit + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(unit), &got); err != nil { + t.Fatal(err) + } + if len(got.Status.RecordSets) != 1 || got.Status.RecordSets[0].RecordSetUID != recordSet.UID { + t.Fatalf("ledger = %#v, want the legacy entry bound to the deleting claim", got.Status.RecordSets) + } +} + +func TestCloudflareRecordSetDoesNotDeleteThroughLegacyReceiptWhenLiveDiffers(t *testing.T) { + ctx := t.Context() + zone := cloudflareProgrammedZone("app", "apps-example-com") + recordSet := cloudflareARecordSet("app", "www-a") + recordSet.UID = types.UID("42f149e7-107d-48bb-885c-7a6c539678a6") + unit := cloudflareRecordSetLifecycleUnit(recordSet, zone, true) + live := cloudflareLegacyLiveRecords() + live[0].Content = "192.0.2.99" + unit.Status.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetStatus{cloudflareLegacyReceipt(t, recordSet, live...)} + k8sClient := cloudflareRecordSetLifecycleClient(t, zone, unit) + provider := &fakeCloudflareRecordSetProvider{records: live} + reconciler := &recordSetReconciler{Client: k8sClient, Provider: provider} + + if _, err := reconciler.reconcileZoneUnitRecordSets(ctx, unit); err != nil { + t.Fatalf("Reconcile returned error: %v", err) + } + if len(provider.deleted) != 0 || len(provider.batches) != 0 { + t.Fatalf("legacy receipt deleted records that differ from the claim: %#v %#v", provider.deleted, provider.batches) + } + var got dnsv1alpha1.ZoneUnit + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(unit), &got); err != nil { + t.Fatal(err) + } + status := cloudflareLifecycleRecordSetStatus(t, &got, recordSet) + if status.DeletionCompleted || len(cloudflareRecordStatusIDs(mustCloudflareStatusData(t, status).Records)) != 0 { + t.Fatalf("ledger = %#v, want an unbound, incomplete entry", status) + } + assertCloudflareCondition(t, status.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionFalse, "ProviderOwnershipNotEstablished") +} + +func TestCloudflareRecordSetPrunesSpentLedgerEntriesWithoutSpecItem(t *testing.T) { + ctx := t.Context() + zone := cloudflareProgrammedZone("app", "apps-example-com") + recordSet := cloudflareARecordSet("app", "www-a") + unit := cloudflareRecordSetLifecycleUnit(recordSet, zone, false) + live := cloudflareLegacyLiveRecords() + current := dnsv1alpha1.ZoneUnitRecordSetStatus{ + RecordSetNamespace: recordSet.Namespace, + RecordSetName: recordSet.Name, + RecordSetUID: recordSet.UID, + ObservedGeneration: recordSet.Generation, + Provider: &dnsv1alpha1.ProviderStatus{Data: cloudflareRaw(t, cloudflarev1alpha1.CloudflareRecordSetStatusData{ + Records: cloudflareDNSRecordStatuses(live), + })}, + } + completedOrphan := cloudflareLegacyReceipt(t, cloudflareARecordSet("app", "gone-completed"), live[0]) + completedOrphan.RecordSetUID = types.UID("386cde02-3890-4ee6-a01a-037ed8354b5d") + completedOrphan.DeletionCompleted = true + emptyOrphan := cloudflareLegacyReceipt(t, cloudflareARecordSet("app", "gone-empty")) + leakedOrphan := cloudflareLegacyReceipt(t, cloudflareARecordSet("app", "gone-leaked"), cloudflareLifecycleARecord("023e105f4ecef8ad9ca31a8372d0c099")) + unit.Status.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetStatus{completedOrphan, current, emptyOrphan, leakedOrphan} + k8sClient := cloudflareRecordSetLifecycleClient(t, zone, unit) + provider := &fakeCloudflareRecordSetProvider{records: live} + reconciler := &recordSetReconciler{Client: k8sClient, Provider: provider} + + result, err := reconciler.reconcileZoneUnitRecordSets(ctx, unit) + if err != nil { + t.Fatalf("Reconcile returned error: %v", err) + } + if !result.Requeue { + t.Fatalf("prune did not request re-observation: %#v", result) + } + if len(provider.batches) != 0 || len(provider.patched) != 0 || len(provider.deleted) != 0 { + t.Fatalf("prune pass mutated Cloudflare: %#v %#v %#v", provider.batches, provider.patched, provider.deleted) + } + var got dnsv1alpha1.ZoneUnit + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(unit), &got); err != nil { + t.Fatal(err) + } + names := make([]string, 0, len(got.Status.RecordSets)) + for _, status := range got.Status.RecordSets { + names = append(names, status.RecordSetName) + } + if len(names) != 2 || names[0] != "www-a" || names[1] != "gone-leaked" { + t.Fatalf("ledger after prune = %#v, want the claimed entry and the leaked-ID orphan", names) + } +} diff --git a/internal/go/providers/route53/controllers/zoneunit/recordset.go b/internal/go/providers/route53/controllers/zoneunit/recordset.go index 716fdab..6c8f0e3 100644 --- a/internal/go/providers/route53/controllers/zoneunit/recordset.go +++ b/internal/go/providers/route53/controllers/zoneunit/recordset.go @@ -28,10 +28,11 @@ type plannedRecordSetChange struct { } func (r *ZoneReconciler) reconcileZoneRecordSets(ctx context.Context, provider Provider, zone *dnsv1alpha1.Zone, zoneClass *dnsv1alpha1.ZoneClass, hostedZone HostedZone) (ctrl.Result, error) { - recordSets, err := r.recordSetsForZone(ctx, zone) - if err != nil { - return ctrl.Result{}, err + var unit dnsv1alpha1.ZoneUnit + if err := r.Get(ctx, client.ObjectKey{Namespace: zone.Namespace, Name: zone.Name}, &unit); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) } + recordSets, legacy := zoneUnitRecordSets(&unit) ownership, err := r.zoneUnitOwnershipForZone(ctx, zone) if err != nil { return ctrl.Result{}, err @@ -49,6 +50,9 @@ func (r *ZoneReconciler) reconcileZoneRecordSets(ctx context.Context, provider P return r.resultForProviderError(err), nil } current := indexRecordSets(currentRecords) + if pruned, err := r.pruneOrphanedRecordSetStatuses(ctx, &unit, current); err != nil || pruned { + return ctrl.Result{Requeue: pruned}, err + } var planned []plannedRecordSetChange deferred := false @@ -56,7 +60,7 @@ func (r *ZoneReconciler) reconcileZoneRecordSets(ctx context.Context, provider P for index := range recordSets { recordSet := &recordSets[index] source := recordSet.DeepCopy() - change, ok, err := r.planRecordSet(ctx, zone, zoneClass, hostedZone, recordSet, ownership, current) + change, ok, err := r.planRecordSet(ctx, zone, zoneClass, hostedZone, recordSet, ownership, current, legacy) if err != nil { return ctrl.Result{}, err } @@ -208,11 +212,13 @@ func (r *ZoneReconciler) planRecordSet( recordSet *dnsv1alpha1.RecordSet, ownership *zoneUnitRecordSetOwnership, current map[string]RecordSetResource, + legacy map[string]dnsv1alpha1.ZoneUnitRecordSetStatus, ) (RecordSetChange, bool, error) { identity := recordSetIdentity(recordSet.Spec.Type, canonicalRecordName(recordSet.Spec.Name, zone.Spec.DomainName)) + receipt, hasReceipt := legacy[recordSetClaimKey(recordSet.Namespace, recordSet.Name)] if !recordSet.DeletionTimestamp.IsZero() { - return r.planRecordSetDelete(ctx, recordSet, ownership, current[identity.key()]) + return r.planRecordSetDelete(ctx, hostedZone, recordSet, ownership, current[identity.key()], identity.recordName, receipt, hasReceipt) } statusData, err := route53RecordSetStatusData(recordSet) @@ -264,6 +270,10 @@ func (r *ZoneReconciler) planRecordSet( } if exists && !managed { + if hasReceipt && legacyReceiptBindsRecordSet(receipt, desired, existing) { + r.recordEvent(recordSet, corev1.EventTypeNormal, "Route53RecordSetLegacyOwnershipBound", fmt.Sprintf("pre-upgrade Route 53 ownership receipt was bound to RecordSet %s for type=%s name=%s", recordSet.UID, recordSet.Spec.Type, recordSet.Spec.Name)) + return RecordSetChange{}, false, r.setRecordSetReady(ctx, recordSet, desired) + } return RecordSetChange{}, false, r.setRecordSetProgrammed(ctx, recordSet, metav1.ConditionFalse, "ProviderConflict", "same Route 53 record set already exists") } if exists && route53RecordSetEqual(existing, desired) { @@ -281,7 +291,7 @@ func (r *ZoneReconciler) planRecordSet( return RecordSetChange{Action: RecordSetChangeActionUpsert, RecordSet: desired}, true, nil } -func (r *ZoneReconciler) planRecordSetDelete(ctx context.Context, recordSet *dnsv1alpha1.RecordSet, ownership *zoneUnitRecordSetOwnership, existing RecordSetResource) (RecordSetChange, bool, error) { +func (r *ZoneReconciler) planRecordSetDelete(ctx context.Context, hostedZone HostedZone, recordSet *dnsv1alpha1.RecordSet, ownership *zoneUnitRecordSetOwnership, existing RecordSetResource, recordName string, receipt dnsv1alpha1.ZoneUnitRecordSetStatus, hasReceipt bool) (RecordSetChange, bool, error) { if !slices.Contains(recordSet.Finalizers, RecordSetFinalizer) { return RecordSetChange{}, false, nil } @@ -305,11 +315,43 @@ func (r *ZoneReconciler) planRecordSetDelete(ctx context.Context, recordSet *dns return RecordSetChange{}, false, r.setRecordSetProgrammed(ctx, recordSet, metav1.ConditionFalse, "ReconcileError", err.Error()) } if !adopting && !providerStatusHasPayload(recordSet.Status.Provider) { - return RecordSetChange{}, false, r.setRecordSetProgrammed(ctx, recordSet, metav1.ConditionFalse, "ProviderConflict", "same Route 53 record set exists without current RecordSet ownership") + bound := false + if hasReceipt { + bound, err = r.bindLegacyReceiptForDeletion(ctx, hostedZone, recordSet, existing, recordName, receipt) + if err != nil { + return RecordSetChange{}, false, err + } + } + if !bound { + return RecordSetChange{}, false, r.setRecordSetProgrammed(ctx, recordSet, metav1.ConditionFalse, "ProviderConflict", "same Route 53 record set exists without current RecordSet ownership") + } } return RecordSetChange{Action: RecordSetChangeActionDelete, RecordSet: existing}, true, nil } +// bindLegacyReceiptForDeletion binds a pre-upgrade receipt to a deleting claim +// when the retained desired values still match the live record. Deletion is +// the one mutation a claim's own receipt must keep authorizing after the UID +// cutover; otherwise every pre-upgrade claim would be stuck in deletion. A +// minimal cleanup item without values cannot match and stays a conflict. +func (r *ZoneReconciler) bindLegacyReceiptForDeletion(ctx context.Context, hostedZone HostedZone, recordSet *dnsv1alpha1.RecordSet, existing RecordSetResource, recordName string, receipt dnsv1alpha1.ZoneUnitRecordSetStatus) (bool, error) { + options, err := route53RecordSetOptions(recordSet) + if err != nil { + return false, nil + } + desired := desiredRoute53RecordSet(recordSet, hostedZone.ID, recordName, options) + if !legacyReceiptBindsRecordSet(receipt, desired, existing) { + return false, nil + } + if err := r.patchRoute53RecordSetStatus(ctx, recordSet, func(data *route53v1alpha1.Route53RecordSetStatusData) { + setRoute53RecordSetStatusData(data, desired) + }); err != nil { + return false, err + } + r.recordEvent(recordSet, corev1.EventTypeNormal, "Route53RecordSetLegacyOwnershipBound", fmt.Sprintf("pre-upgrade Route 53 ownership receipt was bound to deleting RecordSet %s for type=%s name=%s", recordSet.UID, recordSet.Spec.Type, recordSet.Spec.Name)) + return true, nil +} + func (r *ZoneReconciler) acceptRecordSetForZone(ctx context.Context, zone *dnsv1alpha1.Zone, zoneClass *dnsv1alpha1.ZoneClass, recordSet *dnsv1alpha1.RecordSet) (bool, error) { if recordSet.Status.Zone != nil && recordSet.Status.Zone.Ref != (dnsv1alpha1.ObjectReference{Namespace: zone.Namespace, Name: zone.Name}) { return false, r.setRecordSetAccepted(ctx, recordSet, metav1.ConditionFalse, "InvalidZoneRef", "RecordSet status points to another Zone") @@ -402,14 +444,11 @@ func (r *ZoneReconciler) refreshPendingRecordSetChange(ctx context.Context, prov return ctrl.Result{}, false, nil } -func (r *ZoneReconciler) recordSetsForZone(ctx context.Context, zone *dnsv1alpha1.Zone) ([]dnsv1alpha1.RecordSet, error) { - var unit dnsv1alpha1.ZoneUnit - if err := r.Get(ctx, client.ObjectKey{Namespace: zone.Namespace, Name: zone.Name}, &unit); err != nil { - if apierrors.IsNotFound(err) { - return nil, nil - } - return nil, err - } +// zoneUnitRecordSets synthesizes the RecordSet claims from the ZoneUnit spec +// and the UID-bound ledger. It also returns the UID-less pre-upgrade receipts, +// which are deliberately not loaded into RecordSet status: they may only bind +// through legacyReceiptBindsRecordSet evidence, never as ownership by name. +func zoneUnitRecordSets(unit *dnsv1alpha1.ZoneUnit) ([]dnsv1alpha1.RecordSet, map[string]dnsv1alpha1.ZoneUnitRecordSetStatus) { statusByClaim := make(map[string]dnsv1alpha1.ZoneUnitRecordSetStatus, len(unit.Status.RecordSets)) for _, status := range unit.Status.RecordSets { if status.RecordSetUID != "" { @@ -425,9 +464,9 @@ func (r *ZoneReconciler) recordSetsForZone(ctx context.Context, zone *dnsv1alpha if !zoneUnitRecordSetStatusMatchesItem(status, item) || (status.DeletionCompleted && !item.DeletionRequested) { status = dnsv1alpha1.ZoneUnitRecordSetStatus{} } - recordSets = append(recordSets, route53RecordSetFromZoneUnitItem(&unit, item, status)) + recordSets = append(recordSets, route53RecordSetFromZoneUnitItem(unit, item, status)) } - return recordSets, nil + return recordSets, legacyRecordSetReceipts(unit) } func route53RecordSetFromZoneUnitItem(unit *dnsv1alpha1.ZoneUnit, item dnsv1alpha1.ZoneUnitRecordSetSpec, status dnsv1alpha1.ZoneUnitRecordSetStatus) dnsv1alpha1.RecordSet { diff --git a/internal/go/providers/route53/controllers/zoneunit/recordset_ledger.go b/internal/go/providers/route53/controllers/zoneunit/recordset_ledger.go new file mode 100644 index 0000000..f50addf --- /dev/null +++ b/internal/go/providers/route53/controllers/zoneunit/recordset_ledger.go @@ -0,0 +1,89 @@ +package route53 + +import ( + "context" + "slices" + + dnsv1alpha1 "github.com/appthrust/dns-api/pkg/go/api/dns/v1alpha1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// legacyRecordSetReceipts returns the ZoneUnit.status.recordSets[] entries +// written before recordSetUID existed, keyed by claim namespace/name. +// +// Such an entry is the provider's own receipt that it programmed the record for +// that claim name, but it cannot prove which claim incarnation it belongs to. +// Callers must therefore pair it with fresh live evidence before treating the +// record as owned; a completed or payload-less entry carries no receipt at all. +func legacyRecordSetReceipts(unit *dnsv1alpha1.ZoneUnit) map[string]dnsv1alpha1.ZoneUnitRecordSetStatus { + receipts := map[string]dnsv1alpha1.ZoneUnitRecordSetStatus{} + for _, status := range unit.Status.RecordSets { + if status.RecordSetUID != "" || status.DeletionCompleted || !providerStatusHasPayload(status.Provider) { + continue + } + receipts[recordSetClaimKey(status.RecordSetNamespace, status.RecordSetName)] = status + } + return receipts +} + +// legacyReceiptBindsRecordSet reports whether a UID-less receipt may be bound +// to the current claim incarnation. The receipt must name exactly the record +// identity the claim desires, and the live record must already equal the +// desired state: the migration then changes nothing in Route 53, it only +// records which incarnation owns the record. A live record that differs from +// desired stays a conflict, because a pre-upgrade receipt cannot distinguish an +// ordinary desired-value update from a recreated claim inheriting a stranger's +// record, and neither may be mutated on name/generation evidence alone. +func legacyReceiptBindsRecordSet(receipt dnsv1alpha1.ZoneUnitRecordSetStatus, desired, existing RecordSetResource) bool { + data, err := route53RecordSetStatusDataFromProvider(receipt.Provider) + if err != nil { + return false + } + return normalizeHostedZoneID(data.HostedZoneID) == normalizeHostedZoneID(desired.HostedZoneID) && + normalizeRoute53RecordName(data.RecordName) == normalizeRoute53RecordName(desired.Name) && + data.RecordType == string(desired.Type) && + route53RecordSetEqual(existing, desired) +} + +// pruneOrphanedRecordSetStatuses drops status.recordSets[] entries whose claim +// key has no spec item and which can no longer matter: cleanup completed, or +// the record they name is absent from the hosted zone. Without this the ledger +// grows with every deletion and pre-upgrade UID-less entries stay forever. +// +// An orphan whose record is still live is kept on purpose. Core can drop a +// spec item transiently (a claim rejected while its generation changed) and +// re-add it later; pruning that receipt would turn the provider's own record +// into a permanent ProviderConflict. The retained entry is also the only +// evidence of a leaked record. +// +// Returns true after a write so the caller re-observes the ZoneUnit instead of +// patching a stale snapshot. The unit is mutated only when a write is issued. +func (r *ZoneReconciler) pruneOrphanedRecordSetStatuses(ctx context.Context, unit *dnsv1alpha1.ZoneUnit, current map[string]RecordSetResource) (bool, error) { + claims := make(map[string]struct{}, len(unit.Spec.RecordSets)) + for _, item := range unit.Spec.RecordSets { + claims[recordSetClaimKey(item.RecordSetNamespace, item.RecordSetName)] = struct{}{} + } + prunable := func(status dnsv1alpha1.ZoneUnitRecordSetStatus) bool { + if _, claimed := claims[recordSetClaimKey(status.RecordSetNamespace, status.RecordSetName)]; claimed { + return false + } + if status.DeletionCompleted { + return true + } + data, err := route53RecordSetStatusDataFromProvider(status.Provider) + if err != nil || data.RecordType == "" || data.RecordName == "" { + return true + } + _, live := current[recordSetIdentity(dnsv1alpha1.RecordType(data.RecordType), data.RecordName).key()] + return !live + } + if !slices.ContainsFunc(unit.Status.RecordSets, prunable) { + return false, nil + } + base := unit.DeepCopy() + unit.Status.RecordSets = slices.DeleteFunc(unit.Status.RecordSets, prunable) + if err := r.Status().Patch(ctx, unit, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})); err != nil { + return false, client.IgnoreNotFound(err) + } + return true, nil +} diff --git a/internal/go/providers/route53/controllers/zoneunit/recordset_ledger_test.go b/internal/go/providers/route53/controllers/zoneunit/recordset_ledger_test.go new file mode 100644 index 0000000..00a645b --- /dev/null +++ b/internal/go/providers/route53/controllers/zoneunit/recordset_ledger_test.go @@ -0,0 +1,260 @@ +package route53 + +import ( + "context" + "testing" + + dnsv1alpha1 "github.com/appthrust/dns-api/pkg/go/api/dns/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// legacyLedgerFixture models a ZoneUnit whose ledger entry for the claim was +// written before recordSetUID existed, with the provider-created record live. +type legacyLedgerFixture struct { + recordSet *dnsv1alpha1.RecordSet + unit *dnsv1alpha1.ZoneUnit + provider *fakeProvider + client client.Client + recorder *capturingRecorder +} + +func newLegacyLedgerFixture(t *testing.T, recordSet *dnsv1alpha1.RecordSet, mutateReceipt func(*dnsv1alpha1.ZoneUnitRecordSetStatus), liveValues []string) legacyLedgerFixture { + t.Helper() + recordSet.UID = types.UID("11111111-2222-3333-4444-000000000001") + unit := route53ZoneUnit("app", "apps-example-com", "app", "route53-public") + unit.Generation = 1 + unit.Spec.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetSpec{route53ZoneUnitRecordSetSpec(recordSet)} + setRoute53ReadyZoneUnitStatus(t, unit) + legacy := route53RecordSetStatusWithState(t, recordSet, "www.apps.example.com.") + legacy.RecordSetUID = "" + if mutateReceipt != nil { + mutateReceipt(&legacy) + } + unit.Status.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetStatus{legacy} + ttl := int64(300) + provider := newFakeProvider() + provider.zones["Z000001"] = HostedZone{ + ID: "Z000001", + Name: "apps.example.com", + CallerReference: "dns-api:11111111-2222-3333-4444-555555555555", + } + provider.records[recordKey("Z000001", "www.apps.example.com.", dnsv1alpha1.RecordTypeA)] = RecordSetResource{ + HostedZoneID: "Z000001", + Name: "www.apps.example.com.", + Type: dnsv1alpha1.RecordTypeA, + TTL: &ttl, + Values: liveValues, + } + k8sClient := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(route53Provider(), route53ZoneClass("app", "route53-public", nil), acceptedRoute53Identity("app", "route53-dev"), unit). + WithStatusSubresource(&dnsv1alpha1.ZoneUnit{}). + Build() + return legacyLedgerFixture{recordSet: recordSet, unit: unit, provider: provider, client: k8sClient, recorder: &capturingRecorder{}} +} + +func (f legacyLedgerFixture) reconcile(t *testing.T, ctx context.Context) ctrl.Result { + t.Helper() + reconciler := &ZoneReconciler{Client: f.client, Provider: f.provider, Recorder: f.recorder} + result, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(f.unit)}) + if err != nil { + t.Fatalf("Reconcile returned error: %v", err) + } + return result +} + +func (f legacyLedgerFixture) ledger(t *testing.T, ctx context.Context) []dnsv1alpha1.ZoneUnitRecordSetStatus { + t.Helper() + var got dnsv1alpha1.ZoneUnit + if err := f.client.Get(ctx, client.ObjectKeyFromObject(f.unit), &got); err != nil { + t.Fatal(err) + } + return got.Status.RecordSets +} + +func TestZoneReconcileBindsLegacyReceiptWhenLiveRecordMatchesDesired(t *testing.T) { + ctx := t.Context() + fixture := newLegacyLedgerFixture(t, route53ARecordSet("app", "www"), nil, []string{"192.0.2.10"}) + + fixture.reconcile(t, ctx) + + if len(fixture.provider.upserted) != 0 || len(fixture.provider.deletedRRs) != 0 { + t.Fatalf("binding a matching legacy receipt mutated Route 53: upserts=%#v deletes=%#v", fixture.provider.upserted, fixture.provider.deletedRRs) + } + ledger := fixture.ledger(t, ctx) + if len(ledger) != 1 { + t.Fatalf("legacy entry was appended instead of bound in place: %#v", ledger) + } + status := ledger[0] + if status.RecordSetUID != fixture.recordSet.UID { + t.Fatalf("ledger UID = %q, want current %q", status.RecordSetUID, fixture.recordSet.UID) + } + data, err := route53RecordSetStatusDataFromProvider(status.Provider) + if err != nil { + t.Fatal(err) + } + if data.HostedZoneID != "Z000001" || data.RecordName != "www.apps.example.com." || data.RecordType != "A" { + t.Fatalf("bound provider state = %#v, want the desired record identity", data) + } + assertCondition(t, status.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionTrue, "Programmed") + if !fixture.recorder.has("RecordSet", "Route53RecordSetLegacyOwnershipBound") { + t.Fatalf("binding was not recorded as an event: %#v", fixture.recorder.events) + } +} + +func TestZoneReconcileRejectsLegacyReceiptWithoutEvidence(t *testing.T) { + cases := []struct { + name string + mutateReceipt func(*dnsv1alpha1.ZoneUnitRecordSetStatus) + liveValues []string + }{ + { + name: "live record differs from desired", + liveValues: []string{"192.0.2.99"}, + }, + { + name: "receipt names another record identity", + mutateReceipt: func(status *dnsv1alpha1.ZoneUnitRecordSetStatus) { + other := route53RecordSetStatusWithState(t, route53ARecordSet("app", "www"), "other.apps.example.com.") + status.Provider = other.Provider + }, + liveValues: []string{"192.0.2.10"}, + }, + { + name: "receipt already completed deletion", + mutateReceipt: func(status *dnsv1alpha1.ZoneUnitRecordSetStatus) { + status.DeletionCompleted = true + }, + liveValues: []string{"192.0.2.10"}, + }, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + ctx := t.Context() + fixture := newLegacyLedgerFixture(t, route53ARecordSet("app", "www"), testCase.mutateReceipt, testCase.liveValues) + + fixture.reconcile(t, ctx) + + if len(fixture.provider.upserted) != 0 || len(fixture.provider.deletedRRs) != 0 { + t.Fatalf("unbound legacy receipt authorized a provider mutation: upserts=%#v deletes=%#v", fixture.provider.upserted, fixture.provider.deletedRRs) + } + ledger := fixture.ledger(t, ctx) + if len(ledger) != 1 { + t.Fatalf("ledger = %#v, want one entry for the claim", ledger) + } + if ledger[0].RecordSetUID != fixture.recordSet.UID { + t.Fatalf("ledger UID = %q, want current %q", ledger[0].RecordSetUID, fixture.recordSet.UID) + } + if ledger[0].Provider != nil { + t.Fatalf("legacy receipt was rebound without evidence: %#v", ledger[0].Provider) + } + assertCondition(t, ledger[0].Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionFalse, "ProviderConflict") + }) + } +} + +func TestZoneReconcileDeletesRecordThroughLegacyReceipt(t *testing.T) { + ctx := t.Context() + fixture := newLegacyLedgerFixture(t, deletingRoute53ARecordSet(t), nil, []string{"192.0.2.10"}) + + fixture.reconcile(t, ctx) + + if len(fixture.provider.deletedRRs) != 1 { + t.Fatalf("deleting claim with a matching legacy receipt did not delete its record: %#v", fixture.provider.deletedRRs) + } + ledger := fixture.ledger(t, ctx) + if len(ledger) != 1 || ledger[0].RecordSetUID != fixture.recordSet.UID { + t.Fatalf("ledger = %#v, want the legacy entry bound to the deleting claim", ledger) + } + data, err := route53RecordSetStatusDataFromProvider(ledger[0].Provider) + if err != nil { + t.Fatal(err) + } + if data.RecordName != "www.apps.example.com." { + t.Fatalf("bound provider state = %#v", data) + } +} + +func TestZoneReconcileDoesNotDeleteRecordThroughLegacyReceiptWhenLiveDiffers(t *testing.T) { + ctx := t.Context() + fixture := newLegacyLedgerFixture(t, deletingRoute53ARecordSet(t), nil, []string{"192.0.2.99"}) + + fixture.reconcile(t, ctx) + + if len(fixture.provider.deletedRRs) != 0 { + t.Fatalf("legacy receipt deleted a record that differs from the claim: %#v", fixture.provider.deletedRRs) + } + ledger := fixture.ledger(t, ctx) + if len(ledger) != 1 || ledger[0].Provider != nil || ledger[0].DeletionCompleted { + t.Fatalf("ledger = %#v, want an unbound, incomplete entry", ledger) + } + assertCondition(t, ledger[0].Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionFalse, "ProviderConflict") +} + +func TestZoneReconcilePrunesLedgerEntriesWithoutSpecItem(t *testing.T) { + ctx := t.Context() + recordSet := route53ARecordSet("app", "www") + unit := route53ZoneUnit("app", "apps-example-com", "app", "route53-public") + unit.Generation = 1 + unit.Spec.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetSpec{route53ZoneUnitRecordSetSpec(recordSet)} + setRoute53ReadyZoneUnitStatus(t, unit) + current := route53RecordSetStatusWithState(t, recordSet, "www.apps.example.com.") + absentOrphan := route53RecordSetStatusWithState(t, route53ARecordSet("app", "gone-absent"), "gone-absent.apps.example.com.") + absentOrphan.RecordSetUID = "" + completedOrphan := route53RecordSetStatusWithState(t, route53ARecordSet("app", "gone-completed"), "gone-completed.apps.example.com.") + completedOrphan.DeletionCompleted = true + leakedOrphan := route53RecordSetStatusWithState(t, route53ARecordSet("app", "gone-leaked"), "leaked.apps.example.com.") + leakedOrphan.RecordSetUID = "" + unit.Status.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetStatus{absentOrphan, current, completedOrphan, leakedOrphan} + ttl := int64(300) + provider := newFakeProvider() + provider.zones["Z000001"] = HostedZone{ + ID: "Z000001", + Name: "apps.example.com", + CallerReference: "dns-api:11111111-2222-3333-4444-555555555555", + } + provider.records[recordKey("Z000001", "www.apps.example.com.", dnsv1alpha1.RecordTypeA)] = RecordSetResource{ + HostedZoneID: "Z000001", Name: "www.apps.example.com.", Type: dnsv1alpha1.RecordTypeA, TTL: &ttl, Values: []string{"192.0.2.10"}, + } + provider.records[recordKey("Z000001", "leaked.apps.example.com.", dnsv1alpha1.RecordTypeA)] = RecordSetResource{ + HostedZoneID: "Z000001", Name: "leaked.apps.example.com.", Type: dnsv1alpha1.RecordTypeA, TTL: &ttl, Values: []string{"192.0.2.20"}, + } + k8sClient := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(route53Provider(), route53ZoneClass("app", "route53-public", nil), acceptedRoute53Identity("app", "route53-dev"), unit). + WithStatusSubresource(&dnsv1alpha1.ZoneUnit{}). + Build() + reconciler := &ZoneReconciler{Client: k8sClient, Provider: provider} + + result, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(unit)}) + if err != nil { + t.Fatalf("Reconcile returned error: %v", err) + } + if !result.Requeue { + t.Fatalf("prune did not request re-observation: %#v", result) + } + if len(provider.upserted) != 0 || len(provider.deletedRRs) != 0 { + t.Fatalf("prune pass mutated Route 53: upserts=%#v deletes=%#v", provider.upserted, provider.deletedRRs) + } + var got dnsv1alpha1.ZoneUnit + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(unit), &got); err != nil { + t.Fatal(err) + } + names := make([]string, 0, len(got.Status.RecordSets)) + for _, status := range got.Status.RecordSets { + names = append(names, status.RecordSetName) + } + // The orphan whose record is still live is the only evidence of a leaked + // record and may still bind a returning spec item; it survives the prune. + if len(names) != 2 || names[0] != recordSet.Name || names[1] != "gone-leaked" { + t.Fatalf("ledger after prune = %#v, want the claimed entry and the leaked-record orphan", names) + } + if got.Status.RecordSets[0].Provider == nil || got.Status.RecordSets[0].RecordSetUID != recordSet.UID { + t.Fatalf("prune altered the claimed entry: %#v", got.Status.RecordSets[0]) + } + assertCondition(t, got.Status.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionTrue, "Programmed") +} diff --git a/internal/go/providers/route53/controllers/zoneunit/recordset_model.go b/internal/go/providers/route53/controllers/zoneunit/recordset_model.go index 4a727dc..1a71bf8 100644 --- a/internal/go/providers/route53/controllers/zoneunit/recordset_model.go +++ b/internal/go/providers/route53/controllers/zoneunit/recordset_model.go @@ -400,16 +400,20 @@ func route53RecordSetManagedResourceMismatch(recordSet *dnsv1alpha1.RecordSet, s } func route53RecordSetStatusData(recordSet *dnsv1alpha1.RecordSet) (route53v1alpha1.Route53RecordSetStatusData, error) { - if recordSet.Status.Provider != nil && len(recordSet.Status.Provider.State.Raw) > 0 { + return route53RecordSetStatusDataFromProvider(recordSet.Status.Provider) +} + +func route53RecordSetStatusDataFromProvider(provider *dnsv1alpha1.ProviderStatus) (route53v1alpha1.Route53RecordSetStatusData, error) { + if provider != nil && len(provider.State.Raw) > 0 { var data route53v1alpha1.Route53RecordSetStatusData - if err := json.Unmarshal(recordSet.Status.Provider.State.Raw, &data); err != nil { + if err := json.Unmarshal(provider.State.Raw, &data); err != nil { return route53v1alpha1.Route53RecordSetStatusData{}, fmt.Errorf("RecordSet status.provider.state must match Route 53 schema: %w", err) } return data, nil } - if recordSet.Status.Provider != nil && len(recordSet.Status.Provider.Data.Raw) > 0 { + if provider != nil && len(provider.Data.Raw) > 0 { var data route53v1alpha1.Route53RecordSetStatusData - if err := json.Unmarshal(recordSet.Status.Provider.Data.Raw, &data); err != nil { + if err := json.Unmarshal(provider.Data.Raw, &data); err != nil { return route53v1alpha1.Route53RecordSetStatusData{}, fmt.Errorf("RecordSet status.provider.data must match Route 53 schema: %w", err) } return data, nil diff --git a/internal/go/providers/route53/controllers/zoneunit/status_recovery_test.go b/internal/go/providers/route53/controllers/zoneunit/status_recovery_test.go index 709dab9..40bd091 100644 --- a/internal/go/providers/route53/controllers/zoneunit/status_recovery_test.go +++ b/internal/go/providers/route53/controllers/zoneunit/status_recovery_test.go @@ -387,7 +387,7 @@ func TestRecordSetDeleteConflictDoesNotCompleteDeletion(t *testing.T) { source := route53RecordSetFromZoneUnitItem(unit, item, unit.Status.RecordSets[0]) conflictingItem := item conflictingItem.Type = dnsv1alpha1.RecordTypeCNAME - if _, planned, err := reconciler.planRecordSetDelete(ctx, &source, newZoneUnitRecordSetOwnership([]dnsv1alpha1.ZoneUnitRecordSetSpec{conflictingItem}), RecordSetResource{}); err != nil { + if _, planned, err := reconciler.planRecordSetDelete(ctx, HostedZone{ID: "Z000001"}, &source, newZoneUnitRecordSetOwnership([]dnsv1alpha1.ZoneUnitRecordSetSpec{conflictingItem}), RecordSetResource{}, "www.apps.example.com.", dnsv1alpha1.ZoneUnitRecordSetStatus{}, false); err != nil { t.Fatal(err) } else if planned { t.Fatal("delete was planned despite conflicting owner") @@ -453,61 +453,60 @@ func TestZoneReconcileBindsRecordSetReceiptAndPendingChangeToCurrentUID(t *testi } } +// A receipt bound to a different incarnation never transfers, even when the +// live record equals desired. The UID-less pre-upgrade case is covered by +// recordset_ledger_test.go, where evidence may bind it. func TestZoneReconcileDoesNotUseNoncurrentRecordSetReceipt(t *testing.T) { - for _, priorUID := range []types.UID{"", "11111111-2222-3333-4444-000000000002"} { - t.Run(string(priorUID), func(t *testing.T) { - ctx := t.Context() - recordSet := route53ARecordSet("app", "www") - recordSet.UID = types.UID("11111111-2222-3333-4444-000000000001") - unit := route53ZoneUnit("app", "apps-example-com", "app", "route53-public") - unit.Generation = 1 - unit.Spec.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetSpec{route53ZoneUnitRecordSetSpec(recordSet)} - setRoute53ReadyZoneUnitStatus(t, unit) - legacy := route53RecordSetStatusWithState(t, recordSet, "www.apps.example.com.") - legacy.RecordSetUID = priorUID - unit.Status.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetStatus{legacy} - ttl := int64(300) - provider := newFakeProvider() - provider.zones["Z000001"] = HostedZone{ - ID: "Z000001", - Name: "apps.example.com", - CallerReference: "dns-api:11111111-2222-3333-4444-555555555555", - } - provider.records[recordKey("Z000001", "www.apps.example.com.", dnsv1alpha1.RecordTypeA)] = RecordSetResource{ - HostedZoneID: "Z000001", - Name: "www.apps.example.com.", - Type: dnsv1alpha1.RecordTypeA, - TTL: &ttl, - Values: []string{"192.0.2.10"}, - } - k8sClient := fake.NewClientBuilder(). - WithScheme(testScheme(t)). - WithObjects(route53Provider(), route53ZoneClass("app", "route53-public", nil), acceptedRoute53Identity("app", "route53-dev"), unit). - WithStatusSubresource(&dnsv1alpha1.ZoneUnit{}). - Build() - reconciler := &ZoneReconciler{Client: k8sClient, Provider: provider} + ctx := t.Context() + recordSet := route53ARecordSet("app", "www") + recordSet.UID = types.UID("11111111-2222-3333-4444-000000000001") + unit := route53ZoneUnit("app", "apps-example-com", "app", "route53-public") + unit.Generation = 1 + unit.Spec.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetSpec{route53ZoneUnitRecordSetSpec(recordSet)} + setRoute53ReadyZoneUnitStatus(t, unit) + legacy := route53RecordSetStatusWithState(t, recordSet, "www.apps.example.com.") + legacy.RecordSetUID = types.UID("11111111-2222-3333-4444-000000000002") + unit.Status.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetStatus{legacy} + ttl := int64(300) + provider := newFakeProvider() + provider.zones["Z000001"] = HostedZone{ + ID: "Z000001", + Name: "apps.example.com", + CallerReference: "dns-api:11111111-2222-3333-4444-555555555555", + } + provider.records[recordKey("Z000001", "www.apps.example.com.", dnsv1alpha1.RecordTypeA)] = RecordSetResource{ + HostedZoneID: "Z000001", + Name: "www.apps.example.com.", + Type: dnsv1alpha1.RecordTypeA, + TTL: &ttl, + Values: []string{"192.0.2.10"}, + } + k8sClient := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(route53Provider(), route53ZoneClass("app", "route53-public", nil), acceptedRoute53Identity("app", "route53-dev"), unit). + WithStatusSubresource(&dnsv1alpha1.ZoneUnit{}). + Build() + reconciler := &ZoneReconciler{Client: k8sClient, Provider: provider} - if _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(unit)}); err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - if len(provider.upserted) != 0 || len(provider.deletedRRs) != 0 { - t.Fatalf("noncurrent receipt authorized a provider mutation: upserts=%#v deletes=%#v", provider.upserted, provider.deletedRRs) - } + if _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(unit)}); err != nil { + t.Fatalf("Reconcile returned error: %v", err) + } + if len(provider.upserted) != 0 || len(provider.deletedRRs) != 0 { + t.Fatalf("noncurrent receipt authorized a provider mutation: upserts=%#v deletes=%#v", provider.upserted, provider.deletedRRs) + } - var got dnsv1alpha1.ZoneUnit - if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(unit), &got); err != nil { - t.Fatal(err) - } - status := zoneUnitRecordSetStatus(t, &got, recordSet.Namespace, recordSet.Name) - if status.RecordSetUID != recordSet.UID { - t.Fatalf("recordSet status UID = %q, want current %q", status.RecordSetUID, recordSet.UID) - } - if status.Provider != nil { - t.Fatalf("legacy provider receipt was rebound to current claim: %#v", status.Provider) - } - assertCondition(t, status.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionFalse, "ProviderConflict") - }) + var got dnsv1alpha1.ZoneUnit + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(unit), &got); err != nil { + t.Fatal(err) } + status := zoneUnitRecordSetStatus(t, &got, recordSet.Namespace, recordSet.Name) + if status.RecordSetUID != recordSet.UID { + t.Fatalf("recordSet status UID = %q, want current %q", status.RecordSetUID, recordSet.UID) + } + if status.Provider != nil { + t.Fatalf("legacy provider receipt was rebound to current claim: %#v", status.Provider) + } + assertCondition(t, status.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionFalse, "ProviderConflict") } func TestZoneReconcileDoesNotAttestCurrentClaimFromNoncurrentPendingChange(t *testing.T) { diff --git a/internal/go/providers/route53/controllers/zoneunit/zone_recordset_behavior_test.go b/internal/go/providers/route53/controllers/zoneunit/zone_recordset_behavior_test.go index 2e3a92d..9d19282 100644 --- a/internal/go/providers/route53/controllers/zoneunit/zone_recordset_behavior_test.go +++ b/internal/go/providers/route53/controllers/zoneunit/zone_recordset_behavior_test.go @@ -583,10 +583,6 @@ func TestZoneReconcilerDeletesRecordSetFromSpecIdentity(t *testing.T) { if _, err := reconcileRoute53AndProject(t, ctx, k8sClient, reconciler, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "app", Name: "apps-example-com"}}); err != nil { t.Fatalf("second Reconcile returned error: %v", err) } - if _, err := reconcileRoute53AndProject(t, ctx, k8sClient, reconciler, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "app", Name: "apps-example-com"}}); err != nil { - t.Fatalf("third Reconcile returned error: %v", err) - } - var unit dnsv1alpha1.ZoneUnit if err := k8sClient.Get(ctx, client.ObjectKey{Namespace: "app", Name: "apps-example-com"}, &unit); err != nil { t.Fatalf("ZoneUnit was not found: %v", err) @@ -599,9 +595,23 @@ func TestZoneReconcilerDeletesRecordSetFromSpecIdentity(t *testing.T) { unit.Status.RecordSets[statusIndex].RecordSetUID != recordSet.UID { t.Fatalf("ZoneUnit recordSet deletion completion = %#v, want current UID completed", unit.Status.RecordSets) } + if len(unit.Spec.RecordSets) != 0 { + t.Fatalf("core retained spec item after completed deletion: %#v", unit.Spec.RecordSets) + } + + // Once core consumed the completion and dropped the spec item, the ledger + // entry is orphaned and the provider prunes it. + if _, err := reconcileRoute53AndProject(t, ctx, k8sClient, reconciler, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "app", Name: "apps-example-com"}}); err != nil { + t.Fatalf("third Reconcile returned error: %v", err) + } + if err := k8sClient.Get(ctx, client.ObjectKey{Namespace: "app", Name: "apps-example-com"}, &unit); err != nil { + t.Fatalf("ZoneUnit was not found: %v", err) + } + if len(unit.Status.RecordSets) != 0 { + t.Fatalf("orphaned ledger entry was not pruned: %#v", unit.Status.RecordSets) + } } func TestZoneReconcilerIgnoresCompletedDeletionStatusForActiveZoneUnitItem(t *testing.T) { - ctx := context.Background() recordSet := route53ARecordSet("app", "www-a") unit := route53ZoneUnit("app", "apps-example-com", "app", "route53-public") unit.Spec.RecordSets = []dnsv1alpha1.ZoneUnitRecordSetSpec{route53ZoneUnitRecordSetSpec(recordSet)} @@ -621,16 +631,7 @@ func TestZoneReconcilerIgnoresCompletedDeletionStatusForActiveZoneUnitItem(t *te }, }, } - k8sClient := fake.NewClientBuilder(). - WithScheme(testScheme(t)). - WithObjects(unit). - Build() - reconciler := &ZoneReconciler{Client: k8sClient} - - recordSets, err := reconciler.recordSetsForZone(ctx, route53ReadyZone(t, "app", "apps-example-com")) - if err != nil { - t.Fatalf("recordSetsForZone returned error: %v", err) - } + recordSets, _ := zoneUnitRecordSets(unit) if len(recordSets) != 1 { t.Fatalf("recordSets = %#v, want one record set", recordSets) } diff --git a/pkg/go/api/dns/v1alpha1/zoneunit_types.go b/pkg/go/api/dns/v1alpha1/zoneunit_types.go index a83164a..1dc379a 100644 --- a/pkg/go/api/dns/v1alpha1/zoneunit_types.go +++ b/pkg/go/api/dns/v1alpha1/zoneunit_types.go @@ -189,8 +189,10 @@ type ZoneUnitRecordSetStatus struct { RecordSetName string `json:"recordSetName"` // RecordSetUID identifies the claim incarnation observed by the provider. - // Empty or mismatched UIDs cannot authorize cleanup or transfer provider - // ownership to a recreated claim, even when name and generation match. + // A mismatched UID cannot authorize cleanup or transfer provider ownership + // to a recreated claim, even when name and generation match. An empty UID + // is a pre-upgrade receipt: it binds to the current claim only when the + // provider observes the live record equal to the claim's desired state. // +optional // +kubebuilder:validation:Type=string // +kubebuilder:validation:MinLength=1