agent: discovers authorize data-planes via Snapshot by bbartman · Pull Request #3386 · estuary/flow

bbartman · 2026-08-20T12:32:37Z

What

Follow-up promised by #3373 (issue #2781): the discovers executor's data-plane
authorization moves off internal.user_roles() SQL and onto the authorization
Snapshot already pinned for the operation. After this change the discover
path evaluates no authorization decision in SQL.

The inline query fetching the discover's data plane (gated on
internal.user_roles($2, 'read')) is deleted, along with its .sqlx cache
entry, in favor of Snapshot::data_plane_by_catalog_name +
tables::UserGrant::is_authorized at models::Capability::Read — the same
composition the GraphQL dataPlanes resolver uses for the identical
decision.
Scope is deliberately minimal: an unauthorized plane and a missing one
remain conflated as JobStatus::NoDataPlane, exactly like the SQL this
replaces. Distinguishing them (terminal NotAuthorized vs stale-snapshot
retry/deferral) is left to following commits, as is publications' twin of
this query — resolve_live_specs' data_plane_names authorization — which
is annotated here as the remaining SQL duplicate to be strangled out.
A NoDataPlane outcome now cancels the pinned Snapshot's revoke token
(the controlplane.rs precedent), requesting an early background refresh so
a plane or grant created after the Snapshot was taken is visible to a manual
retry within roughly MIN_REFRESH_INTERVAL instead of up to
MAX_REFRESH_INTERVAL.
Behavior change: a data plane with no HMAC keys at all is excluded from
the Snapshot by construction, so it now yields NoDataPlane instead of
proceeding toward a discover that could never sign its way into the plane.

Tests

Written first, red-green (each phase is its own commit):

Characterization, green before and after the swap:
test_discover_unauthorized_data_plane (plane exists outside the tenant's
ops/dp/public/ read grant), test_discover_missing_data_plane
(authorized prefix, no such plane) — both pin NoDataPlane.
New behavior, committed red and turned green by the swap:
test_discover_keyless_data_plane (previously proceeded to Success via
SQL), test_discover_no_data_plane_requests_snapshot_refresh (revoke token
cancelled).
Harness: add_data_plane (a second plane, so denied ≠ missing) and
queue_user_discover_in_plane (caller-chosen data_plane_name).

All discover-related suites pass (user_discovers, auto_discovers,
created_at, discovers::test).

Test-first phase for moving discovers' data-plane authorization off internal.user_roles() SQL and onto the pinned authorization Snapshot, the follow-up promised by #3373 (issue #2781). Harness: add_data_plane inserts a second plane so that *denied* is distinguishable from *missing*, and queue_user_discover_in_plane queues a discover against a caller-chosen plane (queue_user_discover delegates with the default test plane). Four new integration tests: - test_discover_unauthorized_data_plane and test_discover_missing_data_plane are characterization tests pinning today's SQL behavior (both yield NoDataPlane); they pass now and must keep passing after the swap. - test_discover_keyless_data_plane and test_discover_no_data_plane_requests_snapshot_refresh assert the new Snapshot-based behavior and are committed RED: the keyless plane test fails with "expected NoDataPlane, got Success" (the SQL path returns keyless planes), and the refresh test fails because the SQL path never cancels the Snapshot's revoke token. The next commit turns them green.

Replace the last internal.user_roles() evaluation in the discover path -- the inline SQL fetching the discover's data plane -- with in-process evaluation against the Snapshot already pinned for the operation, completing for discovers what #3373 started (issue #2781). The check composes Snapshot::data_plane_by_catalog_name with tables::UserGrant::is_authorized at models::Capability::Read, exactly as the GraphQL dataPlanes resolver decides the same question. An unauthorized plane and a missing one remain deliberately conflated as JobStatus::NoDataPlane, matching the SQL this replaces; distinguishing them (and any retry/deferral behavior) is left to following commits. A NoDataPlane outcome now cancels the pinned Snapshot's revoke token, following the controlplane.rs precedent: the plane or its grant may have been created after the Snapshot was taken, and requesting an early background refresh narrows the staleness window for a manual retry. Behavior change: a data plane with no HMAC keys at all is excluded from the Snapshot by construction, so it now yields NoDataPlane instead of proceeding toward a discover that could never sign its way into the plane. The orphaned .sqlx cache entry for the deleted query is removed. Turns the two red tests from the previous commit green; the two characterization tests keep passing unchanged.

