Fix Issue 227: Generate new PostgreSQL password for role after restoration

[JevgeniF]

Description of your changes

Fixes #227: The change adds an optional boolean field, passwordReset, to the Role spec. This field changes how the provider behaves when it encounters an empty Secret resource after a restoration:

If passwordReset is true and the Secret is empty, the provider generates a new password for the role and populates the Secret.

If passwordReset is false or omitted, the provider acts as usual and leaves the role's password unchanged (meaning the Secret may simply stay empty).

I have:

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

How has this code been tested

Unit tests written to test passwordResetToken work.
Manual test in AWS cluster.

A test linux_amd64 package for you reference can be found in Docker Hub:
docker pull entigolabs/provider-sql:test
Package digest: sha256:6b41c2bc13fc23ecd13f9461d3beeb3d38af3990c6d3c1b75167ad0260382d05

[fernandezcuesta]

@JevgeniF thanks for your contribution!

I like the idea, wondering if it can be extended to become a sort of either:

• autogenerate password
• or bring your own password, in which case the connection secret is redundant

I did some related work in another provider for a User resource in provider-clickhousedbops

[JevgeniF]

Dear @fernandezcuesta,

Your idea to bring BYOP to this project is great, but I’m not sure it should be solved within the context of this specific fix.
The goal of this fix is to allow users to use an already existing PostgreSQL Role (one restored from a snapshot). Currently, we have to delete the old user and create a new one just to get a properly populated Secret, which is why we need this fix so urgently.
Maybe the BYOP feature can be moved into a separate ticket? I’d be happy to see what I can do there.

[fernandezcuesta]

Alternative implementation, considering that:

• passwordSecretRef implements BYOP (and this trigger should have no effect)
• Set a timestamp indicating when you want the password to be reset, so it somehow changes the logic of "reset the password"

(missing: update in role_types to add PasswordRotationTrigger in roleparameters and LastPasswordChange in RoleObservation + tests)

⚠️ take that just as an idea, I'm not pretending you to change that, but just bring more ideas.

[fernandezcuesta]

Suggested change

	if pw == "" {
	pw, err = password.Generate()
	if err != nil {
	return managed.ExternalUpdate{}, err
	}
	}

[JevgeniF]

Returned it back, because now - i.e., when we need to reset a password in the non-BYOP case - c.getPassword returns "", true, nil, and we need to generate the password before the ALTER ROLE ... PASSWORD ... query in the database."

[fernandezcuesta]

