(docdb): Updating secret name or switching from an auto-generated secret to an imported one requires cluster replacement even though it could be updated without replacement

[Booligoosh]

Describe the bug

When updating the masterUser.secretName field of docdb.DatabaseCluster, or switching from an auto-generated secret without masterUser.password to importing one via masterUser.password, the cluster needs to be replaced on deploy. This is because the MasterUsername field in the CFN changes, since it references a secret value. However, the construct already knows the username from the masterUser.username prop, so this replacement could be avoided by just setting MasterUsername directly as a string rather than a secret value lookup.

Expected Behavior

Updating masterUser.secretName on a docdb.DatabaseCluster or adding masterUser.password should not require replacement of the cluster.

Current Behavior

Updating masterUser.secretName on a docdb.DatabaseCluster or adding masterUser.password requires replacement of the cluster.

Reproduction Steps

1. Create a DocDB cluster and deploy
2. Change the secretName and deploy again
3. Observe that a CFN replacement of the cluster is triggered

Possible Solution

This issue could be fixed by changing this line:

-       masterUsername: secret ? secret.secretValueFromJson('username').unsafeUnwrap() : props.masterUser.username,
+       masterUsername: props.masterUser.username,

This would mean that even if the secret name is changed or a custom secret is used, the value for the CFN MasterUsername field would remain the same as long as masterUser.username remains the same, meaning no replacement would be required.

Unfortunately, this would be a breaking change as it would change the value of the MasterUsername field for existing clusters from the secret lookup to the string value, meaning existing clusters would require replacement after upgrading the CDK version. Because of this, it may only be able to be implemented in a DatabaseClusterV2 construct, but this issue should be kept in mind if such a construct is ever developed.

Additional Information/Context

No response

CDK CLI Version

2.115.0

Framework Version

No response

Node.js Version

20.11.1

OS

Windows 10 Enterprise

Language

TypeScript

Language Version

No response

Other information

No response

[pahud]

Yes this makes sense if props.masterUser.username is provided we should not update that prop.

Unfortunately, this would be a breaking change as it would change the value of the MasterUsername field for existing clusters from the secret lookup to the string value, meaning existing clusters would require replacement after upgrading the CDK version

I think we still can do it under a feature flag. Are you interested to submit a PR for that? I will request for more input from the maintainers about the feature flag.

[Booligoosh]

Ah awesome, yes I'd be interested in submitting a PR, if this is something that the team is happy to accept. I can use a temporary flag name for now until you get input.

[GavinZZ]

Hi @Booligoosh, we welcome community contribution and would appreciate if you can draft a PR for the fix. Using feature flag would make sense here as we do not want to bring breaking changes to existing customers. Here's a guide on how to work with feature flags: https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#feature-flags

Feel free to put up a PR and tag me for review. Will try to review ASAP.