From 461509986c42954d697aca0327d121534c29ce16 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Thu, 24 Aug 2023 11:03:16 +0100 Subject: [PATCH] postgres: Expand grant privileges where required (#144) * 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 Signed-off-by: Iain Lane * Sort the grants privileges before comparing them When we use them in the observation the order doesn't matter, but in the test we are generating a diff where it does, so compare them in the test. Signed-off-by: Iain Lane --------- Signed-off-by: Iain Lane Signed-off-by: Iain Lane Signed-off-by: Timotej Avsec --- apis/postgresql/v1alpha1/grant_types.go | 33 ++++++++++++ pkg/controller/postgresql/grant/reconciler.go | 4 +- .../postgresql/grant/reconciler_test.go | 52 +++++++++++++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) diff --git a/apis/postgresql/v1alpha1/grant_types.go b/apis/postgresql/v1alpha1/grant_types.go index c5dd374e..6d4c982d 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 bb59cfc9..85d0175c 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. diff --git a/pkg/controller/postgresql/grant/reconciler_test.go b/pkg/controller/postgresql/grant/reconciler_test.go index 22b8f964..d552931a 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{