-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(secretsmanager): cannot set hourly rotation #28303
Changes from all commits
dc05370
ada07d7
524ea38
66413dc
f1a3cc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
import * as iam from '../../aws-iam'; | ||
import * as kms from '../../aws-kms'; | ||
import * as lambda from '../../aws-lambda'; | ||
|
@@ -37,6 +38,7 @@ export interface RotationScheduleOptions { | |
* Specifies the number of days after the previous rotation before | ||
* Secrets Manager triggers the next automatic rotation. | ||
* | ||
* The minimum value is 4 hours. | ||
* The maximum value is 1000 days. | ||
* | ||
* A value of zero (`Duration.days(0)`) will not create RotationRules. | ||
|
@@ -126,18 +128,26 @@ export class RotationSchedule extends Resource { | |
); | ||
} | ||
|
||
let automaticallyAfterDays: number | undefined = undefined; | ||
if (props.automaticallyAfter && props.automaticallyAfter.toDays() > 1000) { | ||
throw new Error(`automaticallyAfter must not be greater than 1000 days, got ${props.automaticallyAfter.toDays()} days`); | ||
} | ||
if (props.automaticallyAfter?.toMilliseconds() !== 0) { | ||
automaticallyAfterDays = props.automaticallyAfter?.toDays() || 30; | ||
let scheduleExpression: string | undefined; | ||
if (props.automaticallyAfter) { | ||
const automaticallyAfterMillis = props.automaticallyAfter.toMilliseconds(); | ||
if (automaticallyAfterMillis > 0) { | ||
if (automaticallyAfterMillis < Duration.hours(4).toMilliseconds()) { | ||
throw new Error(`automaticallyAfter must not be smaller than 4 hours, got ${props.automaticallyAfter.toHours()} hours`); | ||
} | ||
if (automaticallyAfterMillis > Duration.days(1000).toMilliseconds()) { | ||
throw new Error(`automaticallyAfter must not be greater than 1000 days, got ${props.automaticallyAfter.toDays()} days`); | ||
} | ||
scheduleExpression = Schedule.rate(props.automaticallyAfter).expressionString; | ||
} | ||
} else { | ||
scheduleExpression = Schedule.rate(Duration.days(30)).expressionString; | ||
} | ||
|
||
let rotationRules: CfnRotationSchedule.RotationRulesProperty | undefined = undefined; | ||
if (automaticallyAfterDays !== undefined) { | ||
let rotationRules: CfnRotationSchedule.RotationRulesProperty | undefined; | ||
if (scheduleExpression) { | ||
rotationRules = { | ||
automaticallyAfterDays, | ||
scheduleExpression, | ||
}; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running the integ test to verify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't update
tree.json
, should be good now.