Repository navigation
Address review findings for managed ingress DNS - #1
Merged
Merged
Conversation
The control-plane-operator policy granted hosted-zone create/delete/tag and record access on all hosted zones for every CLI-created cluster, regardless of whether managed ingress DNS was requested. Gate the wider permissions on the --managed-dns option, mirroring the ingress operator policy. Without it, record access is scoped to the cluster's local zone again and shared-VPC clusters get no Route53 access, matching the behavior before managed DNS was introduced. The delegating client keeps the managed-DNS superset of CPO Route53 APIs, so the generated client is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CreatePrivateHostedZone reused the first private zone matching the requested name, without checking its VPC association. Route53 allows same-name private zones on other VPCs, so a cluster could adopt, write records into, and on teardown drain and delete another owner's zone. Look up candidate zones by name, then use GetHostedZone to return only the one associated with the cluster VPC and region, both before creating and after a create conflict. Route53 does not allow a VPC to be associated with two private zones of the same name, so at most one zone can match, and a concurrent create resolves to the same zone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Changing spec.platform.aws.managedDNS.ingressDomainPrefix after the zones were created was accepted by the API but could not converge: the controller kept the old zone, relabeled it in status with the new name and switched the apps domain to a name the zone does not serve. Make ingressDomainPrefix immutable, and add a feature-gate-aware rule on AWSPlatformSpec so managedDNS cannot be added to or removed from an existing cluster, which would otherwise bypass the immutability. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the review of openshift#9348 (managed ingress DNS). One commit per finding.
Finding 1 (P1): CPO IAM policy scoped unless managed DNS is enabled
controlPlaneOperatorPolicynow takes the local zone and the--managed-dnsoption, mirroringingressPermPolicy.IAM is written by the CLI when the cluster is created and is not reconciled from the spec. This is the same convention as the KMS key policy; a mismatch fails with AccessDenied, surfaced on the
AWSManagedDNSAvailablecondition. The delegating client is still generated from the managed-DNS superset, sodelegating_client.gois unchanged.Finding 2 (P1): private zones adopted only if associated with the cluster VPC
CreatePrivateHostedZoneused to reuse the first private zone with a matching name. It now lists the same-name private zones, callsGetHostedZoneon each, and returns only the one associated with the cluster VPC and region. This applies both before creating and after a create conflict. Route53 does not allow a VPC to be associated with two private zones of the same name, so at most one zone can match, and a concurrent create (ConflictingDomainExists) resolves to the same zone. No new IAM permissions are needed.This matches the existing ownership model:
destroy infradeletes every private zone on the cluster VPC, and the existing by-name lookups increate infraand the CPO have no VPC check. Public ingress zones are still adopted by name. Their name is specific to the cluster ({prefix}.{clusterBaseDomain}), and clusters sharing a base domain are unsupported regardless, since their apps/API names would also collide.Finding 3 (P1): not addressed
Managed DNS clusters publish the KAS as a Route (LoadBalancer is legacy on AWS), so there is a single
private-routerAWSEndpointService. Endpoint deletion therefore only happens at teardown or when switching to Public, and in both cases nothing else uses the local zone. Moving zone ownership to the HCP finalizer is left as a follow-up.Finding 4 (P2): ingress prefix immutable
ingressDomainPrefix:self == oldSelf.AWSPlatformSpec: a rule tied to theAWSManagedDNSgate,has(oldSelf.managedDNS) == has(self.managedDNS), so the prefix cannot be changed by removing and re-addingmanagedDNS. It follows the existing EtcdShardingshardsrule. As a result, managed DNS can only be chosen when the cluster is created.Testing
go testforcmd/infra/aws,support/awsutil,support/globalconfig,controllers/awsprivatelinkandcontrollers/hostedcontrolplane.TestControlPlaneOperatorPolicy(sharedVPC × managedDNS).TestCreatePrivateHostedZonecases: zone on another VPC, same VPC ID in another region, several same-name zones, a candidate deleted during lookup, aGetHostedZoneerror, a concurrent create resolved throughConflictingDomainExists, and a conflict with no zone on our VPC.managedDNSadd/remove, and an allowed delegation change, on both TechPreview and Custom.make verify-quick(generate+update): no further diff after regenerating the aggregated docs.make api-lintpasses.make verify(staticcheck, lint, CRD schema check), e2e, or anything against live AWS.🤖 Generated with Claude Code