Repository navigation
feat(docker): scope managed resources by host UID - #5136
vineethsai7 wants to merge 2 commits into
Conversation
Signed-off-by: Vineeth Sai Narajala <vnarajal@cisco.com>
|
I have read the DCO document and I hereby sign the DCO. |
Signed-off-by: Vineeth Sai Narajala <vnarajal@cisco.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06d162c1e6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if docker_config.scope_by_host_uid { | ||
| reject_unscoped_docker_resources(&docker, &docker_config.sandbox_label).await?; | ||
| } |
There was a problem hiding this comment.
Guard the reverse migration out of UID scope
When an operator turns scope_by_host_uid off after creating scoped sandboxes, this condition skips the only migration guard. The runtime namespace then reverts to the bare sandbox_label, while list, delete, and reconciliation paths filter that namespace exactly (for example in list_managed_container_summaries), so the existing uid-<uid>-... containers, volumes, and private state become invisible to the gateway and may remain running or orphaned. Reject startup while the current UID-scoped namespace is nonempty, or provide an explicit reverse migration.
Useful? React with 👍 / 👎.
Summary
Add an opt-in Docker resource scope for per-user gateways on a shared Linux host. Each gateway uses its effective host UID with the existing Docker sandbox namespace, so its startup reconciliation and sandbox operations select its own resources.
This is a draft design and implementation slice. Packaged gateways still need automatic port selection and registration before the full multi-user experience works without manual configuration. The issue is still
state:triage-needed; design feedback is welcome.Related Issue
Part of #4273
Changes
[openshell.drivers.docker].scope_by_host_uid, defaulting tofalse.uid-<effective UID>-<sandbox_label>.Design
Proposed complete path
Keep the per-user gateway model now used by the Linux packages and Snap. Each OS user retains a separate gateway database, TLS material, and client registration. The gateway's loopback port must differ for each concurrent user; loopback alone is not the authentication boundary, so each client must continue to present that gateway's own credentials.
For packaged defaults, the next slice should bind the loopback listener to port
0before constructing the compute driver, pass the assigned nonzero port to Docker and Podman supervisors, and publish the ready endpoint in a private per-user runtime file. Package startup and CLI registration should read that endpoint and refresh the user's gateway record after restarts. Publish only after the listener and gateway are ready, and reject stale records by checking the authenticated endpoint. This avoids a fixed port collision and the collision risk of a UID-to-port hash. The Docker driver's current fixed-port validation must move after the listener binds.For Docker, the existing
sandbox_labelnamespace already controls resource creation, list and watch, startup cleanup, private volume names, and host state. This slice derives a different namespace for each host UID at that point. It uses the compute driver's existing selection path, where Docker resource ownership belongs. There is no new gateway authorization layer or event schema. The namespace is a driver scope, not a privilege boundary against direct Docker API access. A later installation identifier could distinguish two gateways under one UID and handle UID reuse if that becomes a supported case.Alternatives considered
sandbox_labeland port to each user works today but is fragile. This option derives the Docker scope from the process UID, leaving only the port and registration gap.User-facing example
For a fresh per-user Docker gateway, an operator can set:
From that user's account:
For UID
1000, the Docker workload, supervisor, and private volume label isopenshell.ai/sandbox-namespace=uid-1000-default; UID1001selectsuid-1001-default. No new OCSF event is emitted by this option.Compatibility
The option is off by default, so existing gateways retain their labels, volume names, and state paths. Enable it before creating sandboxes. If unscoped managed containers or volumes remain, startup fails with an actionable error; delete the old sandboxes through their original gateway first. Keep each scoped gateway under a stable host UID. Gateways under the same UID still share a Docker scope. Anyone who can use the Docker daemon directly can inspect or alter all users' containers regardless of OpenShell's gateway separation.
Release Notes
Docker gateways can opt in to host-UID-scoped sandbox resources with
scope_by_host_uid = true. Packaged multi-user setup still requires separate ports and registrations.Testing
cargo fmt -p openshell-driver-docker -- --check,cargo clippy --quiet --locked -p openshell-driver-docker --all-targets -- -D warnings,uv run tasks/scripts/check_docs_nav.py, Ferncheck, and Markdown lint. All ran throughdcbwithnice -n 15.cargo test --quiet --locked -p openshell-driver-dockerpassed (154 passed, 2 ignored). Tests cover distinct UID scopes and a mock Docker API with legacy containers or volumes.Fern reported zero errors and three warnings that are also present with the base configuration page: an MDX parse warning, skipped redirect check without authentication, and theme contrast.
Follow-ups
Checklist
Review follow-up
Reviewed upstream PR #5136 against NVIDIA/OpenShell
mainon 2026-11-02. Full findings and scenarios are in REVIEW-5136.md.06d162c1e. Scoped Docker workload, staging, and supervisor names now include a stable namespace digest. Unscoped names retain their previous format. The identical-sandbox-ID regression failed before and passed after the fix.06d162c1e. Pending deletion now verifies Docker ownership labels and removes the inspected container ID. The foreign-container regression failed before and passed after the fix; a separate test checks ID-based removal.06d162c1e. The migration mock now verifies the managed-by and unscoped-namespace Docker filters.06d162c1e. Migration docs and the troubleshooting skill now instruct operators to stop the old gateway after deleting old sandboxes and before enabling UID scoping.GitHub has no inline comments or submitted reviews for this PR. The two issue-level comments are a workflow-vetting bot notice and the author's DCO affirmation; neither requires a code change.
Follow-up checks on
06d162c1e, all throughdcbwithnice -n 15:cargo fmt -p openshell-driver-docker -- --check;cargo clippy --quiet --locked -p openshell-driver-docker --all-targets -- -D warnings;cargo test --quiet --locked -p openshell-driver-docker(157 passed, 2 ignored);uv run tasks/scripts/check_docs_nav.py; Ferncheck(0 errors, 3 existing warnings); andmarkdownlint-cli2@0.22.0(0 errors). A Docker E2E run was not available on the build box because the Docker CLI is absent, and no test host was used.Manual verification
PR: NVIDIA/OpenShell #5136 (feat/4273-v1), head
06d162c1e642.Before: upstream main
be5e49499(openshell 0.1.7-dev.40+gbe5e49499), two per-user gateways on one Docker host with the default sandbox label. After: PR head withscope_by_host_uid = true.Environment
A(uid 1056) andB(uid 1057), both members of thedockergroup. Each runs its own gateway (own SQLite database, own JWT keys, own loopback port, plaintext local-development config), Docker compute driver, and the samesandbox_label(the default,default). No host OpenShell service was involved.cargo build --release), supervisor and sandbox runtime packaged into local images and set assupervisor_image/sandbox_runtime_image. Gateway and CLI were run on RHEL 9 through an Ubuntu 24.04 glibc loader.Steps
Before (main)
A starts a gateway and a sandbox
a-sbx. Then B starts a gateway (same Docker host, same label) and createsb-sbx.Starting B's gateway killed A's running sandbox. B's gateway log has no explicit line about it; the kills coincide with B's gateway start-up, which looks at every OpenShell container under the shared
defaultlabel (A's sandbox is unknown to B's database). Nobody asked B to toucha-sbx.After (PR head, scope_by_host_uid = true)
default-label containers and six private volumes left from the run above):The gateway refuses to start, as documented, instead of hiding or adopting the legacy resources.
scope_by_host_uid = true:A's sandbox stayed
Readythrough B's gateway start, B's sandbox creation and B's gateway restart. Each user's containers carry their ownuid-<uid>-defaultnamespace and namespace-prefixed container names.Result
PASS for the documented behaviour: scoped gateways do not see or touch each other's sandboxes (including across a gateway restart), and scoped startup refuses unscoped legacy resources with the documented message. Before: FAIL on main (one user's gateway start killed the other user's running sandbox).
Limits seen (documented by the PR, not tested as bugs): the scope is a driver-level label, not a privilege boundary against direct Docker API access (both users are in the
dockergroup and could use the daemon directly).sandbox deleteof a name the gateway does not know answers "already deleted" rather than "not found".