fix(kubernetes): address PVC subPath review feedback

Signed-off-by: mjamiv <michael.commack@gmail.com>

Author: mjamiv

crates/openshell-core/src/driver_mounts.rs

@@ -3,6 +3,7 @@
 
 //! Shared validation helpers for driver-config mounts.
 
+use std::collections::HashSet;
 use std::path::Path;
 
 const RESERVED_MOUNT_TARGETS: &[&str] = &[
@@ -12,6 +13,11 @@ const RESERVED_MOUNT_TARGETS: &[&str] = &[
     "/run/netns",
 ];
 
+/// Serde default helper for mount options that default to read-only.
+pub fn default_true() -> bool {
+    true
+}
+
 /// Validate a non-empty driver mount source.
 pub fn validate_mount_source(source: &str, field: &str) -> Result<String, String> {
     let source = source.trim();
@@ -96,13 +102,30 @@ fn normalize_container_mount_target(target: &str) -> String {
     target.trim_end_matches('/').to_string()
 }
 
-fn path_is_or_under(path: &str, parent: &str) -> bool {
+/// Return true when `path` is exactly `parent` or is contained below it.
+pub fn path_is_or_under(path: &str, parent: &str) -> bool {
     path == parent
         || path
             .strip_prefix(parent)
             .is_some_and(|rest| rest.starts_with('/'))
 }
 
+/// Validate that already-normalized driver mount targets are unique.
+pub fn validate_unique_mount_targets<'a>(
+    targets: impl IntoIterator<Item = &'a str>,
+    driver_name: &str,
+) -> Result<(), String> {
+    let mut seen = HashSet::new();
+    for target in targets {
+        if !seen.insert(target) {
+            return Err(format!(
+                "duplicate {driver_name} driver_config mount target '{target}'"
+            ));
+        }
+    }
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -144,6 +167,24 @@ mod tests {
         );
     }
 
+    #[test]
+    fn path_is_or_under_matches_boundaries() {
+        assert!(path_is_or_under("/sandbox", "/sandbox"));
+        assert!(path_is_or_under("/sandbox/work", "/sandbox"));
+        assert!(!path_is_or_under("/sandbox-work", "/sandbox"));
+    }
+
+    #[test]
+    fn unique_mount_targets_rejects_duplicates() {
+        let err =
+            validate_unique_mount_targets(["/sandbox/work", "/sandbox/work"], "test").unwrap_err();
+
+        assert_eq!(
+            err,
+            "duplicate test driver_config mount target '/sandbox/work'"
+        );
+    }
+
     #[test]
     fn mount_subpath_must_be_relative_without_parent_dirs() {
         assert_eq!(validate_mount_subpath(" project/a ").unwrap(), "project/a");

crates/openshell-driver-docker/src/lib.rs

@@ -49,7 +49,7 @@ use openshell_core::proto_struct::{
     deserialize_optional_non_empty_string_list, struct_to_json_value,
 };
 use openshell_core::{Config, Error, Result as CoreResult};
-use std::collections::{HashMap, HashSet};
+use std::collections::HashMap;
 use std::io::Read;
 use std::net::{IpAddr, SocketAddr};
 use std::path::{Path, PathBuf};
@@ -309,13 +309,13 @@ enum DockerDriverMountConfig {
     Bind {
         source: String,
         target: String,
-        #[serde(default = "default_true")]
+        #[serde(default = "driver_mounts::default_true")]
         read_only: bool,
     },
     Volume {
         source: String,
         target: String,
-        #[serde(default = "default_true")]
+        #[serde(default = "driver_mounts::default_true")]
         read_only: bool,
         #[serde(default)]
         subpath: Option<String>,
@@ -332,17 +332,13 @@ enum DockerDriverMountConfig {
     Image {
         source: String,
         target: String,
-        #[serde(default = "default_true")]
+        #[serde(default = "driver_mounts::default_true")]
         read_only: bool,
         #[serde(default)]
         subpath: Option<String>,
     },
 }
 
-fn default_true() -> bool {
-    true
-}
-
 type WatchStream =
     Pin<Box<dyn Stream<Item = Result<WatchSandboxesEvent, Status>> + Send + 'static>>;
 
@@ -1854,7 +1850,7 @@ fn validate_docker_driver_mounts(
     mounts: &[DockerDriverMountConfig],
     enable_bind_mounts: bool,
 ) -> Result<(), Status> {
-    let mut targets = HashSet::new();
+    let mut targets = Vec::with_capacity(mounts.len());
     for mount in mounts {
         let target = match mount {
             DockerDriverMountConfig::Bind { source, target, .. } => {
@@ -1908,13 +1904,10 @@ fn validate_docker_driver_mounts(
         };
         let target = driver_mounts::validate_container_mount_target(target)
             .map_err(Status::failed_precondition)?;
-        if !targets.insert(target.clone()) {
-            return Err(Status::failed_precondition(format!(
-                "duplicate docker driver_config mount target '{target}'"
-            )));
-        }
+        targets.push(target);
     }
-    Ok(())
+    driver_mounts::validate_unique_mount_targets(targets.iter().map(String::as_str), "docker")
+        .map_err(Status::failed_precondition)
 }
 
 fn validate_optional_positive_integral_i64(

crates/openshell-driver-docker/src/tests.rs

@@ -755,6 +755,39 @@ fn driver_config_allows_explicit_writable_volume_mounts() {
     assert_eq!(mounts[0].read_only, Some(false));
 }
 
+#[test]
+fn driver_config_rejects_duplicate_mount_targets() {
+    let mut sandbox = test_sandbox();
+    sandbox
+        .spec
+        .as_mut()
+        .unwrap()
+        .template
+        .as_mut()
+        .unwrap()
+        .driver_config = Some(json_struct(serde_json::json!({
+        "mounts": [
+            {
+                "type": "volume",
+                "source": "work-nfs",
+                "target": "/sandbox/work"
+            },
+            {
+                "type": "tmpfs",
+                "target": "/sandbox/work"
+            }
+        ]
+    })));
+
+    let err = build_container_create_body(&sandbox, &runtime_config()).unwrap_err();
+
+    assert_eq!(err.code(), tonic::Code::FailedPrecondition);
+    assert!(
+        err.message()
+            .contains("duplicate docker driver_config mount target")
+    );
+}
+
 #[test]
 fn driver_config_rejects_bind_mounts_unless_enabled() {
     let mut sandbox = test_sandbox();

crates/openshell-driver-kubernetes/README.md

@@ -114,15 +114,16 @@ resource details.
 
 Use PVC volumes to mount existing Kubernetes PersistentVolumeClaims into the
 agent container. PVC volumes and mounts default to read-only unless
-`read_only: false` is set explicitly. A read-only PVC volume cannot be mounted
-read-write. The driver rejects duplicate volume names, mounts that reference
-unknown volumes, non-normalized or protected mount paths, and absolute or
-parent-traversing `sub_path` values.
-
-Any explicit driver-config mount under `/sandbox/` disables the driver's
-default `/sandbox` workspace PVC injection for that sandbox. This keeps image
-contents fresh while allowing selected durable data paths to come from an
-external PVC.
+`read_only: false` is set explicitly. Read-write access requires
+`read_only: false` on both the PVC volume and each writable mount. The driver
+rejects duplicate volume names, invalid DNS-1123 volume or PVC claim names,
+mounts that reference unknown volumes, non-normalized or protected mount paths,
+and absolute or parent-traversing `sub_path` values.
+
+Any explicit driver-config mount under `/sandbox` disables the driver's
+default `/sandbox` workspace PVC injection for that sandbox. Only the explicit
+mount paths persist through the external PVC; other `/sandbox` paths come from
+the current sandbox image.
 
 ```shell
 openshell sandbox create \