feat(pipeline): allow enabling KMS key rotation for cross-region-stacks

[hross-frae]

Enabling KMS Key rotation for cross-region-stacks as it is a recommended best practice.
https://docs.aws.amazon.com/kms/latest/developerguide/rotate-keys.html

In #13466 it was considered not necessary due to extra costs, but I don't think that reason applies in this use case due to the following reasons:

• There is already extra cost due to the use of codebuild and pipelines, an extra $1/month a year will not greatly affect overall costs
• Cross account/region imply advanced deployments which would already incur extra costs, so similar to above, the extra costs could be considered negligible.
• For simple one account and region deployments, this extra cost would not be applicable.

---

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

[gitpod-io]

Pull request has been modified.

[skinny85]

Thanks for the contribution @hross-frae!

A few minor comments, mainly about naming.

[skinny85]

Also - unit tests are missing 😉.

Pull request has been modified.

[skinny85]

Looks great @hross-frae! A few minor comments before I merge this in.

[gitpod-io]

Pull request has been modified.

[hross-frae]

@skinny85 it's only existing unit tests that are failing now. When I try update them there are some that are deleted snapshot files that are deleted. Is that expected? I've been running full builds and tests

[skinny85]

@hross-frae I'm not sure what you're asking, but because you implemented my comment from here: #14381 (comment) , the generated CloudFormation template will remain exactly the same as it is now.

The build currently is failing on the @aws-cdk/aws-events-targets package:

@aws-cdk/aws-events-targets: Verifying codepipeline/integ.pipeline-event-target.js against codepipeline/integ.pipeline-event-target.expected.json ... CHANGED.
@aws-cdk/aws-events-targets: Resources
@aws-cdk/aws-events-targets: [~] AWS::KMS::Key pipelinePipeline22F2A91DArtifactsBucketEncryptionKey87C796D2 
@aws-cdk/aws-events-targets:  └─ [-] EnableKeyRotation
@aws-cdk/aws-events-targets:      └─ false

If you revert the change you made to the integ.pipeline-event-target.expected.json file, the build should pass again (same comment about the other changes to the "expected" files).

[lukasbuerkle]

please merge, so that i can satisfy my security hub. thank you.

[skinny85]

please merge, so that i can satisfy my security hub. thank you.

Can't really merge it if the build is failing, unfortunately.

[pboeder]

Would be awesome to merge this PR. Unfortunately, my known workarounds for enabling KeyRotation don't work anymore with the recent v2 RCs.
Do I understand it correctly, that the only missing piece is to "revert the change you made to the integ.pipeline-event-target.expected.json file" @skinny85?
Since @hross-frae doesn't seem to respond anymore, is there any strategy on how to proceed with this PR?

[hross-frae]

Hi @ppuritscher, the main issue I can up against in the end was getting all the tests working...
As far as I can tell the code changes are all good, they might be a little out of date though. Also there might have to further changes added now due to the new CodePipeline construct

[pboeder]

Hi @hross-frae, thanks for the update and your work there!
Are you sure, that there are any further changes required for now, concerning the new CodePipeline construct?

I assume that you already checked the suggestion of @skinny85 ("revert the change you made to the integ.pipeline-event-target.expected.json file")?

Would it be possible for you to give git rebase a try?

[skinny85]

@ppuritscher you should be able to take this PR over if you want to (it's just a matter of getting the code from https://github.com/hross-frae/aws-cdk, the master branch, and you can then edit locally, and either push directly to @hross-frae's fork, or open a new PR).

[renner-daniel]

I created a PR hross-frae#1
@hross-frae can you pleas accept the PR? I checked the tests in @aws-cdk/aws-events-targets and reverted one change there. Tests ran green afterwards. I also checked the pipeline tests, but I did not check every single test (I takes too much time to be honest :D ), so lets see if CodeBuild runs green this time.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 8746ac0
• Result: FAILED
• Build Logs (available for 30 days)

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

[renner-daniel]

Ok, the other tests fail as well.
@skinny85 its not clear to me why the key is not published in the tests? I mean I can remove the added property in the json testfiles, but shouldn't the key be propagated?

[renner-daniel]

Ah ok, I think I found the "problem":

 const resource = new CfnKey(this, 'Resource', {
      description: props.description,
      enableKeyRotation: props.enableKeyRotation,
      enabled: props.enabled,
      keySpec: props.keySpec,
      keyUsage: props.keyUsage,
      keyPolicy: this.policy,
      pendingWindowInDays: pendingWindowInDays,
});

this.keyArn = resource.attrArn;

You build now a keyArn with all necessary information (as far as I understand). That means we cant search for enableKeyRotation in the resulting json.
I'm going to create a new PR and remove all the enableKeyRotation to fix the tests.

[pboeder]

Hey @hross-frae, @renner-daniel and @skinny85!
I just created a new PR as successor. The tests run through.

[hross-frae]

Hey @hross-frae, @renner-daniel and @skinny85!
I just created a new PR as successor. The tests run through.

Thanks @ppuritscher for picking this up and following it through. I'll close this PR as it's no longer needed