Suggested change

	func (c *external) shouldResetPassword(ctx context.Context, role *v1alpha1.Role) (bool, error) {
	if role.Spec.ForProvider.PasswordReset == nil || !*role.Spec.ForProvider.PasswordReset {
	return false, nil
	}
	if role.Spec.WriteConnectionSecretToReference == nil {
	return newPwd, false, nil
	return false, nil
	}
	nn = types.NamespacedName{
	nn := types.NamespacedName{
	Name: role.Spec.WriteConnectionSecretToReference.Name,
	Namespace: role.Spec.WriteConnectionSecretToReference.Namespace,
	}
	s = &corev1.Secret{}
	// the output secret may not exist yet, so we can skip returning an
	// error if the error is NotFound
	s := &corev1.Secret{}
	if err := c.kube.Get(ctx, nn, s); resource.IgnoreNotFound(err) != nil {
	return "", false, err
	return false, err
	}
	// if newPwd was set to some value, compare value in output secret with
	// newPwd
	changed = newPwd != "" && newPwd != string(s.Data[xpv1.ResourceCredentialsSecretPasswordKey])
	return newPwd, changed, nil
	return len(s.Data[xpv1.ResourceCredentialsSecretPasswordKey]) == 0, nil
	}
	func (c *external) shouldResetPassword(ctx context.Context, role *v1alpha1.Role) bool {
	if role.Spec.ForProvider.PasswordSecretRef != nil {
	return false
	}
	trigger := role.Spec.ForProvider.PasswordReset
	if trigger == nil {
	return false
	}
	last := role.Status.AtProvider.LastPasswordChange
	if last == nil {
	return true
	}
	return trigger.After(last.Time)
	}

[JevgeniF]

Kept PasswordReset as *bool, because it should fire once when LastPasswordChange is nil (meaning the provider has never set a password for this role), not on a schedule (as in our issue, when we restore database with roles, but provider doesn't generate password).

For time-based rotation I added a separate PasswordRotationTrigger *metav1.Time field, which does use trigger.After(last.Time) in shouldResetPassword. So both use cases are covered:
-- PasswordReset: true - fires once when LastPasswordChange == nil
-- PasswordRotationTrigger - fires whenever the trigger time is after LastPasswordChange

[JevgeniF]

@fernandezcuesta maybe I got your idea wrong and made wrong implementation. Please look again and build and test if BYOP and password rotation works as you wanted.

New field added to RoleParameters:
-- PasswordReset *bool
-- PasswordRotationTrigger *metav1.Time - rotation trigger for auto-generated passwords. Fires whenever the trigger time is after LastPasswordChange.

Both PasswordReset and PasswordRotationTrigger have no effect when passwordSecretRef is set - BYOP rotation behavior unchanged.

A new LastPasswordChange *metav1.Time status field is added to RoleObservation. It is set by the provider after every successful password change (both on create and update) and is used by shouldResetPassword to decide whether action is needed.

shouldResetPassword is refactored from (ctx, role) (bool, error) to (role) bool - no kube call required since the decision is based on fields already present in the role object.

[fernandezcuesta]

Hey, thanks for considering it and your explanation. I'm wondering if both fields aren't a bit redundant. I mean, with a timestamp-alike field, can't you satisfy the scenario in which you lost the password and want to have it immediately recreated? In the case of a "past" time, wouldn't it act similar to PasswordReset?

[JevgeniF]

@fernandezcuesta you are totally right. I was overthinking and left our simple (legacy) logic in parallel with your solution, but PasswordRotationTrigger indeed covers both cases. Furthermore, If we set LastPasswordChange in Create() method, we can trigger password change after database restoration by default, without PasswordRotationTrigger use.

To sum up...
Current logic for password rotation (BYOP - PasswordSecretRef exists):

1. The provider generates its own connection secret based on the referenced user secret (note: writeConnectionSecretToRef must be configured).
2. When a user rotates the password in the source secret, the provider detects the difference between the source and the managed connection secret and triggers an update.

Am I right?

Updated logic for non-BYOP (PasswordSecretRef not set), considering my proposition to keep it simple on LastPasswordChange basis.

For new role creation:

1. The provider generates a secret using Create() function and sets the LastPasswordChange timestamp in the resource status.
2. During subsequent observations, shouldResetPassword returns false if PasswordRotationTrigger is either nil or set to a timestamp in future in comparison with LastPasswordChange. It returns true only if PasswordRotationTrigger is older than LastPasswordChange.
3. if PasswordRotationTrigger is older than LastPasswordChange, the provider updates password and sets new LastPasswordChange value.

For restoration scenarios:

1. The provider observes that the role already exists in the database and bypasses the Create() function. Consequently, LastPasswordChange remains nil in the new resource status.
2. A nil value for LastPasswordChange forces shouldResetPassword to true, prompting the provider to call the Update() function.
3. The provider updates the database password, populates the secret and sets LastPasswordChange .
4. From this point, PasswordRotationTrigger functions normally, triggering a change only when its timestamp is more older than LastPasswordChange.

Looks simplier and works in any scenario:
provider will automatically change password for restored role.
user can reset password manually by changing PasswordRotationTrigger

[fernandezcuesta]

Yes I guess that's correct

[JevgeniF]

@fernandezcuesta OK. Then I push updated version tomorrow. Thank you!

[JevgeniF]

Some comments.
I made password reset by default in case if role created and password in connection secret remains empty.
I also removed lastPasswordChange update fromCreate() function (which I added in previous version). Now the field populated only on password change (on Update())

Current flows:
Normal role creation (non-BYOP):
Role doesn't exist in DB yet. Provider creates it with a generated password and publishes the connection secret. On the next reconcile, lastPasswordChange is nil but the connection secret already has a password - provider knows Create() already ran and leaves it alone.

Role restoration (non-BYOP):
Role already exists in DB (restored from snapshot), but the Role resource is new so there's no connection secret or it is empty. Provider sees lastPasswordChange nil and empty secret - provider gets this as trigger for password generation - generates a new one, sets it on the role in DB via ALTER ROLE, writes the connection secret, stamps lastPasswordChange.

Password rotation (non-BYOP):
passwordRotationTrigger: triggers new password generation on comparison with lastPasswordChange basis.

New inux_amd64 test package:
docker pull entigolabs/platform-apis-test:provider-sql-test-v4
Package digest: sha256:8cc37ccf50c6071587dff2b01da0dcd003637982f63c4a0ea1bc0e8f95ddb983