feat(kms): key rotation period

[badmintoncryer]

Issue # (if applicable)

Closes #29927.

Reason for this change

Cloudformation supports for configuring period of automatic key rotation but CDK does not.

Description of changes

Added rotationPeriod to KeyProps.

Description of how you validated changes

I've added both unit and integ tests.

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[badmintoncryer]

I've simply added rotationPeriod properties to KeyProps.

However, there are other possible implementation approaches, and I'm struggling to decide which one is best. Could I please get your opinions, reviewers?

1. add rotationPeriod property (current implementation)

new kms.Key(this, 'MyKey', {
  enableKeyRotation: true,
  rotationPeriod: Duration.days(180), // Add
});

2. deprecate enableKeyRotation and define KeyRotation property

export interface KeyRotation {
  enableKeyRotation: boolean,
  rotationPeriod?: Duration,
}

new kms.Key(this, 'MyKey', {
  keyRotation: {
    enableKeyRotation: true,
    rotationPeriod: Duration.days(180), // optional
  },
});

3. deprecate enableKeyRotation and add only rotationPeriod

new kms.Key(this, 'MyKey', {
  rotationPeriod: Duration.days(180), // Implicitly enable key rotation by defining this property
});

[nmussy]

I don't think the current implementation is that terrible. It probably wouldn't have been my first choice if enableKeyRotation wasn't already there, but given the circumstances, you're not introducing a deprecation or breaking change, you're allowing the existing construct to customize its default value.

[badmintoncryer]

@nmussy Thank you for your opinion. I'm relieved to hear that.

[comcalvi]

I like the implementation you chose @badmintoncryer, just with a small modification; if rotationPeriod is defined and enableKeyRotation is undefined, let's set enableKeyRotation to true.

[comcalvi]

I like the implementation you chose @badmintoncryer, just with a small modification; if rotationPeriod is defined and enableKeyRotation is undefined, let's set enableKeyRotation to true.

Pull request has been modified.

[badmintoncryer]

@comcalvi
Thank you for your review! I've updated my implementation. Could you please confirm it again?

[comcalvi]

Can we also add a test with enableKeyRotation explicitly set to true? Other than that this looks ready to ship.

[badmintoncryer]

I think this test covers that case.

const key = new kms.Key(stack, 'MyKey', {
    enableKeyRotation: true,
    enabled: false,
    pendingWindow: cdk.Duration.days(7),
    rotationPeriod: cdk.Duration.days(180),
  });

[comcalvi]

you are correct, it does.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 2839a0c
• 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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.