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: DatabaseClusterFromSnapshot creates a new secret when using SnapshotCredentials.fromSecret() #23815

Closed
jheino opened this issue Jan 24, 2023 · 6 comments · Fixed by #27174
Assignees
Labels
@aws-cdk/aws-rds Related to Amazon Relational Database bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@jheino
Copy link

jheino commented Jan 24, 2023

Describe the bug

If I create a new secret in a stack and provide it to rds.DatabaseCluster using the property credentials: rds.Credentials.fromSecret(secret), the secret is attached correctly to the new database cluster.

However, if I provide the same secret to rds.DatabaseClusterFromSnapshot using the property snapshotCredentials: rds.SnapshotCredentials.fromSecret(secret), a new additional secret is also created and attached to the database cluster.

Expected Behavior

rds.DatabaseClusterFromSnapshot uses the secret provided in snapshotCredentials and does not create any additional resources.

Current Behavior

rds.DatabaseClusterFromSnapshot creates a new secret even when snapshotCredentials refers to an existing secret. Two AWS::SecretsManager::SecretTargetAttachment resources are created, one using the provided existing secret and one using the newly created unnecessary secret.

Reproduction Steps

This minimal stack reproduces the problem:

import * as cdk from "aws-cdk-lib";
import { aws_secretsmanager as secretsmanager } from "aws-cdk-lib";
import { aws_ec2 as ec2 } from "aws-cdk-lib";
import { aws_rds as rds } from "aws-cdk-lib";
import { Construct } from "constructs";

export class CdkSandboxStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const vpc = new ec2.Vpc(this, "VPC", {
      natGateways: 0
    });

    const secret = new secretsmanager.Secret(this, "JSONSecret", {
      generateSecretString: {
        secretStringTemplate: JSON.stringify({ username: "Admin" }),
        excludePunctuation: true,
        includeSpace: false,
        passwordLength: 24,
        generateStringKey: "password",
      },
    });

    // Attaching the existing secret to a new cluster works fine
    /*
    const cluster = new rds.DatabaseCluster(this, "Database", {
      engine: rds.DatabaseClusterEngine.auroraMysql({
        version: rds.AuroraMysqlEngineVersion.VER_3_02_1,
      }),
      credentials: rds.Credentials.fromSecret(secret),
      instanceProps: {
        vpcSubnets: {
          subnetType: ec2.SubnetType.PRIVATE_ISOLATED,
        },
        vpc,
      },
    });
    */

    // Attaching the existing secret to a new cluster will create a new DatabaseSecret instead
    const cluster = new rds.DatabaseClusterFromSnapshot(this, "Database", {
      snapshotIdentifier: "test-snapshot-for-cdk",
      engine: rds.DatabaseClusterEngine.auroraMysql({
        version: rds.AuroraMysqlEngineVersion.VER_3_02_1,
      }),
      snapshotCredentials: rds.SnapshotCredentials.fromSecret(secret),
      instanceProps: {
        vpcSubnets: {
          subnetType: ec2.SubnetType.PRIVATE_ISOLATED,
        },
        vpc,
      },
    });
  }
}

When running cdk diff, the resource list output will contain two Secrets/Attachments, not one as expected:

Resources
...
[+] AWS::SecretsManager::Secret JSONSecret JSONSecret6FE68AEF
[+] AWS::SecretsManager::SecretTargetAttachment JSONSecret/Attachment JSONSecretAttachmentCB690F4F
...
[+] AWS::SecretsManager::Secret Database/Secret DatabaseSecret3B817195
[+] AWS::SecretsManager::SecretTargetAttachment Database/Secret/Attachment DatabaseSecretAttachmentE5D1B020
...

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.61.1 (build d319d9c)

Framework Version

No response

Node.js Version

v18.13.0

OS

macOS Monterey (arm64)

Language

Typescript

Language Version

TypeScript (4.9.4)

Other information

No response

@jheino jheino added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 24, 2023
@github-actions github-actions bot added the @aws-cdk/aws-rds Related to Amazon Relational Database label Jan 24, 2023
@pahud
Copy link
Contributor

pahud commented Jan 30, 2023

Thanks for your report. I will try reproduce this in my account.

@pahud pahud added investigating This issue is being investigated and/or work is in progress to resolve the issue. needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Jan 30, 2023
@pahud pahud self-assigned this Jan 30, 2023
@yantsipun
Copy link

Having the same issue.

      return new DatabaseClusterFromSnapshot(this, 'Database', {
        ...clusterProps,
        snapshotIdentifier,
        snapshotCredentials:
          snapshotCredentials ??
          SnapshotCredentials.fromGeneratedSecret('postgres'),
      })

After this I'm dumping the Secret ARN into SSM by using cluster.secret.secretFullArn.
However, two pairs of "AWS::SecretsManager::SecretTargetAttachment" and "AWS::SecretsManager::Secret" resources are created after.
Two secrets are containing different passwords and I can access the cluster with the secret, ARN of which I dumped to SSM.

"aws-cdk-lib": "2.38.1"

@pahud
Copy link
Contributor

pahud commented Mar 15, 2023

Yes I can reproduce this bug:
image

As DatabaseClusterFromSnapshot extends DatabaseClusterNew, which always runs renderCredentials() to generate a new secret if props.credentials is not provided. I guess this is something we should fix.

@pahud pahud added p1 effort/medium Medium work item – several days of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. needs-reproduction This issue needs reproduction. labels Mar 15, 2023
@pahud pahud removed their assignment Mar 15, 2023
@colifran colifran self-assigned this May 1, 2023
@CptBalistico
Copy link

CptBalistico commented Jul 18, 2023

Out of curiosity, is there any update on this issue? We would like to update our version of the library, but newer versions have deprecated .credentials(Credentials.fromSecret(...)).

Since secret rotation is mandatory for us, but since we have no control over the second, extra secret resulting from using .snapshotCredentials(SnapshotCredentials.fromSecret(...)), this is currently not possible.

@colifran
Copy link
Contributor

@dominiquems I'm looking into this to see what we can do to prevent the extra secret from being created.

@mergify mergify bot closed this as completed in #27174 Sep 25, 2023
mergify bot pushed a commit that referenced this issue Sep 25, 2023
…tabase cluster from a snapshot (under feature flag) (#27174)

This PR fixes a bug where an extra database secret is being generated when an RDS database cluster is being created from a snapshot.

On the `DatabaseClusterFromSnapshotProps` interface, we deprecated the `credentials` property and, at the same, introduced `snapshotCredentials` as the recommended replacement. However, the default behavior associated with the `credentials` property was not removed as doing so would introduce a breaking change for some users as detailed in this [PR](#20777). As a result, users just using the recommended `snapshotCredentials` property to create a new RDS database cluster are seeing an extra, unwanted secret being created.

Closes #23815

----

*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.

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 bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
5 participants