Collection of grant fixes for 0.16 by chlunde · Pull Request #421 · crossplane-contrib/provider-sql

chlunde · 2026-07-20T12:46:27Z

Description of your changes

I tasked Claude Code to look for bugs in the recent updates to Grant,
and confirm any issue by creating additional unit or e2e tests.

observe routine grants on functions with 2+ arguments
do not fail grant Connect when the server version is unknown
observe database grants when the role owns the database
cover routine grants on a multi-argument routine
treat an unknown server version as newest, not as pre-16
observe PUBLIC's ACL when revokePublicOnDb is set
converge grants on views and non-text routine argument types

I have:

Read and followed Crossplane's contribution process.
Run make reviewable to ensure this PR is ready for review.

How has this code been tested

e2e tests/unit tests

@niralmehtasb1

selectRoutineGrantQuery joined unnest(p.proargtypes) -- one row per argument -- into the same FROM clause as aclexplode(p.proacl) -- one row per privilege. The comma between two set-returning functions is a cross join, so array_agg(acl.privilege_type ORDER BY privilege_type ASC) collected one entry per argument. EXECUTE is the only privilege a function can hold, so the HAVING equality against the requested ARRAY['EXECUTE'] held only for functions with exactly one argument. The GRANT itself succeeded, so the resource reported Synced=True and ReconcileSuccess while Observe returned ResourceExists=false forever: Create retried indefinitely and Ready stayed False. Format the argument types in a correlated subquery instead, keeping the argument rows out of the ACL aggregation. Verified against PostgreSQL 18.4 for 0, 1, 2 and 9 argument functions, plus negative controls (ungranted function, nonexistent function, wrong signature, multi-routine grants). Reported-by: @niralmehtasb1 Refs: crossplane-contrib#240 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>

… unknown Two regressions, both introduced after v0.15.0 and both unreleased. 1. Connect() called GetServerVersion and returned the error. In v0.15.0 grant Connect did no database I/O at all. Connect gates Observe, Create *and* Delete, so any PostgreSQL-compatible backend that cannot answer 'SELECT current_setting(''server_version_num'')::int' -- CockroachDB, pgbouncer admin targets, connection proxies -- could no longer manage a Grant, and the finalizer wedged so it could not even be removed. Fall back to serverVersion 0. ExpandPrivilegesWithVersion already treats 0 as 'latest, include all privileges', which is exactly what the provider assumed before the version check existed. Only table grants are version dependent. 2. Connect() opened its session against the grant's own spec.forProvider.database whenever it was set. A ROLE_DATABASE grant always sets it, so database-level grants stopped using the ProviderConfig default database. That imposed a new ordering dependency (the target database must exist and be connectable before the grant reconciles) and could lock the provider out of the database it must reconnect to, e.g. once 'revokePublicOnDb: true' removed PUBLIC's CONNECT. GRANT ... ON DATABASE and the pg_database / pg_auth_members lookups all work from any session, so database and role-member grants now use the default database again, as in v0.15.0. Grants on objects inside a database (schema, table, column, sequence, routine, foreign data wrapper, foreign server) keep connecting to the target database -- their catalogs are database local. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>

The owner of a database implicitly holds CONNECT, CREATE and TEMPORARY on it. PostgreSQL materialises all three into pg_database.datacl as soon as anything is granted or revoked -- including the REVOKE/GRANT pair this provider issues in Create. A Grant requesting a subset, typically just CONNECT, could therefore never satisfy HAVING array_agg(acl.privilege_type ORDER BY privilege_type ASC) = (SELECT array(SELECT unnest($4::text[]) ORDER BY 1)) so Observe reported ResourceExists=false forever, Create reapplied the grant on every reconcile, and the resource never became Ready. Requesting ALL masked the bug, because ExpandPrivileges expands it to exactly those three privileges. Compare with containment (@>) when the grantee owns the database, and keep exact equality for every other grantee, whose privileges the provider fully controls. The query deliberately still requires datacl to be non-empty. A plain 'owner => exists' short-circuit would report the grant as existing on the very first Observe, so Create would never run -- and 'revokePublicOnDb' would never revoke PUBLIC's access. Verified against PostgreSQL 18.4 with the query the code emits: owner+CONNECT, owner+all-three, owner-before-Create, non-owner exact match, non-owner holding a superset, wrong privilege, and unknown role all behave as intended. Fixes crossplane-contrib#240 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>

