CXH-2379: fix grant/revoke idempotency for the Db2 engine - #151
al-conductorone merged 19 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}
Connector PR Review: CXH-2379: fix grant/revoke idempotency for the Db2 engineBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit is docs and tests 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.
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.
| // 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.
There was a problem hiding this comment.
Confirmed: Oracle got added (case database.DB2, database.Oracle: in validationNoRowsMeansIdempotent()), so this thread's question is answered — but it lands on exactly the blocking concern @mateoHernandez123 raised separately: Oracle has no build tag (pkg/database/oracle/*.go is plain package oracle, unlike Db2's //go:build db2), so it ships in every default binary, and there's no config-level opt-in flag anywhere in pkg/bsql/config.go. That means every existing Oracle deployment using validation_queries" as an existence precondition (the same pattern this repo's own postgres-test.ymldemonstrates as normal) now silently getsGrantAlreadyExists/GrantAlreadyRevoked` on a missing or mistyped principal instead of a loud error — with zero opt-in. Db2 could take this unconditionally because it's opt-in behind a build tag; Oracle can't. This still looks unresolved and blocking to me.
There was a problem hiding this comment.
reverted oracle, db2-only again. it's a follow-up behind a per-config opt-in since oracle ships default-on.
There was a problem hiding this comment.
intentional, db2 is the only engine i've verified this on. filed the oracle follow-up as CXH-2417:
https://linear.app/ductone/issue/CXH-2417/baton-sql-extend-ddl-grantrevoke-idempotency-to-oracle
- 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.
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.
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.
Addressed across commits a439489..8c0917e: idempotency gated to Db2+Oracle (Oracle verified live), GrantReplaced-on-rollback fixed, principal-exists probe skipped on validation-sourced no-rows, comment/doc accuracy fixes, docs/provisioning.md + README link, DB-name in auth error + driver-coverage note. All inline threads resolved.
| if !valid { | ||
| return fmt.Errorf("validation query returned no rows") | ||
| if s.validationNoRowsMeansIdempotent() { | ||
| return ErrValidationNoRows | ||
| } | ||
| return fmt.Errorf("validation query %q returned no rows", q) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the two branches are now asymmetric in diagnosability. The non-DDL branch gained %q with the offending query, but the DDL branch returns a bare ErrValidationNoRows and emits no log at all — so on Db2/Oracle the exact hazard the new docs/provisioning.md warns about (a mistyped principal_id making a validation query return no rows, reported to C1 as GrantAlreadyExists/GrantAlreadyRevoked) leaves nothing in the logs to diagnose it. Consider a l.Warn("validation query returned no rows; treating as idempotent success", zap.String("query", q)) before the return, or wrapping the query into the sentinel with fmt.Errorf("validation query %q returned no rows: %w", q, ErrValidationNoRows) — errors.Is against both sentinels keeps working either way. (medium confidence)
Adding Oracle to validationNoRowsMeansIdempotent() was a default-on breaking change: unlike Db2 (opt-in behind the db2 build tag), Oracle ships in every default build, so existing Oracle configs that use validation_queries as loud existence preconditions would silently start reporting GrantAlreadyExists / GrantAlreadyRevoked on a missing or mistyped principal. Gate back to Db2 only; Oracle stays a follow-up pending a per-config opt-in. - validationNoRowsMeansIdempotent(): Db2 only again; doc names the build-tag asymmetry so the gate isn't widened again by accident. - Engine-gate test asserts Oracle (and every non-Db2 engine) is false, guarding re-introduction. - Restore Db2-only wording in config.go, docs/db2.md, docs/provisioning.md. - runValidationQueries: on the idempotent path, wrap the query into the sentinel and Warn-log it, so a swallowed no-rows validation stays diagnosable (dropped when the shared helper was extracted).
…x-grant-and-revoke-idempotency-for-ddl-based # Conflicts: # pkg/connector/connector.go # pkg/database/autherror.go # pkg/database/autherror_test.go
| - **Account Provisioning**: Define schemas and credential options for user creation | ||
| - **Entitlements**: Permissions and roles that can be granted to resources | ||
| - **Provisioning Actions**: SQL queries for granting/revoking entitlements | ||
| - **Provisioning Actions**: SQL queries for granting/revoking entitlements; see [docs/provisioning.md](docs/provisioning.md) for `validation_queries` semantics (including the DDL-engine no-rows-means-idempotent behavior on Db2 and Oracle) |
There was a problem hiding this comment.
🟡 Suggestion: This says the no-rows-means-idempotent behavior applies to "Db2 and Oracle", but validationNoRowsMeansIdempotent() (pkg/bsql/query.go:625) gates on database.DB2 only, docs/provisioning.md explicitly states Oracle "still fail[s] loudly like everyone else", and TestValidationNoRowsMeansIdempotent_EngineGate asserts Oracle: false. An Oracle operator reading this line would write validation_queries expecting idempotent no-rows and instead get a hard failure. Drop "and Oracle".
| } | ||
| return annotations.New(&v2.GrantAlreadyExists{}), nil | ||
| } | ||
| return nil, err |
There was a problem hiding this comment.
🟡 Suggestion: The new comment above correctly reasons that on the no_transaction path a grant_replace revoke has already committed — but that reasoning only gets applied to the zero-rows branch. On this generic error path (e.g. the main grant query fails with a constraint violation after the replace revoke committed), anno is discarded and nil, err is returned, so the already-executed removal is never surfaced. The SDK likely drops annotations on an error return anyway, so the practical fix is a l.Warn here when provisioningConfig.Grant.NoTransaction && anno carries GrantReplaced, recording that the replace committed but the grant failed.
| // A zero-rows sentinel means the replace revoke had nothing to remove: either | ||
| // its validation query found no rows on a DDL engine, or the revoke queries | ||
| // matched nothing on any engine. Either way the old grant is already gone, the | ||
| // state a replace aims for, so report GrantReplaced. Any other error aborts. |
There was a problem hiding this comment.
grant_replace reports the old grant as removed even when no revoke statement ran.
Context for whoever reads this cold: grant_replace means "before granting X, revoke the grant Y that this query returns". GrantReplaced is the annotation that tells ConductorOne "Y no longer exists", and C1 drops Y from its graph on that word alone — it does not re-check the database.
This branch swallows every ErrQueryAffectedZeroRows and then emits GrantReplaced unconditionally, but after this PR that sentinel covers two different situations:
- the revoke statements ran and matched nothing → Y really is gone,
GrantReplacedis accurate; ErrValidationNoRows(Db2) → the validation query short-circuited before any revoke statement ran, so whether Y is gone depends entirely on that validation query honoring the contract indocs/provisioning.md.
The PR already draws exactly this distinction on the revoke path: runRevokeQueries threads fromValidation out specifically so RunRevokeProvisioning can skip the principal-exists probe and avoid reporting a deletion that never happened (query.go L492-496). The same reasoning applies here, one level up: a validation short-circuit is not evidence that the old grant is gone.
Either resolution works for me:
- gate the annotation on
!errors.Is(err, ErrValidationNoRows)— the failure mode is safe, C1 keeps Y and the next sync corrects it; or - keep the current behavior and say in the comment that it is load-bearing on the Db2
validation_queriescontract, so a reader understands the guarantee comes from config, not from the code.
What I would avoid is leaving the two paths asymmetric with no note, since the next reader will reasonably assume the fromValidation guard covers this call site too.
There was a problem hiding this comment.
kept the behavior, added a comment that it leans on the db2 validation_queries contract and shouldn't be generalized.
| withGrantReplaceConfig(s, true) // no_transaction: the replace stands on its own | ||
| revoke := s.config.StaticEntitlements[0].Provisioning.Revoke | ||
| revoke.ValidationQueries = []string{ | ||
| `SELECT 1 FROM user_roles WHERE user_id = ?<user_id> AND role = 'does-not-exist'`, |
There was a problem hiding this comment.
This fixture encodes the config the new docs tell users not to write, and then asserts the result as correct.
docs/provisioning.md, added in this PR, is explicit: on Db2 a revoke validation_query must answer "is there work to do?" — for a revoke, "is the old membership present?", so that no rows genuinely means "already revoked". It also warns that using it as an existence check silently masks a bad principal or role.
Here the old membership is viewer, but the validation query asks about role = 'does-not-exist', so it can never match in any database state. That is the masking case the doc warns about, not the idempotency case the PR is adding.
That is also why the test can assert two things that cannot both hold in a correct run: GrantReplaced for the viewer grant (L154-156) and viewer still present in the table (L159). Downstream that means C1 drops the grant while the row survives upstream, so the next sync re-creates it and the grant flaps between syncs.
Concrete suggestion: keep the validation query pointed at the real membership (role = 'viewer') and simply don't insert the viewer row in the setup. No rows then genuinely means "already revoked", GrantReplaced is accurate, and the test proves the idempotency reporting this PR is about. If you also want coverage for the misconfigured-query case, a second test asserting today's behavior and named for it (e.g. ...ValidationQueryIsExistenceCheck...) would make the trade-off explicit instead of implicit.
There was a problem hiding this comment.
intended db2 case, keeping it. the test name and the new code comment spell out the trade-off.
| // DDL engines are a follow-up: they ship default-on, so flipping this would break existing | ||
| // configs that use validation_queries as loud preconditions, and need a per-config opt-in first. | ||
| func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { | ||
| return s.dbEngine == database.DB2 |
There was a problem hiding this comment.
Non-blocking, and mostly about framing rather than this function.
Keeping the gate Db2-only looks right to me, and docs/provisioning.md is clear that Oracle keeps failing loudly until there is a per-config opt-in. Flipping it globally would silently reinterpret existing configs that use validation_queries as preconditions, which is a worse outcome than the current gap.
The mismatch is in the description: the title says "DDL-based engines" (plural) and the body "DDL-based databases (such as Db2)", while the behavior reaches exactly one engine, itself behind the db2 build tag. Someone hitting repeat-grant failures on Oracle will read the title, assume this shipped for them, and re-open the same investigation.
Could you narrow the title to Db2 and link the Oracle follow-up in the body? One thing worth capturing in that follow-up: the DDL-vs-DML label is not the deciding factor, the driver's rows-affected reporting is. The pinned go-ora returns a real RowsAffected for DML (command.go L300-L302), so the Oracle decision needs an observed zero-effect case rather than the category name.
There was a problem hiding this comment.
will narrow the title to db2 and link the oracle follow-up.
| if !valid { | ||
| return fmt.Errorf("validation query returned no rows") | ||
| if s.validationNoRowsMeansIdempotent() { | ||
| l.Warn("validation query returned no rows; treating as idempotent success", zap.String("query", q)) |
There was a problem hiding this comment.
Warn on what this PR is making the supported happy path.
This line fires on every idempotent grant or revoke on Db2 — the case the PR exists to support — and the message itself says "treating as idempotent success". Connector logs surface to operators, so re-running a grant that worked as designed produces warnings, and the level stops carrying information: if expected outcomes warn, real warnings stop standing out.
I realize the repo does use l.Warn elsewhere (resources.go has around a dozen), so this is not me pushing a foreign style. Those are one-shot configuration and parse problems where someone genuinely should look at the config. This one is per-request expected control flow. Debug is the level for that, and the fmt.Errorf on the next line already carries the query text for anyone debugging a specific request.
l.Debug(...) with the same fields keeps the diagnostic value and drops the noise.
…ant_replace contract
al-conductorone
changed the title
…ke doc comment, grant_replace docs
| if !valid { | ||
| return anno, fmt.Errorf("grant provisioning: validation query returned no rows") | ||
| } | ||
| if err := s.runValidationQueries(ctx, validationQueries, "grant provisioning", vars, executor); err != nil { |
There was a problem hiding this comment.
🟡 Suggestion: the grant path still uses rows-affected as a rollback signal, which this PR's own docs say Db2 can't provide. Just below (line ~1135) RunGrantProvisioning returns ErrQueryAffectedZeroRows before the tx.Commit() at ~1139, so the deferred rollback undoes the statements; Grant() then maps that to GrantAlreadyExists with a nil error. docs/db2.md:226 states Db2's GRANT/REVOKE "don't report rows-affected" — if that means RowsAffected() yields 0 (or errors, which also leaves it 0), then on the default transactional path every real Db2 grant is rolled back and reported as already-granted, and the new GrantReplaced behaviour documented at docs/db2.md:233 is discarded too (pkg/bsql/provisioning.go:99 keeps anno only when no_transaction is set). Note runRevokeQueries was explicitly fixed to commit in the all-zero case; the grant path wasn't. Worth confirming against live Db2 what go_ibm_db returns for DDL, and either mirroring the revoke commit-on-zero-rows behaviour or documenting no_transaction: true as required for Db2 grants.
| @@ -421,6 +428,7 @@ func (s *SQLSyncer) RunProvisioningQueries( | |||
| ctx, | |||
There was a problem hiding this comment.
[Review] heads up, this is gonna collide with #152
#152 (the Oracle opt-in follow-up) stacks on this branch, but it currently stacks on the pre-push tip so it doesn't have this operation param yet. It adds its own new param (signalIdempotency bool) to the exact same function chain (RunProvisioningQueries -> runValidationQueries etc). Rebasing #152 onto this will be a real, if mechanical, signature conflict across like 6 functions — worth flagging so whoever does that rebase threads both params through together instead of accidentally dropping this operation prefix fix.
There was a problem hiding this comment.
yeah. when i rebase #152 i'll thread both operation and signalIdempotency through together so the prefix fix doesn't get dropped.
| map[string]any{"principal_id": "user-1", "role": "admin"}, | ||
| db, | ||
| ) | ||
| require.ErrorIs(t, err, ErrQueryAffectedZeroRows) |
There was a problem hiding this comment.
[Review] nit: this test doesn't actually lock in the operation-prefix fix
Only checks errors.Is(err, ErrQueryAffectedZeroRows), nothing asserts the message actually contains "revoke provisioning" or whatever the operation string is. Given #152 is about to touch this exact code path (see my other comment), a require.Contains(t, err.Error(), "revoke provisioning") here would make sure that prefix survives the rebase instead of silently regressing.
…_transaction requirement
| Set `no_transaction: true` on every Db2 grant and revoke (as the shipped Oracle and Redshift DDL | ||
| examples do). Because Db2 `GRANT`/`REVOKE` report no rows-affected, the default transactional path | ||
| reads that as "affected zero rows" and rolls the statement back, so a real grant is undone and | ||
| reported as `GrantAlreadyExists`. With `no_transaction: true` the statement commits on its own and | ||
| the `validation_query` is the sole idempotency signal. |
There was a problem hiding this comment.
🟡 Suggestion: this paragraph turns no_transaction: true into a hard correctness requirement for Db2 grants, but nothing outside prose enforces it. There's no config-load validation, no test pinning the requirement, and no examples/db2-*.yml to copy from (Oracle/Redshift authors get working fixtures; Db2 authors get this paragraph). A config that omits it silently rolls back a real grant and reports GrantAlreadyExists — a success. Consider a warn-or-error at config load when dbEngine == DB2 and a grant's no_transaction is unset, so the requirement fails loudly instead of depending on the operator finding this section.
| ConductorOne to drop the old grant) instead of failing. Write that `validation_query` to answer | ||
| "is the old grant still present?" so no rows genuinely means "already removed". | ||
|
|
||
| Set `no_transaction: true` on every Db2 grant and revoke (as the shipped Oracle and Redshift DDL |
There was a problem hiding this comment.
This makes no_transaction: true a correctness requirement for every Db2 grant/revoke, but nothing in config load or Grant/Revoke enforces it. On the default transactional path a real Db2 GRANT reports no rows-affected, the statement is rolled back, and Grant still returns GrantAlreadyExists — C1 records a no-op while the role was never granted.
Reject the config (or fail Grant/Revoke) when the Db2 engine's grant/revoke omit no_transaction: true, so a missing flag cannot silently undo DDL. Pattern:
baton-sql/examples/oracle-test.yml
Line 303 in 111f499
…te, |keyword renderer, opt-in validation) - validationNoRowsMeansIdempotent: keep Db2 on by engine (unchanged from #151, build-tag gated), require the validation_queries_signal_idempotency opt-in only for Oracle, which ships in every binary. Retarget the opt-in-off tests to Oracle and add Db2 default-on tests; make the engine-gate matrix explicit. - Add a |keyword token renderer for multiword system privileges (e.g. CREATE SESSION): charset-allowlisted (letters/digits/single spaces), whitespace collapsed, injection values rejected. |unquoted stripped the space, |identifier would wrongly quote the clause. - validate.go: run validation_queries through validateVarsInQuery, and when the opt-in is set require at least one validation query plus no_transaction on DDL engines. Warn when the opt-in is set on a non-DDL engine. - examples/oracle-test.yml + docs/oracle.md: quote role/principal operands with |identifier and drop UPPER() so quoted case-sensitive identifiers match; render system privileges with |keyword. - Add an Oracle grant_replace opt-in-off regression test.
…config opt-in (#152) * gate ddl no-rows idempotency behind per-config opt-in for oracle and db2 * CXH-2417: add Oracle full-path idempotency tests; note admin non-idempotency in example Closes the review gap that Oracle was only exercised at the engine-gate unit level: add end-to-end Grant()/Revoke() tests under dbEngine=Oracle + opt-in that assert GrantAlreadyExists/GrantAlreadyRevoked. The SQLite harness can't bind Oracle's ":N" placeholders, so the validation query is bind-free; the main INSERT/DELETE is skipped on the idempotent no-rows path anyway. Also document in examples/oracle-test.yml that the admin (WITH ADMIN OPTION) entitlements omit validation_queries idempotency on purpose. * CXH-2417: address #152 review round (Db2 default-on, Oracle opt-in gate, |keyword renderer, opt-in validation) - validationNoRowsMeansIdempotent: keep Db2 on by engine (unchanged from #151, build-tag gated), require the validation_queries_signal_idempotency opt-in only for Oracle, which ships in every binary. Retarget the opt-in-off tests to Oracle and add Db2 default-on tests; make the engine-gate matrix explicit. - Add a |keyword token renderer for multiword system privileges (e.g. CREATE SESSION): charset-allowlisted (letters/digits/single spaces), whitespace collapsed, injection values rejected. |unquoted stripped the space, |identifier would wrongly quote the clause. - validate.go: run validation_queries through validateVarsInQuery, and when the opt-in is set require at least one validation query plus no_transaction on DDL engines. Warn when the opt-in is set on a non-DDL engine. - examples/oracle-test.yml + docs/oracle.md: quote role/principal operands with |identifier and drop UPPER() so quoted case-sensitive identifiers match; render system privileges with |keyword. - Add an Oracle grant_replace opt-in-off regression test. * CXH-2417: address review feedback on grant/revoke idempotency - reject validation_queries_signal_idempotency on non-DDL engines at config validation instead of logging an ignored-flag Warn - require no_transaction whenever a no-rows validation is reinterpreted as idempotent, so Db2's default-on path is covered, not just the raw flag - UPPER-normalize the Oracle role-membership principal (|unquoted + UPPER()) so default upper-folded users match - correct docs: on Db2 no-rows idempotency is on by default; the validation_queries_signal_idempotency flag is Oracle-only - replace the positional bools on the exported Run* provisioning functions with a ProvisioningOptions struct * CXH-2417: docs: no_transaction is required on Oracle-with-flag too, not just Db2
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.
Scope: Db2 only (behind the
db2build tag, per CXH-2044). Oracle uses the same DDLGRANT/REVOKEshape and is likely exposed on the revoke path, but that needs live verification and its own breaking-change call, tracked in CXH-2435.