From 9da341135df73c0e390dbe079cd7461b86875298 Mon Sep 17 00:00:00 2001 From: Youssef Omar Bouden Date: Fri, 16 Oct 2026 23:28:25 +0100 Subject: [PATCH 1/2] fix(postgresql): compare role values, not pointers, in upToDate upToDate compared ConnectionLimit and the privilege fields as pointers. lateInit copies the observed pointers into the spec once, but every later reconcile arrives with fresh pointers from the API server, so an unchanged Role was never up to date and Update re-issued ALTER ROLE CONNECTION LIMIT on every poll. The fields are compared by value now, in the cluster and the namespaced controller. Fixes #487 Signed-off-by: Youssef Omar Bouden --- .../cluster/postgresql/role/reconciler.go | 16 ++--- .../postgresql/role/reconciler_test.go | 58 ++++++++++++++++ .../namespaced/postgresql/role/reconciler.go | 16 ++--- .../postgresql/role/reconciler_test.go | 68 +++++++++++++++++++ 4 files changed, 142 insertions(+), 16 deletions(-) diff --git a/pkg/controller/cluster/postgresql/role/reconciler.go b/pkg/controller/cluster/postgresql/role/reconciler.go index 2da5d98f..0506b33d 100644 --- a/pkg/controller/cluster/postgresql/role/reconciler.go +++ b/pkg/controller/cluster/postgresql/role/reconciler.go @@ -434,28 +434,28 @@ func (c *external) Delete(ctx context.Context, mg *v1alpha1.Role) (managed.Exter } func upToDate(observed *v1alpha1.RoleParameters, desired *v1alpha1.RoleParameters) bool { - if observed.ConnectionLimit != desired.ConnectionLimit { + if !cmp.Equal(observed.ConnectionLimit, desired.ConnectionLimit) { return false } - if observed.Privileges.SuperUser != desired.Privileges.SuperUser { + if !cmp.Equal(observed.Privileges.SuperUser, desired.Privileges.SuperUser) { return false } - if observed.Privileges.Inherit != desired.Privileges.Inherit { + if !cmp.Equal(observed.Privileges.Inherit, desired.Privileges.Inherit) { return false } - if observed.Privileges.CreateDb != desired.Privileges.CreateDb { + if !cmp.Equal(observed.Privileges.CreateDb, desired.Privileges.CreateDb) { return false } - if observed.Privileges.CreateRole != desired.Privileges.CreateRole { + if !cmp.Equal(observed.Privileges.CreateRole, desired.Privileges.CreateRole) { return false } - if observed.Privileges.Login != desired.Privileges.Login { + if !cmp.Equal(observed.Privileges.Login, desired.Privileges.Login) { return false } - if observed.Privileges.Replication != desired.Privileges.Replication { + if !cmp.Equal(observed.Privileges.Replication, desired.Privileges.Replication) { return false } - if observed.Privileges.BypassRls != desired.Privileges.BypassRls { + if !cmp.Equal(observed.Privileges.BypassRls, desired.Privileges.BypassRls) { return false } if !cmp.Equal(observed.ConfigurationParameters, desired.ConfigurationParameters, diff --git a/pkg/controller/cluster/postgresql/role/reconciler_test.go b/pkg/controller/cluster/postgresql/role/reconciler_test.go index 263d2847..a912ee3a 100644 --- a/pkg/controller/cluster/postgresql/role/reconciler_test.go +++ b/pkg/controller/cluster/postgresql/role/reconciler_test.go @@ -278,6 +278,64 @@ func TestObserve(t *testing.T) { err: nil, }, }, + "SameValuesDifferentPointers": { + reason: "We should return ResourceUpToDate=true when the observed values equal the spec through different pointers", + fields: fields{ + db: mockDB{ + MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { + *(dest[7].(**int32)) = ptr.To(int32(-1)) + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Role{ + Spec: v1alpha1.RoleSpec{ + ForProvider: v1alpha1.RoleParameters{ + Privileges: v1alpha1.RolePrivilege{}, + ConnectionLimit: ptr.To(int32(-1)), + }, + }, + }, + }, + want: want{ + o: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: true, + ResourceLateInitialized: true, + }, + err: nil, + }, + }, + "ConnectionLimitChanged": { + reason: "We should return ResourceUpToDate=false if the connection limit differs from the observed one", + fields: fields{ + db: mockDB{ + MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { + *(dest[7].(**int32)) = ptr.To(int32(-1)) + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Role{ + Spec: v1alpha1.RoleSpec{ + ForProvider: v1alpha1.RoleParameters{ + Privileges: v1alpha1.RolePrivilege{}, + ConnectionLimit: ptr.To(int32(5)), + }, + }, + }, + }, + want: want{ + o: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: false, + ResourceLateInitialized: true, + }, + err: nil, + }, + }, "PasswordChanged": { reason: "We should return ResourceUpToDate=false if the password changed", fields: fields{ diff --git a/pkg/controller/namespaced/postgresql/role/reconciler.go b/pkg/controller/namespaced/postgresql/role/reconciler.go index 41990982..312deef0 100644 --- a/pkg/controller/namespaced/postgresql/role/reconciler.go +++ b/pkg/controller/namespaced/postgresql/role/reconciler.go @@ -417,28 +417,28 @@ func (c *external) Delete(ctx context.Context, mg *namespacedv1alpha1.Role) (man } func upToDate(observed *namespacedv1alpha1.RoleParameters, desired *namespacedv1alpha1.RoleParameters) bool { - if observed.ConnectionLimit != desired.ConnectionLimit { + if !cmp.Equal(observed.ConnectionLimit, desired.ConnectionLimit) { return false } - if observed.Privileges.SuperUser != desired.Privileges.SuperUser { + if !cmp.Equal(observed.Privileges.SuperUser, desired.Privileges.SuperUser) { return false } - if observed.Privileges.Inherit != desired.Privileges.Inherit { + if !cmp.Equal(observed.Privileges.Inherit, desired.Privileges.Inherit) { return false } - if observed.Privileges.CreateDb != desired.Privileges.CreateDb { + if !cmp.Equal(observed.Privileges.CreateDb, desired.Privileges.CreateDb) { return false } - if observed.Privileges.CreateRole != desired.Privileges.CreateRole { + if !cmp.Equal(observed.Privileges.CreateRole, desired.Privileges.CreateRole) { return false } - if observed.Privileges.Login != desired.Privileges.Login { + if !cmp.Equal(observed.Privileges.Login, desired.Privileges.Login) { return false } - if observed.Privileges.Replication != desired.Privileges.Replication { + if !cmp.Equal(observed.Privileges.Replication, desired.Privileges.Replication) { return false } - if observed.Privileges.BypassRls != desired.Privileges.BypassRls { + if !cmp.Equal(observed.Privileges.BypassRls, desired.Privileges.BypassRls) { return false } if !cmp.Equal(observed.ConfigurationParameters, desired.ConfigurationParameters, diff --git a/pkg/controller/namespaced/postgresql/role/reconciler_test.go b/pkg/controller/namespaced/postgresql/role/reconciler_test.go index 36769a48..9db8db68 100644 --- a/pkg/controller/namespaced/postgresql/role/reconciler_test.go +++ b/pkg/controller/namespaced/postgresql/role/reconciler_test.go @@ -335,6 +335,74 @@ func TestObserve(t *testing.T) { err: nil, }, }, + "SameValuesDifferentPointers": { + reason: "We should return ResourceUpToDate=true when the observed values equal the spec through different pointers", + fields: fields{ + db: mockDB{ + MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { + *(dest[7].(**int32)) = ptr.To(int32(-1)) + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Role{ + Spec: v1alpha1.RoleSpec{ + ForProvider: v1alpha1.RoleParameters{ + Privileges: v1alpha1.RolePrivilege{}, + ConnectionLimit: ptr.To(int32(-1)), + }, + }, + Status: v1alpha1.RoleStatus{ + AtProvider: v1alpha1.RoleObservation{ + LastPasswordChange: &metav1.Time{Time: metav1.Now().Time}, + }, + }, + }, + }, + want: want{ + o: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: true, + ResourceLateInitialized: true, + }, + err: nil, + }, + }, + "ConnectionLimitChanged": { + reason: "We should return ResourceUpToDate=false if the connection limit differs from the observed one", + fields: fields{ + db: mockDB{ + MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { + *(dest[7].(**int32)) = ptr.To(int32(-1)) + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Role{ + Spec: v1alpha1.RoleSpec{ + ForProvider: v1alpha1.RoleParameters{ + Privileges: v1alpha1.RolePrivilege{}, + ConnectionLimit: ptr.To(int32(5)), + }, + }, + Status: v1alpha1.RoleStatus{ + AtProvider: v1alpha1.RoleObservation{ + LastPasswordChange: &metav1.Time{Time: metav1.Now().Time}, + }, + }, + }, + }, + want: want{ + o: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: false, + ResourceLateInitialized: true, + }, + err: nil, + }, + }, "PasswordChanged": { reason: "We should return ResourceUpToDate=false if the password changed", fields: fields{ From 265791dda5c8efb5f0ccb04abfd3246a43114b1e Mon Sep 17 00:00:00 2001 From: Youssef Omar Bouden Date: Fri, 6 Nov 2026 11:23:33 +0100 Subject: [PATCH 2/2] Compare the whole RoleParameters with cmp.Equal in upToDate Signed-off-by: Youssef Omar Bouden --- .../cluster/postgresql/role/reconciler.go | 33 +++---------------- .../namespaced/postgresql/role/reconciler.go | 33 +++---------------- 2 files changed, 8 insertions(+), 58 deletions(-) diff --git a/pkg/controller/cluster/postgresql/role/reconciler.go b/pkg/controller/cluster/postgresql/role/reconciler.go index 0506b33d..1503afb0 100644 --- a/pkg/controller/cluster/postgresql/role/reconciler.go +++ b/pkg/controller/cluster/postgresql/role/reconciler.go @@ -434,35 +434,10 @@ func (c *external) Delete(ctx context.Context, mg *v1alpha1.Role) (managed.Exter } func upToDate(observed *v1alpha1.RoleParameters, desired *v1alpha1.RoleParameters) bool { - if !cmp.Equal(observed.ConnectionLimit, desired.ConnectionLimit) { - return false - } - if !cmp.Equal(observed.Privileges.SuperUser, desired.Privileges.SuperUser) { - return false - } - if !cmp.Equal(observed.Privileges.Inherit, desired.Privileges.Inherit) { - return false - } - if !cmp.Equal(observed.Privileges.CreateDb, desired.Privileges.CreateDb) { - return false - } - if !cmp.Equal(observed.Privileges.CreateRole, desired.Privileges.CreateRole) { - return false - } - if !cmp.Equal(observed.Privileges.Login, desired.Privileges.Login) { - return false - } - if !cmp.Equal(observed.Privileges.Replication, desired.Privileges.Replication) { - return false - } - if !cmp.Equal(observed.Privileges.BypassRls, desired.Privileges.BypassRls) { - return false - } - if !cmp.Equal(observed.ConfigurationParameters, desired.ConfigurationParameters, - cmpopts.SortSlices(func(o, d v1alpha1.RoleConfigurationParameter) bool { return o.Name < d.Name })) { - return false - } - return true + return cmp.Equal(observed, desired, + cmpopts.IgnoreFields(v1alpha1.RoleParameters{}, "PasswordSecretRef", "PasswordRotationTrigger"), + cmpopts.SortSlices(func(o, d v1alpha1.RoleConfigurationParameter) bool { return o.Name < d.Name }), + ) } func lateInit(observed *v1alpha1.RoleParameters, desired *v1alpha1.RoleParameters) bool { diff --git a/pkg/controller/namespaced/postgresql/role/reconciler.go b/pkg/controller/namespaced/postgresql/role/reconciler.go index 312deef0..af212843 100644 --- a/pkg/controller/namespaced/postgresql/role/reconciler.go +++ b/pkg/controller/namespaced/postgresql/role/reconciler.go @@ -417,35 +417,10 @@ func (c *external) Delete(ctx context.Context, mg *namespacedv1alpha1.Role) (man } func upToDate(observed *namespacedv1alpha1.RoleParameters, desired *namespacedv1alpha1.RoleParameters) bool { - if !cmp.Equal(observed.ConnectionLimit, desired.ConnectionLimit) { - return false - } - if !cmp.Equal(observed.Privileges.SuperUser, desired.Privileges.SuperUser) { - return false - } - if !cmp.Equal(observed.Privileges.Inherit, desired.Privileges.Inherit) { - return false - } - if !cmp.Equal(observed.Privileges.CreateDb, desired.Privileges.CreateDb) { - return false - } - if !cmp.Equal(observed.Privileges.CreateRole, desired.Privileges.CreateRole) { - return false - } - if !cmp.Equal(observed.Privileges.Login, desired.Privileges.Login) { - return false - } - if !cmp.Equal(observed.Privileges.Replication, desired.Privileges.Replication) { - return false - } - if !cmp.Equal(observed.Privileges.BypassRls, desired.Privileges.BypassRls) { - return false - } - if !cmp.Equal(observed.ConfigurationParameters, desired.ConfigurationParameters, - cmpopts.SortSlices(func(o, d namespacedv1alpha1.RoleConfigurationParameter) bool { return o.Name < d.Name })) { - return false - } - return true + return cmp.Equal(observed, desired, + cmpopts.IgnoreFields(namespacedv1alpha1.RoleParameters{}, "PasswordSecretRef", "PasswordRotationTrigger"), + cmpopts.SortSlices(func(o, d namespacedv1alpha1.RoleConfigurationParameter) bool { return o.Name < d.Name }), + ) } func lateInit(observed *namespacedv1alpha1.RoleParameters, desired *namespacedv1alpha1.RoleParameters) bool {