From a4a410353ee7b237cb025d382afa66d192559ebc Mon Sep 17 00:00:00 2001 From: reoring Date: Sat, 10 Oct 2026 23:35:03 +0900 Subject: [PATCH] feat(route53): adopt an existing hosted zone by name (Zone.spec.adoption.byName) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Route 53 Zone adoption becomes a oneOf: {hostedZoneId: Z...} (unchanged) or {byName: true}. By-name resolution lists same-name hosted zones, keeps those whose private/public type matches the ZoneClass zoneType, requires exactly one match, and records its ID in status through the existing adoption patch; later reconciles and deletion use the recorded ID and never re-list. Zero matches -> Programmed=False ExternalResourceNotFound; several -> ExternalResourceMismatch with the count; both fields set -> Accepted=False InvalidAdoption. No implicit adoption without spec.adoption; record-set adoption, Cloudflare and VPC-association behaviour unchanged. Motivation: appthrust/platform hosting-substrate-design.md S2 — the hosting bundle renders Zone hcp for hcp..internal under a Private class; the hosted zone ID lives only in Terraform state and the installation spec is immutable, so adoption by name removes the need to thread the ID. --- .../config/provider/route53_extensions.yaml | 12 +- .../dns-api/templates/provider-route53.yaml | 12 +- docs/design/route53-provider.md | 24 +- internal/go/core/webhook/validator_test.go | 83 +++++- .../route53/controllers/zoneunit/zone.go | 51 +++- .../zoneunit/zone_adoption_test.go | 246 ++++++++++++++++++ .../controllers/zoneunit/zone_status.go | 45 ++-- .../zoneunit/zone_test_fixtures_test.go | 10 +- 8 files changed, 441 insertions(+), 42 deletions(-) create mode 100644 internal/go/providers/route53/controllers/zoneunit/zone_adoption_test.go diff --git a/app/operator/config/provider/route53_extensions.yaml b/app/operator/config/provider/route53_extensions.yaml index c4002e4..efe3f20 100644 --- a/app/operator/config/provider/route53_extensions.yaml +++ b/app/operator/config/provider/route53_extensions.yaml @@ -60,14 +60,22 @@ spec: adoption: openAPIV3Schema: type: object - required: - - hostedZoneId + oneOf: + - required: + - hostedZoneId + - required: + - byName additionalProperties: false properties: hostedZoneId: type: string description: Route 53 hosted zone ID of the existing hosted zone to adopt. The zone must be public or private as declared by the ZoneClass zoneType. pattern: '^Z[A-Z0-9]+$' + byName: + type: boolean + description: Adopt exactly one existing hosted zone matching domainName and the ZoneClass zoneType. The resolved hosted zone ID is recorded in status and reused without further name lookups. + enum: + - true statusProviderData: openAPIV3Schema: type: object diff --git a/deploy/charts/dns-api/templates/provider-route53.yaml b/deploy/charts/dns-api/templates/provider-route53.yaml index 5b6e83f..5b4c567 100644 --- a/deploy/charts/dns-api/templates/provider-route53.yaml +++ b/deploy/charts/dns-api/templates/provider-route53.yaml @@ -61,14 +61,22 @@ spec: adoption: openAPIV3Schema: type: object - required: - - hostedZoneId + oneOf: + - required: + - hostedZoneId + - required: + - byName additionalProperties: false properties: hostedZoneId: type: string description: Route 53 hosted zone ID of the existing hosted zone to adopt. The zone must be public or private as declared by the ZoneClass zoneType. pattern: '^Z[A-Z0-9]+$' + byName: + type: boolean + description: Adopt exactly one existing hosted zone matching domainName and the ZoneClass zoneType. The resolved hosted zone ID is recorded in status and reused without further name lookups. + enum: + - true statusProviderData: openAPIV3Schema: type: object diff --git a/docs/design/route53-provider.md b/docs/design/route53-provider.md index 91f289e..f2a7c1d 100644 --- a/docs/design/route53-provider.md +++ b/docs/design/route53-provider.md @@ -134,7 +134,7 @@ AWS credentials themselves are not stored in `ZoneClass.spec.parameters`. `ZoneC ### Private hosted zones -With `zoneType: Private`, dns-api adopts an existing private hosted zone through `Zone.spec.adoption.hostedZoneId` and manages its RecordSets. dns-api never creates or deletes private hosted zones and never manages their VPC associations. Create the private hosted zone and configure its VPC associations outside dns-api before adoption. +With `zoneType: Private`, dns-api adopts an existing private hosted zone through `Zone.spec.adoption.hostedZoneId` or `Zone.spec.adoption.byName: true` and manages its RecordSets. dns-api never creates or deletes private hosted zones and never manages their VPC associations. Create the private hosted zone and configure its VPC associations outside dns-api before adoption. The ZoneClass accepts `zoneCreationPolicy` only when absent or `Deny`, and `zoneDeletionPolicy` only when absent or `Retain`. Otherwise it sets `Accepted=False`, reason `InvalidParameters`, with `zoneType Private requires zoneCreationPolicy Deny` or `zoneType Private requires zoneDeletionPolicy Retain`. `sameNameZonePolicy` and `tags` are allowed, but apply only to hosted zone creation and do not cause private zone creation or tagging during adoption. @@ -166,7 +166,7 @@ When `Zone.spec.adoption` is absent, the controller creates public hosted zones Route 53 controller stores `CreateHostedZone` `CallerReference` only as provider-internal state in `ZoneUnit.status.provider.state`. It is not stored in `ZoneUnit.spec` and is not public `status.provider.data`. The controller may reuse a cached caller reference while status is present, but the caller reference is not the recovery source of truth. -If Route 53 status is lost after a hosted zone was created, recovery uses explicit adoption through `Zone.spec.adoption.hostedZoneId` / `ZoneUnit.spec.zone.adoption.hostedZoneId`. If status and adoption are both absent, the controller does not automatically adopt a same-name hosted zone. It reports `Programmed=False`, reason `ProviderConflict`, when provider state is ambiguous. +If Route 53 status is lost after a hosted zone was created, recovery uses explicit adoption through `Zone.spec.adoption` / `ZoneUnit.spec.zone.adoption`, either by hosted zone ID or by name. If status and adoption are both absent, the controller does not automatically adopt a same-name hosted zone. It reports `Programmed=False`, reason `ProviderConflict`, when provider state is ambiguous. Temporary Route 53 ZoneUnit controller pseudocode is not part of the design source of truth. Hosted zone resolution order, finalizers, pending changes, Route 53 API errors, batch limits, and requeue behavior are defined in this document. @@ -174,7 +174,7 @@ Route 53 controller re-observes hosted zones and child record sets even in stabl Route 53 controller also enqueues a `ZoneUnit` when `ZoneUnit.metadata.annotations["dns.appthrust.io/reconcile-request"]` changes. This requests re-observation of the hosted zone and child record sets without waiting for periodic re-observation. It follows the same validation, pending change, batch serialization, and drift self-healing rules. Updating the annotation alone must not set `Programmed=False` or send no-op provider API changes when external state already matches `ZoneUnit.spec`. -Route 53 `Zone.spec.adoption` shape: +Route 53 `Zone.spec.adoption` accepts exactly one of these shapes: ```yaml spec: @@ -182,15 +182,29 @@ spec: hostedZoneId: Z10219583PPQMV0U1KGN2 ``` -`hostedZoneId` is a Route 53 hosted zone ID in `Z...` form. `/hostedzone/Z...` and hosted zone ARNs are not accepted. This shape is validated by Route 53 Provider version `zone.schemas.adoption`. +```yaml +spec: + domainName: hcp.example.internal + adoption: + byName: true +``` + +`hostedZoneId` is a Route 53 hosted zone ID in `Z...` form. `/hostedzone/Z...` and hosted zone ARNs are not accepted. Alternatively, `byName` must be the boolean `true`; `false`, an empty adoption object, or both fields together are invalid. These mutually exclusive shapes are validated by the `oneOf` in Route 53 Provider version `zone.schemas.adoption`. Invalid adoption sets `Accepted=False`, reason `InvalidAdoption`. + +By-name adoption avoids passing an externally managed hosted zone ID (for example, an ID held only in Terraform state) through immutable installation configuration. It is explicit opt-in, not implicit same-name recovery. Route 53 `Provider` schema descriptions for `zone.schemas.adoption`: | Field | `description` | | --- | --- | | `hostedZoneId` | `Route 53 hosted zone ID of the existing hosted zone to adopt. The zone must be public or private as declared by the ZoneClass zoneType.` | +| `byName` | `Adopt exactly one existing hosted zone matching domainName and the ZoneClass zoneType. The resolved hosted zone ID is recorded in status and reused without further name lookups.` | + +For `byName: true` without a recorded hosted zone ID, the controller calls `ListHostedZonesByName` with `domainName` and retains only exact same-name zones after trailing-dot normalization whose public/private type matches the ZoneClass `zoneType` (`Public` by default). **By-name adoption requires exactly one same-name zone of the class's type.** A public and a private zone with the same name are not ambiguous when only one matches the class. Zero matches set `Programmed=False`, reason `ExternalResourceNotFound`. Multiple matches set `Programmed=False`, reason `ExternalResourceMismatch`; the message reports the matching count, never the candidate IDs. + +For either adoption mode, the Route 53 controller gets the selected hosted zone, verifies its ID, verifies that the hosted zone name matches `ZoneUnit.spec.zone.domainName` after trailing-dot normalization, and verifies that its public/private type matches the ZoneClass `zoneType`. If it matches, the controller manages its RecordSets and stores the resolved ID in `ZoneUnit.status.zone.provider.data.hostedZoneID` and the observed type in `zoneType`, using the same status patch for both modes. If the hosted zone does not exist, set `Programmed=False`, reason `ExternalResourceNotFound`. If the ID, type, or name differs, set `Programmed=False`, reason `ExternalResourceMismatch`. A Public class adopting a private zone by ID keeps the message `Route 53 hosted zone is private; only public hosted zones are supported`. A Private class adopting a public zone by ID reports `Route 53 hosted zone is public; ZoneClass zoneType is Private`. -When adoption is specified, the Route 53 controller gets the hosted zone, verifies that the hosted zone name matches `ZoneUnit.spec.zone.domainName`, and verifies that its public/private type matches the ZoneClass `zoneType`. If it matches, the controller manages its RecordSets and stores `hostedZoneId` in `ZoneUnit.status.zone.provider.data.hostedZoneID` and the observed type in `zoneType`. If the hosted zone does not exist, set `Programmed=False`, reason `ExternalResourceNotFound`. If the type or name differs, set `Programmed=False`, reason `ExternalResourceMismatch`. A Public class adopting a private zone keeps the message `Route 53 hosted zone is private; only public hosted zones are supported`. A Private class adopting a public zone reports `Route 53 hosted zone is public; ZoneClass zoneType is Private`. +Once recorded, the status hosted zone ID is authoritative for subsequent reconciles and deletion. By-name adoption does not list again or switch to another same-name zone, even if the recorded zone disappears; that case reports `ExternalResourceNotFound`. An explicit `hostedZoneId` differing from recorded status still sets `Accepted=False`, reason `ManagedResourceMismatch`. Deletion never resolves a previously unresolved by-name adoption: without a recorded ID there is no managed hosted zone to delete. Private zones are always retained; public zones follow the class deletion policy. Created hosted zones receive ownership tags: diff --git a/internal/go/core/webhook/validator_test.go b/internal/go/core/webhook/validator_test.go index 83bd9fb..4f2ebe5 100644 --- a/internal/go/core/webhook/validator_test.go +++ b/internal/go/core/webhook/validator_test.go @@ -1,12 +1,14 @@ package webhook import ( + "bytes" "context" "encoding/json" "os" "path/filepath" "strings" "testing" + "text/template" cloudflarev1alpha1 "github.com/appthrust/dns-api/pkg/go/api/cloudflare/v1alpha1" dnsv1alpha1 "github.com/appthrust/dns-api/pkg/go/api/dns/v1alpha1" @@ -733,6 +735,75 @@ func TestZoneValidationChecksAdoptionSchema(t *testing.T) { } } +func TestRoute53ZoneAdoptionSchemas(t *testing.T) { + builtIn := builtInProviderManifest(t, "route53_extensions.yaml") + path := filepath.Join("..", "..", "..", "..", "deploy", "charts", "dns-api", "templates", "provider-route53.yaml") + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile(%s): %v", path, err) + } + tmpl, err := template.New("provider-route53").Parse(string(data)) + if err != nil { + t.Fatalf("parse chart provider template: %v", err) + } + var rendered bytes.Buffer + if err := tmpl.Execute(&rendered, map[string]any{ + "Values": map[string]any{"providers": map[string]any{"route53": map[string]any{"enabled": true}}}, + }); err != nil { + t.Fatalf("render chart provider template: %v", err) + } + var chart dnsv1alpha1.Provider + if err := yaml.Unmarshal(rendered.Bytes(), &chart); err != nil { + t.Fatalf("unmarshal chart provider: %v", err) + } + if !bytes.Equal(builtIn.Spec.Versions[0].Zone.Schemas.Adoption.OpenAPIV3Schema.Raw, chart.Spec.Versions[0].Zone.Schemas.Adoption.OpenAPIV3Schema.Raw) { + t.Fatal("built-in and chart adoption schemas differ") + } + for name, provider := range map[string]*dnsv1alpha1.Provider{"built-in": &builtIn, "chart": &chart} { + t.Run(name, func(t *testing.T) { + if err := validateProvider(provider); err != nil { + t.Fatalf("validateProvider: %v", err) + } + for _, test := range []struct { + name string + adoption string + valid bool + }{ + {name: "absent", valid: true}, + {name: "hosted zone ID", adoption: `{"hostedZoneId":"ZADOPT"}`, valid: true}, + {name: "by name", adoption: `{"byName":true}`, valid: true}, + {name: "both", adoption: `{"hostedZoneId":"ZADOPT","byName":true}`}, + {name: "both with false", adoption: `{"hostedZoneId":"ZADOPT","byName":false}`}, + {name: "both with null", adoption: `{"hostedZoneId":"ZADOPT","byName":null}`}, + {name: "empty", adoption: `{}`}, + {name: "false", adoption: `{"byName":false}`}, + {name: "string", adoption: `{"byName":"true"}`}, + {name: "null by name", adoption: `{"byName":null}`}, + {name: "unknown field", adoption: `{"byName":true,"other":true}`}, + {name: "path ID", adoption: `{"hostedZoneId":"/hostedzone/ZADOPT"}`}, + {name: "empty ID", adoption: `{"hostedZoneId":""}`}, + } { + t.Run(test.name, func(t *testing.T) { + zone := validZone() + zone.Spec.Adoption = runtime.RawExtension{Raw: []byte(test.adoption)} + validator := newTestValidator(t, + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "app"}}, + provider, route53ZoneClass(t), zone, + ) + err := validator.validateZone(context.Background(), zone, nil) + if test.valid { + if err != nil { + t.Fatalf("validateZone: %v", err) + } + } else if err == nil || !strings.Contains(err.Error(), "spec.adoption") { + t.Fatalf("validateZone error = %v, want adoption schema failure", err) + } + }) + } + }) + } +} + func TestAllowedRecordSetsRequireSelectorRecordsAndTypes(t *testing.T) { err := validateAllowedRecordSets([]dnsv1alpha1.AllowedRecordSet{ { @@ -1933,17 +2004,7 @@ func route53Provider(t *testing.T) *dnsv1alpha1.Provider { }, Zone: dnsv1alpha1.ProviderZone{ Schemas: dnsv1alpha1.ProviderZoneSchemas{ - Adoption: &dnsv1alpha1.ProviderOpenAPISchema{OpenAPIV3Schema: raw(t, map[string]any{ - "type": "object", - "required": []any{"hostedZoneId"}, - "properties": map[string]any{ - "hostedZoneId": map[string]any{ - "type": "string", - "pattern": "^Z[A-Z0-9]+$", - "description": "Route 53 public hosted zone ID of the existing hosted zone to adopt.", - }, - }, - })}, + Adoption: builtInProviderManifest(t, "route53_extensions.yaml").Spec.Versions[0].Zone.Schemas.Adoption, StatusProviderData: &dnsv1alpha1.ProviderOpenAPISchema{OpenAPIV3Schema: raw(t, map[string]any{ "type": "object", "properties": map[string]any{ diff --git a/internal/go/providers/route53/controllers/zoneunit/zone.go b/internal/go/providers/route53/controllers/zoneunit/zone.go index 1c549b6..f5d4f3d 100644 --- a/internal/go/providers/route53/controllers/zoneunit/zone.go +++ b/internal/go/providers/route53/controllers/zoneunit/zone.go @@ -122,7 +122,7 @@ func (r *ZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. return r.createHostedZone(ctx, provider, &zone, zoneClass, params) } - adoptionID, adopting, err := route53ZoneAdoptionID(&zone) + adoption, adopting, err := parseRoute53ZoneAdoption(&zone) if err != nil { return ctrl.Result{}, r.setAccepted(ctx, &zone, metav1.ConditionFalse, "InvalidAdoption", err.Error()) } @@ -134,8 +134,12 @@ func (r *ZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. return r.failProgrammedForProviderError(ctx, &zone, err) } if adopting { - if hostedZone.ID != adoptionID { - return ctrl.Result{}, r.setProgrammed(ctx, &zone, metav1.ConditionFalse, "ExternalResourceMismatch", "adopted Route 53 hosted zone ID does not match spec.adoption.hostedZoneId") + if hostedZone.ID != selection.ID { + message := "adopted Route 53 hosted zone ID does not match spec.adoption.hostedZoneId" + if adoption.ByName { + message = "adopted Route 53 hosted zone ID does not match resolved adoption target" + } + return ctrl.Result{}, r.setProgrammed(ctx, &zone, metav1.ConditionFalse, "ExternalResourceMismatch", message) } if message := hostedZoneSpecMismatch(&zone, params, hostedZone); message != "" { return ctrl.Result{}, r.setProgrammed(ctx, &zone, metav1.ConditionFalse, "ExternalResourceMismatch", message) @@ -358,7 +362,7 @@ func (r *ZoneReconciler) acceptZone(ctx context.Context, zone *dnsv1alpha1.Zone) if err != nil || !accepted { return nil, nil, nil, false, err } - _, adopting, err := route53ZoneAdoptionID(zone) + _, adopting, err := parseRoute53ZoneAdoption(zone) if err != nil { return nil, nil, nil, false, r.setAccepted(ctx, zone, metav1.ConditionFalse, "InvalidAdoption", err.Error()) } @@ -579,10 +583,16 @@ func (r *ZoneReconciler) deleteContextForZone(ctx context.Context, zone *dnsv1al } func (r *ZoneReconciler) selectHostedZone(ctx context.Context, provider Provider, zone *dnsv1alpha1.Zone, statusData route53v1alpha1.Route53ZoneStatusData, params *route53v1alpha1.Route53ZoneClassParameters) (HostedZone, bool, ctrl.Result, error) { - if adoptionID, adopting, err := route53ZoneAdoptionID(zone); err != nil { + if adoption, adopting, err := parseRoute53ZoneAdoption(zone); err != nil { return HostedZone{}, true, ctrl.Result{}, r.setAccepted(ctx, zone, metav1.ConditionFalse, "InvalidAdoption", err.Error()) } else if adopting { - return HostedZone{ID: adoptionID}, false, ctrl.Result{}, nil + if statusID := normalizeHostedZoneID(statusData.HostedZoneID); statusID != "" { + return HostedZone{ID: statusID}, false, ctrl.Result{}, nil + } + if !adoption.ByName { + return HostedZone{ID: adoption.HostedZoneID}, false, ctrl.Result{}, nil + } + return r.selectHostedZoneByName(ctx, provider, zone, params) } statusID := normalizeHostedZoneID(statusData.HostedZoneID) @@ -635,6 +645,35 @@ func (r *ZoneReconciler) selectHostedZone(ctx context.Context, provider Provider return selected, false, ctrl.Result{}, nil } +func (r *ZoneReconciler) selectHostedZoneByName(ctx context.Context, provider Provider, zone *dnsv1alpha1.Zone, params *route53v1alpha1.Route53ZoneClassParameters) (HostedZone, bool, ctrl.Result, error) { + domainName := normalizeDomainName(zone.Spec.DomainName) + zones, err := provider.ListHostedZonesByName(ctx, domainName) + if err != nil { + result, err := r.failProgrammedForProviderError(ctx, zone, err) + return HostedZone{}, true, result, err + } + + wantType := zoneType(params) + var selected HostedZone + matches := 0 + for _, hostedZone := range zones { + if normalizeDomainName(hostedZone.Name) == domainName && hostedZone.Private == (wantType == route53v1alpha1.ZoneTypePrivate) { + selected = hostedZone + matches++ + } + } + switch matches { + case 0: + message := fmt.Sprintf("no same-name Route 53 hosted zone matches ZoneClass zoneType %s", wantType) + return HostedZone{}, true, ctrl.Result{}, r.setProgrammed(ctx, zone, metav1.ConditionFalse, "ExternalResourceNotFound", message) + case 1: + return selected, false, ctrl.Result{}, nil + default: + message := fmt.Sprintf("%d same-name Route 53 hosted zones match ZoneClass zoneType %s; adoption.byName requires exactly one", matches, wantType) + return HostedZone{}, true, ctrl.Result{}, r.setProgrammed(ctx, zone, metav1.ConditionFalse, "ExternalResourceMismatch", message) + } +} + func (r *ZoneReconciler) clearHostedZoneProviderStatus(ctx context.Context, zone *dnsv1alpha1.Zone) error { statusData, err := route53ZoneStatusData(zone) if err != nil { diff --git a/internal/go/providers/route53/controllers/zoneunit/zone_adoption_test.go b/internal/go/providers/route53/controllers/zoneunit/zone_adoption_test.go new file mode 100644 index 0000000..cdd96fc --- /dev/null +++ b/internal/go/providers/route53/controllers/zoneunit/zone_adoption_test.go @@ -0,0 +1,246 @@ +package route53 + +import ( + "context" + "slices" + "testing" + + dnsv1alpha1 "github.com/appthrust/dns-api/pkg/go/api/dns/v1alpha1" + route53v1alpha1 "github.com/appthrust/dns-api/pkg/go/api/route53/v1alpha1" + "github.com/aws/smithy-go" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestZoneReconcilerAdoptsHostedZoneByName(t *testing.T) { + private := HostedZone{ID: "ZPRIVATE", Name: "apps.example.com.", Private: true} + public := HostedZone{ID: "ZPUBLIC", Name: "apps.example.com", NameServers: []string{"ns-1.awsdns.example"}} + for _, test := range []struct { + name string + parameters string + zones []HostedZone + listErr error + wantID string + wantType route53v1alpha1.ZoneType + reason string + message string + }{ + {name: "private single match", parameters: `{"zoneType":"Private"}`, zones: []HostedZone{private}, wantID: "ZPRIVATE", wantType: route53v1alpha1.ZoneTypePrivate}, + {name: "default public single match", parameters: `{}`, zones: []HostedZone{public}, wantID: "ZPUBLIC", wantType: route53v1alpha1.ZoneTypePublic}, + {name: "private filters public", parameters: `{"zoneType":"Private"}`, zones: []HostedZone{private, public}, wantID: "ZPRIVATE", wantType: route53v1alpha1.ZoneTypePrivate}, + {name: "public filters private", parameters: `{"zoneType":"Public"}`, zones: []HostedZone{private, public}, wantID: "ZPUBLIC", wantType: route53v1alpha1.ZoneTypePublic}, + {name: "filters other names", parameters: `{"zoneType":"Private"}`, zones: []HostedZone{private, {ID: "ZOTHER", Name: "other.example.com", Private: true}}, wantID: "ZPRIVATE", wantType: route53v1alpha1.ZoneTypePrivate}, + {name: "no zones", parameters: `{"zoneType":"Private"}`, reason: "ExternalResourceNotFound", message: "no same-name Route 53 hosted zone matches ZoneClass zoneType Private"}, + {name: "only wrong type", parameters: `{"zoneType":"Private"}`, zones: []HostedZone{public}, reason: "ExternalResourceNotFound", message: "no same-name Route 53 hosted zone matches ZoneClass zoneType Private"}, + {name: "only wrong name", parameters: `{"zoneType":"Private"}`, zones: []HostedZone{{ID: "ZOTHER", Name: "other.example.com", Private: true}}, reason: "ExternalResourceNotFound", message: "no same-name Route 53 hosted zone matches ZoneClass zoneType Private"}, + {name: "two private matches", parameters: `{"zoneType":"Private"}`, zones: []HostedZone{private, {ID: "ZDUPLICATE", Name: "apps.example.com", Private: true}}, reason: "ExternalResourceMismatch", message: "2 same-name Route 53 hosted zones match ZoneClass zoneType Private; adoption.byName requires exactly one"}, + {name: "two public matches", parameters: `{}`, zones: []HostedZone{public, {ID: "ZDUPLICATE", Name: "apps.example.com."}}, reason: "ExternalResourceMismatch", message: "2 same-name Route 53 hosted zones match ZoneClass zoneType Public; adoption.byName requires exactly one"}, + {name: "listing access denied", parameters: `{"zoneType":"Private"}`, listErr: &smithy.GenericAPIError{Code: "AccessDenied", Message: "denied"}, reason: "ProviderAccessDenied"}, + } { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + provider := newFakeProvider() + for _, zone := range test.zones { + provider.zones[zone.ID] = zone + } + provider.listHostedZonesByName = func(string) ([]HostedZone, error) { return test.zones, test.listErr } + class := route53ZoneClass("app", "route53", nil) + class.Spec.Parameters = runtime.RawExtension{Raw: []byte(test.parameters)} + unit := route53ZoneUnit("app", "apps-example-com", "app", class.Name) + unit.Spec.Zone.Adoption = runtime.RawExtension{Raw: []byte(`{"byName":true}`)} + k8sClient := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(route53Provider(), class, acceptedRoute53Identity("app", "route53-dev"), unit). + WithStatusSubresource(&dnsv1alpha1.ZoneUnit{}).Build() + reconciler := &ZoneReconciler{Client: k8sClient, Provider: provider} + request := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(unit)} + if _, err := reconciler.Reconcile(ctx, request); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if err := k8sClient.Get(ctx, request.NamespacedName, unit); err != nil { + t.Fatalf("Get ZoneUnit: %v", err) + } + if unit.Status.Zone == nil { + t.Fatal("status.zone was nil") + } + assertCondition(t, unit.Status.Zone.Conditions, string(dnsv1alpha1.ConditionAccepted), metav1.ConditionTrue, "Accepted") + if !slices.Equal(provider.listed, []string{"apps.example.com"}) { + t.Fatalf("ListHostedZonesByName calls = %v, want one domain lookup", provider.listed) + } + if len(provider.created) != 0 || len(provider.deleted) != 0 { + t.Fatalf("unexpected hosted zone mutation: created=%v deleted=%v", provider.created, provider.deleted) + } + if test.reason != "" { + condition := assertCondition(t, unit.Status.Zone.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionFalse, test.reason) + if test.message != "" && condition.Message != test.message { + t.Fatalf("message = %q, want %q", condition.Message, test.message) + } + if len(provider.fetched) != 0 || len(provider.tags) != 0 { + t.Fatalf("unresolved adoption fetched or tagged zones: fetched=%v tags=%v", provider.fetched, provider.tags) + } + state := mustZoneUnitStatusData(t, ctx, k8sClient, unit.Namespace, unit.Name) + if state.HostedZoneID != "" { + t.Fatalf("unresolved adoption recorded hosted zone ID %q", state.HostedZoneID) + } + return + } + assertCondition(t, unit.Status.Zone.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionTrue, "Programmed") + if unit.Status.Zone.Provider == nil { + t.Fatal("status.zone.provider was nil") + } + assertRoute53RawEqual(t, unit.Status.Zone.Provider.Data, map[string]any{"hostedZoneID": test.wantID, "zoneType": string(test.wantType)}) + if !slices.Equal(unit.Status.Zone.NameServers, provider.zones[test.wantID].NameServers) { + t.Fatalf("nameServers = %v", unit.Status.Zone.NameServers) + } + + // Newly ambiguous discovery must not retarget a recorded adoption. + provider.listHostedZonesByName = func(string) ([]HostedZone, error) { + t.Fatal("recorded adoption must not list hosted zones again") + return nil, nil + } + if _, err := reconciler.Reconcile(ctx, request); err != nil { + t.Fatalf("second Reconcile: %v", err) + } + if !slices.Equal(provider.fetched, []string{test.wantID, test.wantID}) { + t.Fatalf("GetHostedZone calls = %v, want recorded ID twice", provider.fetched) + } + if err := k8sClient.Get(ctx, request.NamespacedName, unit); err != nil { + t.Fatalf("Get ZoneUnit: %v", err) + } + assertCondition(t, unit.Status.Zone.Conditions, string(dnsv1alpha1.ConditionAccepted), metav1.ConditionTrue, "Accepted") + assertCondition(t, unit.Status.Zone.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionTrue, "Programmed") + + // Losing the recorded resource must not adopt another same-name zone. + delete(provider.zones, test.wantID) + if _, err := reconciler.Reconcile(ctx, request); err != nil { + t.Fatalf("missing recorded zone Reconcile: %v", err) + } + if err := k8sClient.Get(ctx, request.NamespacedName, unit); err != nil { + t.Fatalf("Get ZoneUnit: %v", err) + } + assertCondition(t, unit.Status.Zone.Conditions, string(dnsv1alpha1.ConditionProgrammed), metav1.ConditionFalse, "ExternalResourceNotFound") + state := mustZoneUnitStatusData(t, ctx, k8sClient, unit.Namespace, unit.Name) + if state.HostedZoneID != test.wantID { + t.Fatalf("recorded ID = %q, want %q", state.HostedZoneID, test.wantID) + } + }) + } +} + +func TestZoneReconcilerRejectsInvalidAdoption(t *testing.T) { + for _, adoption := range []string{ + `{}`, `[]`, `{"byName":false}`, `{"byName":"true"}`, `{"byName":null}`, + `{"hostedZoneId":"ZID","byName":true}`, `{"hostedZoneId":"ZID","byName":false}`, + `{"hostedZoneId":"ZID","byName":null}`, `{"hostedZoneId":null,"byName":true}`, + `{"hostedZoneId":"/hostedzone/ZID"}`, `{"hostedZoneId":""}`, + } { + t.Run(adoption, func(t *testing.T) { + ctx := context.Background() + unit := route53ZoneUnit("app", "apps-example-com", "app", "route53") + unit.Spec.Zone.Adoption = runtime.RawExtension{Raw: []byte(adoption)} + k8sClient := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(unit). + WithStatusSubresource(&dnsv1alpha1.ZoneUnit{}).Build() + provider := newFakeProvider() + reconciler := &ZoneReconciler{Client: k8sClient, Provider: provider} + request := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(unit)} + if _, err := reconciler.Reconcile(ctx, request); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if err := k8sClient.Get(ctx, request.NamespacedName, unit); err != nil { + t.Fatalf("Get ZoneUnit: %v", err) + } + if unit.Status.Zone == nil { + t.Fatal("status.zone was nil") + } + assertCondition(t, unit.Status.Zone.Conditions, string(dnsv1alpha1.ConditionAccepted), metav1.ConditionFalse, "InvalidAdoption") + if len(provider.listed) != 0 || len(provider.fetched) != 0 || len(provider.created) != 0 { + t.Fatal("invalid adoption reached provider") + } + }) + } +} + +func TestZoneReconcilerRetainsByNameAdoptedPrivateZone(t *testing.T) { + for _, parameters := range []string{`{"zoneType":"Private"}`, `{"zoneDeletionPolicy":"Delete"}`} { + t.Run(parameters, func(t *testing.T) { + ctx := context.Background() + provider := newFakeProvider() + provider.zones["ZPRIVATE"] = HostedZone{ID: "ZPRIVATE", Name: "apps.example.com", Private: true} + class := route53ZoneClass("app", "route53", nil) + class.Spec.Parameters = runtime.RawExtension{Raw: []byte(`{"zoneType":"Private"}`)} + unit := route53ZoneUnit("app", "apps-example-com", "app", class.Name) + unit.Spec.Zone.Adoption = runtime.RawExtension{Raw: []byte(`{"byName":true}`)} + k8sClient := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(route53Provider(), class, acceptedRoute53Identity("app", "route53-dev"), unit). + WithStatusSubresource(&dnsv1alpha1.ZoneUnit{}).Build() + recorder := &capturingRecorder{} + reconciler := &ZoneReconciler{Client: k8sClient, Provider: provider, Recorder: recorder} + request := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(unit)} + if _, err := reconciler.Reconcile(ctx, request); err != nil { + t.Fatalf("adoption Reconcile: %v", err) + } + state := mustZoneUnitStatusData(t, ctx, k8sClient, unit.Namespace, unit.Name) + if state.HostedZoneID != "ZPRIVATE" { + t.Fatalf("recorded ID = %q, want ZPRIVATE", state.HostedZoneID) + } + if err := k8sClient.Get(ctx, request.NamespacedName, unit); err != nil { + t.Fatalf("Get ZoneUnit: %v", err) + } + unit.Finalizers = append(unit.Finalizers, ZoneFinalizer) + if err := k8sClient.Update(ctx, unit); err != nil { + t.Fatalf("add deletion finalizer: %v", err) + } + class.Spec.Parameters = runtime.RawExtension{Raw: []byte(parameters)} + if err := k8sClient.Update(ctx, class); err != nil { + t.Fatalf("update deletion policy: %v", err) + } + if err := k8sClient.Delete(ctx, unit); err != nil { + t.Fatalf("delete ZoneUnit: %v", err) + } + if _, err := reconciler.Reconcile(ctx, request); err != nil { + t.Fatalf("deletion Reconcile: %v", err) + } + if len(provider.deleted) != 0 || len(provider.listed) != 1 { + t.Fatalf("deletion calls=%v lookup calls=%v", provider.deleted, provider.listed) + } + if _, exists := provider.zones["ZPRIVATE"]; !exists { + t.Fatal("private hosted zone was deleted") + } + if !recorder.has("Zone", "ExternalResourceRetained") { + t.Fatalf("events = %v, want ExternalResourceRetained", recorder.events) + } + if err := k8sClient.Get(ctx, request.NamespacedName, unit); err != nil { + if !apierrors.IsNotFound(err) { + t.Fatalf("Get deleted ZoneUnit: %v", err) + } + } else if slices.Contains(unit.Finalizers, ZoneFinalizer) { + t.Fatalf("provider deletion finalizer remains: %v", unit.Finalizers) + } + }) + } +} + +func TestHostedZoneIDForDeleteAdoption(t *testing.T) { + for _, test := range []struct { + name string + adoption string + statusID string + wantID string + }{ + {name: "by name uses recorded ID", adoption: `{"byName":true}`, statusID: "ZRECORDED", wantID: "ZRECORDED"}, + {name: "unresolved by name has no delete target", adoption: `{"byName":true}`}, + {name: "explicit ID without status", adoption: `{"hostedZoneId":"ZEXPLICIT"}`, wantID: "ZEXPLICIT"}, + {name: "status wins over explicit ID", adoption: `{"hostedZoneId":"ZEXPLICIT"}`, statusID: "ZRECORDED", wantID: "ZRECORDED"}, + } { + t.Run(test.name, func(t *testing.T) { + zone := &dnsv1alpha1.Zone{Spec: dnsv1alpha1.ZoneSpec{Adoption: runtime.RawExtension{Raw: []byte(test.adoption)}}} + id, err := hostedZoneIDForDelete(zone, route53v1alpha1.Route53ZoneStatusData{HostedZoneID: test.statusID}) + if err != nil || id != test.wantID { + t.Fatalf("hostedZoneIDForDelete = (%q, %v), want (%q, nil)", id, err, test.wantID) + } + }) + } +} diff --git a/internal/go/providers/route53/controllers/zoneunit/zone_status.go b/internal/go/providers/route53/controllers/zoneunit/zone_status.go index 8ed6e2f..0937a38 100644 --- a/internal/go/providers/route53/controllers/zoneunit/zone_status.go +++ b/internal/go/providers/route53/controllers/zoneunit/zone_status.go @@ -428,30 +428,44 @@ func callerReferenceForZoneState(zone *dnsv1alpha1.Zone, statusData route53v1alp return callerReferenceForZone(zone) } -func route53ZoneAdoptionID(zone *dnsv1alpha1.Zone) (string, bool, error) { +type route53ZoneAdoption struct { + HostedZoneID string + ByName bool +} + +func parseRoute53ZoneAdoption(zone *dnsv1alpha1.Zone) (route53ZoneAdoption, bool, error) { if len(zone.Spec.Adoption.Raw) == 0 { - return "", false, nil + return route53ZoneAdoption{}, false, nil } - var adoption struct { - HostedZoneID string `json:"hostedZoneId"` + var payload struct { + HostedZoneID json.RawMessage `json:"hostedZoneId"` + ByName json.RawMessage `json:"byName"` } - if err := json.Unmarshal(zone.Spec.Adoption.Raw, &adoption); err != nil { - return "", true, fmt.Errorf("adoption must be an object with hostedZoneId: %w", err) + if err := json.Unmarshal(zone.Spec.Adoption.Raw, &payload); err != nil { + return route53ZoneAdoption{}, true, fmt.Errorf("adoption must be an object with hostedZoneId or byName: %w", err) } - if !isRoute53HostedZoneExternalRefID(adoption.HostedZoneID) { - return "", true, errors.New("adoption.hostedZoneId must be a Route 53 hosted zone ID in Z... form") + if (len(payload.HostedZoneID) > 0) == (len(payload.ByName) > 0) { + return route53ZoneAdoption{}, true, errors.New("adoption must specify exactly one of hostedZoneId or byName") + } + var adoption route53ZoneAdoption + if len(payload.ByName) > 0 { + if err := json.Unmarshal(payload.ByName, &adoption.ByName); err != nil || !adoption.ByName { + return route53ZoneAdoption{}, true, errors.New("adoption.byName must be true") + } + } else if err := json.Unmarshal(payload.HostedZoneID, &adoption.HostedZoneID); err != nil || !isRoute53HostedZoneExternalRefID(adoption.HostedZoneID) { + return route53ZoneAdoption{}, true, errors.New("adoption.hostedZoneId must be a Route 53 hosted zone ID in Z... form") } - return adoption.HostedZoneID, true, nil + return adoption, true, nil } func route53ZoneManagedResourceMismatch(zone *dnsv1alpha1.Zone, statusData route53v1alpha1.Route53ZoneStatusData) (string, bool, error) { - adoptionID, adopting, err := route53ZoneAdoptionID(zone) - if err != nil || !adopting { + adoption, adopting, err := parseRoute53ZoneAdoption(zone) + if err != nil || !adopting || adoption.ByName { return "", false, err } statusID := normalizeHostedZoneID(statusData.HostedZoneID) - if statusID == "" || statusID == adoptionID { + if statusID == "" || statusID == adoption.HostedZoneID { return "", false, nil } return "spec.adoption points to a different Route 53 hosted zone than the managed resource recorded in status", true, nil @@ -461,10 +475,11 @@ func hostedZoneIDForDelete(zone *dnsv1alpha1.Zone, statusData route53v1alpha1.Ro if hostedZoneID := normalizeHostedZoneID(statusData.HostedZoneID); hostedZoneID != "" { return hostedZoneID, nil } - if adoptionID, adopting, err := route53ZoneAdoptionID(zone); err != nil { + if adoption, adopting, err := parseRoute53ZoneAdoption(zone); err != nil { return "", err } else if adopting { - return adoptionID, nil + // Unresolved by-name adoption has no managed resource to delete. + return adoption.HostedZoneID, nil } return "", nil } @@ -491,7 +506,7 @@ func setRoute53ZoneStatusData(data *route53v1alpha1.Route53ZoneStatusData, hoste } func hostedZoneSpecMismatch(zone *dnsv1alpha1.Zone, params *route53v1alpha1.Route53ZoneClassParameters, hostedZone HostedZone) string { - if hostedZone.Name != zone.Spec.DomainName { + if normalizeDomainName(hostedZone.Name) != normalizeDomainName(zone.Spec.DomainName) { return "Route 53 hosted zone name does not match Zone domainName" } if hostedZone.Private != (zoneType(params) == route53v1alpha1.ZoneTypePrivate) { diff --git a/internal/go/providers/route53/controllers/zoneunit/zone_test_fixtures_test.go b/internal/go/providers/route53/controllers/zoneunit/zone_test_fixtures_test.go index fc9c468..5cf9dd1 100644 --- a/internal/go/providers/route53/controllers/zoneunit/zone_test_fixtures_test.go +++ b/internal/go/providers/route53/controllers/zoneunit/zone_test_fixtures_test.go @@ -549,8 +549,11 @@ type fakeProvider struct { deleted []string upserted []RecordSetResource deletedRRs []RecordSetResource + listed []string + fetched []string - changeRecordSetsErr func(hostedZoneID string, changes []RecordSetChange) error + changeRecordSetsErr func(hostedZoneID string, changes []RecordSetChange) error + listHostedZonesByName func(domainName string) ([]HostedZone, error) } func newFakeProvider() *fakeProvider { @@ -562,6 +565,7 @@ func newFakeProvider() *fakeProvider { } } func (p *fakeProvider) GetHostedZone(_ context.Context, id string) (HostedZone, error) { + p.fetched = append(p.fetched, id) zone, ok := p.zones[normalizeHostedZoneID(id)] if !ok { return HostedZone{}, &smithy.GenericAPIError{ @@ -572,6 +576,10 @@ func (p *fakeProvider) GetHostedZone(_ context.Context, id string) (HostedZone, return zone, nil } func (p *fakeProvider) ListHostedZonesByName(_ context.Context, domainName string) ([]HostedZone, error) { + p.listed = append(p.listed, domainName) + if p.listHostedZonesByName != nil { + return p.listHostedZonesByName(domainName) + } var zones []HostedZone for _, zone := range p.zones { if normalizeDomainName(zone.Name) == domainName {