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

(cloudfront): the auto generated name of cache policy must be unique in the account #13629

Closed
zxkane opened this issue Mar 17, 2021 · 3 comments · Fixed by #13737
Closed

(cloudfront): the auto generated name of cache policy must be unique in the account #13629

zxkane opened this issue Mar 17, 2021 · 3 comments · Fixed by #13737
Assignees
Labels
@aws-cdk/aws-cloudfront Related to Amazon CloudFront bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@zxkane
Copy link
Contributor

zxkane commented Mar 17, 2021

Deploying the same application in different regions, the second deployment failed with error message Internal error reported from downstream service during operation 'AWS::CloudFront::CachePolicy'. It’s caused by the generated name of CachePolicy are same for multiple deployments.

Reproduction Steps

Create an app with a distribution like below,

      new Distribution(this, 'Distribution', {
        defaultBehavior: {
          origin: new S3Origin(websiteBucket),
          viewerProtocolPolicy: ViewerProtocolPolicy.REDIRECT_TO_HTTPS,
          allowedMethods: AllowedMethods.ALLOW_GET_HEAD,
          cachePolicy: new CachePolicy(this, 'defaultCachePolicy', {
            defaultTtl: Duration.days(7),
            minTtl: Duration.seconds(0),
            maxTtl: Duration.days(30),
            enableAcceptEncodingGzip: true,
            enableAcceptEncodingBrotli: true,
          }),
        },
        defaultRootObject: 'index.html',
      });
    }

What did you expect to happen?

The auto generated name of policy name is unique in the account, it might depend on the region and name of stack.

What actually happened?

Environment

  • CDK CLI Version : 1.91.0
  • Framework Version:
  • Node.js Version:
  • OS :
  • Language (Version):

Other


This is 🐛 Bug Report

@zxkane zxkane added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 17, 2021
@github-actions github-actions bot added the @aws-cdk/aws-cloudfront Related to Amazon CloudFront label Mar 17, 2021
@njlynch
Copy link
Contributor

njlynch commented Mar 19, 2021

Thanks for the bug report.

The autogenerated name should take into account Stack name, but it certainly doesn't take into account region today. We absolutely could add the region to the name though. It's not a bad idea. It would be a relatively straightforward change to alter the below to use something like Stack.of(this).region along with the Names.uniqueId() call:

https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-cloudfront/lib/cache-policy.ts#L128

If you (or anyone else) is up for submitting a PR, I'd be happy to work with you to get the fix merged in.

As a work-around, you can specify the cachePolicyName yourself, and add the region to it:

          new CachePolicy(this, 'defaultCachePolicy', {
            cachePolicyName: `MyPolicy-${Stack.of(this).region}`,
            defaultTtl: Duration.days(7),
            minTtl: Duration.seconds(0),
            maxTtl: Duration.days(30),
            enableAcceptEncodingGzip: true,
            enableAcceptEncodingBrotli: true,
          })

@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 Mar 19, 2021
@zxkane
Copy link
Contributor Author

zxkane commented Mar 26, 2021

Hi Nick,

The current cachePolicyName is generated from construct id only. My CDK app is created Distribution in nested stack, which always has unique stack name. I can reproducible this bug when deploying the same application to multiple regions.

Another proposal is making the property cachePolicyName as a mandatory property, the caller makes sure using the unique name in account wide for it. But it’s a breaking change, we probably should avoid.

@mergify mergify bot closed this as completed in #13737 Mar 29, 2021
mergify bot pushed a commit that referenced this issue Mar 29, 2021
…s-region (#13737)

This commit changes the auto-generated name of the `CachePolicy` to include stack name and region, thus providing unique name for the `CachePolicy` account-wide.

Closes #13629.

----

*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

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

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Mar 31, 2021
…s-region (aws#13737)

This commit changes the auto-generated name of the `CachePolicy` to include stack name and region, thus providing unique name for the `CachePolicy` account-wide.

Closes aws#13629.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
…s-region (aws#13737)

This commit changes the auto-generated name of the `CachePolicy` to include stack name and region, thus providing unique name for the `CachePolicy` account-wide.

Closes aws#13629.

----

*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-cloudfront Related to Amazon CloudFront 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.

2 participants