A routine Grant on a single-argument routine is observed correctly even when the Observe query cross joins argument rows with privilege rows, so the existing single-argument fixtures could never catch that bug. Add a two-argument procedure and a Grant against it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>

…e-16 Two independently-correct changes interact badly. Connect falls back to serverVersion 0 when the backend cannot answer 'SELECT current_setting(''server_version_num'')::int', rather than failing and wedging the finalizer. ExpandPrivilegesWithVersion already treats 0 as 'latest, include every privilege', so 0 has always meant 'assume newest'. The withInherit gate added in crossplane-contrib#361 was a plain 'serverVersion < 160000', which reads the same 0 as 'older than PostgreSQL 16'. A genuine PG16+ server that merely cannot report its version would therefore have withInherit rejected with 'withInherit requires PostgreSQL 16 or later (server version 0)'. Name the sentinel and exclude it from the gate. A server that really does predate PG16 still gets a clean error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>

The database-grant Observe query inferred "Create has run" from datacl being non-empty. That inference is false: any GRANT or REVOKE on the database - by another Grant resource or by hand - materialises datacl, including PUBLIC's default CONNECT and TEMPORARY entries. An owner grant with revokePublicOnDb: true whose ACL was materialised before its first reconcile would then observe as existing, so Create - and with it REVOKE ALL ... FROM PUBLIC - never ran. Make the desired state explicit instead: when revokePublicOnDb is set, Observe additionally requires that PUBLIC (grantee oid 0, which has no pg_roles row) holds no privileges on the database. This also turns a later re-grant to PUBLIC into observable drift that the next reconcile corrects. Plain database grants emit the exact same query as before. Verified against PostgreSQL 16 with the emitted query: fresh datacl, ACL materialised by an unrelated grant, post-Create state, PUBLIC re-granted, and drift corrected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>

…nt types Two independent defects in the PostgreSQL grant reconciler, both of which let Create succeed and then make Observe report the resource as nonexistent forever, so the Grant never becomes Ready and Create retries indefinitely. Because Delete shares the same helpers, the finalizer wedges too. 1. Routine argument type names were quoted. Quoting a type name bypasses PostgreSQL's grammar-level alias resolution: the parser accepts "int4" (a literal pg_type.typname) but never "integer", because integer is a grammar keyword mapped to int4. Observe compares against pg_catalog.format_type(), which emits the canonical spelling "integer". So no value of args: converged for any type in the integer, float, char or bit families -- "integer" failed to GRANT, "int4" never matched on read. Emitting the type name unquoted lets integer/int4/int all resolve on the GRANT while still agreeing with what Observe reads back. Schema and routine names stay quoted; argument injection remains bounded by the CRD's identifier-only pattern on args. text-like types happened to work either way, which is exactly why the existing e2e passed. 2. The table and column Observe queries filtered pg_class to relkind = 'r', i.e. plain tables only. But Create emits GRANT ... ON TABLE, which PostgreSQL accepts for partitioned tables ('p'), views ('v'), materialized views ('m') and foreign tables ('f'), storing the ACL on that pg_class row. Granting SELECT on a view is everyday usage. Widened to relkind IN ('r','p','v','m','f') in both queries. The sequence query's relkind = 'S' is correct and deliberately left alone, pinned by a test. Both trees carry byte-identical query changes. The namespaced TestGrantSQL harness only exercised select queries; it now mirrors the cluster harness so the GRANT/REVOKE assertions run in both trees. Every new case was confirmed red against the unfixed reconciler and green after. e2e gains an INTEGER-argument procedure and a view, both granted through examples/{cluster,namespaced}/postgresql/grant.yaml, which kubectl wait already covers -- without these fixes the two new resources time out. Refs crossplane-contrib#217 Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>

chlunde · 2026-07-20T12:53:10Z

