aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.59k stars 3.89k forks source link

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

Closed Ranjith072 closed 2 years ago

Ranjith072 commented 3 years ago

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

Other


This is :bug: Bug Report

njlynch commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

Simply modify the existing aspect so it does not add tags to the resource. See e.g. https://github.com/capralifecycle/liflig-cdk/commit/0d09652ffcfaa1d38e653d22f22128b2225422de#diff-077227343fd51db7365fbf2f2db1be0fd3bb3066376de11f8c381ade83dd6dfc

njlynch commented 3 years ago

(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:

https://github.com/aws/aws-cdk/blob/46a16312825cc7b5bf985bc8b69146a1397690d0/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js#L246-L256

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 commented 3 years ago

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 commented 3 years ago

Hi @njlynch , Any update on this ?

jacobsalway commented 2 years ago

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?

github-actions[bot] commented 2 years ago

⚠️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.