fix(secretsmanager): cannot set hourly rotation #28303

lpizzinidev · 2023-12-08T15:04:26Z

Allows to set hourly rotation up to 4 hours on secrets as per official docs.

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

vinayak-kukreja

Hey @lpizzinidev , thank you for the contribution. Just some minor comments :)

vinayak-kukreja · 2023-12-08T17:30:31Z

packages/aws-cdk-lib/aws-secretsmanager/lib/rotation-schedule.ts

+    let automaticallyAfterDays: number | undefined;
+    let scheduleExpression: string | undefined;
+    if (props.automaticallyAfter) {
+      if (props.automaticallyAfter.toMilliseconds() > 0) {


props.automaticallyAfter.toMilliseconds()

I think this is repeating and we can keep this as a variable.

vinayak-kukreja · 2023-12-08T17:31:26Z

packages/aws-cdk-lib/aws-secretsmanager/lib/rotation-schedule.ts

+        if (props.automaticallyAfter.toMilliseconds() > Duration.days(1000).toMilliseconds()) {
+          throw new Error(`automaticallyAfter must not be greater than 1000 days, got ${props.automaticallyAfter.toDays()} days`);
+        }
+        if (props.automaticallyAfter.toHours() > 23) {


NIT: How about >=24, just easier to interpret for me. Thoughts?

vinayak-kukreja · 2023-12-08T17:33:16Z

packages/@aws-cdk-testing/framework-integ/test/aws-secretsmanager/test/integ.lambda-rotation.ts

@@ -21,6 +21,7 @@ class TestStack extends cdk.Stack {
        handler: 'index.handler',
        code: lambda.Code.fromInline('NOOP'),
      }),
+      automaticallyAfter: cdk.Duration.hours(4),


Running the integ test to verify.

Hey, I see the integ test fails due to the schedule not working. I still see the schedule is at 30 days.

I didn't update tree.json, should be good now.

Pull request has been modified.

kaizencc · 2023-12-13T18:18:05Z

packages/aws-cdk-lib/aws-secretsmanager/lib/rotation-schedule.ts

@@ -2,6 +2,7 @@ import { Construct } from 'constructs';
 import { ISecret, Secret } from './secret';
 import { CfnRotationSchedule } from './secretsmanager.generated';
 import * as ec2 from '../../aws-ec2';
+import { Schedule } from '../../aws-events';


AHhhh I suppose this is ok because the schedule is not exposed in anyway, but we STILL need to get this into class into core.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

kaizencc

Of course, long term we need to get schedule into core so we can use that instead of using the one in aws-events.

kaizencc · 2023-12-13T18:26:39Z

packages/aws-cdk-lib/aws-secretsmanager/lib/rotation-schedule.ts

+    if (automaticallyAfterDays !== undefined || scheduleExpression !== undefined) {
      rotationRules = {
        automaticallyAfterDays,
+        scheduleExpression,


@lpizzinidev I don't like this implementation because we are returning both automaticallyAfterDays and scheduleExpression knowing full well that one is undefined. We're then relying on this information elsewhere, which is okay for now, but is an assumption we'll have to maintain forever.

Instead, I think we should just localize on scheduleExpression. We can keep the automaticallyAfter prop, but wire it to scheduleExpression all the time. We can specify rate(5 days) if we get a unit in days, or rate(4 hours) if the unit is in hours.

scheduleExpression can do everything automaticallyAfterDays can do, and more, so lets standardize on that.

Pull request has been modified.

kaizencc

Thanks @lpizzinidev

mergify · 2023-12-14T23:29:49Z

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 · 2023-12-14T23:57:02Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: f1a3cc9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-12-14T23:57:23Z

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

github-actions bot added distinguished-contributor [Pilot] contributed 50+ PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p2 labels Dec 8, 2023

aws-cdk-automation requested a review from a team December 8, 2023 15:04

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 8, 2023

vinayak-kukreja self-assigned this Dec 8, 2023

vinayak-kukreja previously requested changes Dec 8, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 8, 2023

lpizzinidev force-pushed the gh-28261 branch from 134c250 to 3f76159 Compare December 9, 2023 11:18

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 9, 2023

kaizencc reviewed Dec 13, 2023

View reviewed changes

kaizencc changed the title ~~fix(secretsmanager): cannot set hourly rotation~~ feat(secretsmanager): set hourly rotation Dec 13, 2023

aws-cdk-automation previously requested changes Dec 13, 2023

View reviewed changes

kaizencc changed the title ~~feat(secretsmanager): set hourly rotation~~ fix(secretsmanager): cannot set hourly rotation Dec 13, 2023

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 13, 2023

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 13, 2023

kaizencc previously requested changes Dec 13, 2023

View reviewed changes

aws-cdk-automation removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Dec 13, 2023

lpizzinidev force-pushed the gh-28261 branch from 2942018 to 6467667 Compare December 14, 2023 09:20

lpizzinidev added 4 commits December 14, 2023 10:43

fix(secretsmanager): cannot set hourly rotation

dc05370

minor nits and template update

ada07d7

handle rotation in days with scheduleExpression

524ea38

fixed integration test

66413dc

lpizzinidev force-pushed the gh-28261 branch from 6467667 to 66413dc Compare December 14, 2023 09:44

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 14, 2023

lpizzinidev requested a review from kaizencc December 14, 2023 10:51

kaizencc approved these changes Dec 14, 2023

View reviewed changes

Merge branch 'main' into awsgh-28261

f1a3cc9

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 14, 2023

mergify bot merged commit 09cb003 into aws:main Dec 14, 2023
12 checks passed

github-actions bot mentioned this pull request Jan 1, 2024

Monthly PR metrics report - Dec 2023 #28544

Closed

anentropic mentioned this pull request Jan 12, 2024

bug: spurious AwsSolutions-SMG4 with aws-cdk-lib >= 2.116.0 cdklabs/cdk-nag#1560

Closed

clueleaf mentioned this pull request Jan 17, 2024

bug: SecretsManagerRotationEnabled is non compliant even if rotation is configured after cdk v2.116 cdklabs/cdk-nag#1565

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(secretsmanager): cannot set hourly rotation #28303

fix(secretsmanager): cannot set hourly rotation #28303

lpizzinidev commented Dec 8, 2023

vinayak-kukreja left a comment

vinayak-kukreja Dec 8, 2023

vinayak-kukreja Dec 8, 2023

vinayak-kukreja Dec 8, 2023

vinayak-kukreja Dec 8, 2023

vinayak-kukreja Dec 8, 2023

lpizzinidev Dec 9, 2023

kaizencc Dec 13, 2023

aws-cdk-automation left a comment

kaizencc left a comment

kaizencc Dec 13, 2023

kaizencc left a comment

mergify bot commented Dec 14, 2023

aws-cdk-automation commented Dec 14, 2023

mergify bot commented Dec 14, 2023

fix(secretsmanager): cannot set hourly rotation #28303

fix(secretsmanager): cannot set hourly rotation #28303

Conversation

lpizzinidev commented Dec 8, 2023

vinayak-kukreja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 14, 2023

aws-cdk-automation commented Dec 14, 2023

AWS CodeBuild CI Report

mergify bot commented Dec 14, 2023