fix(acm): Allow tokens as a part of the hosted zone name #6685
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Thanks for the contribution @rhermes62 ! I have 2 small comments.
| @@ -109,7 +109,11 @@ export class DnsValidatedCertificate extends cdk.Resource implements ICertificat | |||
| protected validate(): string[] { | |||
| const errors: string[] = []; | |||
| // Ensure the zone name is a parent zone of the certificate domain name | |||
| if (this.domainName !== this.normalizedZoneName && !this.domainName.endsWith('.' + this.normalizedZoneName)) { | |||
| if ( | |||
There was a problem hiding this comment.
This indentation looks a tad weird to me... how about this?
if (!cdk.Token.isUnresolved(this.normalizedZoneName) &&
this.domainName !== this.normalizedZoneName &&
!this.domainName.endsWith('.' + this.normalizedZoneName)) {
errors.push(`DNS zone ${this.normalizedZoneName} is not authoritative for certificate domain name ${this.domainName}`);
}There was a problem hiding this comment.
Code in the current revision is still the same though 🙂
| hostedZone: helloDotComZone | ||
| }); | ||
|
|
||
| // a bit of a hack: expect(stack) will trigger validation. |
There was a problem hiding this comment.
Nice hack with expect, but there's a simpler way :). We have a SynthUtils class with a synthesize method that we use for this kind of situations, so the test becomes:
'does not try to validate unresolved tokens'(test: Test) {
const stack = new Stack();
const helloDotComZone = new PublicHostedZone(stack, 'HelloDotCom', {
zoneName: Token.asString('hello.com')
});
new DnsValidatedCertificate(stack, 'Cert', {
domainName: 'hello.com',
hostedZone: helloDotComZone
});
test.doesNotThrow(() => {
SynthUtils.synthesize(stack);
});
test.done();
},There was a problem hiding this comment.
Perfect! Ill also update test above this that uses that hack (I didn't come up with it but just went along with it haha).
Do you think it's also worth allowing the domainName property in the DnsValidatedCertificate to be a token as well? (For me specifically, I only have/need the zoneName to be a Token but I can see others having the domainName in the DnsValidatedCertificate be a token.
There was a problem hiding this comment.
I think it's fine to leave it as it is right now - we can always change it if it's needed.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
This comment has still not been addressed, in spite of being Resolved 🙂.
This review adds a basic check to not try and validate unresolved tokens when performing validation for the `DnsValidatedCertificate` construct. fixes #6133
Just updated! Should show correctly now |
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This review adds a basic check to not try and validate unresolved tokens when performing validation for the
DnsValidatedCertificateconstruct.fixes #6133
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license