From b24c089ad9217b29e55d02ba144670d894e95b38 Mon Sep 17 00:00:00 2001 From: Matthew Greenwald Date: Fri, 24 Apr 2026 17:39:55 -0400 Subject: [PATCH 1/3] feat: adding support for psql inherit Signed-off-by: Matthew Greenwald --- .../postgresql/v1alpha1/grant_types.go | 7 + .../v1alpha1/zz_generated.deepcopy.go | 95 +-------- .../postgresql/grant-with-inherit-false.yaml | 25 +++ .../postgresql.sql.crossplane.io_grants.yaml | 137 +----------- .../cluster/postgresql/grant/reconciler.go | 50 ++++- .../postgresql/grant/reconciler_test.go | 199 ++++++++++++++++++ 6 files changed, 287 insertions(+), 226 deletions(-) create mode 100644 examples/cluster/postgresql/grant-with-inherit-false.yaml diff --git a/apis/cluster/postgresql/v1alpha1/grant_types.go b/apis/cluster/postgresql/v1alpha1/grant_types.go index 325701f3..a6a4175a 100644 --- a/apis/cluster/postgresql/v1alpha1/grant_types.go +++ b/apis/cluster/postgresql/v1alpha1/grant_types.go @@ -406,6 +406,13 @@ type GrantParameters struct { // +optional // +kubebuilder:validation:items:Pattern:=^[a-zA-Z_][a-zA-Z0-9_$]*$ ForeignServers []string `json:"foreignServers,omitempty"` + + // WithInherit controls whether the grantee automatically inherits the privileges + // of the granted role. When set to false, emits WITH INHERIT FALSE (PostgreSQL 16+), + // granting membership without automatic privilege inheritance. Only valid when + // memberOf is set. When omitted, PostgreSQL's default behavior (inherit true) applies. + // +optional + WithInherit *bool `json:"withInherit,omitempty"` } // A GrantStatus represents the observed state of a Grant. diff --git a/apis/cluster/postgresql/v1alpha1/zz_generated.deepcopy.go b/apis/cluster/postgresql/v1alpha1/zz_generated.deepcopy.go index dedd46c2..bbb1feaf 100644 --- a/apis/cluster/postgresql/v1alpha1/zz_generated.deepcopy.go +++ b/apis/cluster/postgresql/v1alpha1/zz_generated.deepcopy.go @@ -85,11 +85,6 @@ func (in *DatabaseParameters) DeepCopyInto(out *DatabaseParameters) { *out = new(string) **out = **in } - if in.Strategy != nil { - in, out := &in.Strategy, &out.Strategy - *out = new(DatabaseStrategy) - **out = **in - } if in.Encoding != nil { in, out := &in.Encoding, &out.Encoding *out = new(string) @@ -566,21 +561,6 @@ func (in *GrantParameters) DeepCopyInto(out *GrantParameters) { *out = new(v1.Selector) (*in).DeepCopyInto(*out) } - if in.Schema != nil { - in, out := &in.Schema, &out.Schema - *out = new(string) - **out = **in - } - if in.SchemaRef != nil { - in, out := &in.SchemaRef, &out.SchemaRef - *out = new(v1.Reference) - (*in).DeepCopyInto(*out) - } - if in.SchemaSelector != nil { - in, out := &in.SchemaSelector, &out.SchemaSelector - *out = new(v1.Selector) - (*in).DeepCopyInto(*out) - } if in.MemberOf != nil { in, out := &in.MemberOf, &out.MemberOf *out = new(string) @@ -601,37 +581,10 @@ func (in *GrantParameters) DeepCopyInto(out *GrantParameters) { *out = new(bool) **out = **in } - if in.Columns != nil { - in, out := &in.Columns, &out.Columns - *out = make([]string, len(*in)) - copy(*out, *in) - } - if in.Tables != nil { - in, out := &in.Tables, &out.Tables - *out = make([]string, len(*in)) - copy(*out, *in) - } - if in.Sequences != nil { - in, out := &in.Sequences, &out.Sequences - *out = make([]string, len(*in)) - copy(*out, *in) - } - if in.Routines != nil { - in, out := &in.Routines, &out.Routines - *out = make([]Routine, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } - if in.ForeignDataWrappers != nil { - in, out := &in.ForeignDataWrappers, &out.ForeignDataWrappers - *out = make([]string, len(*in)) - copy(*out, *in) - } - if in.ForeignServers != nil { - in, out := &in.ForeignServers, &out.ForeignServers - *out = make([]string, len(*in)) - copy(*out, *in) + if in.WithInherit != nil { + in, out := &in.WithInherit, &out.WithInherit + *out = new(bool) + **out = **in } } @@ -859,11 +812,6 @@ func (in *ProviderCredentials) DeepCopyInto(out *ProviderCredentials) { *out = new(v1.SecretReference) **out = **in } - if in.SecretKeyMapping != nil { - in, out := &in.SecretKeyMapping, &out.SecretKeyMapping - *out = new(SecretKeyMapping) - **out = **in - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ProviderCredentials. @@ -1098,26 +1046,6 @@ func (in *RoleStatus) DeepCopy() *RoleStatus { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *Routine) DeepCopyInto(out *Routine) { - *out = *in - if in.Arguments != nil { - in, out := &in.Arguments, &out.Arguments - *out = make([]string, len(*in)) - copy(*out, *in) - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Routine. -func (in *Routine) DeepCopy() *Routine { - if in == nil { - return nil - } - out := new(Routine) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Schema) DeepCopyInto(out *Schema) { *out = *in @@ -1264,18 +1192,3 @@ func (in *SchemaStatus) DeepCopy() *SchemaStatus { in.DeepCopyInto(out) return out } - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *SecretKeyMapping) DeepCopyInto(out *SecretKeyMapping) { - *out = *in -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SecretKeyMapping. -func (in *SecretKeyMapping) DeepCopy() *SecretKeyMapping { - if in == nil { - return nil - } - out := new(SecretKeyMapping) - in.DeepCopyInto(out) - return out -} diff --git a/examples/cluster/postgresql/grant-with-inherit-false.yaml b/examples/cluster/postgresql/grant-with-inherit-false.yaml new file mode 100644 index 00000000..24442840 --- /dev/null +++ b/examples/cluster/postgresql/grant-with-inherit-false.yaml @@ -0,0 +1,25 @@ +# Demonstrates WITH INHERIT FALSE on a role membership grant (PostgreSQL 16+). +# +# Use case: RDS IAM database authentication with a shared admin role. +# +# 1. An "admin" role holds rds_iam membership so that migration tools +# (Flyway, Liquibase, Atlas) can authenticate via IAM tokens. +# 2. The RDS master user needs membership in "admin" in order to run +# ALTER DEFAULT PRIVILEGES FOR ROLE admin — but must NOT inherit +# the rds_iam privilege, which would break its password authentication. +# +# GRANT admin TO master_user WITH INHERIT FALSE satisfies both requirements: +# the master user can SET ROLE admin (for ALTER DEFAULT PRIVILEGES) while +# retaining its own password-based login. +--- +apiVersion: postgresql.sql.crossplane.io/v1alpha1 +kind: Grant +metadata: + name: grant-master-user-membership-no-inherit +spec: + forProvider: + withInherit: false + roleRef: + name: master-user + memberOfRef: + name: admin-role diff --git a/package/crds/postgresql.sql.crossplane.io_grants.yaml b/package/crds/postgresql.sql.crossplane.io_grants.yaml index 516fb06a..bc2a7aff 100644 --- a/package/crds/postgresql.sql.crossplane.io_grants.yaml +++ b/package/crds/postgresql.sql.crossplane.io_grants.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.20.0 + controller-gen.kubebuilder.io/version: v0.18.0 name: grants.postgresql.sql.crossplane.io spec: group: postgresql.sql.crossplane.io @@ -83,12 +83,6 @@ spec: description: GrantParameters define the desired state of a PostgreSQL grant instance. properties: - columns: - description: The columns upon which to grant the privileges. - items: - pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ - type: string - type: array database: description: Database this grant is for. type: string @@ -168,19 +162,6 @@ spec: type: string type: object type: object - foreignDataWrappers: - description: The foreign data wrappers upon which to grant the - privileges. - items: - pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ - type: string - type: array - foreignServers: - description: The foreign servers upon which to grant the privileges. - items: - pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ - type: string - type: array memberOf: description: MemberOf is the Role that this grant makes Role a member of. @@ -267,7 +248,7 @@ spec: See https://www.postgresql.org/docs/current/sql-grant.html for available privileges. items: description: GrantPrivilege represents a privilege to be granted - pattern: ^[A-Z]+( [A-Z]+)*$ + pattern: ^[A-Z]+$ type: string minItems: 1 type: array @@ -355,113 +336,13 @@ spec: type: string type: object type: object - routines: - description: The routines upon which to grant the privileges. - items: - properties: - args: - description: The arguments of the routine. - items: - pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ - type: string - type: array - name: - description: The name of the routine. - pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ - type: string - type: object - type: array - schema: - description: Schema this grant is for. - type: string - schemaRef: - description: SchemaRef references the schema object this grant - it for. - properties: - name: - description: Name of the referenced object. - type: string - policy: - description: Policies for referencing. - properties: - resolution: - default: Required - description: |- - Resolution specifies whether resolution of this reference is required. - The default is 'Required', which means the reconcile will fail if the - reference cannot be resolved. 'Optional' means this reference will be - a no-op if it cannot be resolved. - enum: - - Required - - Optional - type: string - resolve: - description: |- - Resolve specifies when this reference should be resolved. The default - is 'IfNotPresent', which will attempt to resolve the reference only when - the corresponding field is not present. Use 'Always' to resolve the - reference on every reconcile. - enum: - - Always - - IfNotPresent - type: string - type: object - required: - - name - type: object - schemaSelector: - description: SchemaSelector selects a reference to a Schema this - grant is for. - properties: - matchControllerRef: - description: |- - MatchControllerRef ensures an object with the same controller reference - as the selecting object is selected. - type: boolean - matchLabels: - additionalProperties: - type: string - description: MatchLabels ensures an object with matching labels - is selected. - type: object - policy: - description: Policies for selection. - properties: - resolution: - default: Required - description: |- - Resolution specifies whether resolution of this reference is required. - The default is 'Required', which means the reconcile will fail if the - reference cannot be resolved. 'Optional' means this reference will be - a no-op if it cannot be resolved. - enum: - - Required - - Optional - type: string - resolve: - description: |- - Resolve specifies when this reference should be resolved. The default - is 'IfNotPresent', which will attempt to resolve the reference only when - the corresponding field is not present. Use 'Always' to resolve the - reference on every reconcile. - enum: - - Always - - IfNotPresent - type: string - type: object - type: object - sequences: - description: The sequences upon which to grant the privileges. - items: - pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ - type: string - type: array - tables: - description: The tables upon which to grant the privileges. - items: - pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ - type: string - type: array + withInherit: + description: |- + WithInherit controls whether the grantee automatically inherits the privileges + of the granted role. When set to false, emits WITH INHERIT FALSE (PostgreSQL 16+), + granting membership without automatic privilege inheritance. Only valid when + memberOf is set. When omitted, PostgreSQL's default behavior (inherit true) applies. + type: boolean withOption: description: |- WithOption allows an option to be set on the grant. diff --git a/pkg/controller/cluster/postgresql/grant/reconciler.go b/pkg/controller/cluster/postgresql/grant/reconciler.go index 81c716b1..7fbec05b 100644 --- a/pkg/controller/cluster/postgresql/grant/reconciler.go +++ b/pkg/controller/cluster/postgresql/grant/reconciler.go @@ -61,6 +61,7 @@ const ( errInvalidParams = "invalid parameters for grant type %s" errGetServerVersion = "cannot get server version" errMemberOfWithDatabaseOrPrivileges = "cannot set privileges or database in the same grant as memberOf" + errWithInheritOnlyForMemberOf = "withInherit is only valid for memberOf grants" maxConcurrency = 5 ) @@ -303,12 +304,33 @@ func createMemberGrantQueries(gp v1alpha1.GrantParameters, ql *[]xsql.Query, ro *ql = append(*ql, xsql.Query{String: fmt.Sprintf("REVOKE %s FROM %s", mo, ro)}, xsql.Query{String: fmt.Sprintf("GRANT %s TO %s %s", mo, ro, - withOption(gp.WithOption), + membershipWithClauses(gp.WithOption, gp.WithInherit), )}, ) return nil } +// membershipWithClauses builds the WITH clause for role membership GRANTs, +// combining the optional ADMIN/SET option and the optional INHERIT flag. +// On PostgreSQL 16+, multiple options are comma-separated: WITH ADMIN OPTION, INHERIT FALSE. +func membershipWithClauses(option *v1alpha1.GrantOption, inherit *bool) string { + var parts []string + if option != nil { + parts = append(parts, fmt.Sprintf("%s OPTION", string(*option))) + } + if inherit != nil { + if *inherit { + parts = append(parts, "INHERIT TRUE") + } else { + parts = append(parts, "INHERIT FALSE") + } + } + if len(parts) == 0 { + return "" + } + return "WITH " + strings.Join(parts, ", ") +} + func createRoutineGrantQueries(gp v1alpha1.GrantParameters, ql *[]xsql.Query, ro string) error { if gp.Database == nil || gp.Schema == nil || len(gp.Routines) < 1 || gp.Role == nil || len(gp.Privileges) < 1 { return errors.Errorf(errInvalidParams, v1alpha1.RoleRoutine) @@ -603,6 +625,10 @@ func resolveGrantType(gp v1alpha1.GrantParameters) (v1alpha1.GrantType, error) { return v1alpha1.RoleMember, nil } + if gp.WithInherit != nil { + return "", errors.New(errWithInheritOnlyForMemberOf) + } + return gp.IdentifyGrantType() } @@ -791,17 +817,27 @@ func selectMemberGrantQuery(gp v1alpha1.GrantParameters, q *xsql.Query) error { // A simpler query would use ::regrole to cast the roleid and member oids // to their role names, but that throws an error for nonexistent roles // rather than returning false. - q.String = "SELECT EXISTS(SELECT 1 FROM pg_auth_members m " + - "INNER JOIN pg_roles mo ON m.roleid = mo.oid " + - "INNER JOIN pg_roles r ON m.member = r.oid " + - "WHERE r.rolname=$1 AND mo.rolname=$2 AND " + - "m.admin_option = $3)" - q.Parameters = []interface{}{ gp.Role, gp.MemberOf, ao, } + + if gp.WithInherit != nil { + // inherit_option is a pg_auth_members column added in PostgreSQL 16. + q.String = "SELECT EXISTS(SELECT 1 FROM pg_auth_members m " + + "INNER JOIN pg_roles mo ON m.roleid = mo.oid " + + "INNER JOIN pg_roles r ON m.member = r.oid " + + "WHERE r.rolname=$1 AND mo.rolname=$2 AND " + + "m.admin_option = $3 AND m.inherit_option = $4)" + q.Parameters = append(q.Parameters, *gp.WithInherit) + } else { + q.String = "SELECT EXISTS(SELECT 1 FROM pg_auth_members m " + + "INNER JOIN pg_roles mo ON m.roleid = mo.oid " + + "INNER JOIN pg_roles r ON m.member = r.oid " + + "WHERE r.rolname=$1 AND mo.rolname=$2 AND " + + "m.admin_option = $3)" + } return nil } diff --git a/pkg/controller/cluster/postgresql/grant/reconciler_test.go b/pkg/controller/cluster/postgresql/grant/reconciler_test.go index 1ccabe70..9c93dc7a 100644 --- a/pkg/controller/cluster/postgresql/grant/reconciler_test.go +++ b/pkg/controller/cluster/postgresql/grant/reconciler_test.go @@ -632,6 +632,112 @@ func TestObserve(t *testing.T) { err: nil, }, }, + "SuccessRoleMembershipWithInheritNil": { + reason: "WithInherit nil should produce a 3-parameter query (no inherit_option filter)", + fields: fields{ + db: mockDB{ + MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { + if len(q.Parameters) != 3 { + return fmt.Errorf("expected 3 query parameters, got %d", len(q.Parameters)) + } + bv := dest[0].(*bool) + *bv = true + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + }, + }, + }, + }, + want: want{ + o: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: true, + }, + err: nil, + }, + }, + "SuccessRoleMembershipWithInheritFalse": { + reason: "WithInherit false should produce a 4-parameter query with $4 == false", + fields: fields{ + db: mockDB{ + MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { + if len(q.Parameters) != 4 { + return fmt.Errorf("expected 4 query parameters, got %d", len(q.Parameters)) + } + inheritParam, ok := q.Parameters[3].(bool) + if !ok || inheritParam != false { + return fmt.Errorf("expected $4 to be false, got %v", q.Parameters[3]) + } + bv := dest[0].(*bool) + *bv = true + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + WithInherit: ptr.To(false), + }, + }, + }, + }, + want: want{ + o: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: true, + }, + err: nil, + }, + }, + "SuccessRoleMembershipWithInheritTrue": { + reason: "WithInherit true should produce a 4-parameter query with $4 == true", + fields: fields{ + db: mockDB{ + MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { + if len(q.Parameters) != 4 { + return fmt.Errorf("expected 4 query parameters, got %d", len(q.Parameters)) + } + inheritParam, ok := q.Parameters[3].(bool) + if !ok || inheritParam != true { + return fmt.Errorf("expected $4 to be true, got %v", q.Parameters[3]) + } + bv := dest[0].(*bool) + *bv = true + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + WithInherit: ptr.To(true), + }, + }, + }, + }, + want: want{ + o: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: true, + }, + err: nil, + }, + }, } for name, tc := range cases { @@ -935,6 +1041,99 @@ func TestCreate(t *testing.T) { err: nil, }, }, + "RoleMembershipWithInheritNil": { + reason: "WithInherit nil should produce a GRANT with no WITH clause", + fields: fields{ + db: &mockDB{ + MockExecTx: func(ctx context.Context, ql []xsql.Query) error { + if len(ql) != 2 { + return fmt.Errorf("expected 2 queries, got %d", len(ql)) + } + grantSQL := ql[1].String + if strings.Contains(grantSQL, "INHERIT") { + return fmt.Errorf("expected no INHERIT clause, got: %s", grantSQL) + } + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + }, + }, + }, + }, + want: want{ + err: nil, + }, + }, + "RoleMembershipWithInheritFalse": { + reason: "WithInherit false should produce a GRANT with WITH INHERIT FALSE", + fields: fields{ + db: &mockDB{ + MockExecTx: func(ctx context.Context, ql []xsql.Query) error { + if len(ql) != 2 { + return fmt.Errorf("expected 2 queries, got %d", len(ql)) + } + grantSQL := ql[1].String + if !strings.Contains(grantSQL, "WITH INHERIT FALSE") { + return fmt.Errorf("expected WITH INHERIT FALSE in grant SQL, got: %s", grantSQL) + } + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + WithInherit: ptr.To(false), + }, + }, + }, + }, + want: want{ + err: nil, + }, + }, + "RoleMembershipWithInheritFalseAndAdminOption": { + reason: "WithInherit false and WithOption ADMIN should produce WITH ADMIN OPTION, INHERIT FALSE", + fields: fields{ + db: &mockDB{ + MockExecTx: func(ctx context.Context, ql []xsql.Query) error { + if len(ql) != 2 { + return fmt.Errorf("expected 2 queries, got %d", len(ql)) + } + grantSQL := ql[1].String + if !strings.Contains(grantSQL, "WITH ADMIN OPTION, INHERIT FALSE") { + return fmt.Errorf("expected WITH ADMIN OPTION, INHERIT FALSE in grant SQL, got: %s", grantSQL) + } + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + WithOption: &goa, + WithInherit: ptr.To(false), + }, + }, + }, + }, + want: want{ + err: nil, + }, + }, } for name, tc := range cases { From fda19470ed9143b9d7f76732a2559a7e9b57c4cb Mon Sep 17 00:00:00 2001 From: Matthew Greenwald Date: Fri, 24 Apr 2026 18:17:39 -0400 Subject: [PATCH 2/3] docs: updating docs for other contributors Signed-off-by: Matthew Greenwald --- README.md | 36 +++++++++++------------------------- 1 file changed, 11 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 174e38a5..41019a3f 100644 --- a/README.md +++ b/README.md @@ -59,9 +59,9 @@ Check the example: 2. Create managed resources for your SQL server flavor: - - **MySQL**: `Database`, `Grant`, `User` (See [the examples](examples/mysql)) - - **PostgreSQL**: `Database`, `Grant`, `DefaultPrivileges`, `Extension`, `Role` (See [the examples](examples/postgresql)) - - **MSSQL**: `Database`, `Grant`, `User` (See [the examples](examples/mssql)) + - **MySQL**: `Database`, `Grant`, `User` (See [the examples](examples/cluster/mysql)) + - **PostgreSQL**: `Database`, `Grant`, `DefaultPrivileges`, `Extension`, `Role`, `Schema` (See [the examples](examples/cluster/postgresql)) + - **MSSQL**: `Database`, `Grant`, `User` (See [the examples](examples/cluster/mssql)) [crossplane]: https://crossplane.io [cloudsqlinstance]: https://doc.crds.dev/github.com/crossplane/provider-gcp/database.gcp.crossplane.io/CloudSQLInstance/v1beta1@v0.18.0 @@ -71,41 +71,27 @@ Check the example: 1. Fork the project and clone locally. 2. Create a branch with the changes. -3. Install go version 1.18. -4. Run `make` to initialize the "build". Make submodules used for CI/CD. +3. Install Go 1.26.1 (see `go.mod`). +4. Run `make submodules` to initialize the build submodule. 5. Run `make reviewable` to run code generation, linters, and tests. 6. Commit, push, and PR. ## Developing locally -**Pre-requisite:** A Kubernetes cluster with Crossplane installed - -To run the `provider-helm` controller against your existing local cluster, -simply run: +To spin up a local [kind](https://kind.sigs.k8s.io/) cluster with Crossplane and the provider CRDs installed and the provider running: ```console -make run +make dev ``` -Since the controller is running outside of the local cluster, you need to make -the API server accessible (on a separate terminal): +To tear it down: ```console -sudo kubectl proxy --port=8081 +make dev-clean ``` -Then we must prepare a `ProviderConfig` for the local cluster (assuming you are -using `kind` for local development): +To run the provider against an existing cluster (uses the current kubeconfig context): ```console -KUBECONFIG=$(kind get kubeconfig | sed -e 's|server:\s*.*$|server: http://localhost:8081|g') -kubectl -n crossplane-system create secret generic cluster-config --from-literal=kubeconfig="${KUBECONFIG}" -kubectl apply -f examples/provider-config/provider-config-with-secret.yaml -``` - -Now you can create `Release` resources with this `ProviderConfig`, for example -[sample release.yaml](examples/sample/release.yaml). - -```console -kubectl create -f examples/sample/release.yaml +make run ``` From c1ea143cd018716cf0083074a0d8bf38374c92be Mon Sep 17 00:00:00 2001 From: Matthew Greenwald Date: Thu, 28 May 2026 10:36:17 -0400 Subject: [PATCH 3/3] addressing comments and rebasing from master Signed-off-by: Matthew Greenwald --- README.md | 36 ++- .../postgresql/v1alpha1/grant_types.go | 3 + .../v1alpha1/zz_generated.deepcopy.go | 92 ++++++++ .../postgresql/v1alpha1/grant_types.go | 10 + .../v1alpha1/zz_generated.deepcopy.go | 5 + build | 2 +- .../postgresql.sql.crossplane.io_grants.yaml | 136 ++++++++++- ...postgresql.sql.m.crossplane.io_grants.yaml | 13 ++ .../cluster/postgresql/grant/reconciler.go | 7 + .../postgresql/grant/reconciler_test.go | 16 +- .../namespaced/postgresql/grant/reconciler.go | 57 ++++- .../postgresql/grant/reconciler_test.go | 216 +++++++++++++++++- 12 files changed, 564 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 41019a3f..174e38a5 100644 --- a/README.md +++ b/README.md @@ -59,9 +59,9 @@ Check the example: 2. Create managed resources for your SQL server flavor: - - **MySQL**: `Database`, `Grant`, `User` (See [the examples](examples/cluster/mysql)) - - **PostgreSQL**: `Database`, `Grant`, `DefaultPrivileges`, `Extension`, `Role`, `Schema` (See [the examples](examples/cluster/postgresql)) - - **MSSQL**: `Database`, `Grant`, `User` (See [the examples](examples/cluster/mssql)) + - **MySQL**: `Database`, `Grant`, `User` (See [the examples](examples/mysql)) + - **PostgreSQL**: `Database`, `Grant`, `DefaultPrivileges`, `Extension`, `Role` (See [the examples](examples/postgresql)) + - **MSSQL**: `Database`, `Grant`, `User` (See [the examples](examples/mssql)) [crossplane]: https://crossplane.io [cloudsqlinstance]: https://doc.crds.dev/github.com/crossplane/provider-gcp/database.gcp.crossplane.io/CloudSQLInstance/v1beta1@v0.18.0 @@ -71,27 +71,41 @@ Check the example: 1. Fork the project and clone locally. 2. Create a branch with the changes. -3. Install Go 1.26.1 (see `go.mod`). -4. Run `make submodules` to initialize the build submodule. +3. Install go version 1.18. +4. Run `make` to initialize the "build". Make submodules used for CI/CD. 5. Run `make reviewable` to run code generation, linters, and tests. 6. Commit, push, and PR. ## Developing locally -To spin up a local [kind](https://kind.sigs.k8s.io/) cluster with Crossplane and the provider CRDs installed and the provider running: +**Pre-requisite:** A Kubernetes cluster with Crossplane installed + +To run the `provider-helm` controller against your existing local cluster, +simply run: ```console -make dev +make run ``` -To tear it down: +Since the controller is running outside of the local cluster, you need to make +the API server accessible (on a separate terminal): ```console -make dev-clean +sudo kubectl proxy --port=8081 ``` -To run the provider against an existing cluster (uses the current kubeconfig context): +Then we must prepare a `ProviderConfig` for the local cluster (assuming you are +using `kind` for local development): ```console -make run +KUBECONFIG=$(kind get kubeconfig | sed -e 's|server:\s*.*$|server: http://localhost:8081|g') +kubectl -n crossplane-system create secret generic cluster-config --from-literal=kubeconfig="${KUBECONFIG}" +kubectl apply -f examples/provider-config/provider-config-with-secret.yaml +``` + +Now you can create `Release` resources with this `ProviderConfig`, for example +[sample release.yaml](examples/sample/release.yaml). + +```console +kubectl create -f examples/sample/release.yaml ``` diff --git a/apis/cluster/postgresql/v1alpha1/grant_types.go b/apis/cluster/postgresql/v1alpha1/grant_types.go index a6a4175a..21f542e6 100644 --- a/apis/cluster/postgresql/v1alpha1/grant_types.go +++ b/apis/cluster/postgresql/v1alpha1/grant_types.go @@ -302,6 +302,7 @@ type Routine struct { } // GrantParameters define the desired state of a PostgreSQL grant instance. +// +kubebuilder:validation:XValidation:rule="!has(self.withInherit) || has(self.memberOf) || has(self.memberOfRef) || has(self.memberOfSelector)",message="withInherit may only be set on memberOf grants" type GrantParameters struct { // Privileges to be granted. // See https://www.postgresql.org/docs/current/sql-grant.html for available privileges. @@ -411,6 +412,8 @@ type GrantParameters struct { // of the granted role. When set to false, emits WITH INHERIT FALSE (PostgreSQL 16+), // granting membership without automatic privilege inheritance. Only valid when // memberOf is set. When omitted, PostgreSQL's default behavior (inherit true) applies. + // Note: this field is only evaluated when non-nil. Removing it from the manifest + // does NOT revert the database-side setting; set withInherit: true explicitly to revert. // +optional WithInherit *bool `json:"withInherit,omitempty"` } diff --git a/apis/cluster/postgresql/v1alpha1/zz_generated.deepcopy.go b/apis/cluster/postgresql/v1alpha1/zz_generated.deepcopy.go index bbb1feaf..05241d2e 100644 --- a/apis/cluster/postgresql/v1alpha1/zz_generated.deepcopy.go +++ b/apis/cluster/postgresql/v1alpha1/zz_generated.deepcopy.go @@ -85,6 +85,11 @@ func (in *DatabaseParameters) DeepCopyInto(out *DatabaseParameters) { *out = new(string) **out = **in } + if in.Strategy != nil { + in, out := &in.Strategy, &out.Strategy + *out = new(DatabaseStrategy) + **out = **in + } if in.Encoding != nil { in, out := &in.Encoding, &out.Encoding *out = new(string) @@ -561,6 +566,21 @@ func (in *GrantParameters) DeepCopyInto(out *GrantParameters) { *out = new(v1.Selector) (*in).DeepCopyInto(*out) } + if in.Schema != nil { + in, out := &in.Schema, &out.Schema + *out = new(string) + **out = **in + } + if in.SchemaRef != nil { + in, out := &in.SchemaRef, &out.SchemaRef + *out = new(v1.Reference) + (*in).DeepCopyInto(*out) + } + if in.SchemaSelector != nil { + in, out := &in.SchemaSelector, &out.SchemaSelector + *out = new(v1.Selector) + (*in).DeepCopyInto(*out) + } if in.MemberOf != nil { in, out := &in.MemberOf, &out.MemberOf *out = new(string) @@ -581,6 +601,38 @@ func (in *GrantParameters) DeepCopyInto(out *GrantParameters) { *out = new(bool) **out = **in } + if in.Columns != nil { + in, out := &in.Columns, &out.Columns + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Tables != nil { + in, out := &in.Tables, &out.Tables + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Sequences != nil { + in, out := &in.Sequences, &out.Sequences + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Routines != nil { + in, out := &in.Routines, &out.Routines + *out = make([]Routine, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } + if in.ForeignDataWrappers != nil { + in, out := &in.ForeignDataWrappers, &out.ForeignDataWrappers + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.ForeignServers != nil { + in, out := &in.ForeignServers, &out.ForeignServers + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.WithInherit != nil { in, out := &in.WithInherit, &out.WithInherit *out = new(bool) @@ -812,6 +864,11 @@ func (in *ProviderCredentials) DeepCopyInto(out *ProviderCredentials) { *out = new(v1.SecretReference) **out = **in } + if in.SecretKeyMapping != nil { + in, out := &in.SecretKeyMapping, &out.SecretKeyMapping + *out = new(SecretKeyMapping) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ProviderCredentials. @@ -1046,6 +1103,26 @@ func (in *RoleStatus) DeepCopy() *RoleStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Routine) DeepCopyInto(out *Routine) { + *out = *in + if in.Arguments != nil { + in, out := &in.Arguments, &out.Arguments + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Routine. +func (in *Routine) DeepCopy() *Routine { + if in == nil { + return nil + } + out := new(Routine) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Schema) DeepCopyInto(out *Schema) { *out = *in @@ -1192,3 +1269,18 @@ func (in *SchemaStatus) DeepCopy() *SchemaStatus { in.DeepCopyInto(out) return out } + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *SecretKeyMapping) DeepCopyInto(out *SecretKeyMapping) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SecretKeyMapping. +func (in *SecretKeyMapping) DeepCopy() *SecretKeyMapping { + if in == nil { + return nil + } + out := new(SecretKeyMapping) + in.DeepCopyInto(out) + return out +} diff --git a/apis/namespaced/postgresql/v1alpha1/grant_types.go b/apis/namespaced/postgresql/v1alpha1/grant_types.go index cbfe9f27..99004214 100644 --- a/apis/namespaced/postgresql/v1alpha1/grant_types.go +++ b/apis/namespaced/postgresql/v1alpha1/grant_types.go @@ -304,6 +304,7 @@ type Routine struct { } // GrantParameters define the desired state of a PostgreSQL grant instance. +// +kubebuilder:validation:XValidation:rule="!has(self.withInherit) || has(self.memberOf) || has(self.memberOfRef) || has(self.memberOfSelector)",message="withInherit may only be set on memberOf grants" type GrantParameters struct { // Privileges to be granted. // See https://www.postgresql.org/docs/current/sql-grant.html for available privileges. @@ -408,6 +409,15 @@ type GrantParameters struct { // +optional // +kubebuilder:validation:items:Pattern:=^[a-zA-Z_][a-zA-Z0-9_$]*$ ForeignServers []string `json:"foreignServers,omitempty"` + + // WithInherit controls whether the grantee automatically inherits the privileges + // of the granted role. When set to false, emits WITH INHERIT FALSE (PostgreSQL 16+), + // granting membership without automatic privilege inheritance. Only valid when + // memberOf is set. When omitted, PostgreSQL's default behavior (inherit true) applies. + // Note: this field is only evaluated when non-nil. Removing it from the manifest + // does NOT revert the database-side setting; set withInherit: true explicitly to revert. + // +optional + WithInherit *bool `json:"withInherit,omitempty"` } // A GrantStatus represents the observed state of a Grant. diff --git a/apis/namespaced/postgresql/v1alpha1/zz_generated.deepcopy.go b/apis/namespaced/postgresql/v1alpha1/zz_generated.deepcopy.go index 7a62990c..fa54e26b 100644 --- a/apis/namespaced/postgresql/v1alpha1/zz_generated.deepcopy.go +++ b/apis/namespaced/postgresql/v1alpha1/zz_generated.deepcopy.go @@ -750,6 +750,11 @@ func (in *GrantParameters) DeepCopyInto(out *GrantParameters) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.WithInherit != nil { + in, out := &in.WithInherit, &out.WithInherit + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GrantParameters. diff --git a/build b/build index 65819b05..907604dd 160000 --- a/build +++ b/build @@ -1 +1 @@ -Subproject commit 65819b05239a0b22f1a98c3698a14acd22bc3cc0 +Subproject commit 907604dd4d57eba8bcf94699d7834f68389b4f96 diff --git a/package/crds/postgresql.sql.crossplane.io_grants.yaml b/package/crds/postgresql.sql.crossplane.io_grants.yaml index bc2a7aff..810a7ec5 100644 --- a/package/crds/postgresql.sql.crossplane.io_grants.yaml +++ b/package/crds/postgresql.sql.crossplane.io_grants.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.18.0 + controller-gen.kubebuilder.io/version: v0.20.0 name: grants.postgresql.sql.crossplane.io spec: group: postgresql.sql.crossplane.io @@ -83,6 +83,12 @@ spec: description: GrantParameters define the desired state of a PostgreSQL grant instance. properties: + columns: + description: The columns upon which to grant the privileges. + items: + pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ + type: string + type: array database: description: Database this grant is for. type: string @@ -162,6 +168,19 @@ spec: type: string type: object type: object + foreignDataWrappers: + description: The foreign data wrappers upon which to grant the + privileges. + items: + pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ + type: string + type: array + foreignServers: + description: The foreign servers upon which to grant the privileges. + items: + pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ + type: string + type: array memberOf: description: MemberOf is the Role that this grant makes Role a member of. @@ -248,7 +267,7 @@ spec: See https://www.postgresql.org/docs/current/sql-grant.html for available privileges. items: description: GrantPrivilege represents a privilege to be granted - pattern: ^[A-Z]+$ + pattern: ^[A-Z]+( [A-Z]+)*$ type: string minItems: 1 type: array @@ -336,12 +355,121 @@ spec: type: string type: object type: object + routines: + description: The routines upon which to grant the privileges. + items: + properties: + args: + description: The arguments of the routine. + items: + pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ + type: string + type: array + name: + description: The name of the routine. + pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ + type: string + type: object + type: array + schema: + description: Schema this grant is for. + type: string + schemaRef: + description: SchemaRef references the schema object this grant + it for. + properties: + name: + description: Name of the referenced object. + type: string + policy: + description: Policies for referencing. + properties: + resolution: + default: Required + description: |- + Resolution specifies whether resolution of this reference is required. + The default is 'Required', which means the reconcile will fail if the + reference cannot be resolved. 'Optional' means this reference will be + a no-op if it cannot be resolved. + enum: + - Required + - Optional + type: string + resolve: + description: |- + Resolve specifies when this reference should be resolved. The default + is 'IfNotPresent', which will attempt to resolve the reference only when + the corresponding field is not present. Use 'Always' to resolve the + reference on every reconcile. + enum: + - Always + - IfNotPresent + type: string + type: object + required: + - name + type: object + schemaSelector: + description: SchemaSelector selects a reference to a Schema this + grant is for. + properties: + matchControllerRef: + description: |- + MatchControllerRef ensures an object with the same controller reference + as the selecting object is selected. + type: boolean + matchLabels: + additionalProperties: + type: string + description: MatchLabels ensures an object with matching labels + is selected. + type: object + policy: + description: Policies for selection. + properties: + resolution: + default: Required + description: |- + Resolution specifies whether resolution of this reference is required. + The default is 'Required', which means the reconcile will fail if the + reference cannot be resolved. 'Optional' means this reference will be + a no-op if it cannot be resolved. + enum: + - Required + - Optional + type: string + resolve: + description: |- + Resolve specifies when this reference should be resolved. The default + is 'IfNotPresent', which will attempt to resolve the reference only when + the corresponding field is not present. Use 'Always' to resolve the + reference on every reconcile. + enum: + - Always + - IfNotPresent + type: string + type: object + type: object + sequences: + description: The sequences upon which to grant the privileges. + items: + pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ + type: string + type: array + tables: + description: The tables upon which to grant the privileges. + items: + pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ + type: string + type: array withInherit: description: |- WithInherit controls whether the grantee automatically inherits the privileges of the granted role. When set to false, emits WITH INHERIT FALSE (PostgreSQL 16+), granting membership without automatic privilege inheritance. Only valid when memberOf is set. When omitted, PostgreSQL's default behavior (inherit true) applies. + Note: this field is only evaluated when non-nil. Removing it from the manifest + does NOT revert the database-side setting; set withInherit: true explicitly to revert. type: boolean withOption: description: |- @@ -353,6 +481,10 @@ spec: - GRANT type: string type: object + x-kubernetes-validations: + - message: withInherit may only be set on memberOf grants + rule: '!has(self.withInherit) || has(self.memberOf) || has(self.memberOfRef) + || has(self.memberOfSelector)' managementPolicies: default: - '*' diff --git a/package/crds/postgresql.sql.m.crossplane.io_grants.yaml b/package/crds/postgresql.sql.m.crossplane.io_grants.yaml index 7dc8c04a..6d140dc4 100644 --- a/package/crds/postgresql.sql.m.crossplane.io_grants.yaml +++ b/package/crds/postgresql.sql.m.crossplane.io_grants.yaml @@ -472,6 +472,15 @@ spec: pattern: ^[a-zA-Z_][a-zA-Z0-9_$]*$ type: string type: array + withInherit: + description: |- + WithInherit controls whether the grantee automatically inherits the privileges + of the granted role. When set to false, emits WITH INHERIT FALSE (PostgreSQL 16+), + granting membership without automatic privilege inheritance. Only valid when + memberOf is set. When omitted, PostgreSQL's default behavior (inherit true) applies. + Note: this field is only evaluated when non-nil. Removing it from the manifest + does NOT revert the database-side setting; set withInherit: true explicitly to revert. + type: boolean withOption: description: |- WithOption allows an option to be set on the grant. @@ -482,6 +491,10 @@ spec: - GRANT type: string type: object + x-kubernetes-validations: + - message: withInherit may only be set on memberOf grants + rule: '!has(self.withInherit) || has(self.memberOf) || has(self.memberOfRef) + || has(self.memberOfSelector)' managementPolicies: default: - '*' diff --git a/pkg/controller/cluster/postgresql/grant/reconciler.go b/pkg/controller/cluster/postgresql/grant/reconciler.go index 7fbec05b..802bc3e9 100644 --- a/pkg/controller/cluster/postgresql/grant/reconciler.go +++ b/pkg/controller/cluster/postgresql/grant/reconciler.go @@ -62,6 +62,7 @@ const ( errGetServerVersion = "cannot get server version" errMemberOfWithDatabaseOrPrivileges = "cannot set privileges or database in the same grant as memberOf" errWithInheritOnlyForMemberOf = "withInherit is only valid for memberOf grants" + errInheritRequiresPG16 = "withInherit requires PostgreSQL 16 or later (server version %d)" maxConcurrency = 5 ) @@ -281,6 +282,9 @@ func createGrantQueriesWithVersion(gp v1alpha1.GrantParameters, ql *[]xsql.Query case v1alpha1.RoleForeignServer: return createForeignServerGrantQueries(gp, ql, ro) case v1alpha1.RoleMember: + if gp.WithInherit != nil && serverVersion < 160000 { + return errors.Errorf(errInheritRequiresPG16, serverVersion) + } return createMemberGrantQueries(gp, ql, ro) case v1alpha1.RoleRoutine: return createRoutineGrantQueries(gp, ql, ro) @@ -797,6 +801,9 @@ func selectGrantQueryWithVersion(gp v1alpha1.GrantParameters, q *xsql.Query, ser case v1alpha1.RoleForeignServer: return selectForeignServerGrantQuery(gp, q) case v1alpha1.RoleMember: + if gp.WithInherit != nil && serverVersion < 160000 { + return errors.Errorf(errInheritRequiresPG16, serverVersion) + } return selectMemberGrantQuery(gp, q) case v1alpha1.RoleRoutine: return selectRoutineGrantQuery(gp, q) diff --git a/pkg/controller/cluster/postgresql/grant/reconciler_test.go b/pkg/controller/cluster/postgresql/grant/reconciler_test.go index 9c93dc7a..e4329a6a 100644 --- a/pkg/controller/cluster/postgresql/grant/reconciler_test.go +++ b/pkg/controller/cluster/postgresql/grant/reconciler_test.go @@ -195,7 +195,8 @@ func TestObserve(t *testing.T) { gog := v1alpha1.GrantOptionGrant type fields struct { - db xsql.DB + db xsql.DB + serverVersion int } type args struct { @@ -635,6 +636,7 @@ func TestObserve(t *testing.T) { "SuccessRoleMembershipWithInheritNil": { reason: "WithInherit nil should produce a 3-parameter query (no inherit_option filter)", fields: fields{ + serverVersion: 160000, db: mockDB{ MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { if len(q.Parameters) != 3 { @@ -667,6 +669,7 @@ func TestObserve(t *testing.T) { "SuccessRoleMembershipWithInheritFalse": { reason: "WithInherit false should produce a 4-parameter query with $4 == false", fields: fields{ + serverVersion: 160000, db: mockDB{ MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { if len(q.Parameters) != 4 { @@ -704,6 +707,7 @@ func TestObserve(t *testing.T) { "SuccessRoleMembershipWithInheritTrue": { reason: "WithInherit true should produce a 4-parameter query with $4 == true", fields: fields{ + serverVersion: 160000, db: mockDB{ MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { if len(q.Parameters) != 4 { @@ -746,7 +750,7 @@ func TestObserve(t *testing.T) { if db == nil { db = mockDB{} } - e := external{db: db} + e := external{db: db, serverVersion: tc.fields.serverVersion} got, err := e.Observe(tc.args.ctx, tc.args.mg) if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\ne.Observe(...): -want error, +got error:\n%s\n", tc.reason, diff) @@ -763,7 +767,8 @@ func TestCreate(t *testing.T) { goa := v1alpha1.GrantOptionAdmin type fields struct { - db xsql.DB + db xsql.DB + serverVersion int } type args struct { @@ -1044,6 +1049,7 @@ func TestCreate(t *testing.T) { "RoleMembershipWithInheritNil": { reason: "WithInherit nil should produce a GRANT with no WITH clause", fields: fields{ + serverVersion: 160000, db: &mockDB{ MockExecTx: func(ctx context.Context, ql []xsql.Query) error { if len(ql) != 2 { @@ -1074,6 +1080,7 @@ func TestCreate(t *testing.T) { "RoleMembershipWithInheritFalse": { reason: "WithInherit false should produce a GRANT with WITH INHERIT FALSE", fields: fields{ + serverVersion: 160000, db: &mockDB{ MockExecTx: func(ctx context.Context, ql []xsql.Query) error { if len(ql) != 2 { @@ -1105,6 +1112,7 @@ func TestCreate(t *testing.T) { "RoleMembershipWithInheritFalseAndAdminOption": { reason: "WithInherit false and WithOption ADMIN should produce WITH ADMIN OPTION, INHERIT FALSE", fields: fields{ + serverVersion: 160000, db: &mockDB{ MockExecTx: func(ctx context.Context, ql []xsql.Query) error { if len(ql) != 2 { @@ -1142,7 +1150,7 @@ func TestCreate(t *testing.T) { if db == nil { db = mockDB{} } - e := external{db: db} + e := external{db: db, serverVersion: tc.fields.serverVersion} got, err := e.Create(tc.args.ctx, tc.args.mg) if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\ne.Create(...): -want error, +got error:\n%s\n", tc.reason, diff) diff --git a/pkg/controller/namespaced/postgresql/grant/reconciler.go b/pkg/controller/namespaced/postgresql/grant/reconciler.go index b1908f22..28205462 100644 --- a/pkg/controller/namespaced/postgresql/grant/reconciler.go +++ b/pkg/controller/namespaced/postgresql/grant/reconciler.go @@ -56,6 +56,8 @@ const ( errInvalidParams = "invalid parameters for grant type %s" errGetServerVersion = "cannot get server version" errMemberOfWithDatabaseOrPrivileges = "cannot set privileges or database in the same grant as memberOf" + errWithInheritOnlyForMemberOf = "withInherit is only valid for memberOf grants" + errInheritRequiresPG16 = "withInherit requires PostgreSQL 16 or later (server version %d)" maxConcurrency = 5 ) @@ -238,6 +240,9 @@ func createGrantQueriesWithVersion(gp v1alpha1.GrantParameters, ql *[]xsql.Query case v1alpha1.RoleForeignServer: return createForeignServerGrantQueries(gp, ql, ro) case v1alpha1.RoleMember: + if gp.WithInherit != nil && serverVersion < 160000 { + return errors.Errorf(errInheritRequiresPG16, serverVersion) + } return createMemberGrantQueries(gp, ql, ro) case v1alpha1.RoleRoutine: return createRoutineGrantQueries(gp, ql, ro) @@ -261,12 +266,33 @@ func createMemberGrantQueries(gp v1alpha1.GrantParameters, ql *[]xsql.Query, ro *ql = append(*ql, xsql.Query{String: fmt.Sprintf("REVOKE %s FROM %s", mo, ro)}, xsql.Query{String: fmt.Sprintf("GRANT %s TO %s %s", mo, ro, - withOption(gp.WithOption), + membershipWithClauses(gp.WithOption, gp.WithInherit), )}, ) return nil } +// membershipWithClauses builds the WITH clause for role membership GRANTs, +// combining the optional ADMIN/SET option and the optional INHERIT flag. +// On PostgreSQL 16+, multiple options are comma-separated: WITH ADMIN OPTION, INHERIT FALSE. +func membershipWithClauses(option *v1alpha1.GrantOption, inherit *bool) string { + var parts []string + if option != nil { + parts = append(parts, fmt.Sprintf("%s OPTION", string(*option))) + } + if inherit != nil { + if *inherit { + parts = append(parts, "INHERIT TRUE") + } else { + parts = append(parts, "INHERIT FALSE") + } + } + if len(parts) == 0 { + return "" + } + return "WITH " + strings.Join(parts, ", ") +} + func createRoutineGrantQueries(gp v1alpha1.GrantParameters, ql *[]xsql.Query, ro string) error { if gp.Database == nil || gp.Schema == nil || len(gp.Routines) < 1 || gp.Role == nil || len(gp.Privileges) < 1 { return errors.Errorf(errInvalidParams, v1alpha1.RoleRoutine) @@ -561,6 +587,10 @@ func resolveGrantType(gp v1alpha1.GrantParameters) (v1alpha1.GrantType, error) { return v1alpha1.RoleMember, nil } + if gp.WithInherit != nil { + return "", errors.New(errWithInheritOnlyForMemberOf) + } + return gp.IdentifyGrantType() } @@ -729,6 +759,9 @@ func selectGrantQueryWithVersion(gp v1alpha1.GrantParameters, q *xsql.Query, ser case v1alpha1.RoleForeignServer: return selectForeignServerGrantQuery(gp, q) case v1alpha1.RoleMember: + if gp.WithInherit != nil && serverVersion < 160000 { + return errors.Errorf(errInheritRequiresPG16, serverVersion) + } return selectMemberGrantQuery(gp, q) case v1alpha1.RoleRoutine: return selectRoutineGrantQuery(gp, q) @@ -749,17 +782,27 @@ func selectMemberGrantQuery(gp v1alpha1.GrantParameters, q *xsql.Query) error { // A simpler query would use ::regrole to cast the roleid and member oids // to their role names, but that throws an error for nonexistent roles // rather than returning false. - q.String = "SELECT EXISTS(SELECT 1 FROM pg_auth_members m " + - "INNER JOIN pg_roles mo ON m.roleid = mo.oid " + - "INNER JOIN pg_roles r ON m.member = r.oid " + - "WHERE r.rolname=$1 AND mo.rolname=$2 AND " + - "m.admin_option = $3)" - q.Parameters = []interface{}{ gp.Role, gp.MemberOf, ao, } + + if gp.WithInherit != nil { + // inherit_option is a pg_auth_members column added in PostgreSQL 16. + q.String = "SELECT EXISTS(SELECT 1 FROM pg_auth_members m " + + "INNER JOIN pg_roles mo ON m.roleid = mo.oid " + + "INNER JOIN pg_roles r ON m.member = r.oid " + + "WHERE r.rolname=$1 AND mo.rolname=$2 AND " + + "m.admin_option = $3 AND m.inherit_option = $4)" + q.Parameters = append(q.Parameters, *gp.WithInherit) + } else { + q.String = "SELECT EXISTS(SELECT 1 FROM pg_auth_members m " + + "INNER JOIN pg_roles mo ON m.roleid = mo.oid " + + "INNER JOIN pg_roles r ON m.member = r.oid " + + "WHERE r.rolname=$1 AND mo.rolname=$2 AND " + + "m.admin_option = $3)" + } return nil } diff --git a/pkg/controller/namespaced/postgresql/grant/reconciler_test.go b/pkg/controller/namespaced/postgresql/grant/reconciler_test.go index 92857b2c..add7099a 100644 --- a/pkg/controller/namespaced/postgresql/grant/reconciler_test.go +++ b/pkg/controller/namespaced/postgresql/grant/reconciler_test.go @@ -21,6 +21,7 @@ import ( "database/sql" "fmt" "sort" + "strings" "testing" "github.com/crossplane-contrib/provider-sql/apis/namespaced/postgresql/v1alpha1" @@ -253,7 +254,8 @@ func TestObserve(t *testing.T) { gog := v1alpha1.GrantOptionGrant type fields struct { - db xsql.DB + db xsql.DB + serverVersion int } type args struct { @@ -681,6 +683,115 @@ func TestObserve(t *testing.T) { err: nil, }, }, + "SuccessRoleMembershipWithInheritNil": { + reason: "WithInherit nil should produce a 3-parameter query (no inherit_option filter)", + fields: fields{ + serverVersion: 160000, + db: mockDB{ + MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { + if len(q.Parameters) != 3 { + return fmt.Errorf("expected 3 query parameters, got %d", len(q.Parameters)) + } + bv := dest[0].(*bool) + *bv = true + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + }, + }, + }, + }, + want: want{ + o: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: true, + }, + err: nil, + }, + }, + "SuccessRoleMembershipWithInheritFalse": { + reason: "WithInherit false should produce a 4-parameter query with $4 == false", + fields: fields{ + serverVersion: 160000, + db: mockDB{ + MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { + if len(q.Parameters) != 4 { + return fmt.Errorf("expected 4 query parameters, got %d", len(q.Parameters)) + } + inheritParam, ok := q.Parameters[3].(bool) + if !ok || inheritParam != false { + return fmt.Errorf("expected $4 to be false, got %v", q.Parameters[3]) + } + bv := dest[0].(*bool) + *bv = true + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + WithInherit: ptr.To(false), + }, + }, + }, + }, + want: want{ + o: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: true, + }, + err: nil, + }, + }, + "SuccessRoleMembershipWithInheritTrue": { + reason: "WithInherit true should produce a 4-parameter query with $4 == true", + fields: fields{ + serverVersion: 160000, + db: mockDB{ + MockScan: func(ctx context.Context, q xsql.Query, dest ...interface{}) error { + if len(q.Parameters) != 4 { + return fmt.Errorf("expected 4 query parameters, got %d", len(q.Parameters)) + } + inheritParam, ok := q.Parameters[3].(bool) + if !ok || inheritParam != true { + return fmt.Errorf("expected $4 to be true, got %v", q.Parameters[3]) + } + bv := dest[0].(*bool) + *bv = true + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + WithInherit: ptr.To(true), + }, + }, + }, + }, + want: want{ + o: managed.ExternalObservation{ + ResourceExists: true, + ResourceUpToDate: true, + }, + err: nil, + }, + }, } for name, tc := range cases { @@ -689,7 +800,7 @@ func TestObserve(t *testing.T) { if db == nil { db = mockDB{} } - e := external{db: db} + e := external{db: db, serverVersion: tc.fields.serverVersion} got, err := e.Observe(tc.args.ctx, tc.args.mg) if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\ne.Observe(...): -want error, +got error:\n%s\n", tc.reason, diff) @@ -706,7 +817,8 @@ func TestCreate(t *testing.T) { goa := v1alpha1.GrantOptionAdmin type fields struct { - db xsql.DB + db xsql.DB + serverVersion int } type args struct { @@ -975,6 +1087,102 @@ func TestCreate(t *testing.T) { err: nil, }, }, + "RoleMembershipWithInheritNil": { + reason: "WithInherit nil should produce a GRANT with no WITH clause", + fields: fields{ + serverVersion: 160000, + db: &mockDB{ + MockExecTx: func(ctx context.Context, ql []xsql.Query) error { + if len(ql) != 2 { + return fmt.Errorf("expected 2 queries, got %d", len(ql)) + } + grantSQL := ql[1].String + if strings.Contains(grantSQL, "INHERIT") { + return fmt.Errorf("expected no INHERIT clause, got: %s", grantSQL) + } + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + }, + }, + }, + }, + want: want{ + err: nil, + }, + }, + "RoleMembershipWithInheritFalse": { + reason: "WithInherit false should produce a GRANT with WITH INHERIT FALSE", + fields: fields{ + serverVersion: 160000, + db: &mockDB{ + MockExecTx: func(ctx context.Context, ql []xsql.Query) error { + if len(ql) != 2 { + return fmt.Errorf("expected 2 queries, got %d", len(ql)) + } + grantSQL := ql[1].String + if !strings.Contains(grantSQL, "WITH INHERIT FALSE") { + return fmt.Errorf("expected WITH INHERIT FALSE in grant SQL, got: %s", grantSQL) + } + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + WithInherit: ptr.To(false), + }, + }, + }, + }, + want: want{ + err: nil, + }, + }, + "RoleMembershipWithInheritFalseAndAdminOption": { + reason: "WithInherit false and WithOption ADMIN should produce WITH ADMIN OPTION, INHERIT FALSE", + fields: fields{ + serverVersion: 160000, + db: &mockDB{ + MockExecTx: func(ctx context.Context, ql []xsql.Query) error { + if len(ql) != 2 { + return fmt.Errorf("expected 2 queries, got %d", len(ql)) + } + grantSQL := ql[1].String + if !strings.Contains(grantSQL, "WITH ADMIN OPTION, INHERIT FALSE") { + return fmt.Errorf("expected WITH ADMIN OPTION, INHERIT FALSE in grant SQL, got: %s", grantSQL) + } + return nil + }, + }, + }, + args: args{ + mg: &v1alpha1.Grant{ + Spec: v1alpha1.GrantSpec{ + ForProvider: v1alpha1.GrantParameters{ + Role: ptr.To("testrole"), + MemberOf: ptr.To("parentrole"), + WithOption: &goa, + WithInherit: ptr.To(false), + }, + }, + }, + }, + want: want{ + err: nil, + }, + }, } for name, tc := range cases { @@ -983,7 +1191,7 @@ func TestCreate(t *testing.T) { if db == nil { db = mockDB{} } - e := external{db: db} + e := external{db: db, serverVersion: tc.fields.serverVersion} got, err := e.Create(tc.args.ctx, tc.args.mg) if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\ne.Create(...): -want error, +got error:\n%s\n", tc.reason, diff)