fix: use public Route 53 zone for Amazon-issued ACM DNS validation in split-horizon DNS - #4847
fix: use public Route 53 zone for Amazon-issued ACM DNS validation in split-horizon DNS#4847niv1612 wants to merge 2 commits into
Conversation
|
|
|
Welcome @niv1612! |
|
Hi @niv1612. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
This PR fixes ACM DNS validation failures in split-horizon Route 53 setups by ensuring Amazon-issued certificate validation records are written to the nearest matching public hosted zone (instead of accidentally choosing a more-specific private zone).
Changes:
- Add
GetPublicHostedZoneIDto the Route 53 service and reuse the longest-suffix matcher while skipping private hosted zones. - Update the ACM certificate create path to pre-check/select public hosted zones only; update delete path to attempt cleanup in both legacy (most-specific) and public zones.
- Add/adjust unit tests and extend documentation to describe public-zone selection and the fail-fast behavior.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/deploy/acm/certificate_synthesizer_test.go | Updates mocks/expectations to use public-zone lookup and to reflect dual-zone cleanup behavior. |
| pkg/deploy/acm/certificate_manager.go | Uses public hosted zone selection for DNS validation pre-checks and deletes validation records from both possible zones. |
| pkg/deploy/acm/certificate_manager_test.go | Adds a split-horizon delete test ensuring cleanup is attempted in both private and public zones. |
| pkg/aws/services/route53.go | Adds GetPublicHostedZoneID and refactors matching logic to support public-only selection. |
| pkg/aws/services/route53_test.go | Adds coverage for public-only selection behavior and confirms unfiltered lookup still returns private zones. |
| pkg/aws/services/route53_mocks.go | Extends Route 53 mock with GetPublicHostedZoneID. |
| docs/guide/ingress/certificate_management.md | Documents public-zone selection for Amazon-issued ACM DNS validation and the fail-fast fallback guidance. |
Files not reviewed (1)
- pkg/aws/services/route53_mocks.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
79321ee to
ad8e0d3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pkg/aws/services/route53_mocks.go: Generated file
Comments suppressed due to low confidence (1)
pkg/deploy/acm/certificate_manager.go:89
- The pre-check error message doesn’t include the actionable guidance described in the PR (fall back to pre-created cert via the certificate-arn annotation). As written, users with private-only domains or public zones in another account will see a generic pre-check failure without the recommended remediation.
zoneID, err := c.route53Client.GetPublicHostedZoneID(ctx, host)
if err != nil {
return nil, fmt.Errorf("pre-check failed for domain %q: %w", host, err)
}
The create-acm-cert (EnableCertificateManagement) feature writes the ACM DNS validation CNAME into the most-specific matching Route 53 hosted zone. In split-horizon DNS (a private zone that is a subdomain of a public zone) that most-specific zone is the private one. ACM validates Amazon-issued (public) certificates over public DNS, so the record never resolves and the certificate is stuck in PENDING_VALIDATION forever; the HTTPS listener never finalizes. Select the validation record's hosted zone from public zones only (nearest public ancestor) for Amazon-issued certificates. If no public zone matches, fail fast instead of creating a certificate that hangs in PENDING_VALIDATION. The delete path attempts cleanup in both the most-specific zone (records written by earlier controller versions) and the public zone (records written after this change), so validation records are not orphaned across the upgrade. Validation options whose resource record has not yet been populated by ACM are skipped during cleanup. Signed-off-by: niv1612 <35202955+niv1612@users.noreply.github.com>
ad8e0d3 to
71264af
Compare
Signed-off-by: niv1612 <35202955+niv1612@users.noreply.github.com>
the-technat
left a comment
There was a problem hiding this comment.
From the perspective of the original feature author I'd say this looks good and is reasonable. Over to the maintainers for the final verdict 😁.
| E-Mail validation is not supported due to significant higher delays between requesting a certificate and its issuance. | ||
| When using a PCA, certificates don't have to be validated. | ||
|
|
||
| Because ACM validates Amazon-issued certificates over **public** DNS, the controller writes the validation record into the nearest-ancestor **public** Route53 hosted zone. In split-horizon setups (a private zone that is a subdomain of a public zone), the private zone is skipped so the record lands where ACM can resolve it. If no public hosted zone matches the domain (private-only domain, or the public parent lives in an account the controller can't see), the controller fails fast. In that case, pre-create the certificate yourself and reference it with the [`certificate-arn`](annotations.md#certificate-arn) annotation. |
There was a problem hiding this comment.
Just to note: there is another FR for cross-account records: #4727, but that doesn't affect the status quo as you mentioned in the docs.
|
|
||
| for _, opts := range desc.Certificate.DomainValidationOptions { | ||
| if opts.ValidationMethod == acmtypes.ValidationMethodDns { | ||
| if opts.ResourceRecord == nil { |
There was a problem hiding this comment.
Good catch! Noticed this one too in some of my tests, should make it more clear what's going on instead of writing misleading logs.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: niv1612, the-technat The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Issue
Fixes #4840
Description
The
create-acm-cert(EnableCertificateManagement) feature writes the ACM DNS validation CNAME into the most-specific matching Route 53 hosted zone. In split-horizon DNS — a private zone that is a subdomain of a public zone — that most-specific zone is the private one. ACM validates Amazon-issued (public) certificates over public DNS, so the record never resolves, the certificate stays inPENDING_VALIDATION, and the HTTPS listener never finalizes.How it works
GetPublicHostedZoneIDto theRoute53service, reusing the existing longest-suffix matcher but skipping private zones (zone.Config.PrivateZone).CreateWithValidationRecords) now selects the validation zone from public zones only (nearest public ancestor). If no public zone matches, it fails fast with an actionable error pointing at thecertificate-arnannotation, instead of requesting a certificate that hangs inPENDING_VALIDATION.acm-pca-arn) path is unaffected — the synthesizer routes it toCreate, which never touches validation records.Unit tests cover
GetPublicHostedZoneID(split-horizon selects the public zone, private-only fails fast, longest-suffix among public zones) and the split-horizon delete cleaning records in both zones; existing synthesizer create/delete expectations are updated.docs/guide/ingress/certificate_management.mddocuments public-zone selection and the fail-fast fallback tocertificate-arn.Manually tested end-to-end against live Route 53 and ACM: split-horizon ingresses wrote the validation CNAME to the public zone and the certificate reached
ISSUED; an ingress under a private-only domain failed fast before any certificate or record was created; and deleting the ingresses removed the auto-provisioned certificate and its validation record while leaving unrelated records in the shared zone untouched.Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