…rangling The data_plane_names branch of resolve_live_specs is now the last duplicate of the data-plane read-authorization decision that discovers evaluate in-process against the authorization Snapshot. Annotate it as such; migrating publications onto the Snapshot is left to following commits (issue #2781).

bbartman · 2026-08-20T12:32:45Z

Implementation plan: discovers data-plane authorization via Snapshot

Issue #2781 follow-up promised by #3373: replace the last internal.user_roles()
SQL evaluation in the discover path — the inline data-plane fetch in
crates/agent/src/discovers.rs — with an in-process check against the
authorization Snapshot already pinned for the operation.

Design decisions

Scope: mechanical data-source swap only. None of the retry/deferral
machinery from the control-plane: Snapshot authorization model and retryable staleness error (1/7) #3341–agent: discovers executor authorizes data-planes via Snapshot and defers on staleness (4/7) #3344 stack lands here; DiscoverState stays dead.
Only discovery is affected — no other caller changes behavior.
Shape: inline composition in DiscoverExecutor::process, mirroring the
GraphQL dataPlanes resolver — Snapshot::data_plane_by_catalog_name for
the lookup plus tables::UserGrant::is_authorized for the decision. No new
shared helper yet.
Capability: models::Capability::Read (legacy → Viewer bundle bits,
all required), byte-identical to the GraphQL resolver, the
authorize_user_* endpoints, and the agent: discovers executor authorizes data-planes via Snapshot and defers on staleness (4/7) #3344 stack's own choice. A dedicated
fine-grained "use data plane" bit is a possible future sweep, out of scope.
Denied vs absent stay conflated as JobStatus::NoDataPlane, exactly
like today's SQL. Untangling them (terminal NotAuthorized vs stale-retry)
is for following commits.
Revoke cancel: on NoDataPlane, cancel the pinned Snapshot's revoke
token (the controlplane.rs precedent) to request an early background
refresh, narrowing the staleness window for a manual retry from up to 5
minutes to roughly the 20s MIN_REFRESH_INTERVAL. Not retry behavior — the
discover still fails terminally.
Accepted behavior change: a data plane with no HMAC keys at all is
absent from the Snapshot by construction (try_fetch filters them), so it
now yields NoDataPlane instead of proceeding toward a doomed discover.
Strangle annotation: one comment on the data_plane_names
authorization branch of publications' resolve_live_specs
(crates/control-plane-api/src/publications/specs.rs) marking it as the
remaining SQL duplicate of this decision, to be strangled out in following
commits. No other internal.user_roles caller is a duplicate of this
decision; none are touched.

Phases (one commit each)

Phase 1 — tests first (red-green)

Harness additions:

add_data_plane(name, hmac_keys) — a second plane, so denied is
distinguishable from missing in tests.
queue_user_discover_in_plane(...) — queue a discover against a
caller-chosen data_plane_name (the existing helper hardcodes
ops/dp/public/test and now delegates).

Four new integration tests in user_discovers.rs:

Test	Scenario	Before swap	After swap
`test_discover_unauthorized_data_plane`	plane exists, outside tenant's `ops/dp/public/` read grant	green (`NoDataPlane`)	green
`test_discover_missing_data_plane`	authorized prefix, plane doesn't exist	green (`NoDataPlane`)	green
`test_discover_keyless_data_plane`	readable plane with no HMAC keys	red (SQL returns it; discover proceeds)	green (`NoDataPlane`)
`test_discover_no_data_plane_requests_snapshot_refresh`	`NoDataPlane` outcome cancels the Snapshot's revoke token	red (SQL path never touches revoke)	green

The first two are characterization tests pinning today's behavior; the last
two assert the new behavior and are committed red with the failure shape
documented, then flipped green by Phase 2.

Phase 2 — the swap

In DiscoverExecutor::process:

Delete the inline sqlx::query_as! joining internal.user_roles() (and
its orphaned .sqlx cache entry).
Evaluate tables::UserGrant::is_authorized(&snapshot.role_grants, &snapshot.user_grants, row.user_id, &row.data_plane_name, models::Capability::Read) and resolve the plane via
snapshot.data_plane_by_catalog_name, cloning the tables::DataPlane into
the prepared Discover.
Denied-or-absent → cancel snapshot.revoke, warn, return
JobStatus::NoDataPlane (unchanged status).

All four Phase-1 tests green, plus the existing discover suites
(user_discovers, auto_discovers, created_at, discovers::test).

Phase 3 — strangle annotation

Comment on the publications resolve_live_specs data-plane query naming it
the remaining SQL duplicate of this decision, to be strangled out in
following commits. No functional change.

Discover now holds a &Snapshot rather than the Arc'd Refresh token: snapshot.result().unwrap() is called exactly once per discover path (in DiscoverExecutor::poll for user discovers, and in PGControlPlane::discover for auto-discovers, which already resolved it) and a reference flows through prepare_discover and build_merged_catalog. The callers own the Refresh for the duration of the awaited discover, so the Snapshot remains pinned exactly as before.

bbartman · 2026-08-20T12:57:37Z

Refactored so the pinned authorization Snapshot is resolved exactly once per discover: Discover now holds a &Snapshot instead of the Arc<tokens::Refresh<Snapshot>>, and the single snapshot.result().unwrap() happens in DiscoverExecutor::poll (user discovers) / PGControlPlane::discover (auto-discovers, which already resolved it for the data-plane lookup). prepare_discover and build_merged_catalog take the reference, so their redundant unwraps are gone. The callers own the Refresh for the duration of the awaited discover, so pinning semantics are unchanged. Commit: 7ae5c78.

Extract the queue/run/assert-NoDataPlane shape shared by the four data-plane authorization tests into a discover_expecting_no_data_plane helper, leaving each test only its distinguishing setup. The harness's default test plane is now inserted through add_data_plane -- which gains an explicit data_plane_fqdn parameter so the default row is unchanged -- instead of restating the insert in the setup CTE. The revoke-cancel rationale comment duplicated from crate::discovers is trimmed to a pointer.

jgraettinger

LGTM % nits

- Collapse the four NoDataPlane integration tests into one, asserting the Snapshot revoke cancellation for every case (unauthorized, missing, keyless) instead of only the missing-plane one. - Collapse the authorization check and data-plane lookup into a single let-else chain. - Trim comments: describe present behavior rather than the replaced SQL, and drop prose restating what the borrow already enforces.

jgraettinger

LGTM

Thread the authorization Snapshot through the publications call chain, ahead of moving publications' authorization enforcement off internal.user_roles() SQL and onto the Snapshot (issue #2781), mirroring the discovers migration (#3373, #3386). No behavior change: the Snapshot is carried but not yet consulted. - DraftPublication carries a Snapshot pinned for the entire publication, so that once the following commits consult it, authorization decisions cannot flip between initialization, build, and retry attempts. - ExpandDraft holds the same pinned Snapshot as a field -- rather than changing the Initialize trait signature -- as it's the only Initialize which evaluates authorization. - PublicationsExecutor gains a snapshot_watch and pins one Snapshot per poll, as DiscoverExecutor does. - The remaining construction sites update mechanically: PGControlPlane::publish pins from its watch; the create-data-plane and update-l2-reporting handlers use the request Envelope's pinned Snapshot; and the test harness refreshes the shared watch before each publications poll (as it already did for discovers), which makes the RED refresh test's explicit refresh redundant.

Replace the last internal.user_roles() evaluation in the publications path -- the data_plane_names CTE filter of resolve_live_specs -- with in-process evaluation against the pinned Snapshot (issue #2781), completing for publications what discovers established in #3386. Names from storage mappings and the explicit build parameter are partitioned via tables::UserGrant::is_authorized at Capability::Read, and only authorized names reach the SQL fetch, so a denied plane stays exactly as invisible as a missing one. As with the SQL it replaces, the check applies unconditionally: controller publications by the system user included. Planes referenced by id are fetched unchecked, as those ids come from live specs which were themselves authorized. A denied name cancels the pinned Snapshot's revoke token, following the discovers precedent: the grant may have been created after the Snapshot was taken, and requesting an early background refresh narrows the staleness window for a manual retry. The cancel fires at partition time, before it's known whether the denied plane is actually used, so an unreadable-but-unused storage-mapping plane also triggers it: the publication still succeeds, at the cost of one early refresh. Turns the remaining RED test from the first commit green: test_publication_no_data_plane_requests_snapshot_refresh. The three characterization tests keep passing unchanged, and the .sqlx entry for the rewritten query is regenerated.

Test-first phase for moving publications' authorization enforcement off internal.user_roles() SQL and onto the pinned authorization Snapshot (issue #2781), mirroring the discovers migration (#3373, #3386). Harness: async_publication takes a caller-chosen data-plane name, with user_publication_in_plane exposing it; user_publication and create_user_publication delegate with the default test plane. Four new integration tests: - test_publication_drafted_name_requires_admin, test_publication_no_data_plane, and test_publication_storage_mapping_unreadable_plane are characterization tests pinning today's SQL behavior: drafting a spec requires admin on its name; a denied data-plane is indistinguishable from a missing one, whether requested explicitly or reached as a storage mapping's default plane; and an unreadable-but-unused mapping plane is silently tolerated. They pass now and must keep passing after the swap. - test_publication_no_data_plane_requests_snapshot_refresh asserts the new Snapshot-based behavior and is committed RED: the SQL path never cancels the Snapshot's revoke token. Following commits turn it green.

Thread the authorization Snapshot through the publications call chain, ahead of moving publications' authorization enforcement off internal.user_roles() SQL and onto the Snapshot (issue #2781), mirroring the discovers migration (#3373, #3386). No behavior change: the Snapshot is carried but not yet consulted. - DraftPublication carries a Snapshot pinned for the entire publication, so that once the following commits consult it, authorization decisions cannot flip between initialization, build, and retry attempts. - ExpandDraft holds the same pinned Snapshot as a field -- rather than changing the Initialize trait signature -- as it's the only Initialize which evaluates authorization. - PublicationsExecutor gains a snapshot_watch and pins one Snapshot per poll, as DiscoverExecutor does. - The remaining construction sites update mechanically: PGControlPlane::publish pins from its watch; the create-data-plane and update-l2-reporting handlers use the request Envelope's pinned Snapshot; and the test harness refreshes the shared watch before each publications poll (as it already did for discovers), which makes the RED refresh test's explicit refresh redundant.

Replace the last internal.user_roles() evaluation in the publications path -- the data_plane_names CTE filter of resolve_live_specs -- with in-process evaluation against the pinned Snapshot (issue #2781), completing for publications what discovers established in #3386. Names from storage mappings and the explicit build parameter are partitioned via tables::UserGrant::is_authorized at Capability::Read, and only authorized names reach the SQL fetch, so a denied plane stays exactly as invisible as a missing one. As with the SQL it replaces, the check applies unconditionally: controller publications by the system user included. Planes referenced by id are fetched unchecked, as those ids come from live specs which were themselves authorized. A denied name cancels the pinned Snapshot's revoke token, following the discovers precedent: the grant may have been created after the Snapshot was taken, and requesting an early background refresh narrows the staleness window for a manual retry. The cancel fires at partition time, before it's known whether the denied plane is actually used, so an unreadable-but-unused storage-mapping plane also triggers it: the publication still succeeds, at the cost of one early refresh. Turns the remaining RED test from the first commit green: test_publication_no_data_plane_requests_snapshot_refresh. The three characterization tests keep passing unchanged, and the .sqlx entry for the rewritten query is regenerated.

bbartman added 3 commits August 20, 2026 12:28

jgraettinger previously approved these changes Aug 20, 2026

View reviewed changes

Comment thread crates/agent/src/integration_tests/user_discovers.rs Outdated

Comment thread crates/agent/src/discovers.rs Outdated

Comment thread crates/agent/src/discovers.rs Outdated

Comment thread crates/control-plane-api/src/discovers/mod.rs Outdated

bbartman dismissed jgraettinger’s stale review via a957400 August 20, 2026 16:05

jgraettinger approved these changes Aug 20, 2026

View reviewed changes

bbartman merged commit 2b116b6 into master Aug 20, 2026
11 checks passed

bbartman mentioned this pull request Aug 20, 2026

agent, control-plane-api: publications authorize via Snapshot #3387

Open

bbartman mentioned this pull request Aug 21, 2026

agent: discovers require SpecEdit to the capture #3389

Merged

Navigation Menu

Uh oh!

agent: discovers authorize data-planes via Snapshot - #3386

agent: discovers authorize data-planes via Snapshot#3386
bbartman merged 6 commits into
masterfrom
bmb/2781-discovers-data-plane-authz

bbartman commented Aug 20, 2026

Uh oh!

bbartman commented Aug 20, 2026

Uh oh!

bbartman commented Aug 20, 2026

Uh oh!

jgraettinger left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgraettinger left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

bbartman commented Aug 20, 2026

What

Tests

Uh oh!

bbartman commented Aug 20, 2026

Implementation plan: discovers data-plane authorization via Snapshot

Design decisions

Phases (one commit each)

Phase 1 — tests first (red-green)

Phase 2 — the swap

Phase 3 — strangle annotation

Uh oh!

bbartman commented Aug 20, 2026

Uh oh!

jgraettinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgraettinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants