Skip to content
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): add secret rotation to DatabaseClusterFromSnapshot #20020

Conversation

davenix-palmetto
Copy link
Contributor

@davenix-palmetto davenix-palmetto commented Apr 21, 2022

Bring RDS DatabaseClusterFromSnapshot API to parity with DatabaseCluster in being able to add Secrets Manager credential rotation with addRotationSingleUser or addRotationMultiUser.

My first PR here! There may be some potential to DRY up this approach by moving up the method to the parent DatabaseClusterNew class as for now the code is duplicative between the classes, but I am frankly not comfortable doing it myself. Any input and suggestions very welcome -- thanks in advance!

closes #12877


All Submissions:

Adding new Unconventional Dependencies:

N/A

New Features

N/A

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

@gitpod-io
Copy link

gitpod-io bot commented Apr 21, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team April 21, 2022 17:04
@github-actions github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Apr 21, 2022
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

I would say that we do definitely want to move addRotationSingleUser and addRotationMultiUser into DatabaseClusterNew rather than rewriting them again since the functions and added fields are identical in both places. I'd encourage you to go ahead and give it a shot!

In fact, a bit more of the duplicated functionality could be moved into DatabaseClusterNew but I won't ask for that refactor since you're not making changes to those pieces of code. Still, feel free to give that a shot as well, if you'd like.

@davenix-palmetto
Copy link
Contributor Author

davenix-palmetto commented Apr 27, 2022

I would say that we do definitely want to move addRotationSingleUser and addRotationMultiUser into DatabaseClusterNew rather than rewriting them again since the functions and added fields are identical in both places. I'd encourage you to go ahead and give it a shot!

Thank you for the reply and comments @TheRealAmazonKendra! I tried putting the methods up into the parent class, and it is turning out to be a bit stickier than expected -- it is a significant lift for me to attempt a refactor given I am not so familiar with the internals here, but I will keep trying...

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review April 28, 2022 03:21

Pull request has been modified.

@davenix-palmetto
Copy link
Contributor Author

Should be good to go for another look @TheRealAmazonKendra 🙏

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this. Just a couple more things and this should be good to go.

packages/@aws-cdk/aws-rds/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-rds/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-rds/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-rds/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-rds/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-rds/lib/cluster.ts Show resolved Hide resolved
expect(() => cluster.addRotationSingleUser()).toThrow(/A single user rotation was already added to this cluster/);
});

test('create a cluster from a snapshot with multi user secret rotation', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do an error case for a missing secret on multi user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like right now a secret is required in RotationMultiUserOptions, unlike in RotationSingleUserOptions, so a similar test as in addRotationSingleUser cannot be constructed for addRotationMultiUser. Can you advise a bit further? Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I was wrong here. Your error cases are fine.

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>
@mergify mergify bot dismissed TheRealAmazonKendra’s stale review April 28, 2022 22:46

Pull request has been modified.

@TheRealAmazonKendra TheRealAmazonKendra added pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes labels Apr 30, 2022
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks you for your contribution!

@mergify
Copy link
Contributor

mergify bot commented Apr 30, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: a9fb890
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit abc3502 into aws:master Apr 30, 2022
@mergify
Copy link
Contributor

mergify bot commented Apr 30, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@davenix-palmetto
Copy link
Contributor Author

Great work! Thanks you for your contribution!

Thank you!

wphilipw pushed a commit to wphilipw/aws-cdk that referenced this pull request May 23, 2022
…20020)

Bring RDS `DatabaseClusterFromSnapshot` API to parity with `DatabaseCluster` in being able to add Secrets Manager credential rotation with `addRotationSingleUser` or `addRotationMultiUser`.

My first PR here! There may be some potential to DRY up this approach by moving up the method to the parent `DatabaseClusterNew` class as for now the code is duplicative between the classes, but I am frankly not comfortable doing it myself. Any input and suggestions very welcome -- thanks in advance!

closes aws#12877

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:
N/A

### New Features
N/A

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
4 participants