feat(k8s): add namespace-per-workspace support (RFC 0011 Phase 3) - #2656
feat(k8s): add namespace-per-workspace support (RFC 0011 Phase 3)#2656derekwaynecarr wants to merge 14 commits into
Conversation
|
/ok to test 628a4b7 |
|
Label |
|
/ok to test 6a067bb |
Rereview verdict: still request changesThe update fixed:
Remaining blockers
Coverage/process
|
Rereview — changes still requestedReviewed the latest head (
Additional outstanding items:
Addressed since the previous review
Validation
|
|
/ok to test c655db1 |
|
/ok to test 11d9c21 |
|
/ok to test dfd3306 |
Rereview — changes still requestedReviewed the latest head (
Addressed since the previous review
Validation
|
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: Project-valid because @derekwaynecarr is a CODEOWNER; implementation review remains required.
Head SHA: dfd33068830456ddca41334ace628c6d6ca8bc4c
Base SHA: 4cb77a900ebd6b789d2b68daaba4830866833b1c
Merge base SHA: 4cb77a900ebd6b789d2b68daaba4830866833b1c
Patch ID: 60c066e46381c7e205f9f952acdecc99461c4c8b
Gator payload: 3
Review mode: initial
Previous reviewed SHA: none
Thanks @mrunalp. I independently checked the exact-head concerns you raised. The TLS RBAC mismatch, shared-mode upgrade regression, missing managed-namespace SSH isolation, image-pull Secret gap, and non-durable namespace cleanup all satisfy the blocking evidence contract. I also checked the multi-namespace Event path and the docs/skill-maintenance notes; they did not meet the blocking contract in this normalized review.
Blocking findings:
GATOR-dfd33068-01: default TLS-enabled multi-namespace installs lack the Secretpatchverb used during every sandbox create.GATOR-dfd33068-02: shared-mode upgrades cannot discover legacy Sandbox CRs without the new gateway-ID label.GATOR-dfd33068-03: managed namespaces omit the chart's default SSH NetworkPolicy isolation.GATOR-dfd33068-04: managed namespaces reference configured image-pull Secrets without provisioning them.GATOR-dfd33068-05: workspace deletion discards the durable retry handle before managed namespace cleanup succeeds.
GATOR-dfd33068-04 — managed image-pull Secrets are not provisioned
Invariant: A driver-created managed namespace must contain configured image-pull Secrets before creating pods that reference them.
Prerequisite: An operator enables managed mode and configures server.sandboxImagePullSecrets for a private sandbox or supervisor image.
Entry point → sink: CreateSandbox in managed mode → kubelet image pull for the generated pod.
Changed location: crates/openshell-driver-kubernetes/src/driver.rs:1210. This is summarized here because the reference is an unchanged context line outside GitHub's commentable diff range; the defect is the missing provisioning step in the newly added namespace lifecycle.
Base → head: Shared mode required the named Secret only in the configured sandbox namespace → managed mode creates pods in a new namespace while still injecting the same Secret name, but ensure_namespace creates only the Namespace and ServiceAccount and the new TLS path copies only TLS material.
Impact: Private images enter ImagePullBackOff unless an operator separately pre-populates every dynamically named namespace, defeating managed lifecycle behavior.
Reproducer: Create regcred in the release/sandbox namespace, configure server.sandboxImagePullSecrets[0].name=regcred, enable managed mode, and create a private-image sandbox. The pod references regcred in openshell-<gateway>-<workspace>, where that Secret is absent.
PR ownership: This PR moves managed pods into driver-created namespaces while preserving namespace-local Secret references without provisioning them.
Requested change: Copy configured image-pull Secrets before Sandbox creation, or fail with a clear precondition requiring an operator-managed propagation mechanism; cover the supported private-image path.
Carried findings: None.
Docs: Fern gateway configuration is updated; the blocking runtime gaps above remain.
Next state: gator:in-review
ddb67ae to
3051217
Compare
Implement three workspace namespace modes for the Kubernetes compute driver: shared (default, preserves current single-namespace behavior), managed (auto-creates/deletes namespaces per workspace), and operator (pre-provisioned namespaces with dynamic discovery via label selector or drop-in allowlist file). Key changes: - WorkspaceMode enum and namespace resolution in driver config - Managed namespace lifecycle with ServiceAccount and OpenShift SCC annotation propagation - Cluster-wide sandbox CR watchers for managed/operator modes - NamespaceValidator (Exact/Prefix/Allowlist) for SA token auth - Workspace-aware credential secret storage - Helm ClusterRole for multi-namespace RBAC - Gateway config, architecture, and reference docs Signed-off-by: Derek Carr <decarr@redhat.com>
Add end-to-end tests for managed and operator workspace modes introduced in RFC 0011 Phase 3. The managed mode tests verify namespace creation with correct labels, ServiceAccount provisioning, sandbox CR placement, and namespace survival with remaining sandboxes. The operator mode tests verify rejection of unlabeled and nonexistent namespaces. The positive operator path (sandbox in labeled namespace) is known to fail due to an RBAC gap and will be addressed separately. Also fixes Helm 4 compatibility: move SPDX license headers inside conditional guards in 8 chart templates to prevent empty comment-only documents, and fix a trailing whitespace trimmer in clusterrole.yaml that concatenated the license header with apiVersion. Adds cleanup sweep in with-kube-gateway.sh to remove managed and operator namespaces before Helm uninstall, and mise tasks for running each mode independently. Signed-off-by: Derek Carr <decarr@redhat.com>
Spawn a background kube::runtime::watcher in the K8s driver that watches namespaces matching the configured label selector and populates the OperatorNamespaceAllowlist at runtime. The driver owns the allowlist and exposes its Arc so the server can share the same set with the SA token authenticator. create_sandbox now gates pod creation on the allowlist in operator mode — workspaces whose namespace is not yet labeled are rejected at resource render time rather than silently proceeding. Workspace lifecycle itself is unaffected; only sandbox (resource) creation is gated. Signed-off-by: Derek Carr <decarr@redhat.com>
Close the fail-open gap in operator mode when only operator_namespace_file is configured: the allowlist is now created unconditionally in operator mode (fail-closed from startup). Implement the namespace file watcher using the notify crate, following the TLS hot-reload pattern (parent-directory watch, 1s debounce, ConfigMap symlink-swap safe). The file format is a JSON array of namespace name strings. Additional fixes from the 10-reviewer audit: - Change allowlist rejection from InvalidArgument to FailedPrecondition so callers know the request may succeed later once the namespace is provisioned. - NamespaceValidator::Allowlist now holds the OperatorNamespaceAllowlist newtype instead of a raw Arc<RwLock<BTreeSet>>, eliminating silent denial on RwLock poison. - Verify LABEL_MANAGED_BY and LABEL_GATEWAY_ID ownership before deleting a managed namespace. - Replace fixed 5s sleep in operator e2e test with a 30s poll loop. - Add Helm validation for workspaceMode values. - Fix Helm README type column and description for operator fields. - Add insert/remove methods to OperatorNamespaceAllowlist; label watcher now uses them instead of reaching through shared(). - Reject configs with both operator_namespace_label and operator_namespace_file set. Signed-off-by: Derek Carr <decarr@redhat.com>
Decouple namespace lifecycle from sandbox lifecycle by adding EnsureWorkspace/DeleteWorkspace RPCs to the ComputeDriver service. Namespace creation now happens before credential storage and namespace deletion happens on workspace delete, fixing credential storage in managed workspace mode. - Add EnsureWorkspace and DeleteWorkspace proto RPCs with implementations across all compute drivers (K8s managed delegates to ensure_namespace/delete_namespace_if_empty; others no-op) - Wire ensure_workspace into provider create/update/refresh paths so the namespace exists before the credential driver writes secrets - Wire delete_workspace into workspace deletion for cleanup - Remove delete_namespace_if_empty from sandbox deletion path - Scope ClusterRole secrets access to non-shared workspace modes - Add TODO for TLS cert hot-reload in sandbox gRPC client - Harden e2e tests with control-plane sandbox resolution assertions - Fix docker image save --platform flag for OCI index manifests Signed-off-by: Derek Carr <decarr@redhat.com>
- Use server-side apply for TLS secret sync (fixes second sandbox creation failure when TLS is enabled) - Scope gateway-ID label selector unconditionally across all workspace modes (fixes operator reads/watches/deletes seeing foreign sandboxes) - Validate operator allowlist in EnsureWorkspace and DeleteWorkspace RPCs (prevents credential writes to namespaces outside the allowlist) - Extend ClusterRole secrets patch+delete to all non-shared modes with credential driver enabled (fixes operator credential storage RBAC) - Validate namespace ownership on 409 conflict in ensure_namespace (prevents adopting unowned namespaces in managed mode) - Replace delete_namespace_if_empty with unconditional delete_namespace letting Kubernetes cascade cleanup (fixes stuck terminating CRs) - Strengthen NetworkPolicy TODO to cover both managed and operator modes - Extract selector and ownership logic into testable free functions - Add unit tests for gateway-ID selectors and namespace ownership - Add Helm ClusterRole RBAC tests for operator credential driver Signed-off-by: Derek Carr <decarr@redhat.com>
Wire the existing e2e:kubernetes:workspace-managed and e2e:kubernetes:workspace-operator mise tasks into the branch-e2e workflow so they run alongside the other core Kubernetes e2e suites. Both are gated by run_core_e2e and included in the Core E2E result gate. Signed-off-by: Derek Carr <decarr@redhat.com>
Add 7 new e2e tests covering workspace namespace lifecycle, TLS secret copying, ownership conflict detection, DNS-1123 validation, operator namespace preservation, and dynamic label watcher behavior. Fix async sandbox deletion race condition in existing tests by polling sandbox list instead of asserting immediately after delete. Signed-off-by: Derek Carr <decarr@redhat.com>
… labels Address two review findings: 1. RBAC: server-side apply (PATCH) is used for TLS secret sync in multi-namespace modes, but the ClusterRole only granted patch when the kubernetes-secrets credential driver was enabled. Grant patch unconditionally for non-shared modes since TLS sync always needs it; keep delete gated on the credential driver. 2. Upgrade safety: the new gateway-id label selector would orphan legacy Sandbox CRs that predate its introduction. Add a startup backfill in shared mode that patches any managed Sandbox CR missing the gateway-id label before the driver begins serving requests. Signed-off-by: Derek Carr <decarr@redhat.com>
Signed-off-by: Derek Carr <decarr@redhat.com>
Signed-off-by: Derek Carr <decarr@redhat.com>
Signed-off-by: Derek Carr <decarr@redhat.com>
Signed-off-by: Derek Carr <decarr@redhat.com>
Signed-off-by: Derek Carr <decarr@redhat.com>
3051217 to
0810025
Compare
|
/ok to test 0810025 |
Summary
Implement three workspace namespace modes for the Kubernetes compute driver: shared (default, preserves current single-namespace behavior), managed (auto-creates/deletes namespaces per workspace), and operator (pre-provisioned namespaces with dynamic discovery via label selector or drop-in allowlist file).
Related Issue
Closes #2486
Changes
Key changes:
Testing
mise run pre-commitpassesChecklist