Collection of grant fixes for 0.16 - #421
Merged
chlunde merged 11 commits intoAug 11, 2026
Merged
Conversation
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
commented
Jul 20, 2026
chlunde
commented
Jul 20, 2026
* log at debug level when the server version cannot be determined, instead of silently falling back to "assume latest" * use reference.FromPtrValue in the namespaced connectDatabase, matching the cluster variant * add the missing connect_test.go for the namespaced grant controller * move the format_type() argument-spelling comment from the multi-arg routine example to the INTEGER one, where it applies * drop the CockroachDB mention: it is not covered by e2e tests, so it is not a backend this provider claims to support. Connection proxies that do not expose server_version_num are the case that matters. * drop database_grant_test.go: asserting on the generated SQL text is brittle, and both behaviours it guarded (owner-implicit privileges and revokePublicOnDb) are covered by the grant examples the e2e job applies * bump the copyright year on the files this branch adds, and in hack/boilerplate.go.txt Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>
Mechanical result of `make generate` after bumping the year in hack/boilerplate.go.txt. Header line only, no other changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>
chlunde
marked this pull request as ready for review
August 11, 2026 11:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
e2e tests/unit tests