control-plane, agent: discovers authorize live specs via Snapshot - #3373
Merged
Conversation
Begin strangling get_live_specs: replace it with an authorized variant that evaluates the user's CatalogRead capability against the in-memory authorization Snapshot before any database access, plus an unfiltered plain-fetch core. All three callers move over and the old function is deleted; the SQL authz-computing queries leave the discover path entirely. Discover executors pin one Snapshot per poll. An authorization denial is raised as a typed NotAuthorized error computed purely from the requested names, and rendered from its bare Display: denials are byte-identical whether the Snapshot was stale or authoritative, and whether the denied specs exist, so responses cannot be used to probe for spec existence. A denial under a Snapshot predating the queued discover cancels the Snapshot's revoke token to request an early background refresh; retries are user-initiated (no automatic deferral yet). Data-plane authorization still evaluates internal.user_roles() in SQL and is marked for a follow-up change.
…ge-phase denial Review follow-ups: NotAuthorized carries the required CapabilitySet and its Display derives the action phrase from it (CatalogRead keeps rendering as "read"), so a future caller requiring a different capability renders truthfully instead of claiming "read". New integration test covers a denial raised by the merge-phase collection fetch — a readable capture whose retained binding targets an unauthorized collection — asserting it renders identically to a precheck denial and requests no refresh when authoritative.
…actor - The 512-name chunking rationale referenced the deleted authz-computing query; note it's inherited and now only bounds statement size. - Document that get_live_specs_unfiltered binds but never evaluates user_id. - refresh_snapshot_taken_at positions the Snapshot in either direction (stale or authoritative), not only stale. - The "snapshots are never automatically refreshed" claim in the GraphQL helper is now specific to the API app's fixed watch, since the discovers executor's snapshot does refresh by default. - The prepare_discover expect message claimed validation "in poll", but the unit test calls it directly; state the caller invariant instead.
…phase denial Closes the two cheap gaps from the test review: Snapshot::taken_after gains its first unit test, pinning the TEMPORAL_SKEW boundary (ambiguous ordering within the allowance is not authoritative, including exactly at it); the merge-phase denial test gains the stale half of the matrix, asserting the early-refresh request and byte-identical rendering against the authoritative case.
Reverses the error-on-denial behavior: discovers now trust the pinned authorization Snapshot and silently omit any requested name the user lacks CatalogRead to, restoring the prior semantics where an unreadable spec is treated as absent and authorization errors surface at publish time. A filtered name is indistinguishable from one that doesn't exist, and no denial reaches the user. The Snapshot is trusted as-is: no staleness classification, no early-refresh request, and no retryable error. A grant committed after the Snapshot was taken becomes visible on the watch's own refresh cadence. Removes NotAuthorized, its capability-derived rendering, and the executor's downcast/revoke-cancel arm; get_live_specs_authorized becomes get_live_specs_filtered over a partition helper.
bbartman
force-pushed
the
bmb/2781-snapshot-refactor-2
branch
from
August 18, 2026 18:31
0d2460c to
2e2377f
Compare
jgraettinger
previously approved these changes
Aug 19, 2026
Snapshot refreshes are infallible -- a failed refresh only delays the next one, and the watch is awaited ready at startup -- so drop the unreachable error check when pinning the discover Snapshot, and correct the comments and expect messages that told callers to verify it. partition_by_authorization returns references into the requested names rather than cloning them. To make that stick, get_live_specs_unfiltered and fetch_live_specs take &str names, which also drops a name-cloning loop in the test harness. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
bbartman
added a commit
that referenced
this pull request
Aug 20, 2026
* agent: add data-plane authorization tests for discovers (red-green) 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. * agent: discovers authorize data-planes via Snapshot 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. * control-plane: mark publications' SQL data-plane authorization for strangling 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). * agent, control-plane-api: resolve pinned Snapshot once per discover 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. * agent: deduplicate discover test fixtures 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. * agent: address review nits on discover data-plane authorization - 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.
bbartman
added a commit
that referenced
this pull request
Aug 20, 2026
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.
bbartman
added a commit
that referenced
this pull request
Aug 25, 2026
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.
bbartman
added a commit
that referenced
this pull request
Aug 25, 2026
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.
GregorShear
added
pending:agent-api
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
3 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.
What
First slice of re-deriving #2781 as a strangler series (superseding the
#3341–#3344 stack): the discovers path moves its live-spec authorization off
SQL and onto the in-memory authorization
Snapshot.live_specs:get_live_specsis deleted, replaced by:get_live_specs_filtered— evaluates the user's capability to everyrequested name against a pinned
Snapshot, before any database access,and silently omits the names they don't hold it to.
get_live_specs_unfiltered— the plain-fetch core, used by theauto-discover path and
PGControlPlane::get_live_specs(controllers),byte-identical to the prior
None-filter behavior.partition_by_authorization— the pure decision function both thefiltering wrapper and its unit tests are built on.
Discovercarries an authorizationSnapshotpinned onceper executor poll, so decisions can't flip mid-operation (for example,
during a long-running connector RPC). Both the live-capture precheck and
the merge-phase collection fetch authorize via that Snapshot.
The Snapshot is trusted as-is: there is no staleness classification, no
early-refresh request, and no retryable error. A grant committed after the
Snapshot was taken becomes visible on the watch's own refresh cadence
(
MIN_REFRESH_INTERVAL20s /MAX_REFRESH_INTERVAL5m).Behavior
Authorization semantics are unchanged from master: a spec the user cannot
read is treated as absent, exactly as the SQL-filtered fetch did, and any
authorization error surfaces when they try to publish. A filtered name is
indistinguishable from one that doesn't exist.
What changes is where the decision is made — in-process against the
Snapshot's grant graph rather than by
internal.user_roles()in Postgres —which is the latency win #2781 asks for.
One consequence to be aware of: a re-discover queued in the window between a
grant landing in Postgres and the next Snapshot refresh evaluates against the
older grants, so an existing capture the user just gained access to can be
re-drafted as new. Bounded by the refresh cadence above.
Data-plane authorization still evaluates
internal.user_roles()in SQL andis commented as the next slice.
Tests
live_specs::test::test_partition_by_authorization(DB-free, overSnapshot::build_fixture): the authorized/denied split, sorted anddeduplicated, with unknown names and existing-but-unauthorized names
partitioning identically.
snapshot::tests::test_taken_after_allows_for_temporal_skew: first unitcoverage of the
TEMPORAL_SKEWboundary, which the API-layer callers oftaken_afteralso depend on.user_discovers::test_discover_filters_unauthorized_capture: the precheckfilter — the discover succeeds and drafts the unreadable capture as new.
user_discovers::test_discover_merge_filters_unauthorized_collection: themerge-phase filter — an unreadable binding target is drafted fresh rather
than merged against a live spec the user can't read.
Snapshot watch that the harness refreshes before each discover poll, so
grants created by a test are visible;
queue_user_discover/run_queued_discoversplit the queue and poll steps.🤖 Generated with Claude Code