feat(aws-certificatemanager): Add ability to specify the certificate name #22301

jmortlock · 2022-09-30T06:50:36Z

Like other AWS resources (VPC, TargetGroups,etc) they do not have an actual physical name but can be assigned an AWS designated tag which will be displayed in the web console. This is useful when you have many certificates to determine what is what.

I largely followed the pattern set about in other CDK constructs, for example in vpc.ts I also followed the convention set elsewhere name tag is used by defaulting the value too this.node.path

I believe this is also an important first step towards any implementation of #10792

I tried added integration tests, however due to the requirement of needing to validate the certificate I don't think this is possible. Currently there are no other integration tests for this module.

I have attached a screenshot of the failed integ-test (validation required) which shows the name tag in action

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-09-30T06:50:50Z

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

Pull request has been modified.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

jmortlock · 2022-09-30T08:45:31Z

The build is failing due to test failure in ecs-patterns. However the integ test cannot be regenerate due to certificate validation issue. I'm not sure how the original snapshot was generated.

aws-ecs-integ-alb-fg-idletimeout | 6:02:19 pm | CREATE_FAILED        | AWS::CertificateManager::Certificate      | myService/Certificate (myServiceCertificate152F9DDA) DNS Record Set is not available. Certificate is in FAILED status.

A workaround would be to remove the default certificateName but I feel consistency with other constructs its important.

Pull request has been modified.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

jmortlock · 2022-09-30T22:59:21Z

For now I have manually updated the snapshot

Pull Request updated. Dissmissing previous PRLinter Review.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

Pull Request updated. Dissmissing previous PRLinter Review.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

corymhall

Just a couple of nits

packages/@aws-cdk/aws-certificatemanager/lib/certificate.ts

corymhall · 2022-10-03T12:46:13Z

I tried added integration tests, however due to the requirement of needing to validate the certificate I don't think this is possible. Currently there are no other integration tests for this module.

I just added an integration test for DnsValidatedCertificate. If you do have the ability to request a valid certificate then you can follow this model. Essentially You set some env vars that the runner will use when performing the deploy and then it will use some generic built in values when it saves the snapshot.

aws-cdk/packages/@aws-cdk/aws-certificatemanager/test/integ.dns-validated-certificate.ts

Lines 22 to 27 in 8b2b381

    
           const hostedZoneId = process.env.CDK_INTEG_HOSTED_ZONE_ID ?? process.env.HOSTED_ZONE_ID; 
        
           if (!hostedZoneId) throw new Error('For this test you must provide your own HostedZoneId as an env var "HOSTED_ZONE_ID"'); 
        
           const hostedZoneName = process.env.CDK_INTEG_HOSTED_ZONE_NAME ?? process.env.HOSTED_ZONE_NAME; 
        
           if (!hostedZoneName) throw new Error('For this test you must provide your own HostedZoneName as an env var "HOSTED_ZONE_NAME"'); 
        
           const domainName = process.env.CDK_INTEG_DOMAIN_NAME ?? process.env.DOMAIN_NAME; 
        
           if (!domainName) throw new Error('For this test you must provide your own Domain Name as an env var "DOMAIN_NAME"');

aws-cdk/packages/@aws-cdk/aws-certificatemanager/test/integ.dns-validated-certificate.ts

Line 49 in 8b2b381

enableLookups: true,

Pull request has been modified.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

…certificate-name

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

corymhall

Thanks for adding the integ test! Just 1 last minor comment and then I think we are good.

corymhall · 2022-10-06T12:28:12Z

packages/@aws-cdk/aws-certificatemanager/lib/certificate.ts

@@ -247,6 +261,8 @@ export class Certificate extends CertificateBase implements ICertificate {
      certificateTransparencyLoggingPreference,
    });

+    Tags.of(cert).add(NAME_TAG, props.certificateName || this.node.path);


The Tag value has a limit of 255 characters. Can you add something here to restrict the value to that? I don't think it is as much of a problem if the user provides a certificateName, but node.path might hit that limit.

Maybe just a value.slice(0, 255);

https://docs.aws.amazon.com/acm/latest/userguide/tags-restrictions.html

Nice catch; I have sliced the auto-generated certificate name but left the user inputted value alone.

Pull request has been modified.

mergify · 2022-10-07T13:08:09Z

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).

