Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

certificatemanager: DnsValidatedCertificate tags with cross-stack usage fails on upgrade to 1.100 #14519

Closed
Ranjith072 opened this issue May 4, 2021 · 10 comments · Fixed by #22122
Labels
@aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@Ranjith072
Copy link

i am using certificatemanager.DnsValidatedCertificate(python version) to create and validate the certificate ,
this was working fine until cdk version(1.100.0) with 1.100.0 DnsValidatedCertificate is adding Tags to the custom resource as shown below .
image

because of these tags it is trying to update the custom resource and fails with the error as shown below .
image

i have tried to remove these tags explicitly by using the remove tags method but it could not remove them.
cdk_core.Tags.of(core).remove(
"ApplicationName", include_resource_types=["AWS::CloudFormation::CustomResource"]
)

Reproduction Steps

self.hosted_zone_wildcard_certificate_us_east_1 = certificatemanager.DnsValidatedCertificate(
self,
"DnsValidationUsEast1",
hosted_zone=self.hosted_zone, # type: ignore
domain_name="*." + self.hosted_zone_name,
region="us-east-1",
)
if self.hosted_zone_wildcard_certificate_us_east_1 is used in another stack then it will fail.

What did you expect to happen?

this should not update the custom custom .

What actually happened?

it was not suppose to update the custom resource by adding the Tags to the custom resource.

Environment

  • **CDK CLI Version :1.100.0
  • **Framework Version:1.100.0
  • **Node.js Version:14.15.4
  • MAC:big sur 11.3OS :**
  • **Language (Version):python3.8.8

Other


This is 🐛 Bug Report

@Ranjith072 Ranjith072 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 4, 2021
@github-actions github-actions bot added the @aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager label May 4, 2021
@Ranjith072 Ranjith072 changed the title certificatemanager: DnsValidatedCertificate is trying to add Tags to the custom resource which is trying to replace the existing custom resource and resulting in failure because the certificate is already in use in other stacks. certificatemanager: DnsValidatedCertificate is trying to add Tags to the custom resource which is trying to update the custom resource and resulting in failure because the certificate is already in use in other stacks. May 4, 2021
@njlynch njlynch added effort/small Small work item – less than a day of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels May 4, 2021
@njlynch
Copy link
Contributor

njlynch commented May 4, 2021

Thanks for filing the issue, and sorry for the inconvenience.

Can you provide a bit more detail to help us come up with a solution? I believe this is because the modification to the Custom Resource is triggering an ID change, which in turn alters the ID of the export, but it's not clear that's the case from the images shown. Can you show the output of cdk diff after the upgrade to 1.101.0?

It also seems like the workaround (explicitly removing tags from the custom resource) should work. In your example (cdk_core.Tags.of(core)), what is core? Is that a higher-level construct and/or stack that contains the custom resource?

