From 631c5cb65fa23749a4268b008f1082e40cce8305 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kova=C4=BE?= Date: Thu, 21 May 2026 12:17:42 +0200 Subject: [PATCH 1/8] feat(cli): add --volume flag to sandbox create Register a repeatable `--volume :[:ro]` clap arg on `SandboxCommands::Create`. Adds an `#[allow(clippy::large_enum_variant)]` to suppress the size-difference lint that fires now that Create has multiple Vec fields. The new field is destructured as `_volumes` in the dispatch match arm; plumbing through the create flow is deferred to a follow-up task. --- crates/openshell-cli/src/main.rs | 17 +++ .../tests/sandbox_create_volume_flag.rs | 118 ++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 crates/openshell-cli/tests/sandbox_create_volume_flag.rs diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index 3a8c344d3..ca16a4c6c 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -1126,6 +1126,7 @@ enum DoctorCommands { } #[derive(Subcommand, Debug)] +#[allow(clippy::large_enum_variant)] // Create variant holds many clap fields by design enum SandboxCommands { /// Create a sandbox. #[command(help_template = LEAF_HELP_TEMPLATE, next_help_heading = "FLAGS")] @@ -1234,6 +1235,21 @@ enum SandboxCommands { #[arg(long = "label")] labels: Vec, + /// Bind-mount a host path into the sandbox. + /// + /// Format: `:[:ro]`. Repeatable. + /// Host path must be absolute and exist. Container path must be + /// absolute. The optional `:ro` suffix makes the mount read-only. + /// + /// On rootless podman, the driver auto-applies + /// `--userns=keep-id:uid=,gid=` + /// when any `--volume` is set, so bind file ownership maps + /// bidirectionally between host and container. + /// + /// Not supported on the vm driver. + #[arg(long = "volume", help_heading = "MOUNT FLAGS")] + volumes: Vec, + /// Command to run after "--" (defaults to an interactive shell). #[arg(last = true, allow_hyphen_values = true)] command: Vec, @@ -2483,6 +2499,7 @@ async fn main() -> Result<()> { auto_providers, no_auto_providers, labels, + volumes: _volumes, command, } => { // Resolve --tty / --no-tty into an Option override. diff --git a/crates/openshell-cli/tests/sandbox_create_volume_flag.rs b/crates/openshell-cli/tests/sandbox_create_volume_flag.rs new file mode 100644 index 000000000..b3586c45c --- /dev/null +++ b/crates/openshell-cli/tests/sandbox_create_volume_flag.rs @@ -0,0 +1,118 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Smoke tests that verify the `--volume` flag is registered on `sandbox create`. +//! +//! These tests run the compiled `openshell` binary and inspect exit codes / help +//! output — no gRPC server required. + +use std::process::Command; + +fn openshell_bin() -> String { + // Cargo sets CARGO_BIN_EXE_ for integration tests in the same + // crate. Fall back to a cargo-run invocation for environments where the + // pre-built binary is not cached. + std::env::var("CARGO_BIN_EXE_openshell") + .unwrap_or_else(|_| "cargo run -p openshell-cli --".to_string()) +} + +/// Assert that `--volume` appears in `sandbox create --help`. +#[test] +fn volume_flag_appears_in_help() { + let bin = openshell_bin(); + let output = Command::new(&bin) + .args(["sandbox", "create", "--help"]) + .output() + .unwrap_or_else(|_| panic!("failed to run {bin}")); + + let combined = String::from_utf8_lossy(&output.stdout).to_string() + + &String::from_utf8_lossy(&output.stderr); + assert!( + combined.contains("--volume"), + "expected --volume in `sandbox create --help` output, got:\n{combined}" + ); +} + +/// Passing `--volume /host:/container` must be accepted (exit 0 up to the +/// gateway connection attempt; before any gateway is reachable the command +/// exits non-zero due to connection failure, but the argument must at least +/// parse without a clap error). +/// +/// We detect a clap parse error (exit code 2) vs a runtime error (exit != 2). +#[test] +fn volume_flag_two_field_spec_parses() { + let bin = openshell_bin(); + let output = Command::new(&bin) + .args([ + "sandbox", + "create", + "--from", + "python", + "--volume", + "/host:/container", + ]) + .env("OPENSHELL_ENDPOINT", "https://127.0.0.1:1") // unreachable → runtime error, not clap error + .env("XDG_CONFIG_HOME", std::env::temp_dir().to_str().unwrap()) + .env("HOME", std::env::temp_dir().to_str().unwrap()) + .output() + .unwrap_or_else(|_| panic!("failed to run {bin}")); + + assert_ne!( + output.status.code(), + Some(2), + "--volume /host:/container should not produce a clap parse error (exit 2);\nstdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); +} + +/// Three-field spec `::ro` must also parse without a clap error. +#[test] +fn volume_flag_three_field_ro_spec_parses() { + let bin = openshell_bin(); + let output = Command::new(&bin) + .args([ + "sandbox", + "create", + "--from", + "python", + "--volume", + "/host:/container:ro", + ]) + .env("OPENSHELL_ENDPOINT", "https://127.0.0.1:1") + .env("XDG_CONFIG_HOME", std::env::temp_dir().to_str().unwrap()) + .env("HOME", std::env::temp_dir().to_str().unwrap()) + .output() + .unwrap_or_else(|_| panic!("failed to run {bin}")); + + assert_ne!( + output.status.code(), + Some(2), + "--volume /host:/container:ro should not produce a clap parse error (exit 2);\nstdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); +} + +/// The flag must be repeatable: two `--volume` flags on the same invocation. +#[test] +fn volume_flag_repeats() { + let bin = openshell_bin(); + let output = Command::new(&bin) + .args([ + "sandbox", "create", "--from", "python", "--volume", "/a:/b", "--volume", "/c:/d:ro", + ]) + .env("OPENSHELL_ENDPOINT", "https://127.0.0.1:1") + .env("XDG_CONFIG_HOME", std::env::temp_dir().to_str().unwrap()) + .env("HOME", std::env::temp_dir().to_str().unwrap()) + .output() + .unwrap_or_else(|_| panic!("failed to run {bin}")); + + assert_ne!( + output.status.code(), + Some(2), + "repeated --volume flags should not produce a clap parse error (exit 2);\nstdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); +} From 32f520277f05e7d5d421d10b6bf7d162ff4d5287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kova=C4=BE?= Date: Thu, 21 May 2026 12:31:06 +0200 Subject: [PATCH 2/8] refactor(cli): tighten --volume flag test (in-process) + verify clippy allow - Replace dead openshell_bin() fallback with env!("CARGO_BIN_EXE_openshell") so a missing binary fails at compile time rather than panicking at runtime with a space-in-path exec error. - Strengthen the three parse-acceptance tests: assert exit code == 1 (no gateway configured) rather than != 2, pinning the exact failure mode and ruling out silent success (exit 0) as well as clap errors (exit 2). - Drop OPENSHELL_ENDPOINT env var from parse tests; the "No active gateway" runtime check fires first (exit 1) before any network I/O. - Add module-level doc comment explaining why subprocess is retained over in-process (Cli is private; run::sandbox_create bypasses clap). - Verify large_enum_variant clippy allow: lint fires at 297 bytes (Create) vs 78 bytes (next variant). Update comment to cite actual sizes instead of "by design". --- crates/openshell-cli/src/main.rs | 4 +- .../tests/sandbox_create_volume_flag.rs | 81 ++++++++++++------- 2 files changed, 54 insertions(+), 31 deletions(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index ca16a4c6c..aa6c93b85 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -1126,7 +1126,9 @@ enum DoctorCommands { } #[derive(Subcommand, Debug)] -#[allow(clippy::large_enum_variant)] // Create variant holds many clap fields by design +// Create variant is 297 bytes (vs next-largest 78 bytes) because it holds every clap field for +// `sandbox create`. Boxing would scatter heap allocations across command startup for no benefit. +#[allow(clippy::large_enum_variant)] enum SandboxCommands { /// Create a sandbox. #[command(help_template = LEAF_HELP_TEMPLATE, next_help_heading = "FLAGS")] diff --git a/crates/openshell-cli/tests/sandbox_create_volume_flag.rs b/crates/openshell-cli/tests/sandbox_create_volume_flag.rs index b3586c45c..c366b5f26 100644 --- a/crates/openshell-cli/tests/sandbox_create_volume_flag.rs +++ b/crates/openshell-cli/tests/sandbox_create_volume_flag.rs @@ -5,22 +5,37 @@ //! //! These tests run the compiled `openshell` binary and inspect exit codes / help //! output — no gRPC server required. +//! +//! # Why subprocess instead of in-process +//! +//! `Cli` is a private type in `main.rs`, so `Cli::command()` / `Cli::try_parse_from` +//! cannot be called from tests. The public `run::sandbox_create` entry point accepts +//! already-parsed arguments, so calling it directly would bypass clap entirely. The +//! lifecycle integration test (`sandbox_create_lifecycle_integration.rs`) tests the +//! *runtime* path and requires a full mock gRPC+TLS server — that infrastructure is +//! out of scope for a pure parse-acceptance check. +//! +//! We therefore retain the subprocess approach. With `HOME` and `XDG_CONFIG_HOME` +//! pointing to an empty temp directory, no gateway is configured, so the binary +//! immediately exits with code 1 ("No active gateway") before any network I/O. +//! A clap parse failure exits with code 2; the test asserts the exact value is 1. use std::process::Command; -fn openshell_bin() -> String { - // Cargo sets CARGO_BIN_EXE_ for integration tests in the same - // crate. Fall back to a cargo-run invocation for environments where the - // pre-built binary is not cached. - std::env::var("CARGO_BIN_EXE_openshell") - .unwrap_or_else(|_| "cargo run -p openshell-cli --".to_string()) +/// Canonical path to the compiled `openshell` binary. +/// +/// `CARGO_BIN_EXE_openshell` is set by Cargo for every integration test in the +/// same crate. Using `env!` fails at compile time rather than silently falling +/// back to a broken runtime path. +fn openshell_bin() -> &'static str { + env!("CARGO_BIN_EXE_openshell") } /// Assert that `--volume` appears in `sandbox create --help`. #[test] fn volume_flag_appears_in_help() { let bin = openshell_bin(); - let output = Command::new(&bin) + let output = Command::new(bin) .args(["sandbox", "create", "--help"]) .output() .unwrap_or_else(|_| panic!("failed to run {bin}")); @@ -33,16 +48,16 @@ fn volume_flag_appears_in_help() { ); } -/// Passing `--volume /host:/container` must be accepted (exit 0 up to the -/// gateway connection attempt; before any gateway is reachable the command -/// exits non-zero due to connection failure, but the argument must at least -/// parse without a clap error). +/// Passing `--volume /host:/container` must be accepted by clap. /// -/// We detect a clap parse error (exit code 2) vs a runtime error (exit != 2). +/// With no gateway configured (empty HOME / XDG_CONFIG_HOME) the binary exits +/// with code 1 ("No active gateway") before any network I/O. A clap parse +/// failure would produce exit code 2. We assert the exact code is 1 to confirm +/// clap accepted the flag and only the runtime path failed. #[test] fn volume_flag_two_field_spec_parses() { let bin = openshell_bin(); - let output = Command::new(&bin) + let output = Command::new(bin) .args([ "sandbox", "create", @@ -51,16 +66,18 @@ fn volume_flag_two_field_spec_parses() { "--volume", "/host:/container", ]) - .env("OPENSHELL_ENDPOINT", "https://127.0.0.1:1") // unreachable → runtime error, not clap error .env("XDG_CONFIG_HOME", std::env::temp_dir().to_str().unwrap()) .env("HOME", std::env::temp_dir().to_str().unwrap()) .output() .unwrap_or_else(|_| panic!("failed to run {bin}")); - assert_ne!( - output.status.code(), - Some(2), - "--volume /host:/container should not produce a clap parse error (exit 2);\nstdout: {}\nstderr: {}", + let exit_code = output.status.code(); + assert_eq!( + exit_code, + Some(1), + "--volume /host:/container should fail with exit 1 (no gateway configured), \ + not 2 (clap parse error) or 0 (unexpected success); \ + got exit {exit_code:?}\nstdout: {}\nstderr: {}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr), ); @@ -70,7 +87,7 @@ fn volume_flag_two_field_spec_parses() { #[test] fn volume_flag_three_field_ro_spec_parses() { let bin = openshell_bin(); - let output = Command::new(&bin) + let output = Command::new(bin) .args([ "sandbox", "create", @@ -79,16 +96,18 @@ fn volume_flag_three_field_ro_spec_parses() { "--volume", "/host:/container:ro", ]) - .env("OPENSHELL_ENDPOINT", "https://127.0.0.1:1") .env("XDG_CONFIG_HOME", std::env::temp_dir().to_str().unwrap()) .env("HOME", std::env::temp_dir().to_str().unwrap()) .output() .unwrap_or_else(|_| panic!("failed to run {bin}")); - assert_ne!( - output.status.code(), - Some(2), - "--volume /host:/container:ro should not produce a clap parse error (exit 2);\nstdout: {}\nstderr: {}", + let exit_code = output.status.code(); + assert_eq!( + exit_code, + Some(1), + "--volume /host:/container:ro should fail with exit 1 (no gateway configured), \ + not 2 (clap parse error) or 0 (unexpected success); \ + got exit {exit_code:?}\nstdout: {}\nstderr: {}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr), ); @@ -98,20 +117,22 @@ fn volume_flag_three_field_ro_spec_parses() { #[test] fn volume_flag_repeats() { let bin = openshell_bin(); - let output = Command::new(&bin) + let output = Command::new(bin) .args([ "sandbox", "create", "--from", "python", "--volume", "/a:/b", "--volume", "/c:/d:ro", ]) - .env("OPENSHELL_ENDPOINT", "https://127.0.0.1:1") .env("XDG_CONFIG_HOME", std::env::temp_dir().to_str().unwrap()) .env("HOME", std::env::temp_dir().to_str().unwrap()) .output() .unwrap_or_else(|_| panic!("failed to run {bin}")); - assert_ne!( - output.status.code(), - Some(2), - "repeated --volume flags should not produce a clap parse error (exit 2);\nstdout: {}\nstderr: {}", + let exit_code = output.status.code(); + assert_eq!( + exit_code, + Some(1), + "repeated --volume flags should fail with exit 1 (no gateway configured), \ + not 2 (clap parse error) or 0 (unexpected success); \ + got exit {exit_code:?}\nstdout: {}\nstderr: {}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr), ); From 60219bee90b602563c4fed9404da1387455f3a8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kova=C4=BE?= Date: Thu, 21 May 2026 15:13:07 +0200 Subject: [PATCH 3/8] feat(cli): parse --volume HOST:CONTAINER[:ro] specs into typed struct Introduces volume_spec module with BindVolumeSpec, VolumeParseError, and parse_volume_spec, validated by 7 unit tests covering both happy paths and all error variants (bad field count, bad ro token, non-absolute/missing host, non-absolute container). --- crates/openshell-cli/src/lib.rs | 1 + crates/openshell-cli/src/volume_spec.rs | 122 ++++++++++++++++++ .../tests/sandbox_create_volume_flag.rs | 2 +- 3 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 crates/openshell-cli/src/volume_spec.rs diff --git a/crates/openshell-cli/src/lib.rs b/crates/openshell-cli/src/lib.rs index 84a87acd2..3af44065c 100644 --- a/crates/openshell-cli/src/lib.rs +++ b/crates/openshell-cli/src/lib.rs @@ -16,3 +16,4 @@ pub(crate) mod policy_update; pub mod run; pub mod ssh; pub mod tls; +pub mod volume_spec; diff --git a/crates/openshell-cli/src/volume_spec.rs b/crates/openshell-cli/src/volume_spec.rs new file mode 100644 index 000000000..9fe4752b2 --- /dev/null +++ b/crates/openshell-cli/src/volume_spec.rs @@ -0,0 +1,122 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Parsing for `--volume HOST:CONTAINER[:ro]` specs. + +use std::path::Path; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct BindVolumeSpec { + pub host: String, + pub container: String, + pub read_only: bool, +} + +#[derive(Debug, thiserror::Error, PartialEq, Eq)] +pub enum VolumeParseError { + #[error("--volume spec must have 2 or 3 colon-separated fields: {0}")] + BadFieldCount(String), + #[error("--volume spec third field must be 'ro': {0}")] + BadReadOnlyToken(String), + #[error("--volume host path must be absolute: {0}")] + HostNotAbsolute(String), + #[error("--volume host path does not exist: {0}")] + HostMissing(String), + #[error("--volume container path must be absolute: {0}")] + ContainerNotAbsolute(String), +} + +pub fn parse_volume_spec(s: &str) -> Result { + let parts: Vec<&str> = s.split(':').collect(); + let (host, container, read_only) = match parts.as_slice() { + [h, c] => (h.to_string(), c.to_string(), false), + [h, c, ro] => { + if *ro != "ro" { + return Err(VolumeParseError::BadReadOnlyToken(s.to_string())); + } + (h.to_string(), c.to_string(), true) + } + _ => return Err(VolumeParseError::BadFieldCount(s.to_string())), + }; + if !Path::new(&host).is_absolute() { + return Err(VolumeParseError::HostNotAbsolute(host)); + } + if !Path::new(&host).exists() { + return Err(VolumeParseError::HostMissing(host)); + } + if !Path::new(&container).is_absolute() { + return Err(VolumeParseError::ContainerNotAbsolute(container)); + } + Ok(BindVolumeSpec { + host, + container, + read_only, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + #[test] + fn parses_two_field_spec() { + let dir = TempDir::new().unwrap(); + let host = dir.path().to_string_lossy().to_string(); + let spec = format!("{host}:/sandbox/repo"); + let parsed = parse_volume_spec(&spec).unwrap(); + assert_eq!(parsed.host, host); + assert_eq!(parsed.container, "/sandbox/repo"); + assert!(!parsed.read_only); + } + + #[test] + fn parses_three_field_ro_spec() { + let dir = TempDir::new().unwrap(); + let host = dir.path().to_string_lossy().to_string(); + let spec = format!("{host}:/c:ro"); + let parsed = parse_volume_spec(&spec).unwrap(); + assert!(parsed.read_only); + } + + #[test] + fn rejects_bad_field_count() { + assert!(matches!( + parse_volume_spec("a:b:c:d"), + Err(VolumeParseError::BadFieldCount(_)) + )); + assert!(matches!( + parse_volume_spec("only-one"), + Err(VolumeParseError::BadFieldCount(_)) + )); + } + + #[test] + fn rejects_bad_ro_token() { + // /tmp exists on every Unix CI host so the path checks pass before + // reaching the read-only token check. + let r = parse_volume_spec("/tmp:/c:readonly"); + assert!(matches!(r, Err(VolumeParseError::BadReadOnlyToken(_)))); + } + + #[test] + fn rejects_non_absolute_host() { + let r = parse_volume_spec("rel/path:/c"); + assert!(matches!(r, Err(VolumeParseError::HostNotAbsolute(_)))); + } + + #[test] + fn rejects_missing_host() { + let r = parse_volume_spec("/definitely/does/not/exist/here:/c"); + assert!(matches!(r, Err(VolumeParseError::HostMissing(_)))); + } + + #[test] + fn rejects_non_absolute_container() { + let dir = TempDir::new().unwrap(); + let host = dir.path().to_string_lossy().to_string(); + let spec = format!("{host}:rel/path"); + let r = parse_volume_spec(&spec); + assert!(matches!(r, Err(VolumeParseError::ContainerNotAbsolute(_)))); + } +} diff --git a/crates/openshell-cli/tests/sandbox_create_volume_flag.rs b/crates/openshell-cli/tests/sandbox_create_volume_flag.rs index c366b5f26..3bae5140f 100644 --- a/crates/openshell-cli/tests/sandbox_create_volume_flag.rs +++ b/crates/openshell-cli/tests/sandbox_create_volume_flag.rs @@ -50,7 +50,7 @@ fn volume_flag_appears_in_help() { /// Passing `--volume /host:/container` must be accepted by clap. /// -/// With no gateway configured (empty HOME / XDG_CONFIG_HOME) the binary exits +/// With no gateway configured (empty HOME / `XDG_CONFIG_HOME`) the binary exits /// with code 1 ("No active gateway") before any network I/O. A clap parse /// failure would produce exit code 2. We assert the exact code is 1 to confirm /// clap accepted the flag and only the runtime path failed. From 4cac925746ac6a387da3f6a5b505527565bf0cdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kova=C4=BE?= Date: Thu, 21 May 2026 15:36:11 +0200 Subject: [PATCH 4/8] feat: plumb --volume specs from CLI through proto to DriverSandbox Add BindVolume message to openshell.proto (SandboxSpec.volumes = 11) and compute_driver.proto (DriverSandboxSpec.volumes = 11). Wire parsed BindVolumeSpec entries from the CLI handler through run::sandbox_create into the CreateSandboxRequest, and map them to DriverBindVolume in driver_sandbox_spec_from_public on the server side. Fix the docker driver test fixture and integration test call sites for the updated sandbox_create signature. --- crates/openshell-cli/src/main.rs | 11 ++++++++++- crates/openshell-cli/src/run.rs | 14 +++++++++++++- .../sandbox_create_lifecycle_integration.rs | 9 +++++++++ crates/openshell-driver-docker/src/tests.rs | 1 + crates/openshell-server/src/compute/mod.rs | 17 +++++++++++++---- proto/compute_driver.proto | 10 ++++++++++ proto/openshell.proto | 10 ++++++++++ 7 files changed, 66 insertions(+), 6 deletions(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index aa6c93b85..2d14519fa 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -19,6 +19,7 @@ use openshell_bootstrap::{ use openshell_cli::completers; use openshell_cli::run; use openshell_cli::tls::TlsOptions; +use openshell_cli::volume_spec; /// Resolved gateway context: name + gateway endpoint. struct GatewayContext { @@ -2501,7 +2502,7 @@ async fn main() -> Result<()> { auto_providers, no_auto_providers, labels, - volumes: _volumes, + volumes, command, } => { // Resolve --tty / --no-tty into an Option override. @@ -2547,6 +2548,13 @@ async fn main() -> Result<()> { .transpose()?; let keep = keep || !no_keep || editor.is_some() || forward.is_some(); + // Parse --volume specs into BindVolumeSpec entries. + let parsed_volumes = volumes + .iter() + .map(|s| volume_spec::parse_volume_spec(s)) + .collect::, _>>() + .map_err(|e| miette::miette!("{}", e))?; + let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let endpoint = &ctx.endpoint; let mut tls = tls.with_gateway_name(&ctx.name); @@ -2570,6 +2578,7 @@ async fn main() -> Result<()> { tty_override, auto_providers_override, &labels_map, + &parsed_volumes, &tls, )) .await?; diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 2e3cb0531..e77a40b71 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -8,6 +8,7 @@ use crate::tls::{ TlsOptions, build_insecure_rustls_config, build_rustls_config, grpc_client, grpc_inference_client, require_tls_materials, }; +use crate::volume_spec::BindVolumeSpec; use bytes::Bytes; use chrono::DateTime; use dialoguer::{Confirm, Select, theme::ColorfulTheme}; @@ -32,7 +33,7 @@ use openshell_core::progress::{ use openshell_core::proto::ProviderProfileCategory; use openshell_core::proto::{ ApproveAllDraftChunksRequest, ApproveDraftChunkRequest, AttachSandboxProviderRequest, - ClearDraftChunksRequest, ConfigureProviderRefreshRequest, CreateProviderRequest, + BindVolume, ClearDraftChunksRequest, ConfigureProviderRefreshRequest, CreateProviderRequest, CreateSandboxRequest, CreateSshSessionRequest, DeleteProviderProfileRequest, DeleteProviderRefreshRequest, DeleteProviderRequest, DeleteSandboxRequest, DeleteServiceRequest, DetachSandboxProviderRequest, ExecSandboxRequest, ExposeServiceRequest, @@ -1626,6 +1627,7 @@ pub async fn sandbox_create( tty_override: Option, auto_providers_override: Option, labels: &HashMap, + volumes: &[BindVolumeSpec], tls: &TlsOptions, ) -> Result<()> { if editor.is_some() && !command.is_empty() { @@ -1692,6 +1694,15 @@ pub async fn sandbox_create( None }; + let proto_volumes: Vec = volumes + .iter() + .map(|v| BindVolume { + host_path: v.host.clone(), + container_path: v.container.clone(), + read_only: v.read_only, + }) + .collect(); + let request = CreateSandboxRequest { spec: Some(SandboxSpec { gpu: requested_gpu, @@ -1699,6 +1710,7 @@ pub async fn sandbox_create( policy, providers: configured_providers, template, + volumes: proto_volumes, ..SandboxSpec::default() }), name: name.unwrap_or_default().to_string(), diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 8e606beea..2a82ae33f 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -769,6 +769,7 @@ async fn sandbox_create_keeps_command_sessions_by_default() { Some(false), Some(false), &HashMap::new(), + &[], &tls, ) .await @@ -810,6 +811,7 @@ async fn sandbox_create_sends_cpu_and_memory_limits_only() { Some(false), Some(false), &HashMap::new(), + &[], &tls, ) .await @@ -895,6 +897,7 @@ async fn sandbox_create_returns_vm_error_without_waiting_for_timeout() { Some(false), Some(false), &HashMap::new(), + &[], &tls, ) .await @@ -947,6 +950,7 @@ async fn sandbox_create_keeps_waiting_while_vm_progress_arrives() { Some(false), Some(false), &HashMap::new(), + &[], &tls, ) .await @@ -991,6 +995,7 @@ async fn sandbox_create_times_out_when_only_logs_arrive() { Some(false), Some(false), &HashMap::new(), + &[], &tls, ) .await @@ -1031,6 +1036,7 @@ async fn sandbox_create_deletes_command_sessions_with_no_keep() { Some(false), Some(false), &HashMap::new(), + &[], &tls, ) .await @@ -1075,6 +1081,7 @@ async fn sandbox_create_deletes_shell_sessions_with_no_keep() { Some(true), Some(false), &HashMap::new(), + &[], &tls, ) .await @@ -1119,6 +1126,7 @@ async fn sandbox_create_keeps_sandbox_with_hidden_keep_flag() { Some(false), Some(false), &HashMap::new(), + &[], &tls, ) .await @@ -1163,6 +1171,7 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { Some(false), Some(false), &HashMap::new(), + &[], &tls, ) .await diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index 2ac2da1ee..bf6106a85 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -33,6 +33,7 @@ fn test_sandbox() -> DriverSandbox { }), gpu: false, gpu_device: String::new(), + volumes: vec![], }), status: None, } diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index a69231dea..11b55d0f4 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -16,10 +16,10 @@ use crate::supervisor_session::SupervisorSessionRegistry; use crate::tracing_bus::TracingLogBus; use futures::{Stream, StreamExt}; use openshell_core::proto::compute::v1::{ - CreateSandboxRequest, DeleteSandboxRequest, DriverCondition, DriverPlatformEvent, - DriverResourceRequirements, DriverSandbox, DriverSandboxSpec, DriverSandboxStatus, - DriverSandboxTemplate, GetCapabilitiesRequest, GetSandboxRequest, ListSandboxesRequest, - ValidateSandboxCreateRequest, WatchSandboxesEvent, WatchSandboxesRequest, + BindVolume as DriverBindVolume, CreateSandboxRequest, DeleteSandboxRequest, DriverCondition, + DriverPlatformEvent, DriverResourceRequirements, DriverSandbox, DriverSandboxSpec, + DriverSandboxStatus, DriverSandboxTemplate, GetCapabilitiesRequest, GetSandboxRequest, + ListSandboxesRequest, ValidateSandboxCreateRequest, WatchSandboxesEvent, WatchSandboxesRequest, compute_driver_client::ComputeDriverClient, compute_driver_server::ComputeDriver, watch_sandboxes_event, }; @@ -1229,6 +1229,15 @@ fn driver_sandbox_spec_from_public(spec: &SandboxSpec) -> DriverSandboxSpec { .map(driver_sandbox_template_from_public), gpu: spec.gpu, gpu_device: spec.gpu_device.clone(), + volumes: spec + .volumes + .iter() + .map(|v| DriverBindVolume { + host_path: v.host_path.clone(), + container_path: v.container_path.clone(), + read_only: v.read_only, + }) + .collect(), } } diff --git a/proto/compute_driver.proto b/proto/compute_driver.proto index 3c4308f3f..bd97ff1b5 100644 --- a/proto/compute_driver.proto +++ b/proto/compute_driver.proto @@ -90,6 +90,16 @@ message DriverSandboxSpec { // (e.g. "0", "1"). When empty with gpu=true, the driver assigns the // first available GPU. string gpu_device = 10; + // Bind-mount entries: live host-path passthrough into the sandbox. + repeated BindVolume volumes = 11; +} + +// A bind-mounted volume from the host into the sandbox. +// Mirrors openshell.v1.BindVolume; server maps between them. +message BindVolume { + string host_path = 1; + string container_path = 2; + bool read_only = 3; } // Driver-owned runtime template consumed by the compute platform. diff --git a/proto/openshell.proto b/proto/openshell.proto index ca62646e3..21bb04d6d 100644 --- a/proto/openshell.proto +++ b/proto/openshell.proto @@ -277,6 +277,16 @@ message SandboxSpec { // (e.g. "0", "1"). When empty with gpu=true, the driver assigns the // first available GPU. string gpu_device = 10; + // Bind-mount entries: live host-path passthrough into the sandbox. + repeated BindVolume volumes = 11; +} + +// A bind-mounted volume from the host into the sandbox. +// Equivalent to a `podman -v HOST:CONTAINER[:ro]` argument. +message BindVolume { + string host_path = 1; + string container_path = 2; + bool read_only = 3; } // Public sandbox template mapped onto compute-driver template inputs. From db1952716abfd70ee5f8a8a272871d9402dd7527 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kova=C4=BE?= Date: Thu, 21 May 2026 16:15:45 +0200 Subject: [PATCH 5/8] feat(podman): emit bind mounts + auto userns=keep-id when volumes present Append a libpod bind-mount entry for each DriverSandboxSpec.volume, and set userns=keep-id:uid=N,gid=N (from the image USER directive) when any volumes are present. Falls back to uid/gid 1000660000 for unset or non-numeric USER directives. --- crates/openshell-driver-podman/src/client.rs | 25 +++ .../openshell-driver-podman/src/container.rs | 156 ++++++++++++++++-- crates/openshell-driver-podman/src/driver.rs | 8 +- 3 files changed, 172 insertions(+), 17 deletions(-) diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 834fb21a2..4486d9577 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -580,6 +580,31 @@ impl PodmanClient { Ok(()) } + /// Inspect an image and return its USER directive parsed as (uid, gid). + /// + /// Returns `Ok((1_000_660_000, 1_000_660_000))` if USER is unset or + /// non-numeric (community-image convention for the rootless sandbox UID). + pub async fn image_user(&self, image_ref: &str) -> Result<(u32, u32), PodmanApiError> { + let path = format!("/libpod/images/{}/json", url_encode(image_ref)); + let v: Value = self + .request_json(hyper::Method::GET, &path, None::<&Value>) + .await?; + let user = v + .pointer("/Config/User") + .and_then(|u| u.as_str()) + .unwrap_or(""); + if user.is_empty() { + return Ok((1_000_660_000, 1_000_660_000)); + } + let parts: Vec<&str> = user.split(':').collect(); + let uid: u32 = parts + .first() + .and_then(|s| s.parse().ok()) + .unwrap_or(1_000_660_000); + let gid: u32 = parts.get(1).and_then(|s| s.parse().ok()).unwrap_or(uid); + Ok((uid, gid)) + } + // ── System operations ──────────────────────────────────────────────── /// Ping the Podman API to verify connectivity. diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index c3f2c3282..74c9bcc2a 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -119,6 +119,8 @@ struct ContainerSpec { /// Port mappings from host to container. Using `host_port=0` requests an /// ephemeral port, readable back from the inspect response. portmappings: Vec, + #[serde(skip_serializing_if = "Option::is_none")] + userns: Option, } /// A port mapping entry for the libpod `SpecGenerator`. @@ -200,6 +202,14 @@ struct NetNS { nsmode: String, } +/// libpod `Namespace` entry for `userns`. +#[derive(Serialize)] +struct UserNamespace { + nsmode: String, + #[serde(skip_serializing_if = "String::is_empty")] + value: String, +} + #[derive(Serialize)] struct NetworkAttachment {} @@ -364,7 +374,11 @@ fn build_devices(sandbox: &DriverSandbox) -> Option> { /// Build the Podman container creation JSON spec. #[must_use] -pub fn build_container_spec(sandbox: &DriverSandbox, config: &PodmanComputeConfig) -> Value { +pub fn build_container_spec( + sandbox: &DriverSandbox, + config: &PodmanComputeConfig, + image_sandbox_user: Option<(u32, u32)>, +) -> Value { let image = resolve_image(sandbox, config); let name = container_name(&sandbox.name); let vol = volume_name(&sandbox.id); @@ -381,7 +395,7 @@ pub fn build_container_spec(sandbox: &DriverSandbox, config: &PodmanComputeConfi let mut networks = BTreeMap::new(); networks.insert(config.network_name.clone(), NetworkAttachment {}); - let container_spec = ContainerSpec { + let mut container_spec = ContainerSpec { name, image: image.to_string(), labels, @@ -573,8 +587,32 @@ pub fn build_container_spec(sandbox: &DriverSandbox, config: &PodmanComputeConfi container_port: openshell_core::config::DEFAULT_SSH_PORT, protocol: "tcp".into(), }], + userns: None, }; + // Bind mounts requested via the CLI's --volume flag. + if let Some(spec) = sandbox.spec.as_ref() { + for v in &spec.volumes { + let mut options = vec!["rbind".to_string()]; + if v.read_only { + options.push("ro".into()); + } + container_spec.mounts.push(Mount { + kind: "bind".into(), + source: v.host_path.clone(), + destination: v.container_path.clone(), + options, + }); + } + if !spec.volumes.is_empty() { + let (uid, gid) = image_sandbox_user.unwrap_or((1_000_660_000, 1_000_660_000)); + container_spec.userns = Some(UserNamespace { + nsmode: "keep-id".into(), + value: format!("uid={uid},gid={gid}"), + }); + } + } + serde_json::to_value(container_spec).expect("ContainerSpec serialization cannot fail") } @@ -688,7 +726,7 @@ mod tests { ..Default::default() }); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); assert_eq!( spec["resource_limits"]["cpu"]["quota"].as_u64(), @@ -723,7 +761,7 @@ mod tests { fn container_spec_omits_devices_without_gpu_request() { let sandbox = test_sandbox("test-id", "test-name"); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); assert!(spec.get("devices").is_none()); } @@ -739,7 +777,7 @@ mod tests { ..Default::default() }); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); assert_eq!( spec["devices"][0]["path"].as_str(), @@ -758,7 +796,7 @@ mod tests { ..Default::default() }); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); assert_eq!( spec["devices"][0]["path"].as_str(), @@ -770,7 +808,7 @@ mod tests { fn container_spec_includes_required_capabilities() { let sandbox = test_sandbox("test-id", "test-name"); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); let added: Vec<&str> = spec["cap_add"] .as_array() @@ -818,7 +856,7 @@ mod tests { fn container_spec_sets_sandbox_name_in_env() { let sandbox = test_sandbox("test-id", "my-sandbox"); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); let env_map = spec["env"].as_object().expect("env should be an object"); assert_eq!( @@ -833,7 +871,7 @@ mod tests { fn container_spec_sets_ssh_socket_path_in_env() { let sandbox = test_sandbox("test-id", "test-name"); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); let env_map = spec["env"].as_object().expect("env should be an object"); assert_eq!( @@ -848,7 +886,7 @@ mod tests { fn container_spec_healthcheck_accepts_supervisor_socket() { let sandbox = test_sandbox("test-id", "test-name"); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); let healthcheck = spec["healthconfig"]["test"] .as_array() @@ -885,7 +923,7 @@ mod tests { }); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); let env_map = spec["env"].as_object().expect("env should be an object"); @@ -933,7 +971,7 @@ mod tests { }); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); let labels = spec["labels"] .as_object() @@ -963,7 +1001,7 @@ mod tests { fn container_spec_injects_host_aliases() { let sandbox = test_sandbox("test-id", "test-name"); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); let hostadd: Vec<&str> = spec["hostadd"] .as_array() @@ -1022,7 +1060,7 @@ mod tests { fn container_spec_includes_supervisor_image_volume() { let sandbox = test_sandbox("test-id", "test-name"); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); let image_volumes = spec["image_volumes"] .as_array() @@ -1059,7 +1097,7 @@ mod tests { config.guest_tls_cert = Some(std::path::PathBuf::from("/host/tls.crt")); config.guest_tls_key = Some(std::path::PathBuf::from("/host/tls.key")); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); // Verify TLS env vars are set. let env_map = spec["env"].as_object().expect("env should be an object"); @@ -1120,7 +1158,7 @@ mod tests { let sandbox = test_sandbox("notls-id", "notls-name"); let config = test_config(); - let spec = build_container_spec(&sandbox, &config); + let spec = build_container_spec(&sandbox, &config, None); let env_map = spec["env"].as_object().expect("env should be an object"); assert!( @@ -1137,4 +1175,90 @@ mod tests { .count(); assert_eq!(bind_count, 0, "no bind mounts without TLS config"); } + + #[test] + fn build_container_spec_emits_bind_mount_entries() { + let mut sandbox = test_sandbox("id-1", "name-1"); + sandbox.spec = Some(openshell_core::proto::compute::v1::DriverSandboxSpec { + volumes: vec![ + openshell_core::proto::compute::v1::BindVolume { + host_path: "/host/a".into(), + container_path: "/container/a".into(), + read_only: false, + }, + openshell_core::proto::compute::v1::BindVolume { + host_path: "/host/b".into(), + container_path: "/container/b".into(), + read_only: true, + }, + ], + ..Default::default() + }); + let cfg = test_config(); + let spec_value = build_container_spec(&sandbox, &cfg, None); + let mounts = spec_value + .get("mounts") + .and_then(|v| v.as_array()) + .expect("mounts array"); + let bind_mounts: Vec<_> = mounts + .iter() + .filter(|m| m.get("type").and_then(|t| t.as_str()) == Some("bind")) + .collect(); + assert_eq!(bind_mounts.len(), 2, "expected exactly 2 bind mounts"); + assert_eq!( + bind_mounts[0] + .get("source") + .and_then(|v| v.as_str()) + .unwrap(), + "/host/a" + ); + assert_eq!( + bind_mounts[0] + .get("destination") + .and_then(|v| v.as_str()) + .unwrap(), + "/container/a" + ); + let opts: Vec<&str> = bind_mounts[1] + .get("options") + .and_then(|v| v.as_array()) + .unwrap() + .iter() + .filter_map(|o| o.as_str()) + .collect(); + assert!(opts.contains(&"rbind")); + assert!(opts.contains(&"ro")); + } + + #[test] + fn build_container_spec_sets_userns_keep_id_when_volumes_present() { + let mut sandbox = test_sandbox("id-1", "name-1"); + sandbox.spec = Some(openshell_core::proto::compute::v1::DriverSandboxSpec { + volumes: vec![openshell_core::proto::compute::v1::BindVolume { + host_path: "/host".into(), + container_path: "/container".into(), + read_only: false, + }], + ..Default::default() + }); + let cfg = test_config(); + let spec_value = build_container_spec(&sandbox, &cfg, Some((1_000_660_000, 1_000_660_000))); + let userns = spec_value.get("userns").expect("userns set"); + assert_eq!( + userns.get("nsmode").and_then(|v| v.as_str()).unwrap(), + "keep-id" + ); + assert_eq!( + userns.get("value").and_then(|v| v.as_str()).unwrap(), + "uid=1000660000,gid=1000660000" + ); + } + + #[test] + fn build_container_spec_omits_userns_when_no_volumes() { + let sandbox = test_sandbox("id-1", "name-1"); + let cfg = test_config(); + let spec_value = build_container_spec(&sandbox, &cfg, None); + assert!(spec_value.get("userns").is_none() || spec_value.get("userns").unwrap().is_null()); + } } diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index 3d88e12ac..0d962fac6 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -274,7 +274,13 @@ impl PodmanComputeDriver { } // 3. Create container. - let spec = container::build_container_spec(sandbox, &self.config); + let image_sandbox_user = if sandbox.spec.as_ref().is_some_and(|s| !s.volumes.is_empty()) { + let image_ref = container::resolve_image(sandbox, &self.config); + Some(self.client.image_user(image_ref).await?) + } else { + None + }; + let spec = container::build_container_spec(sandbox, &self.config, image_sandbox_user); match self.client.create_container(&spec).await { Ok(_) => {} Err(PodmanApiError::Conflict(_)) => { From b57030a62d1274ebc0fd01c03689d95bab111a65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kova=C4=BE?= Date: Thu, 21 May 2026 16:26:09 +0200 Subject: [PATCH 6/8] refactor(podman): tighten image_user parsing + extract COMMUNITY_SANDBOX_UID Extract the magic number 1_000_660_000 into a named pub constant COMMUNITY_SANDBOX_UID so all four usage sites share a single source of truth with a doc-comment linking to the Dockerfile origin. Fix a split-uid bug in image_user: when Config.User is ":1000", the uid field is "" which fails to parse as u32. Previously gid was still extracted from parts[1], producing a mismatched (1_000_660_000, 1000) pair. Now the function returns the fallback pair immediately on any uid parse failure, matching the documented contract. Expand the image_user doc-comment to cover the uid=0/root case (keep-id:uid=0,gid=0 is harmless on rootless Podman), the empty-user case, and the non-numeric-user case. Add an rbind/read-write assertion on bind_mounts[0] in the existing build_container_spec_emits_bind_mount_entries test. --- crates/openshell-driver-podman/src/client.rs | 32 +++++++++++++++---- .../openshell-driver-podman/src/container.rs | 13 +++++++- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 4486d9577..bae40ed0e 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -20,6 +20,12 @@ use tracing::debug; /// Podman libpod API version prefix. const API_VERSION: &str = "v5.0.0"; +/// Default sandbox user UID for the openshell community sandbox images. +/// Used as the userns-remap target when an image's `Config.User` is unset +/// or non-numeric. Source: examples/bring-your-own-container/Dockerfile +/// uses `useradd -m -u 1000660000 -g sandbox sandbox`. +pub const COMMUNITY_SANDBOX_UID: u32 = 1_000_660_000; + /// Timeout for individual Podman API calls. const API_TIMEOUT: Duration = Duration::from_secs(30); @@ -582,8 +588,21 @@ impl PodmanClient { /// Inspect an image and return its USER directive parsed as (uid, gid). /// - /// Returns `Ok((1_000_660_000, 1_000_660_000))` if USER is unset or - /// non-numeric (community-image convention for the rootless sandbox UID). + /// Parsing rules: + /// + /// - `""` (unset USER): returns the community-sandbox fallback + /// `(COMMUNITY_SANDBOX_UID, COMMUNITY_SANDBOX_UID)`. + /// - `"uid"` or `"uid:gid"`: parses both fields as `u32`. If the uid + /// field is missing or non-numeric (e.g. `"sandbox"` or `":1000"`), + /// returns the community-sandbox fallback immediately — the gid field + /// is not examined, preventing a mismatched `(fallback_uid, 1000)` pair. + /// - `"0"` (root): returns `(0, 0)`. The resulting `keep-id:uid=0,gid=0` + /// directive is harmless on rootless Podman because the host caller uid + /// already maps to container uid 0 by default; the userns just makes + /// the mapping explicit. + /// - Non-numeric (e.g. `"sandbox"`): returns the community-sandbox + /// fallback. Image authors using name-style USER are expected to align + /// the numeric uid with the community convention. pub async fn image_user(&self, image_ref: &str) -> Result<(u32, u32), PodmanApiError> { let path = format!("/libpod/images/{}/json", url_encode(image_ref)); let v: Value = self @@ -594,13 +613,12 @@ impl PodmanClient { .and_then(|u| u.as_str()) .unwrap_or(""); if user.is_empty() { - return Ok((1_000_660_000, 1_000_660_000)); + return Ok((COMMUNITY_SANDBOX_UID, COMMUNITY_SANDBOX_UID)); } let parts: Vec<&str> = user.split(':').collect(); - let uid: u32 = parts - .first() - .and_then(|s| s.parse().ok()) - .unwrap_or(1_000_660_000); + let Some(uid) = parts.first().and_then(|s| s.parse::().ok()) else { + return Ok((COMMUNITY_SANDBOX_UID, COMMUNITY_SANDBOX_UID)); + }; let gid: u32 = parts.get(1).and_then(|s| s.parse().ok()).unwrap_or(uid); Ok((uid, gid)) } diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 74c9bcc2a..0b0b07e41 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -3,6 +3,7 @@ //! Container spec construction for the Podman driver. +use crate::client::COMMUNITY_SANDBOX_UID; use crate::config::PodmanComputeConfig; use openshell_core::gpu::cdi_gpu_device_ids; use openshell_core::proto::compute::v1::DriverSandbox; @@ -605,7 +606,8 @@ pub fn build_container_spec( }); } if !spec.volumes.is_empty() { - let (uid, gid) = image_sandbox_user.unwrap_or((1_000_660_000, 1_000_660_000)); + let (uid, gid) = + image_sandbox_user.unwrap_or((COMMUNITY_SANDBOX_UID, COMMUNITY_SANDBOX_UID)); container_spec.userns = Some(UserNamespace { nsmode: "keep-id".into(), value: format!("uid={uid},gid={gid}"), @@ -1219,6 +1221,15 @@ mod tests { .unwrap(), "/container/a" ); + let opts0: Vec<&str> = bind_mounts[0] + .get("options") + .and_then(|v| v.as_array()) + .unwrap() + .iter() + .filter_map(|o| o.as_str()) + .collect(); + assert!(opts0.contains(&"rbind")); + assert!(!opts0.contains(&"ro")); // first mount is read-write let opts: Vec<&str> = bind_mounts[1] .get("options") .and_then(|v| v.as_array()) From 66c6b984a8a0c24fe03c1768290bfe6309fc6d9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kova=C4=BE?= Date: Thu, 21 May 2026 17:52:04 +0200 Subject: [PATCH 7/8] feat(drivers): docker -v passthrough + vm bind reject for --volume Extend DockerDriver's build_binds to append user-declared bind volumes from sandbox.spec.volumes using host:container[:ro] format. Extend validate_vm_sandbox to reject requests that carry any volumes with Status::invalid_argument. --- crates/openshell-driver-docker/src/lib.rs | 13 ++++++++-- crates/openshell-driver-docker/src/tests.rs | 24 +++++++++++++++++- crates/openshell-driver-vm/src/driver.rs | 28 +++++++++++++++++++++ 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index ef9ff80dc..7c5899860 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -908,7 +908,7 @@ impl ComputeDriver for DockerComputeDriver { } } -fn build_binds(config: &DockerDriverRuntimeConfig) -> Vec { +fn build_binds(config: &DockerDriverRuntimeConfig, sandbox: &DriverSandbox) -> Vec { let mut binds = vec![format!( "{}:{}:ro,z", config.supervisor_bin.display(), @@ -923,6 +923,15 @@ fn build_binds(config: &DockerDriverRuntimeConfig) -> Vec { )); binds.push(format!("{}:{}:ro,z", tls.key.display(), TLS_KEY_MOUNT_PATH)); } + if let Some(spec) = sandbox.spec.as_ref() { + for v in &spec.volumes { + if v.read_only { + binds.push(format!("{}:{}:ro", v.host_path, v.container_path)); + } else { + binds.push(format!("{}:{}", v.host_path, v.container_path)); + } + } + } binds } @@ -1042,7 +1051,7 @@ fn build_container_create_body( nano_cpus: resource_limits.nano_cpus, memory: resource_limits.memory_bytes, device_requests: docker_gpu_device_requests(spec.gpu, &spec.gpu_device), - binds: Some(build_binds(config)), + binds: Some(build_binds(config, sandbox)), restart_policy: Some(RestartPolicy { name: Some(RestartPolicyNameEnum::UNLESS_STOPPED), maximum_retry_count: None, diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index bf6106a85..072a8bf37 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -423,7 +423,7 @@ fn build_environment_keeps_path_driver_controlled() { #[test] fn build_binds_uses_docker_tls_directory() { - let binds = build_binds(&runtime_config()); + let binds = build_binds(&runtime_config(), &test_sandbox()); let targets = binds .iter() .filter_map(|bind| bind.split(':').nth(1).map(String::from)) @@ -439,6 +439,28 @@ fn build_binds_uses_docker_tls_directory() { ); } +#[test] +fn build_binds_emits_user_volume_entries() { + let mut sandbox = test_sandbox(); + if let Some(spec) = sandbox.spec.as_mut() { + spec.volumes = vec![ + openshell_core::proto::compute::v1::BindVolume { + host_path: "/host/a".into(), + container_path: "/container/a".into(), + read_only: false, + }, + openshell_core::proto::compute::v1::BindVolume { + host_path: "/host/b".into(), + container_path: "/container/b".into(), + read_only: true, + }, + ]; + } + let binds = build_binds(&runtime_config(), &sandbox); + assert!(binds.contains(&"/host/a:/container/a".to_string())); + assert!(binds.contains(&"/host/b:/container/b:ro".to_string())); +} + #[test] fn managed_container_label_filters_include_gateway_namespace() { let filters = diff --git a/crates/openshell-driver-vm/src/driver.rs b/crates/openshell-driver-vm/src/driver.rs index 52a9729f8..bfbef51b8 100644 --- a/crates/openshell-driver-vm/src/driver.rs +++ b/crates/openshell-driver-vm/src/driver.rs @@ -2582,6 +2582,12 @@ fn validate_vm_sandbox(sandbox: &Sandbox, gpu_enabled: bool) -> Result<(), Statu return Err(Status::invalid_argument("gpu_device requires gpu=true")); } + if !spec.volumes.is_empty() { + return Err(Status::invalid_argument( + "bind mounts not supported on vm driver; remove --volume flags or use podman/docker driver", + )); + } + if let Some(template) = spec.template.as_ref() { if !template.agent_socket_path.is_empty() { return Err(Status::failed_precondition( @@ -4567,6 +4573,28 @@ mod tests { .expect("template.resources should be accepted and ignored"); } + #[test] + fn validate_vm_sandbox_rejects_bind_volumes() { + use openshell_core::proto::compute::v1::BindVolume; + + let sandbox = Sandbox { + id: "sandbox-123".to_string(), + spec: Some(SandboxSpec { + volumes: vec![BindVolume { + host_path: "/host/data".into(), + container_path: "/data".into(), + read_only: false, + }], + ..Default::default() + }), + ..Default::default() + }; + let err = validate_vm_sandbox(&sandbox, false) + .expect_err("volumes should be rejected by vm driver"); + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("not supported on vm driver")); + } + #[test] fn validate_vm_sandbox_rejects_path_unsafe_ids() { let mut unsafe_ids = [ From a79fd02127b23064ce5e7acc9eaa77000bef3d1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kova=C4=BE?= Date: Thu, 21 May 2026 17:59:05 +0200 Subject: [PATCH 8/8] docs(volume): e2e tests + podman driver README + user docs for --volume Add two e2e tests for `openshell sandbox create --volume` (rw round-trip and ro write-block) gated behind the new `e2e` Cargo feature. Add bind-volume section to the podman driver README documenting auto userns-remap. Add `--volume` flag documentation and userns-remap note to the user-facing manage-sandboxes page and the compute-runtimes architecture doc. --- architecture/compute-runtimes.md | 18 +++++ crates/openshell-cli/Cargo.toml | 1 + .../tests/sandbox_create_volume_e2e.rs | 81 +++++++++++++++++++ crates/openshell-driver-podman/README.md | 23 ++++++ docs/sandboxes/manage-sandboxes.mdx | 18 +++++ 5 files changed, 141 insertions(+) create mode 100644 crates/openshell-cli/tests/sandbox_create_volume_e2e.rs diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index d79b7366d..478b677ef 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -64,6 +64,24 @@ Driver-controlled environment variables must override sandbox image or template values for sandbox ID, sandbox name, gateway endpoint, relay socket path, TLS paths, and command metadata. +## User Bind Volumes + +Sandboxes accept `--volume :[:ro]` at creation time. The host +path must be absolute and exist; the container path must be absolute. The +optional `:ro` suffix makes the bind read-only. + +On rootless Podman, the driver inspects the sandbox image's `Config.User` +directive and sets the libpod `userns` field to `keep-id` with the image uid. +This maps the container sandbox uid bidirectionally to the host caller's uid so +bind files are mutually readable and writable across the namespace boundary +without manual ownership changes. The `userns` override is applied only when at +least one `--volume` is present; sandboxes without bind volumes continue to use +the default rootless mapping. + +Docker and Kubernetes do not receive automatic userns remapping from the driver. +Docker rootless requires daemon-wide `userns-remap` configuration. Kubernetes +bind volumes follow cluster storage and security context policies. + ## Images The gateway image and Helm chart are built from this repository. Sandbox images diff --git a/crates/openshell-cli/Cargo.toml b/crates/openshell-cli/Cargo.toml index b69a9629b..ab8cd319e 100644 --- a/crates/openshell-cli/Cargo.toml +++ b/crates/openshell-cli/Cargo.toml @@ -87,6 +87,7 @@ workspace = true [features] bundled-z3 = ["openshell-prover/bundled-z3"] dev-settings = ["openshell-core/dev-settings"] +e2e = [] [dev-dependencies] futures = { workspace = true } diff --git a/crates/openshell-cli/tests/sandbox_create_volume_e2e.rs b/crates/openshell-cli/tests/sandbox_create_volume_e2e.rs new file mode 100644 index 000000000..7ac3049db --- /dev/null +++ b/crates/openshell-cli/tests/sandbox_create_volume_e2e.rs @@ -0,0 +1,81 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! E2E tests for `openshell sandbox create --volume`. Gated behind the +//! `e2e` feature because they require a running podman daemon and pull +//! the community python image. + +#![cfg(feature = "e2e")] + +use std::process::Command; +use tempfile::TempDir; + +fn openshell_bin() -> &'static str { + env!("CARGO_BIN_EXE_openshell") +} + +#[test] +fn volume_bind_round_trips() { + let host = TempDir::new().expect("tempdir"); + std::fs::write(host.path().join("marker"), b"hello").expect("write marker"); + let host_path = host.path().to_str().expect("tempdir is utf8").to_string(); + + let out = Command::new(openshell_bin()) + .args([ + "sandbox", + "create", + "--from", + "python", + "--volume", + &format!("{host_path}:/host-bind"), + "--no-tty", + "--no-keep", + "--", + "cat", + "/host-bind/marker", + ]) + .output() + .expect("run openshell"); + + assert!( + out.status.success(), + "openshell exited with {:?}; stderr: {}", + out.status, + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + stdout.contains("hello"), + "expected 'hello' in stdout, got: {stdout}" + ); +} + +#[test] +fn volume_bind_ro_blocks_write() { + let host = TempDir::new().expect("tempdir"); + let host_path = host.path().to_str().expect("tempdir is utf8").to_string(); + + let out = Command::new(openshell_bin()) + .args([ + "sandbox", + "create", + "--from", + "python", + "--volume", + &format!("{host_path}:/host-bind:ro"), + "--no-tty", + "--no-keep", + "--", + "sh", + "-c", + "touch /host-bind/x && echo OK || echo BLOCKED", + ]) + .output() + .expect("run openshell"); + + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + stdout.contains("BLOCKED"), + "expected ro mount to block write; stdout: {stdout}" + ); +} diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index dbf508c03..80a8f37ca 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -325,6 +325,29 @@ matter compared to cluster or rootful runtimes: 8. tmpfs at `/run/netns`: a private tmpfs lets the supervisor create named network namespaces via `ip netns add`. +## Bind Volumes (`--volume`) + +`openshell sandbox create --volume :[:ro]` passes +through to podman's `-v` flag at libpod-spec construction. The host +path must be absolute and exist; the container path must be absolute. +The optional `:ro` suffix makes the bind read-only. + +When any `--volume` is present on rootless podman, the driver: + +1. Inspects the sandbox image's `Config.User` directive via libpod + `/images/{name}/json`. The directive is parsed as `uid` or + `uid:gid`; empty or non-numeric values fall back to the + community-image default of `(1000660000, 1000660000)`. +2. Sets the libpod `userns` field to + `{ nsmode: "keep-id", value: "uid=N,gid=N" }`. This maps the + container sandbox uid bidirectionally to the host caller's uid, + so bind files are mutually readable + writable across the + boundary. + +When no `--volume` is present the driver leaves `userns` unset +(libpod default mapping), preserving prior behaviour for copy-only +sandboxes. + ## Implementation References - Gateway integration: `crates/openshell-server/src/compute/mod.rs` diff --git a/docs/sandboxes/manage-sandboxes.mdx b/docs/sandboxes/manage-sandboxes.mdx index a62f57bf5..d16e4e8a0 100644 --- a/docs/sandboxes/manage-sandboxes.mdx +++ b/docs/sandboxes/manage-sandboxes.mdx @@ -43,6 +43,24 @@ value as both the request and the limit so the scheduler reserves the same amount the sandbox can use. The VM driver currently accepts these flags but does not change VM allocation. +### Bind Volumes + +Mount a host directory into the sandbox with `--volume`: + +```shell +openshell sandbox create --volume /path/on/host:/path/in/sandbox -- claude +openshell sandbox create --volume /path/on/host:/path/in/sandbox:ro -- claude +``` + +The host path must be absolute and exist before the sandbox starts. The optional +`:ro` suffix mounts the directory read-only inside the sandbox. + +On rootless Podman, OpenShell automatically configures user-namespace remapping +so that files in the bind mount are readable and writable by the sandbox process +without manual `chown`. Docker rootless requires daemon-wide `userns-remap` +configuration; without it, bind-mount file ownership may not align with the +sandbox user. + ### GPU Resources To request GPU resources, add `--gpu`: