Repository: crossplane-contrib/provider-sql
Pull request #144 (2023-07-27 16:10:23)

postgres: Expand grant privileges where required
Inside postgresql, some privileges are [altered][0] when issuing a `GRANT`, for example `ALL` is expanded to `CREATE, TEMPORARY, CONNECT`.

When we observe the grant, we query privileges on the database to see if the ones in the grant exist. Given that they are expanded, we can't simply query for what the user gave us. Expand the privileges which need expanding before we make the query.

Closes: #92

[0]: https://www.postgresql.org/docs/15/ddl-priv.html

### How has this code been tested

  * There's a unit test to check the permissions are expanded
  * We extracted the queries the provider is using and ran them manually against a database

iainlane: Hmm, it works here 🤔 

<img width="335" alt="image" src="https://github.com/crossplane-contrib/provider-sql/assets/321014/eb16332e-4ce4-46c2-a484-b2e9da4c60e3">

I'll take a look later and see if CI does something different to me.

iainlane: @Duologic I found the cause, nondeterministic map iteration order, should be fixed in e761eaa, could you take a look please?

Patch:
diff --git a/apis/postgresql/v1alpha1/grant_types.go b/apis/postgresql/v1alpha1/grant_types.go
index c5dd374..6d4c982 100644
--- a/apis/postgresql/v1alpha1/grant_types.go
+++ b/apis/postgresql/v1alpha1/grant_types.go
@@ -43,6 +43,39 @@ type GrantPrivilege string
 // +kubebuilder:validation:MinItems:=1
 type GrantPrivileges []GrantPrivilege
 
+// Some privileges are shorthands for multiple privileges. These translations
+// happen internally inside postgresql when making grants. When we query the
+// privileges back, we need to look for the expanded set.
+// https://www.postgresql.org/docs/15/ddl-priv.html
+var grantReplacements = map[GrantPrivilege]GrantPrivileges{
+	"ALL":            {"CREATE", "TEMPORARY", "CONNECT"},
+	"ALL PRIVILEGES": {"CREATE", "TEMPORARY", "CONNECT"},
+	"TEMP":           {"TEMPORARY"},
+}
+
+// ExpandPrivileges expands any shorthand privileges to their full equivalents.
+func (gp *GrantPrivileges) ExpandPrivileges() GrantPrivileges {
+	privilegeSet := make(map[GrantPrivilege]struct{})
+
+	// Replace any shorthand privileges with their full equivalents
+	for _, p := range *gp {
+		if _, ok := grantReplacements[p]; ok {
+			for _, rp := range grantReplacements[p] {
+				privilegeSet[rp] = struct{}{}
+			}
+		} else {
+			privilegeSet[p] = struct{}{}
+		}
+	}
+
+	privileges := make([]GrantPrivilege, 0, len(privilegeSet))
+	for p := range privilegeSet {
+		privileges = append(privileges, p)
+	}
+
+	return privileges
+}
+
 // ToStringSlice converts the slice of privileges to strings
 func (gp *GrantPrivileges) ToStringSlice() []string {
 	if gp == nil {
diff --git a/pkg/controller/postgresql/grant/reconciler.go b/pkg/controller/postgresql/grant/reconciler.go
index bb59cfc..85d0175 100644
--- a/pkg/controller/postgresql/grant/reconciler.go
+++ b/pkg/controller/postgresql/grant/reconciler.go
@@ -191,7 +191,9 @@ func selectGrantQuery(gp v1alpha1.GrantParameters, q *xsql.Query) error {
 		return nil
 	case roleDatabase:
 		gro := gp.WithOption != nil && *gp.WithOption == v1alpha1.GrantOptionGrant
-		sp := gp.Privileges.ToStringSlice()
+
+		ep := gp.Privileges.ExpandPrivileges()
+		sp := ep.ToStringSlice()
 		// Join grantee. Filter by database name and grantee name.
 		// Finally, perform a permission comparison against expected
 		// permissions.


Test patch:
diff --git a/pkg/controller/postgresql/grant/reconciler_test.go b/pkg/controller/postgresql/grant/reconciler_test.go
index 22b8f96..d552931 100644
--- a/pkg/controller/postgresql/grant/reconciler_test.go
+++ b/pkg/controller/postgresql/grant/reconciler_test.go
@@ -19,12 +19,15 @@ package grant
 import (
 	"context"
 	"database/sql"
+	"fmt"
+	"sort"
 
 	"testing"
 
 	"github.com/crossplane-contrib/provider-sql/apis/postgresql/v1alpha1"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/lib/pq"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/utils/pointer"
@@ -241,6 +244,55 @@ func TestObserve(t *testing.T) {
 				o: managed.ExternalObservation{ResourceExists: false},
 			},
 		},
+		"AllMapsToExpandedPrivileges": {
+			reason: "We expand ALL to CREATE, TEMPORARY, CONNECT when checking for existing grants",
+			fields: fields{
+				db: mockDB{
+					MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error {
+						privileges := q.Parameters[3]
+
+						privs, ok := privileges.(*pq.StringArray)
+						if !ok {
+							return fmt.Errorf("expected Scan parameter to be pq.StringArray, got %T", privileges)
+						}
+
+						// The order is not guaranteed, so sort the slices before comparing
+						sort.Strings(*privs)
+
+						// Return if there's a diff between the expected and actual privileges
+						diff := cmp.Diff(&pq.StringArray{"CONNECT", "CREATE", "TEMPORARY"}, privileges)
+
+						bv := dest[0].(*bool)
+						*bv = diff == ""
+
+						// Extra logging in case this test is going to fail
+						if diff != "" {
+							t.Logf("expected empty diff, got: %s", diff)
+						}
+
+						return nil
+					},
+				},
+			},
+			args: args{
+				mg: &v1alpha1.Grant{
+					Spec: v1alpha1.GrantSpec{
+						ForProvider: v1alpha1.GrantParameters{
+							Database:   pointer.StringPtr("test-example"),
+							Role:       pointer.StringPtr("test-example"),
+							Privileges: v1alpha1.GrantPrivileges{"ALL"},
+						},
+					},
+				},
+			},
+			want: want{
+				o: managed.ExternalObservation{
+					ResourceExists:   true,
+					ResourceUpToDate: true,
+				},
+				err: nil,
+			},
+		},
 		"ErrSelectGrant": {
 			reason: "We should return any errors encountered while trying to show the grant",
 			fields: fields{