fix(rds): issue 1121 Fix restore rdsinstance passwords set

[dee0sap]

Description of your changes

[Currrently, 2023-09-27 20:15 Pacific TZ this is a strawman/draft/wip PR]

We found RDSInstance:restoreFrom snapshot isn't useful 'out of the box' if you didn't know the password of the RDS that was the source of the snapshot. Instead you have to take an extra step to set the password after the RDS is provisioned.

When creating an RDS the provider is able to pass the password to the AWS api. So the password is set 'out of the box'. However the AWS restore function doesn't take a password parameter and currently the provider makes no effort to set the password in the case of restore.

This change adds a PasswordSet condition which is used to track whether the provider has managed to set the master password yet or not. Necessary cause it generally isn't possible to set the RDS password immediately after the call to the AWS restore function.

The condition is set to False with a reason of Pending when the provider sees the password needs to be set. This happens in

• Create
• Update, if a password change has been detected
  The condition is set to False with a reason of Failed if an attempt to set the password was made but it failed. This happens in
• CreateOrRestore, where an attempt to set the password is made immediately after the call to the AWS restore function
• Update, where an attempt to modify DB is made after changes are detected
  The condition is set to True with a reason of PasswordSet if the password was successfully set. This happens in
• CreateOrRestore, after successful creation of a new RDS 'from scratch'
• CreateOrRestore, where an attempt to set the password is made immediately after the call to the AWS restore function
• Update, where an attempt to modify DB is made after changes are detected

The IsUpToDate function, called by Observe, now takes into account the value of the PasswordSet condition when determining if there was a password change. If the existing checks don't indicate a password change but PasswordSet == False then IsUpTodate concludes that there was a password change and IsUpdateToDate returns false.

Update's password change check has has been updated similar to IsUpToDate. Additionally it has been updated to use GeneratePassword like Create does.

Fixes #1121

I have:

• [ x] Read and followed Crossplane's contribution process.
• [ X] Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

After updating condition checks all existing tests pass.
I have performed manual tests.
TODO: Add unit tests specifically for the new logic that was added.

[MisterMX]

I think this solution is quite complex and I wonder if password handling for RDSInstance could be done in same way as it was done in #1756. Instead of using the spec.writeConnectionSecretToRef to cache the initial password an internal cache secret is used. If this does not exist a password update is always performed. I think that would also fix the issue with automatic password resets after restore.

[dee0sap]

Thanks @MisterMX for taking the time to look at this.
I'll look at that PR.

However a comment you made makes me think that perhaps you are misunderstanding what this PR does. This PR isn't using writeConnectionSecretToRef to cache anything. Rather it is just

• Using a condition to track whether the password needs setting or not
• Changing Update to check the condition and extending the password setting code in Update to use Generate if that makes sense.

[MisterMX]

Yep, I got that. But I think it is better to use the same password mechanic that is used in #1756: Store the set password in a cache and compare it with what the user wants to set. Relying on a status condition is more obscure and harder to understand imo. It is also inconsistent to rds.aws.crossplane.io.

[dee0sap]

Hey @MisterMX
I disagree with the assessment that this is obscure :

• It is modeled after changes that @negz made when resolving some issues for for us ( Leaked RouteTable due to eventually consistent DescribeRouteTables API #802, Give all EC2 managed resources a 3 minute creation grace period #825, Account for two different kinds of consistency issues crossplane/crossplane-runtime#283 but I use a condition rather than an annotation ) .
• Using a condition allows us to specifically watch for the password to be set. ( Fwiw, when restoring the RDS may be Ready, Synced and Available and yet the password you need isn't available )

However I do see that the official provider is working like the PR you referenced for the type instance ( which is the replacement of the community-contrib's rdsintance ). So I think I'll create a 2nd PR which follows the approach in the PR you referenced.

And again, thanks for taking the time to review this PR. I'll bug the provider-aws maintainers again once that 2nd PR is ready.

[dee0sap]

Oh @MisterMX forgot...

Today rdsinstance doesn't have an autoGeneratePassword field under forProvider. Do I need to increase the CRD version from v1beta1 to v1beta2 ?

[dee0sap]

@MisterMX
Today RDSInstance, despite the fact that it doesn't have the field AutogeneratePassword, will automatically generate a password into writeConnectionSecretToRef if nothing has been specified for masterPasswordSecretRef.

If we're going to add AutoGeneratePassword how do you see us breaking expectations for current users? e.g. If neither AutoGeneratePassword nor masterPasswordSecretRef we retain the current behaviour but if AutoGeneratePassword is true then you have to specify masterPasswordSecretRef? And if it is explicitly set to false then we refuse to generate a password?

[dee0sap]

@MisterMX

Looking at
https://github.com/crossplane-contrib/provider-aws/blob/master/pkg/controller/rds/dbinstance/setup.go#L122
in DBInstance AutogeneratePassword won't do what people expect since it seems a password will only be generated if you are also doing restoreFrom.

Adding such behaviour to RDSInstance will be a surprise because to auto password generation happens without requiring use of restoreFrom.

[github-actions]

Crossplane does not currently have enough maintainers to address every issue and pull request. This pull request has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Adding a comment starting with /fresh will mark this PR as not stale.