aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.47k stars 3.83k forks source link

(aws-rds): DatabaseClusterFromSnapshot no credentials property #14771

Open C0en opened 3 years ago

C0en commented 3 years ago

Would it be possible to add a credentials property/parameter to DatabaseClusterFromSnapshot? It should be similar to the credentials option in DatabaseInstanceFromSnapshot.

Use Case

Now when you restore a cluster from snapshot you don't have the master user in aws secrets manager

Proposed Solution

See the description.


This is a :rocket: Feature Request

jumic commented 3 years ago

Is this already supported in CloudFormation? In the DBCluster documentation, I found this note for property MasterUserPassword:

If you specify the SourceDBClusterIdentifier or SnapshotIdentifier property, don't specify this property. The value is inherited from the source DB instance or snapshot.

If the documentation is up to date and the feature is missing, you could request it in the CloudFormation Public Coverage Roadmap.

You have mentioned that this feature is supported in DatabaseInstanceFromSnapshot. CloudFormation resource DBInstance doesn't have this restriction for property MasterUserPassword according to the documentation.

blimmer commented 2 years ago

To add more color to this issue, consider the following scenario.

First, you create a database instance like this:

const vpc = new Vpc(this, 'VPC')
new DatabaseCluster(this, 'DatabaseCluster', {
  engine: DatabaseClusterEngine.auroraPostgres({ version: AuroraPostgresEngineVersion.VER_13_4 }),
  instanceProps: {
    vpc
  }
})

This is great because it's easy to stand up a new RDS instance with a secrets-manager backed set of credentials: https://github.com/aws/aws-cdk/blob/789e8d2aaa0aefb6d17e4ebc0d56c17e9999add0/packages/%40aws-cdk/aws-rds/lib/cluster.ts#L498-L503

However, let's say we need to recover from a snapshot in the future. It's a common use-case to just swap out new DatabaseCluster with new DatabaseClusterFromSnapshot and a snapshot identifier, like this:

new DatabaseClusterFromSnapshot(this, 'DatabaseClusterFromSnapshot', {
  snapshotIdentifier: 'arn:aws:rds:us-east-1:12345678910:cluster-snapshot:rds:example-2022-04-12-09-26',
  engine: DatabaseClusterEngine.auroraPostgres({ version: AuroraPostgresEngineVersion.VER_13_4 }),
  instanceProps: {
    vpc
  }
})

However, using default settings in CDK, you'll also have your admin secrets deleted during this process. Here's a diff from one of my projects:

[-] AWS::SecretsManager::Secret DatabasecdkpipelineexampledevClusterSecretE1E64056 destroy
[-] AWS::SecretsManager::SecretTargetAttachment DatabasecdkpipelineexampledevClusterSecretAttachment8EBC0CAD destroy
[~] AWS::RDS::DBCluster CdkPipelinesPrototype-dev-us-east-1/Database/Database/cdk-pipeline-example-dev-Cluster DatabasecdkpipelineexampledevClusterC6927963 replace
 ├─ [-] MasterUserPassword
 │   └─ {"Fn::Join":["",["{{resolve:secretsmanager:",{"Ref":"DatabasecdkpipelineexampledevClusterSecretE1E64056"},":SecretString:password::}}"]]}
 ├─ [-] MasterUsername (requires replacement)
 │   └─ {"Fn::Join":["",["{{resolve:secretsmanager:",{"Ref":"DatabasecdkpipelineexampledevClusterSecretE1E64056"},":SecretString:username::}}"]]}

This is dangerous because, after the deletion window, the Database could become inaccessible because the admin username and passwords were deleted.

So, even if MaserUsername and MasterUserPassword should not be passed to the CloudFormation resource, it feels like there should be a way to retain the generated secret.

Workaround

To work around this issue, you can set the retention policy of the secret to Retain like this:

const dbAdminSecret = cluster.node.children.find((child) => child instanceof rds.DatabaseSecret) as
  | rds.DatabaseSecret
  | undefined

if (!dbAdminSecret) {
  return
}

dbAdminSecret.applyRemovalPolicy(RemovalPolicy.RETAIN)

However, this isn't ideal because the Secret is now orphaned and no longer part of the CDK stack.

Ideally, even when switching from DatabaseCluster to DatabaseClusterFromSnapshot, the credentials should be retained as a first-class Resource of the Stack.

cc @jumic and @skinny85 - because of the potential to lose admin access to the database, I think this ticket warrants reconsideration of the p2 classification.

C0en commented 2 years ago

Sorry! It's been a long time and I'm working on other projects so I'll have to dig in my memory a bit. Retaining the secret wouldn't be my first choice since it might have been changed since the snapshot you're trying to restore. Just resetting it the same way like the database instance from snapshot would be the simplest (and most consistent option in my opinion)

@jumic the problem with not passing the credentials is that you'll get the snapshot and the password might not match the one in the secrets manager...

ericzbeard commented 2 years ago

A bit more information for this one.

Let's say you leave the original cluster in place.

const cluster = new rds.DatabaseCluster(this, 'db', {
    engine: rds.DatabaseClusterEngine.auroraPostgres({ version: rds.AuroraPostgresEngineVersion.VER_13_6 }),
    instanceProps: {
    vpcSubnets: {
        subnetType: ec2.SubnetType.PUBLIC,
    },
    vpc,
    securityGroups: [sg],
    },
});

// Deploy the above first, then take a snapshot
// Let's assume there's a data corruption and you want to restore the snapshot
// Add the code below and attempt to deploy the stack

const creds = rds.Credentials.fromSecret(cluster.secret)
creds.password = cluster.secret.SecretValue

const restored = new rds.DatabaseClusterFromSnapshot(this, 'db-restored', {
    engine: rds.DatabaseClusterEngine.auroraPostgres({ version: rds.AuroraPostgresEngineVersion.VER_13_6 }),
    instanceProps: {
    vpcSubnets: {
        subnetType: ec2.SubnetType.PUBLIC,
    },
    vpc,
    securityGroups: [sg],
    snapshotIdentifier: 'mySnapshot',
    credentials: creds 
    },
});

This results in the password being set in the template:

"MasterUserPassword": {
     "Fn::Join": [
      "",
      [
       "{{resolve:secretsmanager:",
       {
        "Ref": "dbSecret8003E3A7"
       },
       ":SecretString:password::}}"
      ]
     ]
    },

And yet you get this error when attempting to deploy the change:

❌  TryCdkRdsRestoreStack failed: Error: The stack named TryCdkRdsRestoreStack failed to deploy: UPDATE_ROLLBACK_COMPLETE: Property MasterUserPassword cannot be empty.
github-actions[bot] commented 1 year ago

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

blimmer commented 1 year ago

This is still valid.