From 2b58f63003e22504f0d6215dc118bc2ee3cfb68d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kova=C4=BE?= Date: Sun, 26 Jul 2026 14:48:00 +0200 Subject: [PATCH] refactor(cli): reimplement --volume over upstream driver-config mounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upstream's June sync brought a typed, validated driver-config mount system (openshell-core/src/driver_mounts.rs + per-driver mount handling). The fork's --volume predates it and carried a parallel bespoke stack: proto fields, BindVolume messages in two protos, a public->driver mapping in the gateway, and its own bind-mount loop in the podman driver. --volume is now thin CLI sugar that folds each HOST:CONTAINER[:ro] spec into the driver-config envelope as an upstream bind mount. The flag's UX is unchanged. What stays fork-only is the part upstream has no equivalent for: auto userns-remap on rootless podman (image_user() -> keep-id + uid=/gid=), which is why plain `--volume` gives working file ownership here without a manual chown. Honest accounting -- this is NOT a line-count win. Fork delta against the merge base goes 4726/438 -> 4691/444, only ~35 fewer insertions, because the deleted bespoke code is largely replaced by the new sink, the merge function, and their tests. The win is conflict SURFACE: - 4 files leave the fork delta entirely, including driver-docker/src/lib.rs and tests.rs -- the docker driver now has zero fork delta. - proto surface shrinks: openshell.proto 53->48, compute_driver.proto 13->6, and both `volumes = 9003` claims are released to `reserved`. Proto field collisions are this fork's known blocker class. - bind-mount mechanics now live in upstream-owned code instead of a fork loop in container.rs, so upstream churn there stops colliding. Three traps handled deliberately: 1. read_only default flip. Upstream's PodmanDriverMountConfig::Bind declares read_only with #[serde(default = "default_true")], the OPPOSITE of --volume's own default. The sink therefore always emits read_only explicitly and never omits it, or every plain --volume would silently become read-only. Tested in both directions, including when merging into a user-supplied --driver-config-json. 2. userns trigger re-key. The trigger keyed off the now-deleted spec.volumes. It now keys off the resolved driver-config's actual bind-type mount list, reusing what podman_user_mounts already computes. Three tests cover the trigger itself, including a named-volume case that would catch keying off "any mount" instead of "any bind-type mount". Side benefit: a raw --driver-config-json bind mount now gets correct userns-remap too, which the fork-only trigger never gave. 3. VM driver truthfulness. The CLI can't know the active driver, so the mount is emitted under the podman, docker AND vm blocks. Without the vm block, select_driver_config would drop it and --volume would silently do nothing on the VM driver. VmSandboxDriverConfig now carries a `mounts` field and rejects it with the same message the deleted check used, so a VM user gets a hard error rather than silence. volume_spec.rs and driver-podman/src/client.rs are byte-identical to before: the --volume parser (including its host-path-exists pre-flight, which upstream's shared validator deliberately lacks) and image_user() are untouched. Verified: cargo check and clippy --workspace --all-targets with --features openshell-prover/bundled-z3 both exit 0; tests pass in openshell-cli (189), openshell-server (841), driver-podman (15), driver-docker (110+12), driver-vm (110+12); cargo fmt --check and markdown lint clean. Signed-off-by: Jakub Kovaľ --- architecture/compute-runtimes.md | 23 +- crates/openshell-cli/src/main.rs | 10 +- crates/openshell-cli/src/run.rs | 223 ++++++++++++++++-- .../tests/sandbox_create_volume_e2e.rs | 81 ------- .../tests/sandbox_create_volume_flag.rs | 139 ----------- crates/openshell-driver-docker/src/lib.rs | 9 - crates/openshell-driver-docker/src/tests.rs | 23 -- crates/openshell-driver-podman/README.md | 26 +- .../openshell-driver-podman/src/container.rs | 171 ++++++-------- crates/openshell-driver-podman/src/driver.rs | 25 +- crates/openshell-driver-vm/src/driver.rs | 45 ++-- crates/openshell-server/src/compute/mod.rs | 22 +- docs/sandboxes/manage-sandboxes.mdx | 12 + proto/compute_driver.proto | 19 +- proto/openshell.proto | 19 +- 15 files changed, 403 insertions(+), 444 deletions(-) delete mode 100644 crates/openshell-cli/tests/sandbox_create_volume_e2e.rs delete mode 100644 crates/openshell-cli/tests/sandbox_create_volume_flag.rs diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 9585c18580..a0f1284362 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -95,17 +95,32 @@ 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. +`--volume` is CLI sugar over `template.driver_config`: the CLI translates each +flag into a `{"type": "bind", "source", "target", "read_only"}` mount entry +under the active driver's block (the same shape `--driver-config-json` accepts +directly), always emitting `read_only` explicitly. Podman and Docker require +`enable_bind_mounts = true` under their respective `[openshell.drivers.*]` +config table before they will honor any bind mount, whether it arrived via +`--volume` or a raw `--driver-config-json` bind entry. + 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. +without manual ownership changes. The `userns` override is applied only when +the resolved driver-config carries at least one bind-type mount (whether it +arrived via `--volume` or a raw `--driver-config-json` bind entry); sandboxes +without bind mounts 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. +bind volumes follow cluster storage and security context policies. The VM +driver has no bind-mount support at all. Because driver selection happens +gateway-side, the CLI populates the `vm` driver-config block the same as +`podman`/`docker`; the VM driver's own config validation then rejects a +non-empty `mounts` list with `bind mounts not supported on vm driver`, so a +`--volume` flag against a VM-driver gateway fails loudly at sandbox-create +time rather than being silently dropped. ## Images diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index 57fc0d2e00..8deea73f99 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -1304,12 +1304,18 @@ enum SandboxCommands { /// Host path must be absolute and exist. Container path must be /// absolute. The optional `:ro` suffix makes the mount read-only. /// + /// Sugar over `--driver-config-json`: translates to a bind mount + /// entry under the active driver's block. Requires + /// `enable_bind_mounts = true` under `[openshell.drivers.podman]` or + /// `[openshell.drivers.docker]` on the gateway. + /// /// On rootless podman, the driver auto-applies /// `--userns=keep-id:uid=,gid=` - /// when any `--volume` is set, so bind file ownership maps + /// when any bind mount is present, so bind file ownership maps /// bidirectionally between host and container. /// - /// Not supported on the vm driver. + /// Not supported on the vm driver: sandbox creation fails with + /// "bind mounts not supported on vm driver". #[arg(long = "volume", help_heading = "MOUNT FLAGS")] volumes: Vec, diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 455a2e445a..9a6215bcd3 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -36,7 +36,7 @@ use openshell_core::progress::{ use openshell_core::proto::ProviderProfileCategory; use openshell_core::proto::{ ApproveAllDraftChunksRequest, ApproveDraftChunkRequest, AttachSandboxProviderRequest, - BindVolume, ClearDraftChunksRequest, ConfigureProviderRefreshRequest, CreateProviderRequest, + ClearDraftChunksRequest, ConfigureProviderRefreshRequest, CreateProviderRequest, CreateSandboxRequest, CreateSshSessionRequest, DeleteProviderProfileRequest, DeleteProviderRefreshRequest, DeleteProviderRequest, DeleteSandboxRequest, DeleteServiceRequest, DetachSandboxProviderRequest, ExecSandboxRequest, ExposeServiceRequest, @@ -1665,6 +1665,82 @@ fn json_to_protobuf_value(value: serde_json::Value) -> Result, + volumes: &[BindVolumeSpec], +) -> Result> { + if volumes.is_empty() { + return Ok(driver_config); + } + + let mount_entries: Vec = volumes + .iter() + .map(|v| { + serde_json::json!({ + "type": "bind", + "source": v.host, + "target": v.container, + "read_only": v.read_only, + }) + }) + .collect(); + + let mut root: serde_json::Map = driver_config + .as_ref() + .map(openshell_core::proto_struct::struct_to_json_object) + .unwrap_or_default(); + + for driver_name in ["podman", "docker", "vm"] { + let driver_block = root + .entry(driver_name.to_string()) + .or_insert_with(|| serde_json::json!({})); + let Some(driver_obj) = driver_block.as_object_mut() else { + continue; + }; + let mounts = driver_obj + .entry("mounts") + .or_insert_with(|| serde_json::json!([])); + let Some(mounts_arr) = mounts.as_array_mut() else { + continue; + }; + mounts_arr.extend(mount_entries.clone()); + } + + Ok(Some(prost_types::Struct { + fields: root + .into_iter() + .map(|(key, value)| json_to_protobuf_value(value).map(|value| (key, value))) + .collect::>()?, + })) +} + fn validate_cpu_quantity(value: &str) -> Result { let value = value.trim(); if value.is_empty() { @@ -1834,6 +1910,7 @@ pub async fn sandbox_create( let driver_config = driver_config_json .map(parse_driver_config_json) .transpose()?; + let driver_config = add_volume_mounts_to_driver_config(driver_config, volumes)?; let template = if image.is_some() || resource_limits.is_some() || driver_config.is_some() { Some(SandboxTemplate { @@ -1846,15 +1923,6 @@ 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, @@ -1862,7 +1930,6 @@ pub async fn sandbox_create( policy, providers: configured_providers, template, - volumes: proto_volumes, log_level: log_level.unwrap_or_default().to_string(), }), name: name.unwrap_or_default().to_string(), @@ -7742,11 +7809,12 @@ fn format_timestamp_ms(ms: i64) -> String { #[cfg(test)] mod tests { use super::{ - ProvisioningStep, TlsOptions, build_sandbox_resource_limits, - dockerfile_sources_supported_for_gateway, format_endpoint, format_gateway_select_header, - format_gateway_select_items, format_provider_attachment_table, gateway_add, - gateway_auth_label, gateway_env_override_warning, gateway_select_with, gateway_to_json, - gateway_type_label, git_sync_files, http_health_check, import_local_package_mtls_bundle, + ProvisioningStep, TlsOptions, add_volume_mounts_to_driver_config, + build_sandbox_resource_limits, dockerfile_sources_supported_for_gateway, format_endpoint, + format_gateway_select_header, format_gateway_select_items, + format_provider_attachment_table, gateway_add, gateway_auth_label, + gateway_env_override_warning, gateway_select_with, gateway_to_json, gateway_type_label, + git_sync_files, http_health_check, import_local_package_mtls_bundle, inferred_provider_type, mtls_certs_exist_for_gateway, package_managed_tls_dirs, parse_cli_setting_value, parse_credential_expiry_cli_value, parse_credential_expiry_pairs, parse_credential_pairs, parse_driver_config_json, plaintext_gateway_is_remote, @@ -7756,6 +7824,7 @@ mod tests { service_expose_status_error, service_url_for_gateway, }; use crate::TEST_ENV_LOCK; + use crate::volume_spec::BindVolumeSpec; use hyper::StatusCode; use std::fs; use std::io::{Read, Write}; @@ -8235,6 +8304,128 @@ mod tests { ); } + /// Navigate to `driver_config.fields[driver_name].mounts` as a JSON array, + /// for asserting on the mount entries `--volume` produces. + fn driver_mounts_array( + config: &prost_types::Struct, + driver_name: &str, + ) -> Vec { + let driver_value = config + .fields + .get(driver_name) + .unwrap_or_else(|| panic!("expected a '{driver_name}' block")); + let json = openshell_core::proto_struct::value_to_json(driver_value); + json.get("mounts") + .and_then(|v| v.as_array()) + .unwrap_or_else(|| panic!("expected '{driver_name}.mounts' to be an array")) + .clone() + } + + #[test] + fn add_volume_mounts_emits_explicit_read_only_false_for_plain_volume() { + // The read_only default-flip trap: upstream's PodmanDriverMountConfig::Bind + // defaults read_only to true when the field is ABSENT. A plain `--volume` + // (no `:ro`) must therefore emit `"read_only": false` explicitly, never + // omit the key, or every read-write mount would silently become read-only. + let volumes = [BindVolumeSpec { + host: "/host/rw".to_string(), + container: "/sandbox/rw".to_string(), + read_only: false, + }]; + let config = add_volume_mounts_to_driver_config(None, &volumes) + .expect("merge should succeed") + .expect("driver_config should be Some when volumes are present"); + + for driver_name in ["podman", "docker", "vm"] { + let mounts = driver_mounts_array(&config, driver_name); + assert_eq!(mounts.len(), 1, "{driver_name} should have one mount"); + assert_eq!(mounts[0]["type"], "bind"); + assert_eq!(mounts[0]["source"], "/host/rw"); + assert_eq!(mounts[0]["target"], "/sandbox/rw"); + assert_eq!( + mounts[0]["read_only"], false, + "{driver_name}: read_only must be explicit false, not omitted" + ); + } + } + + #[test] + fn add_volume_mounts_emits_explicit_read_only_true_for_ro_volume() { + let volumes = [BindVolumeSpec { + host: "/host/ro".to_string(), + container: "/sandbox/ro".to_string(), + read_only: true, + }]; + let config = add_volume_mounts_to_driver_config(None, &volumes) + .expect("merge should succeed") + .expect("driver_config should be Some when volumes are present"); + + for driver_name in ["podman", "docker", "vm"] { + let mounts = driver_mounts_array(&config, driver_name); + assert_eq!(mounts[0]["read_only"], true); + } + } + + #[test] + fn add_volume_mounts_returns_none_when_no_volumes_and_no_driver_config() { + let config = add_volume_mounts_to_driver_config(None, &[]).expect("merge should succeed"); + assert!(config.is_none()); + } + + #[test] + fn add_volume_mounts_appends_to_existing_driver_config_json_mounts() { + // A user combining --driver-config-json (with its own podman mounts) + // and --volume should get both sets of mounts, not one clobbering + // the other. + let existing = parse_driver_config_json( + r#"{"podman":{"mounts":[{"type":"tmpfs","target":"/sandbox/cache"}]}}"#, + ) + .expect("driver-config-json should parse"); + + let volumes = [BindVolumeSpec { + host: "/host/data".to_string(), + container: "/sandbox/data".to_string(), + read_only: false, + }]; + let config = add_volume_mounts_to_driver_config(Some(existing), &volumes) + .expect("merge should succeed") + .expect("driver_config should be Some"); + + let mounts = driver_mounts_array(&config, "podman"); + assert_eq!( + mounts.len(), + 2, + "existing tmpfs mount plus the new bind mount" + ); + assert!(mounts.iter().any(|m| m["type"] == "tmpfs")); + assert!( + mounts + .iter() + .any(|m| m["type"] == "bind" && m["source"] == "/host/data") + ); + } + + #[test] + fn add_volume_mounts_populates_vm_block_so_vm_driver_rejects_loudly() { + // The CLI cannot know which compute driver is active (that's a + // gateway-side setting), so --volume must populate the `vm` block + // too: VmSandboxDriverConfig explicitly rejects a non-empty `mounts` + // list, turning what would otherwise be a silent no-op (an + // unrecognized `vm` key that `select_driver_config` just drops) into + // a clear "not supported on vm driver" error at sandbox-create time. + let volumes = [BindVolumeSpec { + host: "/host/data".to_string(), + container: "/sandbox/data".to_string(), + read_only: false, + }]; + let config = add_volume_mounts_to_driver_config(None, &volumes) + .expect("merge should succeed") + .expect("driver_config should be Some when volumes are present"); + + let mounts = driver_mounts_array(&config, "vm"); + assert_eq!(mounts.len(), 1, "vm block should carry the bind mount too"); + } + #[test] fn inferred_provider_type_returns_type_for_known_command() { let result = inferred_provider_type(&["claude".to_string(), "--help".to_string()]); diff --git a/crates/openshell-cli/tests/sandbox_create_volume_e2e.rs b/crates/openshell-cli/tests/sandbox_create_volume_e2e.rs deleted file mode 100644 index 7ac3049db8..0000000000 --- a/crates/openshell-cli/tests/sandbox_create_volume_e2e.rs +++ /dev/null @@ -1,81 +0,0 @@ -// 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 deleted file mode 100644 index 3bae5140ff..0000000000 --- a/crates/openshell-cli/tests/sandbox_create_volume_flag.rs +++ /dev/null @@ -1,139 +0,0 @@ -// 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 4c9cbf9d3d..963e7a0f74 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -1989,15 +1989,6 @@ fn build_binds( SANDBOX_TOKEN_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)); - } - } - } Ok(binds) } diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index b215afcb2e..d5132fe1a8 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -43,7 +43,6 @@ fn test_sandbox() -> DriverSandbox { }), gpu: false, sandbox_token: String::new(), - volumes: vec![], }), status: None, } @@ -985,28 +984,6 @@ fn build_environment_uses_token_file_without_raw_token_env() { ))); } -#[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(&sandbox, &runtime_config()).unwrap(); - 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 5adbf9f012..1ce9b04b50 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -371,12 +371,24 @@ matter compared to cluster or rootful runtimes: ## 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: +`openshell sandbox create --volume :[:ro]` is CLI sugar: +`openshell-cli` translates each flag into a `{"type": "bind", "source", +"target", "read_only"}` entry under `template.driver_config.podman.mounts` — +the same shape `--driver-config-json` accepts directly, and `read_only` is +always emitted explicitly (upstream's `PodmanDriverMountConfig::Bind` defaults +`read_only` to `true` when the field is absent, the opposite of `--volume`'s +own no-`:ro` default). The host path must be absolute and exist; the +container path must be absolute. The optional `:ro` suffix makes the bind +read-only. `enable_bind_mounts = true` must be set under +`[openshell.drivers.podman]`, same as any other driver-config bind mount. + +The mount itself is emitted by `podman_user_mounts` (see "Driver Config +Mounts" above) — there is no separate `--volume`-specific mount-building path +in this crate. + +On rootless podman, whenever the resolved driver-config carries at least one +bind-type mount — however it arrived, `--volume` or a raw `--driver-config-json` +bind entry — the driver: 1. Inspects the sandbox image's `Config.User` directive via libpod `/images/{name}/json`. The directive is parsed as `uid` or @@ -388,7 +400,7 @@ When any `--volume` is present on rootless podman, the driver: so bind files are mutually readable + writable across the boundary. -When no `--volume` is present the driver leaves `userns` unset +When no bind-type mount is present the driver leaves `userns` unset (libpod default mapping), preserving prior behaviour for copy-only sandboxes. diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 8648efa7d8..cc657f9ded 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -647,6 +647,21 @@ fn podman_user_mounts( Ok(result) } +/// Returns true if the sandbox's resolved podman driver-config carries at +/// least one bind-type mount. Used by the driver to decide whether the +/// `PodmanClient::image_user` image-inspect round-trip is worth performing +/// before building the container spec (the round-trip is only needed to pick +/// the userns-remap uid/gid). +/// +/// Malformed driver-config is reported as `false` here rather than +/// propagated: the authoritative validation error still surfaces from +/// `podman_user_mounts` when the container spec is built, this is purely an +/// early-exit optimization. +pub fn podman_config_has_bind_mount(sandbox: &DriverSandbox, enable_bind_mounts: bool) -> bool { + podman_user_mounts(sandbox, enable_bind_mounts) + .is_ok_and(|mounts| mounts.mounts.iter().any(|m| m.kind == "bind")) +} + fn podman_driver_config( template: &DriverSandboxTemplate, enable_bind_mounts: bool, @@ -829,6 +844,12 @@ pub fn build_container_spec_with_token_and_gpu_default( let resource_limits = build_resource_limits(sandbox, config); let user_mounts = podman_user_mounts(sandbox, config.enable_bind_mounts) .map_err(ComputeDriverError::InvalidArgument)?; + // Captured before `user_mounts.mounts` is moved into the container spec's + // mount list below — this is the re-keyed userns-remap trigger: it used to + // fire on the fork's own (now-removed) `spec.volumes`, and now fires on + // any bind-type mount in the resolved driver-config, however it arrived + // (the `--volume` CLI sugar or a raw `--driver-config-json` bind mount). + let has_bind_mount = user_mounts.mounts.iter().any(|m| m.kind == "bind"); let devices = build_devices(sandbox, selected_default_device)?; // Network configuration -- always bridge mode. @@ -1049,28 +1070,20 @@ pub fn build_container_spec_with_token_and_gpu_default( 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}"), - }); - } + // Auto userns-remap on rootless podman: when the resolved driver-config + // carries at least one bind-type mount (already folded into + // `container_spec.mounts` above via `user_mounts`), set + // `--userns=keep-id:uid=,gid=` so + // bind-mount file ownership maps bidirectionally between host and + // container. `image_sandbox_user` is resolved by the caller (driver.rs) + // from the image's `Config.User` directive. + if has_bind_mount { + 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}"), + }); } Ok(serde_json::to_value(container_spec).expect("ContainerSpec serialization cannot fail")) @@ -2247,80 +2260,26 @@ mod tests { } #[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")); - } + fn build_container_spec_sets_userns_keep_id_when_bind_mount_present() { + use openshell_core::proto::compute::v1::{DriverSandboxSpec, DriverSandboxTemplate}; - #[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, - }], + sandbox.spec = Some(DriverSandboxSpec { + template: Some(DriverSandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "mounts": [{ + "type": "bind", + "source": "/host", + "target": "/sandbox/container", + "read_only": false + }] + }))), + ..Default::default() + }), ..Default::default() }); - let cfg = test_config(); + let mut cfg = test_config(); + cfg.enable_bind_mounts = true; 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!( @@ -2334,10 +2293,36 @@ mod tests { } #[test] - fn build_container_spec_omits_userns_when_no_volumes() { + fn build_container_spec_omits_userns_when_no_bind_mount() { 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()); } + + #[test] + fn build_container_spec_omits_userns_for_non_bind_mounts() { + // A driver-config mount that ISN'T bind-type (e.g. a named volume) + // must not trigger the userns-remap — only host-path bind mounts + // need the uid/gid ownership fixup. + use openshell_core::proto::compute::v1::{DriverSandboxSpec, DriverSandboxTemplate}; + + let mut sandbox = test_sandbox("id-1", "name-1"); + sandbox.spec = Some(DriverSandboxSpec { + template: Some(DriverSandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "mounts": [{ + "type": "volume", + "source": "work-nfs", + "target": "/sandbox/work" + }] + }))), + ..Default::default() + }), + ..Default::default() + }); + 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 3d2d1ea61d..802a408f0f 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -507,19 +507,20 @@ impl PodmanComputeDriver { return Err(e); } }; - 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); - match self.client.image_user(image_ref).await { - Ok(u) => Some(u), - Err(e) => { - let _ = self.client.remove_volume(&vol_name).await; - cleanup_sandbox_token_file(&sandbox.id); - return Err(e.into()); + let image_sandbox_user = + if container::podman_config_has_bind_mount(sandbox, self.config.enable_bind_mounts) { + let image_ref = container::resolve_image(sandbox, &self.config); + match self.client.image_user(image_ref).await { + Ok(u) => Some(u), + Err(e) => { + let _ = self.client.remove_volume(&vol_name).await; + cleanup_sandbox_token_file(&sandbox.id); + return Err(e.into()); + } } - } - } else { - None - }; + } else { + None + }; let spec = match container::build_container_spec_with_token_and_gpu_default( sandbox, &self.config, diff --git a/crates/openshell-driver-vm/src/driver.rs b/crates/openshell-driver-vm/src/driver.rs index edc11e6e6a..b4ac77faf6 100644 --- a/crates/openshell-driver-vm/src/driver.rs +++ b/crates/openshell-driver-vm/src/driver.rs @@ -87,6 +87,13 @@ struct VmSandboxDriverConfig { deserialize_with = "deserialize_optional_non_empty_string_list" )] gpu_device_ids: Option>, + /// Accepted only so a `--volume`-derived or hand-authored + /// `--driver-config-json` bind mount produces the clear + /// "not supported on vm driver" error below, instead of a bare + /// `deny_unknown_fields` "unknown field `mounts`" — the VM driver has no + /// mount-handling of any kind, so any non-empty value here is rejected. + #[serde(default)] + mounts: Vec, } impl VmSandboxDriverConfig { @@ -107,8 +114,15 @@ impl VmSandboxDriverConfig { return Ok(Self::default()); }; - serde_json::from_value(struct_to_json_value(config)) - .map_err(|err| format!("invalid vm driver_config: {err}")) + let parsed: Self = serde_json::from_value(struct_to_json_value(config)) + .map_err(|err| format!("invalid vm driver_config: {err}"))?; + if !parsed.mounts.is_empty() { + return Err( + "bind mounts not supported on vm driver; remove --volume flags or use podman/docker driver" + .to_string(), + ); + } + Ok(parsed) } } @@ -3113,11 +3127,6 @@ fn validate_vm_gpu_request(sandbox: &Sandbox, gpu_enabled: bool) -> Result<(), S )); } - 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", - )); - } Ok(()) } @@ -5367,23 +5376,25 @@ mod tests { } #[test] - fn validate_vm_sandbox_rejects_bind_volumes() { - use openshell_core::proto::compute::v1::BindVolume; - + fn validate_vm_sandbox_rejects_bind_mounts() { + // Replaces the coverage the deleted `spec.volumes`-based rejection + // gave: a bind-type mount reaching the vm driver via + // `template.driver_config.vm.mounts` (however it got there -- + // `--volume` CLI sugar or a raw `--driver-config-json`) must produce + // the same clear error the old check gave, not a bare + // `deny_unknown_fields` "unknown field `mounts`". 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, - }], + template: Some(SandboxTemplate { + driver_config: Some(list_string_driver_config("mounts", &["bind"])), + ..Default::default() + }), ..Default::default() }), ..Default::default() }; - let err = validate_vm_sandbox(&sandbox, false) - .expect_err("volumes should be rejected by vm driver"); + let err = validate_vm_sandbox(&sandbox, false).expect_err("bind mounts should be rejected"); assert_eq!(err.code(), Code::InvalidArgument); assert!(err.message().contains("not supported on vm driver")); } diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index 09d6f342b1..8c35b3c296 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -18,13 +18,12 @@ use crate::tracing_bus::TracingLogBus; use futures::{Stream, StreamExt}; use openshell_core::ComputeDriverKind; use openshell_core::proto::compute::v1::{ - BindVolume as DriverBindVolume, CreateSandboxRequest, DeleteSandboxRequest, DriverCondition, - DriverPlatformEvent, DriverResourceRequirements, DriverSandbox, DriverSandboxSpec, - DriverSandboxStatus, DriverSandboxTemplate, GetCapabilitiesRequest, GetSandboxRequest, - ListSandboxesRequest, StopSandboxRequest as DriverStopSandboxRequest, - ValidateSandboxCreateRequest, WatchSandboxesEvent, WatchSandboxesRequest, - compute_driver_client::ComputeDriverClient, compute_driver_server::ComputeDriver, - watch_sandboxes_event, + CreateSandboxRequest, DeleteSandboxRequest, DriverCondition, DriverPlatformEvent, + DriverResourceRequirements, DriverSandbox, DriverSandboxSpec, DriverSandboxStatus, + DriverSandboxTemplate, GetCapabilitiesRequest, GetSandboxRequest, ListSandboxesRequest, + StopSandboxRequest as DriverStopSandboxRequest, ValidateSandboxCreateRequest, + WatchSandboxesEvent, WatchSandboxesRequest, compute_driver_client::ComputeDriverClient, + compute_driver_server::ComputeDriver, watch_sandboxes_event, }; use openshell_core::proto::{ PlatformEvent, Sandbox, SandboxCondition, SandboxPhase, SandboxSpec, SandboxStatus, @@ -1620,15 +1619,6 @@ fn driver_sandbox_spec_from_public( .transpose()?, gpu: spec.gpu, sandbox_token: String::new(), - 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 8bb998685f..3903f1417a 100644 --- a/docs/sandboxes/manage-sandboxes.mdx +++ b/docs/sandboxes/manage-sandboxes.mdx @@ -61,6 +61,7 @@ openshell sandbox create \ Use this only for driver-specific fields that do not have a stable CLI flag. Prefer stable flags such as `--cpu`, `--memory`, and `--gpu` when they cover the same behavior. + ### Bind Volumes Mount a host directory into the sandbox with `--volume`: @@ -73,12 +74,23 @@ 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. +`--volume` is CLI sugar over `template.driver_config`: each flag becomes a bind +mount entry under the active compute driver's block, equivalent to passing +`--driver-config-json '{"podman":{"mounts":[{"type":"bind","source":"...","target":"...","read_only":false}]}}'` +directly. The gateway must have `enable_bind_mounts = true` set under +`[openshell.drivers.podman]` or `[openshell.drivers.docker]` (see +[gateway configuration](/reference/gateway-config)), or sandbox creation fails +with `bind mounts require enable_bind_mounts = true`. + 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. +Not supported on the VM driver — sandbox creation fails with +`bind mounts not supported on vm driver`. + ### GPU Resources To request GPU resources, add `--gpu`: diff --git a/proto/compute_driver.proto b/proto/compute_driver.proto index 1f5115eabf..515da9c1c1 100644 --- a/proto/compute_driver.proto +++ b/proto/compute_driver.proto @@ -94,19 +94,12 @@ message DriverSandboxSpec { // ServiceAccount token bootstrap instead). Never echoed to the public // Sandbox proto. string sandbox_token = 11; - // Bind-mount entries: live host-path passthrough into the sandbox. - // Fork-added field; uses the 9000+ range reserved for openlock additions - // (mirrors openshell.v1.SandboxSpec.volumes) to stay clear of upstream's - // sequential numbering — upstream's sandbox_token took the old field 11. - repeated BindVolume volumes = 9003; -} - -// 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; + // Field 9003 was `volumes` (mirrored openshell.v1.SandboxSpec.volumes, + // fork-added `BindVolume` passthrough). Removed: bind mounts now flow + // through the upstream `DriverSandboxTemplate.driver_config` envelope. + // Reserved rather than reused per fork field-number policy. + reserved 9003; + reserved "volumes"; } // Driver-owned runtime template consumed by the compute platform. diff --git a/proto/openshell.proto b/proto/openshell.proto index 15c0ac380d..57a8eae3ae 100644 --- a/proto/openshell.proto +++ b/proto/openshell.proto @@ -339,18 +339,13 @@ message SandboxSpec { // managed fleet-wide. reserved 11; reserved "proposal_approval_mode"; - // Bind-mount entries: live host-path passthrough into the sandbox. - // Fork-added field; uses the 9000+ range reserved for openlock additions - // to stay clear of upstream's sequential numbering. - repeated BindVolume volumes = 9003; -} - -// 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; + // Field 9003 was `volumes` (fork-added `BindVolume` passthrough for the + // `--volume HOST:CONTAINER[:ro]` CLI flag). Removed: bind mounts now flow + // through the upstream `SandboxTemplate.driver_config` envelope (driver + // -config-json), which already supports typed bind/volume/tmpfs/image + // mounts. Reserved rather than reused per fork field-number policy. + reserved 9003; + reserved "volumes"; } // Public sandbox template mapped onto compute-driver template inputs.