Skip to content

feat(k8s): add namespace-per-workspace support (RFC 0011 Phase 3) - #2656

Merged
derekwaynecarr merged 16 commits into
NVIDIA:mainfrom
derekwaynecarr:feat/rfc-0011-phase3-namespace-per-workspace
Aug 14, 2026
Merged

feat(k8s): add namespace-per-workspace support (RFC 0011 Phase 3)#2656
derekwaynecarr merged 16 commits into
NVIDIA:mainfrom
derekwaynecarr:feat/rfc-0011-phase3-namespace-per-workspace

Conversation

@derekwaynecarr

@derekwaynecarrderekwaynecarr commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

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:

  • 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

Testing

  • [ x] mise run pre-commit passes
  • [ x] Unit tests added/updated
  • [ x] E2E tests added/updated (if applicable)

Checklist

  • [x ] Follows Conventional Commits
  • [ x] Commits are signed off (DCO)
  • [ x] Architecture docs updated (if applicable)

@copy-pr-bot

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@derekwaynecarr
derekwaynecarr marked this pull request as draft August 7, 2026 21:40
@derekwaynecarr
derekwaynecarr marked this pull request as ready for review August 8, 2026 19:15
@derekwaynecarr

Copy link
Copy Markdown
CollaboratorAuthor

/ok to test 628a4b7

@derekwaynecarrderekwaynecarr added the test:e2e Requires end-to-end coverage label Aug 8, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied, but pull-request/2656 is at 628a4b7 while the PR head is 6a067bb. A maintainer needs to comment /ok to test 6a067bbb29ef5fdde7d395fd248deef8b9d2ee69 to refresh the mirror. Once the mirror catches up, re-run Branch E2E Checks from the Actions tab.

@derekwaynecarr

Copy link
Copy Markdown
CollaboratorAuthor

/ok to test 6a067bb

@mrunalp

mrunalp commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Rereview verdict: still request changes

The update fixed:

  • Invalid Helm ClusterRole rendering.
  • Operator namespace label/file watchers.
  • Shared operator allowlist for SA authentication.
  • Ownership checks before managed namespace deletion.
  • Helm mode validation and several tests.

