Add read-only dataset mounts to Docker runtimes - #2578
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR introduces opt-in Docker host dataset mounts and substantial filesystem/artifact validation across runtime startup, collection, and restoration rather than a small isolated configuration change. An unresolved high-severity finding also flags a race that could allow mounted data to be archived, so the data-boundary behavior needs human review. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
|
|
||
| existence: list[str] = [] | ||
| for service, sources in batches: | ||
| await validate_runtime_mounts(runtime.service(service), sources) |
There was a problem hiding this comment.
🟠 High utils/artifacts.py:195
collect can archive mounted data despite validate_runtime_mounts passing: a concurrent rollout can replace an artifact-path parent with a symlink before _tar_out runs, causing tar to follow the relocated path and export the mount. Make validation and archiving race-safe, for example by collecting through safe directory descriptors.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/utils/artifacts.py around line 195:
`collect` can archive mounted data despite `validate_runtime_mounts` passing: a concurrent rollout can replace an artifact-path parent with a symlink before `_tar_out` runs, causing `tar` to follow the relocated path and export the mount. Make validation and archiving race-safe, for example by collecting through safe directory descriptors.
Adds shared host datasets to Docker rollouts through
DockerConfig.mounts, so each rollout can access the same data without copying it into the image or artifact bundle.The mapping pairs absolute container paths with typed
BindMountsources for existing host files or directories. Mounts attach before task setup and default to recursively read-only access; writable access is an explicit opt-in. Recursive read-only enforcement requires Linux 5.12 or newer on the Docker daemon.Artifact collection and restoration reject overlapping paths, symlink traversal, and relocated mounts. Sources are never created automatically, and runtime teardown removes containers without removing host data.
This implementation supports standalone Docker runtimes. Harbor Compose rejects this mount configuration; storage adapters for Prime Sandbox, Modal, and S3 remain outside this PR.
Stacks on #2549 in the existing #2528 → #2547 → #2549 container-runtime stack.
Note
Medium Risk
Changes container filesystem layout and tightens artifact restore/collect with new failures on mount overlap, symlinks, and Harbor Compose; misconfigured mounts or overlapping artifact paths will error where they might have been ignored before.
Overview
Adds Docker bind mounts so rollouts can read shared host datasets without baking them into the image or artifact tarballs.
Introduces
BindMount(absolute hostsource,read_onlydefaulting totrue) andDockerConfig.mounts, mapping container paths to bind sources with validation for safe absolute targets and non-overlapping paths.DockerRuntimeattaches these via--mountbefore task setup, using recursive read-only bind options when configured. Harbor Compose explicitly rejects anyDockerConfigwith mounts.Artifact collect, restore, and
resolve_runtime_confignow run mount safety checks: artifact roots must not overlap mount targets, paths must not traverse symlinks, and mount targets must still appear in/proc/self/mountinfoso relocated parents cannot bypass overlap rules.Reviewed by Cursor Bugbot for commit 53f9cab. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add read-only bind mounts to
DockerRuntimewith path validationBindMountconfig model with absolute host source path andread_onlydefault oftrue; exports it fromverifiers.v1andverifiers.v1.runtimesDockerConfigaccepts amountsmapping of container targets toBindMountvalues, canonicalizes target keys, and rejects invalid, overlapping, or duplicate targetsDockerRuntimestartup generates Docker mount arguments for configured mounts; read-only mounts also get recursive read-only and private-propagation optionsvalidate_runtime_mountsandvalidate_artifact_mountsin artifacts.py to reject symlink traversal, verify mount targets via mountinfo, and block artifact roots that overlap configured mounts before restorationHarborComposeRuntimeconstruction now raisesValueErrorwhen aDockerConfigdeclares bind mountsvalidate_runtime_mountsreads/proc/<pid>/root/mountinfoinside the runtime; if that path is unreadable or the runtime lacks procfs, mount validation will fail and block artifact restoration📊 Macroscope summarized 53f9cab. 2 files reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues