crossplane-contrib / crossplane-contrib/provider-sql
Role: password regenerated on restore can be permanently lost when privilege comparison fails in the same Update() call
- Dominant language
- Go
- Stars
- 154
- Forks
- 119
- Avg merge
- 6d 17h
- Merged PRs (30d)
- 8
Description
### What happened?
The "auto-regenerate role password on restore" feature from [#348](https://github.com/crossplane-contrib/provider-sql/pull/348) (v0.16.1) can regenerate and apply a new password in Postgres, then lose it permanently if a later step in the same `Update()` call fails.
In [`Update()`](https://github.com/crossplane-contrib/provider-sql/blob/fe39828c16548e9d8d9d5b061a29526fad7f4ffc/pkg/controller/cluster/postgresql/role/reconciler.go#L333-L367) (mirrored in `pkg/controller/namespaced/postgresql/role/reconciler.go`): `shouldResetPassword()` detects a restored role (`LastPasswordChange` nil, empty secret), runs `ALTER ROLE ... PASSWORD ...`, and sets [`mg.Status.AtProvider.LastPasswordChange`](https://github.com/crossplane-contrib/provider-sql/blob/fe39828c16548e9d8d9d5b061a29526fad7f4ffc/pkg/controller/cluster/postgresql/role/reconciler.go#L359) before the function has finished. The next step, [`changedPrivs(mg.Status.AtProvider.PrivilegesAsClauses, privs)`](https://github.com/crossplane-contrib/provider-sql/blob/fe39828c16548e9d8d9d5b061a29526fad7f4ffc/pkg/controller/cluster/postgresql/role/reconciler.go#L363), errors with `errComparePrivileges` if `PrivilegesAsClauses` is empty or shorter than desired ([guard](https://github.com/crossplane-contrib/provider-sql/blob/fe39828c16548e9d8d9d5b061a29526fad7f4ffc/pkg/controller/cluster/postgresql/role/reconciler.go#L186)), which happens whenever `.status` hasn't been observed yet. `Update()` returns early with an error, so `PublishConnection()` is never called and the new password never reaches the connection secret. The generic reconciler persists `mg.Status` regardless of the error, so `LastPasswordChange` is saved anyway. `shouldResetPassword()` won't fire again (no longer nil, and nothing sets `PasswordRotationTrigger` automatically), so the role is stuck with a password that's live in Postgres but recorded nowhere.
`PrivilegesAsClauses` arrives empty/short because a plain `client.Update()` earlier in the same reconcile can return a stale `.status` view, from either crossplane-runtime's `AddFinalizer` call (runs unconditionally on any object's first-ever reconcile) or this provider's own late-init persistence. The latter is deterministic: [`lateInit()`](https://github.com/crossplane-contrib/provider-sql/blob/fe39828c16548e9d8d9d5b061a29526fad7f4ffc/pkg/controller/cluster/postgresql/role/reconciler.go#L468-L501) backfills any of `Privileges.SuperUser`, `Inherit`, `CreateDb`, `CreateRole`, `Login`, `Replication`, `BypassRls`, or `ConnectionLimit` left `nil` on the spec, and reports `ResourceLateInitialized` whenever it does. A `Role` spec that doesn't set all of these hits this path on nearly every reconcile.
### How can we reproduce it?
1. Create a `Role` with `managementPolicies` excluding `Delete`, leaving `bypassRls`/`connectionLimit`/the privilege booleans unset (so late-init has something to backfill each reconcile).
2. Wait for it to be `Ready`, then delete and immediately recreate the object (fresh `.status`, external role still exists).
3. Check the connection secret: `password` comes back empty while `status.atProvider.lastPasswordChange` is set.
4. Check events for `cannot update role: cannot compare desired and observed privileges`.
### What environment did it happen in?
provider-sql version: `v0.16.1`
Crossplane version: `v2.2.0`
### Suggested fix
- Don't persist `LastPasswordChange` until `Update()` fully succeeds. Move the assignment to just before the function's final successful return.
- Make `changedPrivs` tolerate an empty/short `existing` instead of erroring. Treat "no prior observation yet" as "apply the full desired list."
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in pkg/controller/cluster/postgresql/role/reconciler.go, especially Update(), shouldResetPassword(), changedPrivs(), and lateInit(); inspect the mirrored namespaced role reconciler as well. Reproduce the restore and stale-status sequence described in the issue. Done means a failed update cannot persist LastPasswordChange prematurely, and missing privilege observations no longer lose the regenerated password or block connection-secret publication.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, postgresql
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100