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/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/main.rs b/crates/openshell-cli/src/main.rs index 3a8c344d3..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 { @@ -1126,6 +1127,9 @@ enum DoctorCommands { } #[derive(Subcommand, Debug)] +// 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")] @@ -1234,6 +1238,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 +2502,7 @@ async fn main() -> Result<()> { auto_providers, no_auto_providers, labels, + volumes, command, } => { // Resolve --tty / --no-tty into an Option override. @@ -2528,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); @@ -2551,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/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_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-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-cli/tests/sandbox_create_volume_flag.rs b/crates/openshell-cli/tests/sandbox_create_volume_flag.rs new file mode 100644 index 000000000..3bae5140f --- /dev/null +++ b/crates/openshell-cli/tests/sandbox_create_volume_flag.rs @@ -0,0 +1,139 @@ +// 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. +//! +//! # 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; + +/// 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) + .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 by clap. +/// +/// 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) + .args([ + "sandbox", + "create", + "--from", + "python", + "--volume", + "/host:/container", + ]) + .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}")); + + 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), + ); +} + +/// 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("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}")); + + 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), + ); +} + +/// 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("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}")); + + 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), + ); +} 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 2ac2da1ee..072a8bf37 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, } @@ -422,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)) @@ -438,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-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/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 834fb21a2..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); @@ -580,6 +586,43 @@ impl PodmanClient { Ok(()) } + /// Inspect an image and return its USER directive parsed as (uid, gid). + /// + /// 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 + .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((COMMUNITY_SANDBOX_UID, COMMUNITY_SANDBOX_UID)); + } + let parts: Vec<&str> = user.split(':').collect(); + 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)) + } + // ── 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..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; @@ -119,6 +120,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 +203,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 +375,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 +396,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 +588,33 @@ 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((COMMUNITY_SANDBOX_UID, COMMUNITY_SANDBOX_UID)); + 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 +728,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 +763,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 +779,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 +798,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 +810,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 +858,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 +873,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 +888,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 +925,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 +973,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 +1003,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 +1062,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 +1099,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 +1160,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 +1177,99 @@ 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 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()) + .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(_)) => { 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 = [ 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/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`: 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.