+//	owner, datacl still NULL     -> not exists (nothing granted yet; Create runs)
+//	non-owner, exact match       -> exists
+//	non-owner holding a superset -> not exists (equality preserved)
+func TestSelectDatabaseGrantQueryToleratesOwnerImplicitPrivileges(t *testing.T) {


This kind of test feels weird

Probably a bit cleaner if using testify?

chlunde · 2026-07-20T12:56:35Z

+	// treats 0 as "latest", which is what this provider assumed before the
+	// version check existed. Failing here would gate Observe, Create *and*
+	// Delete, wedging the finalizer on any PostgreSQL-compatible backend that
+	// does not expose server_version_num (CockroachDB, connection proxies).


I don't think we should support CockroachDB (if we want to, we should have e2e tests) but proxies we should support.

fernandezcuesta · 2026-07-20T13:58:03Z

 	serverVersion, err := xdb.GetServerVersion(ctx)
 	if err != nil {
-		return nil, errors.Wrap(err, errGetServerVersion)
+		serverVersion = versionUnknown


Suggested change

serverVersion = versionUnknown

c.log.Debug("cannot determine server version, assuming latest", "error", err)

serverVersion = versionUnknown

keep some log there, feel free to raise it to Info (I guess debug is more than enough)

fernandezcuesta · 2026-07-20T13:59:00Z

@@ -0,0 +1,190 @@
+/*


missing for namespaced

fernandezcuesta · 2026-07-20T13:59:36Z

@@ -0,0 +1,190 @@
+/*
+Copyright 2021 The Crossplane Authors.


now that I see this © year, shall we update hack/boilerplate.go.txt? It doesnt look to apply here though, a good'ol sed to update all?

fernandezcuesta · 2026-07-20T14:01:04Z

+	// report server_version_num. It means "assume the newest behaviour":
+	// ExpandPrivilegesWithVersion already treats 0 as "include every privilege".
+	// Version-gated features must therefore not treat it as an old server.
+	versionUnknown = 0


fernandezcuesta · 2026-07-20T14:16:57Z

+        # Argument types are written in the spelling pg_catalog.format_type()
+        # returns, which is what Observe compares against -- so INTEGER, not
+        # int4. The provider emits type names unquoted, which is what lets
+        # PostgreSQL resolve the grammar alias integer -> int4 on the GRANT
+        # while Observe still matches format_type()'s "integer".


?? weird comment: does it belong to the next example (example-grant-role-1-on-routine-int)?

fernandezcuesta · 2026-07-20T14:23:02Z

+		if gp.Database != nil && *gp.Database != "" {
+			return *gp.Database


Suggested change

if gp.Database != nil && *gp.Database != "" {

return *gp.Database

if db := reference.FromPtrValue(gp.Database); db != ""

return db

align to cluster/postgresql/grant/reconciler.go?

chlunde and others added 7 commits July 20, 2026 14:43

chlunde commented Jul 20, 2026

View reviewed changes

fernandezcuesta reviewed Jul 20, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Collection of grant fixes for 0.16 - #421

Collection of grant fixes for 0.16#421
chlunde wants to merge 7 commits into
crossplane-contrib:masterfrom
chlunde:issue-217-grant-mechanical

chlunde commented Jul 20, 2026

Uh oh!

chlunde Jul 20, 2026

Uh oh!

fernandezcuesta Jul 27, 2026

Uh oh!

chlunde Jul 20, 2026

Uh oh!

fernandezcuesta Jul 20, 2026

Uh oh!

fernandezcuesta Jul 20, 2026

Uh oh!

fernandezcuesta Jul 20, 2026

Uh oh!

fernandezcuesta Jul 20, 2026 •

edited

Loading

Uh oh!

fernandezcuesta Jul 20, 2026

Uh oh!

fernandezcuesta Jul 20, 2026

Uh oh!

fernandezcuesta Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	serverVersion = versionUnknown
	c.log.Debug("cannot determine server version, assuming latest", "error", err)
	serverVersion = versionUnknown

		if gp.Database != nil && *gp.Database != "" {
		return *gp.Database

Uh oh!

Conversation

chlunde commented Jul 20, 2026

Description of your changes

How has this code been tested

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fernandezcuesta Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fernandezcuesta Jul 20, 2026 •

edited

Loading