Fix ACM certificate creation for Ingresses with wildcard hosts#4786
Fix ACM certificate creation for Ingresses with wildcard hosts#4786nbr wants to merge 2 commits into
Conversation
Ingress hosts are a sorted set, so a wildcard host (e.g. *.app.example.com)
sorts first and became the certificate DomainName. DomainName is stamped into
the certificate resourceID tag, and ACM's tag-value pattern
([\p{L}\p{Z}\p{N}_.:/=+\-@]*) forbids '*', so RequestCertificate failed with a
400 ValidationException for ingresses fronted by a wildcard host.
Pick the first non-wildcard host as DomainName, leaving the wildcard in the
SAN list so coverage is unchanged. Add a unit test asserting a non-wildcard
host is chosen as DomainName.
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @nbr. 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. |
k8s-ci-robot
added
size/M
the-technat
left a comment
the-technat
left a comment
There was a problem hiding this comment.
Good catch!
I think your fix is valid. But regarding the case you mentioned with an ingress only containing wildcards, I think it could also be elegant just doing a search/replace action on the tag value. AWS tag values can be up to 255 chars long, and the hash consists of the namespace/name and hostname which all have to be max 64 characters, so replacing * with "wildcard" would be valid too.
nbr
changed the title
@the-technat Thanks for the feedback, I pivoted the PR to address the tag issue directly |
The certificate resourceID embeds the DomainName and is written as an ACM
tag, but ACM rejects '*' in tag values ([\p{L}\p{Z}\p{N}_.:/=+\-@]*), so
RequestCertificate fails for any ingress fronted by a wildcard host.
Replace '*' with 'wildcard' in the resourceID before it is used as a tag.
The DomainName and SANs sent to ACM are unchanged, so coverage is the same
and the resourceID stays unique per ingress. This also covers ingresses
whose hosts are all wildcards, which the previous non-wildcard DomainName
preference could not handle.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nbr, 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 |
3 participants
Description
The
create-acm-certannotation requests one certificate per ingress, using every host as a SAN and the first host as the Domain Name (buildCertificateSpec).*.app.example.comsorts first and becomes the Domain Name.buildCertificateResourceID, surfaced byResourceTags).*in tag values ([\p{L}\p{Z}\p{N}_.:/=+\-@]*), soRequestCertificatefails for any ingress with a wildcard host:Note this is only the tag value. ACM accepts a wildcard Domain Name on the certificate itself.
Fix
*withwildcardin the resource ID before it's used as a tag value.Tests
model_build_certificates_test.go: an all-wildcard ingress case, plus a test asserting the resource ID contains no*.Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