control-plane: data-plane selection at signup + colocated trial bucket (gated) - #3318
Draft
SeanWhelan wants to merge 8 commits into
Draft
control-plane: data-plane selection at signup + colocated trial bucket (gated)#3318SeanWhelan wants to merge 8 commits into
SeanWhelan wants to merge 8 commits into
Conversation
provision_tenant() now computes the tenant's default public data-plane and
storage-mapping specs in Rust instead of embedding a fixed CTE:
order_public_planes() puts the chosen (or default aws-us-east-1-c1) plane
first, and storage_specs() derives that plane's colocated S3 trial bucket
(estuary-trial-{region}-{sha256(name)[:8]}, matching est-dry-dock#326's
trial_bucket_name()) when it's a public AWS plane and colocation is enabled,
falling back to the existing gs://estuary-trial bucket otherwise. Behavior
is unchanged for existing callers (requested_data_plane=None,
colocate_trial_bucket=false).
sha2/hex move from dev-dependencies to dependencies since trial_bucket_name()
is now library code, not just test code. parse_data_plane_name and
DataPlaneCloudProvider are re-exported from the graphql module so
storage_specs() can classify the chosen plane's cloud provider.
The onboarding UI can now submit a chosen public data-plane alongside requestedTenant. The claim is untrusted client input: it must name an existing, non-deprecated ops/dp/public/ plane or the directive fails with invalidClaims, naming the rejected plane. A valid choice becomes the first (default) entry of the tenant's storage-mapping data_planes and, once COLOCATED_TRIAL_BUCKETS is enabled, resolves to that plane's colocated S3 bucket instead of gs://estuary-trial. The gate stays off until est-dry-dock#326 merges and real public AWS planes have converged (created their buckets) — flipping it early would point new tenants at buckets that don't exist yet. Test cases added: an out-of-namespace plane (private), a well-formed but nonexistent plane, and a valid plane that becomes PlaneTenantC's default — verified end to end via the existing directive-application integration harness, including the resulting storage_mappings rows.
Regenerated against a freshly-migrated scratch database using the supabase/migrations sqlx::test-style auth stub (00_polyfill.sql's ELSE branch), matching how #[sqlx::test]'s own ephemeral databases are built — so this cache is correct for local `cargo build`/`cargo test`. Caveat: the one entry touching auth.users (query-13b90322..., provision_tenant's main insert) infers its accounts_user_email parameter against the stub's `email text` column; real Supabase/GoTrue defines auth.users.email as varchar. This doesn't affect compiled behavior (both bind &str identically) but `cargo sqlx prepare --workspace --check` against the real local stack may still want to regenerate this one file — run `mise run build:sqlx-prepare` there before merging to be sure.
…e env read - Move DataPlaneCloudProvider and parse_data_plane_name out of the server::public::graphql module into crate::data_plane, alongside the other plane-domain helpers. Directive/provisioning logic (beta_onboard) reaching into a GraphQL-presentation module for domain parsing was a layering smell; graphql::data_planes.rs and create_data_plane.rs now both consume the shared home instead. - Add PUBLIC_DATA_PLANE_PREFIX and is_selectable_public_plane() so the "public, non-deprecated plane" predicate has one Rust-side definition instead of being re-expressed inline in the agent; the prefix is also bound as a SQL parameter in provision_tenant's public_planes query instead of a duplicated string literal. - Cache COLOCATED_TRIAL_BUCKETS behind a OnceLock so the gate is read from the environment once per process instead of on every signup. - Reorder Cargo.toml's sha2/hex to match the file's existing alphabetical dependency ordering. No behavior change: all existing unit/integration tests pass unmodified, verified against a real migrated Postgres.
provision_tenant's public_planes query now binds PUBLIC_DATA_PLANE_PREFIX as $2 instead of embedding it as a literal (see previous commit), which changes the query's hash.
…pecs - trial_bucket_name now parses the plane name itself and returns Option<(bucket, region)>, so a bucket can no longer be derived from a region that disagrees with its plane. Collapses the caller's match and makes the mismatched pair unrepresentable. Golden vector unchanged. - storage_specs drops its default_plane parameter, which was always all_planes.first(), and builds one store value that the collection and recovery specs derive from, instead of four json! literals that could drift apart. Output is byte-identical (serde_json maps are sorted). - Narrow the data_planes re-export left over from the rebase to a plain use; nothing outside the module referenced it.
… selection Selectability was defined in three places that could disagree: a hardcoded EXCLUDED_PUBLIC_DATA_PLANES list, the SQL that builds a new tenant's data_planes, and a separate existence query in the agent. Worse, provision_tenant ignored data_planes.closed entirely — so closing a plane removed it from the signup picker (publicDataPlanes already honors closed) while provisioning kept writing it into every new tenant's storage mapping. Now provision_tenant selects on 'not closed' and validates the requested plane against that same list, returning ProvisionError::PlaneNotSelectable which the agent maps to invalidClaims. The hardcoded list, the exported is_selectable_public_plane predicate, and the agent's existence query are all deleted, so signup does one query where it used to do two, and retiring a plane is a data change instead of a code change and a deploy. A migration marks gcp-us-central1-c1/c2 closed, preserving the behavior the deleted list encoded. Existing tenants on those planes are unaffected; closed only governs new selection. Verified with nextest (the repo serializes these DB tests via a serial-db-tests group): 262 tests across agent + control-plane-api pass, and the beta_onboard snapshot gains exactly one case — a closed public plane is rejected — with all ten pre-existing cases byte-identical.
6 tasks
Removing the re-export from graphql/mod.rs merged two separately-sorted runs of mod declarations into one, and removing the relocated tests left a trailing blank line. No functional change.
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
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:
New tenants can now pick a specific public data plane at signup. The
betaOnboarddirective claims gain an optionalrequestedDataPlane(full plane catalog name, e.g.ops/dp/public/aws-us-east-1-c1), validated server-side before it becomes the tenant's default data plane in theirstorage_mappings.This also lands the backend half of colocated trial buckets (Phase 2 of estuary/sre#29). When the chosen plane is a public AWS plane and
COLOCATED_TRIAL_BUCKETSis enabled,provision_tenantderives that plane's colocated S3 bucket instead ofgs://estuary-trial, using the same naming formula as est-dry-dock#326 (estuary-trial-{region}-{sha256(plane_name)[:8]}, golden-vector-tested on both sides). The gate stays off in this PR. Flipping it is a separate follow-up once est-dry-dock#326 merges and real public planes have actually converged and gotten a bucket. Until then, and always for GCP/Azure planes, nothing changes: tenants still land ongs://estuary-trial.Companion PR: estuary/ui#2045, which adds the picker itself. That one must merge and deploy after this one, not before (see notes below).
Workflow steps:
The onboarding UI shows a data-plane picker between the org name field and the survey (companion PR). Submitting without touching it still records an explicit default:
aws-us-east-1-c1, or the first available plane if that one isn't present.The submitted plane is untrusted client input.
provision_tenantselects the planes a new tenant may use — public prefix,not closed— and rejects anything outside that set withProvisionError::PlaneNotSelectable, which the directive turns intoinvalidClaimsnaming the rejected plane. If no claim is submitted at all, behavior is identical to today:aws-us-east-1-c1default,gs://estuary-trial.To turn on colocated buckets later, set
COLOCATED_TRIAL_BUCKETS=1(ortrue) on the agent's environment. No code change needed.Documentation links affected:
None. This is control-plane/agent-internal behavior and no user-facing docs cover the signup bucket-selection mechanics today.
Notes for reviewers:
Deploy order matters here. The
Claimsstruct usesdeny_unknown_fields, so this needs to merge and deploy before the UI PR ships. If an old agent getsrequestedDataPlanefrom a new UI, signup fails withinvalidClaimsnaming the unknown field. I checked this failure mode live, not just in theory.data_planes.closedis the single source of truth for what a new tenant may be placed on, and the migration in this PR marksgcp-us-central1-c1/c2closed. I started out with a hardcoded exclusion list for those two, which left "selectable" defined in three places that could disagree — the list, the SQL building the tenant'sdata_planes, and a separate existence query in the agent — whileprovision_tenantignoredclosedaltogether. That combination meant closing a plane would hide it from the picker (publicDataPlanesalready honorsclosed) but keep writing it into every new tenant's storage mapping. Now there's one query and one predicate, and retiring a plane is a data change rather than a code change plus a deploy. Existing tenants on those planes are unaffected;closedonly governs new selection.In
crates/control-plane-api/src/directives/beta_onboard.rs,provision_tenantnow computes plane ordering (order_public_planes) and storage specs (storage_specs) in Rust instead of a single hardcoded CTE. This is behavior-preserving for existing callers (requested_data_plane: None,colocate_trial_bucket: false), confirmed by the integration test's unchangedAcmeTenantsnapshot.trial_bucket_nameparses the plane name itself and returnsOption<(bucket, region)>rather than taking a region alongside the name, so a bucket can't be derived from a region that disagrees with its plane.parse_data_plane_nameandDataPlaneCloudProvideralso moved fromserver::public::graphql::data_planestocrate::data_plane— that logic was living in a GraphQL-presentation module despite being plain domain parsing the directive code needed too.The golden vector is pinned as a unit test:
ops/dp/public/aws-us-east-1-c1givesestuary-trial-us-east-1-ccc98e22. If this ever fails, the Rust and Python (est-dry-dock) derivations have drifted apart. Fix the drift, not the test.262 tests across
agentandcontrol-plane-apipass, along withcargo sqlx prepare --workspace --check. Note these have to run undercargo nextest—.config/nextest.tomlserializes the agent's DB tests via theserial-db-testsgroup, and under plaincargo testthey run in parallel against one database, truncate each other's fixtures, and throw dozens of failures that look like real breakage but aren't.Worth a look during review: the
beta_onboardsnapshot gains exactly one case in this PR — a closed public plane rejected withinvalidClaims— and every pre-existing case is byte-identical. That diff is the evidence the refactor didn't change behavior.I also live-tested this end to end on a local stack: a real signup through the actual onboarding form, gate on, plane matching the AWS naming convention, produced a
storage_mappingsrow with the exact derived S3 bucket name. After applying the migration,publicDataPlanesdrops the closed plane and the picker renders the rest.Known follow-ups, deliberately not in this PR:
storage_specsstill builds its JSON by hand rather than throughmodels::StorageDef, andCOLOCATED_TRIAL_BUCKETSis read via an env var rather than a clapArgsfield. The second is why the colocated S3 path has unit coverage but no end-to-end coverage — worth closing before the gate is flipped on, since the harness could then exercise it.