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

[aws-rds] When moving from DatabaseCluster to DatabaseClusterFromSnapshot, lose ability to rotate master passwords #12877

Closed
jonahbeckford opened this issue Feb 4, 2021 · 5 comments · Fixed by #20020
Labels
@aws-cdk/aws-rds Related to Amazon Relational Database effort/small Small work item – less than a day of effort feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. feature-request A feature should be added or improved. p1

Comments

@jonahbeckford
Copy link

If I start with a DatabaseCluster and then need to switch it to DatabaseClusterFromSnapshot, I am forced to weaken security because DatabaseClusterFromSnapshot does not have addRotationSingleUser().

Reproduction Steps

Starting point was:

const vpc = ...
const cluster = new DatabaseCluster(this, 'Database', {
    engine: DatabaseClusterEngine.auroraPostgres({ version: AuroraPostgresEngineVersion.VER_11_9 }),
    instanceProps: {
        vpc: vpc,
    },
})
cluster.addRotationSingleUser({ automaticallyAfter: Duration.days(2) })

Then, after removing the cluster (which creates a snapshot) and recreating it with:

const vpc = ...
const cluster = new DatabaseClusterFromSnapshot(this, 'Database', {
    snapshotIdentifier: "",
    engine: DatabaseClusterEngine.auroraPostgres({ version: AuroraPostgresEngineVersion.VER_11_9 }),
    instanceProps: {
        vpc: vpc,
    },
})
// The next line fails to compile!
cluster.addRotationSingleUser({ automaticallyAfter: Duration.days(2) })

the last line cannot be used.

What did you expect to happen?

I expected DatabaseClusterFromSnapshot to have the same API as DatabaseCluster.

What actually happened?

error TS2339: Property 'addRotationSingleUser' does not exist on type 'DatabaseClusterFromSnapshot'.

Environment

  • CDK CLI Version : 1.88.0 (build f65009b)
  • Framework Version: aws-cli/2.1.8 Python/3.7.3 Linux/4.19.128-microsoft-standard exe/x86_64.ubuntu.18 prompt/off
  • Node.js Version: v12.20.1
  • OS : Ubuntu 18.04 on WSL 2
  • Language (Version): TypeScript 3.9.7

Other

I would suggest pulling DatabaseCluster.addRotationSingleUser and DatabaseCluster.addRotationMultiUser up into DatabaseClusterNew (which is the shared superclass of DatabaseClusterFromSnapshot)


This is 🐛 Bug Report

@jonahbeckford jonahbeckford added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 4, 2021
@skinny85 skinny85 self-assigned this Feb 5, 2021
@skinny85 skinny85 added @aws-cdk/aws-rds Related to Amazon Relational Database effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 5, 2021
@skinny85
Copy link
Contributor

skinny85 commented Feb 5, 2021

Thanks for opening the issue @jonahbeckford! Should be relatively straightforward change to add the addRotationSingleUser() and addRotationMultiUser() methods to DatabaseClusterFromSnapshot.

We really encourage contributions, so if you'd be willing to open us a PR adding this feature, that would be amazing 🙂. Here's our 'Contributing' guide: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md.

Thanks,
Adam

@jonahbeckford
Copy link
Author

Okay, will try to create my first PR for aws-cdk.

@ericzbeard ericzbeard added the feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. label Apr 2, 2021
@mergify mergify bot closed this as completed in #20020 Apr 30, 2022
mergify bot pushed a commit that referenced this issue Apr 30, 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:

* [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*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@anthonygerrard
Copy link

This change adds a credentials property to DatabaseClusterFromSnapshotProps. Documented as

Credentials for the administrative user.

But will these credentials work if you restore from snapshot? My understanding is that you get the admin user of the snapshot.

@skinny85
Copy link
Contributor

The username has to be from the snapshot, yes, but the password can be updated. See here for the details.

wphilipw pushed a commit to wphilipw/aws-cdk that referenced this issue 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
@aws-cdk/aws-rds Related to Amazon Relational Database effort/small Small work item – less than a day of effort feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants