docs(osep): add OSEP-0020 for explicit subPath directory initialization - #1573
docs(osep): add OSEP-0020 for explicit subPath directory initialization#1573gegemeimingzi wants to merge 4 commits into
Conversation
Adds OSEP-0020 proposing a Kubernetes-only opt-in (pvc.createSubPathIfMissing) that guarantees the directory referenced by Volume.subPath exists on a PVC before the main sandbox container starts, implemented via an init container. Includes API surface, validation rules, error semantics, idempotency guarantees, test plan, and upgrade strategy. Refs opensandbox-group#1437
…uirements Expand OSEP-0020 to explicitly address the remaining maintainer-requested concerns from issue opensandbox-group#1437: creator identity (UID/GID/mode/umask/ACL) for newly created directories, retry and stable-error semantics for init container failures, and the volumes/poolRef incompatibility constraint.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3ac70f0cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - **Server version gating**: servers that implement the new schema field | ||
| accept the opt-in; older servers reject it via `additionalProperties: false` | ||
| schema validation (the field will not exist in their published schema), so | ||
| the guarantee can never be silently lost. |
There was a problem hiding this comment.
Older servers do not reject this field as claimed: FastAPI validates requests using server/opensandbox_server/api/schema.py::PVC, whose Pydantic model does not set extra="forbid", so unknown properties are ignored by default; the OpenAPI additionalProperties: false declaration is not runtime enforcement. A new client calling any such server can therefore have createSubPathIfMissing silently discarded and proceed without the promised directory guarantee. The protocol needs explicit capability/version negotiation or another client-visible rejection mechanism rather than relying on the published schema.
AGENTS.md reference: AGENTS.md:L43-L48
Useful? React with 👍 / 👎.
| 3. Exit 0 on success; exit non-zero on failure, which fails the pod and | ||
| surfaces as a sandbox create/run failure with the init container's log | ||
| attached to the error. |
There was a problem hiding this comment.
When mkdir fails, for example because the PVC denies writes, the existing KubernetesSandboxService._wait_for_sandbox_ready recognizes only Running/Allocated and unschedulable states; it neither detects failed init containers nor fetches their logs, so this path eventually returns the generic K8S_POD_READY_TIMEOUT rather than the stable error and attached log promised here. The design and test plan need an explicit init-container failure detection/diagnostics change, not only the proposed schema and volume-helper work.
AGENTS.md reference: AGENTS.md:L44-L49
Useful? React with 👍 / 👎.
| 1. Mount the PVC at a scratch mount path (e.g. `/mnt/subpath-init`). | ||
| 2. Run `mkdir -p <subPath>` with the resolved relative path. |
There was a problem hiding this comment.
The existing ensure_valid_sub_path rejects only absolute paths and .. components, so contract-valid names beginning with - reach this command; for example, subPath: -m is interpreted by mkdir as an option and fails instead of creating that directory. If the value is interpolated into a shell command, metacharacters introduce a still larger command-injection risk. Require an argv-safe invocation with an option terminator, equivalent to mkdir -p -- "$SUBPATH", and cover option-like and shell-special path names in the test plan.
AGENTS.md reference: AGENTS.md:L43-L49
Useful? React with 👍 / 👎.
github-actions
Bot
added
documentation
…etection, argv safety P1: The runtime request models do not set extra='forbid', so an unknown field on an older server is silently ignored — replace the incorrect 'additionalProperties: false' rejection claim with explicit capability negotiation (server capability flag + SDK guard + optional strict model). P2: The existing _wait_for_sandbox_ready does not detect failed init containers; specify extending the wait path to inspect pod.status.initContainerStatuses and surface SUBPATH_INIT_FAILED with log tail, with an e2e test for the failure path. P2: Invoke mkdir -p -- with an argv array (no shell) so option-like and shell-special subPath names are treated as paths; add unit/e2e coverage.
|
✅ All three Codex review points addressed in ea902f3: P1 — Real version gating: Corrected. The runtime request models in server/opensandbox_server/api/schema.py do not set extra='forbid', so the OpenAPI additionalProperties: false declaration is not runtime enforcement. Replaced the incorrect schema-rejection claim with an explicit capability-negotiation design: a server capability flag (extending the stable diagnostics API), an optional extra='forbid' hardening on the PVC model, and an SDK-side guard that refuses to serialize the opt-in against servers that do not advertise support. Updated the Upgrade & Migration Strategy accordingly. P2 — Init failure detection: Corrected. The existing KubernetesSandboxService._wait_for_sandbox_ready does not inspect pod.status.initContainerStatuses, so a failed mkdir would surface as the generic K8S_POD_READY_TIMEOUT. The OSEP now requires extending the wait path to detect init-container termination and surface SUBPATH_INIT_FAILED with the log tail, plus an e2e test for the failure path. P2 — argv-safe command: Corrected. The design now specifies invoking mkdir -p -- with the subPath as a single argv argument and the '--' option terminator, never through a shell, with unit and e2e coverage for option-like (-m, --help) and shell-special names. |
1 participant
Summary
Adds OSEP-0020: Explicit SubPath Directory Initialization for PVC Volumes — a proposal for issue #1437.
Today, callers must pre-create a directory referenced by Volume.subPath out of band (e.g. with a temporary provisioner sandbox) because the Kubernetes pvc backend maps subPath to �olumeMounts.subPath, which does not create missing directories. OSEP-0003 documents a creation guarantee that no provider actually implements. This OSEP proposes a Kubernetes-only opt-in (pvc.createSubPathIfMissing) that guarantees the directory exists before the main sandbox container starts, using an init container with mkdir -p.
Scope of this PR
This is a design proposal only — per the project's OSEP process (see oseps/CONTRIBUTING.md), changes to the core sandbox API (specs/sandbox-lifecycle.yml) require an approved OSEP before implementation. No runtime code changes are included.
Contents
Maintainer questions from #1437
The proposal answers the three questions raised in the issue:
Testing
Breaking Changes
Checklist
Volume.subPathon thepvcbackend #1437)