CXH-2379: fix grant/revoke idempotency for DDL-based engines - #151
CXH-2379: fix grant/revoke idempotency for DDL-based engines#151al-conductorone wants to merge 8 commits into
Conversation
Validation-query "no rows" now wraps ErrQueryAffectedZeroRows so the provisioning layer's errors.Is check reports GrantAlreadyExists / GrantAlreadyRevoked instead of failing the task. DDL dialects (e.g. Db2) whose GRANT/REVOKE raise an error rather than affecting rows can only signal prior state through validation_queries, which previously landed on the failing path. Adds regression tests driving Grant/Revoke end-to-end over in-memory sqlite for both the already-applied (idempotent) and apply cases.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return fmt.Errorf("validation query returned no rows") | ||
| // Wrap the sentinel so the idempotency path reports already-applied instead of | ||
| // failing; validation "no rows" is the only zero-effect signal DDL dialects (Db2) emit. | ||
| return fmt.Errorf("validation query returned no rows: %w", ErrQueryAffectedZeroRows) |
There was a problem hiding this comment.
🟠 Bug: This reinterprets every validation_queries "no rows" as "already applied", for all engines and all configs — not just DDL dialects. This repo's own shipped example (examples/postgres-test.yml:384-391) uses the grant validation query as an existence precondition (FROM users u, roles r WHERE u.username = ? AND r.role_name = ?), so after this change a grant for a nonexistent user or role returns nil error + GrantAlreadyExists and ConductorOne records access that was never applied. Suggest gating the new semantics behind an opt-in field on EntitlementProvisioningQueries (e.g. validation_queries_signal_idempotency: true) so existing precondition-style configs keep failing loudly.
| return anno, fmt.Errorf("grant provisioning: validation query returned no rows") | ||
| // Wrap the sentinel so the caller reports GrantAlreadyExists instead of failing; | ||
| // validation "no rows" is the only zero-effect signal DDL dialects (Db2) emit. | ||
| return anno, fmt.Errorf("grant provisioning: validation query returned no rows: %w", ErrQueryAffectedZeroRows) |
There was a problem hiding this comment.
🟠 Bug: Same concern as the sibling change in RunProvisioningQueriesWithExecutor — Grant (pkg/bsql/provisioning.go:89) now converts this into nil error + GrantAlreadyExists, so a genuine precondition failure (missing user/role, wrong tenant) is reported to ConductorOne as a successful grant instead of an error. Gate this behind opt-in config, or scope it to DDL dialects via s.dbEngine, rather than changing behavior for all existing configs.
Connector PR Review: CXH-2379: fix grant/revoke idempotency for DDL-based enginesBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe new commit is documentation and coverage only: a Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
A validation query returning no rows now maps to ErrQueryAffectedZeroRows (reported as GrantAlreadyExists / GrantAlreadyRevoked) only on DDL-based engines (Db2), which don't report rows-affected. Other engines keep using validation queries as existence preconditions that fail loudly, so a grant against a missing user or role is no longer silently reported as success. This also restores the grant_replace abort behavior on those engines: a replaced-grant revoke whose validation returns no rows returns a plain error instead of the sentinel, so GrantReplaced is not emitted. Document the engine-specific ValidationQueries semantics and add a test covering the non-DDL loud-failure path.
| } | ||
|
|
||
| func TestRevoke_ValidationNoRowsReportsAlreadyRevoked(t *testing.T) { | ||
| s, _ := newRevokeProvisioningTestSyncer(t) |
There was a problem hiding this comment.
🟡 Suggestion: the engine gate is covered asymmetrically — TestGrant_ValidationNoRowsOnNonDDLEngineFailsLoudly pins the non-DDL behavior for RunGrantProvisioning, but there's no equivalent for the revoke path through RunProvisioningQueriesWithExecutor. Adding a mirror test that leaves s.dbEngine at the default and asserts Revoke returns an error (and no GrantAlreadyRevoked) when the revoke validation query matches nothing would lock in both halves of validationNoRowsMeansIdempotent(). (medium confidence)
Mirror TestGrant_ValidationNoRowsOnNonDDLEngineFailsLoudly on the revoke path: on a non-DB2 engine, a revoke validation query returning no rows is a failed precondition, so Revoke returns an error with nil annotations rather than GrantAlreadyRevoked. Pins the false branch of validationNoRowsMeansIdempotent() for RunProvisioningQueriesWithExecutor.
|
|
||
| if !valid { | ||
| if s.validationNoRowsMeansIdempotent() { | ||
| return anno, fmt.Errorf("grant provisioning: validation query returned no rows: %w", ErrQueryAffectedZeroRows) |
There was a problem hiding this comment.
🟡 Suggestion: on the DB2 path this returns the sentinel with anno already populated, but Grant (pkg/bsql/provisioning.go:89-94) discards the returned annotations and builds a fresh annotations.Annotations{} with only GrantAlreadyExists. With no_transaction: true there is no rollback, so a grant_replace revoke that already committed above (line 1042) is lost: the DB revoked the old grant but GrantReplaced never reaches ConductorOne. Consider preserving the returned annotations in the errors.Is(err, ErrQueryAffectedZeroRows) branch of Grant (medium confidence — requires the DB2 + grant_replace + validation_queries + no_transaction combination).
| // don't report rows-affected, so the validation query is the only zero-effect signal | ||
| // available. Engines that report rows-affected keep using validation queries as | ||
| // existence preconditions that fail loudly. | ||
| func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { |
There was a problem hiding this comment.
[Review] this might mask real failures, not just idempotency
So validationNoRowsMeansIdempotent() just checks the engine, it doesn't care what the validation query was actually checking. Problem is, examples/postgres-test.yml:384-391 already does validation_queries as a plain existence precondition (checks a user + role row exist) that has nothing to do with grant idempotency — idempotency there is handled separately via ON CONFLICT DO NOTHING in the actual insert.
If someone copies that exact pattern into a DB2 config, a typo'd or dropped role would make the validation query return 0 rows too, and now that silently becomes GrantAlreadyExists/GrantAlreadyRevoked instead of a hard error. Kinda the opposite failure mode from what this PR is fixing lol. Nothing else in the config (RejectIf is opposite polarity + grant-only, PrincipalExistsCheck runs after commit and is non-fatal) would catch it, and the new tests only cover the intended idempotency-gate case.
Might be worth a flag/second list to distinguish "precondition" queries from "idempotency gate" queries, or at least a loud warning in the ValidationQueries doc comment so DB2 authors don't reuse the postgres-test.yml pattern verbatim.
| // don't report rows-affected, so the validation query is the only zero-effect signal | ||
| // available. Engines that report rows-affected keep using validation queries as | ||
| // existence preconditions that fail loudly. | ||
| func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { |
There was a problem hiding this comment.
[Review] why only DB2?
The bug report and this PR's own description frame this as a general "DDL-based engines" problem, not a DB2-only thing — and examples/oracle-test.yml has the exact same DDL-shaped GRANT ... TO / REVOKE pattern, so Oracle is probably exposed to the same bug. Hardcoding database.DB2 here means it's still broken there.
Could totally be intentional (only fix what's actually verified, per the stability-first vibe of this repo) — if so no action needed, just curious if that's the reasoning or if it's worth a quick follow-up ticket for Oracle/MSSQL/HDB/Vertica too.
| } | ||
|
|
||
| if !valid { | ||
| if s.validationNoRowsMeansIdempotent() { |
There was a problem hiding this comment.
[Review] duplicated with the same block in RunGrantProvisioning
This loop and the one in RunGrantProvisioning (~line 1087) were already near-identical before this PR, and now they both got the same 4-line idempotency check copy-pasted in. They've actually already drifted from each other independently — the RunGrantProvisioning copy does _ = result.Close() on the result.Err() failure path, this one doesn't.
Not blocking, just a nit — might be worth pulling into a shared runValidationQueries() helper since this PR touched both spots identically anyway, so future changes (like extending the DB2 check to other engines) don't need to be applied twice.
- Extract shared runValidationQueries helper so the grant and revoke validation loops stop drifting (the copies had diverged on result.Close). - Warn in the ValidationQueries doc comment that DDL-engine authors must not reuse validation_queries as an existence precondition, since a no-rows result is reported as idempotent success and would mask real failures. - Preserve annotations returned by RunGrantProvisioning in the already-exists branch so a GrantReplaced from a committed grant_replace revoke survives.
| // Reuse the returned annotations so a GrantReplaced from an already-committed | ||
| // grant_replace revoke survives; a fresh set would drop it. | ||
| anno.Update(&v2.GrantAlreadyExists{}) |
There was a problem hiding this comment.
🟠 Bug: reusing anno here is only safe when the grant ran with no_transaction: true. In the default transactional path, RunGrantProvisioning returns anno, ErrQueryAffectedZeroRows (query.go:1079 / 1112) before committed = true, so the deferred tx.Rollback() undoes the grant_replace revoke — yet the GrantReplaced annotation set at query.go:1070 now survives and is reported to C1 as if the old grant were removed. Gate the reuse on provisioningConfig.Grant.NoTransaction (or drop GrantReplaced in RunGrantProvisioning when the tx is rolled back).
On the transactional grant path, RunGrantProvisioning returns the zero-rows sentinel before commit, so the deferred rollback undoes any grant_replace revoke. Grant reused those annotations, reporting GrantReplaced for a removal the database no longer reflects. Keep the returned annotations only on the no_transaction path, where the replace already committed; otherwise return a fresh GrantAlreadyExists. Adds regression tests for both the rolled-back (no GrantReplaced, old grant survives) and committed (GrantReplaced, old grant gone) paths.
| anno := annotations.Annotations{} | ||
| // Reuse the returned annotations so a GrantReplaced from an already-committed | ||
| // grant_replace revoke survives; a fresh set would drop it. | ||
| anno.Update(&v2.GrantAlreadyExists{}) |
There was a problem hiding this comment.
Reusing anno here can report a replacement that never committed. In the default transactional path, RunGrantProvisioning can add GrantReplaced and then return ErrQueryAffectedZeroRows; the deferred rollback restores the revoked grant, but this branch preserves GrantReplaced. Please drop that annotation when the transaction rolls back, or only preserve it when the revoke was already committed.
| // don't report rows-affected, so the validation query is the only zero-effect signal | ||
| // available. Engines that report rows-affected keep using validation queries as | ||
| // existence preconditions that fail loudly. | ||
| func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { |
There was a problem hiding this comment.
Why is this limited to Db2? The pinned go-ora v2.8.24 driver classifies Oracle GRANT/REVOKE statements as OTHERS, and RowsAffected() is populated from CurRowNumber, so Oracle can expose the same zero-row DDL behavior. Please either include Oracle with an executable regression test or document concrete runtime evidence that its validation semantics differ.
| if !valid { | ||
| return anno, fmt.Errorf("grant provisioning: validation query returned no rows") | ||
| } | ||
| if err := s.runValidationQueries(ctx, validationQueries, vars, executor); err != nil { |
There was a problem hiding this comment.
🟡 Suggestion: runValidationQueries is also reached from the grant_replace path above via RunProvisioningQueriesWithExecutor (line 1057). On DB2 the revoke's own validation_queries returning no rows now yields the wrapped ErrQueryAffectedZeroRows sentinel, which lines 1064-1068 swallow — so anno.Update(&v2.GrantReplaced{...}) at line 1070 still fires even though no revoke query ran. Previously that validation failure aborted the grant. The reported end state (old grant absent) is arguably still true, but this is the same reporting class as the bug fixed in provisioning.go, and neither new test covers DB2 + grant_replace; worth an explicit decision plus a case in provisioning_grant_replace_test.go.
| // ValidationQueries is a list of SQL statements run before the provisioning queries. | ||
| // On engines that report rows-affected, a query returning no rows fails the operation | ||
| // (an existence precondition). On DDL-based engines (Db2) that don't report rows-affected, | ||
| // a query returning no rows instead means the state is already as desired, so the operation | ||
| // is reported as an idempotent success (GrantAlreadyExists / GrantAlreadyRevoked). | ||
| // | ||
| // Warning: on DDL-based engines, do NOT use these as existence preconditions | ||
| // (e.g. "does this user/role exist?"). A no-rows result is reported as idempotent | ||
| // success, so a missing or mistyped principal is silently swallowed instead of erroring. |
There was a problem hiding this comment.
🟡 Suggestion: this footgun warning only lives in a Go struct comment, but the people it targets are writing DB2 YAML configs. docs/db2.md has no provisioning/validation_queries section at all, so a config author has no way to learn that on Db2 a no-rows validation result is silently converted into an idempotent success. Consider adding the same warning to docs/db2.md.
On Db2 a grant_replace revoke whose validation query returns no rows swallows ErrQueryAffectedZeroRows and still reports GrantReplaced: the old grant is already gone, which is the state a replace aims for. Document this at the guard, cover it with a DB2 test, and add a validation_queries section to docs/db2.md warning against using them as existence preconditions on Db2.
| // On DDL engines (Db2) a zero-rows revoke means "nothing to revoke": the | ||
| // old grant is already gone, which is the state a replace aims for. Treat | ||
| // that as success and still report GrantReplaced. Any other error aborts. |
There was a problem hiding this comment.
🟡 Suggestion: this comment attributes an engine gate the guard doesn't have. ErrQueryAffectedZeroRows reaches here two ways: from runValidationQueries (DB2-only, via validationNoRowsMeansIdempotent) and from the revoke queries themselves affecting zero rows (query.go:705-707) — which is engine-agnostic. So on Postgres/MySQL/etc. a replace revoke whose DELETE matches nothing is also swallowed and still reports GrantReplaced. That swallow predates this PR, so the fix is wording, not gating: say the sentinel covers both sources rather than framing it as DDL/Db2-specific, so a future reader doesn't assume validationNoRowsMeansIdempotent() guards this branch.
| - **Db2:** a `validation_query` returning no rows is reported as an **idempotent success** | ||
| (`GrantAlreadyExists` on grant, `GrantAlreadyRevoked` on revoke). No rows means "the state | ||
| is already as desired, there is no work to do". |
There was a problem hiding this comment.
🟡 Suggestion: this section covers grant and revoke but omits the grant_replace case, which is the highest-consequence instance of the footgun it warns about. On Db2, when a grant_replace revoke's own validation_queries return no rows, query.go:1064-1075 swallows the sentinel and still emits GrantReplaced with the old grant's ID — so ConductorOne drops that grant from its graph even though no REVOKE ran and the membership may still exist in the database. Worth a third bullet stating that on Db2 a grant_replace revoke's validation query returning no rows is reported as GrantReplaced, so it must answer "is the old membership present?" and never be an existence check.
Oracle GRANT/REVOKE are DDL like Db2: an already-applied REVOKE raises ORA-01951 (and re-GRANT succeeds without affecting rows), so the validation query is the only zero-effect signal. Add Oracle to validationNoRowsMeansIdempotent() so validation 'no rows' maps to GrantAlreadyExists / GrantAlreadyRevoked instead of failing the task. Verified live against Oracle XE 21c (grant/re-grant -> GrantAlreadyExists, revoke/re-revoke -> GrantAlreadyRevoked, no ORA-01951, revoke DDL skipped). Adds an engine-gate regression test and updates the ValidationQueries doc.
4 participants
Repeat grant or revoke requests against DDL-based databases (such as Db2) no longer fail; the connector now recognizes when access is already in the requested state and reports the operation as a successful no-op.