From d4edcd970786b495ab136bc3c166ed66a3979599 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 29 Sep 2026 22:12:32 -0700 Subject: [PATCH 01/16] test(oci): share OCI image checks across Docker and Podman Add an oci-image feature testsuite that runs against installed artifacts. It covers custom WORKDIR placement, image content and ownership, workspace writes from the main process and exec, file transfer including directory-merge upload, OCI user identity, the managed /sandbox fallback, and rejection of an unwritable WORKDIR. CI runs the suite on the Docker rootful, Podman rootful, and Podman rootless tmachine guests. Replace the Docker-only custom_image e2e with the shared suite. Signed-off-by: Matthew Grossman Signed-off-by: Evan Lezar --- .github/workflows/branch-e2e.yml | 5 +- .github/workflows/release-dev.yml | 5 +- .github/workflows/release-tag.yml | 5 +- TESTING.md | 19 +- e2e/rust/Cargo.toml | 5 - e2e/rust/tests/custom_image.rs | 253 ------------ .../ansible/playbooks/features/oci-image.yaml | 118 ++++++ tests/artifacts.nix | 11 + tests/config.nix | 7 + tests/suites/features/Cargo.lock | 9 + tests/suites/features/Cargo.toml | 2 +- tests/suites/features/oci-image/Cargo.toml | 12 + .../features/oci-image/tests/oci_image.rs | 386 ++++++++++++++++++ 13 files changed, 574 insertions(+), 263 deletions(-) delete mode 100644 e2e/rust/tests/custom_image.rs create mode 100644 tests/ansible/playbooks/features/oci-image.yaml create mode 100644 tests/suites/features/oci-image/Cargo.toml create mode 100644 tests/suites/features/oci-image/tests/oci_image.rs diff --git a/.github/workflows/branch-e2e.yml b/.github/workflows/branch-e2e.yml index d46f3fb31f..e171d2cc23 100644 --- a/.github/workflows/branch-e2e.yml +++ b/.github/workflows/branch-e2e.yml @@ -275,7 +275,10 @@ jobs: test-matrix: >- [ {"environment":"fedora-podman-rootful","installer":"rpm","testsuite":"provider-refresh"}, - {"environment":"fedora-podman-rootless","installer":"rpm","testsuite":"provider-refresh"} + {"environment":"fedora-podman-rootless","installer":"rpm","testsuite":"provider-refresh"}, + {"environment":"ubuntu-docker-rootful","installer":"deb","testsuite":"oci-image"}, + {"environment":"fedora-podman-rootful","installer":"rpm","testsuite":"oci-image"}, + {"environment":"fedora-podman-rootless","installer":"rpm","testsuite":"oci-image"} ] # Run driver-specific integration tests: diff --git a/.github/workflows/release-dev.yml b/.github/workflows/release-dev.yml index 0da5d1e186..f9b14bb2e7 100644 --- a/.github/workflows/release-dev.yml +++ b/.github/workflows/release-dev.yml @@ -145,7 +145,10 @@ jobs: test-matrix: >- [ {"environment":"fedora-podman-rootful","installer":"rpm","testsuite":"provider-refresh"}, - {"environment":"fedora-podman-rootless","installer":"rpm","testsuite":"provider-refresh"} + {"environment":"fedora-podman-rootless","installer":"rpm","testsuite":"provider-refresh"}, + {"environment":"ubuntu-docker-rootful","installer":"deb","testsuite":"oci-image"}, + {"environment":"fedora-podman-rootful","installer":"rpm","testsuite":"oci-image"}, + {"environment":"fedora-podman-rootless","installer":"rpm","testsuite":"oci-image"} ] docker-e2e: diff --git a/.github/workflows/release-tag.yml b/.github/workflows/release-tag.yml index 7a225270a6..ec1a4feedf 100644 --- a/.github/workflows/release-tag.yml +++ b/.github/workflows/release-tag.yml @@ -196,7 +196,10 @@ jobs: test-matrix: >- [ {"environment":"fedora-podman-rootful","installer":"rpm","testsuite":"provider-refresh"}, - {"environment":"fedora-podman-rootless","installer":"rpm","testsuite":"provider-refresh"} + {"environment":"fedora-podman-rootless","installer":"rpm","testsuite":"provider-refresh"}, + {"environment":"ubuntu-docker-rootful","installer":"deb","testsuite":"oci-image"}, + {"environment":"fedora-podman-rootful","installer":"rpm","testsuite":"oci-image"}, + {"environment":"fedora-podman-rootless","installer":"rpm","testsuite":"oci-image"} ] docker-e2e: diff --git a/TESTING.md b/TESTING.md index a1c2f9476e..fb9d0cfa05 100644 --- a/TESTING.md +++ b/TESTING.md @@ -311,6 +311,23 @@ binary. `tests/artifacts.nix` keeps the follow-up exclusions explicit and uses the same filter for the generated inventory, so excluded binaries cannot appear as false passes or silently re-enter the archive. +The `oci-image` feature testsuite (`tests/suites/features/oci-image`) checks +OCI image identity and working-directory behavior shared by the Docker and +Podman drivers against installed deb packages on Ubuntu and rpm packages on +Fedora. CI runs it on Docker rootful, Podman rootful, and Podman rootless guests: + +```shell +nix run .#tmachine -- test ubuntu-docker-rootful deb oci-image +``` + +Run it against a local gateway by naming the command that builds images into +the gateway's image store: + +```shell +OPENSHELL_TEST_CONTAINER_ENGINE=podman e2e/with-podman-gateway.sh \ + cargo test --manifest-path tests/suites/features/Cargo.toml -p openshell-test-feature-oci-image -- --test-threads 1 +``` + Run the VM-backed Rust CLI e2e suite: ```shell @@ -490,7 +507,7 @@ cargo test --manifest-path e2e/rust/Cargo.toml --features e2e --test sync Run a single Docker-only test directly with cargo: ```shell -cargo test --manifest-path e2e/rust/Cargo.toml --features e2e-docker --test custom_image +cargo test --manifest-path e2e/rust/Cargo.toml --features e2e-docker --test docker_preflight ``` The harness (`e2e/rust/src/harness/`) provides: diff --git a/e2e/rust/Cargo.toml b/e2e/rust/Cargo.toml index 6b1fc73723..fbab884a8d 100644 --- a/e2e/rust/Cargo.toml +++ b/e2e/rust/Cargo.toml @@ -58,11 +58,6 @@ name = "vm_overlay" path = "tests/vm_overlay.rs" required-features = ["e2e-vm"] -[[test]] -name = "custom_image" -path = "tests/custom_image.rs" -required-features = ["e2e-docker"] - [[test]] name = "service_bearer_passthrough" path = "tests/service_bearer_passthrough.rs" diff --git a/e2e/rust/tests/custom_image.rs b/e2e/rust/tests/custom_image.rs deleted file mode 100644 index 96fdd56612..0000000000 --- a/e2e/rust/tests/custom_image.rs +++ /dev/null @@ -1,253 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -#![cfg(feature = "e2e-local-container-driver")] - -//! E2E test: build custom container images and run sandboxes with them. -//! -//! Prerequisites: -//! - A running Docker- or Podman-backed openshell gateway -//! - The matching container runtime running (for image builds) -//! - The `openshell` binary (built automatically from the workspace) - -use std::{fs, io::Write}; - -use openshell_e2e::harness::container::ImageGuard; -use openshell_e2e::harness::output::strip_ansi; -use openshell_e2e::harness::sandbox::SandboxGuard; -use serial_test::serial; - -const DOCKERFILE_CONTENT: &str = r#"FROM public.ecr.aws/docker/library/python:3.13-slim - -# iproute2 is required for sandbox network namespace isolation. -RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ - && rm -rf /var/lib/apt/lists/* - -RUN groupadd -g 1235 appstaff && \ - useradd -m -u 1234 -g appstaff app - -# The final image identity already owns the OCI working directory. Existing -# root-owned content remains root-owned. -WORKDIR /workspace/project -RUN printf root-owned > root-owned.txt && chown app:appstaff . - -# Write a marker file so we can verify this is our custom image. -# Place under /etc (Landlock baseline read-only path) so the sandbox -# can read it when filesystem restrictions are properly enforced. -RUN echo "custom-image-e2e-marker" > /etc/marker.txt - -USER app -CMD ["sleep", "infinity"] -"#; - -const NUMERIC_DOCKERFILE_CONTENT: &str = r#"FROM public.ecr.aws/docker/library/python:3.13-slim - -RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ - && rm -rf /var/lib/apt/lists/* - -USER 2345:2346 -CMD ["sleep", "infinity"] -"#; - -const UNWRITABLE_WORKDIR_DOCKERFILE_CONTENT: &str = r#"FROM public.ecr.aws/docker/library/python:3.13-slim - -RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ - && rm -rf /var/lib/apt/lists/* \ - && groupadd -g 3235 appstaff \ - && useradd -m -u 3234 -g appstaff app - -WORKDIR /workspace/project -USER app -CMD ["sleep", "infinity"] -"#; - -const MARKER: &str = "custom-image-e2e-marker"; - -/// A named OCI user can write through direct and SSH children when the image -/// already grants that authority; existing content retains its ownership. -#[tokio::test] -#[serial(custom_image)] -async fn sandbox_from_custom_image() { - // Step 1: Write a temporary Dockerfile. - let tmpdir = tempfile::tempdir().expect("create tmpdir"); - let dockerfile_path = tmpdir.path().join("Dockerfile"); - { - let mut f = std::fs::File::create(&dockerfile_path).expect("create Dockerfile"); - f.write_all(DOCKERFILE_CONTENT.as_bytes()) - .expect("write Dockerfile"); - } - - // Step 2: Build the image out-of-band and create a sandbox from it. - // `--from` no longer builds local Dockerfiles itself (pre-0.1.0 - // breaking change); tests build explicitly and pass the resulting tag. - let image = ImageGuard::build("custom-dockerfile", &dockerfile_path, tmpdir.path()) - .expect("build custom image with selected container engine"); - let mut guard = SandboxGuard::create_keep_with_args( - &["--from", image.tag(), "--no-tty"], - &[ - "sh", - "-c", - "set -eu; id -u; id -g; test \"$(pwd -P)\" = /workspace/project; \ - test \"$HOME\" = /workspace/project; test \"$(cat root-owned.txt)\" = root-owned; \ - test \"$(stat -c %u:%g .)\" = 1234:1235; \ - test \"$(stat -c %u:%g root-owned.txt)\" = 0:0; \ - touch direct-oci-user-write; cat /etc/marker.txt; echo Ready; sleep infinity", - ], - "Ready", - ) - .await - .expect("sandbox create from custom image"); - - // Step 3: Verify the marker file content appears in the output. - let clean_output = strip_ansi(&guard.create_output); - assert!( - clean_output.contains(MARKER), - "expected marker '{MARKER}' in sandbox output:\n{clean_output}" - ); - assert!( - clean_output.contains("1234") && clean_output.contains("1235"), - "expected named OCI identity 1234:1235 in sandbox output:\n{clean_output}" - ); - - let ssh_output = guard - .exec(&[ - "sh", - "-c", - "set -eu; test \"$(id -u):$(id -g)\" = 1234:1235; \ - test \"$(pwd -P)\" = /workspace/project; test \"$HOME\" = /workspace/project; \ - touch ssh-oci-user-write; echo ssh-write-ok", - ]) - .await - .expect("SSH child should write to prepared workspace"); - assert!( - ssh_output.contains("ssh-write-ok"), - "expected SSH write marker:\n{ssh_output}" - ); - - let transfer_source = tmpdir.path().join("workspace-transfer.txt"); - fs::write(&transfer_source, "workspace-transfer-ok").expect("write transfer fixture"); - guard - .upload_to_workdir( - transfer_source - .to_str() - .expect("transfer fixture path is UTF-8"), - ) - .await - .expect("upload should default to the OCI workspace"); - let transfer_download = tmpdir.path().join("workspace-transfer-downloaded.txt"); - guard - .download( - "workspace-transfer.txt", - transfer_download - .to_str() - .expect("download destination path is UTF-8"), - ) - .await - .expect("download should resolve relative to the OCI workspace"); - assert_eq!( - fs::read_to_string(transfer_download).expect("read downloaded transfer fixture"), - "workspace-transfer-ok" - ); - - guard - .exec(&[ - "sh", - "-c", - "set -eu; mkdir -p merge-upload; \ - printf remote-conflict > merge-upload/conflict.txt; \ - printf remote-preserved > merge-upload/unrelated.txt", - ]) - .await - .expect("seed existing remote upload directory"); - let merge_source = tmpdir.path().join("merge-upload"); - fs::create_dir(&merge_source).expect("create local upload directory"); - fs::write(merge_source.join("conflict.txt"), "local-conflict") - .expect("write conflicting local upload file"); - fs::write(merge_source.join("added.txt"), "local-added") - .expect("write added local upload file"); - guard - .upload_to_workdir(merge_source.to_str().expect("merge upload path is UTF-8")) - .await - .expect("upload should merge into the existing remote directory"); - guard - .exec(&[ - "sh", - "-c", - "set -eu; \ - test \"$(cat merge-upload/conflict.txt)\" = local-conflict; \ - test \"$(cat merge-upload/added.txt)\" = local-added; \ - test \"$(cat merge-upload/unrelated.txt)\" = remote-preserved", - ]) - .await - .expect("upload should overwrite conflicts and preserve unrelated remote files"); - - // Explicit cleanup (also happens in Drop, but explicit is clearer in tests). - guard.cleanup().await; -} - -/// A numeric OCI user/group pair works without passwd or group entries. -/// The image intentionally has no pre-existing `/sandbox`. -#[tokio::test] -#[serial(custom_image)] -async fn sandbox_from_passwd_less_numeric_oci_user() { - let tmpdir = tempfile::tempdir().expect("create tmpdir"); - let dockerfile_path = tmpdir.path().join("Dockerfile"); - { - let mut f = std::fs::File::create(&dockerfile_path).expect("create Dockerfile"); - f.write_all(NUMERIC_DOCKERFILE_CONTENT.as_bytes()) - .expect("write Dockerfile"); - } - - let image = ImageGuard::build("passwd-less-numeric", &dockerfile_path, tmpdir.path()) - .expect("build numeric OCI image with selected container engine"); - let mut guard = SandboxGuard::create(&[ - "--from", - image.tag(), - "--", - "sh", - "-c", - "set -eu; id -u; id -g; test \"$(pwd -P)\" = /sandbox; \ - test \"$HOME\" = /sandbox; touch numeric-oci-user-write", - ]) - .await - .expect("sandbox create from numeric OCI Dockerfile"); - - let clean_output = strip_ansi(&guard.create_output); - assert!( - clean_output.contains("2345") && clean_output.contains("2346"), - "expected numeric OCI identity 2345:2346 in sandbox output:\n{clean_output}" - ); - - guard.cleanup().await; -} - -#[tokio::test] -#[serial(custom_image)] -async fn sandbox_rejects_image_workdir_that_would_require_new_authority() { - let tmpdir = tempfile::tempdir().expect("create tmpdir"); - let dockerfile_path = tmpdir.path().join("Dockerfile"); - fs::write(&dockerfile_path, UNWRITABLE_WORKDIR_DOCKERFILE_CONTENT).expect("write Dockerfile"); - let image = ImageGuard::build("unwritable-workdir", &dockerfile_path, tmpdir.path()) - .expect("build unwritable-workdir image with selected container engine"); - - let result = SandboxGuard::create_keep_with_args( - &["--from", image.tag(), "--no-tty"], - &["sh", "-c", "echo should-not-run"], - "should-not-run", - ) - .await; - let error = match result { - Ok(mut guard) => { - guard.cleanup().await; - panic!("root-owned workdir must not be made writable for the image user"); - } - Err(error) => error, - }; - let message = error.to_string(); - assert!( - (message.contains("WorkspaceValidationFailed") && message.contains("WorkingDir")) - || message.contains("subsystem request failed") - || message.contains("image workspace validation failed"), - "expected rejected image to fail provisioning, got: {message}" - ); -} diff --git a/tests/ansible/playbooks/features/oci-image.yaml b/tests/ansible/playbooks/features/oci-image.yaml new file mode 100644 index 0000000000..7c8b5e62f9 --- /dev/null +++ b/tests/ansible/playbooks/features/oci-image.yaml @@ -0,0 +1,118 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +--- +- name: Run OCI image feature tests + hosts: all + gather_facts: false + vars: + oci_image_test_root: /var/lib/openshell-oci-image/tests + tasks: + - name: Wait for SSH + ansible.builtin.wait_for_connection: + + - name: Detect tmachine container runtime + ansible.builtin.include_role: + name: tmachine_container_runtime + + # Tests build images into the store the gateway reads. Rootful Podman's + # store belongs to root, so the unprivileged test user builds through sudo. + - name: Select the gateway's container engine command + ansible.builtin.set_fact: + oci_image_container_engine: >- + {{ 'docker' if tmachine_container_runtime_name == 'docker' + else 'podman' if tmachine_container_runtime_is_rootless + else 'sudo -n podman' }} + + - name: Create OCI image test directory + become: true + ansible.builtin.file: + path: "{{ oci_image_test_root }}" + state: directory + owner: tmachine + group: tmachine + mode: "0700" + + - name: Extract OCI image test bundle + become: true + ansible.builtin.unarchive: + src: "{{ oci_image_test_bundle }}" + dest: "{{ oci_image_test_root }}" + owner: tmachine + group: tmachine + + - name: Check OCI image nextest archive + ansible.builtin.stat: + path: "{{ oci_image_test_root }}/tests.tar.zst" + register: oci_image_archive + + - name: Require OCI image nextest archive + ansible.builtin.assert: + that: + - oci_image_archive.stat.isreg | default(false) + fail_msg: OCI image test bundle did not contain tests.tar.zst + + - name: Resolve installed OpenShell CLI + ansible.builtin.command: + argv: + - /bin/sh + - -c + - command -v openshell + register: openshell_cli + changed_when: false + + - name: Run OCI image archive + ansible.builtin.command: + argv: + - cargo-nextest + - nextest + - run + - --archive-file + - "{{ oci_image_test_root }}/tests.tar.zst" + - --workspace-remap + - "{{ oci_image_test_root }}" + - --no-capture + # The guest has 4 GiB; keep sandbox creates from competing for it. + - --test-threads + - "1" + - --no-fail-fast + environment: + HOME: /home/tmachine + OPENSHELL_BIN: "{{ openshell_cli.stdout }}" + OPENSHELL_TEST_CONTAINER_ENGINE: "{{ oci_image_container_engine }}" + XDG_RUNTIME_DIR: "/run/user/{{ tmachine_container_runtime_tmachine_uid.stdout }}" + register: oci_image_result + changed_when: false + failed_when: false + + - name: Show OCI image diagnostics + ansible.builtin.debug: + var: oci_image_result + when: oci_image_result.rc != 0 + + - name: Read OpenShell gateway logs + become: true + ansible.builtin.command: + argv: + - journalctl + - --no-pager + - --lines + - "500" + - _SYSTEMD_UNIT=openshell-gateway.service + - "+" + - _SYSTEMD_USER_UNIT=openshell-gateway.service + register: openshell_gateway_logs + changed_when: false + failed_when: false + when: oci_image_result.rc != 0 + + - name: Show OpenShell gateway logs + ansible.builtin.debug: + var: openshell_gateway_logs.stdout_lines + when: oci_image_result.rc != 0 + + - name: Require OCI image success + ansible.builtin.assert: + that: + - oci_image_result.rc == 0 + fail_msg: OCI image feature tests failed diff --git a/tests/artifacts.nix b/tests/artifacts.nix index c86d8f6d30..d63c354095 100644 --- a/tests/artifacts.nix +++ b/tests/artifacts.nix @@ -95,6 +95,14 @@ let target = muslToolchain.target; output = "artifacts/test-archives/${muslToolchain.target}/provider-refresh-keycloak-tests.tar"; }; + ociImageArchive = mkTestArchive { + name = "oci-image"; + workspacePath = "tests/suites/features"; + manifestPath = "tests/suites/features/Cargo.toml"; + package = "openshell-test-feature-oci-image"; + target = muslToolchain.target; + output = "artifacts/test-archives/${muslToolchain.target}/oci-image-tests.tar"; + }; # Follow-up: migrate these wrapper-coupled tests once tmachine provides their # managed-gateway controls, SPIFFE fixtures, caller driver-config setting, @@ -198,6 +206,7 @@ rec { inherit conformanceCliArchive providerRefreshKeycloakArchive + ociImageArchive podmanDriverArchive podmanE2eArchive podmanE2eCiTests @@ -246,12 +255,14 @@ rec { runtimeInputs = [ conformanceCliArchive providerRefreshKeycloakArchive + ociImageArchive podmanDriverArchive podmanE2eArchive ]; text = '' build-openshell-conformance-test-archive build-provider-refresh-keycloak-test-archive + build-oci-image-test-archive build-podman-driver-test-archive build-podman-e2e-test-archive ''; diff --git a/tests/config.nix b/tests/config.nix index 26d27b712b..1a0ce763d7 100644 --- a/tests/config.nix +++ b/tests/config.nix @@ -168,6 +168,13 @@ let provider_refresh_keycloak_test_bundle = "../artifacts/test-archives/${muslTarget}/provider-refresh-keycloak-tests.tar"; }; } + { + name = "oci-image"; + playbooks = [ "ansible/playbooks/features/oci-image.yaml" ]; + inputs = { + oci_image_test_bundle = "../artifacts/test-archives/${muslTarget}/oci-image-tests.tar"; + }; + } { name = "e2e-podman"; playbooks = [ "ansible/playbooks/drivers/podman/e2e.yaml" ]; diff --git a/tests/suites/features/Cargo.lock b/tests/suites/features/Cargo.lock index 8561dc97df..34d50c47fc 100644 --- a/tests/suites/features/Cargo.lock +++ b/tests/suites/features/Cargo.lock @@ -919,6 +919,15 @@ dependencies = [ "url", ] +[[package]] +name = "openshell-test-feature-oci-image" +version = "0.0.0" +dependencies = [ + "openshell-conformance", + "tempfile", + "tokio", +] + [[package]] name = "openshell-test-feature-provider-refresh-keycloak" version = "0.0.0" diff --git a/tests/suites/features/Cargo.toml b/tests/suites/features/Cargo.toml index 7acd72b5d2..9259450a16 100644 --- a/tests/suites/features/Cargo.toml +++ b/tests/suites/features/Cargo.toml @@ -3,4 +3,4 @@ [workspace] resolver = "2" -members = ["provider-refresh/keycloak"] +members = ["oci-image", "provider-refresh/keycloak"] diff --git a/tests/suites/features/oci-image/Cargo.toml b/tests/suites/features/oci-image/Cargo.toml new file mode 100644 index 0000000000..1107587ca7 --- /dev/null +++ b/tests/suites/features/oci-image/Cargo.toml @@ -0,0 +1,12 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +[package] +name = "openshell-test-feature-oci-image" +version = "0.0.0" +edition = "2024" + +[dependencies] +openshell-conformance = { path = "../../../../crates/openshell-conformance" } +tempfile = "3" +tokio = { version = "1.43", features = ["macros", "rt"] } diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs new file mode 100644 index 0000000000..3bf00ae7a2 --- /dev/null +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -0,0 +1,386 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! OCI image behavior shared by the Docker and Podman compute drivers. +//! +//! Each test builds a small image with the container engine that backs the +//! target gateway, creates a sandbox from it through the candidate CLI, and +//! checks the process identity, workspace, and image content seen by both the +//! sandbox main process and `sandbox exec`. +//! +//! `OPENSHELL_BIN` names the candidate CLI. `OPENSHELL_TEST_CONTAINER_ENGINE` +//! is the command that builds images into the gateway's image store, such as +//! `docker`, `podman`, or `sudo -n podman`. + +use std::process::Command; +use std::time::Duration; + +use openshell_conformance::OpenShellRunner; + +const BASE_IMAGE: &str = "nvcr.io/nvidia/base/ubuntu:24.04"; +const ENGINE_ENV: &str = "OPENSHELL_TEST_CONTAINER_ENGINE"; +const CREATE_TIMEOUT: Duration = Duration::from_mins(10); +const COMMAND_TIMEOUT: Duration = Duration::from_mins(2); + +/// A named image user that owns its custom `WORKDIR`. Existing image content +/// keeps its ownership. +#[tokio::test] +async fn custom_workdir_with_named_user() { + run("oci-image/custom-workdir-named-user", async |runner| { + let image = TestImage::build( + "named-workdir", + &format!( + "FROM {BASE_IMAGE} +RUN groupadd -g 1235 appstaff && useradd -m -u 1234 -g appstaff app +WORKDIR /workspace/project +RUN printf root-owned > root-owned.txt && chown app:appstaff . +USER app +" + ), + )?; + let checks = format!( + "{} test \"$(stat -c %u:%g .)\" = 1234:1235;", + workspace_checks("1234:1235", "/workspace/project", true) + ); + let sandbox = create_sandbox(runner, "named", "nu", &image, &checks).await?; + file_transfer_uses_workspace(runner, &sandbox).await + }) + .await; +} + +/// A numeric image user without passwd entries can reach a custom `WORKDIR` +/// whose parent directories are private to that user. +#[tokio::test] +async fn custom_workdir_with_numeric_user_and_private_parents() { + run("oci-image/custom-workdir-numeric-user", async |runner| { + let image = TestImage::build( + "numeric-workdir", + &format!( + "FROM {BASE_IMAGE} +RUN mkdir -p /home/app/project && \\ + chown 2345:2346 /home/app /home/app/project && \\ + chmod 0700 /home/app /home/app/project +WORKDIR /home/app/project +RUN printf root-owned > root-owned.txt +USER 2345:2346 +" + ), + )?; + let checks = workspace_checks("2345:2346", "/home/app/project", true); + create_sandbox(runner, "numeric", "pu", &image, &checks) + .await + .map(drop) + }) + .await; +} + +/// An image without a `WORKDIR` uses the managed `/sandbox` workspace, owned +/// by the image user, even when the image does not contain `/sandbox`. +#[tokio::test] +async fn default_workdir_uses_managed_workspace() { + run("oci-image/default-workdir", async |runner| { + let image = TestImage::build( + "default-workdir", + &format!("FROM {BASE_IMAGE}\nUSER 2345:2346\n"), + )?; + let checks = workspace_checks("2345:2346", "/sandbox", false); + create_sandbox(runner, "default", "dw", &image, &checks) + .await + .map(drop) + }) + .await; +} + +/// OpenShell rejects a custom `WORKDIR` that the image user cannot write +/// instead of granting the user new access to it. +#[tokio::test] +async fn unwritable_custom_workdir_is_rejected() { + run("oci-image/unwritable-workdir", async |runner| { + let image = TestImage::build( + "unwritable-workdir", + &format!( + "FROM {BASE_IMAGE} +RUN groupadd -g 3235 appstaff && useradd -m -u 3234 -g appstaff app +WORKDIR /workspace/project +USER app +" + ), + )?; + let name = format!("oi-{}-uw", runner.id()); + runner.track_sandbox(&name); + let create = runner + .step("unwritable/create") + .description("sandbox creation fails before the command runs") + .with_timeout(CREATE_TIMEOUT) + .run(&[ + "sandbox", + "create", + "--name", + &name, + "--from", + &image.tag, + "--no-tty", + "--", + "sh", + "-c", + "echo should-not-run", + ]) + .await + .map_err(|error| error.to_string())?; + if create.success() || create.stdout().contains("should-not-run") { + return Err(create.failure_diagnostic("sandbox creation fails before the command runs")); + } + Ok(()) + }) + .await; +} + +async fn run(scenario: &str, test: impl AsyncFnOnce(&mut OpenShellRunner) -> Result<(), String>) { + let mut runner = + OpenShellRunner::from_env(scenario).expect("candidate openshell CLI is available"); + let result = async { + runner.check_gateway_status().await?; + test(&mut runner).await + } + .await; + if let Err(error) = runner.finish(result).await { + panic!("{scenario} failed:\n{error}"); + } +} + +/// Shell checks for the identity, working directory, and `HOME` of a sandbox +/// child. With `image_file`, also check that root-owned image content is +/// present and unchanged in the workspace. +fn workspace_checks(identity: &str, workspace: &str, image_file: bool) -> String { + let mut checks = format!( + "test \"$(id -u):$(id -g)\" = {identity}; \ + test \"$(pwd -P)\" = {workspace}; \ + test \"$HOME\" = {workspace};" + ); + if image_file { + checks.push_str( + " test \"$(cat root-owned.txt)\" = root-owned; \ + test \"$(stat -c %u:%g root-owned.txt)\" = 0:0;", + ); + } + checks +} + +/// Create a detached sandbox whose main process runs `checks` and writes to +/// the workspace, then run the same checks and a write through `sandbox exec`. +async fn create_sandbox( + runner: &mut OpenShellRunner, + suffix: &str, + short: &str, + image: &TestImage, + checks: &str, +) -> Result { + // Sandbox names are limited to 19 characters on some drivers. + let name = format!("oi-{}-{short}", runner.id()); + runner.track_sandbox(&name); + let main = format!( + "(set -eu; {checks} touch main-write) >/tmp/oci-main.log 2>&1; \ + echo $? >/tmp/oci-main.status; exec sleep infinity" + ); + runner + .step(format!("{suffix}/create")) + .description("sandbox starts from the test image") + .with_timeout(CREATE_TIMEOUT) + .run(&[ + "sandbox", "create", "--name", &name, "--from", &image.tag, "--detach", "--", "sh", + "-c", &main, + ]) + .await + .map_err(|error| error.to_string())? + .require_success()?; + + let exec = format!( + "set -eu; i=0; \ + while [ ! -f /tmp/oci-main.status ]; do \ + i=$((i + 1)); [ \"$i\" -le 60 ] || {{ echo main process checks did not finish >&2; exit 1; }}; \ + sleep 1; \ + done; \ + if [ \"$(cat /tmp/oci-main.status)\" != 0 ]; then \ + echo main process checks failed: >&2; cat /tmp/oci-main.log >&2; exit 1; \ + fi; \ + test -f main-write; {checks} touch exec-write" + ); + runner + .step(format!("{suffix}/exec")) + .description("main process and exec children see the image workspace") + .with_timeout(COMMAND_TIMEOUT) + .run(&[ + "sandbox", "exec", "--name", &name, "--no-tty", "--", "sh", "-c", &exec, + ]) + .await + .map_err(|error| error.to_string())? + .require_success()?; + Ok(name) +} + +/// Upload and download default to paths relative to the image workspace. +async fn file_transfer_uses_workspace( + runner: &OpenShellRunner, + sandbox: &str, +) -> Result<(), String> { + let local = tempfile::tempdir().map_err(|error| format!("create temp dir: {error}"))?; + let upload = local.path().join("oci-transfer.txt"); + std::fs::write(&upload, "oci-transfer-ok").map_err(|error| format!("write upload: {error}"))?; + let upload = upload.to_str().ok_or("upload path is not UTF-8")?; + runner + .step("named/upload") + .description("upload without a destination writes to the workspace") + .with_timeout(COMMAND_TIMEOUT) + .run(&["sandbox", "upload", sandbox, upload, "--no-git-ignore"]) + .await + .map_err(|error| error.to_string())? + .require_success()?; + runner + .step("named/uploaded") + .description("uploaded file is in the workspace") + .with_timeout(COMMAND_TIMEOUT) + .run(&[ + "sandbox", + "exec", + "--name", + sandbox, + "--no-tty", + "--", + "sh", + "-c", + "test \"$(cat oci-transfer.txt)\" = oci-transfer-ok", + ]) + .await + .map_err(|error| error.to_string())? + .require_success()?; + + let download = local.path().join("downloaded.txt"); + let download_path = download.to_str().ok_or("download path is not UTF-8")?; + runner + .step("named/download") + .description("download resolves relative paths in the workspace") + .with_timeout(COMMAND_TIMEOUT) + .run(&[ + "sandbox", + "download", + sandbox, + "oci-transfer.txt", + download_path, + ]) + .await + .map_err(|error| error.to_string())? + .require_success()?; + let downloaded = + std::fs::read_to_string(&download).map_err(|error| format!("read download: {error}"))?; + if downloaded != "oci-transfer-ok" { + return Err(format!( + "downloaded file has unexpected content: {downloaded:?}" + )); + } + + // A directory upload merges into an existing workspace directory. + let merge = local.path().join("merge-upload"); + std::fs::create_dir(&merge).map_err(|error| format!("create merge dir: {error}"))?; + std::fs::write(merge.join("conflict.txt"), "local-conflict") + .and_then(|()| std::fs::write(merge.join("added.txt"), "local-added")) + .map_err(|error| format!("write merge files: {error}"))?; + let merge = merge.to_str().ok_or("merge path is not UTF-8")?; + let seed = "mkdir merge-upload && printf remote-conflict > merge-upload/conflict.txt \ + && printf remote-preserved > merge-upload/unrelated.txt"; + let verify = "test \"$(cat merge-upload/conflict.txt)\" = local-conflict \ + && test \"$(cat merge-upload/added.txt)\" = local-added \ + && test \"$(cat merge-upload/unrelated.txt)\" = remote-preserved"; + for (step, description, args) in [ + ( + "named/merge-seed", + "seed an existing workspace directory", + [ + "sandbox", "exec", "--name", sandbox, "--no-tty", "--", "sh", "-c", seed, + ] + .as_slice(), + ), + ( + "named/merge-upload", + "directory upload merges into the existing directory", + ["sandbox", "upload", sandbox, merge, "--no-git-ignore"].as_slice(), + ), + ( + "named/merged", + "upload overwrites conflicts and keeps unrelated files", + [ + "sandbox", "exec", "--name", sandbox, "--no-tty", "--", "sh", "-c", verify, + ] + .as_slice(), + ), + ] { + runner + .step(step) + .description(description) + .with_timeout(COMMAND_TIMEOUT) + .run(args) + .await + .map_err(|error| error.to_string())? + .require_success()?; + } + Ok(()) +} + +/// An image built into the gateway's image store and removed on drop. +struct TestImage { + engine: Vec, + tag: String, +} + +impl TestImage { + fn build(name: &str, containerfile: &str) -> Result { + let engine: Vec = std::env::var(ENGINE_ENV) + .map_err(|_| format!("{ENGINE_ENV} must name the gateway's container engine"))? + .split_whitespace() + .map(str::to_string) + .collect(); + if engine.is_empty() { + return Err(format!("{ENGINE_ENV} is empty")); + } + let context = tempfile::tempdir().map_err(|error| format!("create context: {error}"))?; + let file = context.path().join("Containerfile"); + std::fs::write(&file, containerfile) + .map_err(|error| format!("write Containerfile: {error}"))?; + let image = Self { + engine, + tag: format!("localhost/openshell-test-oci-{name}:{}", std::process::id()), + }; + image.engine_command(&[ + "build", + "--file", + file.to_str().ok_or("Containerfile path is not UTF-8")?, + "--tag", + &image.tag, + context.path().to_str().ok_or("context path is not UTF-8")?, + ])?; + Ok(image) + } + + fn engine_command(&self, args: &[&str]) -> Result<(), String> { + let command = format!("{} {}", self.engine.join(" "), args.join(" ")); + let output = Command::new(&self.engine[0]) + .args(&self.engine[1..]) + .args(args) + .output() + .map_err(|error| format!("failed to run {command}: {error}"))?; + if !output.status.success() { + return Err(format!( + "{command} failed ({}):\n{}{}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )); + } + Ok(()) + } +} + +impl Drop for TestImage { + fn drop(&mut self) { + let _ = self.engine_command(&["image", "rm", "--force", &self.tag]); + } +} From 0b207cec9e268beac57b504c30d6400bf6340391 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Wed, 30 Sep 2026 01:30:57 -0700 Subject: [PATCH 02/16] test(oci): require WorkspaceValidationFailed and cover image USER fallback The unwritable WORKDIR scenario now requires the WorkspaceValidationFailed reason, so unrelated provisioning failures cannot pass it. The numeric USER scenario supplies a policy without a process section, so the image USER is the only source of the sandbox identity. Signed-off-by: Matthew Grossman Signed-off-by: Evan Lezar --- .../features/oci-image/tests/oci_image.rs | 54 +++++++++++++++---- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index 3bf00ae7a2..065a49789c 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -21,6 +21,22 @@ const BASE_IMAGE: &str = "nvcr.io/nvidia/base/ubuntu:24.04"; const ENGINE_ENV: &str = "OPENSHELL_TEST_CONTAINER_ENGINE"; const CREATE_TIMEOUT: Duration = Duration::from_mins(10); const COMMAND_TIMEOUT: Duration = Duration::from_mins(2); +/// Sandbox condition reason for an image `WORKDIR` the sandbox identity +/// cannot use. +const WORKSPACE_VALIDATION_FAILED: &str = "WorkspaceValidationFailed"; +/// A complete sandbox policy without a `process` section, so the sandbox +/// identity falls back to the image `USER`. +const IMAGE_IDENTITY_POLICY: &str = "version: 1 + +filesystem_policy: + include_workdir: true + read_only: [/usr, /lib, /lib64, /proc, /dev/urandom, /etc] + read_write: [/sandbox, /tmp, /dev/null] +landlock: + compatibility: best_effort + +network_policies: {} +"; /// A named image user that owns its custom `WORKDIR`. Existing image content /// keeps its ownership. @@ -42,14 +58,15 @@ USER app "{} test \"$(stat -c %u:%g .)\" = 1234:1235;", workspace_checks("1234:1235", "/workspace/project", true) ); - let sandbox = create_sandbox(runner, "named", "nu", &image, &checks).await?; + let sandbox = create_sandbox(runner, "named", "nu", &image, None, &checks).await?; file_transfer_uses_workspace(runner, &sandbox).await }) .await; } /// A numeric image user without passwd entries can reach a custom `WORKDIR` -/// whose parent directories are private to that user. +/// whose parent directories are private to that user. The sandbox policy omits +/// `process`, so the image `USER` is the only source of the sandbox identity. #[tokio::test] async fn custom_workdir_with_numeric_user_and_private_parents() { run("oci-image/custom-workdir-numeric-user", async |runner| { @@ -66,8 +83,16 @@ USER 2345:2346 " ), )?; + let policy_file = tempfile::NamedTempFile::new() + .map_err(|error| format!("create policy file: {error}"))?; + std::fs::write(policy_file.path(), IMAGE_IDENTITY_POLICY) + .map_err(|error| format!("write policy file: {error}"))?; + let policy = policy_file + .path() + .to_str() + .ok_or("policy path is not UTF-8")?; let checks = workspace_checks("2345:2346", "/home/app/project", true); - create_sandbox(runner, "numeric", "pu", &image, &checks) + create_sandbox(runner, "numeric", "pu", &image, Some(policy), &checks) .await .map(drop) }) @@ -84,7 +109,7 @@ async fn default_workdir_uses_managed_workspace() { &format!("FROM {BASE_IMAGE}\nUSER 2345:2346\n"), )?; let checks = workspace_checks("2345:2346", "/sandbox", false); - create_sandbox(runner, "default", "dw", &image, &checks) + create_sandbox(runner, "default", "dw", &image, None, &checks) .await .map(drop) }) @@ -92,7 +117,8 @@ async fn default_workdir_uses_managed_workspace() { } /// OpenShell rejects a custom `WORKDIR` that the image user cannot write -/// instead of granting the user new access to it. +/// instead of granting the user new access to it, and reports that reason +/// rather than a generic startup failure. #[tokio::test] async fn unwritable_custom_workdir_is_rejected() { run("oci-image/unwritable-workdir", async |runner| { @@ -130,6 +156,13 @@ USER app if create.success() || create.stdout().contains("should-not-run") { return Err(create.failure_diagnostic("sandbox creation fails before the command runs")); } + if !create.stdout().contains(WORKSPACE_VALIDATION_FAILED) + && !create.stderr().contains(WORKSPACE_VALIDATION_FAILED) + { + return Err(create.failure_diagnostic(&format!( + "sandbox creation fails with {WORKSPACE_VALIDATION_FAILED}" + ))); + } Ok(()) }) .await; @@ -173,6 +206,7 @@ async fn create_sandbox( suffix: &str, short: &str, image: &TestImage, + policy: Option<&str>, checks: &str, ) -> Result { // Sandbox names are limited to 19 characters on some drivers. @@ -182,14 +216,16 @@ async fn create_sandbox( "(set -eu; {checks} touch main-write) >/tmp/oci-main.log 2>&1; \ echo $? >/tmp/oci-main.status; exec sleep infinity" ); + let mut args = vec!["sandbox", "create", "--name", &name, "--from", &image.tag]; + if let Some(policy) = policy { + args.extend(["--policy", policy]); + } + args.extend(["--detach", "--", "sh", "-c", &main]); runner .step(format!("{suffix}/create")) .description("sandbox starts from the test image") .with_timeout(CREATE_TIMEOUT) - .run(&[ - "sandbox", "create", "--name", &name, "--from", &image.tag, "--detach", "--", "sh", - "-c", &main, - ]) + .run(&args) .await .map_err(|error| error.to_string())? .require_success()?; From 27bfea80ababf880987bf5ff412e0d8e221e980d Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Wed, 30 Sep 2026 01:37:41 -0700 Subject: [PATCH 03/16] test(podman): leave shared OCI identity checks to the oci-image suite The oci-image feature suite covers the sandbox identity seen by the main process and sandbox exec on Docker and Podman. The Podman e2e keeps the checks only it can make: the pinned image ID and the isolated workload/supervisor container pair. Signed-off-by: Matthew Grossman Signed-off-by: Evan Lezar --- e2e/rust/tests/podman_oci_identity.rs | 50 ++++++--------------------- 1 file changed, 11 insertions(+), 39 deletions(-) diff --git a/e2e/rust/tests/podman_oci_identity.rs b/e2e/rust/tests/podman_oci_identity.rs index 142da82796..42e91e37c6 100644 --- a/e2e/rust/tests/podman_oci_identity.rs +++ b/e2e/rust/tests/podman_oci_identity.rs @@ -3,20 +3,21 @@ #![cfg(feature = "e2e-podman")] -//! Podman-specific E2E coverage for OCI identity inspection and immutable-image -//! launch. +//! Podman-specific E2E coverage for immutable-image launch and the isolated +//! workload/supervisor container pair. //! //! The test builds an image through the selected Podman engine, creates a -//! sandbox from its mutable tag, and verifies both the child identity and the -//! image ID recorded on the real sandbox container. This exercises the Podman -//! API inspect → protected metadata → create path rather than only its unit -//! serialization boundaries. Workspace behavior shared with Docker is covered -//! by the `oci-image` feature suite in `tests/suites/features`. +//! sandbox from its mutable tag, and inspects the real Podman containers: the +//! image ID recorded on the workload, each container's user, the supervisor's +//! capabilities, networking, and mounts. This exercises the Podman API +//! inspect → protected metadata → create path rather than only its unit +//! serialization boundaries. The sandbox identity and workspace seen by the +//! main process and `sandbox exec` are shared with Docker and covered by the +//! `oci-image` feature suite in `tests/suites/features`. use std::process::Stdio; use openshell_e2e::harness::container::{ContainerEngine, is_e2e_driver}; -use openshell_e2e::harness::output::strip_ansi; use openshell_e2e::harness::sandbox::SandboxGuard; const BASE_IMAGE: &str = "nvcr.io/nvidia/base/ubuntu:24.04"; @@ -199,42 +200,13 @@ async fn podman_uses_oci_identity_and_inspected_image_id() { std::fs::write(policy.path(), OCI_FALLBACK_POLICY).expect("write OCI fallback policy"); let policy_path = policy.path().to_str().expect("policy path is UTF-8"); let mut sandbox = SandboxGuard::create_keep_with_args( - &[ - "--from", - &image.tag, - "--policy", - policy_path, - "--no-tty", - ], - &[ - "sh", - "-c", - "set -eu; printf 'direct-identity=%s:%s\n' \"$(id -u)\" \"$(id -g)\"; echo podman-oci-identity-ready; sleep infinity", - ], + &["--from", &image.tag, "--policy", policy_path, "--no-tty"], + &["sh", "-c", "echo podman-oci-identity-ready; sleep infinity"], READY_MARKER, ) .await .expect("create sandbox from Podman-built OCI identity image"); - let direct_output = strip_ansi(&sandbox.create_output); - assert!( - direct_output.contains("direct-identity=2345:2346"), - "expected direct child identity {OCI_UID}:{OCI_GID}:\n{direct_output}" - ); - - let ssh_output = sandbox - .exec(&[ - "sh", - "-c", - "test \"$(id -u):$(id -g)\" = 2345:2346; echo podman-ssh-identity-ok", - ]) - .await - .expect("SSH child should use Podman OCI identity"); - assert!( - ssh_output.contains("podman-ssh-identity-ok"), - "expected SSH identity marker:\n{ssh_output}" - ); - let container_id = sandbox_container_id(&image.engine, &sandbox.name).expect("find Podman sandbox container"); let launched_image_id = run_engine( From 9ebe52b37ed4dcddc89525004ccbe612235c2cfc Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 6 Oct 2026 09:55:52 -0700 Subject: [PATCH 04/16] test(oci): accept existing workspace rejection diagnostics Signed-off-by: Matthew Grossman Signed-off-by: Evan Lezar --- .../features/oci-image/tests/oci_image.rs | 60 +++++++++++++++---- 1 file changed, 49 insertions(+), 11 deletions(-) diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index 065a49789c..a3510b4ccc 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -21,9 +21,6 @@ const BASE_IMAGE: &str = "nvcr.io/nvidia/base/ubuntu:24.04"; const ENGINE_ENV: &str = "OPENSHELL_TEST_CONTAINER_ENGINE"; const CREATE_TIMEOUT: Duration = Duration::from_mins(10); const COMMAND_TIMEOUT: Duration = Duration::from_mins(2); -/// Sandbox condition reason for an image `WORKDIR` the sandbox identity -/// cannot use. -const WORKSPACE_VALIDATION_FAILED: &str = "WorkspaceValidationFailed"; /// A complete sandbox policy without a `process` section, so the sandbox /// identity falls back to the image `USER`. const IMAGE_IDENTITY_POLICY: &str = "version: 1 @@ -117,8 +114,9 @@ async fn default_workdir_uses_managed_workspace() { } /// OpenShell rejects a custom `WORKDIR` that the image user cannot write -/// instead of granting the user new access to it, and reports that reason -/// rather than a generic startup failure. +/// instead of granting the user new access to it. Drivers may surface the +/// rejection through different startup diagnostics rather than one condition +/// reason. #[tokio::test] async fn unwritable_custom_workdir_is_rejected() { run("oci-image/unwritable-workdir", async |runner| { @@ -156,18 +154,58 @@ USER app if create.success() || create.stdout().contains("should-not-run") { return Err(create.failure_diagnostic("sandbox creation fails before the command runs")); } - if !create.stdout().contains(WORKSPACE_VALIDATION_FAILED) - && !create.stderr().contains(WORKSPACE_VALIDATION_FAILED) - { - return Err(create.failure_diagnostic(&format!( - "sandbox creation fails with {WORKSPACE_VALIDATION_FAILED}" - ))); + let diagnostic = format!("{}\n{}", create.stdout(), create.stderr()); + if !has_workspace_rejection_diagnostic(&diagnostic) { + return Err(create.failure_diagnostic( + "sandbox creation reports a workspace, permission, or workload startup rejection", + )); } Ok(()) }) .await; } +fn has_workspace_rejection_diagnostic(diagnostic: &str) -> bool { + let diagnostic = diagnostic.to_ascii_lowercase(); + // The CLI may wrap the human message across lines with diagnostic gutters. + // Match the existing startup reason and exit detail independently. + if diagnostic.contains("containerexited") && diagnostic.contains("exited with code") { + return true; + } + [ + "workspace", + "workingdir", + "permission denied", + // Some drivers expose the rejected workload launch through SSH rather + // than propagating the runtime's workspace validation text. + "subsystem request failed", + ] + .iter() + .any(|message| diagnostic.contains(message)) +} + +#[test] +fn workspace_rejection_diagnostics_do_not_require_one_condition_reason() { + for diagnostic in [ + "image workspace validation failed", + "WorkingDir /workspace/project is not writable", + "Permission denied (os error 13)", + "ContainerExited: Container exited with code 1", + "Error: × sandbox entered error phase while provisioning: ContainerExited: Container\n │ exited with code 1", + "subsystem request failed", + ] { + assert!(has_workspace_rejection_diagnostic(diagnostic)); + } + for diagnostic in [ + "", + "gateway connection refused", + "image pull failed", + "ContainerExited", + ] { + assert!(!has_workspace_rejection_diagnostic(diagnostic)); + } +} + async fn run(scenario: &str, test: impl AsyncFnOnce(&mut OpenShellRunner) -> Result<(), String>) { let mut runner = OpenShellRunner::from_env(scenario).expect("candidate openshell CLI is available"); From 77540f20bdba0ead02ebd65c853e332dca376e00 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 6 Oct 2026 11:15:25 -0700 Subject: [PATCH 05/16] test(oci): fix cleanup ordering and restore coverage parity Signed-off-by: Matthew Grossman Signed-off-by: Evan Lezar --- .github/workflows/e2e-docker-test.yml | 2 +- TESTING.md | 7 + .../features/oci-image/tests/oci_image.rs | 301 +++++++++++++----- 3 files changed, 225 insertions(+), 85 deletions(-) diff --git a/.github/workflows/e2e-docker-test.yml b/.github/workflows/e2e-docker-test.yml index f26c905d0a..b13d7d15f1 100644 --- a/.github/workflows/e2e-docker-test.yml +++ b/.github/workflows/e2e-docker-test.yml @@ -33,7 +33,7 @@ on: required: false type: string default: >- - [{"suite":"python","cmd":"mise run --no-deps --skip-deps e2e:python","apt_packages":"","python_proto":true,"mcp":false},{"suite":"oidc-python","cmd":"mise run --no-deps --skip-deps e2e:oidc-python:docker","apt_packages":"","python_proto":true,"mcp":false},{"suite":"oidc-pkce-docker","cmd":"mise run --no-deps --skip-deps e2e:oidc-pkce:docker","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"rust-docker","cmd":"mise run --no-deps --skip-deps e2e:rust","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"mcp","cmd":"mise run --no-deps --skip-deps e2e:mcp","apt_packages":"","python_proto":false,"mcp":true}] + [{"suite":"python","cmd":"mise run --no-deps --skip-deps e2e:python","apt_packages":"","python_proto":true,"mcp":false},{"suite":"oidc-python","cmd":"mise run --no-deps --skip-deps e2e:oidc-python:docker","apt_packages":"","python_proto":true,"mcp":false},{"suite":"oidc-pkce-docker","cmd":"mise run --no-deps --skip-deps e2e:oidc-pkce:docker","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"rust-docker","cmd":"mise run --no-deps --skip-deps e2e:rust","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"oci-image","cmd":"OPENSHELL_TEST_CONTAINER_ENGINE=docker e2e/with-docker-gateway.sh cargo test --locked --manifest-path tests/suites/features/Cargo.toml -p openshell-test-feature-oci-image -- --test-threads 1 --nocapture","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"mcp","cmd":"mise run --no-deps --skip-deps e2e:mcp","apt_packages":"","python_proto":false,"mcp":true}] permissions: actions: read diff --git a/TESTING.md b/TESTING.md index fb9d0cfa05..3347dac0ed 100644 --- a/TESTING.md +++ b/TESTING.md @@ -320,6 +320,13 @@ Fedora. CI runs it on Docker rootful, Podman rootful, and Podman rootless guests nix run .#tmachine -- test ubuntu-docker-rootful deb oci-image ``` +The shared suite also runs in the existing ARM64 Docker E2E workflow for +branch checks and both release workflows. That interim lane compiles the same +test crate natively and uses the architecture-matched prebuilt candidate CLI +and gateway plus the candidate multi-architecture sandbox and supervisor +images through `e2e/with-docker-gateway.sh`. The tmachine lanes still use +x86_64 artifacts; they do not replace ARM64 image coverage. + Run it against a local gateway by naming the command that builds images into the gateway's image store: diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index a3510b4ccc..175357c179 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -12,6 +12,7 @@ //! is the command that builds images into the gateway's image store, such as //! `docker`, `podman`, or `sudo -n podman`. +use std::future::Future; use std::process::Command; use std::time::Duration; @@ -39,25 +40,32 @@ network_policies: {} /// keeps its ownership. #[tokio::test] async fn custom_workdir_with_named_user() { - run("oci-image/custom-workdir-named-user", async |runner| { - let image = TestImage::build( - "named-workdir", - &format!( - "FROM {BASE_IMAGE} + run( + "oci-image/custom-workdir-named-user", + async |runner, images| { + let image = TestImage::build( + images, + "named-workdir", + &format!( + "FROM {BASE_IMAGE} RUN groupadd -g 1235 appstaff && useradd -m -u 1234 -g appstaff app WORKDIR /workspace/project -RUN printf root-owned > root-owned.txt && chown app:appstaff . +RUN printf root-owned > root-owned.txt && chown app:appstaff . && \ + printf custom-image-e2e-marker > /etc/oci-image-marker USER app " - ), - )?; - let checks = format!( - "{} test \"$(stat -c %u:%g .)\" = 1234:1235;", - workspace_checks("1234:1235", "/workspace/project", true) - ); - let sandbox = create_sandbox(runner, "named", "nu", &image, None, &checks).await?; - file_transfer_uses_workspace(runner, &sandbox).await - }) + ), + )?; + let checks = format!( + "{} test \"$(stat -c %u:%g .)\" = 1234:1235; \ + test \"$(cat /etc/oci-image-marker)\" = custom-image-e2e-marker; \ + test \"$(stat -c %u:%g /etc/oci-image-marker)\" = 0:0;", + workspace_checks("1234:1235", "/workspace/project", true) + ); + let sandbox = create_sandbox(runner, "named", "nu", image, None, &checks).await?; + file_transfer_uses_workspace(runner, &sandbox).await + }, + ) .await; } @@ -66,11 +74,14 @@ USER app /// `process`, so the image `USER` is the only source of the sandbox identity. #[tokio::test] async fn custom_workdir_with_numeric_user_and_private_parents() { - run("oci-image/custom-workdir-numeric-user", async |runner| { - let image = TestImage::build( - "numeric-workdir", - &format!( - "FROM {BASE_IMAGE} + run( + "oci-image/custom-workdir-numeric-user", + async |runner, images| { + let image = TestImage::build( + images, + "numeric-workdir", + &format!( + "FROM {BASE_IMAGE} RUN mkdir -p /home/app/project && \\ chown 2345:2346 /home/app /home/app/project && \\ chmod 0700 /home/app /home/app/project @@ -78,21 +89,22 @@ WORKDIR /home/app/project RUN printf root-owned > root-owned.txt USER 2345:2346 " - ), - )?; - let policy_file = tempfile::NamedTempFile::new() - .map_err(|error| format!("create policy file: {error}"))?; - std::fs::write(policy_file.path(), IMAGE_IDENTITY_POLICY) - .map_err(|error| format!("write policy file: {error}"))?; - let policy = policy_file - .path() - .to_str() - .ok_or("policy path is not UTF-8")?; - let checks = workspace_checks("2345:2346", "/home/app/project", true); - create_sandbox(runner, "numeric", "pu", &image, Some(policy), &checks) - .await - .map(drop) - }) + ), + )?; + let policy_file = tempfile::NamedTempFile::new() + .map_err(|error| format!("create policy file: {error}"))?; + std::fs::write(policy_file.path(), IMAGE_IDENTITY_POLICY) + .map_err(|error| format!("write policy file: {error}"))?; + let policy = policy_file + .path() + .to_str() + .ok_or("policy path is not UTF-8")?; + let checks = workspace_checks("2345:2346", "/home/app/project", true); + create_sandbox(runner, "numeric", "pu", image, Some(policy), &checks) + .await + .map(drop) + }, + ) .await; } @@ -100,13 +112,14 @@ USER 2345:2346 /// by the image user, even when the image does not contain `/sandbox`. #[tokio::test] async fn default_workdir_uses_managed_workspace() { - run("oci-image/default-workdir", async |runner| { + run("oci-image/default-workdir", async |runner, images| { let image = TestImage::build( + images, "default-workdir", &format!("FROM {BASE_IMAGE}\nUSER 2345:2346\n"), )?; let checks = workspace_checks("2345:2346", "/sandbox", false); - create_sandbox(runner, "default", "dw", &image, None, &checks) + create_sandbox(runner, "default", "dw", image, None, &checks) .await .map(drop) }) @@ -119,46 +132,70 @@ async fn default_workdir_uses_managed_workspace() { /// reason. #[tokio::test] async fn unwritable_custom_workdir_is_rejected() { - run("oci-image/unwritable-workdir", async |runner| { - let image = TestImage::build( - "unwritable-workdir", - &format!( - "FROM {BASE_IMAGE} + run("oci-image/unwritable-workdir", async |runner, images| { + // First prove that this user, workspace and command can run. Only the + // directory owner differs between the control and rejected image. + for (suffix, owner, writable) in [("wc", "3234:3235", true), ("uw", "0:0", false)] { + let image = TestImage::build( + images, + suffix, + &format!( + "FROM {BASE_IMAGE} RUN groupadd -g 3235 appstaff && useradd -m -u 3234 -g appstaff app WORKDIR /workspace/project +RUN chown {owner} . USER app " - ), - )?; - let name = format!("oi-{}-uw", runner.id()); - runner.track_sandbox(&name); - let create = runner - .step("unwritable/create") - .description("sandbox creation fails before the command runs") - .with_timeout(CREATE_TIMEOUT) - .run(&[ - "sandbox", - "create", - "--name", - &name, - "--from", - &image.tag, - "--no-tty", - "--", - "sh", - "-c", - "echo should-not-run", - ]) - .await - .map_err(|error| error.to_string())?; - if create.success() || create.stdout().contains("should-not-run") { - return Err(create.failure_diagnostic("sandbox creation fails before the command runs")); - } - let diagnostic = format!("{}\n{}", create.stdout(), create.stderr()); - if !has_workspace_rejection_diagnostic(&diagnostic) { - return Err(create.failure_diagnostic( + ), + )?; + let name = format!("oi-{}-{suffix}", runner.id()); + runner.track_sandbox(&name); + let create = runner + .step(format!("{suffix}/create")) + .description(if writable { + "writable control runs the same command successfully" + } else { + "sandbox creation fails before the command runs" + }) + .with_timeout(CREATE_TIMEOUT) + .run(&[ + "sandbox", + "create", + "--name", + &name, + "--from", + &image.tag, + "--no-tty", + "--", + "sh", + "-c", + "echo workspace-access-marker", + ]) + .await + .map_err(|error| error.to_string())?; + if writable { + create.require_success()?; + if !create.stdout().contains("workspace-access-marker") { + return Err( + create.failure_diagnostic("writable control executes the command marker") + ); + } + continue; + } + if create.success() + || create.stdout().contains("workspace-access-marker") + || create.stderr().contains("workspace-access-marker") + { + return Err( + create.failure_diagnostic("sandbox creation fails before the command runs") + ); + } + let diagnostic = format!("{}\n{}", create.stdout(), create.stderr()); + if !has_workspace_rejection_diagnostic(&diagnostic) { + return Err(create.failure_diagnostic( "sandbox creation reports a workspace, permission, or workload startup rejection", )); + } } Ok(()) }) @@ -167,6 +204,24 @@ USER app fn has_workspace_rejection_diagnostic(diagnostic: &str) -> bool { let diagnostic = diagnostic.to_ascii_lowercase(); + // A successful control does not excuse a later connectivity or image-pull + // failure. Those are not evidence of workspace rejection, even if their + // messages happen to mention the workspace. + if [ + "connection refused", + "connection reset", + "imagepull", + "image pull", + "failed to pull", + "pull access denied", + "manifest unknown", + "no such image", + ] + .iter() + .any(|message| diagnostic.contains(message)) + { + return false; + } // The CLI may wrap the human message across lines with diagnostic gutters. // Match the existing startup reason and exit detail independently. if diagnostic.contains("containerexited") && diagnostic.contains("exited with code") { @@ -200,25 +255,100 @@ fn workspace_rejection_diagnostics_do_not_require_one_condition_reason() { "", "gateway connection refused", "image pull failed", + "image pull failed for workspace fixture", + "gateway connection refused while creating workspace", + "ImagePullFailed: failed to pull image for WorkingDir test", "ContainerExited", ] { assert!(!has_workspace_rejection_diagnostic(diagnostic)); } } -async fn run(scenario: &str, test: impl AsyncFnOnce(&mut OpenShellRunner) -> Result<(), String>) { +async fn run( + scenario: &str, + test: impl AsyncFnOnce(&mut OpenShellRunner, &mut Vec) -> Result<(), String>, +) { let mut runner = OpenShellRunner::from_env(scenario).expect("candidate openshell CLI is available"); + let mut images = Vec::new(); let result = async { runner.check_gateway_status().await?; - test(&mut runner).await + test(&mut runner, &mut images).await } .await; - if let Err(error) = runner.finish(result).await { + if let Err(error) = + finish_with_image_cleanup(runner.finish(result), &images, TestImage::remove).await + { panic!("{scenario} failed:\n{error}"); } } +/// Finish OpenShell sandbox cleanup before removing image tags. Non-forced +/// image removal must report remaining users rather than deleting workloads +/// behind the gateway's back. Keep functional/sandbox failures primary. +async fn finish_with_image_cleanup( + finish: impl Future>, + images: &[T], + mut remove: impl FnMut(&T) -> Result<(), String>, +) -> Result<(), String> { + let result = finish.await; + let errors: Vec<_> = images + .iter() + .filter_map(|image| remove(image).err()) + .collect(); + if errors.is_empty() { + return result; + } + let cleanup = format!("image cleanup failed:\n{}", errors.join("\n")); + match result { + Ok(()) => Err(cleanup), + Err(primary) => Err(format!("{primary}\n{cleanup}")), + } +} + +#[tokio::test] +async fn sandbox_cleanup_precedes_image_removal_on_success_and_failure() { + use std::cell::RefCell; + + for fail_scenario in [false, true] { + for fail_cleanup in [false, true] { + let events = RefCell::new(Vec::new()); + let finish = async { + events.borrow_mut().push("sandbox delete"); + if fail_scenario { + Err("functional failure".to_string()) + } else { + Ok(()) + } + }; + let result = finish_with_image_cleanup(finish, &["first", "second"], |image| { + events.borrow_mut().push(image); + if fail_cleanup { + Err(format!("cannot remove {image}")) + } else { + Ok(()) + } + }) + .await; + assert_eq!(*events.borrow(), ["sandbox delete", "first", "second"]); + assert_eq!(result.is_err(), fail_scenario || fail_cleanup); + if fail_scenario { + assert!( + result + .as_ref() + .unwrap_err() + .starts_with("functional failure") + ); + } + if fail_cleanup { + let error = result.unwrap_err(); + assert!(error.contains("cannot remove first")); + assert!(error.contains("cannot remove second")); + } + } + } +} + /// Shell checks for the identity, working directory, and `HOME` of a sandbox /// child. With `image_file`, also check that root-owned image content is /// present and unchanged in the workspace. @@ -399,14 +529,18 @@ async fn file_transfer_uses_workspace( Ok(()) } -/// An image built into the gateway's image store and removed on drop. +/// An image owned by `run`, kept alive until sandbox cleanup completes. struct TestImage { engine: Vec, tag: String, } impl TestImage { - fn build(name: &str, containerfile: &str) -> Result { + fn build<'a>( + images: &'a mut Vec, + name: &str, + containerfile: &str, + ) -> Result<&'a Self, String> { let engine: Vec = std::env::var(ENGINE_ENV) .map_err(|_| format!("{ENGINE_ENV} must name the gateway's container engine"))? .split_whitespace() @@ -419,10 +553,11 @@ impl TestImage { let file = context.path().join("Containerfile"); std::fs::write(&file, containerfile) .map_err(|error| format!("write Containerfile: {error}"))?; - let image = Self { + images.push(Self { engine, tag: format!("localhost/openshell-test-oci-{name}:{}", std::process::id()), - }; + }); + let image = images.last().expect("registered image"); image.engine_command(&[ "build", "--file", @@ -434,6 +569,10 @@ impl TestImage { Ok(image) } + fn remove(&self) -> Result<(), String> { + self.engine_command(&["image", "rm", &self.tag]) + } + fn engine_command(&self, args: &[&str]) -> Result<(), String> { let command = format!("{} {}", self.engine.join(" "), args.join(" ")); let output = Command::new(&self.engine[0]) @@ -452,9 +591,3 @@ impl TestImage { Ok(()) } } - -impl Drop for TestImage { - fn drop(&mut self) { - let _ = self.engine_command(&["image", "rm", "--force", &self.tag]); - } -} From 9642f7e11cfebde82c04803a21df7177eb9f0e57 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Wed, 7 Oct 2026 16:04:09 -0700 Subject: [PATCH 06/16] test(oci): remove migrated legacy workspace helpers and checks Signed-off-by: Matthew Grossman --- e2e/rust/src/harness/sandbox.rs | 33 --------------------------- e2e/rust/tests/podman_oci_identity.rs | 17 -------------- 2 files changed, 50 deletions(-) diff --git a/e2e/rust/src/harness/sandbox.rs b/e2e/rust/src/harness/sandbox.rs index 0ebfde9081..5791729433 100644 --- a/e2e/rust/src/harness/sandbox.rs +++ b/e2e/rust/src/harness/sandbox.rs @@ -510,39 +510,6 @@ impl SandboxGuard { Ok(combined) } - /// Upload local files to the sandbox's discovered working directory. - /// - /// # Errors - /// - /// Returns an error if the upload command fails. - pub async fn upload_to_workdir(&self, local_path: &str) -> Result { - let mut cmd = openshell_cmd(); - cmd.arg("sandbox") - .arg("upload") - .arg(&self.name) - .arg(local_path) - .arg("--no-git-ignore"); - cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); - - let output = cmd - .output() - .await - .map_err(|e| format!("failed to spawn openshell upload: {e}"))?; - - let stdout = String::from_utf8_lossy(&output.stdout).to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).to_string(); - let combined = format!("{stdout}{stderr}"); - - if !output.status.success() { - return Err(format!( - "sandbox upload failed (exit {:?}):\n{combined}", - output.status.code() - )); - } - - Ok(combined) - } - /// Upload local files with `.gitignore` filtering (default behavior). /// /// Unlike [`upload`], this does NOT pass `--no-git-ignore`, so the CLI diff --git a/e2e/rust/tests/podman_oci_identity.rs b/e2e/rust/tests/podman_oci_identity.rs index 42e91e37c6..a7df58585c 100644 --- a/e2e/rust/tests/podman_oci_identity.rs +++ b/e2e/rust/tests/podman_oci_identity.rs @@ -226,23 +226,6 @@ async fn podman_uses_oci_identity_and_inspected_image_id() { "Podman sandbox must launch the immutable image ID inspected before creation" ); - let workspace_output = sandbox - .exec(&[ - "sh", - "-c", - "set -eu; test \"$(pwd -P)\" = /home/app/project; test ! -e /sandbox; stat -c 'workspace-owner=%u:%g' .; touch probe; rm probe; echo podman-workspace-write-ok", - ]) - .await - .expect("OCI workload should be able to write to its image workspace"); - assert!( - workspace_output.contains(&format!("workspace-owner={OCI_UID}:{OCI_GID}")), - "expected workspace owner {OCI_UID}:{OCI_GID}:\n{workspace_output}" - ); - assert!( - workspace_output.contains("podman-workspace-write-ok"), - "expected workspace write marker:\n{workspace_output}" - ); - assert_isolated_pair(&image, &sandbox, &container_id).await; sandbox.cleanup().await; } From 6d0295123b8acd0212d3094dba78814ba23e43ce Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Thu, 8 Oct 2026 09:08:54 -0700 Subject: [PATCH 07/16] fix(drivers): report rejected OCI working directories as WorkspaceValidationFailed The supervisor exits with the reserved status when the sandbox rejects the image working directory. Docker maps that exit during readiness and monitoring and records it before removing the supervisor container, and Podman maps the supervisor companion's exit instead of the workload's. Signed-off-by: Matthew Grossman --- crates/openshell-core/src/driver_utils.rs | 13 +- crates/openshell-driver-docker/src/lib.rs | 192 +++++++++++++----- crates/openshell-driver-docker/src/tests.rs | 95 +++++++++ crates/openshell-driver-podman/src/client.rs | 4 + crates/openshell-driver-podman/src/watcher.rs | 44 +++- crates/openshell-sandbox/src/delegated.rs | 2 +- crates/openshell-supervisor/src/main.rs | 44 +++- 7 files changed, 337 insertions(+), 57 deletions(-) diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index 71fb790b11..dee215e447 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -58,11 +58,22 @@ pub const CONDITION_WORKSPACE_VALIDATION_FAILED: &str = "WorkspaceValidationFail /// Supervisor exit status reserved for OCI workspace validation failures. /// -/// Local container drivers translate this status into +/// Local container drivers translate a supervisor exit with this status into /// [`CONDITION_WORKSPACE_VALIDATION_FAILED`] so users receive the specific /// provisioning failure rather than a generic container exit. pub const SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED: i32 = 78; +/// Error context for a rejected image-provided OCI working directory. +/// +/// The supervisor recognizes it in a failed agent start and exits with +/// [`SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED`]. +pub const WORKSPACE_VALIDATION_ERROR_CONTEXT: &str = "image workspace validation failed"; + +/// Driver condition message for [`CONDITION_WORKSPACE_VALIDATION_FAILED`]. +/// Fixed text, because supervisor output may contain secrets. +pub const WORKSPACE_VALIDATION_FAILED_MESSAGE: &str = + "OCI WorkingDir is not usable by the sandbox identity"; + /// Ready-condition reason when a container was terminated by an external signal. /// /// SIGKILL/SIGTERM (exit 137/143) is what a Podman/Docker machine or daemon diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 0958763730..8a27f3ca94 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -12,9 +12,9 @@ use bollard::Docker; use bollard::errors::Error as BollardError; use bollard::models::{ ContainerCreateBody, ContainerState, ContainerStateStatusEnum, ContainerSummary, - ContainerSummaryStateEnum, CreateImageInfo, DeviceRequest, HealthConfig, HealthStatusEnum, - HostConfig, Mount, MountTmpfsOptions, MountTypeEnum, MountVolumeOptions, ProgressDetail, - SystemInfo, VolumeCreateRequest, + ContainerSummaryStateEnum, ContainerWaitResponse, CreateImageInfo, DeviceRequest, HealthConfig, + HealthStatusEnum, HostConfig, Mount, MountTmpfsOptions, MountTypeEnum, MountVolumeOptions, + ProgressDetail, SystemInfo, VolumeCreateRequest, }; use bollard::query_parameters::{ CreateContainerOptionsBuilder, CreateImageOptions, DownloadFromContainerOptionsBuilder, @@ -26,10 +26,12 @@ use futures::{Stream, StreamExt}; use openshell_core::config::DEFAULT_STOP_TIMEOUT_SECS; use openshell_core::driver_mounts; use openshell_core::driver_utils::{ - CONDITION_EXITED, CONDITION_RUNTIME_RESTART, LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, - LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME, LABEL_SANDBOX_NAMESPACE, LABEL_SANDBOX_WORKSPACE, - SANDBOX_RUNTIME_IMAGE_BINARY_PATH, extract_first_tar_entry, supervisor_image_should_refresh, - temp_extract_container_name, validate_linux_elf_binary, + CONDITION_EXITED, CONDITION_RUNTIME_RESTART, CONDITION_WORKSPACE_VALIDATION_FAILED, + LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME, + LABEL_SANDBOX_NAMESPACE, LABEL_SANDBOX_WORKSPACE, SANDBOX_RUNTIME_IMAGE_BINARY_PATH, + SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED, WORKSPACE_VALIDATION_FAILED_MESSAGE, + extract_first_tar_entry, supervisor_image_should_refresh, temp_extract_container_name, + validate_linux_elf_binary, }; use openshell_core::gpu::{ CdiGpuDefaultSelector, CdiGpuInventory, CdiGpuSelectionError, driver_gpu_requirements, @@ -1863,7 +1865,7 @@ impl DockerComputeDriver { .await; cleanup_docker_boundary_state(sandbox, &self.config); return Err(DockerProvisioningFailure::new( - "ControlSupervisorStartFailed", + supervisor_start_failure_reason(&status, "ControlSupervisorStartFailed"), status.message(), )); } @@ -2155,7 +2157,7 @@ impl DockerComputeDriver { Err(status) => { handle_docker_runtime_failure( failure_context, - "ControlSupervisorExited", + supervisor_start_failure_reason(&status, "ControlSupervisorExited"), format!( "failed to start Docker control supervisor: {}", status.message() @@ -5238,7 +5240,9 @@ async fn spawn_docker_control_process( let supervisor_id = created.id; let monitored_supervisor_id = supervisor_id.clone(); let monitored_docker = failure_context.docker.clone(); - let readiness_sandbox_id = failure_context.container_id.clone(); + let readiness_container_id = failure_context.container_id.clone(); + let readiness_sandbox_id = sandbox_id.clone(); + let readiness_failures = failure_context.failures.clone(); let task = tokio::spawn(async move { let wait = async { let mut stream = monitored_docker.wait_container( @@ -5282,26 +5286,13 @@ async fn spawn_docker_control_process( ).await; return; } - let mut message = match result { - Some(Ok(status)) => { - warn!(%sandbox_id, status = status.status_code, "Docker supervisor container exited unexpectedly"); - format!("Docker supervisor container exited with status {}", status.status_code) - } - Some(Err(error)) => { - warn!(%sandbox_id, %error, "Failed to wait for Docker supervisor container"); - format!("failed to wait for Docker supervisor container: {error}") - } - None => "Docker supervisor wait stream ended unexpectedly".to_string(), - }; + let (reason, mut message) = docker_supervisor_wait_failure(result); + warn!(%sandbox_id, %reason, %message, "Docker supervisor container exited unexpectedly"); let log_tail = docker_container_log_tail(&monitored_docker, &monitored_supervisor_id).await; if !log_tail.is_empty() { write!(message, "; log tail: {log_tail}").ok(); } - let _ = monitored_docker.remove_container( - &monitored_supervisor_id, - Some(RemoveContainerOptionsBuilder::default().force(true).build()), - ).await; // The gateway can close the supervisor session as soon as it // commits Stopping. Re-check after collecting diagnostics so // an overlapping driver stop cannot be published as an @@ -5309,12 +5300,13 @@ async fn spawn_docker_control_process( if monitored_shutdown.load(Ordering::Acquire) { return; } - handle_docker_runtime_failure( - failure_context, - "ControlSupervisorExited", - message, - ) - .await; + handle_docker_runtime_failure(failure_context, reason, message).await; + // Publish the reason before removing the container. Readiness + // may inspect it after cleanup and must still see exit 78. + let _ = monitored_docker.remove_container( + &monitored_supervisor_id, + Some(RemoveContainerOptionsBuilder::default().force(true).build()), + ).await; }, } }); @@ -5323,8 +5315,14 @@ async fn spawn_docker_control_process( intentional_shutdown, task, }; - if let Err(error) = - wait_for_docker_supervisor_ready(docker, &supervisor_id, &readiness_sandbox_id).await + if let Err(error) = Box::pin(wait_for_docker_supervisor_ready( + docker, + &supervisor_id, + &readiness_container_id, + &readiness_sandbox_id, + &readiness_failures, + )) + .await { stop_docker_control_process(process).await; return Err(error); @@ -5335,33 +5333,60 @@ async fn spawn_docker_control_process( async fn wait_for_docker_supervisor_ready( docker: &Docker, supervisor_id: &str, + workload_container_id: &str, sandbox_id: &str, + failures: &Arc>>, ) -> Result<(), Status> { // Gateway provisioning deadlines own startup expiration, including policy // repairs that extend the window. Unhealthy means not ready while the // supervisor is quarantined; only a stopped process is a startup failure. loop { - let sandbox = docker - .inspect_container(sandbox_id, None) - .await - .map_err(|error| { - Status::internal(format!("inspect Docker sandbox container: {error}")) - })?; + if let Some(status) = recorded_workspace_validation_failure(failures, sandbox_id).await { + return Err(status); + } + let sandbox = match docker.inspect_container(workload_container_id, None).await { + Ok(sandbox) => sandbox, + Err(error) => { + return Err(recorded_workspace_validation_failure(failures, sandbox_id) + .await + .unwrap_or_else(|| { + Status::internal(format!("inspect Docker sandbox container: {error}")) + })); + } + }; if sandbox.state.unwrap_or_default().running == Some(false) { - return Err(Status::unavailable( - "Docker sandbox exited before supervisor became ready", - )); + return Err(recorded_workspace_validation_failure(failures, sandbox_id) + .await + .unwrap_or_else(|| { + Status::unavailable("Docker sandbox exited before supervisor became ready") + })); } - let inspected = docker - .inspect_container(supervisor_id, None) - .await - .map_err(|error| { - Status::internal(format!("inspect Docker supervisor container: {error}")) - })?; + let inspected = match docker.inspect_container(supervisor_id, None).await { + Ok(inspected) => inspected, + Err(error) => { + return Err(recorded_workspace_validation_failure(failures, sandbox_id) + .await + .unwrap_or_else(|| { + Status::internal(format!("inspect Docker supervisor container: {error}")) + })); + } + }; let state = inspected.state.unwrap_or_default(); match state.health.and_then(|health| health.status) { Some(HealthStatusEnum::HEALTHY) => return Ok(()), + _ if state.running == Some(false) + && state.exit_code + == Some(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)) => + { + let log_tail = docker_container_log_tail(docker, supervisor_id).await; + return Err(workspace_validation_status(&log_tail)); + } _ if state.running == Some(false) => { + if let Some(status) = + recorded_workspace_validation_failure(failures, sandbox_id).await + { + return Err(status); + } let log_tail = docker_container_log_tail(docker, supervisor_id).await; warn!(sandbox_id, supervisor_id, supervisor_logs = %log_tail, "Docker supervisor exited before becoming ready"); return Err(Status::unavailable(format!( @@ -5374,6 +5399,77 @@ async fn wait_for_docker_supervisor_ready( } } +/// Bollard reports nonzero Docker wait statuses as errors, not successful +/// responses. Both forms must preserve the sandbox's workspace failure reason. +fn docker_supervisor_wait_failure( + result: Option>, +) -> (&'static str, String) { + match result { + Some(Ok(status)) => ( + supervisor_exit_reason(status.status_code), + format!( + "Docker supervisor container exited with status {}", + status.status_code + ), + ), + Some(Err(BollardError::DockerContainerWaitError { code, error })) => ( + supervisor_exit_reason(code), + format!("Docker supervisor container exited with status {code}: {error}"), + ), + Some(Err(error)) => ( + "ControlSupervisorExited", + format!("failed to wait for Docker supervisor container: {error}"), + ), + None => ( + "ControlSupervisorExited", + "Docker supervisor wait stream ended unexpectedly".to_string(), + ), + } +} + +async fn recorded_workspace_validation_failure( + failures: &Arc>>, + sandbox_id: &str, +) -> Option { + let failures = failures.lock().await; + failures + .get(sandbox_id) + .filter(|failure| failure.reason == CONDITION_WORKSPACE_VALIDATION_FAILED) + .map(|failure| workspace_validation_status(&failure.message)) +} + +/// Startup failure for a supervisor that exited because the sandbox rejected +/// the image working directory. The fixed message prefix identifies it for +/// [`supervisor_start_failure_reason`]. +fn workspace_validation_status(log_tail: &str) -> Status { + Status::failed_precondition(format!( + "{WORKSPACE_VALIDATION_FAILED_MESSAGE}{}", + format_log_tail(log_tail) + )) +} + +/// Condition reason for a supervisor that failed before becoming ready. +fn supervisor_start_failure_reason(status: &Status, default: &'static str) -> &'static str { + if status.code() == tonic::Code::FailedPrecondition + && status + .message() + .starts_with(WORKSPACE_VALIDATION_FAILED_MESSAGE) + { + CONDITION_WORKSPACE_VALIDATION_FAILED + } else { + default + } +} + +/// Condition reason for a supervisor that exited with `status_code`. +fn supervisor_exit_reason(status_code: i64) -> &'static str { + if status_code == i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED) { + CONDITION_WORKSPACE_VALIDATION_FAILED + } else { + "ControlSupervisorExited" + } +} + fn format_log_tail(log_tail: &str) -> String { // gRPC status messages travel in HTTP/2 headers. A full 16 KiB container // tail can exceed the client's 16 KiB header budget and hide the real diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index e0e1b5237b..ebc87f88aa 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -3653,6 +3653,101 @@ fn docker_oom_kill_stays_terminal_despite_137() { assert_eq!(ready_reason(&sandbox), CONDITION_EXITED); } +#[test] +fn supervisor_workspace_validation_exit_is_reported_explicitly() { + let status = workspace_validation_status("image workspace validation failed: denied"); + assert!(status.message().contains("log tail: image workspace")); + assert_eq!( + supervisor_start_failure_reason(&status, "ControlSupervisorStartFailed"), + CONDITION_WORKSPACE_VALIDATION_FAILED + ); + assert_eq!( + supervisor_start_failure_reason( + &Status::failed_precondition("provider SPIFFE socket has no parent directory"), + "ControlSupervisorStartFailed" + ), + "ControlSupervisorStartFailed" + ); + assert_eq!( + supervisor_exit_reason(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)), + CONDITION_WORKSPACE_VALIDATION_FAILED + ); + assert_eq!(supervisor_exit_reason(1), "ControlSupervisorExited"); + + let (reason, message) = + docker_supervisor_wait_failure(Some(Err(BollardError::DockerContainerWaitError { + code: i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED), + error: "image workspace validation failed".into(), + }))); + assert_eq!(reason, CONDITION_WORKSPACE_VALIDATION_FAILED); + assert!(message.contains("status 78")); + assert_eq!( + docker_supervisor_wait_failure(Some(Err(BollardError::DockerContainerWaitError { + code: 1, + error: String::new(), + }))) + .0, + "ControlSupervisorExited" + ); +} + +#[tokio::test] +async fn docker_wait_exit_78_keeps_workspace_failure_reason() { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + let (mut socket, _) = listener.accept().await.unwrap(); + let mut request = [0; 4096]; + let size = socket.read(&mut request).await.unwrap(); + assert!(String::from_utf8_lossy(&request[..size]).contains("/containers/supervisor/wait")); + let body = r#"{"StatusCode":78,"Error":null}"#; + let response = format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + socket.write_all(response.as_bytes()).await.unwrap(); + }); + let docker = + Docker::connect_with_http(&address.to_string(), 5, bollard::API_DEFAULT_VERSION).unwrap(); + let mut wait = docker.wait_container( + "supervisor", + None::, + ); + let (reason, message) = docker_supervisor_wait_failure(wait.next().await); + assert_eq!(reason, CONDITION_WORKSPACE_VALIDATION_FAILED); + assert!(message.contains("status 78")); + server.await.unwrap(); +} + +#[tokio::test] +async fn workspace_failure_survives_supervisor_cleanup_before_readiness_inspection() { + let failures = Arc::new(Mutex::new(HashMap::from([( + "sandbox-1".to_string(), + DockerRuntimeFailure { + reason: CONDITION_WORKSPACE_VALIDATION_FAILED, + message: "Docker supervisor container exited with status 78".into(), + }, + )]))); + // There is no Docker daemon at this address: a monitor that has removed + // the supervisor must not need another inspection to retain the reason. + let docker = Docker::connect_with_http("127.0.0.1:1", 1, bollard::API_DEFAULT_VERSION).unwrap(); + let status = wait_for_docker_supervisor_ready( + &docker, + "removed-supervisor", + "workload-container-1", + "sandbox-1", + &failures, + ) + .await + .expect_err("the recorded workspace failure takes precedence over inspection"); + assert_eq!( + supervisor_start_failure_reason(&status, "ControlSupervisorStartFailed"), + CONDITION_WORKSPACE_VALIDATION_FAILED + ); +} + #[test] fn concurrent_container_removal_is_idempotent() { let removing = BollardError::DockerResponseServerError { diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 96b483a2da..1cf017ab19 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -121,6 +121,10 @@ pub struct ContainerState { /// container-log marker. It is never deserialized from Podman. #[serde(skip)] pub startup_diagnostic: Option, + /// Exit status of the sandbox's supervisor companion when it stopped + /// before the workload. It is never deserialized from Podman. + #[serde(skip)] + pub supervisor_exit_code: Option, } #[derive(Debug, Clone, serde::Deserialize)] diff --git a/crates/openshell-driver-podman/src/watcher.rs b/crates/openshell-driver-podman/src/watcher.rs index d7364d4f6c..27c459c72c 100644 --- a/crates/openshell-driver-podman/src/watcher.rs +++ b/crates/openshell-driver-podman/src/watcher.rs @@ -29,6 +29,7 @@ const CONDITION_STARTING: &str = "ContainerStarting"; use openshell_core::driver_utils::{ CONDITION_EXITED, CONDITION_RUNTIME_RESTART, CONDITION_STOPPED, CONDITION_WORKSPACE_VALIDATION_FAILED, SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED, + WORKSPACE_VALIDATION_FAILED_MESSAGE, }; pub type WatchStream = @@ -390,7 +391,12 @@ pub async fn inspect_workload( // Both containers exist before initial start. A missing or exited // companion therefore requires containment, including after a // gateway restart that missed the original Podman exit event. - Ok(_) | Err(PodmanApiError::NotFound(_)) => { + Ok(supervisor) => { + client.stop_container(&workload.id, 0).await?; + workload = client.inspect_container(&workload.id).await?; + workload.state.supervisor_exit_code = exited_exit_code(&supervisor.state); + } + Err(PodmanApiError::NotFound(_)) => { client.stop_container(&workload.id, 0).await?; workload = client.inspect_container(&workload.id).await?; } @@ -402,10 +408,17 @@ pub async fn inspect_workload( .await .ok() .and_then(|logs| boundary_startup_termination_marker(&logs)); + workload.state.supervisor_exit_code = supervisor + .ok() + .and_then(|supervisor| exited_exit_code(&supervisor.state)); } Ok(workload) } +fn exited_exit_code(state: &ContainerState) -> Option { + matches!(state.status.as_str(), "exited" | "stopped").then_some(state.exit_code) +} + /// Extract only fixed, OpenShell-owned startup diagnostics from container /// output. Workload and supervisor output may contain secrets, so it must not /// be propagated to driver conditions or tracing. @@ -553,10 +566,12 @@ fn condition_from_state(state: &ContainerState) -> DriverCondition { "OOMKilled", "Container was killed by the OOM killer".to_string(), ) - } else if state.exit_code == i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED) { + } else if state.supervisor_exit_code + == Some(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)) + { ( CONDITION_WORKSPACE_VALIDATION_FAILED, - "OCI WorkingDir is not usable by the sandbox identity".to_string(), + WORKSPACE_VALIDATION_FAILED_MESSAGE.to_string(), ) } else if matches!(state.exit_code, 137 | 143) { ( @@ -692,6 +707,7 @@ mod tests { health: None, started_at: Some("2026-08-12T16:38:58Z".to_string()), finished_at: Some("2026-08-12T16:39:13Z".to_string()), + supervisor_exit_code: None, startup_diagnostic: None, }; @@ -741,6 +757,7 @@ mod tests { }), started_at: Some("2026-04-14T10:00:00Z".to_string()), finished_at: None, + supervisor_exit_code: None, startup_diagnostic: None, }; let cond = condition_from_state(&state); @@ -760,6 +777,7 @@ mod tests { health: None, started_at: Some("2026-04-14T10:00:00Z".to_string()), finished_at: None, + supervisor_exit_code: None, startup_diagnostic: None, }; let cond = condition_from_state(&state); @@ -782,6 +800,7 @@ mod tests { }), started_at: Some("2026-04-14T10:00:00Z".to_string()), finished_at: None, + supervisor_exit_code: None, startup_diagnostic: None, }; let condition = condition_from_state(&state); @@ -800,6 +819,7 @@ mod tests { health: None, started_at: None, finished_at: Some("2026-04-14T11:00:00Z".to_string()), + supervisor_exit_code: None, startup_diagnostic: None, }; let cond = condition_from_state(&state); @@ -818,6 +838,7 @@ mod tests { health: None, started_at: None, finished_at: Some("2026-04-14T12:00:00Z".to_string()), + supervisor_exit_code: None, startup_diagnostic: None, }; let cond = condition_from_state(&state); @@ -836,6 +857,7 @@ mod tests { health: None, started_at: None, finished_at: Some("2026-04-14T12:00:00Z".to_string()), + supervisor_exit_code: None, startup_diagnostic: boundary_startup_termination_marker( b"untrusted workload output\nsandbox boundary received SIGTERM before supervisor confirmation\n", ), @@ -860,21 +882,30 @@ mod tests { #[test] fn condition_workspace_validation_exit_is_reported_explicitly() { - let state = ContainerState { + // The supervisor exits with the reserved status and the watcher then + // stops the workload, so the workload itself reports SIGKILL. + let mut state = ContainerState { status: "exited".to_string(), running: false, - exit_code: i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED), + exit_code: 137, oom_killed: false, health: None, started_at: None, finished_at: Some("2026-04-14T12:00:00Z".to_string()), + supervisor_exit_code: Some(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)), startup_diagnostic: None, }; let cond = condition_from_state(&state); assert_eq!(cond.reason, CONDITION_WORKSPACE_VALIDATION_FAILED); - assert!(cond.message.contains("WorkingDir")); + assert_eq!(cond.message, WORKSPACE_VALIDATION_FAILED_MESSAGE); + + // A workload's own exit with the reserved status is not a supervisor + // workspace rejection. + state.exit_code = i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED); + state.supervisor_exit_code = Some(1); + assert_eq!(condition_from_state(&state).reason, CONDITION_EXITED); } #[test] @@ -892,6 +923,7 @@ mod tests { health: None, started_at: None, finished_at: Some("2026-04-14T12:30:00Z".to_string()), + supervisor_exit_code: None, startup_diagnostic: None, }; let cond = condition_from_state(&state); diff --git a/crates/openshell-sandbox/src/delegated.rs b/crates/openshell-sandbox/src/delegated.rs index f594fa35f2..f3493769b8 100644 --- a/crates/openshell-sandbox/src/delegated.rs +++ b/crates/openshell-sandbox/src/delegated.rs @@ -54,7 +54,7 @@ pub async fn spawn_workload( crate::process::validate_oci_workspace_as_effective_identity(std::path::Path::new( workspace_root, )) - .wrap_err("image workspace validation failed")?; + .wrap_err(openshell_core::driver_utils::WORKSPACE_VALIDATION_ERROR_CONTEXT)?; } #[cfg(target_os = "linux")] diff --git a/crates/openshell-supervisor/src/main.rs b/crates/openshell-supervisor/src/main.rs index 0d86964820..a876dad9f4 100644 --- a/crates/openshell-supervisor/src/main.rs +++ b/crates/openshell-supervisor/src/main.rs @@ -10,7 +10,7 @@ use std::sync::atomic::AtomicBool; use clap::{Parser, ValueEnum}; use miette::{IntoDiagnostic, Result}; use openshell_isolation_interface::contract::BackendDescriptor; -use tracing::{info, warn}; +use tracing::{error, info, warn}; use tracing_subscriber::EnvFilter; use tracing_subscriber::filter::LevelFilter; use tracing_subscriber::{Layer as _, layer::SubscriberExt as _, util::SubscriberInitExt as _}; @@ -446,6 +446,7 @@ fn main() -> Result<()> { args.main_exit_marker, )) .await + .or_else(workspace_validation_exit) } SupervisorRole::NetworkProxy => { let listen = args.listen.unwrap_or_else(|| ([127, 0, 0, 1], 3128).into()); @@ -486,6 +487,24 @@ fn otlp_span_filter(log_level: &str) -> EnvFilter { EnvFilter::new(format!("info,openshell={level}")) } +/// Exit with the reserved status when the sandbox rejects the image working +/// directory, so the compute driver can report the specific failure. +fn workspace_validation_exit(error: miette::Report) -> Result { + use openshell_core::driver_utils::{ + SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED, WORKSPACE_VALIDATION_ERROR_CONTEXT, + }; + // Display keeps the message on one line; Debug may wrap it. + if !error + .to_string() + .contains(WORKSPACE_VALIDATION_ERROR_CONTEXT) + { + return Err(error); + } + error!("Image workspace validation failed"); + eprintln!("{error:?}"); + Ok(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED) +} + #[cfg(test)] mod tests { use super::*; @@ -499,6 +518,29 @@ mod tests { assert_eq!(directives("info,h2=debug"), "openshell=info,info"); } + #[test] + fn workspace_validation_failure_exits_with_reserved_status() { + use miette::WrapErr as _; + use openshell_core::driver_utils::{ + SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED, WORKSPACE_VALIDATION_ERROR_CONTEXT, + }; + // Mirror how the sandbox reports the rejection across the boundary. + let sandbox_error = Err::<(), _>(miette::miette!( + "workspace path component '/workspace/project' is not writable by the sandbox \ + identity in the image: Permission denied (os error 13)" + )) + .wrap_err(WORKSPACE_VALIDATION_ERROR_CONTEXT) + .unwrap_err(); + let error = miette::miette!( + "process error: boundary process leaf: start process supervisor leaf: {sandbox_error:?}" + ); + assert_eq!( + workspace_validation_exit(error).expect("reserved exit status"), + SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED + ); + assert!(workspace_validation_exit(miette::miette!("connect failed")).is_err()); + } + #[test] fn isolation_backend_is_the_default_role() { let directory = tempfile::tempdir().expect("temporary runtime descriptor directory"); From 2b1c5d209912a003a70a9cc3ab1061b11754c2e5 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Thu, 8 Oct 2026 09:08:54 -0700 Subject: [PATCH 08/16] test(oci): require WorkspaceValidationFailed for unwritable WORKDIR Unrelated provisioning failures can no longer pass the rejection scenario. Signed-off-by: Matthew Grossman --- .../features/oci-image/tests/oci_image.rs | 81 +++---------------- 1 file changed, 11 insertions(+), 70 deletions(-) diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index 175357c179..ffba58d1b6 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -22,6 +22,9 @@ const BASE_IMAGE: &str = "nvcr.io/nvidia/base/ubuntu:24.04"; const ENGINE_ENV: &str = "OPENSHELL_TEST_CONTAINER_ENGINE"; const CREATE_TIMEOUT: Duration = Duration::from_mins(10); const COMMAND_TIMEOUT: Duration = Duration::from_mins(2); +/// Sandbox condition reason for an image `WORKDIR` the sandbox identity +/// cannot use. +const WORKSPACE_VALIDATION_FAILED: &str = "WorkspaceValidationFailed"; /// A complete sandbox policy without a `process` section, so the sandbox /// identity falls back to the image `USER`. const IMAGE_IDENTITY_POLICY: &str = "version: 1 @@ -127,9 +130,8 @@ async fn default_workdir_uses_managed_workspace() { } /// OpenShell rejects a custom `WORKDIR` that the image user cannot write -/// instead of granting the user new access to it. Drivers may surface the -/// rejection through different startup diagnostics rather than one condition -/// reason. +/// instead of granting the user new access to it, and reports that reason +/// rather than a generic startup failure. #[tokio::test] async fn unwritable_custom_workdir_is_rejected() { run("oci-image/unwritable-workdir", async |runner, images| { @@ -190,11 +192,12 @@ USER app create.failure_diagnostic("sandbox creation fails before the command runs") ); } - let diagnostic = format!("{}\n{}", create.stdout(), create.stderr()); - if !has_workspace_rejection_diagnostic(&diagnostic) { - return Err(create.failure_diagnostic( - "sandbox creation reports a workspace, permission, or workload startup rejection", - )); + if !create.stdout().contains(WORKSPACE_VALIDATION_FAILED) + && !create.stderr().contains(WORKSPACE_VALIDATION_FAILED) + { + return Err(create.failure_diagnostic(&format!( + "sandbox creation fails with {WORKSPACE_VALIDATION_FAILED}" + ))); } } Ok(()) @@ -202,68 +205,6 @@ USER app .await; } -fn has_workspace_rejection_diagnostic(diagnostic: &str) -> bool { - let diagnostic = diagnostic.to_ascii_lowercase(); - // A successful control does not excuse a later connectivity or image-pull - // failure. Those are not evidence of workspace rejection, even if their - // messages happen to mention the workspace. - if [ - "connection refused", - "connection reset", - "imagepull", - "image pull", - "failed to pull", - "pull access denied", - "manifest unknown", - "no such image", - ] - .iter() - .any(|message| diagnostic.contains(message)) - { - return false; - } - // The CLI may wrap the human message across lines with diagnostic gutters. - // Match the existing startup reason and exit detail independently. - if diagnostic.contains("containerexited") && diagnostic.contains("exited with code") { - return true; - } - [ - "workspace", - "workingdir", - "permission denied", - // Some drivers expose the rejected workload launch through SSH rather - // than propagating the runtime's workspace validation text. - "subsystem request failed", - ] - .iter() - .any(|message| diagnostic.contains(message)) -} - -#[test] -fn workspace_rejection_diagnostics_do_not_require_one_condition_reason() { - for diagnostic in [ - "image workspace validation failed", - "WorkingDir /workspace/project is not writable", - "Permission denied (os error 13)", - "ContainerExited: Container exited with code 1", - "Error: × sandbox entered error phase while provisioning: ContainerExited: Container\n │ exited with code 1", - "subsystem request failed", - ] { - assert!(has_workspace_rejection_diagnostic(diagnostic)); - } - for diagnostic in [ - "", - "gateway connection refused", - "image pull failed", - "image pull failed for workspace fixture", - "gateway connection refused while creating workspace", - "ImagePullFailed: failed to pull image for WorkingDir test", - "ContainerExited", - ] { - assert!(!has_workspace_rejection_diagnostic(diagnostic)); - } -} - async fn run( scenario: &str, test: impl AsyncFnOnce(&mut OpenShellRunner, &mut Vec) -> Result<(), String>, From 7fbdc2358bc9339c07505b2073b9d1031d63eb2d Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Thu, 8 Oct 2026 16:21:35 -0700 Subject: [PATCH 09/16] fix(oci): keep exit 78 reserved and report fixed workspace diagnostics The supervisor no longer passes on a workload's own exit status 78, so drivers cannot misreport it as a workspace rejection after a runtime restart. The rejection is recognized once at agent start and carried as a typed error. Docker reports the same fixed condition message as Podman and logs the supervisor output instead, and its monitor removes the supervisor container when a stop overlaps diagnostics collection. The OCI image suite registers images for cleanup only after they build. Signed-off-by: Matthew Grossman --- crates/openshell-driver-docker/src/lib.rs | 47 ++++++++-------- crates/openshell-driver-docker/src/tests.rs | 4 +- crates/openshell-supervisor/src/lib.rs | 29 +++++++++- crates/openshell-supervisor/src/main.rs | 55 +++++++++++++++---- .../features/oci-image/tests/oci_image.rs | 9 +-- 5 files changed, 102 insertions(+), 42 deletions(-) diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 8a27f3ca94..7fafba9e91 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -5244,6 +5244,12 @@ async fn spawn_docker_control_process( let readiness_sandbox_id = sandbox_id.clone(); let readiness_failures = failure_context.failures.clone(); let task = tokio::spawn(async move { + let remove_supervisor = || { + monitored_docker.remove_container( + &monitored_supervisor_id, + Some(RemoveContainerOptionsBuilder::default().force(true).build()), + ) + }; let wait = async { let mut stream = monitored_docker.wait_container( &monitored_supervisor_id, @@ -5258,17 +5264,11 @@ async fn spawn_docker_control_process( &monitored_supervisor_id, Some(StopContainerOptionsBuilder::default().t(5).build()), ).await; - let _ = monitored_docker.remove_container( - &monitored_supervisor_id, - Some(RemoveContainerOptionsBuilder::default().force(true).build()), - ).await; + let _ = remove_supervisor().await; } result = wait => { if monitored_shutdown.load(Ordering::Acquire) { - let _ = monitored_docker.remove_container( - &monitored_supervisor_id, - Some(RemoveContainerOptionsBuilder::default().force(true).build()), - ).await; + let _ = remove_supervisor().await; return; } if matches!( @@ -5280,17 +5280,17 @@ async fn spawn_docker_control_process( Ok(Ok(())), ) || monitored_shutdown.load(Ordering::Acquire) { - let _ = monitored_docker.remove_container( - &monitored_supervisor_id, - Some(RemoveContainerOptionsBuilder::default().force(true).build()), - ).await; + let _ = remove_supervisor().await; return; } let (reason, mut message) = docker_supervisor_wait_failure(result); warn!(%sandbox_id, %reason, %message, "Docker supervisor container exited unexpectedly"); let log_tail = docker_container_log_tail(&monitored_docker, &monitored_supervisor_id).await; - if !log_tail.is_empty() { + if reason == CONDITION_WORKSPACE_VALIDATION_FAILED { + warn!(%sandbox_id, supervisor_logs = %log_tail, "Docker sandbox rejected the image working directory"); + message = WORKSPACE_VALIDATION_FAILED_MESSAGE.to_string(); + } else if !log_tail.is_empty() { write!(message, "; log tail: {log_tail}").ok(); } // The gateway can close the supervisor session as soon as it @@ -5298,15 +5298,13 @@ async fn spawn_docker_control_process( // an overlapping driver stop cannot be published as an // unexpected control failure. if monitored_shutdown.load(Ordering::Acquire) { + let _ = remove_supervisor().await; return; } handle_docker_runtime_failure(failure_context, reason, message).await; // Publish the reason before removing the container. Readiness // may inspect it after cleanup and must still see exit 78. - let _ = monitored_docker.remove_container( - &monitored_supervisor_id, - Some(RemoveContainerOptionsBuilder::default().force(true).build()), - ).await; + let _ = remove_supervisor().await; }, } }); @@ -5379,7 +5377,8 @@ async fn wait_for_docker_supervisor_ready( == Some(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)) => { let log_tail = docker_container_log_tail(docker, supervisor_id).await; - return Err(workspace_validation_status(&log_tail)); + warn!(sandbox_id, supervisor_id, supervisor_logs = %log_tail, "Docker sandbox rejected the image working directory"); + return Err(workspace_validation_status()); } _ if state.running == Some(false) => { if let Some(status) = @@ -5435,17 +5434,15 @@ async fn recorded_workspace_validation_failure( failures .get(sandbox_id) .filter(|failure| failure.reason == CONDITION_WORKSPACE_VALIDATION_FAILED) - .map(|failure| workspace_validation_status(&failure.message)) + .map(|_| workspace_validation_status()) } /// Startup failure for a supervisor that exited because the sandbox rejected -/// the image working directory. The fixed message prefix identifies it for +/// the image working directory. The message is fixed text, because supervisor +/// output may contain secrets; the fixed message identifies the failure for /// [`supervisor_start_failure_reason`]. -fn workspace_validation_status(log_tail: &str) -> Status { - Status::failed_precondition(format!( - "{WORKSPACE_VALIDATION_FAILED_MESSAGE}{}", - format_log_tail(log_tail) - )) +fn workspace_validation_status() -> Status { + Status::failed_precondition(WORKSPACE_VALIDATION_FAILED_MESSAGE) } /// Condition reason for a supervisor that failed before becoming ready. diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index ebc87f88aa..154b316d1a 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -3655,8 +3655,8 @@ fn docker_oom_kill_stays_terminal_despite_137() { #[test] fn supervisor_workspace_validation_exit_is_reported_explicitly() { - let status = workspace_validation_status("image workspace validation failed: denied"); - assert!(status.message().contains("log tail: image workspace")); + let status = workspace_validation_status(); + assert_eq!(status.message(), WORKSPACE_VALIDATION_FAILED_MESSAGE); assert_eq!( supervisor_start_failure_reason(&status, "ControlSupervisorStartFailed"), CONDITION_WORKSPACE_VALIDATION_FAILED diff --git a/crates/openshell-supervisor/src/lib.rs b/crates/openshell-supervisor/src/lib.rs index 8d498e162d..1ed5537d4f 100644 --- a/crates/openshell-supervisor/src/lib.rs +++ b/crates/openshell-supervisor/src/lib.rs @@ -65,6 +65,33 @@ use openshell_ocsf::{ /// `run_sandbox()` startup via `openshell_ocsf::ctx::set_ctx`. pub(crate) use openshell_ocsf::ctx::ctx as ocsf_ctx; +/// The sandbox rejected the image working directory while starting the agent. +#[derive(Debug)] +pub struct WorkspaceValidationFailed(String); + +impl std::fmt::Display for WorkspaceValidationFailed { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +impl std::error::Error for WorkspaceValidationFailed {} + +impl miette::Diagnostic for WorkspaceValidationFailed {} + +/// Convert an agent start failure into a report. +/// +/// The sandbox reports errors across the boundary as text, so a workspace +/// rejection is recognized here, once, and carried as +/// [`WorkspaceValidationFailed`] from then on. +pub fn start_agent_error(message: String) -> miette::Report { + if message.contains(openshell_core::driver_utils::WORKSPACE_VALIDATION_ERROR_CONTEXT) { + miette::Report::new(WorkspaceValidationFailed(message)) + } else { + miette::miette!(message) + } +} + async fn retain_remote_access_plane( proxy_exited: impl Future, shutdown_requested: impl Future, @@ -1241,7 +1268,7 @@ async fn run_sandbox_with_backend( .start_agent() .instrument(tracing::info_span!(parent: &startup, "supervisor.boundary.start_agent")) .await - .map_err(|error| miette::miette!(error.to_string()))?; + .map_err(|error| start_agent_error(error.to_string()))?; workload_started_tx.send_replace(true); info!(backend = %backend_name, "Isolation boundary agent started"); let agent = running.agent(); diff --git a/crates/openshell-supervisor/src/main.rs b/crates/openshell-supervisor/src/main.rs index a876dad9f4..5ffac8be41 100644 --- a/crates/openshell-supervisor/src/main.rs +++ b/crates/openshell-supervisor/src/main.rs @@ -446,6 +446,7 @@ fn main() -> Result<()> { args.main_exit_marker, )) .await + .map(supervisor_exit_status) .or_else(workspace_validation_exit) } SupervisorRole::NetworkProxy => { @@ -490,13 +491,10 @@ fn otlp_span_filter(log_level: &str) -> EnvFilter { /// Exit with the reserved status when the sandbox rejects the image working /// directory, so the compute driver can report the specific failure. fn workspace_validation_exit(error: miette::Report) -> Result { - use openshell_core::driver_utils::{ - SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED, WORKSPACE_VALIDATION_ERROR_CONTEXT, - }; - // Display keeps the message on one line; Debug may wrap it. - if !error - .to_string() - .contains(WORKSPACE_VALIDATION_ERROR_CONTEXT) + use openshell_core::driver_utils::SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED; + if error + .downcast_ref::() + .is_none() { return Err(error); } @@ -505,6 +503,19 @@ fn workspace_validation_exit(error: miette::Report) -> Result { Ok(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED) } +/// The supervisor exits with the workload's status after the workload exits. +/// Keep the reserved workspace-validation status for workspace rejections, so +/// drivers do not misreport a workload that itself exited with it. The gateway +/// receives the workload's exact status separately. +fn supervisor_exit_status(workload_status: i32) -> i32 { + use openshell_core::driver_utils::SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED; + if workload_status == SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED { + 1 + } else { + workload_status + } +} + #[cfg(test)] mod tests { use super::*; @@ -531,14 +542,38 @@ mod tests { )) .wrap_err(WORKSPACE_VALIDATION_ERROR_CONTEXT) .unwrap_err(); - let error = miette::miette!( + let error = openshell_supervisor::start_agent_error(format!( "process error: boundary process leaf: start process supervisor leaf: {sandbox_error:?}" - ); + )); assert_eq!( workspace_validation_exit(error).expect("reserved exit status"), SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED ); - assert!(workspace_validation_exit(miette::miette!("connect failed")).is_err()); + assert!( + workspace_validation_exit(openshell_supervisor::start_agent_error( + "connect failed".into() + )) + .is_err() + ); + // Only an agent start failure carries the rejection, even when another + // error's text contains the same words. + assert!( + workspace_validation_exit(miette::miette!( + "policy sync failed: {WORKSPACE_VALIDATION_ERROR_CONTEXT}" + )) + .is_err() + ); + } + + #[test] + fn workload_exit_does_not_use_reserved_workspace_status() { + use openshell_core::driver_utils::SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED; + assert_eq!( + supervisor_exit_status(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED), + 1 + ); + assert_eq!(supervisor_exit_status(0), 0); + assert_eq!(supervisor_exit_status(143), 143); } #[test] diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index ffba58d1b6..17f99f4e6b 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -494,11 +494,10 @@ impl TestImage { let file = context.path().join("Containerfile"); std::fs::write(&file, containerfile) .map_err(|error| format!("write Containerfile: {error}"))?; - images.push(Self { + let image = Self { engine, tag: format!("localhost/openshell-test-oci-{name}:{}", std::process::id()), - }); - let image = images.last().expect("registered image"); + }; image.engine_command(&[ "build", "--file", @@ -507,7 +506,9 @@ impl TestImage { &image.tag, context.path().to_str().ok_or("context path is not UTF-8")?, ])?; - Ok(image) + // A failed build leaves no tag to remove. + images.push(image); + Ok(images.last().expect("registered image")) } fn remove(&self) -> Result<(), String> { From d181609a7d0ae94a4b2a36c5d42e69f43722296a Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Fri, 9 Oct 2026 14:05:06 -0700 Subject: [PATCH 10/16] refactor(oci): simplify workspace failure handling and OCI image tests Report a recorded Docker workspace rejection once, where supervisor readiness fails, and match its fixed message exactly. Read the Podman supervisor exit status once per inspection. Choose the supervisor exit status in one function and derive the workspace error type. The OCI image suite shares step helpers, checks that a custom workspace does not also create /sandbox, and its playbook waits for a connected gateway and collects failure diagnostics through the shared role. Signed-off-by: Matthew Grossman --- Cargo.lock | 1 + crates/openshell-driver-docker/src/lib.rs | 72 +++--- crates/openshell-driver-podman/src/watcher.rs | 15 +- crates/openshell-supervisor/Cargo.toml | 1 + crates/openshell-supervisor/src/lib.rs | 13 +- crates/openshell-supervisor/src/main.rs | 74 +++--- .../ansible/playbooks/features/oci-image.yaml | 47 ++-- .../features/oci-image/tests/oci_image.rs | 218 ++++++++++-------- 8 files changed, 222 insertions(+), 219 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 35193cd8f4..24bc267288 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5030,6 +5030,7 @@ dependencies = [ "socket2", "temp-env", "tempfile", + "thiserror 2.0.20", "tokio", "tokio-tungstenite 0.26.2", "tonic", diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index a8dd66bff8..5cfa80007a 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -5334,41 +5334,46 @@ async fn wait_for_docker_supervisor_ready( workload_container_id: &str, sandbox_id: &str, failures: &Arc>>, +) -> Result<(), Status> { + // The monitor records a workspace rejection before it removes the + // supervisor, so a later failed inspection must still report it. + match poll_docker_supervisor_ready(docker, supervisor_id, workload_container_id, sandbox_id) + .await + { + Ok(()) => Ok(()), + Err(status) => Err(recorded_workspace_validation_failure(failures, sandbox_id) + .await + .unwrap_or(status)), + } +} + +async fn poll_docker_supervisor_ready( + docker: &Docker, + supervisor_id: &str, + workload_container_id: &str, + sandbox_id: &str, ) -> Result<(), Status> { // Gateway provisioning deadlines own startup expiration, including policy // repairs that extend the window. Unhealthy means not ready while the // supervisor is quarantined; only a stopped process is a startup failure. loop { - if let Some(status) = recorded_workspace_validation_failure(failures, sandbox_id).await { - return Err(status); - } - let sandbox = match docker.inspect_container(workload_container_id, None).await { - Ok(sandbox) => sandbox, - Err(error) => { - return Err(recorded_workspace_validation_failure(failures, sandbox_id) - .await - .unwrap_or_else(|| { - Status::internal(format!("inspect Docker sandbox container: {error}")) - })); - } - }; + let sandbox = docker + .inspect_container(workload_container_id, None) + .await + .map_err(|error| { + Status::internal(format!("inspect Docker sandbox container: {error}")) + })?; if sandbox.state.unwrap_or_default().running == Some(false) { - return Err(recorded_workspace_validation_failure(failures, sandbox_id) - .await - .unwrap_or_else(|| { - Status::unavailable("Docker sandbox exited before supervisor became ready") - })); + return Err(Status::unavailable( + "Docker sandbox exited before supervisor became ready", + )); } - let inspected = match docker.inspect_container(supervisor_id, None).await { - Ok(inspected) => inspected, - Err(error) => { - return Err(recorded_workspace_validation_failure(failures, sandbox_id) - .await - .unwrap_or_else(|| { - Status::internal(format!("inspect Docker supervisor container: {error}")) - })); - } - }; + let inspected = docker + .inspect_container(supervisor_id, None) + .await + .map_err(|error| { + Status::internal(format!("inspect Docker supervisor container: {error}")) + })?; let state = inspected.state.unwrap_or_default(); match state.health.and_then(|health| health.status) { Some(HealthStatusEnum::HEALTHY) => return Ok(()), @@ -5381,11 +5386,6 @@ async fn wait_for_docker_supervisor_ready( return Err(workspace_validation_status()); } _ if state.running == Some(false) => { - if let Some(status) = - recorded_workspace_validation_failure(failures, sandbox_id).await - { - return Err(status); - } let log_tail = docker_container_log_tail(docker, supervisor_id).await; warn!(sandbox_id, supervisor_id, supervisor_logs = %log_tail, "Docker supervisor exited before becoming ready"); return Err(Status::unavailable(format!( @@ -5439,7 +5439,7 @@ async fn recorded_workspace_validation_failure( /// Startup failure for a supervisor that exited because the sandbox rejected /// the image working directory. The message is fixed text, because supervisor -/// output may contain secrets; the fixed message identifies the failure for +/// output may contain secrets, and identifies the failure for /// [`supervisor_start_failure_reason`]. fn workspace_validation_status() -> Status { Status::failed_precondition(WORKSPACE_VALIDATION_FAILED_MESSAGE) @@ -5448,9 +5448,7 @@ fn workspace_validation_status() -> Status { /// Condition reason for a supervisor that failed before becoming ready. fn supervisor_start_failure_reason(status: &Status, default: &'static str) -> &'static str { if status.code() == tonic::Code::FailedPrecondition - && status - .message() - .starts_with(WORKSPACE_VALIDATION_FAILED_MESSAGE) + && status.message() == WORKSPACE_VALIDATION_FAILED_MESSAGE { CONDITION_WORKSPACE_VALIDATION_FAILED } else { diff --git a/crates/openshell-driver-podman/src/watcher.rs b/crates/openshell-driver-podman/src/watcher.rs index 27c459c72c..2163054a48 100644 --- a/crates/openshell-driver-podman/src/watcher.rs +++ b/crates/openshell-driver-podman/src/watcher.rs @@ -375,6 +375,10 @@ pub async fn inspect_workload( let supervisor = client .inspect_container(&crate::isolation::supervisor_name(sandbox_id)) .await; + let supervisor_exit_code = supervisor + .as_ref() + .ok() + .and_then(|supervisor| exited_exit_code(&supervisor.state)); if workload.state.running { match supervisor { Ok(supervisor) if supervisor.state.running => { @@ -391,12 +395,7 @@ pub async fn inspect_workload( // Both containers exist before initial start. A missing or exited // companion therefore requires containment, including after a // gateway restart that missed the original Podman exit event. - Ok(supervisor) => { - client.stop_container(&workload.id, 0).await?; - workload = client.inspect_container(&workload.id).await?; - workload.state.supervisor_exit_code = exited_exit_code(&supervisor.state); - } - Err(PodmanApiError::NotFound(_)) => { + Ok(_) | Err(PodmanApiError::NotFound(_)) => { client.stop_container(&workload.id, 0).await?; workload = client.inspect_container(&workload.id).await?; } @@ -408,10 +407,8 @@ pub async fn inspect_workload( .await .ok() .and_then(|logs| boundary_startup_termination_marker(&logs)); - workload.state.supervisor_exit_code = supervisor - .ok() - .and_then(|supervisor| exited_exit_code(&supervisor.state)); } + workload.state.supervisor_exit_code = supervisor_exit_code; Ok(workload) } diff --git a/crates/openshell-supervisor/Cargo.toml b/crates/openshell-supervisor/Cargo.toml index 04fe45b8d1..b44b963952 100644 --- a/crates/openshell-supervisor/Cargo.toml +++ b/crates/openshell-supervisor/Cargo.toml @@ -39,6 +39,7 @@ serde = { workspace = true } serde_json = { workspace = true } socket2 = { workspace = true } tempfile = "3" +thiserror = { workspace = true } tokio = { workspace = true } tonic = { workspace = true, features = ["channel", "tls-native-roots"] } tracing = { workspace = true } diff --git a/crates/openshell-supervisor/src/lib.rs b/crates/openshell-supervisor/src/lib.rs index 1ed5537d4f..be0fda01ca 100644 --- a/crates/openshell-supervisor/src/lib.rs +++ b/crates/openshell-supervisor/src/lib.rs @@ -66,19 +66,10 @@ use openshell_ocsf::{ pub(crate) use openshell_ocsf::ctx::ctx as ocsf_ctx; /// The sandbox rejected the image working directory while starting the agent. -#[derive(Debug)] +#[derive(Debug, thiserror::Error, miette::Diagnostic)] +#[error("{0}")] pub struct WorkspaceValidationFailed(String); -impl std::fmt::Display for WorkspaceValidationFailed { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(&self.0) - } -} - -impl std::error::Error for WorkspaceValidationFailed {} - -impl miette::Diagnostic for WorkspaceValidationFailed {} - /// Convert an agent start failure into a report. /// /// The sandbox reports errors across the boundary as text, so a workspace diff --git a/crates/openshell-supervisor/src/main.rs b/crates/openshell-supervisor/src/main.rs index 5ffac8be41..bfd657040a 100644 --- a/crates/openshell-supervisor/src/main.rs +++ b/crates/openshell-supervisor/src/main.rs @@ -423,7 +423,7 @@ fn main() -> Result<()> { }; let admitted_isolation_backend = std::env::var(openshell_core::sandbox_env::ADMITTED_ISOLATION_BACKEND).ok(); - Box::pin(openshell_supervisor::run_sandbox( + let result = Box::pin(openshell_supervisor::run_sandbox( command, workdir, args.timeout, @@ -445,9 +445,8 @@ fn main() -> Result<()> { admitted_isolation_backend, args.main_exit_marker, )) - .await - .map(supervisor_exit_status) - .or_else(workspace_validation_exit) + .await; + supervisor_exit(result) } SupervisorRole::NetworkProxy => { let listen = args.listen.unwrap_or_else(|| ([127, 0, 0, 1], 3128).into()); @@ -488,31 +487,27 @@ fn otlp_span_filter(log_level: &str) -> EnvFilter { EnvFilter::new(format!("info,openshell={level}")) } -/// Exit with the reserved status when the sandbox rejects the image working -/// directory, so the compute driver can report the specific failure. -fn workspace_validation_exit(error: miette::Report) -> Result { - use openshell_core::driver_utils::SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED; - if error - .downcast_ref::() - .is_none() - { - return Err(error); - } - error!("Image workspace validation failed"); - eprintln!("{error:?}"); - Ok(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED) -} - -/// The supervisor exits with the workload's status after the workload exits. -/// Keep the reserved workspace-validation status for workspace rejections, so -/// drivers do not misreport a workload that itself exited with it. The gateway -/// receives the workload's exact status separately. -fn supervisor_exit_status(workload_status: i32) -> i32 { +/// Choose the supervisor's exit status for a sandbox run. +/// +/// The reserved status tells the compute driver that the sandbox rejected the +/// image working directory. After the workload exits, the supervisor exits +/// with the workload's status, so a workload that itself exits with the +/// reserved status is reported as 1 instead. The gateway receives the +/// workload's exact status separately. +fn supervisor_exit(result: Result) -> Result { use openshell_core::driver_utils::SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED; - if workload_status == SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED { - 1 - } else { - workload_status + match result { + Ok(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED) => Ok(1), + Err(error) + if error + .downcast_ref::() + .is_some() => + { + error!("Image workspace validation failed"); + eprintln!("{error:?}"); + Ok(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED) + } + other => other, } } @@ -530,7 +525,7 @@ mod tests { } #[test] - fn workspace_validation_failure_exits_with_reserved_status() { + fn only_workspace_validation_failure_exits_with_reserved_status() { use miette::WrapErr as _; use openshell_core::driver_utils::{ SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED, WORKSPACE_VALIDATION_ERROR_CONTEXT, @@ -546,34 +541,31 @@ mod tests { "process error: boundary process leaf: start process supervisor leaf: {sandbox_error:?}" )); assert_eq!( - workspace_validation_exit(error).expect("reserved exit status"), + supervisor_exit(Err(error)).expect("reserved exit status"), SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED ); assert!( - workspace_validation_exit(openshell_supervisor::start_agent_error( + supervisor_exit(Err(openshell_supervisor::start_agent_error( "connect failed".into() - )) + ))) .is_err() ); // Only an agent start failure carries the rejection, even when another // error's text contains the same words. assert!( - workspace_validation_exit(miette::miette!( + supervisor_exit(Err(miette::miette!( "policy sync failed: {WORKSPACE_VALIDATION_ERROR_CONTEXT}" - )) + ))) .is_err() ); - } - #[test] - fn workload_exit_does_not_use_reserved_workspace_status() { - use openshell_core::driver_utils::SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED; + // A workload's own exit with the reserved status is not a rejection. assert_eq!( - supervisor_exit_status(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED), + supervisor_exit(Ok(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)).unwrap(), 1 ); - assert_eq!(supervisor_exit_status(0), 0); - assert_eq!(supervisor_exit_status(143), 143); + assert_eq!(supervisor_exit(Ok(0)).unwrap(), 0); + assert_eq!(supervisor_exit(Ok(143)).unwrap(), 143); } #[test] diff --git a/tests/ansible/playbooks/features/oci-image.yaml b/tests/ansible/playbooks/features/oci-image.yaml index 7c8b5e62f9..6c6361263a 100644 --- a/tests/ansible/playbooks/features/oci-image.yaml +++ b/tests/ansible/playbooks/features/oci-image.yaml @@ -7,6 +7,8 @@ gather_facts: false vars: oci_image_test_root: /var/lib/openshell-oci-image/tests + # The tests are skipped when the gateway never connects. + oci_image_failed: "{{ gateway_status is failed or oci_image_result.rc != 0 }}" tasks: - name: Wait for SSH ansible.builtin.wait_for_connection: @@ -61,6 +63,20 @@ register: openshell_cli changed_when: false + # Every test boot restarts the gateway, and some installers take minutes to + # recover. Wait once here; the runner's per-scenario preflight stays short. + - name: Wait for the registered gateway + ansible.builtin.command: + argv: [timeout, 10s, "{{ openshell_cli.stdout }}", status, --output, json] + register: gateway_status + changed_when: false + until: >- + gateway_status.rc == 0 and + (gateway_status.stdout | from_json).status == 'connected' + retries: 60 + delay: 5 + ignore_errors: true + - name: Run OCI image archive ansible.builtin.command: argv: @@ -84,32 +100,23 @@ register: oci_image_result changed_when: false failed_when: false + when: gateway_status is succeeded - name: Show OCI image diagnostics ansible.builtin.debug: var: oci_image_result - when: oci_image_result.rc != 0 + when: oci_image_failed - - name: Read OpenShell gateway logs - become: true - ansible.builtin.command: - argv: - - journalctl - - --no-pager - - --lines - - "500" - - _SYSTEMD_UNIT=openshell-gateway.service - - "+" - - _SYSTEMD_USER_UNIT=openshell-gateway.service - register: openshell_gateway_logs - changed_when: false - failed_when: false - when: oci_image_result.rc != 0 + - name: Collect OpenShell gateway diagnostics + ansible.builtin.include_role: + name: openshell_diagnostics + when: oci_image_failed - - name: Show OpenShell gateway logs - ansible.builtin.debug: - var: openshell_gateway_logs.stdout_lines - when: oci_image_result.rc != 0 + - name: Require a connected gateway + ansible.builtin.assert: + that: + - gateway_status is succeeded + fail_msg: OpenShell gateway did not connect before the OCI image tests; see gateway diagnostics - name: Require OCI image success ansible.builtin.assert: diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index 17f99f4e6b..6c8a559db9 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -184,17 +184,14 @@ USER app } continue; } - if create.success() - || create.stdout().contains("workspace-access-marker") - || create.stderr().contains("workspace-access-marker") - { + let printed = + |text: &str| create.stdout().contains(text) || create.stderr().contains(text); + if create.success() || printed("workspace-access-marker") { return Err( create.failure_diagnostic("sandbox creation fails before the command runs") ); } - if !create.stdout().contains(WORKSPACE_VALIDATION_FAILED) - && !create.stderr().contains(WORKSPACE_VALIDATION_FAILED) - { + if !printed(WORKSPACE_VALIDATION_FAILED) { return Err(create.failure_diagnostic(&format!( "sandbox creation fails with {WORKSPACE_VALIDATION_FAILED}" ))); @@ -291,14 +288,18 @@ async fn sandbox_cleanup_precedes_image_removal_on_success_and_failure() { } /// Shell checks for the identity, working directory, and `HOME` of a sandbox -/// child. With `image_file`, also check that root-owned image content is -/// present and unchanged in the workspace. +/// child. A custom workspace must not also get the managed `/sandbox`. With +/// `image_file`, also check that root-owned image content is present and +/// unchanged in the workspace. fn workspace_checks(identity: &str, workspace: &str, image_file: bool) -> String { let mut checks = format!( "test \"$(id -u):$(id -g)\" = {identity}; \ test \"$(pwd -P)\" = {workspace}; \ test \"$HOME\" = {workspace};" ); + if workspace != "/sandbox" { + checks.push_str(" test ! -e /sandbox;"); + } if image_file { checks.push_str( " test \"$(cat root-owned.txt)\" = root-owned; \ @@ -330,16 +331,16 @@ async fn create_sandbox( args.extend(["--policy", policy]); } args.extend(["--detach", "--", "sh", "-c", &main]); - runner - .step(format!("{suffix}/create")) - .description("sandbox starts from the test image") - .with_timeout(CREATE_TIMEOUT) - .run(&args) - .await - .map_err(|error| error.to_string())? - .require_success()?; + run_ok( + runner, + format!("{suffix}/create"), + "sandbox starts from the test image", + CREATE_TIMEOUT, + &args, + ) + .await?; - let exec = format!( + let script = format!( "set -eu; i=0; \ while [ ! -f /tmp/oci-main.status ]; do \ i=$((i + 1)); [ \"$i\" -le 60 ] || {{ echo main process checks did not finish >&2; exit 1; }}; \ @@ -350,16 +351,14 @@ async fn create_sandbox( fi; \ test -f main-write; {checks} touch exec-write" ); - runner - .step(format!("{suffix}/exec")) - .description("main process and exec children see the image workspace") - .with_timeout(COMMAND_TIMEOUT) - .run(&[ - "sandbox", "exec", "--name", &name, "--no-tty", "--", "sh", "-c", &exec, - ]) - .await - .map_err(|error| error.to_string())? - .require_success()?; + exec( + runner, + &name, + format!("{suffix}/exec"), + "main process and exec children see the image workspace", + &script, + ) + .await?; Ok(name) } @@ -372,49 +371,39 @@ async fn file_transfer_uses_workspace( let upload = local.path().join("oci-transfer.txt"); std::fs::write(&upload, "oci-transfer-ok").map_err(|error| format!("write upload: {error}"))?; let upload = upload.to_str().ok_or("upload path is not UTF-8")?; - runner - .step("named/upload") - .description("upload without a destination writes to the workspace") - .with_timeout(COMMAND_TIMEOUT) - .run(&["sandbox", "upload", sandbox, upload, "--no-git-ignore"]) - .await - .map_err(|error| error.to_string())? - .require_success()?; - runner - .step("named/uploaded") - .description("uploaded file is in the workspace") - .with_timeout(COMMAND_TIMEOUT) - .run(&[ - "sandbox", - "exec", - "--name", - sandbox, - "--no-tty", - "--", - "sh", - "-c", - "test \"$(cat oci-transfer.txt)\" = oci-transfer-ok", - ]) - .await - .map_err(|error| error.to_string())? - .require_success()?; + run_ok( + runner, + "named/upload", + "upload without a destination writes to the workspace", + COMMAND_TIMEOUT, + &["sandbox", "upload", sandbox, upload, "--no-git-ignore"], + ) + .await?; + exec( + runner, + sandbox, + "named/uploaded", + "uploaded file is in the workspace", + "test \"$(cat oci-transfer.txt)\" = oci-transfer-ok", + ) + .await?; let download = local.path().join("downloaded.txt"); let download_path = download.to_str().ok_or("download path is not UTF-8")?; - runner - .step("named/download") - .description("download resolves relative paths in the workspace") - .with_timeout(COMMAND_TIMEOUT) - .run(&[ + run_ok( + runner, + "named/download", + "download resolves relative paths in the workspace", + COMMAND_TIMEOUT, + &[ "sandbox", "download", sandbox, "oci-transfer.txt", download_path, - ]) - .await - .map_err(|error| error.to_string())? - .require_success()?; + ], + ) + .await?; let downloaded = std::fs::read_to_string(&download).map_err(|error| format!("read download: {error}"))?; if downloaded != "oci-transfer-ok" { @@ -430,44 +419,71 @@ async fn file_transfer_uses_workspace( .and_then(|()| std::fs::write(merge.join("added.txt"), "local-added")) .map_err(|error| format!("write merge files: {error}"))?; let merge = merge.to_str().ok_or("merge path is not UTF-8")?; - let seed = "mkdir merge-upload && printf remote-conflict > merge-upload/conflict.txt \ - && printf remote-preserved > merge-upload/unrelated.txt"; - let verify = "test \"$(cat merge-upload/conflict.txt)\" = local-conflict \ - && test \"$(cat merge-upload/added.txt)\" = local-added \ - && test \"$(cat merge-upload/unrelated.txt)\" = remote-preserved"; - for (step, description, args) in [ - ( - "named/merge-seed", - "seed an existing workspace directory", - [ - "sandbox", "exec", "--name", sandbox, "--no-tty", "--", "sh", "-c", seed, - ] - .as_slice(), - ), - ( - "named/merge-upload", - "directory upload merges into the existing directory", - ["sandbox", "upload", sandbox, merge, "--no-git-ignore"].as_slice(), - ), - ( - "named/merged", - "upload overwrites conflicts and keeps unrelated files", - [ - "sandbox", "exec", "--name", sandbox, "--no-tty", "--", "sh", "-c", verify, - ] - .as_slice(), - ), - ] { - runner - .step(step) - .description(description) - .with_timeout(COMMAND_TIMEOUT) - .run(args) - .await - .map_err(|error| error.to_string())? - .require_success()?; - } - Ok(()) + exec( + runner, + sandbox, + "named/merge-seed", + "seed an existing workspace directory", + "mkdir merge-upload && printf remote-conflict > merge-upload/conflict.txt \ + && printf remote-preserved > merge-upload/unrelated.txt", + ) + .await?; + run_ok( + runner, + "named/merge-upload", + "directory upload merges into the existing directory", + COMMAND_TIMEOUT, + &["sandbox", "upload", sandbox, merge, "--no-git-ignore"], + ) + .await?; + exec( + runner, + sandbox, + "named/merged", + "upload overwrites conflicts and keeps unrelated files", + "test \"$(cat merge-upload/conflict.txt)\" = local-conflict \ + && test \"$(cat merge-upload/added.txt)\" = local-added \ + && test \"$(cat merge-upload/unrelated.txt)\" = remote-preserved", + ) + .await +} + +/// Run a CLI step that must succeed. +async fn run_ok( + runner: &OpenShellRunner, + step: impl Into, + description: &str, + timeout: Duration, + args: &[&str], +) -> Result<(), String> { + runner + .step(step) + .description(description) + .with_timeout(timeout) + .run(args) + .await + .map_err(|error| error.to_string())? + .require_success() +} + +/// Run a shell script in `sandbox` through `sandbox exec`. +async fn exec( + runner: &OpenShellRunner, + sandbox: &str, + step: impl Into, + description: &str, + script: &str, +) -> Result<(), String> { + run_ok( + runner, + step, + description, + COMMAND_TIMEOUT, + &[ + "sandbox", "exec", "--name", sandbox, "--no-tty", "--", "sh", "-c", script, + ], + ) + .await } /// An image owned by `run`, kept alive until sandbox cleanup completes. From e69b2e4dd3b759ae7ae485c03c3026ae3aeafc39 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Mon, 12 Oct 2026 09:01:29 -0700 Subject: [PATCH 11/16] test(oci): replace podman_oci_identity with shared and driver suite checks The oci-image feature suite now checks, on Docker and Podman, that the image user has no capabilities, has no_new_privs set, and cannot read the sandbox control credentials, and that a custom WORKDIR is not a mount. The checks that need container inspection (pinned image ID, network=none, supervisor user and capabilities) move to a small test in the Podman driver suite. Remove the legacy podman_oci_identity e2e test. Signed-off-by: Matthew Grossman --- TESTING.md | 9 +- e2e/rust/Cargo.toml | 5 - e2e/rust/e2e-podman.sh | 1 - e2e/rust/tests/podman_oci_identity.rs | 289 ------------------ .../playbooks/drivers/podman/tests.yaml | 5 + tests/artifacts.nix | 1 - .../drivers/podman/tests/oci_identity.rs | 170 +++++++++++ .../features/oci-image/tests/oci_image.rs | 19 +- 8 files changed, 194 insertions(+), 305 deletions(-) delete mode 100644 e2e/rust/tests/podman_oci_identity.rs create mode 100644 tests/suites/drivers/podman/tests/oci_identity.rs diff --git a/TESTING.md b/TESTING.md index 2fe14e16a7..74e629304c 100644 --- a/TESTING.md +++ b/TESTING.md @@ -313,10 +313,11 @@ The `e2e-podman` testsuite runs a nextest archive built with the corresponding Rust feature and preloads its Python workload image into the rootless Podman store. The separate `driver-podman` testsuite compares OpenShell and direct Podman user-namespace mappings for the default, `auto`, `keep-id`, and private -profiles. The E2E archive excludes binaries that still depend on wrapper-owned -gateway controls, host fixtures, missing guest tools, or nondeterministic relay -setup. The `driver-podman` suite replaces the removed `podman_userns` E2E -binary. `tests/artifacts.nix` keeps the follow-up exclusions explicit and uses +profiles, and inspects the Podman containers created from an OCI image. The E2E +archive excludes binaries that still depend on wrapper-owned gateway controls, +host fixtures, missing guest tools, or nondeterministic relay setup. The +`driver-podman` suite replaces the removed `podman_userns` and +`podman_oci_identity` E2E binaries. `tests/artifacts.nix` keeps the follow-up exclusions explicit and uses the same filter for the generated inventory, so excluded binaries cannot appear as false passes or silently re-enter the archive. diff --git a/e2e/rust/Cargo.toml b/e2e/rust/Cargo.toml index 65ec296b8c..ada076d713 100644 --- a/e2e/rust/Cargo.toml +++ b/e2e/rust/Cargo.toml @@ -114,11 +114,6 @@ name = "podman_corporate_proxy" path = "tests/podman_corporate_proxy.rs" required-features = ["e2e-podman"] -[[test]] -name = "podman_oci_identity" -path = "tests/podman_oci_identity.rs" -required-features = ["e2e-podman"] - [[test]] name = "podman_resource_limits" path = "tests/podman_resource_limits.rs" diff --git a/e2e/rust/e2e-podman.sh b/e2e/rust/e2e-podman.sh index 0ebdc2359e..c0deb2c750 100755 --- a/e2e/rust/e2e-podman.sh +++ b/e2e/rust/e2e-podman.sh @@ -39,7 +39,6 @@ PODMAN_CI_TESTS=( podman_corporate_proxy podman_gateway_start podman_host_gateway - podman_oci_identity provider_token_exchange ) diff --git a/e2e/rust/tests/podman_oci_identity.rs b/e2e/rust/tests/podman_oci_identity.rs deleted file mode 100644 index a7df58585c..0000000000 --- a/e2e/rust/tests/podman_oci_identity.rs +++ /dev/null @@ -1,289 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -#![cfg(feature = "e2e-podman")] - -//! Podman-specific E2E coverage for immutable-image launch and the isolated -//! workload/supervisor container pair. -//! -//! The test builds an image through the selected Podman engine, creates a -//! sandbox from its mutable tag, and inspects the real Podman containers: the -//! image ID recorded on the workload, each container's user, the supervisor's -//! capabilities, networking, and mounts. This exercises the Podman API -//! inspect → protected metadata → create path rather than only its unit -//! serialization boundaries. The sandbox identity and workspace seen by the -//! main process and `sandbox exec` are shared with Docker and covered by the -//! `oci-image` feature suite in `tests/suites/features`. - -use std::process::Stdio; - -use openshell_e2e::harness::container::{ContainerEngine, is_e2e_driver}; -use openshell_e2e::harness::sandbox::SandboxGuard; - -const BASE_IMAGE: &str = "nvcr.io/nvidia/base/ubuntu:24.04"; -const READY_MARKER: &str = "podman-oci-identity-ready"; -const OCI_UID: &str = "2345"; -const OCI_GID: &str = "2346"; -const OCI_FALLBACK_POLICY: &str = r"version: 1 - -filesystem_policy: - include_workdir: true - read_only: [/usr, /lib, /lib64, /proc, /dev/urandom, /etc] - read_write: [/sandbox, /tmp, /dev/null] -landlock: - compatibility: best_effort - -network_policies: {} -"; - -struct ImageGuard { - engine: ContainerEngine, - tag: String, - id: String, -} - -impl ImageGuard { - fn build() -> Result { - let engine = ContainerEngine::from_env()?; - if engine.name() != "podman" { - return Err(format!( - "Podman OCI identity E2E requires podman, got {}", - engine.name() - )); - } - - let context = tempfile::tempdir().map_err(|err| format!("create build context: {err}"))?; - let containerfile = context.path().join("Containerfile"); - std::fs::write( - &containerfile, - format!( - r"FROM {BASE_IMAGE} -USER 0:0 -RUN mkdir -p /home/app/project && \ - chown {OCI_UID}:{OCI_GID} /home/app /home/app/project && \ - chmod 0700 /home/app /home/app/project -WORKDIR /home/app/project -USER {OCI_UID}:{OCI_GID} -" - ), - ) - .map_err(|err| format!("write Containerfile: {err}"))?; - - let tag = format!( - "localhost/openshell-e2e-podman-oci-identity:{}", - std::process::id() - ); - run_engine( - &engine, - &[ - "build", - "--pull=never", - "--file", - containerfile - .to_str() - .ok_or_else(|| "Containerfile path is not UTF-8".to_string())?, - "--tag", - &tag, - context - .path() - .to_str() - .ok_or_else(|| "build context path is not UTF-8".to_string())?, - ], - )?; - let id = run_engine(&engine, &["image", "inspect", "--format", "{{.Id}}", &tag])?; - let user = run_engine( - &engine, - &["image", "inspect", "--format", "{{.Config.User}}", &tag], - )?; - if user != format!("{OCI_UID}:{OCI_GID}") { - return Err(format!( - "Podman-built image has OCI user '{user}', expected {OCI_UID}:{OCI_GID}" - )); - } - - Ok(Self { engine, tag, id }) - } -} - -impl Drop for ImageGuard { - fn drop(&mut self) { - let _ = self - .engine - .command() - .args(["image", "rm", "--force", &self.tag]) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status(); - } -} - -fn run_engine(engine: &ContainerEngine, args: &[&str]) -> Result { - let output = engine - .command() - .args(args) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .output() - .map_err(|err| format!("failed to run {} {}: {err}", engine.name(), args.join(" ")))?; - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); - if !output.status.success() { - return Err(format!( - "{} {} failed (exit {:?}):\n{stdout}{stderr}", - engine.name(), - args.join(" "), - output.status.code() - )); - } - Ok(stdout.trim().to_string()) -} - -fn sandbox_container_id(engine: &ContainerEngine, sandbox_name: &str) -> Result { - container_id_for_role(engine, sandbox_name, "sandbox") -} - -fn container_id_for_role( - engine: &ContainerEngine, - sandbox_name: &str, - role: &str, -) -> Result { - let name_filter = format!("label=openshell.ai/sandbox-name={sandbox_name}"); - let role_filter = format!("label=openshell.ai/isolation-role={role}"); - let stdout = run_engine( - engine, - &[ - "ps", - "-aq", - "--filter", - "label=openshell.managed=true", - "--filter", - &name_filter, - "--filter", - &role_filter, - ], - )?; - let ids = stdout - .lines() - .map(str::trim) - .filter(|line| !line.is_empty()) - .collect::>(); - match ids.as_slice() { - [id] => Ok((*id).to_string()), - [] => Err(format!( - "no Podman container found for sandbox '{sandbox_name}'" - )), - _ => Err(format!( - "multiple Podman containers found for sandbox '{sandbox_name}': {ids:?}" - )), - } -} - -fn normalized_image_id(image_id: &str) -> &str { - image_id - .trim() - .strip_prefix("sha256:") - .unwrap_or(image_id.trim()) -} - -#[tokio::test] -async fn podman_uses_oci_identity_and_inspected_image_id() { - if !is_e2e_driver("podman") { - eprintln!("Skipping Podman OCI identity test: e2e driver is not podman"); - return; - } - - let image = ImageGuard::build().expect("build Podman OCI identity image"); - // The fixture image contains a policy with an - // explicit `sandbox` process identity. Supply a complete policy that - // intentionally omits `process` so this test exercises OCI fallback. - let policy = tempfile::NamedTempFile::new().expect("create OCI fallback policy"); - std::fs::write(policy.path(), OCI_FALLBACK_POLICY).expect("write OCI fallback policy"); - let policy_path = policy.path().to_str().expect("policy path is UTF-8"); - let mut sandbox = SandboxGuard::create_keep_with_args( - &["--from", &image.tag, "--policy", policy_path, "--no-tty"], - &["sh", "-c", "echo podman-oci-identity-ready; sleep infinity"], - READY_MARKER, - ) - .await - .expect("create sandbox from Podman-built OCI identity image"); - - let container_id = - sandbox_container_id(&image.engine, &sandbox.name).expect("find Podman sandbox container"); - let launched_image_id = run_engine( - &image.engine, - &[ - "container", - "inspect", - "--format", - "{{.Image}}", - &container_id, - ], - ) - .expect("inspect Podman sandbox container image"); - assert_eq!( - normalized_image_id(&launched_image_id), - normalized_image_id(&image.id), - "Podman sandbox must launch the immutable image ID inspected before creation" - ); - - assert_isolated_pair(&image, &sandbox, &container_id).await; - sandbox.cleanup().await; -} - -async fn assert_isolated_pair(image: &ImageGuard, sandbox: &SandboxGuard, container_id: &str) { - let supervisor_id = container_id_for_role(&image.engine, &sandbox.name, "supervisor") - .expect("find separate supervisor companion"); - assert_ne!(supervisor_id, container_id); - let workload_user = run_engine( - &image.engine, - &["inspect", "--format", "{{.Config.User}}", container_id], - ) - .unwrap(); - assert_eq!( - workload_user, - format!("{OCI_UID}:{OCI_GID}"), - "a custom OCI workspace must start directly as the final identity" - ); - let supervisor_user = run_engine( - &image.engine, - &["inspect", "--format", "{{.Config.User}}", &supervisor_id], - ) - .unwrap(); - assert_eq!(supervisor_user, format!("{OCI_UID}:{OCI_GID}")); - let supervisor_caps = run_engine( - &image.engine, - &["inspect", "--format", "{{.EffectiveCaps}}", &supervisor_id], - ) - .unwrap(); - assert_eq!( - supervisor_caps, "[]", - "the supervisor companion may not have effective capabilities" - ); - let network = run_engine( - &image.engine, - &[ - "inspect", - "--format", - "{{.HostConfig.NetworkMode}}", - container_id, - ], - ) - .unwrap(); - assert_eq!(network, "none"); - let mounts = run_engine( - &image.engine, - &[ - "inspect", - "--format", - "{{range .Mounts}}{{println .Destination}}{{end}}", - container_id, - ], - ) - .unwrap(); - assert!(!mounts.contains("/etc/openshell/tls")); - assert!(!mounts.contains("/.openshell/supervisor")); - assert!(!mounts.lines().any(|path| path == "/home/app/project")); - assert!(!mounts.lines().any(|path| path == "/sandbox")); - let posture = sandbox.exec(&["sh", "-c", "set -eu; awk '/^CapEff:|^CapBnd:|^NoNewPrivs:/ {print}' /proc/self/status; test ! -r /.openshell/channel/sandbox/server.key; test ! -r /.openshell/supervisor/runtime-descriptor.json"]).await.expect("workload cannot read either control credential set"); - assert!(posture.contains("0000000000000000")); -} diff --git a/tests/ansible/playbooks/drivers/podman/tests.yaml b/tests/ansible/playbooks/drivers/podman/tests.yaml index abe2f6e553..1d6127dfeb 100644 --- a/tests/ansible/playbooks/drivers/podman/tests.yaml +++ b/tests/ansible/playbooks/drivers/podman/tests.yaml @@ -53,9 +53,14 @@ ansible.builtin.command: argv: [cargo-nextest, nextest, run, --archive-file, "{{ podman_test_root }}/tests.tar.zst", --workspace-remap, "{{ podman_test_root }}", --no-capture] environment: + HOME: /home/tmachine OPENSHELL_BIN: /usr/local/bin/openshell OPENSHELL_TEST_INPUT_DIR: "{{ podman_test_input_dir }}" OPENSHELL_PODMAN_TEST_IMAGE: "{{ openshell_podman_test_image | default('') }}" + # Tests that inspect the gateway's containers reach its Podman store. + # Rootful Podman's store belongs to root. + OPENSHELL_TEST_CONTAINER_ENGINE: "{{ 'podman' if tmachine_container_runtime_is_rootless else 'sudo -n podman' }}" + XDG_RUNTIME_DIR: "/run/user/{{ tmachine_container_runtime_tmachine_uid.stdout }}" register: podman_test_result changed_when: false failed_when: false diff --git a/tests/artifacts.nix b/tests/artifacts.nix index 73195e6c22..67efc9a7d4 100644 --- a/tests/artifacts.nix +++ b/tests/artifacts.nix @@ -123,7 +123,6 @@ let "no_proxy" "podman_corporate_proxy" "podman_gateway_start" - "podman_oci_identity" "provider_auto_create" # The provider-refresh feature suite covers revoked Keycloak grants. This # binary instead covers stable workload handles across repeated rotations diff --git a/tests/suites/drivers/podman/tests/oci_identity.rs b/tests/suites/drivers/podman/tests/oci_identity.rs new file mode 100644 index 0000000000..a26653766c --- /dev/null +++ b/tests/suites/drivers/podman/tests/oci_identity.rs @@ -0,0 +1,170 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Podman container settings for a sandbox created from an image with a custom +//! `USER` and `WORKDIR` that the sandbox cannot observe from inside. +//! +//! The shared `oci-image` feature suite checks what the workload sees. This +//! test inspects the Podman containers directly: the workload runs the image +//! ID inspected before creation rather than the mutable tag, and is fenced by +//! `network=none`; the supervisor runs as the image user with no effective +//! capabilities. +//! +//! `OPENSHELL_TEST_CONTAINER_ENGINE` is the command that reaches the gateway's +//! Podman store, such as `podman` or `sudo -n podman`. + +mod support; + +use std::io::Write; +use std::process::{Command, Stdio}; +use std::time::Duration; + +use openshell_conformance::OpenShellRunner; + +use support::assert_podman_gateway; + +const ENGINE_ENV: &str = "OPENSHELL_TEST_CONTAINER_ENGINE"; +const CREATE_TIMEOUT: Duration = Duration::from_mins(10); +const IMAGE_USER: &str = "2345:2346"; + +#[tokio::test] +async fn podman_containers_use_pinned_image_and_image_user() { + let mut runner = OpenShellRunner::from_env("podman-oci-identity") + .expect("candidate openshell CLI is available"); + let tag = format!( + "localhost/openshell-test-podman-oci-identity:{}", + std::process::id() + ); + let result = async { + runner.check_gateway_status().await?; + assert_podman_gateway(&runner).await?; + + let containerfile = format!( + "FROM nvcr.io/nvidia/base/ubuntu:24.04 +RUN mkdir -p /workspace/project && chown {IMAGE_USER} /workspace/project +WORKDIR /workspace/project +USER {IMAGE_USER} +" + ); + podman(&["build", "--tag", &tag, "-"], Some(&containerfile))?; + let image_id = podman(&["image", "inspect", "--format", "{{.Id}}", &tag], None)?; + + let name = format!("po-{}", runner.id()); + runner.track_sandbox(&name); + runner + .step("oci-identity/create") + .description("sandbox starts from the test image") + .with_timeout(CREATE_TIMEOUT) + .run(&[ + "sandbox", "create", "--name", &name, "--from", &tag, "--detach", "--", "sleep", + "infinity", + ]) + .await + .map_err(|error| error.to_string())? + .require_success()?; + + let workload = container_for_role(&name, "sandbox")?; + let supervisor = container_for_role(&name, "supervisor")?; + for (what, container, format, expected) in [ + ("workload image", &workload, "{{.Image}}", image_id.as_str()), + ("workload user", &workload, "{{.Config.User}}", IMAGE_USER), + ( + "workload network", + &workload, + "{{.HostConfig.NetworkMode}}", + "none", + ), + ( + "supervisor user", + &supervisor, + "{{.Config.User}}", + IMAGE_USER, + ), + ( + "supervisor capabilities", + &supervisor, + "{{.EffectiveCaps}}", + "[]", + ), + ] { + let actual = podman(&["inspect", "--format", format, container], None)?; + if actual.trim_start_matches("sha256:") != expected.trim_start_matches("sha256:") { + return Err(format!("{what} is {actual:?}, expected {expected:?}")); + } + } + Ok(()) + } + .await; + // Remove the tag only after the sandbox is gone. A test failure takes + // precedence over a cleanup failure. + let result = runner.finish(result).await; + let cleanup = podman(&["image", "rm", &tag], None); + if let Err(error) = result { + panic!("Podman OCI identity failed:\n{error}"); + } + cleanup.expect("remove test image"); +} + +/// The one container with `role` for `sandbox`. +fn container_for_role(sandbox: &str, role: &str) -> Result { + let ids = podman( + &[ + "ps", + "-aq", + "--filter", + &format!("label=openshell.ai/sandbox-name={sandbox}"), + "--filter", + &format!("label=openshell.ai/isolation-role={role}"), + ], + None, + )?; + match ids.lines().collect::>().as_slice() { + [id] => Ok((*id).to_string()), + ids => Err(format!( + "expected one {role} container for sandbox {sandbox}, found {ids:?}" + )), + } +} + +/// Run a command against the gateway's Podman store and return its stdout. +fn podman(args: &[&str], stdin: Option<&str>) -> Result { + let engine = std::env::var(ENGINE_ENV) + .map_err(|_| format!("{ENGINE_ENV} must name the gateway's Podman command"))?; + let command = format!("{engine} {}", args.join(" ")); + let mut engine = engine.split_whitespace(); + let program = engine + .next() + .ok_or_else(|| format!("{ENGINE_ENV} is empty"))?; + let mut child = Command::new(program) + .args(engine) + .args(args) + .stdin(if stdin.is_some() { + Stdio::piped() + } else { + Stdio::null() + }) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .map_err(|error| format!("failed to run {command}: {error}"))?; + if let Some(input) = stdin { + child + .stdin + .take() + .expect("piped stdin") + .write_all(input.as_bytes()) + .map_err(|error| format!("write stdin of {command}: {error}"))?; + } + let output = child + .wait_with_output() + .map_err(|error| format!("wait for {command}: {error}"))?; + if !output.status.success() { + return Err(format!( + "{command} failed ({}):\n{}{}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )); + } + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) +} diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index 6c8a559db9..1dd7be39cc 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -5,8 +5,8 @@ //! //! Each test builds a small image with the container engine that backs the //! target gateway, creates a sandbox from it through the candidate CLI, and -//! checks the process identity, workspace, and image content seen by both the -//! sandbox main process and `sandbox exec`. +//! checks the process identity, privileges, workspace, and image content seen +//! by both the sandbox main process and `sandbox exec`. //! //! `OPENSHELL_BIN` names the candidate CLI. `OPENSHELL_TEST_CONTAINER_ENGINE` //! is the command that builds images into the gateway's image store, such as @@ -288,17 +288,26 @@ async fn sandbox_cleanup_precedes_image_removal_on_success_and_failure() { } /// Shell checks for the identity, working directory, and `HOME` of a sandbox -/// child. A custom workspace must not also get the managed `/sandbox`. With +/// child. The image user gains no capabilities and cannot read the sandbox's +/// control credentials. A custom workspace is the image directory itself, not +/// a mount over it, and does not also get the managed `/sandbox`. With /// `image_file`, also check that root-owned image content is present and /// unchanged in the workspace. fn workspace_checks(identity: &str, workspace: &str, image_file: bool) -> String { let mut checks = format!( "test \"$(id -u):$(id -g)\" = {identity}; \ test \"$(pwd -P)\" = {workspace}; \ - test \"$HOME\" = {workspace};" + test \"$HOME\" = {workspace}; \ + awk '/^(CapEff|CapBnd):/ && $2 != \"0000000000000000\" {{ exit 1 }} \ + /^NoNewPrivs:/ && $2 != 1 {{ exit 1 }}' /proc/self/status; \ + test ! -r /.openshell/channel/sandbox/server.key; \ + test ! -r /.openshell/supervisor/runtime-descriptor.json;" ); if workspace != "/sandbox" { - checks.push_str(" test ! -e /sandbox;"); + checks.push_str(&format!( + " awk -v w={workspace} '$5 == w {{ exit 1 }}' /proc/self/mountinfo; \ + test ! -e /sandbox;" + )); } if image_file { checks.push_str( From 6884f1d0933cbfed7fcf34e6b0b7bf1bf3f36246 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Mon, 12 Oct 2026 09:16:50 -0700 Subject: [PATCH 12/16] test(oci): keep podman_oci_identity for Podman-only isolation checks Restore the Podman e2e test removed in the previous commit and drop only the checks the shared oci-image suite now runs on Docker and Podman: workload capabilities, no_new_privs, credential readability, and the workspace and /sandbox mount checks. The pinned image ID and the workload/supervisor isolation checks stay, as does the Podman driver suite unchanged. Signed-off-by: Matthew Grossman --- TESTING.md | 9 +- e2e/rust/Cargo.toml | 5 + e2e/rust/e2e-podman.sh | 1 + e2e/rust/tests/podman_oci_identity.rs | 285 ++++++++++++++++++ .../playbooks/drivers/podman/tests.yaml | 5 - tests/artifacts.nix | 1 + .../drivers/podman/tests/oci_identity.rs | 170 ----------- 7 files changed, 296 insertions(+), 180 deletions(-) create mode 100644 e2e/rust/tests/podman_oci_identity.rs delete mode 100644 tests/suites/drivers/podman/tests/oci_identity.rs diff --git a/TESTING.md b/TESTING.md index 74e629304c..2fe14e16a7 100644 --- a/TESTING.md +++ b/TESTING.md @@ -313,11 +313,10 @@ The `e2e-podman` testsuite runs a nextest archive built with the corresponding Rust feature and preloads its Python workload image into the rootless Podman store. The separate `driver-podman` testsuite compares OpenShell and direct Podman user-namespace mappings for the default, `auto`, `keep-id`, and private -profiles, and inspects the Podman containers created from an OCI image. The E2E -archive excludes binaries that still depend on wrapper-owned gateway controls, -host fixtures, missing guest tools, or nondeterministic relay setup. The -`driver-podman` suite replaces the removed `podman_userns` and -`podman_oci_identity` E2E binaries. `tests/artifacts.nix` keeps the follow-up exclusions explicit and uses +profiles. The E2E archive excludes binaries that still depend on wrapper-owned +gateway controls, host fixtures, missing guest tools, or nondeterministic relay +setup. The `driver-podman` suite replaces the removed `podman_userns` E2E +binary. `tests/artifacts.nix` keeps the follow-up exclusions explicit and uses the same filter for the generated inventory, so excluded binaries cannot appear as false passes or silently re-enter the archive. diff --git a/e2e/rust/Cargo.toml b/e2e/rust/Cargo.toml index ada076d713..65ec296b8c 100644 --- a/e2e/rust/Cargo.toml +++ b/e2e/rust/Cargo.toml @@ -114,6 +114,11 @@ name = "podman_corporate_proxy" path = "tests/podman_corporate_proxy.rs" required-features = ["e2e-podman"] +[[test]] +name = "podman_oci_identity" +path = "tests/podman_oci_identity.rs" +required-features = ["e2e-podman"] + [[test]] name = "podman_resource_limits" path = "tests/podman_resource_limits.rs" diff --git a/e2e/rust/e2e-podman.sh b/e2e/rust/e2e-podman.sh index c0deb2c750..0ebdc2359e 100755 --- a/e2e/rust/e2e-podman.sh +++ b/e2e/rust/e2e-podman.sh @@ -39,6 +39,7 @@ PODMAN_CI_TESTS=( podman_corporate_proxy podman_gateway_start podman_host_gateway + podman_oci_identity provider_token_exchange ) diff --git a/e2e/rust/tests/podman_oci_identity.rs b/e2e/rust/tests/podman_oci_identity.rs new file mode 100644 index 0000000000..d13f35c91e --- /dev/null +++ b/e2e/rust/tests/podman_oci_identity.rs @@ -0,0 +1,285 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +#![cfg(feature = "e2e-podman")] + +//! Podman-specific E2E coverage for immutable-image launch and the isolated +//! workload/supervisor container pair. +//! +//! The test builds an image through the selected Podman engine, creates a +//! sandbox from its mutable tag, and inspects the real Podman containers: the +//! image ID recorded on the workload, each container's user, the supervisor's +//! capabilities, networking, and mounts. This exercises the Podman API +//! inspect → protected metadata → create path rather than only its unit +//! serialization boundaries. The sandbox identity, privileges, and workspace +//! seen by the main process and `sandbox exec` are shared with Docker and +//! covered by the `oci-image` feature suite in `tests/suites/features`. + +use std::process::Stdio; + +use openshell_e2e::harness::container::{ContainerEngine, is_e2e_driver}; +use openshell_e2e::harness::sandbox::SandboxGuard; + +const BASE_IMAGE: &str = "nvcr.io/nvidia/base/ubuntu:24.04"; +const READY_MARKER: &str = "podman-oci-identity-ready"; +const OCI_UID: &str = "2345"; +const OCI_GID: &str = "2346"; +const OCI_FALLBACK_POLICY: &str = r"version: 1 + +filesystem_policy: + include_workdir: true + read_only: [/usr, /lib, /lib64, /proc, /dev/urandom, /etc] + read_write: [/sandbox, /tmp, /dev/null] +landlock: + compatibility: best_effort + +network_policies: {} +"; + +struct ImageGuard { + engine: ContainerEngine, + tag: String, + id: String, +} + +impl ImageGuard { + fn build() -> Result { + let engine = ContainerEngine::from_env()?; + if engine.name() != "podman" { + return Err(format!( + "Podman OCI identity E2E requires podman, got {}", + engine.name() + )); + } + + let context = tempfile::tempdir().map_err(|err| format!("create build context: {err}"))?; + let containerfile = context.path().join("Containerfile"); + std::fs::write( + &containerfile, + format!( + r"FROM {BASE_IMAGE} +USER 0:0 +RUN mkdir -p /home/app/project && \ + chown {OCI_UID}:{OCI_GID} /home/app /home/app/project && \ + chmod 0700 /home/app /home/app/project +WORKDIR /home/app/project +USER {OCI_UID}:{OCI_GID} +" + ), + ) + .map_err(|err| format!("write Containerfile: {err}"))?; + + let tag = format!( + "localhost/openshell-e2e-podman-oci-identity:{}", + std::process::id() + ); + run_engine( + &engine, + &[ + "build", + "--pull=never", + "--file", + containerfile + .to_str() + .ok_or_else(|| "Containerfile path is not UTF-8".to_string())?, + "--tag", + &tag, + context + .path() + .to_str() + .ok_or_else(|| "build context path is not UTF-8".to_string())?, + ], + )?; + let id = run_engine(&engine, &["image", "inspect", "--format", "{{.Id}}", &tag])?; + let user = run_engine( + &engine, + &["image", "inspect", "--format", "{{.Config.User}}", &tag], + )?; + if user != format!("{OCI_UID}:{OCI_GID}") { + return Err(format!( + "Podman-built image has OCI user '{user}', expected {OCI_UID}:{OCI_GID}" + )); + } + + Ok(Self { engine, tag, id }) + } +} + +impl Drop for ImageGuard { + fn drop(&mut self) { + let _ = self + .engine + .command() + .args(["image", "rm", "--force", &self.tag]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); + } +} + +fn run_engine(engine: &ContainerEngine, args: &[&str]) -> Result { + let output = engine + .command() + .args(args) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .output() + .map_err(|err| format!("failed to run {} {}: {err}", engine.name(), args.join(" ")))?; + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + if !output.status.success() { + return Err(format!( + "{} {} failed (exit {:?}):\n{stdout}{stderr}", + engine.name(), + args.join(" "), + output.status.code() + )); + } + Ok(stdout.trim().to_string()) +} + +fn sandbox_container_id(engine: &ContainerEngine, sandbox_name: &str) -> Result { + container_id_for_role(engine, sandbox_name, "sandbox") +} + +fn container_id_for_role( + engine: &ContainerEngine, + sandbox_name: &str, + role: &str, +) -> Result { + let name_filter = format!("label=openshell.ai/sandbox-name={sandbox_name}"); + let role_filter = format!("label=openshell.ai/isolation-role={role}"); + let stdout = run_engine( + engine, + &[ + "ps", + "-aq", + "--filter", + "label=openshell.managed=true", + "--filter", + &name_filter, + "--filter", + &role_filter, + ], + )?; + let ids = stdout + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .collect::>(); + match ids.as_slice() { + [id] => Ok((*id).to_string()), + [] => Err(format!( + "no Podman container found for sandbox '{sandbox_name}'" + )), + _ => Err(format!( + "multiple Podman containers found for sandbox '{sandbox_name}': {ids:?}" + )), + } +} + +fn normalized_image_id(image_id: &str) -> &str { + image_id + .trim() + .strip_prefix("sha256:") + .unwrap_or(image_id.trim()) +} + +#[tokio::test] +async fn podman_uses_oci_identity_and_inspected_image_id() { + if !is_e2e_driver("podman") { + eprintln!("Skipping Podman OCI identity test: e2e driver is not podman"); + return; + } + + let image = ImageGuard::build().expect("build Podman OCI identity image"); + // The fixture image contains a policy with an + // explicit `sandbox` process identity. Supply a complete policy that + // intentionally omits `process` so this test exercises OCI fallback. + let policy = tempfile::NamedTempFile::new().expect("create OCI fallback policy"); + std::fs::write(policy.path(), OCI_FALLBACK_POLICY).expect("write OCI fallback policy"); + let policy_path = policy.path().to_str().expect("policy path is UTF-8"); + let mut sandbox = SandboxGuard::create_keep_with_args( + &["--from", &image.tag, "--policy", policy_path, "--no-tty"], + &["sh", "-c", "echo podman-oci-identity-ready; sleep infinity"], + READY_MARKER, + ) + .await + .expect("create sandbox from Podman-built OCI identity image"); + + let container_id = + sandbox_container_id(&image.engine, &sandbox.name).expect("find Podman sandbox container"); + let launched_image_id = run_engine( + &image.engine, + &[ + "container", + "inspect", + "--format", + "{{.Image}}", + &container_id, + ], + ) + .expect("inspect Podman sandbox container image"); + assert_eq!( + normalized_image_id(&launched_image_id), + normalized_image_id(&image.id), + "Podman sandbox must launch the immutable image ID inspected before creation" + ); + + assert_isolated_pair(&image, &sandbox, &container_id); + sandbox.cleanup().await; +} + +fn assert_isolated_pair(image: &ImageGuard, sandbox: &SandboxGuard, container_id: &str) { + let supervisor_id = container_id_for_role(&image.engine, &sandbox.name, "supervisor") + .expect("find separate supervisor companion"); + assert_ne!(supervisor_id, container_id); + let workload_user = run_engine( + &image.engine, + &["inspect", "--format", "{{.Config.User}}", container_id], + ) + .unwrap(); + assert_eq!( + workload_user, + format!("{OCI_UID}:{OCI_GID}"), + "a custom OCI workspace must start directly as the final identity" + ); + let supervisor_user = run_engine( + &image.engine, + &["inspect", "--format", "{{.Config.User}}", &supervisor_id], + ) + .unwrap(); + assert_eq!(supervisor_user, format!("{OCI_UID}:{OCI_GID}")); + let supervisor_caps = run_engine( + &image.engine, + &["inspect", "--format", "{{.EffectiveCaps}}", &supervisor_id], + ) + .unwrap(); + assert_eq!( + supervisor_caps, "[]", + "the supervisor companion may not have effective capabilities" + ); + let network = run_engine( + &image.engine, + &[ + "inspect", + "--format", + "{{.HostConfig.NetworkMode}}", + container_id, + ], + ) + .unwrap(); + assert_eq!(network, "none"); + let mounts = run_engine( + &image.engine, + &[ + "inspect", + "--format", + "{{range .Mounts}}{{println .Destination}}{{end}}", + container_id, + ], + ) + .unwrap(); + assert!(!mounts.contains("/etc/openshell/tls")); + assert!(!mounts.contains("/.openshell/supervisor")); +} diff --git a/tests/ansible/playbooks/drivers/podman/tests.yaml b/tests/ansible/playbooks/drivers/podman/tests.yaml index 1d6127dfeb..abe2f6e553 100644 --- a/tests/ansible/playbooks/drivers/podman/tests.yaml +++ b/tests/ansible/playbooks/drivers/podman/tests.yaml @@ -53,14 +53,9 @@ ansible.builtin.command: argv: [cargo-nextest, nextest, run, --archive-file, "{{ podman_test_root }}/tests.tar.zst", --workspace-remap, "{{ podman_test_root }}", --no-capture] environment: - HOME: /home/tmachine OPENSHELL_BIN: /usr/local/bin/openshell OPENSHELL_TEST_INPUT_DIR: "{{ podman_test_input_dir }}" OPENSHELL_PODMAN_TEST_IMAGE: "{{ openshell_podman_test_image | default('') }}" - # Tests that inspect the gateway's containers reach its Podman store. - # Rootful Podman's store belongs to root. - OPENSHELL_TEST_CONTAINER_ENGINE: "{{ 'podman' if tmachine_container_runtime_is_rootless else 'sudo -n podman' }}" - XDG_RUNTIME_DIR: "/run/user/{{ tmachine_container_runtime_tmachine_uid.stdout }}" register: podman_test_result changed_when: false failed_when: false diff --git a/tests/artifacts.nix b/tests/artifacts.nix index 67efc9a7d4..73195e6c22 100644 --- a/tests/artifacts.nix +++ b/tests/artifacts.nix @@ -123,6 +123,7 @@ let "no_proxy" "podman_corporate_proxy" "podman_gateway_start" + "podman_oci_identity" "provider_auto_create" # The provider-refresh feature suite covers revoked Keycloak grants. This # binary instead covers stable workload handles across repeated rotations diff --git a/tests/suites/drivers/podman/tests/oci_identity.rs b/tests/suites/drivers/podman/tests/oci_identity.rs deleted file mode 100644 index a26653766c..0000000000 --- a/tests/suites/drivers/podman/tests/oci_identity.rs +++ /dev/null @@ -1,170 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -//! Podman container settings for a sandbox created from an image with a custom -//! `USER` and `WORKDIR` that the sandbox cannot observe from inside. -//! -//! The shared `oci-image` feature suite checks what the workload sees. This -//! test inspects the Podman containers directly: the workload runs the image -//! ID inspected before creation rather than the mutable tag, and is fenced by -//! `network=none`; the supervisor runs as the image user with no effective -//! capabilities. -//! -//! `OPENSHELL_TEST_CONTAINER_ENGINE` is the command that reaches the gateway's -//! Podman store, such as `podman` or `sudo -n podman`. - -mod support; - -use std::io::Write; -use std::process::{Command, Stdio}; -use std::time::Duration; - -use openshell_conformance::OpenShellRunner; - -use support::assert_podman_gateway; - -const ENGINE_ENV: &str = "OPENSHELL_TEST_CONTAINER_ENGINE"; -const CREATE_TIMEOUT: Duration = Duration::from_mins(10); -const IMAGE_USER: &str = "2345:2346"; - -#[tokio::test] -async fn podman_containers_use_pinned_image_and_image_user() { - let mut runner = OpenShellRunner::from_env("podman-oci-identity") - .expect("candidate openshell CLI is available"); - let tag = format!( - "localhost/openshell-test-podman-oci-identity:{}", - std::process::id() - ); - let result = async { - runner.check_gateway_status().await?; - assert_podman_gateway(&runner).await?; - - let containerfile = format!( - "FROM nvcr.io/nvidia/base/ubuntu:24.04 -RUN mkdir -p /workspace/project && chown {IMAGE_USER} /workspace/project -WORKDIR /workspace/project -USER {IMAGE_USER} -" - ); - podman(&["build", "--tag", &tag, "-"], Some(&containerfile))?; - let image_id = podman(&["image", "inspect", "--format", "{{.Id}}", &tag], None)?; - - let name = format!("po-{}", runner.id()); - runner.track_sandbox(&name); - runner - .step("oci-identity/create") - .description("sandbox starts from the test image") - .with_timeout(CREATE_TIMEOUT) - .run(&[ - "sandbox", "create", "--name", &name, "--from", &tag, "--detach", "--", "sleep", - "infinity", - ]) - .await - .map_err(|error| error.to_string())? - .require_success()?; - - let workload = container_for_role(&name, "sandbox")?; - let supervisor = container_for_role(&name, "supervisor")?; - for (what, container, format, expected) in [ - ("workload image", &workload, "{{.Image}}", image_id.as_str()), - ("workload user", &workload, "{{.Config.User}}", IMAGE_USER), - ( - "workload network", - &workload, - "{{.HostConfig.NetworkMode}}", - "none", - ), - ( - "supervisor user", - &supervisor, - "{{.Config.User}}", - IMAGE_USER, - ), - ( - "supervisor capabilities", - &supervisor, - "{{.EffectiveCaps}}", - "[]", - ), - ] { - let actual = podman(&["inspect", "--format", format, container], None)?; - if actual.trim_start_matches("sha256:") != expected.trim_start_matches("sha256:") { - return Err(format!("{what} is {actual:?}, expected {expected:?}")); - } - } - Ok(()) - } - .await; - // Remove the tag only after the sandbox is gone. A test failure takes - // precedence over a cleanup failure. - let result = runner.finish(result).await; - let cleanup = podman(&["image", "rm", &tag], None); - if let Err(error) = result { - panic!("Podman OCI identity failed:\n{error}"); - } - cleanup.expect("remove test image"); -} - -/// The one container with `role` for `sandbox`. -fn container_for_role(sandbox: &str, role: &str) -> Result { - let ids = podman( - &[ - "ps", - "-aq", - "--filter", - &format!("label=openshell.ai/sandbox-name={sandbox}"), - "--filter", - &format!("label=openshell.ai/isolation-role={role}"), - ], - None, - )?; - match ids.lines().collect::>().as_slice() { - [id] => Ok((*id).to_string()), - ids => Err(format!( - "expected one {role} container for sandbox {sandbox}, found {ids:?}" - )), - } -} - -/// Run a command against the gateway's Podman store and return its stdout. -fn podman(args: &[&str], stdin: Option<&str>) -> Result { - let engine = std::env::var(ENGINE_ENV) - .map_err(|_| format!("{ENGINE_ENV} must name the gateway's Podman command"))?; - let command = format!("{engine} {}", args.join(" ")); - let mut engine = engine.split_whitespace(); - let program = engine - .next() - .ok_or_else(|| format!("{ENGINE_ENV} is empty"))?; - let mut child = Command::new(program) - .args(engine) - .args(args) - .stdin(if stdin.is_some() { - Stdio::piped() - } else { - Stdio::null() - }) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .map_err(|error| format!("failed to run {command}: {error}"))?; - if let Some(input) = stdin { - child - .stdin - .take() - .expect("piped stdin") - .write_all(input.as_bytes()) - .map_err(|error| format!("write stdin of {command}: {error}"))?; - } - let output = child - .wait_with_output() - .map_err(|error| format!("wait for {command}: {error}"))?; - if !output.status.success() { - return Err(format!( - "{command} failed ({}):\n{}{}", - output.status, - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) - )); - } - Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) -} From 3c0990e6fcad85ce4fbcd91e74c2b570d47556d7 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Mon, 12 Oct 2026 11:25:11 -0700 Subject: [PATCH 13/16] refactor(oci): simplify workspace failure reporting and OCI image suite Carry the Docker control supervisor's failure reason in a typed error instead of matching status text, build the fixed workspace message where the wait result is classified, and drop the fake Docker server test. Inline image cleanup ordering in the suite, read the container engine once, share sandbox naming, and stop granting a literal /sandbox in the image identity policy. Add e2e:oci-image mise tasks and use them from the ARM64 Docker workflow and TESTING.md. Signed-off-by: Matthew Grossman --- .github/workflows/e2e-docker-test.yml | 2 +- TESTING.md | 10 +- crates/openshell-driver-docker/src/lib.rs | 159 ++++++++-------- crates/openshell-driver-docker/src/tests.rs | 92 ++++------ crates/openshell-supervisor/src/main.rs | 3 +- tasks/test.toml | 8 + .../features/oci-image/tests/oci_image.rs | 172 ++++++------------ 7 files changed, 192 insertions(+), 254 deletions(-) diff --git a/.github/workflows/e2e-docker-test.yml b/.github/workflows/e2e-docker-test.yml index 9784949e24..c7f21684a1 100644 --- a/.github/workflows/e2e-docker-test.yml +++ b/.github/workflows/e2e-docker-test.yml @@ -29,7 +29,7 @@ on: required: false type: string default: >- - [{"suite":"python","cmd":"mise run --no-deps --skip-deps e2e:python","apt_packages":"","python_proto":true,"mcp":false},{"suite":"oidc-python","cmd":"mise run --no-deps --skip-deps e2e:oidc-python:docker","apt_packages":"","python_proto":true,"mcp":false},{"suite":"oidc-pkce-docker","cmd":"mise run --no-deps --skip-deps e2e:oidc-pkce:docker","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"rust-docker","cmd":"mise run --no-deps --skip-deps e2e:rust","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"oci-image","cmd":"OPENSHELL_TEST_CONTAINER_ENGINE=docker e2e/with-docker-gateway.sh cargo test --locked --manifest-path tests/suites/features/Cargo.toml -p openshell-test-feature-oci-image -- --test-threads 1 --nocapture","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"mcp","cmd":"mise run --no-deps --skip-deps e2e:mcp","apt_packages":"","python_proto":false,"mcp":true}] + [{"suite":"python","cmd":"mise run --no-deps --skip-deps e2e:python","apt_packages":"","python_proto":true,"mcp":false},{"suite":"oidc-python","cmd":"mise run --no-deps --skip-deps e2e:oidc-python:docker","apt_packages":"","python_proto":true,"mcp":false},{"suite":"oidc-pkce-docker","cmd":"mise run --no-deps --skip-deps e2e:oidc-pkce:docker","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"rust-docker","cmd":"mise run --no-deps --skip-deps e2e:rust","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"oci-image","cmd":"mise run --no-deps --skip-deps e2e:oci-image","apt_packages":"openssh-client","python_proto":false,"mcp":false},{"suite":"mcp","cmd":"mise run --no-deps --skip-deps e2e:mcp","apt_packages":"","python_proto":false,"mcp":true}] permissions: actions: read diff --git a/TESTING.md b/TESTING.md index 2fe14e16a7..c6e91401e4 100644 --- a/TESTING.md +++ b/TESTING.md @@ -336,14 +336,16 @@ and gateway plus the candidate multi-architecture sandbox and supervisor images through `e2e/with-docker-gateway.sh`. The tmachine lanes still use x86_64 artifacts; they do not replace ARM64 image coverage. -Run it against a local gateway by naming the command that builds images into -the gateway's image store: +Run it against a local Docker or Podman gateway: ```shell -OPENSHELL_TEST_CONTAINER_ENGINE=podman e2e/with-podman-gateway.sh \ - cargo test --manifest-path tests/suites/features/Cargo.toml -p openshell-test-feature-oci-image -- --test-threads 1 +mise run e2e:oci-image +mise run e2e:oci-image:podman ``` +Both tasks set `OPENSHELL_TEST_CONTAINER_ENGINE`, the command that builds test +images into the gateway's image store. + Run the VM-backed Rust CLI e2e suite: ```shell diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 5cfa80007a..d4726d19fb 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -1843,7 +1843,7 @@ impl DockerComputeDriver { .await { Ok(control) => control, - Err(status) => { + Err(failure) => { if self .lifecycle_event_fences .stop_requested(&sandbox.id, &sandbox.name) @@ -1865,8 +1865,8 @@ impl DockerComputeDriver { .await; cleanup_docker_boundary_state(sandbox, &self.config); return Err(DockerProvisioningFailure::new( - supervisor_start_failure_reason(&status, "ControlSupervisorStartFailed"), - status.message(), + failure.reason_or("ControlSupervisorStartFailed"), + failure.status.message(), )); } }; @@ -2154,17 +2154,17 @@ impl DockerComputeDriver { .await { Ok(process) => process, - Err(status) => { + Err(failure) => { handle_docker_runtime_failure( failure_context, - supervisor_start_failure_reason(&status, "ControlSupervisorExited"), + failure.reason_or("ControlSupervisorExited"), format!( "failed to start Docker control supervisor: {}", - status.message() + failure.status.message() ), ) .await; - return Err(status); + return Err(failure.status); } }; self.replace_control_process(&sandbox.id, process).await; @@ -5045,7 +5045,7 @@ async fn spawn_docker_control_process( sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig, failure_context: DockerRuntimeFailureContext, -) -> Result { +) -> Result { let directory = docker_boundary_state_dir(sandbox, config)?; let main_process_spec = tokio::fs::read_to_string(directory.join(MAIN_PROCESS_SPEC_FILE)) .await @@ -5229,9 +5229,7 @@ async fn spawn_docker_control_process( Some(RemoveContainerOptionsBuilder::default().force(true).build()), ) .await; - return Err(Status::internal(format!( - "start Docker supervisor container: {error}" - ))); + return Err(Status::internal(format!("start Docker supervisor container: {error}")).into()); } let sandbox_id = sandbox.id.clone(); let (shutdown, mut shutdown_requested) = oneshot::channel(); @@ -5288,8 +5286,9 @@ async fn spawn_docker_control_process( let log_tail = docker_container_log_tail(&monitored_docker, &monitored_supervisor_id).await; if reason == CONDITION_WORKSPACE_VALIDATION_FAILED { + // The condition message stays fixed; supervisor output may + // contain secrets. warn!(%sandbox_id, supervisor_logs = %log_tail, "Docker sandbox rejected the image working directory"); - message = WORKSPACE_VALIDATION_FAILED_MESSAGE.to_string(); } else if !log_tail.is_empty() { write!(message, "; log tail: {log_tail}").ok(); } @@ -5334,17 +5333,26 @@ async fn wait_for_docker_supervisor_ready( workload_container_id: &str, sandbox_id: &str, failures: &Arc>>, -) -> Result<(), Status> { - // The monitor records a workspace rejection before it removes the - // supervisor, so a later failed inspection must still report it. - match poll_docker_supervisor_ready(docker, supervisor_id, workload_container_id, sandbox_id) - .await - { - Ok(()) => Ok(()), - Err(status) => Err(recorded_workspace_validation_failure(failures, sandbox_id) +) -> Result<(), ControlStartFailure> { + let Err(failure) = + poll_docker_supervisor_ready(docker, supervisor_id, workload_container_id, sandbox_id) .await - .unwrap_or(status)), + else { + return Ok(()); + }; + // The monitor records a workspace rejection before it stops the workload + // and removes the supervisor, so readiness may only observe the cleanup. + // Other recorded failures keep the readiness status: their messages carry + // an untruncated log tail that must not travel in a gRPC status. + let recorded = failures + .lock() + .await + .get(sandbox_id) + .map(|failure| failure.reason); + if recorded == Some(CONDITION_WORKSPACE_VALIDATION_FAILED) { + return Err(ControlStartFailure::workspace_validation()); } + Err(failure) } async fn poll_docker_supervisor_ready( @@ -5352,7 +5360,7 @@ async fn poll_docker_supervisor_ready( supervisor_id: &str, workload_container_id: &str, sandbox_id: &str, -) -> Result<(), Status> { +) -> Result<(), ControlStartFailure> { // Gateway provisioning deadlines own startup expiration, including policy // repairs that extend the window. Unhealthy means not ready while the // supervisor is quarantined; only a stopped process is a startup failure. @@ -5366,7 +5374,8 @@ async fn poll_docker_supervisor_ready( if sandbox.state.unwrap_or_default().running == Some(false) { return Err(Status::unavailable( "Docker sandbox exited before supervisor became ready", - )); + ) + .into()); } let inspected = docker .inspect_container(supervisor_id, None) @@ -5377,91 +5386,91 @@ async fn poll_docker_supervisor_ready( let state = inspected.state.unwrap_or_default(); match state.health.and_then(|health| health.status) { Some(HealthStatusEnum::HEALTHY) => return Ok(()), - _ if state.running == Some(false) - && state.exit_code - == Some(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)) => - { - let log_tail = docker_container_log_tail(docker, supervisor_id).await; - warn!(sandbox_id, supervisor_id, supervisor_logs = %log_tail, "Docker sandbox rejected the image working directory"); - return Err(workspace_validation_status()); - } _ if state.running == Some(false) => { let log_tail = docker_container_log_tail(docker, supervisor_id).await; + if state.exit_code == Some(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)) { + warn!(sandbox_id, supervisor_id, supervisor_logs = %log_tail, "Docker sandbox rejected the image working directory"); + return Err(ControlStartFailure::workspace_validation()); + } warn!(sandbox_id, supervisor_id, supervisor_logs = %log_tail, "Docker supervisor exited before becoming ready"); return Err(Status::unavailable(format!( "Docker supervisor exited before becoming ready{}", format_log_tail(&log_tail) - ))); + )) + .into()); } _ => tokio::time::sleep(Duration::from_millis(100)).await, } } } +/// A control supervisor that failed to start. `reason` is set only when the +/// failure has a specific condition reason; callers choose their own default. +#[derive(Debug)] +struct ControlStartFailure { + reason: Option<&'static str>, + status: Status, +} + +impl From for ControlStartFailure { + fn from(status: Status) -> Self { + Self { + reason: None, + status, + } + } +} + +impl ControlStartFailure { + /// The sandbox rejected the image working directory. The message is fixed + /// text, because supervisor output may contain secrets. + fn workspace_validation() -> Self { + Self { + reason: Some(CONDITION_WORKSPACE_VALIDATION_FAILED), + status: Status::failed_precondition(WORKSPACE_VALIDATION_FAILED_MESSAGE), + } + } + + fn reason_or(&self, default: &'static str) -> &'static str { + self.reason.unwrap_or(default) + } +} + +/// Condition reason and message for a supervisor that exited unexpectedly. +/// /// Bollard reports nonzero Docker wait statuses as errors, not successful /// responses. Both forms must preserve the sandbox's workspace failure reason. fn docker_supervisor_wait_failure( result: Option>, ) -> (&'static str, String) { - match result { + let (exit_code, message) = match result { Some(Ok(status)) => ( - supervisor_exit_reason(status.status_code), + Some(status.status_code), format!( "Docker supervisor container exited with status {}", status.status_code ), ), Some(Err(BollardError::DockerContainerWaitError { code, error })) => ( - supervisor_exit_reason(code), + Some(code), format!("Docker supervisor container exited with status {code}: {error}"), ), Some(Err(error)) => ( - "ControlSupervisorExited", + None, format!("failed to wait for Docker supervisor container: {error}"), ), None => ( - "ControlSupervisorExited", + None, "Docker supervisor wait stream ended unexpectedly".to_string(), ), - } -} - -async fn recorded_workspace_validation_failure( - failures: &Arc>>, - sandbox_id: &str, -) -> Option { - let failures = failures.lock().await; - failures - .get(sandbox_id) - .filter(|failure| failure.reason == CONDITION_WORKSPACE_VALIDATION_FAILED) - .map(|_| workspace_validation_status()) -} - -/// Startup failure for a supervisor that exited because the sandbox rejected -/// the image working directory. The message is fixed text, because supervisor -/// output may contain secrets, and identifies the failure for -/// [`supervisor_start_failure_reason`]. -fn workspace_validation_status() -> Status { - Status::failed_precondition(WORKSPACE_VALIDATION_FAILED_MESSAGE) -} - -/// Condition reason for a supervisor that failed before becoming ready. -fn supervisor_start_failure_reason(status: &Status, default: &'static str) -> &'static str { - if status.code() == tonic::Code::FailedPrecondition - && status.message() == WORKSPACE_VALIDATION_FAILED_MESSAGE - { - CONDITION_WORKSPACE_VALIDATION_FAILED - } else { - default - } -} - -/// Condition reason for a supervisor that exited with `status_code`. -fn supervisor_exit_reason(status_code: i64) -> &'static str { - if status_code == i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED) { - CONDITION_WORKSPACE_VALIDATION_FAILED + }; + if exit_code == Some(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)) { + ( + CONDITION_WORKSPACE_VALIDATION_FAILED, + WORKSPACE_VALIDATION_FAILED_MESSAGE.to_string(), + ) } else { - "ControlSupervisorExited" + ("ControlSupervisorExited", message) } } diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index 38e8ea4d69..06abffde68 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -3740,32 +3740,25 @@ fn docker_oom_kill_stays_terminal_despite_137() { #[test] fn supervisor_workspace_validation_exit_is_reported_explicitly() { - let status = workspace_validation_status(); - assert_eq!(status.message(), WORKSPACE_VALIDATION_FAILED_MESSAGE); - assert_eq!( - supervisor_start_failure_reason(&status, "ControlSupervisorStartFailed"), - CONDITION_WORKSPACE_VALIDATION_FAILED - ); - assert_eq!( - supervisor_start_failure_reason( - &Status::failed_precondition("provider SPIFFE socket has no parent directory"), - "ControlSupervisorStartFailed" - ), - "ControlSupervisorStartFailed" - ); - assert_eq!( - supervisor_exit_reason(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)), - CONDITION_WORKSPACE_VALIDATION_FAILED - ); - assert_eq!(supervisor_exit_reason(1), "ControlSupervisorExited"); - - let (reason, message) = - docker_supervisor_wait_failure(Some(Err(BollardError::DockerContainerWaitError { - code: i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED), + let workspace_exit = i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED); + for result in [ + Ok(ContainerWaitResponse { + status_code: workspace_exit, + error: None, + }), + Err(BollardError::DockerContainerWaitError { + code: workspace_exit, error: "image workspace validation failed".into(), - }))); - assert_eq!(reason, CONDITION_WORKSPACE_VALIDATION_FAILED); - assert!(message.contains("status 78")); + }), + ] { + assert_eq!( + docker_supervisor_wait_failure(Some(result)), + ( + CONDITION_WORKSPACE_VALIDATION_FAILED, + WORKSPACE_VALIDATION_FAILED_MESSAGE.to_string() + ) + ); + } assert_eq!( docker_supervisor_wait_failure(Some(Err(BollardError::DockerContainerWaitError { code: 1, @@ -3774,36 +3767,23 @@ fn supervisor_workspace_validation_exit_is_reported_explicitly() { .0, "ControlSupervisorExited" ); -} - -#[tokio::test] -async fn docker_wait_exit_78_keeps_workspace_failure_reason() { - use tokio::io::{AsyncReadExt, AsyncWriteExt}; - let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); - let address = listener.local_addr().unwrap(); - let server = tokio::spawn(async move { - let (mut socket, _) = listener.accept().await.unwrap(); - let mut request = [0; 4096]; - let size = socket.read(&mut request).await.unwrap(); - assert!(String::from_utf8_lossy(&request[..size]).contains("/containers/supervisor/wait")); - let body = r#"{"StatusCode":78,"Error":null}"#; - let response = format!( - "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", - body.len() - ); - socket.write_all(response.as_bytes()).await.unwrap(); - }); - let docker = - Docker::connect_with_http(&address.to_string(), 5, bollard::API_DEFAULT_VERSION).unwrap(); - let mut wait = docker.wait_container( - "supervisor", - None::, + let failure = ControlStartFailure::workspace_validation(); + assert_eq!( + failure.reason_or("ControlSupervisorStartFailed"), + CONDITION_WORKSPACE_VALIDATION_FAILED + ); + assert_eq!( + failure.status.message(), + WORKSPACE_VALIDATION_FAILED_MESSAGE + ); + assert_eq!( + ControlStartFailure::from(Status::failed_precondition( + WORKSPACE_VALIDATION_FAILED_MESSAGE + )) + .reason_or("ControlSupervisorStartFailed"), + "ControlSupervisorStartFailed" ); - let (reason, message) = docker_supervisor_wait_failure(wait.next().await); - assert_eq!(reason, CONDITION_WORKSPACE_VALIDATION_FAILED); - assert!(message.contains("status 78")); - server.await.unwrap(); } #[tokio::test] @@ -3812,13 +3792,13 @@ async fn workspace_failure_survives_supervisor_cleanup_before_readiness_inspecti "sandbox-1".to_string(), DockerRuntimeFailure { reason: CONDITION_WORKSPACE_VALIDATION_FAILED, - message: "Docker supervisor container exited with status 78".into(), + message: WORKSPACE_VALIDATION_FAILED_MESSAGE.into(), }, )]))); // There is no Docker daemon at this address: a monitor that has removed // the supervisor must not need another inspection to retain the reason. let docker = Docker::connect_with_http("127.0.0.1:1", 1, bollard::API_DEFAULT_VERSION).unwrap(); - let status = wait_for_docker_supervisor_ready( + let failure = wait_for_docker_supervisor_ready( &docker, "removed-supervisor", "workload-container-1", @@ -3828,7 +3808,7 @@ async fn workspace_failure_survives_supervisor_cleanup_before_readiness_inspecti .await .expect_err("the recorded workspace failure takes precedence over inspection"); assert_eq!( - supervisor_start_failure_reason(&status, "ControlSupervisorStartFailed"), + failure.reason_or("ControlSupervisorStartFailed"), CONDITION_WORKSPACE_VALIDATION_FAILED ); } diff --git a/crates/openshell-supervisor/src/main.rs b/crates/openshell-supervisor/src/main.rs index f078523a8c..45343da980 100644 --- a/crates/openshell-supervisor/src/main.rs +++ b/crates/openshell-supervisor/src/main.rs @@ -494,8 +494,7 @@ fn supervisor_exit(result: Result) -> Result { .downcast_ref::() .is_some() => { - error!("Image workspace validation failed"); - eprintln!("{error:?}"); + error!(error = ?error, "Image workspace validation failed"); Ok(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED) } other => other, diff --git a/tasks/test.toml b/tasks/test.toml index 74ee585205..77b34dcc45 100644 --- a/tasks/test.toml +++ b/tasks/test.toml @@ -131,6 +131,14 @@ hide = true description = "Run Rust CLI e2e tests against a Docker-backed gateway" run = "e2e/rust/e2e-docker.sh" +["e2e:oci-image"] +description = "Run the shared OCI image feature suite against a Docker-backed gateway" +run = "OPENSHELL_TEST_CONTAINER_ENGINE=docker e2e/with-docker-gateway.sh cargo test --locked --manifest-path tests/suites/features/Cargo.toml -p openshell-test-feature-oci-image -- --test-threads 1 --nocapture" + +["e2e:oci-image:podman"] +description = "Run the shared OCI image feature suite against a Podman-backed gateway" +run = "OPENSHELL_TEST_CONTAINER_ENGINE=podman e2e/with-podman-gateway.sh cargo test --locked --manifest-path tests/suites/features/Cargo.toml -p openshell-test-feature-oci-image -- --test-threads 1 --nocapture" + ["e2e:workload:build"] description = "Build the Docker E2E workload fixture" run = "CONTAINER_ENGINE=docker bash tasks/scripts/e2e-build-workload.sh" diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index 1dd7be39cc..7a3005d7a6 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -12,7 +12,6 @@ //! is the command that builds images into the gateway's image store, such as //! `docker`, `podman`, or `sudo -n podman`. -use std::future::Future; use std::process::Command; use std::time::Duration; @@ -32,7 +31,7 @@ const IMAGE_IDENTITY_POLICY: &str = "version: 1 filesystem_policy: include_workdir: true read_only: [/usr, /lib, /lib64, /proc, /dev/urandom, /etc] - read_write: [/sandbox, /tmp, /dev/null] + read_write: [/tmp, /dev/null] landlock: compatibility: best_effort @@ -46,8 +45,7 @@ async fn custom_workdir_with_named_user() { run( "oci-image/custom-workdir-named-user", async |runner, images| { - let image = TestImage::build( - images, + let image = images.build( "named-workdir", &format!( "FROM {BASE_IMAGE} @@ -65,7 +63,7 @@ USER app test \"$(stat -c %u:%g /etc/oci-image-marker)\" = 0:0;", workspace_checks("1234:1235", "/workspace/project", true) ); - let sandbox = create_sandbox(runner, "named", "nu", image, None, &checks).await?; + let sandbox = create_sandbox(runner, "nu", &image, None, &checks).await?; file_transfer_uses_workspace(runner, &sandbox).await }, ) @@ -80,8 +78,7 @@ async fn custom_workdir_with_numeric_user_and_private_parents() { run( "oci-image/custom-workdir-numeric-user", async |runner, images| { - let image = TestImage::build( - images, + let image = images.build( "numeric-workdir", &format!( "FROM {BASE_IMAGE} @@ -103,7 +100,7 @@ USER 2345:2346 .to_str() .ok_or("policy path is not UTF-8")?; let checks = workspace_checks("2345:2346", "/home/app/project", true); - create_sandbox(runner, "numeric", "pu", image, Some(policy), &checks) + create_sandbox(runner, "pu", &image, Some(policy), &checks) .await .map(drop) }, @@ -116,13 +113,12 @@ USER 2345:2346 #[tokio::test] async fn default_workdir_uses_managed_workspace() { run("oci-image/default-workdir", async |runner, images| { - let image = TestImage::build( - images, + let image = images.build( "default-workdir", &format!("FROM {BASE_IMAGE}\nUSER 2345:2346\n"), )?; let checks = workspace_checks("2345:2346", "/sandbox", false); - create_sandbox(runner, "default", "dw", image, None, &checks) + create_sandbox(runner, "dw", &image, None, &checks) .await .map(drop) }) @@ -138,8 +134,7 @@ async fn unwritable_custom_workdir_is_rejected() { // First prove that this user, workspace and command can run. Only the // directory owner differs between the control and rejected image. for (suffix, owner, writable) in [("wc", "3234:3235", true), ("uw", "0:0", false)] { - let image = TestImage::build( - images, + let image = images.build( suffix, &format!( "FROM {BASE_IMAGE} @@ -150,8 +145,7 @@ USER app " ), )?; - let name = format!("oi-{}-{suffix}", runner.id()); - runner.track_sandbox(&name); + let name = track_sandbox(runner, suffix); let create = runner .step(format!("{suffix}/create")) .description(if writable { @@ -166,7 +160,7 @@ USER app "--name", &name, "--from", - &image.tag, + &image, "--no-tty", "--", "sh", @@ -204,87 +198,32 @@ USER app async fn run( scenario: &str, - test: impl AsyncFnOnce(&mut OpenShellRunner, &mut Vec) -> Result<(), String>, + test: impl AsyncFnOnce(&mut OpenShellRunner, &mut TestImages) -> Result<(), String>, ) { let mut runner = OpenShellRunner::from_env(scenario).expect("candidate openshell CLI is available"); - let mut images = Vec::new(); + let mut images = TestImages::from_env(); let result = async { runner.check_gateway_status().await?; test(&mut runner, &mut images).await } .await; - if let Err(error) = - finish_with_image_cleanup(runner.finish(result), &images, TestImage::remove).await - { - panic!("{scenario} failed:\n{error}"); + // Delete sandboxes before their images. Non-forced image removal must + // report remaining users rather than deleting workloads behind the + // gateway's back. Keep functional and sandbox failures first. + let mut errors: Vec<_> = runner.finish(result).await.err().into_iter().collect(); + errors.extend(images.remove_all()); + if !errors.is_empty() { + panic!("{scenario} failed:\n{}", errors.join("\n")); } } -/// Finish OpenShell sandbox cleanup before removing image tags. Non-forced -/// image removal must report remaining users rather than deleting workloads -/// behind the gateway's back. Keep functional/sandbox failures primary. -async fn finish_with_image_cleanup( - finish: impl Future>, - images: &[T], - mut remove: impl FnMut(&T) -> Result<(), String>, -) -> Result<(), String> { - let result = finish.await; - let errors: Vec<_> = images - .iter() - .filter_map(|image| remove(image).err()) - .collect(); - if errors.is_empty() { - return result; - } - let cleanup = format!("image cleanup failed:\n{}", errors.join("\n")); - match result { - Ok(()) => Err(cleanup), - Err(primary) => Err(format!("{primary}\n{cleanup}")), - } -} - -#[tokio::test] -async fn sandbox_cleanup_precedes_image_removal_on_success_and_failure() { - use std::cell::RefCell; - - for fail_scenario in [false, true] { - for fail_cleanup in [false, true] { - let events = RefCell::new(Vec::new()); - let finish = async { - events.borrow_mut().push("sandbox delete"); - if fail_scenario { - Err("functional failure".to_string()) - } else { - Ok(()) - } - }; - let result = finish_with_image_cleanup(finish, &["first", "second"], |image| { - events.borrow_mut().push(image); - if fail_cleanup { - Err(format!("cannot remove {image}")) - } else { - Ok(()) - } - }) - .await; - assert_eq!(*events.borrow(), ["sandbox delete", "first", "second"]); - assert_eq!(result.is_err(), fail_scenario || fail_cleanup); - if fail_scenario { - assert!( - result - .as_ref() - .unwrap_err() - .starts_with("functional failure") - ); - } - if fail_cleanup { - let error = result.unwrap_err(); - assert!(error.contains("cannot remove first")); - assert!(error.contains("cannot remove second")); - } - } - } +/// Name a sandbox for this scenario and register it for cleanup. Sandbox +/// names are limited to 19 characters on some drivers. +fn track_sandbox(runner: &mut OpenShellRunner, short: &str) -> String { + let name = format!("oi-{}-{short}", runner.id()); + runner.track_sandbox(&name); + name } /// Shell checks for the identity, working directory, and `HOME` of a sandbox @@ -322,27 +261,24 @@ fn workspace_checks(identity: &str, workspace: &str, image_file: bool) -> String /// the workspace, then run the same checks and a write through `sandbox exec`. async fn create_sandbox( runner: &mut OpenShellRunner, - suffix: &str, short: &str, - image: &TestImage, + image: &str, policy: Option<&str>, checks: &str, ) -> Result { - // Sandbox names are limited to 19 characters on some drivers. - let name = format!("oi-{}-{short}", runner.id()); - runner.track_sandbox(&name); + let name = track_sandbox(runner, short); let main = format!( "(set -eu; {checks} touch main-write) >/tmp/oci-main.log 2>&1; \ echo $? >/tmp/oci-main.status; exec sleep infinity" ); - let mut args = vec!["sandbox", "create", "--name", &name, "--from", &image.tag]; + let mut args = vec!["sandbox", "create", "--name", &name, "--from", image]; if let Some(policy) = policy { args.extend(["--policy", policy]); } args.extend(["--detach", "--", "sh", "-c", &main]); run_ok( runner, - format!("{suffix}/create"), + format!("{short}/create"), "sandbox starts from the test image", CREATE_TIMEOUT, &args, @@ -363,7 +299,7 @@ async fn create_sandbox( exec( runner, &name, - format!("{suffix}/exec"), + format!("{short}/exec"), "main process and exec children see the image workspace", &script, ) @@ -495,49 +431,53 @@ async fn exec( .await } -/// An image owned by `run`, kept alive until sandbox cleanup completes. -struct TestImage { +/// Images built for one scenario, removed by `run` after sandbox cleanup. +struct TestImages { engine: Vec, - tag: String, + tags: Vec, } -impl TestImage { - fn build<'a>( - images: &'a mut Vec, - name: &str, - containerfile: &str, - ) -> Result<&'a Self, String> { +impl TestImages { + fn from_env() -> Self { let engine: Vec = std::env::var(ENGINE_ENV) - .map_err(|_| format!("{ENGINE_ENV} must name the gateway's container engine"))? + .unwrap_or_else(|_| panic!("{ENGINE_ENV} must name the gateway's container engine")) .split_whitespace() .map(str::to_string) .collect(); - if engine.is_empty() { - return Err(format!("{ENGINE_ENV} is empty")); + assert!(!engine.is_empty(), "{ENGINE_ENV} is empty"); + Self { + engine, + tags: Vec::new(), } + } + + /// Build `containerfile` and return the image tag. + fn build(&mut self, name: &str, containerfile: &str) -> Result { let context = tempfile::tempdir().map_err(|error| format!("create context: {error}"))?; let file = context.path().join("Containerfile"); std::fs::write(&file, containerfile) .map_err(|error| format!("write Containerfile: {error}"))?; - let image = Self { - engine, - tag: format!("localhost/openshell-test-oci-{name}:{}", std::process::id()), - }; - image.engine_command(&[ + let tag = format!("localhost/openshell-test-oci-{name}:{}", std::process::id()); + self.engine_command(&[ "build", "--file", file.to_str().ok_or("Containerfile path is not UTF-8")?, "--tag", - &image.tag, + &tag, context.path().to_str().ok_or("context path is not UTF-8")?, ])?; // A failed build leaves no tag to remove. - images.push(image); - Ok(images.last().expect("registered image")) + self.tags.push(tag.clone()); + Ok(tag) } - fn remove(&self) -> Result<(), String> { - self.engine_command(&["image", "rm", &self.tag]) + /// Remove every built image, returning one error per failed removal. + fn remove_all(&self) -> Vec { + self.tags + .iter() + .filter_map(|tag| self.engine_command(&["image", "rm", tag]).err()) + .map(|error| format!("image cleanup failed: {error}")) + .collect() } fn engine_command(&self, args: &[&str]) -> Result<(), String> { From 7fdecf3413ced3c1f42a0e7171b73932c58bd804 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Mon, 12 Oct 2026 13:40:17 -0700 Subject: [PATCH 14/16] test(oci): move shared container checks into the OCI image suite Inspect the containers the driver created through the gateway's engine in the shared suite: the workload runs the inspected image ID as the OCI identity without a network or supervisor credential mounts, and the supervisor is a separate non-root container. Keep only the Podman-specific supervisor identity and capability checks in podman_oci_identity. Signed-off-by: Matthew Grossman --- e2e/rust/tests/podman_oci_identity.rs | 94 ++--------- .../features/oci-image/tests/oci_image.rs | 154 +++++++++++++----- 2 files changed, 128 insertions(+), 120 deletions(-) diff --git a/e2e/rust/tests/podman_oci_identity.rs b/e2e/rust/tests/podman_oci_identity.rs index d13f35c91e..6b137d1a2b 100644 --- a/e2e/rust/tests/podman_oci_identity.rs +++ b/e2e/rust/tests/podman_oci_identity.rs @@ -3,17 +3,15 @@ #![cfg(feature = "e2e-podman")] -//! Podman-specific E2E coverage for immutable-image launch and the isolated -//! workload/supervisor container pair. +//! Podman-specific E2E coverage for the supervisor companion container. //! -//! The test builds an image through the selected Podman engine, creates a -//! sandbox from its mutable tag, and inspects the real Podman containers: the -//! image ID recorded on the workload, each container's user, the supervisor's -//! capabilities, networking, and mounts. This exercises the Podman API -//! inspect → protected metadata → create path rather than only its unit -//! serialization boundaries. The sandbox identity, privileges, and workspace -//! seen by the main process and `sandbox exec` are shared with Docker and -//! covered by the `oci-image` feature suite in `tests/suites/features`. +//! Podman runs the supervisor as the resolved OCI workload identity with no +//! effective capabilities; Docker runs it as a fixed unprivileged user. The +//! test builds an image through the selected Podman engine, creates a sandbox +//! from it, and inspects the real supervisor container. The workload's image +//! ID, identity, network, and mounts, and what the main process and +//! `sandbox exec` see, are shared with Docker and covered by the `oci-image` +//! feature suite in `tests/suites/features`. use std::process::Stdio; @@ -39,7 +37,6 @@ network_policies: {} struct ImageGuard { engine: ContainerEngine, tag: String, - id: String, } impl ImageGuard { @@ -90,7 +87,6 @@ USER {OCI_UID}:{OCI_GID} .ok_or_else(|| "build context path is not UTF-8".to_string())?, ], )?; - let id = run_engine(&engine, &["image", "inspect", "--format", "{{.Id}}", &tag])?; let user = run_engine( &engine, &["image", "inspect", "--format", "{{.Config.User}}", &tag], @@ -101,7 +97,7 @@ USER {OCI_UID}:{OCI_GID} )); } - Ok(Self { engine, tag, id }) + Ok(Self { engine, tag }) } } @@ -138,10 +134,6 @@ fn run_engine(engine: &ContainerEngine, args: &[&str]) -> Result Ok(stdout.trim().to_string()) } -fn sandbox_container_id(engine: &ContainerEngine, sandbox_name: &str) -> Result { - container_id_for_role(engine, sandbox_name, "sandbox") -} - fn container_id_for_role( engine: &ContainerEngine, sandbox_name: &str, @@ -178,15 +170,8 @@ fn container_id_for_role( } } -fn normalized_image_id(image_id: &str) -> &str { - image_id - .trim() - .strip_prefix("sha256:") - .unwrap_or(image_id.trim()) -} - #[tokio::test] -async fn podman_uses_oci_identity_and_inspected_image_id() { +async fn podman_supervisor_runs_as_oci_identity_without_capabilities() { if !is_e2e_driver("podman") { eprintln!("Skipping Podman OCI identity test: e2e driver is not podman"); return; @@ -207,43 +192,8 @@ async fn podman_uses_oci_identity_and_inspected_image_id() { .await .expect("create sandbox from Podman-built OCI identity image"); - let container_id = - sandbox_container_id(&image.engine, &sandbox.name).expect("find Podman sandbox container"); - let launched_image_id = run_engine( - &image.engine, - &[ - "container", - "inspect", - "--format", - "{{.Image}}", - &container_id, - ], - ) - .expect("inspect Podman sandbox container image"); - assert_eq!( - normalized_image_id(&launched_image_id), - normalized_image_id(&image.id), - "Podman sandbox must launch the immutable image ID inspected before creation" - ); - - assert_isolated_pair(&image, &sandbox, &container_id); - sandbox.cleanup().await; -} - -fn assert_isolated_pair(image: &ImageGuard, sandbox: &SandboxGuard, container_id: &str) { let supervisor_id = container_id_for_role(&image.engine, &sandbox.name, "supervisor") .expect("find separate supervisor companion"); - assert_ne!(supervisor_id, container_id); - let workload_user = run_engine( - &image.engine, - &["inspect", "--format", "{{.Config.User}}", container_id], - ) - .unwrap(); - assert_eq!( - workload_user, - format!("{OCI_UID}:{OCI_GID}"), - "a custom OCI workspace must start directly as the final identity" - ); let supervisor_user = run_engine( &image.engine, &["inspect", "--format", "{{.Config.User}}", &supervisor_id], @@ -259,27 +209,5 @@ fn assert_isolated_pair(image: &ImageGuard, sandbox: &SandboxGuard, container_id supervisor_caps, "[]", "the supervisor companion may not have effective capabilities" ); - let network = run_engine( - &image.engine, - &[ - "inspect", - "--format", - "{{.HostConfig.NetworkMode}}", - container_id, - ], - ) - .unwrap(); - assert_eq!(network, "none"); - let mounts = run_engine( - &image.engine, - &[ - "inspect", - "--format", - "{{range .Mounts}}{{println .Destination}}{{end}}", - container_id, - ], - ) - .unwrap(); - assert!(!mounts.contains("/etc/openshell/tls")); - assert!(!mounts.contains("/.openshell/supervisor")); + sandbox.cleanup().await; } diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index 7a3005d7a6..59c975ba7e 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -6,10 +6,11 @@ //! Each test builds a small image with the container engine that backs the //! target gateway, creates a sandbox from it through the candidate CLI, and //! checks the process identity, privileges, workspace, and image content seen -//! by both the sandbox main process and `sandbox exec`. +//! by both the sandbox main process and `sandbox exec`. One scenario also +//! inspects the containers the driver created, through the same engine. //! //! `OPENSHELL_BIN` names the candidate CLI. `OPENSHELL_TEST_CONTAINER_ENGINE` -//! is the command that builds images into the gateway's image store, such as +//! is the command for the gateway's container engine and image store, such as //! `docker`, `podman`, or `sudo -n podman`. use std::process::Command; @@ -44,8 +45,8 @@ network_policies: {} async fn custom_workdir_with_named_user() { run( "oci-image/custom-workdir-named-user", - async |runner, images| { - let image = images.build( + async |runner, engine| { + let image = engine.build_image( "named-workdir", &format!( "FROM {BASE_IMAGE} @@ -73,12 +74,14 @@ USER app /// A numeric image user without passwd entries can reach a custom `WORKDIR` /// whose parent directories are private to that user. The sandbox policy omits /// `process`, so the image `USER` is the only source of the sandbox identity. +/// The driver's containers launch the inspected image and keep the +/// supervisor separate from the workload. #[tokio::test] async fn custom_workdir_with_numeric_user_and_private_parents() { run( "oci-image/custom-workdir-numeric-user", - async |runner, images| { - let image = images.build( + async |runner, engine| { + let image = engine.build_image( "numeric-workdir", &format!( "FROM {BASE_IMAGE} @@ -100,9 +103,8 @@ USER 2345:2346 .to_str() .ok_or("policy path is not UTF-8")?; let checks = workspace_checks("2345:2346", "/home/app/project", true); - create_sandbox(runner, "pu", &image, Some(policy), &checks) - .await - .map(drop) + let sandbox = create_sandbox(runner, "pu", &image, Some(policy), &checks).await?; + container_checks(engine, &sandbox, &image, "2345:2346") }, ) .await; @@ -112,8 +114,8 @@ USER 2345:2346 /// by the image user, even when the image does not contain `/sandbox`. #[tokio::test] async fn default_workdir_uses_managed_workspace() { - run("oci-image/default-workdir", async |runner, images| { - let image = images.build( + run("oci-image/default-workdir", async |runner, engine| { + let image = engine.build_image( "default-workdir", &format!("FROM {BASE_IMAGE}\nUSER 2345:2346\n"), )?; @@ -130,11 +132,11 @@ async fn default_workdir_uses_managed_workspace() { /// rather than a generic startup failure. #[tokio::test] async fn unwritable_custom_workdir_is_rejected() { - run("oci-image/unwritable-workdir", async |runner, images| { + run("oci-image/unwritable-workdir", async |runner, engine| { // First prove that this user, workspace and command can run. Only the // directory owner differs between the control and rejected image. for (suffix, owner, writable) in [("wc", "3234:3235", true), ("uw", "0:0", false)] { - let image = images.build( + let image = engine.build_image( suffix, &format!( "FROM {BASE_IMAGE} @@ -198,21 +200,21 @@ USER app async fn run( scenario: &str, - test: impl AsyncFnOnce(&mut OpenShellRunner, &mut TestImages) -> Result<(), String>, + test: impl AsyncFnOnce(&mut OpenShellRunner, &mut Engine) -> Result<(), String>, ) { let mut runner = OpenShellRunner::from_env(scenario).expect("candidate openshell CLI is available"); - let mut images = TestImages::from_env(); + let mut engine = Engine::from_env(); let result = async { runner.check_gateway_status().await?; - test(&mut runner, &mut images).await + test(&mut runner, &mut engine).await } .await; // Delete sandboxes before their images. Non-forced image removal must // report remaining users rather than deleting workloads behind the // gateway's back. Keep functional and sandbox failures first. let mut errors: Vec<_> = runner.finish(result).await.err().into_iter().collect(); - errors.extend(images.remove_all()); + errors.extend(engine.remove_images()); if !errors.is_empty() { panic!("{scenario} failed:\n{}", errors.join("\n")); } @@ -393,6 +395,59 @@ async fn file_transfer_uses_workspace( .await } +/// Check the containers the driver created for `sandbox`. The workload runs +/// the image ID inspected before creation, so retagging cannot swap it, and +/// starts directly as the sandbox `identity` without a network or the +/// supervisor's credentials. The supervisor runs in a separate non-root +/// container. +fn container_checks( + engine: &Engine, + sandbox: &str, + image: &str, + identity: &str, +) -> Result<(), String> { + let workload = engine.container_for_role(sandbox, "sandbox")?; + let supervisor = engine.container_for_role(sandbox, "supervisor")?; + if workload == supervisor { + return Err(format!("{sandbox} has no separate supervisor container")); + } + let image_id = engine.output(&["image", "inspect", "--format", "{{.Id}}", image])?; + let launched = engine.inspect_container(&workload, "{{.Image}}")?; + let digest = |id: &str| id.trim_start_matches("sha256:").to_string(); + if digest(&launched) != digest(&image_id) { + return Err(format!( + "workload launched image {launched}, expected the inspected {image_id}" + )); + } + let user = engine.inspect_container(&workload, "{{.Config.User}}")?; + if user != identity { + return Err(format!("workload starts as {user:?}, expected {identity}")); + } + let network = engine.inspect_container(&workload, "{{.HostConfig.NetworkMode}}")?; + if network != "none" { + return Err(format!( + "workload network mode is {network:?}, expected none" + )); + } + let mounts = engine.inspect_container( + &workload, + "{{range .Mounts}}{{println .Destination}}{{end}}", + )?; + if let Some(mount) = mounts.lines().find(|mount| { + mount.starts_with("/etc/openshell/tls") || mount.starts_with("/.openshell/supervisor") + }) { + return Err(format!("workload mounts supervisor credentials at {mount}")); + } + let supervisor_user = engine.inspect_container(&supervisor, "{{.Config.User}}")?; + if matches!( + supervisor_user.split(':').next(), + None | Some("" | "0" | "root") + ) { + return Err(format!("supervisor runs as root ({supervisor_user:?})")); + } + Ok(()) +} + /// Run a CLI step that must succeed. async fn run_ok( runner: &OpenShellRunner, @@ -431,34 +486,35 @@ async fn exec( .await } -/// Images built for one scenario, removed by `run` after sandbox cleanup. -struct TestImages { - engine: Vec, - tags: Vec, +/// The gateway's container engine. Images it builds for a scenario are +/// removed by `run` after sandbox cleanup. +struct Engine { + command: Vec, + images: Vec, } -impl TestImages { +impl Engine { fn from_env() -> Self { - let engine: Vec = std::env::var(ENGINE_ENV) + let command: Vec = std::env::var(ENGINE_ENV) .unwrap_or_else(|_| panic!("{ENGINE_ENV} must name the gateway's container engine")) .split_whitespace() .map(str::to_string) .collect(); - assert!(!engine.is_empty(), "{ENGINE_ENV} is empty"); + assert!(!command.is_empty(), "{ENGINE_ENV} is empty"); Self { - engine, - tags: Vec::new(), + command, + images: Vec::new(), } } /// Build `containerfile` and return the image tag. - fn build(&mut self, name: &str, containerfile: &str) -> Result { + fn build_image(&mut self, name: &str, containerfile: &str) -> Result { let context = tempfile::tempdir().map_err(|error| format!("create context: {error}"))?; let file = context.path().join("Containerfile"); std::fs::write(&file, containerfile) .map_err(|error| format!("write Containerfile: {error}"))?; let tag = format!("localhost/openshell-test-oci-{name}:{}", std::process::id()); - self.engine_command(&[ + self.output(&[ "build", "--file", file.to_str().ok_or("Containerfile path is not UTF-8")?, @@ -467,23 +523,47 @@ impl TestImages { context.path().to_str().ok_or("context path is not UTF-8")?, ])?; // A failed build leaves no tag to remove. - self.tags.push(tag.clone()); + self.images.push(tag.clone()); Ok(tag) } /// Remove every built image, returning one error per failed removal. - fn remove_all(&self) -> Vec { - self.tags + fn remove_images(&self) -> Vec { + self.images .iter() - .filter_map(|tag| self.engine_command(&["image", "rm", tag]).err()) + .filter_map(|tag| self.output(&["image", "rm", tag]).err()) .map(|error| format!("image cleanup failed: {error}")) .collect() } - fn engine_command(&self, args: &[&str]) -> Result<(), String> { - let command = format!("{} {}", self.engine.join(" "), args.join(" ")); - let output = Command::new(&self.engine[0]) - .args(&self.engine[1..]) + /// The one container with isolation `role` for `sandbox`. Docker and + /// Podman label sandbox containers the same way. + fn container_for_role(&self, sandbox: &str, role: &str) -> Result { + let ids = self.output(&[ + "ps", + "-aq", + "--filter", + &format!("label=openshell.ai/sandbox-name={sandbox}"), + "--filter", + &format!("label=openshell.ai/isolation-role={role}"), + ])?; + match ids.split_whitespace().collect::>().as_slice() { + [id] => Ok((*id).to_string()), + ids => Err(format!( + "expected one {role} container for {sandbox}, found {ids:?}" + )), + } + } + + fn inspect_container(&self, id: &str, format: &str) -> Result { + self.output(&["container", "inspect", "--format", format, id]) + } + + /// Run the engine and return its trimmed standard output. + fn output(&self, args: &[&str]) -> Result { + let command = format!("{} {}", self.command.join(" "), args.join(" ")); + let output = Command::new(&self.command[0]) + .args(&self.command[1..]) .args(args) .output() .map_err(|error| format!("failed to run {command}: {error}"))?; @@ -495,6 +575,6 @@ impl TestImages { String::from_utf8_lossy(&output.stderr) )); } - Ok(()) + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) } } From 2bb7f665e90dd1e56ef4649e0f429bfd71c83931 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Mon, 12 Oct 2026 14:49:10 -0700 Subject: [PATCH 15/16] test(oci): check the workspace rejection reason from sandbox JSON Read the WorkspaceValidationFailed condition reason from sandbox get --output json instead of matching CLI output, and restore the Docker driver's original workspace failure helpers. Signed-off-by: Matthew Grossman --- crates/openshell-driver-docker/src/lib.rs | 159 +++++++++--------- crates/openshell-driver-docker/src/tests.rs | 66 +++----- tests/suites/features/Cargo.lock | 1 + tests/suites/features/oci-image/Cargo.toml | 1 + .../features/oci-image/tests/oci_image.rs | 27 ++- 5 files changed, 129 insertions(+), 125 deletions(-) diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index d4726d19fb..5cfa80007a 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -1843,7 +1843,7 @@ impl DockerComputeDriver { .await { Ok(control) => control, - Err(failure) => { + Err(status) => { if self .lifecycle_event_fences .stop_requested(&sandbox.id, &sandbox.name) @@ -1865,8 +1865,8 @@ impl DockerComputeDriver { .await; cleanup_docker_boundary_state(sandbox, &self.config); return Err(DockerProvisioningFailure::new( - failure.reason_or("ControlSupervisorStartFailed"), - failure.status.message(), + supervisor_start_failure_reason(&status, "ControlSupervisorStartFailed"), + status.message(), )); } }; @@ -2154,17 +2154,17 @@ impl DockerComputeDriver { .await { Ok(process) => process, - Err(failure) => { + Err(status) => { handle_docker_runtime_failure( failure_context, - failure.reason_or("ControlSupervisorExited"), + supervisor_start_failure_reason(&status, "ControlSupervisorExited"), format!( "failed to start Docker control supervisor: {}", - failure.status.message() + status.message() ), ) .await; - return Err(failure.status); + return Err(status); } }; self.replace_control_process(&sandbox.id, process).await; @@ -5045,7 +5045,7 @@ async fn spawn_docker_control_process( sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig, failure_context: DockerRuntimeFailureContext, -) -> Result { +) -> Result { let directory = docker_boundary_state_dir(sandbox, config)?; let main_process_spec = tokio::fs::read_to_string(directory.join(MAIN_PROCESS_SPEC_FILE)) .await @@ -5229,7 +5229,9 @@ async fn spawn_docker_control_process( Some(RemoveContainerOptionsBuilder::default().force(true).build()), ) .await; - return Err(Status::internal(format!("start Docker supervisor container: {error}")).into()); + return Err(Status::internal(format!( + "start Docker supervisor container: {error}" + ))); } let sandbox_id = sandbox.id.clone(); let (shutdown, mut shutdown_requested) = oneshot::channel(); @@ -5286,9 +5288,8 @@ async fn spawn_docker_control_process( let log_tail = docker_container_log_tail(&monitored_docker, &monitored_supervisor_id).await; if reason == CONDITION_WORKSPACE_VALIDATION_FAILED { - // The condition message stays fixed; supervisor output may - // contain secrets. warn!(%sandbox_id, supervisor_logs = %log_tail, "Docker sandbox rejected the image working directory"); + message = WORKSPACE_VALIDATION_FAILED_MESSAGE.to_string(); } else if !log_tail.is_empty() { write!(message, "; log tail: {log_tail}").ok(); } @@ -5333,26 +5334,17 @@ async fn wait_for_docker_supervisor_ready( workload_container_id: &str, sandbox_id: &str, failures: &Arc>>, -) -> Result<(), ControlStartFailure> { - let Err(failure) = - poll_docker_supervisor_ready(docker, supervisor_id, workload_container_id, sandbox_id) - .await - else { - return Ok(()); - }; - // The monitor records a workspace rejection before it stops the workload - // and removes the supervisor, so readiness may only observe the cleanup. - // Other recorded failures keep the readiness status: their messages carry - // an untruncated log tail that must not travel in a gRPC status. - let recorded = failures - .lock() +) -> Result<(), Status> { + // The monitor records a workspace rejection before it removes the + // supervisor, so a later failed inspection must still report it. + match poll_docker_supervisor_ready(docker, supervisor_id, workload_container_id, sandbox_id) .await - .get(sandbox_id) - .map(|failure| failure.reason); - if recorded == Some(CONDITION_WORKSPACE_VALIDATION_FAILED) { - return Err(ControlStartFailure::workspace_validation()); + { + Ok(()) => Ok(()), + Err(status) => Err(recorded_workspace_validation_failure(failures, sandbox_id) + .await + .unwrap_or(status)), } - Err(failure) } async fn poll_docker_supervisor_ready( @@ -5360,7 +5352,7 @@ async fn poll_docker_supervisor_ready( supervisor_id: &str, workload_container_id: &str, sandbox_id: &str, -) -> Result<(), ControlStartFailure> { +) -> Result<(), Status> { // Gateway provisioning deadlines own startup expiration, including policy // repairs that extend the window. Unhealthy means not ready while the // supervisor is quarantined; only a stopped process is a startup failure. @@ -5374,8 +5366,7 @@ async fn poll_docker_supervisor_ready( if sandbox.state.unwrap_or_default().running == Some(false) { return Err(Status::unavailable( "Docker sandbox exited before supervisor became ready", - ) - .into()); + )); } let inspected = docker .inspect_container(supervisor_id, None) @@ -5386,91 +5377,91 @@ async fn poll_docker_supervisor_ready( let state = inspected.state.unwrap_or_default(); match state.health.and_then(|health| health.status) { Some(HealthStatusEnum::HEALTHY) => return Ok(()), + _ if state.running == Some(false) + && state.exit_code + == Some(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)) => + { + let log_tail = docker_container_log_tail(docker, supervisor_id).await; + warn!(sandbox_id, supervisor_id, supervisor_logs = %log_tail, "Docker sandbox rejected the image working directory"); + return Err(workspace_validation_status()); + } _ if state.running == Some(false) => { let log_tail = docker_container_log_tail(docker, supervisor_id).await; - if state.exit_code == Some(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)) { - warn!(sandbox_id, supervisor_id, supervisor_logs = %log_tail, "Docker sandbox rejected the image working directory"); - return Err(ControlStartFailure::workspace_validation()); - } warn!(sandbox_id, supervisor_id, supervisor_logs = %log_tail, "Docker supervisor exited before becoming ready"); return Err(Status::unavailable(format!( "Docker supervisor exited before becoming ready{}", format_log_tail(&log_tail) - )) - .into()); + ))); } _ => tokio::time::sleep(Duration::from_millis(100)).await, } } } -/// A control supervisor that failed to start. `reason` is set only when the -/// failure has a specific condition reason; callers choose their own default. -#[derive(Debug)] -struct ControlStartFailure { - reason: Option<&'static str>, - status: Status, -} - -impl From for ControlStartFailure { - fn from(status: Status) -> Self { - Self { - reason: None, - status, - } - } -} - -impl ControlStartFailure { - /// The sandbox rejected the image working directory. The message is fixed - /// text, because supervisor output may contain secrets. - fn workspace_validation() -> Self { - Self { - reason: Some(CONDITION_WORKSPACE_VALIDATION_FAILED), - status: Status::failed_precondition(WORKSPACE_VALIDATION_FAILED_MESSAGE), - } - } - - fn reason_or(&self, default: &'static str) -> &'static str { - self.reason.unwrap_or(default) - } -} - -/// Condition reason and message for a supervisor that exited unexpectedly. -/// /// Bollard reports nonzero Docker wait statuses as errors, not successful /// responses. Both forms must preserve the sandbox's workspace failure reason. fn docker_supervisor_wait_failure( result: Option>, ) -> (&'static str, String) { - let (exit_code, message) = match result { + match result { Some(Ok(status)) => ( - Some(status.status_code), + supervisor_exit_reason(status.status_code), format!( "Docker supervisor container exited with status {}", status.status_code ), ), Some(Err(BollardError::DockerContainerWaitError { code, error })) => ( - Some(code), + supervisor_exit_reason(code), format!("Docker supervisor container exited with status {code}: {error}"), ), Some(Err(error)) => ( - None, + "ControlSupervisorExited", format!("failed to wait for Docker supervisor container: {error}"), ), None => ( - None, + "ControlSupervisorExited", "Docker supervisor wait stream ended unexpectedly".to_string(), ), - }; - if exit_code == Some(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)) { - ( - CONDITION_WORKSPACE_VALIDATION_FAILED, - WORKSPACE_VALIDATION_FAILED_MESSAGE.to_string(), - ) + } +} + +async fn recorded_workspace_validation_failure( + failures: &Arc>>, + sandbox_id: &str, +) -> Option { + let failures = failures.lock().await; + failures + .get(sandbox_id) + .filter(|failure| failure.reason == CONDITION_WORKSPACE_VALIDATION_FAILED) + .map(|_| workspace_validation_status()) +} + +/// Startup failure for a supervisor that exited because the sandbox rejected +/// the image working directory. The message is fixed text, because supervisor +/// output may contain secrets, and identifies the failure for +/// [`supervisor_start_failure_reason`]. +fn workspace_validation_status() -> Status { + Status::failed_precondition(WORKSPACE_VALIDATION_FAILED_MESSAGE) +} + +/// Condition reason for a supervisor that failed before becoming ready. +fn supervisor_start_failure_reason(status: &Status, default: &'static str) -> &'static str { + if status.code() == tonic::Code::FailedPrecondition + && status.message() == WORKSPACE_VALIDATION_FAILED_MESSAGE + { + CONDITION_WORKSPACE_VALIDATION_FAILED + } else { + default + } +} + +/// Condition reason for a supervisor that exited with `status_code`. +fn supervisor_exit_reason(status_code: i64) -> &'static str { + if status_code == i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED) { + CONDITION_WORKSPACE_VALIDATION_FAILED } else { - ("ControlSupervisorExited", message) + "ControlSupervisorExited" } } diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index 06abffde68..64a0b0da49 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -3740,25 +3740,32 @@ fn docker_oom_kill_stays_terminal_despite_137() { #[test] fn supervisor_workspace_validation_exit_is_reported_explicitly() { - let workspace_exit = i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED); - for result in [ - Ok(ContainerWaitResponse { - status_code: workspace_exit, - error: None, - }), - Err(BollardError::DockerContainerWaitError { - code: workspace_exit, + let status = workspace_validation_status(); + assert_eq!(status.message(), WORKSPACE_VALIDATION_FAILED_MESSAGE); + assert_eq!( + supervisor_start_failure_reason(&status, "ControlSupervisorStartFailed"), + CONDITION_WORKSPACE_VALIDATION_FAILED + ); + assert_eq!( + supervisor_start_failure_reason( + &Status::failed_precondition("provider SPIFFE socket has no parent directory"), + "ControlSupervisorStartFailed" + ), + "ControlSupervisorStartFailed" + ); + assert_eq!( + supervisor_exit_reason(i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED)), + CONDITION_WORKSPACE_VALIDATION_FAILED + ); + assert_eq!(supervisor_exit_reason(1), "ControlSupervisorExited"); + + let (reason, message) = + docker_supervisor_wait_failure(Some(Err(BollardError::DockerContainerWaitError { + code: i64::from(SUPERVISOR_EXIT_WORKSPACE_VALIDATION_FAILED), error: "image workspace validation failed".into(), - }), - ] { - assert_eq!( - docker_supervisor_wait_failure(Some(result)), - ( - CONDITION_WORKSPACE_VALIDATION_FAILED, - WORKSPACE_VALIDATION_FAILED_MESSAGE.to_string() - ) - ); - } + }))); + assert_eq!(reason, CONDITION_WORKSPACE_VALIDATION_FAILED); + assert!(message.contains("status 78")); assert_eq!( docker_supervisor_wait_failure(Some(Err(BollardError::DockerContainerWaitError { code: 1, @@ -3767,23 +3774,6 @@ fn supervisor_workspace_validation_exit_is_reported_explicitly() { .0, "ControlSupervisorExited" ); - - let failure = ControlStartFailure::workspace_validation(); - assert_eq!( - failure.reason_or("ControlSupervisorStartFailed"), - CONDITION_WORKSPACE_VALIDATION_FAILED - ); - assert_eq!( - failure.status.message(), - WORKSPACE_VALIDATION_FAILED_MESSAGE - ); - assert_eq!( - ControlStartFailure::from(Status::failed_precondition( - WORKSPACE_VALIDATION_FAILED_MESSAGE - )) - .reason_or("ControlSupervisorStartFailed"), - "ControlSupervisorStartFailed" - ); } #[tokio::test] @@ -3792,13 +3782,13 @@ async fn workspace_failure_survives_supervisor_cleanup_before_readiness_inspecti "sandbox-1".to_string(), DockerRuntimeFailure { reason: CONDITION_WORKSPACE_VALIDATION_FAILED, - message: WORKSPACE_VALIDATION_FAILED_MESSAGE.into(), + message: "Docker supervisor container exited with status 78".into(), }, )]))); // There is no Docker daemon at this address: a monitor that has removed // the supervisor must not need another inspection to retain the reason. let docker = Docker::connect_with_http("127.0.0.1:1", 1, bollard::API_DEFAULT_VERSION).unwrap(); - let failure = wait_for_docker_supervisor_ready( + let status = wait_for_docker_supervisor_ready( &docker, "removed-supervisor", "workload-container-1", @@ -3808,7 +3798,7 @@ async fn workspace_failure_survives_supervisor_cleanup_before_readiness_inspecti .await .expect_err("the recorded workspace failure takes precedence over inspection"); assert_eq!( - failure.reason_or("ControlSupervisorStartFailed"), + supervisor_start_failure_reason(&status, "ControlSupervisorStartFailed"), CONDITION_WORKSPACE_VALIDATION_FAILED ); } diff --git a/tests/suites/features/Cargo.lock b/tests/suites/features/Cargo.lock index 127aff83b6..b92ccd05f5 100644 --- a/tests/suites/features/Cargo.lock +++ b/tests/suites/features/Cargo.lock @@ -923,6 +923,7 @@ name = "openshell-test-feature-oci-image" version = "0.0.0" dependencies = [ "openshell-conformance", + "serde_json", "tempfile", "tokio", ] diff --git a/tests/suites/features/oci-image/Cargo.toml b/tests/suites/features/oci-image/Cargo.toml index 1107587ca7..9ec58feccd 100644 --- a/tests/suites/features/oci-image/Cargo.toml +++ b/tests/suites/features/oci-image/Cargo.toml @@ -8,5 +8,6 @@ edition = "2024" [dependencies] openshell-conformance = { path = "../../../../crates/openshell-conformance" } +serde_json = "1" tempfile = "3" tokio = { version = "1.43", features = ["macros", "rt"] } diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index 59c975ba7e..ede1df1838 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -187,9 +187,30 @@ USER app create.failure_diagnostic("sandbox creation fails before the command runs") ); } - if !printed(WORKSPACE_VALIDATION_FAILED) { - return Err(create.failure_diagnostic(&format!( - "sandbox creation fails with {WORKSPACE_VALIDATION_FAILED}" + // Require the condition reason, so another provisioning failure + // cannot pass as a workspace rejection. + let sandbox = runner + .step("uw/get") + .description("rejected sandbox reports the workspace validation reason") + .with_timeout(COMMAND_TIMEOUT) + .run(&["sandbox", "get", &name, "--output", "json"]) + .await + .map_err(|error| error.to_string())?; + sandbox.require_success()?; + let reasons: Vec = sandbox + .json::() + .map_err(|error| error.to_string())?["conditions"] + .as_array() + .into_iter() + .flatten() + .filter_map(|condition| condition["reason"].as_str().map(str::to_string)) + .collect(); + if !reasons + .iter() + .any(|reason| reason == WORKSPACE_VALIDATION_FAILED) + { + return Err(sandbox.failure_diagnostic(&format!( + "sandbox condition reason is {WORKSPACE_VALIDATION_FAILED}, got {reasons:?}" ))); } } From fde1cc52672e3643181197851187fda4d5ea552d Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 13 Oct 2026 13:45:37 +0200 Subject: [PATCH 16/16] test(oci): capture fixture image identity before sandbox creation Signed-off-by: Evan Lezar --- .../features/oci-image/tests/oci_image.rs | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/tests/suites/features/oci-image/tests/oci_image.rs b/tests/suites/features/oci-image/tests/oci_image.rs index ede1df1838..aac57e5c68 100644 --- a/tests/suites/features/oci-image/tests/oci_image.rs +++ b/tests/suites/features/oci-image/tests/oci_image.rs @@ -162,7 +162,7 @@ USER app "--name", &name, "--from", - &image, + &image.tag, "--no-tty", "--", "sh", @@ -285,7 +285,7 @@ fn workspace_checks(identity: &str, workspace: &str, image_file: bool) -> String async fn create_sandbox( runner: &mut OpenShellRunner, short: &str, - image: &str, + image: &TestImage, policy: Option<&str>, checks: &str, ) -> Result { @@ -294,7 +294,7 @@ async fn create_sandbox( "(set -eu; {checks} touch main-write) >/tmp/oci-main.log 2>&1; \ echo $? >/tmp/oci-main.status; exec sleep infinity" ); - let mut args = vec!["sandbox", "create", "--name", &name, "--from", image]; + let mut args = vec!["sandbox", "create", "--name", &name, "--from", &image.tag]; if let Some(policy) = policy { args.extend(["--policy", policy]); } @@ -417,14 +417,14 @@ async fn file_transfer_uses_workspace( } /// Check the containers the driver created for `sandbox`. The workload runs -/// the image ID inspected before creation, so retagging cannot swap it, and +/// the image ID captured after the fixture build, before sandbox creation, and /// starts directly as the sandbox `identity` without a network or the /// supervisor's credentials. The supervisor runs in a separate non-root /// container. fn container_checks( engine: &Engine, sandbox: &str, - image: &str, + image: &TestImage, identity: &str, ) -> Result<(), String> { let workload = engine.container_for_role(sandbox, "sandbox")?; @@ -432,12 +432,12 @@ fn container_checks( if workload == supervisor { return Err(format!("{sandbox} has no separate supervisor container")); } - let image_id = engine.output(&["image", "inspect", "--format", "{{.Id}}", image])?; let launched = engine.inspect_container(&workload, "{{.Image}}")?; let digest = |id: &str| id.trim_start_matches("sha256:").to_string(); - if digest(&launched) != digest(&image_id) { + if digest(&launched) != digest(&image.id) { return Err(format!( - "workload launched image {launched}, expected the inspected {image_id}" + "workload launched image {launched}, expected the built image {}", + image.id )); } let user = engine.inspect_container(&workload, "{{.Config.User}}")?; @@ -507,6 +507,12 @@ async fn exec( .await } +/// A built fixture's mutable tag and image ID captured before sandbox creation. +struct TestImage { + tag: String, + id: String, +} + /// The gateway's container engine. Images it builds for a scenario are /// removed by `run` after sandbox cleanup. struct Engine { @@ -528,8 +534,8 @@ impl Engine { } } - /// Build `containerfile` and return the image tag. - fn build_image(&mut self, name: &str, containerfile: &str) -> Result { + /// Build `containerfile` and capture its image ID before sandbox creation. + fn build_image(&mut self, name: &str, containerfile: &str) -> Result { let context = tempfile::tempdir().map_err(|error| format!("create context: {error}"))?; let file = context.path().join("Containerfile"); std::fs::write(&file, containerfile) @@ -545,7 +551,8 @@ impl Engine { ])?; // A failed build leaves no tag to remove. self.images.push(tag.clone()); - Ok(tag) + let id = self.output(&["image", "inspect", "--format", "{{.Id}}", &tag])?; + Ok(TestImage { tag, id }) } /// Remove every built image, returning one error per failed removal.