(secretsmanager): cannot make 'RotationRules' unset for RotationSchedule

[RobReus]

What is the problem?

Without tapping into the CfnRotationSchedule construct directly, it is not possible to leave the RotationRules (automaticallyAfter in the RotationSchedule construct) property of AWS::SecretsManager::RotationSchedule empty. When using the CfnRotationSchedule and not setting the rotationRules property, it is left unset as is expected.

The CloudFormation docs (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-secretsmanager-rotationschedule.html#cfn-secretsmanager-rotationschedule-rotationrules) has this property set as optional, and when left empty, allows you to configure a rotation lambda without any schedule.

My use-case for this is for secrets that do not need to be automatically rotated, but only on demand when needed.

Reproduction Steps

const rotationSchedule = new secretsmanager.RotationSchedule(this, 'MyRotationSchedule', {
  secret: secret,
  // automaticallyAfter: cdk.Duration.minutes(30),
  rotationLambda: function_,
});

What did you expect to happen?

The resulting CloudFormation template to not have the property RotationRules set for the AWS::SecretsManager::RotationSchedule resource.

What actually happened?

The resulting CloudFormation template has the property RotationRules set to a default of 30 days for the AWS::SecretsManager::RotationSchedule resource.

CDK CLI Version

2.10.0

Framework Version

2.10.0

Node.js Version

16.13.2

OS

Ubuntu 20.04 LTS (WSL2)

Language

Typescript

Language Version

4.5.5

Other information

No response

[ryparker]

Hey @RobReus 👋🏻 Thanks for reporting this.

It does look like we force the default to be 30 days. We should allow this to be undefined to match CloudFormation's implementation.

Marked this as p1 which means it is prioritized as important to our devs. As always we appreciate any contributions if anyone is interested in tackling this as there are many issues that our devs are currently working through (see our contribution guidelines).

Meanwhile if you need a quick workaround you should be able to override the generated template properties using escape hatches.

[AdamVD]

Hi @RobReus and @ryparker 👋

I'd be happy to contribute a fix here, but we need to evaluate what the best non-breaking fix would be. If we stopped applying the default 30 days then automatic rotation would be turned off for any users relying on the default. Clearly this is breaking behavior and not acceptable.

I've thought of a couple options to fix this without a breaking change:

1. We treat a automaticallyAfter input duration of zero (e.g. Duration.days(0)) to indicate that automatic rotation should be disabled. When a zero duration is received then we would set the underlying Cfn option to undefined.
2. We add a manualRotation option that defaults false. If it is set true we will have to check if automaticallyAfter is set and throw an error if so, as these would be incompatible options. When manualRotation is true we would set the underlying Cfn option to undefined.

I think the second option is clearer and more quickly recognizable to anyone trying to turn off automatic rotation. I'm also open to names other than manualRotation.

Let me know what you think!

[RobReus]

@AdamVD thank you for your willingness to contribute to fix this issue.

Personally I like the second option better as well. I feel like the way you have described it will be clear and fully backwards compatible. It would be the perfect solution for me.

[github-actions]

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