mergify · 2022-10-07T13:46:17Z

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).

aws-cdk-automation · 2022-10-07T14:58:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: c0836ee
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-10-07T14:58:23Z

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).

…name (aws#22301) ---- Like other AWS resources (VPC, TargetGroups,etc) they do not have an actual physical name but can be assigned an AWS designated tag which will be displayed in the web console. This is useful when you have many certificates to determine what is what. I largely followed the pattern set about in other CDK constructs, for example in `vpc.ts` I also followed the convention set elsewhere name tag is used by defaulting the value too `this.node.path` I believe this is also an important first step towards any implementation of aws#10792 I tried added integration tests, however due to the requirement of needing to validate the certificate I don't think this is possible. Currently there are no other integration tests for this module. I have attached a screenshot of the failed integ-test (validation required) which shows the name tag in action ![CustomCertificate](https://user-images.githubusercontent.com/10041761/193207874-871d55e6-9a8e-4e8a-aa77-ae718e4bc1d4.png) ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Add ability to specify the certificateName

86aa687

github-actions bot added repeat-contributor p2 labels Sep 30, 2022

aws-cdk-automation requested a review from a team September 30, 2022 06:50

aws-cdk-automation previously requested changes Sep 30, 2022

View reviewed changes

Add a change to the README

b440cbb

aws-cdk-automation previously requested changes Sep 30, 2022

View reviewed changes

Manudally adjust snapshots

d72beb4

aws-cdk-automation previously requested changes Sep 30, 2022

View reviewed changes

corymhall previously requested changes Oct 3, 2022

View reviewed changes

packages/@aws-cdk/aws-certificatemanager/lib/certificate.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-certificatemanager/lib/certificate.ts Outdated Show resolved Hide resolved

jmortlock added 2 commits October 4, 2022 16:18

Update documentation

887b45b

Merge branch 'main' into certificate-name

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

af2b54d

aws-cdk-automation previously requested changes Oct 4, 2022

View reviewed changes

jmortlock added 3 commits October 4, 2022 16:26

Merge branch 'main' into certificate-name

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

6f22e72

Create integration test

8360e8f

Merge branch 'certificate-name' of github.com:jmortlock/aws-cdk into …

718ebe3

…certificate-name

Enable lookups

629064f

Switch to values used in build

0af1c0d

corymhall previously requested changes Oct 6, 2022

View reviewed changes

Ensure the autogenerated tag will not exceed the tag length limit

2eb851a

corymhall approved these changes Oct 7, 2022

View reviewed changes

Merge branch 'main' into certificate-name

d161e43

Merge branch 'main' into certificate-name

c0836ee

mergify bot merged commit 614ba92 into aws:main Oct 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-certificatemanager): Add ability to specify the certificate name #22301

feat(aws-certificatemanager): Add ability to specify the certificate name #22301

jmortlock commented Sep 30, 2022 •

edited

Loading

gitpod-io bot commented Sep 30, 2022

aws-cdk-automation left a comment

aws-cdk-automation left a comment

jmortlock commented Sep 30, 2022

aws-cdk-automation left a comment

jmortlock commented Sep 30, 2022

aws-cdk-automation left a comment

aws-cdk-automation left a comment

corymhall left a comment

corymhall commented Oct 3, 2022

aws-cdk-automation left a comment

corymhall left a comment

corymhall Oct 6, 2022

jmortlock Oct 6, 2022

mergify bot commented Oct 7, 2022

mergify bot commented Oct 7, 2022

aws-cdk-automation commented Oct 7, 2022

mergify bot commented Oct 7, 2022

feat(aws-certificatemanager): Add ability to specify the certificate name #22301

feat(aws-certificatemanager): Add ability to specify the certificate name #22301

Conversation

jmortlock commented Sep 30, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Sep 30, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

jmortlock commented Sep 30, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

jmortlock commented Sep 30, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

corymhall commented Oct 3, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

corymhall Oct 6, 2022

Choose a reason for hiding this comment

jmortlock Oct 6, 2022

Choose a reason for hiding this comment

mergify bot commented Oct 7, 2022

mergify bot commented Oct 7, 2022

aws-cdk-automation commented Oct 7, 2022

AWS CodeBuild CI Report

mergify bot commented Oct 7, 2022

jmortlock commented Sep 30, 2022 •

edited

Loading