fix(k8s): harden operator mode and address review findings

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>

Author: derekwaynecarr

Cargo.lock

@@ -34,6 +34,7 @@ tracing = { workspace = true }
 tracing-subscriber = { workspace = true }
 thiserror = { workspace = true }
 miette = { workspace = true }
+notify = "8"
 
 [dev-dependencies]
 temp-env = "0.3"

crates/openshell-driver-kubernetes/Cargo.toml

@@ -293,8 +293,8 @@ pub struct KubernetesComputeConfig {
     /// this label and builds the allowlist dynamically.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub operator_namespace_label: Option<String>,
-    /// Path to a drop-in JSON file mapping workspace names to namespace names.
-    /// Hot-reloaded on change. Delivered via `ConfigMap` volume mount.
+    /// Path to a JSON file containing an array of namespace names allowed in
+    /// operator mode. Hot-reloaded on change. Delivered via `ConfigMap` volume mount.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub operator_namespace_file: Option<String>,
     /// Kubernetes `ServiceAccount` assigned to sandbox pods and accepted by
@@ -623,10 +623,16 @@ impl KubernetesComputeConfig {
             WorkspaceMode::Operator => {
                 if self.operator_namespace_label.is_none() && self.operator_namespace_file.is_none()
                 {
-                    return Err("operator workspace mode requires at least one of \
+                    return Err("operator workspace mode requires exactly one of \
                          operator_namespace_label or operator_namespace_file"
                         .into());
                 }
+                if self.operator_namespace_label.is_some() && self.operator_namespace_file.is_some()
+                {
+                    return Err("operator workspace mode requires exactly one of \
+                         operator_namespace_label or operator_namespace_file, not both"
+                        .into());
+                }
                 if let Some(ref label) = self.operator_namespace_label
                     && label.is_empty()
                 {
@@ -738,6 +744,22 @@ impl OperatorNamespaceAllowlist {
             .contains(namespace)
     }
 
+    /// Insert a namespace into the allowlist. Returns `true` if it was new.
+    pub fn insert(&self, name: String) -> bool {
+        self.inner
+            .write()
+            .expect("allowlist lock poisoned")
+            .insert(name)
+    }
+
+    /// Remove a namespace from the allowlist. Returns `true` if it was present.
+    pub fn remove(&self, name: &str) -> bool {
+        self.inner
+            .write()
+            .expect("allowlist lock poisoned")
+            .remove(name)
+    }
+
     /// Return a clone of the inner `Arc` for sharing with background tasks.
     #[must_use]
     pub fn shared(&self) -> Arc<RwLock<BTreeSet<String>>> {

crates/openshell-driver-kubernetes/src/config.rs

@@ -41,7 +41,7 @@ use openshell_core::proto::compute::v1::{
 use openshell_core::proto_struct::{struct_to_json_object, value_to_json};
 use serde::Deserialize;
 use std::collections::{BTreeMap, HashSet};
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::pin::Pin;
 use std::sync::Arc;
 use std::time::Duration;
@@ -487,15 +487,21 @@ impl KubernetesComputeDriver {
             Client::try_from(watch_kube_config).map_err(KubernetesDriverError::from_kube)?;
 
         let operator_allowlist = if matches!(config.workspace_mode, WorkspaceMode::Operator) {
-            config.operator_namespace_label.as_ref().map(|label| {
-                let allowlist = OperatorNamespaceAllowlist::new();
+            let allowlist = OperatorNamespaceAllowlist::new();
+
+            if let Some(ref label) = config.operator_namespace_label {
                 spawn_namespace_label_watcher(
                     watch_client.clone(),
                     label.clone(),
                     allowlist.clone(),
                 );
-                allowlist
-            })
+            }
+
+            if let Some(ref path) = config.operator_namespace_file {
+                spawn_namespace_file_watcher(path.into(), allowlist.clone());
+            }
+
+            Some(allowlist)
         } else {
             None
         };
@@ -673,6 +679,36 @@ impl KubernetesComputeDriver {
         }
 
         let ns_api: Api<Namespace> = Api::all(self.client.clone());
+
+        let ns = match tokio::time::timeout(KUBE_API_TIMEOUT, ns_api.get(&ns_name)).await {
+            Ok(Ok(ns)) => ns,
+            Ok(Err(KubeError::Api(api))) if api.code == 404 => {
+                debug!(namespace = %ns_name, "managed namespace already deleted");
+                return Ok(());
+            }
+            Ok(Err(e)) => return Err(KubernetesDriverError::from_kube(e)),
+            Err(_) => {
+                return Err(KubernetesDriverError::Message(format!(
+                    "timeout getting namespace {ns_name}"
+                )));
+            }
+        };
+
+        let labels = ns.metadata.labels.as_ref();
+        let is_owned = labels
+            .and_then(|l| l.get(LABEL_MANAGED_BY))
+            .is_some_and(|v| v == LABEL_MANAGED_BY_VALUE)
+            && labels
+                .and_then(|l| l.get(LABEL_GATEWAY_ID))
+                .is_some_and(|v| v == &self.config.gateway_id);
+        if !is_owned {
+            debug!(
+                namespace = %ns_name,
+                "namespace not owned by this gateway, skipping delete"
+            );
+            return Ok(());
+        }
+
         match tokio::time::timeout(
             KUBE_API_TIMEOUT,
             ns_api.delete(&ns_name, &DeleteParams::default()),
@@ -1070,7 +1106,7 @@ impl KubernetesComputeDriver {
                 if let Some(ref allowlist) = self.operator_allowlist
                     && !allowlist.contains(workspace)
                 {
-                    return Err(KubernetesDriverError::InvalidArgument(format!(
+                    return Err(KubernetesDriverError::Precondition(format!(
                         "workspace '{workspace}' is not in the operator namespace allowlist"
                     )));
                 }
@@ -3523,33 +3559,20 @@ fn spawn_namespace_label_watcher(
             loop {
                 match stream.try_next().await {
                     Ok(Some(Event::Applied(ns))) => {
-                        if let Some(name) = ns.metadata.name.as_deref() {
-                            let inner = allowlist.shared();
-                            let mut guard = inner.write().expect("allowlist lock poisoned");
-                            if guard.insert(name.to_string()) {
-                                let count = guard.len();
-                                drop(guard);
-                                info!(
-                                    namespace = name,
-                                    total = count,
-                                    "operator namespace added to allowlist"
-                                );
-                            }
+                        if let Some(name) = ns.metadata.name.as_deref()
+                            && allowlist.insert(name.to_string())
+                        {
+                            info!(namespace = name, "operator namespace added to allowlist");
                         }
                     }
                     Ok(Some(Event::Deleted(ns))) => {
-                        if let Some(name) = ns.metadata.name.as_deref() {
-                            let inner = allowlist.shared();
-                            let mut guard = inner.write().expect("allowlist lock poisoned");
-                            if guard.remove(name) {
-                                let count = guard.len();
-                                drop(guard);
-                                info!(
-                                    namespace = name,
-                                    total = count,
-                                    "operator namespace removed from allowlist"
-                                );
-                            }
+                        if let Some(name) = ns.metadata.name.as_deref()
+                            && allowlist.remove(name)
+                        {
+                            info!(
+                                namespace = name,
+                                "operator namespace removed from allowlist"
+                            );
                         }
                     }
                     Ok(Some(Event::Restarted(namespaces))) => {
@@ -3581,10 +3604,126 @@ fn spawn_namespace_label_watcher(
 
     info!(
         label_selector = %label_selector,
-        "operator namespace label watcher started"
+        "operator namespace label watcher spawned"
     );
 }
 
+fn load_namespace_file(path: &Path) -> Result<std::collections::BTreeSet<String>, String> {
+    let contents = std::fs::read_to_string(path)
+        .map_err(|e| format!("failed to read {}: {e}", path.display()))?;
+    let names: Vec<String> = serde_json::from_str(&contents)
+        .map_err(|e| format!("failed to parse {}: {e}", path.display()))?;
+    Ok(names.into_iter().collect())
+}
+
+fn spawn_namespace_file_watcher(path: PathBuf, allowlist: OperatorNamespaceAllowlist) {
+    match load_namespace_file(&path) {
+        Ok(names) => {
+            let count = names.len();
+            allowlist.replace(names);
+            info!(
+                path = %path.display(),
+                total = count,
+                "operator namespace allowlist loaded from file"
+            );
+        }
+        Err(err) => {
+            warn!(
+                error = %err,
+                "failed to load initial operator namespace file, allowlist empty"
+            );
+        }
+    }
+
+    let watch_dir = path
+        .parent()
+        .unwrap_or_else(|| Path::new("."))
+        .to_path_buf();
+    let debounce = Duration::from_secs(1);
+
+    tokio::spawn(async move {
+        let (tx, mut rx) = mpsc::unbounded_channel();
+
+        let mut watcher =
+            match notify::recommended_watcher(move |res: Result<notify::Event, notify::Error>| {
+                if let Ok(event) = res
+                    && matches!(
+                        event.kind,
+                        notify::EventKind::Modify(_) | notify::EventKind::Create(_)
+                    )
+                {
+                    let _ = tx.send(());
+                }
+            }) {
+                Ok(w) => w,
+                Err(e) => {
+                    warn!(
+                        error = %e,
+                        "failed to start operator namespace file watcher, hot-reload disabled"
+                    );
+                    return;
+                }
+            };
+
+        if let Err(e) = notify::Watcher::watch(
+            &mut watcher,
+            &watch_dir,
+            notify::RecursiveMode::NonRecursive,
+        ) {
+            warn!(
+                error = %e,
+                dir = %watch_dir.display(),
+                "failed to watch operator namespace file directory, hot-reload disabled"
+            );
+            return;
+        }
+
+        info!(
+            path = %path.display(),
+            "operator namespace file watcher started"
+        );
+
+        loop {
+            let got_event = rx.recv().await.is_some();
+            if !got_event {
+                warn!("operator namespace file watcher disconnected");
+                break;
+            }
+
+            loop {
+                tokio::select! {
+                    () = tokio::time::sleep(debounce) => {
+                        match load_namespace_file(&path) {
+                            Ok(names) => {
+                                let count = names.len();
+                                allowlist.replace(names);
+                                info!(
+                                    total = count,
+                                    "operator namespace allowlist reloaded from file"
+                                );
+                            }
+                            Err(err) => {
+                                warn!(
+                                    error = %err,
+                                    "failed to reload operator namespace file, keeping existing allowlist"
+                                );
+                            }
+                        }
+                        break;
+                    }
+                    r = rx.recv() => {
+                        if r.is_some() {
+                            continue;
+                        }
+                        warn!("operator namespace file watcher disconnected");
+                        return;
+                    }
+                }
+            }
+        }
+    });
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/openshell-driver-kubernetes/src/driver.rs

@@ -26,8 +26,8 @@ use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta;
 use kube::Error as KubeError;
 use kube::api::{Api, ApiResource, PostParams};
 use kube::core::{DynamicObject, gvk::GroupVersionKind};
-use std::collections::BTreeSet;
-use std::sync::{Arc, RwLock};
+use openshell_driver_kubernetes::OperatorNamespaceAllowlist;
+use std::sync::Arc;
 use tonic::Status;
 use tracing::{debug, info, warn};
 
@@ -146,15 +146,15 @@ pub enum NamespaceValidator {
     /// (`openshell-{gateway_id}-`).
     Prefix(String),
     /// Operator mode: accept namespaces in the dynamic allowlist.
-    Allowlist(Arc<RwLock<BTreeSet<String>>>),
+    Allowlist(OperatorNamespaceAllowlist),
 }
 
 impl NamespaceValidator {
     pub fn accepts(&self, namespace: &str) -> bool {
         match self {
             Self::Exact(expected) => namespace == expected,
             Self::Prefix(prefix) => namespace.starts_with(prefix.as_str()),
-            Self::Allowlist(set) => set.read().is_ok_and(|s| s.contains(namespace)),
+            Self::Allowlist(al) => al.contains(namespace),
         }
     }
 }
@@ -842,11 +842,11 @@ mod tests {
 
     #[test]
     fn namespace_validator_allowlist_accepts_known_namespaces() {
-        let set = Arc::new(RwLock::new(BTreeSet::from([
+        let al = OperatorNamespaceAllowlist::from_set(std::collections::BTreeSet::from([
             "ns-a".to_string(),
             "ns-b".to_string(),
-        ])));
-        let v = NamespaceValidator::Allowlist(set);
+        ]));
+        let v = NamespaceValidator::Allowlist(al);
         assert!(v.accepts("ns-a"));
         assert!(v.accepts("ns-b"));
         assert!(!v.accepts("ns-c"));

crates/openshell-server/src/auth/k8s_sa.rs

@@ -750,19 +750,11 @@ impl ComputeRuntime {
         sandbox_watch_bus: SandboxWatchBus,
         tracing_log_bus: TracingLogBus,
         supervisor_sessions: Arc<SupervisorSessionRegistry>,
-    ) -> Result<
-        (
-            Self,
-            Option<Arc<std::sync::RwLock<std::collections::BTreeSet<String>>>>,
-        ),
-        ComputeError,
-    > {
+    ) -> Result<(Self, Option<OperatorNamespaceAllowlist>), ComputeError> {
         let driver = KubernetesComputeDriver::new(config)
             .await
             .map_err(|err| ComputeError::Message(err.to_string()))?;
-        let operator_allowlist_arc = driver
-            .operator_allowlist()
-            .map(OperatorNamespaceAllowlist::shared);
+        let operator_allowlist_arc = driver.operator_allowlist().cloned();
         let driver: SharedComputeDriver = Arc::new(KubernetesDriverService::new(driver));
         let runtime = Self::from_driver(
             ComputeDriverKind::Kubernetes.as_str().to_string(),