Skip to content

fix(postgresql): compare role values, not pointers, in upToDate #488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
blacksoxx wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(postgresql): compare role values, not pointers, in upToDate #488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9da3411
fix(postgresql): compare role values, not pointers, in upToDate
blacksoxx Oct 16, 2026
265791d
Compare the whole RoleParameters with cmp.Equal in upToDate
blacksoxx Nov 6, 2026
ab67a61
Merge branch 'master' into role-uptodate-compare-values
fernandezcuesta Nov 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 4 additions & 29 deletions pkg/controller/cluster/postgresql/role/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,35 +436,10 @@ func (c *external) Delete(ctx context.Context, mg *v1alpha1.Role) (managed.Exter
}

func upToDate(observed *v1alpha1.RoleParameters, desired *v1alpha1.RoleParameters) bool {

@fernandezcuesta fernandezcuesta Nov 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it's better to do cmp.Equal in observed vs desired with IgnoreFields (the ones that Observe never fills) plus SortSlices to avoid spurious diffs:

func upToDate(observed *v1alpha1.RoleParameters, desired *v1alpha1.RoleParameters) bool {
	return cmp.Equal(
		observed,
		desired,
		cmpopts.IgnoreFields(v1alpha1.RoleParameters{}, "PasswordSecretRef", "PasswordRotationTrigger"),
		cmpopts.SortSlices(func(o, d v1alpha1.RoleConfigurationParameter) bool { return o.Name < d.Name })
	)
}

@fernandezcuesta fernandezcuesta Nov 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but IIRC this will be implemented from crossplane/crossplane-runtime (see http://github.com/crossplane/crossplane-runtime/pull/945)

@blacksoxx blacksoxx Nov 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that is cleaner. Done in 265791d for both controllers, the two TestObserve cases still cover it.

On #945: as I read it, it adds an UpToDate condition that surfaces whatever Observe returns in ResourceUpToDate, so the comparison itself stays in the provider and this fix is still needed for it to report true.

if observed.ConnectionLimit != desired.ConnectionLimit {
return false
}
if observed.Privileges.SuperUser != desired.Privileges.SuperUser {
return false
}
if observed.Privileges.Inherit != desired.Privileges.Inherit {
return false
}
if observed.Privileges.CreateDb != desired.Privileges.CreateDb {
return false
}
if observed.Privileges.CreateRole != desired.Privileges.CreateRole {
return false
}
if observed.Privileges.Login != desired.Privileges.Login {
return false
}
if observed.Privileges.Replication != desired.Privileges.Replication {
return false
}
if 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 {
Expand Down
58 changes: 58 additions & 0 deletions pkg/controller/cluster/postgresql/role/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
33 changes: 4 additions & 29 deletions pkg/controller/namespaced/postgresql/role/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,35 +419,10 @@ 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 {

@fernandezcuesta fernandezcuesta Nov 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

return false
}
if observed.Privileges.SuperUser != desired.Privileges.SuperUser {
return false
}
if observed.Privileges.Inherit != desired.Privileges.Inherit {
return false
}
if observed.Privileges.CreateDb != desired.Privileges.CreateDb {
return false
}
if observed.Privileges.CreateRole != desired.Privileges.CreateRole {
return false
}
if observed.Privileges.Login != desired.Privileges.Login {
return false
}
if observed.Privileges.Replication != desired.Privileges.Replication {
return false
}
if 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 {
Expand Down
68 changes: 68 additions & 0 deletions pkg/controller/namespaced/postgresql/role/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down