feat(route53-patterns): use Certificate as the default certificate (under feature flag)
#23575
Conversation
…(under feature flag) The `HttpsRedirect` construct creates a certificate using the `DnsValidatedCertificate` if a certificate is not provided by the user. As part of [deprecating]() the `DnsValidatedCertificate` we need to replace all instances of `DnsValidatedCertificate` with `Certificate`. I have placed this behind a feature flag since I think it should be opt-in behavior for existing users, _but_ it is not a breaking change because of the way CloudFormation works. If the flag is enabled on an existing `HttpsRedirect` CloudFormation will: 1. Create a new certificate using `Certificate` 2. Update CloudFront to use the newly created `Certificate` 3. Delete the old certificate created with the `DnsValidatedCertificate` I have tested this scenario using the two newly created integration tests.
|
|
| if (Token.isUnresolved(stack.region)) { | ||
| throw new Error(`When ${ROUTE53_PATTERNS_USE_CERTIFICATE} is enabled, a region must be defined on the Stack`); | ||
| } |
There was a problem hiding this comment.
whoah, what is this? In the PR description, you said that there's no breaking change, but what about scenarios where you're using DnsValidatedCertificate without a region? Wouldn't we throw an error when we move it to Certificate?
There was a problem hiding this comment.
Why is that a breaking change? We are not breaking any existing user since we'll only move to Certificate when you enable the feature flag.
There was a problem hiding this comment.
fair. not a breaking change, but I was sounding the alarm that users might move from DnsValidatedCertificate to Certificate without a defined region and would hit this error. I wonder what the story is for these people, or if that is a valid use case to begin with. Either way, its not breaking, so I'm not blocking.
There was a problem hiding this comment.
Yeah those users would have to provide a region, which I don't think is too much to ask when you are going cross region stuff. The alternative is that we make an assumption
- Assume that they are in
us-east-1and if they are not the error is thrown by CloudFormation - Assume that they are not in
us-east-1and always create a support stack. We can always switch this later if this is the direction users want us to go.
I mean we could deprecate |
|
@kaizencc ready for another review |
|
DNM because I want to call attention to this comment, but its non blocking. |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
@Mergifyio update |
❌ Base branch update has failedrefusing to allow a GitHub App to create or update workflow |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
The
HttpsRedirectconstruct creates a certificate using theDnsValidatedCertificateif a certificate is not provided by the user. As part of deprecating theDnsValidatedCertificatewe need to replace all instances ofDnsValidatedCertificatewithCertificate.I have placed this behind a feature flag since I think it should be opt-in behavior for existing users, but it is not a breaking change because of the way CloudFormation works. If the flag is enabled on an existing
HttpsRedirectCloudFormation will:CertificateCertificateDnsValidatedCertificateI have tested this scenario using the two newly created integration tests.
All Submissions:
Adding new Construct Runtime Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license