It seems like the bug here is less that the custom resource is being tagged, and more that the change to the custom resources is causing the cross-stack references to break. This is a general problem we talk about in our docs (see https://docs.aws.amazon.com/cdk/api/latest/docs/core-readme.html#removing-automatic-cross-stack-references), but in this case we should try to find an automated way to work around this.

@Ranjith072
Copy link
Author

Hi ,
please find the below screen shots for cdk diff after upgrading to 1.101.0:
image
image
image
image
image
image

"core" is the stack that contains the custom resource.
usually Tags will be list of dict but here it is just a dict which is strange and also custom resources itself do not exist physically does it really help having Tags to custom resources?
i have used custom resources in many different stacks but i didn't get tags to any of the custom resources i manage , tags are added only to the custom resource created by "DnsValidatedCertificate" construct which is managed by cdk.

@henrist
Copy link

henrist commented May 5, 2021

The tags visible in the example is due to an Aspect tagging all resources in the CDK app. As of CDK 1.100, by #13990, the DnsValidatedCertificate construct is taggable and will propagate them to the custom resource.

Both the create and update operations of the custom resource of DnsValidatedCertificate will create a new certificate, and perform cleanup afterwards. So tags can never be modified, nor added initially on an existing resource, without causing a replacement.

The update operation should be able to reuse an existing physical resource if possible, and only create a new physical resource if it is incompatible.

(As a workaround for this example, modifying the Aspect so it ignores DnsValidatedCertificate is needed to prevent the tags to be added.)

@Ranjith072
Copy link
Author

@henrist ,
I have already tried to run this code in the app.py but still it does not remove the tags.

@jsii.implements(cdk_core.IAspect)
class RemoveTags:
@staticmethod
def remove(node: cdk_core.Stack, name: str):
cdk_core.Tags.of(node).remove(
name, include_resource_types=["AWS::CloudFormation::CustomResource"]
)

def visit(self, node):
    if isinstance(node, cdk_core.Stack):
        self.remove(node, "ApplicationName")
        self.remove(node, "StackName")
        self.remove(node, "ProjectName")

Aspects.of(app).add(RemoveTags())

@henrist
Copy link

henrist commented May 6, 2021

Simply modify the existing aspect so it does not add tags to the resource. See e.g. capralifecycle/liflig-cdk@0d09652#diff-077227343fd51db7365fbf2f2db1be0fd3bb3066376de11f8c381ade83dd6dfc

@njlynch njlynch changed the title certificatemanager: DnsValidatedCertificate is trying to add Tags to the custom resource which is trying to update the custom resource and resulting in failure because the certificate is already in use in other stacks. certificatemanager: DnsValidatedCertificate tags with cross-stack usage fails on upgrade to 1.100 May 7, 2021
@njlynch
Copy link
Contributor

njlynch commented May 7, 2021

(Tagging @timothy-farestad just for awareness.)

Reproduced with a minimal example (deploy both stacks with cdk 1.99.0; upgrade to 1.100.0; deploy Stack A (or both)):

import * as cdk from '@aws-cdk/core';
import * as r53 from '@aws-cdk/aws-route53';
import * as acm from '@aws-cdk/aws-certificatemanager';

export class StackA extends cdk.Stack {
  public readonly cert: acm.ICertificate;
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    this.cert = new acm.DnsValidatedCertificate(this, 'Cert', {
      domainName: 'issue14519.example.com',
      hostedZone: r53.HostedZone.fromHostedZoneAttributes(this, 'Zone', {
        hostedZoneId: 'Z2UMGPBOS12345',
        zoneName: 'example.com',
      }),
      region: 'eu-west-2',
    });
  }
}

export class StackB extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, cert: acm.ICertificate) {
    super(scope, id);

    new cdk.CfnOutput(this, 'CertArn', {
      value: cert.certificateArn
    });
  }
}

const app = new cdk.App();
const stackA = new StackA(app, 'StackA');
cdk.Tags.of(app).add('myTag', 'myValue');
new StackB(app, 'StackB', stackA.cert);

The Tags can actually be removed, you just need to specify the Certificate resource type, since that's what the tags are labeled as:

cdk.Tags.of(app).remove('myTag', {
  includeResourceTypes: ['AWS::CertificateManager::Certificate'],
});

However, this doesn't actually solve the problem. The IAM Policy and Lambda Function associated with the Custom Resource have changed, regardless of if tags are applied or not, which still causes the same issue. The same issue would present if the Lambda function was changed in any way at all (even adding a comment).

The real solution is likely to make the Lambda function smarter; currently, the on "Update", the function still just requests a new certificate. If the actual properties of the certificate haven't changed, this should be a no-op; if only tags have changed, we should be able to add/remove tags intelligently.

This is the relevant bit of the code that's not intelligent enough yet:

case 'Create':
case 'Update':
certificateArn = await requestCertificate(
event.RequestId,
event.ResourceProperties.DomainName,
event.ResourceProperties.SubjectAlternativeNames,
event.ResourceProperties.HostedZoneId,
event.ResourceProperties.Region,
event.ResourceProperties.Route53Endpoint,
event.ResourceProperties.Tags,
);

The solution here will likely involve comparing the event.ResourceProperties and event.OldResourceProperties (see the docs) on Update, and then taking the right next steps based on that.

