fix(secretsmanager): cannot set hourly rotation#28303
Conversation
vinayak-kukreja
left a comment
There was a problem hiding this comment.
Hey @lpizzinidev , thank you for the contribution. Just some minor comments :)
| let automaticallyAfterDays: number | undefined; | ||
| let scheduleExpression: string | undefined; | ||
| if (props.automaticallyAfter) { | ||
| if (props.automaticallyAfter.toMilliseconds() > 0) { |
There was a problem hiding this comment.
props.automaticallyAfter.toMilliseconds()
I think this is repeating and we can keep this as a variable.
| 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) { |
There was a problem hiding this comment.
NIT: How about >=24, just easier to interpret for me. Thoughts?
| handler: 'index.handler', | ||
| code: lambda.Code.fromInline('NOOP'), | ||
| }), | ||
| automaticallyAfter: cdk.Duration.hours(4), |
There was a problem hiding this comment.
Running the integ test to verify.
There was a problem hiding this comment.
Hey, I see the integ test fails due to the schedule not working. I still see the schedule is at 30 days.
There was a problem hiding this comment.
I didn't update tree.json, should be good now.
Pull request has been modified.
| import { ISecret, Secret } from './secret'; | ||
| import { CfnRotationSchedule } from './secretsmanager.generated'; | ||
| import * as ec2 from '../../aws-ec2'; | ||
| import { Schedule } from '../../aws-events'; |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Of course, long term we need to get schedule into core so we can use that instead of using the one in aws-events.
| if (automaticallyAfterDays !== undefined || scheduleExpression !== undefined) { | ||
| rotationRules = { | ||
| automaticallyAfterDays, | ||
| scheduleExpression, |
There was a problem hiding this comment.
@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.
2942018 to
6467667
Compare
6467667 to
66413dc
Compare
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
Allows to set hourly rotation up to 4 hours on secrets as per official docs.
Closes #28261.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license