-
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
feat(rds): more extensive secret rotation support #5281
Conversation
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Add support for Redshift clusters, DocumentDB databases and the multi user rotation scheme. Move `SecretRotation` from `aws-rds` to `aws-secretsmanager`. Add resource policy for secrets and use it to prevent deletion of secrets for which rotation is enabled. Update instance class to `t3` in `aws-rds` integration tests (`t2` is being deprecated and Oracle `t2` instances cannot be created anymore). Closes aws#5194 BREAKING CHANGE: `addRotationSingleUser(id: string, options: SecretRotationOptions)` is now `addRotationSingleUser(automaticallyAfter?: Duration)`
8f5b725
to
d6360ce
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
*/ | ||
public addRotationSingleUser(id: string, options: SecretRotationOptions = {}): SecretRotation { | ||
public addRotationSingleUser(automaticallyAfter?: Duration): secretsmanager.SecretRotation { |
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.
Why is it a good idea to break this API this strongly? I appreciate that the options are drastically simplified, but the following seems nicely standardized and not too onerous to me:
cluster.addRotationSingleUser('id', {
automaticallyAfter: Duration.days(14)
});
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.
This is only to reflect the fact that a single rotation can only be added once to a cluster/instance so there's no point naming it...
} | ||
} | ||
|
||
public denyAccountRootDelete() { |
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.
How sure are you this won't affect deletions via CloudFormation?
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.
Yes this has been tested.
This approach is now recommended in the docs, see YAML comments in https://docs.aws.amazon.com/secretsmanager/latest/userguide/integrating_cloudformation.html
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
5 similar comments
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Add support for Redshift clusters, DocumentDB databases and the multi user rotation scheme.
Move
SecretRotation
fromaws-rds
toaws-secretsmanager
.Add resource policy for secrets and use it to prevent deletion of secrets for which rotation is
enabled.
Update instance class to
t3
inaws-rds
integration tests (t2
is being deprecated and Oraclet2
instances cannot be created anymore).Closes #5194
BREAKING CHANGE:
addRotationSingleUser(id: string, options: SecretRotationOptions)
is nowaddRotationSingleUser(automaticallyAfter?: Duration)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license