fix(cloudfront): trim autogenerated cache policy name

[robertd]

Closes #18918

---

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

[gitpod-io]

[robertd]

@RigoIce Thanks for figuring out the main cause given the generic cloudformation error. 👍

[ghost]

Hi @robertd. I just wanted to jump into a discussion about that PR.

I already had a look into the code and found out that a fix just slicing the name would probably effect the appended region. Under some circumstances we could again have same names in different regions for the policies. The region suffix was introduced in #13737 closing #13629.

I thought more about slicing the expression Names.uniqueId(this). This would probably cut of the hash what would be less problematic, I think. Also, there shouldn't be any backward problems as this only effects non-working policies with name lengths above 128 and still keeps the uniqueness across the regions.

However, that seemed not to cover all facets for me as well because the hash could serve as a unique identifier within a region. I wasn't convinced whether my proposed slicing approach could cause problems with uniqueness in one region, i.e. multiple policies.

So, this is the reason I haven't proposed a PR on my own as I see a possible solution with a backward compatible optional parameter for uniqueId() limiting the part before the hash. But this is going all the way down to CDK core just for handling exceptional cases.

Maybe my considerations are too complicated or maybe the idea could also help in other cases where a shorter ID is required.

Happy to discuss and support.

[robertd]

@RigoIce Makes sense... Although, slicing the uniqueId will have to take in consideration region concatenation... and that becomes somewhat complicated once you get into longer regions (i.e. ap-southeast-3).

I'm not 100% sure what would be the best scenario here... slice uniqueId down to 110 characters and then append region? ... just to be on the safe side?

@comcalvi Any thoughts?

[comcalvi]

I agree with that @robertd, we need to maintain the cross-region safety introduced in #13737. I see no issue with slicing it down to 110 characters and then appending the region.

[robertd]

Thanks for chiming in @comcalvi. I’ll update the PR.

[robertd]

@comcalvi ready for your review. Thanks.

[comcalvi]

Thanks for the fix, this is great!

[mergify]

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

Pull request has been modified.

[robertd]

@comcalvi I had to merge in the latest master because build failed for some weird reason. Please reapprove the PR again. TIA.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: ff6cb52
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

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