feat(workspace): add workspace resource model with scoping, membershi…#2243
feat(workspace): add workspace resource model with scoping, membershi…#2243derekwaynecarr wants to merge 13 commits into
Conversation
77ef1fc to
7bba756
Compare
Fix bugs found during PR NVIDIA#2243 review: correct migration type strings, resolve platform-scoped profile workspace handling, fix sandbox workspace_tx single-fire, add missing workspace deletion blockers, reset TUI row indices on workspace switch, fix TUI provider update workspace, and update e2e Python tests for workspace and inference route API changes. Add Phase 2 TODO comments for authorization gaps. Fix pre-existing clippy errors (too_many_arguments, cast_sign_loss, iter_on_single_items). Signed-off-by: Derek Carr <decarr@redhat.com>
Fix bugs found during PR NVIDIA#2243 review: correct migration type strings, resolve platform-scoped profile workspace handling, fix sandbox workspace_tx single-fire, add missing workspace deletion blockers, reset TUI row indices on workspace switch, fix TUI provider update workspace, and update e2e Python tests for workspace and inference route API changes. Add Phase 2 TODO comments for authorization gaps. Fix pre-existing clippy errors (too_many_arguments, cast_sign_loss, iter_on_single_items). Signed-off-by: Derek Carr <decarr@redhat.com>
|
Follow-up to my earlier review comment. That pass reviewed the diff file-by-file; this pass audited the PR along four cross-cutting axes instead: (1) workspace provenance at every store call site, (2) workspace provenance at every client request-construction site, (3) the full cross-object reference graph under delete/recreate, and (4) a cross-layer behavior matrix (server/CLI/TUI/SDK/three drivers). Same scope as before: fresh install assumed, upgrade paths out of scope, missing authz deferred to Phase 2. The new findings are mostly relational — they live between components rather than inside any one file, which is why the first pass missed them. Additional must-fix candidates1. Watch-adoption path persists driver-supplied workspaces verbatim — one ghost record halts gateway-wide reconciliationThe create branch of
Consequences of the resulting Fix shape: normalize/validate the workspace at the adoption boundary (empty → 2. Inference routes reference providers by name with no delete guard and no cleanup — silent traffic rebinding
Secondary defect on the same edge: while the route dangles, the The codebase's own invariant elsewhere (the sandbox-attachment guard) is that name references must not dangle, so this is a gap, not name-as-identity by design. Fix shape: include inference routes in the provider delete guard (or cascade-delete them), and make 3. Provider deletion doesn't synchronize with attach — dangling attachment adopts a recreated provider's credentials
4. Service endpoints are never cleaned up on sandbox delete
Worth fixing, borderline blocking
Non-blocking notesDocs/help claim workspace names are DNS-1123/63 chars while the server enforces 19 ( Phase 2 readiness (informational — no action needed in this PR)Classified all 73 RPCs plus the data-plane paths for "can a membership check bolt on here." Good news: the What the audit confirmed cleanFor completeness, the coverage side: all 118 production store call sites classified (72 via |
russellb
left a comment
russellb
left a comment
There was a problem hiding this comment.
Automated workspace-scoping review of RFC 0011 Phase 1, focused on cross-workspace read/write confusion. Core control-plane read/write path scoped correctly; three actionable items below (one latent driver inconsistency + two hardening/upgrade notes).
|
Feedback from my agent review [P1] Do not discard startup activity before workspace discoveryLocations:
The denial and activity aggregators drain a batch before invoking their flush callbacks. When the workspace is not yet known, these callbacks return successfully, so the drained events are permanently lost. This can drop policy proposals and activity telemetry during normal sandbox startup. Buffer the batch until workspace discovery completes, wait on the workspace receiver before starting the aggregators, or return a retryable error so the aggregator retains the batch. [P1] Preserve global profile scope throughout provider creationLocation:
Pass [P1] Prevent resource creation from racing workspace deletionLocation:
Use a persisted deleting state checked by resource creation, transactional exclusion, or another mechanism that closes the check/delete race. [P2] Paginate sandbox-owned service cleanupLocation: The new service-endpoint cleanup lists only the first 1,000 endpoints in the workspace and then deletes the sandbox. A sandbox whose endpoints fall after that page is deleted while its endpoints remain. Those orphaned records then block workspace deletion and can be presented as belonging to a later same-named sandbox. Query endpoints by their persisted sandbox label, paginate until exhaustion, or store them under a sandbox-owned scope that supports [P2] Keep Python workspace synchronization test-cleanLocations:
The new unconditional Only resynchronize the workspace on the [P2] Scope TUI profile joins by workspaceLocations:
The all-workspaces provider view loads profiles only from Resolve profiles by [P2] Give all-workspace pagination a total orderingLocations:
Cross-workspace queries order results only by Add [P2] Persist workspace labels in the selector indexLocation:
Serialize the already-validated workspace labels into the persistence label column before calling [P2] Apply the new workspace-routing limit to generated sandbox namesLocations:
This PR lowers the explicit sandbox-name limit from 253 to 19 characters so Validate the generated name and regenerate or fall back until it satisfies the new routable-name constraint. [P2] Require managed-resource labels in driver ID lookupsLocations:
Several get, existence, and deletion paths filter only by the OpenShell sandbox ID. They can select an unmanaged platform object carrying the same label, and deletion may act on that object. Include the OpenShell managed-by label in every lookup, not only list/watch paths. Lower-priority feedback[P2] Enforce persistence workspace invariantsLocation: The new raw [P3] Complete the global-profile CLI surface
[P3] Qualify platform-profile dependency diagnosticsGlobal-profile validation and deletion diagnostics identify dependent sandboxes only by name and then deduplicate those names. For example, dependencies on both |
PR #2243 follow-up reviewPrincipal-engineer verdict: request changes. Reviewed the nine commits after the previous review, through head Prior findings
Critical findings
Warnings
Adjacent findings
Verification
|
Implements workspace and membership model providing hard isolation boundaries for multi-player OpenShell deployments. Workspace CRUD with Kubernetes-style Terminating phase for graceful deletion. All resources scoped by workspace via ObjectMeta. Membership RPCs for workspace access control. Persistence migration shifts name uniqueness to (object_type, workspace, name). Provider profiles support platform and workspace scoping. Service routing uses workspace-prefixed DNS labels. Inference routes renamed and workspace-scoped with DeleteInferenceRoute RPC. Python SDK with WorkspaceClient, two-method list pattern (workspace-scoped and for_all_workspaces), and workspace parameter on all methods. CLI workspace flags, TUI workspace cycling. K8s driver filters unmanaged CRs and uses delete preconditions. Podman driver uses immutable container IDs. Label serialization fixed across all put_if call sites. Signed-off-by: Derek Carr <decarr@redhat.com>
The standalone `sandbox upload` command reimplemented upload logic inline with two bugs: it used `Path::exists()` which follows symlinks (rejecting dangling symlinks), and it ran git-aware filtering on symlink sources. The `run::sandbox_upload()` function already handles both cases correctly via `sandbox_upload_plan()`. Replace the inline logic with a call to the existing function. Signed-off-by: Derek Carr <decarr@redhat.com>
Shorten the sandbox name in initial_sparse_policy_is_acknowledged_as_loaded from 'e2e-2159-sparse-enrich' (22 chars) to 'e2e-sparse-enrich' (17 chars) to comply with MAX_ROUTABLE_NAME_LEN (19 chars). Also capture stderr in create_keep_with_args so future sandbox creation failures include the actual CLI error instead of reporting empty output. Signed-off-by: Derek Carr <decarr@redhat.com>
… isolation Add unit tests for workspace create happy path, get round-trip, get not-found, get empty-name rejection, already-exists error, and resolve_workspace not-found. Add persistence test proving cross-workspace name uniqueness (same name in different workspaces produces separate records). Add workspace name max-length boundary tests. Fix e2e harness to include stderr in name-parse-failure error path. Align Python e2e test_workspace_crud with try/finally pattern. Document provider profile catalog workspace scoping gap in RFC 0011. Signed-off-by: Derek Carr <decarr@redhat.com>
Shorten sandbox names in demo scripts to fit the 19-character
MAX_ROUTABLE_NAME_LEN limit: policy-demo prefix to pd-, multi-agent
notepad derives a short SANDBOX_TAG from the run ID, governance
interceptor uses gs-PID-RANDOM. Update vscode-remote-sandbox.md SSH
host aliases from openshell-{name} to openshell-{name}.{workspace}
format.
Signed-off-by: Derek Carr <decarr@redhat.com>
Add WorkspaceScopedClient modeled after kube::Api::namespaced — captures workspace once and injects it into every sandbox request. Add workspace CRUD methods (create, get, list, delete) and list_sandboxes_all_workspaces on OpenShellClient. Extend SandboxRef with workspace field and add WorkspaceRef type. Include mock tests for all new operations. Signed-off-by: Derek Carr <decarr@redhat.com>
Signed-off-by: Derek Carr <decarr@redhat.com>
2410c7d to
a0985f7
Compare
|
/ok to test |
@derekwaynecarr, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test a0985f7 |
Signed-off-by: Derek Carr <decarr@redhat.com>
Auto-format with cargo fmt and fix clippy warnings exposed by the reformat: unnecessary qualifications, map_unwrap_or, identical match arms, unused variable prefix, dead code annotations, and let-unit-value in e2e harness. Signed-off-by: Derek Carr <decarr@redhat.com>
|
/ok to test 30136f7 |
- Add workspace field to settings JSON output (CLI) - Skip Podman containers missing workspace label instead of defaulting to empty string, matching K8s driver behavior - Add resource_version to list_by_scope SELECT in both SQLite and Postgres backends, with regression test - Gate PolicyLocalContext proposal/lookup routes on workspace readiness, returning 503 when workspace is not yet discovered - Block sandbox and provider creation in TUI all-workspaces mode - Clear workspace vectors in TUI reset_sandbox_state Signed-off-by: Derek Carr <decarr@redhat.com>
Thread workspace through snapshot_catalog so the EffectiveProviderProfileCatalog enforces workspace boundaries on both read and write paths. UserProviderProfileSource now loads platform-scoped profiles (workspace "") plus the target workspace's profiles, preventing cross-workspace duplicate profile ID collisions that previously caused global catalog failures. Update RFC 0011 to reflect catalog scoping is implemented in Phase 1 rather than deferred to future work. Signed-off-by: Derek Carr <decarr@redhat.com>
…INSERT put_policy_revision_atomic omitted the workspace column from the INSERT into the objects table in both SQLite and Postgres backends, causing atomically-written policy revisions to lose their workspace association. Add workspace field to AtomicPolicyRevisionWrite and thread it through both backend INSERT statements, matching the non-atomic put_policy_revision path which already included it. Signed-off-by: Derek Carr <decarr@redhat.com>
collect_ancestor_identities walked the entire process tree above the entrypoint when the connecting process was the entrypoint itself, SHA256-hashing every ancestor binary (IDE, shell, container runtime). On dev machines with large binaries in the ancestor chain this exceeded the 30-second test timeout. When start_pid == stop_pid there are no intermediate ancestors to verify, so return an empty list immediately. Signed-off-by: Derek Carr <decarr@redhat.com>
|
/ok to test 1682948 |
|
Label |
5 participants
Summary
Implements Phase 1 of RFC 0011 — the workspace and membership model that provides hard isolation boundaries for multi-player OpenShell deployments.
deletion_timestamp_mson ObjectMeta and phase on WorkspaceStatus) before scanning for blockers, closing the TOCTOU race between blocker scan and resource creation. Once Terminating, create-path operations are rejected with FailedPrecondition. CAS conflict on concurrent deletes is handled by re-fetching and proceeding if already Terminating. No UpdateWorkspace RPC — workspaces are immutable after creation. Workspace labels persisted in selector index column for label_selector queries.delete_by_scopestore method for sandbox-owned record cleanup.update_message_casenforces name and workspace immutability — mutation closures that attempt to change either field are rejected with an Encode error. Cross-workspace query ordering includes workspace and id tie-breakers for deterministic pagination.list_by_scopeSELECT in both SQLite and Postgres includesresource_versioncolumn — previously omitted, causing deserialized records to report the default version instead of the authoritative DB value.provider list-profiles --globallists platform-scoped profiles.resolve_profile_workspacepreserves empty string for platform scope whileresolve_workspacenormalizes to "default". Platform-profile attachment checks scan all workspaces to detect consumers; dependency diagnostics qualify sandbox names withworkspace/namefor platform-scope scans to prevent dedup collisions. Scope-mismatched providers contribute their real token-grant bindings for ambiguity detection instead of being skipped or matched against the wrong profile scope. TheEffectiveProviderProfileCatalog(snapshot_catalog) takes aworkspaceparameter so both read and write paths enforce workspace boundaries —UserProviderProfileSourceloads platform-scoped profiles (workspace"") plus target workspace profiles, preventing cross-workspace duplicate profile ID collisions. Builtins and interceptor-provided profiles remain workspace-agnostic.{workspace}--{sandbox}--{service}.{domain}format for all workspaces including default. Workspace, sandbox, and service names capped at 19 characters to guarantee the combined DNS label fits within the 63-character RFC 1035 limit. Auto-generated sandbox names (petname) truncated to the routable limit with trailing hyphen cleanup.put_ifcall sites now serializeobject_labels()into the persistence index column. Previously, most call sites passedNonefor labels, causinglist_with_selectorqueries to miss resources even when the protobuf payload contained labels. Fixed across sandbox create, provider create, inference route upsert, SSH session create/revoke, workspace member create, stored provider profile import, and default workspace creation.put_policy_revision_atomicINSERT statements (both SQLite and Postgres) now include theworkspacecolumn and bind. TheAtomicPolicyRevisionWritestruct gains aworkspace: Stringfield threaded through both production callers (handle_update_config_inner,apply_merge_operations_with_retry). Previously the atomic path — introduced in commit 83003e8 for interceptors (feat(interceptors): initial gateway interceptor implementation and reference example #2005) — omitted workspace, causing atomic policy writes to persist with an empty workspace regardless of the sandbox's actual workspace. The non-atomicput_policy_revisionwas already correct. Regression testpolicy_atomic_write_persists_workspaceverifies the workspace round-trips through the atomic path.collect_ancestor_identitiesreturns an empty vec immediately whenstart_pid == stop_pid(socket owner is the entrypoint). Previously it walked and hashed the full process ancestry above the entrypoint — desktop infrastructure binaries (cargo, IDE, shell) outside the sandbox trust boundary. No security regression: the connecting binary itself is still hashed byresolve_owner_identity, and ancestor verification only applies to intermediate processes between socket owner and entrypoint (there are none when they are the same PID). Fixes two proxy tests that consistently timed out at 30s due to hashing ~500MB+ of ancestor binaries in development environments.submit_proposalandopen_lookup_sessiongate on non-empty workspace — return 503 withworkspace_unavailableif the workspace watch channel has not yet been populated, preventing requests from being processed with an empty workspace before supervisor discovery completes. Denial and activity aggregators use aready_gateclosure checked beforedrain()— events are buffered until workspace discovery completes, with a warning emitted on shutdown if events must be dropped because workspace was never learned.sandbox_snapshotpropagates workspace so watch events round-trip the correct workspace back to the server.sandbox_from_objectgates onmanaged-by=openshelllabel — CRs written by other actors in the namespace are silently skipped. Managed CRs missing required fields (orphans) log a warning and are skipped rather than hard-erroring. Label selector added to list, watch, get, delete, and existsListParamsso unmanaged CRs are filtered at the K8s API level.is_openshell_managedhelper centralizes the ownership check.list_containersAPI changed to accept&[&str]label filters. All ID-based lookups (find, exists, get) include the managed filter alongside the sandbox ID filter to prevent matching unmanaged containers with colliding labels. Workspace label lookup indriver_sandbox_from_inspect,driver_sandbox_from_list_entry, and error event construction changed from.unwrap_or_default()to.cloned()?— containers missing the workspace label are skipped instead of defaulting to empty string, preventing pre-upgrade containers from being reconciled with incorrect workspace attribution.settings_to_json_sandboxincludesworkspacefield in JSON output so sandbox settings correctly reflect the workspace context. --workspace and --all-workspaces flags on all resource commands; --all-workspaces intentionally takes precedence over --workspace (rather than erroring) because --workspace can be set viaOPENSHELL_WORKSPACEenv var. WORKSPACE column as first column in list outputs when --all-workspaces (matching kubectl --all-namespaces convention);-o json/-o yamloutput always includesworkspacefield;--namesoutput usesworkspace/nameformat under--all-workspaces(matching kubectl-o name --all-namespaces). Workspace CRUD and membership subcommands; STATUS column in workspace list (Active/Terminating); inference help text updated from "gateway-level" to "workspace-level".provider create --global-profileusesprofile_workspaceconsistently across all profile lookup paths (discovery, runtime credentials, from-existing). Last-used sandbox fallback keyed by (gateway, workspace) — saves workspace alongside sandbox name and only matches the requested workspace, preventing cross-workspace mis-targeting.--workspace/OPENSHELL_WORKSPACE(no longer hardcoded to "default"). Selection indices reset on workspace cycle. Post-create exec uses the creation workspace (carried in CreateResult) instead of the selected row's workspace. cycle_workspace resets current_workspace to "default" when entering all-workspaces mode to prevent silent cross-workspace creates. Sandbox and provider create operations blocked in all-workspaces mode with a status message — resources require a specific workspace target.reset_sandbox_stateclearssandbox_workspacesandprovider_workspacesvectors to prevent stale workspace data after state transitions. Provider profile refresh fetches profiles per distinctprofile_workspaceand caches by(workspace, type)— provider detail joins by unique ID instead of name to prevent cross-workspace misattribution.wait_ready/wait_deleteduse the correct workspace when attaching viaSandboxRef)._workspaceaccess guarded withgetattrfallback for session objects that lack the attribute (e.g. test doubles). Workspace-scoped listing vialist(workspace=...)/list_ids(workspace=...)and cross-workspace listing vialist_for_all_workspaces()/list_ids_for_all_workspaces()(follows Kubernetes two-method pattern — no "required but ignored" parameters). NewWorkspaceClientwith CRUD operations (create, get, list, delete) andWorkspaceRefdataclass; membership management deferred to Phase 2.WorkspaceScopedClientmodeled afterkube::Api::namespaced— captures workspace once and injects it into every sandbox request (create, get, list, delete, wait_ready, wait_deleted, exec). Constructed viaOpenShellClient::workspace("name"). Workspace CRUD methods onOpenShellClient(create_workspace, get_workspace, list_workspaces, delete_workspace) andlist_sandboxes_all_workspacesfor cross-workspace listing.SandboxRefextended withworkspacefield populated fromObjectMeta.workspace. NewWorkspaceReftype with name, phase, and labels. Proto re-exports for workspace request/response types.openshell-{name}toopenshell-{name}.{workspace}in vscode-remote-sandbox documentation.sandbox uploadcommand reimplemented upload logic inline with bugs (symlink handling, git-aware filtering). Replaced with delegation to the existingsandbox_upload()function.Breaking changes
{workspace}--{sandbox}.{domain}(previously{sandbox}.{domain}). The default workspace does not preserve the old format.sandbox-nameandsandbox-workspacelabels. Pre-existing managed CRs lacking these labels are silently skipped by the driver — the reconciler will treat them as disappeared and prune their database records. Operators must drain all running sandboxes before upgrading the gateway.openshell.ai/sandbox-workspacelabel for workspace tracking. This is intentional — pre-existing containers from before the upgrade will not be visible to the watcher and require a clean shutdown before upgrade.openshell-{name}.{workspace}(previouslyopenshell-{name}). Old entries are not automatically cleaned up.workspacekeyword argument with no default — this is intentional to make workspace selection explicit. The CLI defaults to"default"via--workspace, but the SDK requires callers to be explicit about which workspace they operate in.SetClusterInferenceConfig→SetInferenceRoute,GetClusterInferenceConfig→GetInferenceRoute,ClusterInferenceConfig→InferenceRouteConfig. This is a wire-breaking change — the inference route API is pre-GA with no deployed consumers of the old RPC names.workspace\nname. Old single-line files are ignored on load (user re-selects on first use after upgrade).workspacecolumn to"default"so existing records are queryable, but the protobuf payload is not re-encoded — decoded objects carryworkspace="". CAS updates on migrated records will be rejected until the resource is deleted and re-created. This is intentional: the protobuf payload is authoritative for workspace identity at creation time, and re-encoding every blob during migration adds complexity for a pre-GA system with no production deployments to migrate.Related Issue
#1977
Changes
Testing
Known remaining work (Phase 2)
Non-blocking follow-ups noted in review (no action needed for this PR)
--workspacedefaults to"default"client-side; consider removing the default and relying on server-sideresolve_workspacenormalization, with client-side operations (last-sandbox, completers) resolving workspace from server responses.unmapped("workspace", ...)as part of a broader workspace attribution pass across all OCSF event sitessandbox_sync_guard— attach racing delete can leave a dangling by-name attachment (pre-existing TOCTOU)N_workspaces * timeout. Needs bounded concurrent fetches or a background task. Low practical impact pre-GA.serialize_labelshelper — the label serialization block is repeated across ~12 call sites; a shared helper inpersistence/mod.rswould reduce duplication and prevent divergenceWorkspaceClientmembership management (AddWorkspaceMember, RemoveWorkspaceMember, ListWorkspaceMembers) deferred to Phase 2workspace membersubcommands define their own--workspacearg which shadows the global--workspaceflag — semantically correct (you're targeting a workspace to manage its members) but confusing when both appear on the same command line. Consider renaming the member-level arg to--nameor having member commands consume the global--workspaceinstead.put_ifwith arequires_parentsubquery) to close generically; per-call-site double-checks are fragile. The Terminating phase narrows the window significantly for workspaces. Tracked for a dedicated persistence-layer follow-up.Checklist
provider list-profiles --globalflag for platform-scoped profilesput_ifcall sites (sandbox, provider, inference route, SSH session, workspace member, provider profile import, default workspace)list/list_idsuses two-method pattern: workspace-scoped and_for_all_workspaces(K8s convention)WorkspaceClientwith CRUD (create, get, list, delete) and e2e testsWorkspaceScopedClientwith sandbox CRUD, wait, and execlist_sandboxes_all_workspacesSandboxRef.workspacefield andWorkspaceReftypeopenshell-{name}.{workspace}formatsandbox uploaddelegated to existing upload function (bug fix)snapshot_catalog) workspace-aware — prevents cross-workspace duplicate ID collisionslist_by_scopeincludesresource_versionin SELECT (SQLite + Postgres)put_policy_revision_atomicincludes workspace column (SQLite + Postgres) with regression testcollect_ancestor_identitiesshort-circuits when socket owner is entrypoint (fixes flaky 30s timeout tests)