It seems like this issue impacts a significant number of customers, and I've tagged it as P1, which means it should be on our near-term roadmap. If anyone is interested in taking a stab at the fix, I'd be more than happy to work with you. If you are able, we encourage you to contribute a bug fix.

@Ranjith072
Copy link
Author

Hi ,
after making the changes as @henrist suggested above it has removed the tags from custom resource but as @njlynch said the issue is still the same . i will wait for it to be fixed and continue to use 1.99.0.

@Ranjith072
Copy link
Author

Hi @njlynch ,
Any update on this ?

@jacobsalway
Copy link

Encountered this in a personal project of mine and happy to take a stab at a fix, but this is my first PR and might be good to have some help on this. I don't assume there's been any update since last year?

@mergify mergify bot closed this as completed in #22122 Oct 2, 2022
mergify bot pushed a commit that referenced this issue Oct 2, 2022
…Certificate (#22122)

This PR adds a method override for applyRemovalPolicy which allows the user to specify a removal policy for the DnsValidatedCertificate construct. Since this construct is backed by a custom resource, the lambda handler was updated to no longer delete the certificate if the RemovalPolicy is set to retain.

This is also needed to allow for an easier migration from DnsValidatedCertificate -> Certificate

reroll of #22040
This has the same changes as #22040 with the addition of some logic to handle only processing updates for certain parameters. If `RemovalPolicy` is changed for example, the update will not be processed.

I also added an integration test with some manual instructions. In order to test ACM certificates I also updated the integ-runner to handle some additional special env variables.

fixes #20649, fixes #14519


----

### All Submissions:

* [ ] 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

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
@github-actions
Copy link

github-actions bot commented Oct 2, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

arewa pushed a commit to arewa/aws-cdk that referenced this issue Oct 8, 2022
…Certificate (aws#22122)

This PR adds a method override for applyRemovalPolicy which allows the user to specify a removal policy for the DnsValidatedCertificate construct. Since this construct is backed by a custom resource, the lambda handler was updated to no longer delete the certificate if the RemovalPolicy is set to retain.

This is also needed to allow for an easier migration from DnsValidatedCertificate -> Certificate

reroll of aws#22040
This has the same changes as aws#22040 with the addition of some logic to handle only processing updates for certain parameters. If `RemovalPolicy` is changed for example, the update will not be processed.

I also added an integration test with some manual instructions. In order to test ACM certificates I also updated the integ-runner to handle some additional special env variables.

fixes aws#20649, fixes aws#14519


----

### All Submissions:

* [ ] 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

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
homakk pushed a commit to homakk/aws-cdk that referenced this issue Dec 1, 2022
…Certificate (aws#22122)

This PR adds a method override for applyRemovalPolicy which allows the user to specify a removal policy for the DnsValidatedCertificate construct. Since this construct is backed by a custom resource, the lambda handler was updated to no longer delete the certificate if the RemovalPolicy is set to retain.

This is also needed to allow for an easier migration from DnsValidatedCertificate -> Certificate

reroll of aws#22040
This has the same changes as aws#22040 with the addition of some logic to handle only processing updates for certain parameters. If `RemovalPolicy` is changed for example, the update will not be processed.

I also added an integration test with some manual instructions. In order to test ACM certificates I also updated the integ-runner to handle some additional special env variables.

fixes aws#20649, fixes aws#14519


----

### All Submissions:

* [ ] 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

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
mergify bot pushed a commit that referenced this issue Jan 25, 2023
Now that the official CloudFormation resource `AWS::CertificateManager::Certificate` (CDK's `Certificate` construct) supports DNS validation we do not want to recommend using the `DnsValidatedCertificate` construct.

The `DnsValidatedCertificate` construct uses CloudFormation custom resources to perform the certificate creation and this creates a lot of maintenance burden on our team (see the list of linked issues). Currently the primary use case for using `DnsValidatedCertificate` over `Certificate` is for cross region use cases. For this use case I have updated the README to have our suggested solution.

The example in the README is tested in this [integration test](https://github.com/aws/aws-cdk/blob/main/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-cross-region-cert.ts)

fixes #8934, #2914, #20698, #17349, #15217, #14519


----

### All Submissions:

* [ ] 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

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants