feat(rds): add support for manageMasterUserPassword in L2 construct

[epoctic]

Issue # (if applicable)

Closes #29239

Reason for this change

In order to make the properties in the manageMasterUserPassword of the L1 Construct CfnDBCluster configurable in the L2 Construct.

Even when using a CFN override to set manageMasterUserPassword, the L2 cluster construct would create an additional unused secret. This PR adds native support for the manageMasterUserPassword CloudFormation attribute and eliminates the extra secret that the workaround caused.

Description of changes

To ensure existing clusters wouldn't have a new cfn property added and set to false, I had to use a few conditional statements. I also wanted to make sure that the created secret supported a customer defined username and kms key.

Description of how you validated changes

I deployed a database cluster using the new code and logged into it using the managed password with custom username. I also manually triggered rotations on the secret and ensured that the newly generated secret also worked for logging into the database.

Checklist

• [ x] My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[lpizzinidev]

Thanks 👍 Left some comments for adjustments.
Also, can you please add unit test coverage?

[lpizzinidev]

From docs.

Can't manage the master user password with AWS Secrets Manager if MasterUserPassword is specified.

We should add validation instead of simply ignoring the specified password.

Valid for Cluster Type: Aurora DB clusters and Multi-AZ DB clusters

Can we validate that the RDS cluster is Multi-AZ if manageMasterUserPassword: true?

[epoctic]

Does the L2 DatabaseCluster construct support non-Aurora engines? The DatabaseClusterEngine appears to only support Aurora, so I don't think that the multi-AZ check is required. I'm also sure that manageMasterUserPassword works on a RDS cluster with a single instance when the db engine is Aurora based.

[a-bigelow]

The non Aurora IClusterEngine variants are supported by the construct (you need to implement them yourself), but the mysql and postgres engines are both "Multi-AZ DB clusters". As far as I can tell, the only way to deploy those two engines not as "Multi-AZ DB clusters" is to use DatabaseInstance instead of this construct.

[epoctic]

Thanks for the clarification! Based on that information I believe that a specific set of checks for a multi-AZ cluster wouldn't be necessary.

[epoctic]

Thanks 👍 Left some comments for adjustments. Also, can you please add unit test coverage?

Done, sorry about the wait!

[lpizzinidev]

Can we remove the if statement given the above validation?

      const credentials = renderCredentials(this, props.engine, props.credentials);
      const secret = credentials.secret;

      const cluster = new CfnDBCluster(this, 'Resource', {
        ...this.newCfnProps,
        // Admin
        masterUsername: credentials.username,
        masterUserPassword: credentials.password?.unsafeUnwrap(),
        // Define kms key the RDS Managed Secret will use.
        ...props.manageMasterUserPassword && credentials.encryptionKey && {
          masterUserSecret: {
            kmsKeyId: credentials.encryptionKey.keyId,
          },
        },
      });

      this.clusterIdentifier = cluster.ref;
      this.clusterResourceIdentifier = cluster.attrDbClusterResourceId;

      if (secret) {
        this.secret = secret.attach(this);
      }

[epoctic]

We cannot unfortunately. The only way renderCredentials doesn't create a new secret is if the credentials prop is passed the output from a Credentials.[method] call with a SecretsManager Secret object, or a password that's an instance of SecretValue. Since we disallow password and secret from being passed as props to credentials, the renderCredential function would always create a new secret. Not desirable.

[lpizzinidev]

I see, thanks for clarifying 👍
We could remove a bit of duplication by moving

      this.clusterIdentifier = cluster.ref;
      this.clusterResourceIdentifier = cluster.attrDbClusterResourceId;

after the if statement block.
Also, note the other comment on unit tests.

[epoctic]

Good catch, I'll get that changed, thanks!

[epoctic]

I see, thanks for clarifying 👍 We could remove a bit of duplication by moving

      this.clusterIdentifier = cluster.ref;
      this.clusterResourceIdentifier = cluster.attrDbClusterResourceId;

after the if statement block. Also, note the other comment on unit tests.

I just tested this change, and it can't be done. A secret is created in one of the if blocks, and the secret needs those values to be set, so the lines must be duplicated no matter what.

[lpizzinidev]

We could unify these test cases:

test.each([
  ['excludeCharacters', { excludeCharacters: '1234' }],
  ['secret', { secret: new sm.Secret(stack, 'secret') }],
])('throw error for setting `manageMasterUserPassword` to true while `credentials.%s` is defined', (_, credentials) => { ... }

[epoctic]

We could unify these test cases:

test.each([
  ['excludeCharacters', { excludeCharacters: '1234' }],
  ['secret', { secret: new sm.Secret(stack, 'secret') }],
])('throw error for setting `manageMasterUserPassword` to true while `credentials.%s` is defined', (_, credentials) => { ... }

Sorry about the wait! It took a bit to work through how I could define an external stack and still keep resources unique.

[epoctic]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 02cebb7
• Result: FAILED
• Build Logs (available for 30 days)

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

This failure doesn't seem to be related to code I've committed.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 744e065
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[Hawxy]

Shouldn't this.secret be set by CfnDBCluster's masterUserSecret.secretArn? I don't see a direct way to access the created secret for use within CDK.