Remaining blockers

  1. P1 — Managed sandboxes are still invisible after creation.
    Managed queries require openshell.ai/gateway-id, but Sandbox CR labels still omit it. selector (

    fnsandbox_lookup_selector(&self,sandbox_id:&str) -> String{
    ), created labels
    ( ), label helper
    (
    fnsandbox_labels(sandbox:&Sandbox) -> BTreeMap<String,String>{
    )

  2. P1 — Managed/operator namespaces lack the client TLS Secret.
    Pods mount openshell-client-tls, but it exists only in the Helm release namespace. Default TLS-enabled sandboxes cannot start. namespace provisioning
    (

    self.ensure_service_account(&ns_name).await?;
    ), Secret volume
    (
    if !params.client_tls_secret_name.is_empty(){
    )

  3. P1/security — Dynamic namespaces lose SSH isolation.
    The default NetworkPolicy remains only in the static sandbox namespace, leaving port 2222 reachable laterally in workspace namespaces. networkpolicy.yaml:14
    (

    namespace: {{ include "openshell.sandboxNamespace" . }}
    )

  4. P1 — Kubernetes credential storage remains unusable.
    Credentials target workspace namespaces before those namespaces exist, are destroyed when the last sandbox deletes the namespace, and Helm grants Secret access only in the static credential namespace. credential namespace selection
    (

    namespace:self.settings.target_namespace(&request.workspace),
    )

  5. P1/security — Operator reads and watches remain cluster-wide.
    Creation is now allowlisted, but get/list/delete/watch do not filter results by the operator allowlist. One gateway can observe or operate on another gateway’s OpenShell CRs. lookup and selectors
    (

    asyncfnsupported_sandbox_api_for_lookup(
    )

  6. P1/security — Managed namespace adoption still trusts names alone.
    A create conflict is accepted without validating ownership, and SA authentication accepts any namespace with the managed prefix. 409 handling
    (

    match tokio::time::timeout(KUBE_API_TIMEOUT, ns_api.create(&PostParams::default(),&ns))
    ), prefix authentication
    (
    openshell_driver_kubernetes::WorkspaceMode::Managed => {
    )

Coverage/process

@mrunalp

Copy link
Copy Markdown
Collaborator

Rereview — changes still requested

Reviewed the latest head (b9ab04ad). Several earlier issues are fixed, but these blockers remain:

  1. P1 – Second sandbox creation fails with TLS enabled

    ensure_tls_secret calls replace without the existing resourceVersion. Kubernetes rejects that update once the Secret already exists.

    let target_api:Api<Secret> = Api::namespaced(self.client.clone(), namespace);
    let copy = Secret{
    metadata:ObjectMeta{
    name:Some(self.config.client_tls_secret_name.clone()),
    namespace:Some(namespace.to_string()),
    labels:Some(BTreeMap::from([(
    LABEL_MANAGED_BY.to_string(),
    LABEL_MANAGED_BY_VALUE.to_string(),
    )])),
    ..Default::default()
    },
    data: source.data,
    type_: source.type_,
    ..Default::default()
    };
    match tokio::time::timeout(
    KUBE_API_TIMEOUT,
    target_api.create(&PostParams::default(),&copy),
    )
    .await
    {
    Ok(Ok(_)) => {
    info!(
    namespace = %namespace,
    secret = %self.config.client_tls_secret_name,
    "created TLS secret copy"
    );
    }
    Ok(Err(KubeError::Api(api)))if api.code == 409 => {
    match tokio::time::timeout(
    KUBE_API_TIMEOUT,
    target_api.replace(
    &self.config.client_tls_secret_name,
    &PostParams::default(),
    &copy,
    ),
    )
    .await
    {
    Ok(Ok(_)) => {
    debug!(
    namespace = %namespace,
    secret = %self.config.client_tls_secret_name,
    "updated TLS secret copy"
    );
    }
    Ok(Err(e)) => returnErr(KubernetesDriverError::from_kube(e)),
    Err(_) => {
    returnErr(KubernetesDriverError::Message(format!(
    "timeout updating TLS secret in {namespace}"
    )));
    }
    }

  2. P1/security – Operator allowlist does not scope reads, watches, or deletes

    Operator mode uses the cluster-wide API, but gateway selectors are only added in managed mode. The gateway can therefore list or delete Sandboxes outside its allowlist.

    asyncfnsupported_sandbox_api_for_lookup(
    &self,
    client:Client,
    ) -> Result<AgentSandboxApi,String>{
    let sandbox_api_version = self.supported_sandbox_api_version(client.clone()).await?;
    ifself.config.is_multi_namespace(){
    Ok(Self::cluster_wide_sandbox_api(client, sandbox_api_version))
    }else{
    Ok(Self::agent_sandbox_api(
    client,
    sandbox_api_version,
    &self.config.namespace,
    ))
    }
    }
    fnsandbox_lookup_selector(&self,sandbox_id:&str) -> String{
    letmut selector =
    format!("{LABEL_MANAGED_BY}={LABEL_MANAGED_BY_VALUE},{LABEL_SANDBOX_ID}={sandbox_id}");
    ifself.config.workspace_mode == WorkspaceMode::Managed{
    use std::fmt::Write;
    write!(selector,",{LABEL_GATEWAY_ID}={}",self.config.gateway_id).unwrap();
    }
    selector
    }
    fnopenshell_sandbox_selector(&self) -> String{
    letmut selector = openshell_sandbox_label_selector();
    ifself.config.workspace_mode == WorkspaceMode::Managed{
    use std::fmt::Write;
    write!(selector,",{LABEL_GATEWAY_ID}={}",self.config.gateway_id).unwrap();
    }
    selector

  3. P1/security – Credential writes bypass the operator allowlist

    EnsureWorkspace validates only managed mode. In operator mode it succeeds without checking the allowlist, after which the credential driver uses the workspace name as the Secret namespace.

    asyncfnensure_workspace(
    &self,
    request:Request<EnsureWorkspaceRequest>,
    ) -> Result<Response<EnsureWorkspaceResponse>,Status>{
    let workspace = request.into_inner().workspace;
    if workspace.is_empty(){
    returnErr(Status::invalid_argument("workspace is required"));
    }
    ifself.driver.workspace_mode() == WorkspaceMode::Managed{
    self.driver
    .ensure_namespace(&workspace)
    .await
    .map_err(|e| Status::internal(e.to_string()))?;
    }
    Ok(Response::new(EnsureWorkspaceResponse{}))

  4. P1 – Operator credential storage lacks required RBAC

    Operator rendering grants Secret get/create/update, but credential updates use patch and deletion uses delete. Those verbs are currently added only for managed mode.

    {{- if ne $workspaceMode "shared" }}
    # TLS secret sync: read the source Secret in the release namespace and
    # create/update copies in workspace namespaces so sandbox pods can mount
    # client TLS material for mTLS to the gateway.
    {{- if and (eq $workspaceMode "managed") .Values.server.credentialDrivers.kubernetesSecrets.enabled }}
    # Managed mode with kubernetes-secrets credential driver: credentials are
    # stored as Secrets in workspace namespaces, requiring patch+delete in
    # addition to the TLS sync verbs.
    {{- end }}
    - apiGroups:
    - ""
    resources:
    - secrets
    verbs:
    - get
    - create
    - update
    {{- if and (eq $workspaceMode "managed") .Values.server.credentialDrivers.kubernetesSecrets.enabled }}
    - patch
    - delete
    {{- end }}

  5. P1/security – Dynamic namespaces lack the SSH NetworkPolicy

    The policy is still created only in the static sandbox namespace. Without a policy in each dynamic namespace, workloads elsewhere in the cluster can reach sandbox port 2222.

    {{- if .Values.networkPolicy.enabled }}
    # NetworkPolicy restricting SSH ingress on sandbox pods to the gateway pod.
    # Sandbox pods are dynamically created by the server and labelled with
    # openshell.ai/managed-by=openshell. This policy ensures only the gateway
    # (openshell server) pod can reach the sandbox SSH port (2222), blocking
    # lateral movement from other in-cluster workloads.
    apiVersion: networking.k8s.io/v1
    kind: NetworkPolicy
    metadata:
    name: {{ include "openshell.fullname" . }}-sandbox-ssh
    namespace: {{ include "openshell.sandboxNamespace" . }}
    labels:
    {{- include "openshell.labels" . | nindent 4 }}
    spec:
    podSelector:
    matchLabels:
    openshell.ai/managed-by: openshell
    policyTypes:
    - Ingress
    ingress:
    - from:
    - namespaceSelector:
    matchLabels:
    kubernetes.io/metadata.name: {{ .Release.Namespace }}
    podSelector:
    matchLabels:
    app.kubernetes.io/name: {{ include "openshell.name" . }}
    app.kubernetes.io/instance: {{ .Release.Name }}
    ports:
    - protocol: TCP
    port: 2222

  6. P1 – Existing managed namespaces are adopted without validation

    A namespace-create 409 proceeds directly to ServiceAccount, Secret, and Sandbox creation without verifying ownership. Deletion validation does not make initial adoption safe.

    match tokio::time::timeout(KUBE_API_TIMEOUT, ns_api.create(&PostParams::default(),&ns))
    .await
    {
    Ok(Ok(_)) => {
    info!(namespace = %ns_name, workspace = %workspace,"created managed namespace");
    }
    Ok(Err(KubeError::Api(api)))if api.code == 409 => {
    debug!(namespace = %ns_name,"managed namespace already exists");
    }
    Ok(Err(e)) => returnErr(KubernetesDriverError::from_kube(e)),
    Err(_) => {
    returnErr(KubernetesDriverError::Message(format!(
    "timeout creating namespace {ns_name}"
    )));
    }
    }
    self.ensure_service_account(&ns_name).await?;

  7. P2 – Namespace cleanup remains one-shot

    A terminating Sandbox CR causes cleanup to skip permanently. Cleanup errors are logged only after the workspace record is deleted, leaving no retry path.

    /// Delete the managed namespace if it contains no sandboxes (managed mode
    /// only). Called via the `DeleteWorkspace` RPC after workspace deletion.
    pubasyncfndelete_namespace_if_empty(
    &self,
    workspace:&str,
    ) -> Result<(),KubernetesDriverError>{
    let ns_name = managed_namespace(&self.config.gateway_id, workspace);
    let sandbox_api_version = self
    .supported_sandbox_api_version(self.client.clone())
    .await
    .map_err(KubernetesDriverError::Message)?;
    let agent_api = Self::agent_sandbox_api(self.client.clone(), sandbox_api_version,&ns_name);
    let lp = ListParams::default()
    .labels(&openshell_sandbox_label_selector())
    .limit(1);
    let list = tokio::time::timeout(KUBE_API_TIMEOUT, agent_api.api.list(&lp))
    .await
    .map_err(|_| {
    KubernetesDriverError::Message(format!("timeout listing sandboxes in {ns_name}"))
    })?
    .map_err(KubernetesDriverError::from_kube)?;
    if !list.items.is_empty(){
    debug!(namespace = %ns_name,"namespace still has sandboxes, skipping delete");
    returnOk(());
    }

Additional outstanding items:

  • Configured image-pull Secrets are not copied into managed namespaces.
  • Multi-namespace watches still omit Kubernetes Events.
  • The new workspace-mode E2E tasks are not wired into GitHub Actions.
  • debug-openshell-cluster remains stale for the new namespace modes.

Addressed since the previous review

  • Fixed ClusterRole apiVersion rendering.
  • Wired the operator allowlist into authentication and sandbox creation.
  • Added gateway-ID labels to managed Sandbox CRs.
  • Improved managed credential namespace lifecycle.
  • Added partial ownership validation before namespace deletion.
  • Fixed the Helm documentation metadata.

Validation

  • Kubernetes driver: 168 tests passed.
  • Kubernetes credential driver: 21 tests passed.
  • Server: 1,288 tests passed.
  • Helm lint and managed/operator rendering passed.
  • Required PR checks were still pending at review time.

@derekwaynecarr

Copy link
Copy Markdown
CollaboratorAuthor

/ok to test c655db1

@derekwaynecarr

Copy link
Copy Markdown
CollaboratorAuthor

/ok to test 11d9c21

@derekwaynecarr

Copy link
Copy Markdown
CollaboratorAuthor

/ok to test dfd3306

@mrunalp

Copy link
Copy Markdown
Collaborator

Rereview — changes still requested

Reviewed the latest head (dfd33068). Most earlier issues are fixed, but these blockers remain:

  1. P1 – Default TLS deployments lack Secret patch RBAC

    TLS synchronization now uses server-side apply (PATCH), but the ClusterRole grants patch only when the Kubernetes credential driver is enabled. Default managed/operator deployments will receive a 403.

    The workspace E2Es do not catch this because their Skaffold values disable TLS, causing the TLS test to skip.

    match tokio::time::timeout(
    KUBE_API_TIMEOUT,
    target_api.patch(
    &self.config.client_tls_secret_name,
    &PatchParams::apply("openshell"),
    &Patch::Apply(&copy),
    ),

    {{- if ne $workspaceMode "shared" }}
    # TLS secret sync: read the source Secret in the release namespace and
    # create/update copies in workspace namespaces so sandbox pods can mount
    # client TLS material for mTLS to the gateway.
    {{- if .Values.server.credentialDrivers.kubernetesSecrets.enabled }}
    # kubernetes-secrets credential driver: credentials are stored as Secrets
    # in workspace namespaces, requiring patch+delete in addition to TLS sync.
    {{- end }}
    - apiGroups:
    - ""
    resources:
    - secrets
    verbs:
    - get
    - create
    - update
    {{- if .Values.server.credentialDrivers.kubernetesSecrets.enabled }}
    - patch
    - delete
    {{- end }}

  2. P1 – Shared-mode upgrades lose existing Sandboxes

    Lookup, list, delete, and watch selectors now always require openshell.ai/gateway-id. Sandbox CRs created before this PR do not have that label, so they become invisible after an upgrade.

    Apply the gateway selector only in multi-namespace modes or provide an upgrade migration/compatibility path.

    fnsandbox_lookup_selector(&self,sandbox_id:&str) -> String{
    sandbox_lookup_selector_for(sandbox_id,&self.config.gateway_id)
    }
    fnopenshell_sandbox_selector(&self) -> String{
    openshell_sandbox_selector_for(&self.config.gateway_id)
    }

  3. P1/security – Dynamic namespaces still lack the SSH NetworkPolicy

    The NetworkPolicy remains limited to the static sandbox namespace. Managed namespaces therefore allow other cluster workloads to reach sandbox port 2222. Namespace separation alone does not prevent cross-namespace traffic.

    {{- if .Values.networkPolicy.enabled }}
    # NetworkPolicy restricting SSH ingress on sandbox pods to the gateway pod.
    # Sandbox pods are dynamically created by the server and labelled with
    # openshell.ai/managed-by=openshell. This policy ensures only the gateway
    # (openshell server) pod can reach the sandbox SSH port (2222), blocking
    # lateral movement from other in-cluster workloads.
    apiVersion: networking.k8s.io/v1
    kind: NetworkPolicy
    metadata:
    name: {{ include "openshell.fullname" . }}-sandbox-ssh
    namespace: {{ include "openshell.sandboxNamespace" . }}
    labels:
    {{- include "openshell.labels" . | nindent 4 }}
    spec:
    podSelector:
    matchLabels:
    openshell.ai/managed-by: openshell
    policyTypes:
    - Ingress
    ingress:
    - from:
    - namespaceSelector:
    matchLabels:
    kubernetes.io/metadata.name: {{ .Release.Namespace }}
    podSelector:
    matchLabels:
    app.kubernetes.io/name: {{ include "openshell.name" . }}
    app.kubernetes.io/instance: {{ .Release.Name }}
    ports:
    - protocol: TCP
    port: 2222

  4. P1 – Configured image-pull Secrets are not copied

    Sandbox pods in managed namespaces reference image-pull Secret names configured in the release namespace. Because Secrets are namespace-scoped, private sandbox images cannot be pulled unless those Secrets are separately provisioned in every workspace namespace.

  5. P2 – Failed namespace deletion has no retry

    The workspace database record is deleted before platform cleanup. Kubernetes deletion errors are only logged, leaving no durable retry path and potentially leaking the managed namespace permanently.

    Namespace deletion also lacks a UID precondition.

  6. P2 – Multi-namespace watches still omit Kubernetes Events

    Managed/operator users do not receive the scheduling and image-pull diagnostic events available in shared mode.

  7. Maintenance and process gaps

    • Architecture documentation still says managed namespaces are deleted after the last sandbox rather than on workspace deletion.
    • debug-openshell-cluster still assumes a single static sandbox namespace.
    • Related issue feat(kubernetes): map workspaces to sandbox namespaces #2486 remains labeled state:triage-needed.

Addressed since the previous review

  • TLS updates no longer omit resourceVersion.
  • Operator credential provisioning enforces the namespace allowlist.
  • Operator credential RBAC includes patch/delete when enabled.
  • Cluster-wide Sandbox queries are gateway-scoped.
  • Existing managed namespaces receive ownership validation.
  • Finalizer timing no longer blocks normal workspace deletion.
  • Managed/operator E2Es are wired into the required CI gate.

Validation

  • Kubernetes driver: 174 tests passed.
  • Kubernetes credential driver: 21 tests passed.
  • Helm: 77 tests passed.
  • Managed workspace E2E passed.
  • Operator workspace E2E passed.
  • Overall required E2E was still running at review time.

@drewdrew left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Secret patch verb 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

Comment threaddeploy/helm/openshell/templates/clusterrole.yaml
Comment threadcrates/openshell-driver-kubernetes/src/driver.rs
Comment threadcrates/openshell-driver-kubernetes/src/driver.rs Outdated
Comment threadcrates/openshell-server/src/grpc/workspace.rs Outdated
@drewdrew added gator:in-review Gator is reviewing or awaiting PR review feedback gator:blocked Gator is blocked by process or repository gates and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Aug 11, 2026
@derekwaynecarr
derekwaynecarrforce-pushed the feat/rfc-0011-phase3-namespace-per-workspace branch 2 times, most recently from 3051217 to 0810025CompareAugust 14, 2026 02:13
@derekwaynecarr

Copy link
Copy Markdown
CollaboratorAuthor

/ok to test 0810025

@rhussrhuss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc-review Summary

What Went Well

  • Fail-closed security posture (config.rs:572-584): Operator mode rejects unknown workspaces by default. NamespaceValidator uses allowlist semantics, ensuring new namespaces are denied until explicitly added.
  • Comprehensive validation at config boundaries (config.rs:606-674): validate_workspace_mode() checks gateway_id DNS compliance, namespace length limits, managed SSH ingress completeness, and operator discovery source exclusivity.
  • Idempotent namespace lifecycle (driver.rs:654-739): ensure_namespace handles 409 conflicts gracefully by verifying gateway ownership. Prevents namespace hijacking while supporting retry-safe operations.
  • Thorough E2E coverage (workspace_namespace_managed.rs): Tests cover namespace creation, label verification, SA provisioning, Secret copying, NetworkPolicy verification, cross-namespace isolation, lifecycle, and DNS-1123 validation.
  • Least-privilege RBAC (clusterrole.yaml:76-114): Secret access restricted by resourceNames to only configured TLS and image-pull secrets.

Inline Findings

IDSeverityFileDescriptionSource
1Importantkubernetes-secrets/lib.rs:51Missing gateway_id validation in secrets driver configcoderabbit
9Importantconfig.rs:748OperatorNamespaceAllowlist panics on poisoned lockproduction
14Importantgrpc.rs:228delete_workspace fails for absent allowlist entriescoderabbit
20Importantprovider_refresh.rs:1121Provider refresh worker has no shutdown integrationproduction
2Minorconfig.rs:504Unused parameter in resolve_sandbox_gidarchitecture
4Minorconfig.rs:648Missing test: operator both label AND filetest-quality
5Minorconfig.rs:572Missing test: operator None allowlisttest-quality
8Minorconfig.rs:692DNS validation logic duplicated within cratearchitecture
10Minordriver.rs:658Duplicate API handle and silent error swallowingcoderabbit
13Minorgrpc.rs:243Fragile string-based error matchingarchitecture
18MinorREADME.md:14README misrepresents operator namespace configcoderabbit
25Minormanaged.rs:64E2E timestamp modulo allows collisionstest-quality
22Notableclusterrole.yaml:76Secret RBAC could be narrower for operator modecoderabbit, security

Summary-Only Findings (outside PR diff)

IDSeverityLocationDescriptionSource
15Importantmain.rs:231No graceful shutdown in K8s driver binary. .serve() without shutdown handler; SIGTERM drops in-flight gRPC calls, leaving sandbox resources inconsistent.production
12Importantdriver.rs:4163Background label/file watchers spawn infinite-loop tokio tasks with no CancellationToken. Tasks run until runtime is forcefully dropped.production
3Minorconfig.rs:452effective_sa_token_ttl_secs has 3 code paths with zero test coveragetest-quality
6Minorconfig.rs:525from_open_shift_supplemental_groups lacks negative tests (unlike from_open_shift_uid_range)test-quality
7Minorconfig.rs:219AppArmorProfile serde deserialization of "Localhost/" untestedtest-quality
11Minordriver.rs:4209Namespace label watcher uses fixed 2s retry without backoff or jitterproduction
16Minormain.rs:2103 config fields use raw std::env::var instead of clap #[arg] attributesarchitecture
17Minordriver.rs:109, k8s_sa.rs:44Sandbox API group/kind/version constants duplicated across crate boundaryarchitecture
19Minorprovider_refresh.rs:113list_all_refresh_states loads all records into unbounded Vecproduction
21Minorprovider_refresh.rs:973New reqwest::Client created per token request instead of reusingproduction

Notable Observations

  • Handler-level auth inconsistency (workspace.rs:142,287): handle_create_workspace and handle_delete_workspace skip handler-level extract_principal(), relying solely on proto-level descriptor_authz annotations. Mitigated but inconsistent with other workspace handlers. (security)
  • Rate limiting disabled by default (values.yaml): gRPC rate limiting defaults to disabled. Deployments exposed to untrusted clients should enable it. (security)

Review Details

  • Findings posted: 25 (13 inline, 12 in summary body)
  • Findings reviewed and not posted: 0
  • Gate outcome: FAIL (6 Important findings)
  • Participating agents: correctness, architecture, security, production, test-quality, goal-alignment, coderabbit

Comment threadcrates/openshell-driver-kubernetes-secrets/src/lib.rs
Comment threadcrates/openshell-driver-kubernetes/src/config.rs
Comment threadcrates/openshell-driver-kubernetes/src/config.rs
Comment threadcrates/openshell-driver-kubernetes/src/config.rs
Comment threadcrates/openshell-driver-kubernetes/src/config.rs
Comment threadcrates/openshell-driver-kubernetes/src/grpc.rs Outdated
Comment threadcrates/openshell-driver-kubernetes/src/grpc.rs Outdated
Comment threadcrates/openshell-server/src/provider_refresh.rs
Comment threaddeploy/helm/openshell/templates/clusterrole.yaml
Comment threade2e/rust/tests/workspace_namespace_managed.rs
@derekwaynecarr

Copy link
Copy Markdown
CollaboratorAuthor

/ok to test bac95bf

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>
Signed-off-by: Derek Carr <decarr@redhat.com>
Signed-off-by: Derek Carr <decarr@redhat.com>
@derekwaynecarr
derekwaynecarrforce-pushed the feat/rfc-0011-phase3-namespace-per-workspace branch from bac95bf to 6699658CompareAugust 14, 2026 20:17
@derekwaynecarr

Copy link
Copy Markdown
CollaboratorAuthor

/ok to test 6699658

@sjenningsjenning added gator:blocked Gator is blocked by process or repository gates and removed gator:blocked Gator is blocked by process or repository gates labels Aug 14, 2026
@derekwaynecarr
derekwaynecarr added this pull request to the merge queueAug 14, 2026
Merged via the queue into NVIDIA:main with commit 59479f4Aug 14, 2026
101 of 103 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:blockedGator is blocked by process or repository gatestest:e2eRequires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(kubernetes): map workspaces to sandbox namespaces

5 participants

@derekwaynecarr@mrunalp@drew@rhuss@sjenning