feat(deployments): OpenShell sandboxed deployment backend (AIRCORE-872) - #662
Conversation
|
5b25cc0 to
3c9f550
Compare
04b9b31 to
0f88d5f
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OpenShell sandbox deployment support, including runtime image packaging, policy generation, gateway-backed deployment lifecycle operations, local configuration, discovery, runbooks, examples, and targeted CI coverage. ChangesOpenShell sandbox deployment
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
plugins/nemo-deployments/src/nemo_deployments_plugin/skills/deploy-sandbox/SKILL.md (1)
80-83: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the
generate-certsimage instead of:latest.This skill is agent-executed (
allowed-tools: [Bash, ...]); an unpinned mutable tag onghcr.io/nvidia/openshell/gateway:latestis a rug-pull vector if the tag is ever repointed to a malicious image.🔒 Pin the image
- docker run --rm --user 0 -v /var/lib/openshell:/var/lib/openshell \ - ghcr.io/nvidia/openshell/gateway:latest generate-certs \ + docker run --rm --user 0 -v /var/lib/openshell:/var/lib/openshell \ + ghcr.io/nvidia/openshell/gateway@sha256:<digest> generate-certs \Based on the SkillSpector hint: "Docker image references without a specific tag (:latest is implicit) or digest ... can be silently replaced by a malicious image."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-deployments/src/nemo_deployments_plugin/skills/deploy-sandbox/SKILL.md` around lines 80 - 83, Update the docker run command for generate-certs to replace the mutable ghcr.io/nvidia/openshell/gateway:latest reference with a specific immutable image tag or digest. Keep the existing generate-certs arguments and volume configuration unchanged.Source: Linters/SAST tools
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/config.py (1)
53-113: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider
extra="forbid"andLiteralfor the enum-ish fields.A typo in a config key (e.g.
platform_egres:) is silently ignored today and the sandbox gets the default egress instead of the intended one — a silent security-relevant divergence. Same forprotocol/access/tls, which document fixed value sets but accept anything;landlock_compatibilityalready usesLiteral.♻️ Proposed change
class OpenShellExecutorConfig(BaseModel): """Knobs for a named openshell executor instance.""" + model_config = ConfigDict(extra="forbid") + gateway_endpoint: str = Field(class PlatformEgressConfig(BaseModel): - protocol: str = Field(default="rest", description="OpenShell endpoint protocol: rest | websocket | sql.") + protocol: Literal["rest", "websocket", "sql"] = Field(default="rest", description="OpenShell endpoint protocol.") ... - access: str = Field(default="full", description="OpenShell access level: read-only | read-write | full.") + access: Literal["read-only", "read-write", "full"] = Field(default="full", description="OpenShell access level.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/config.py` around lines 53 - 113, Update the relevant Pydantic configuration models, including OpenShellExecutorConfig and the models defining protocol/access/tls fields, to reject unknown configuration keys with extra="forbid". Replace enum-like string annotations for protocol, access, and tls with Literal-constrained values matching their documented supported options, while preserving the existing landlock_compatibility Literal behavior.plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py (1)
462-486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
touchalways succeeds, so a serve command that dies instantly is still marked launched.
setsid <serve> ... & touch markerreturns the exit status oftouch, not the workload; onlycd/shell errors surface here. The docstring's "catches launch-time failures" overstates it. Consider marking after a short liveness check (e.g.sleep 1; kill -0 $!) or reword the docstring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py` around lines 462 - 486, The _launch_serve implementation marks the launch successful based on touch alone, so immediately failing serve processes are not detected. Add a short post-launch liveness check using the background process PID before creating _LAUNCH_MARKER, ensuring the marker is written only when the process remains alive; update the _launch_serve docstring to describe this behavior accurately.plugins/nemo-deployments/examples/openshell/DEMO.ipynb (1)
452-457: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale step reference.
MODELis set in the "Pick a model" cell, not Step 1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-deployments/examples/openshell/DEMO.ipynb` around lines 452 - 457, The troubleshooting text in the notebook incorrectly refers to Step 1 for the MODEL configuration. Update that reference to identify the “Pick a model” cell, leaving the surrounding troubleshooting guidance unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-agents/src/nemo_agents_plugin/cli.py`:
- Around line 349-357: Prevent sandbox compatibility from being silently
bypassed: in plugins/nemo-agents/src/nemo_agents_plugin/cli.py lines 349-357,
reject --sandbox-runtime combined with --dockerfile before packaging; in
plugins/nemo-agents/src/nemo_agents_plugin/container/builder.py line 102, add
the same guard for direct callers using sandbox_runtime and dockerfile; in
plugins/nemo-agents/src/nemo_agents_plugin/container/template.py lines 329-349,
reject sandbox_runtime with external templates unless the template explicitly
enforces the required sandbox fragments.
In `@plugins/nemo-deployments/examples/openshell/DEMO.ipynb`:
- Line 355: Update the Step 3 description in the demo documentation to match the
zero-egress configuration: remove claims that a platform_egress or mandatory
nemo_platform egress rule is generated, and state that the policy contains no
network rules when platform_egress is null. Preserve the filesystem permissions
and run_as_user details.
In `@plugins/nemo-deployments/examples/openshell/docker-compose.yml`:
- Around line 41-63: The docker-compose gateway configuration exposes a root
Docker-socket service on all host interfaces without an explicit local-only
warning. Update the comments near user and ports to clearly state that this
setup is for local development only and must not run on shared or untrusted
networks, or change the port binding to the Docker bridge host IP while
preserving sandbox bridge reachability.
In `@plugins/nemo-deployments/examples/openshell/local-sandbox-policy.yaml`:
- Around line 46-49: Update the binaries section to document that the pinned
/opt/uv/python/cpython-3.13.7-linux-aarch64-gnu/bin/python3.13 entry is
host-specific and must be replaced for each architecture and Python version;
alternatively, verify policy.py supports globbing before using a portable
pattern.
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py`:
- Around line 440-460: Update the serve-status flow around _serve_launched so
grpc.RpcError from its marker probe is caught and converted into the same
appropriate UNKNOWN/STARTING BackendStatusUpdate used for transient RPC
failures, rather than escaping read_status. Preserve the existing launch,
port-exposure, and READY behavior when the probe succeeds.
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/sandbox_profile.py`:
- Around line 21-23: Update the OpenShell sandbox profile in sandbox_profile.py
to explicitly require glibc >= 2.39 rather than only documenting it. Ensure the
profile rendering includes a build-time preflight check that validates the
selected base image’s glibc version and fails image creation when it is below
2.39, including for arbitrary base-image overrides.
---
Nitpick comments:
In `@plugins/nemo-deployments/examples/openshell/DEMO.ipynb`:
- Around line 452-457: The troubleshooting text in the notebook incorrectly
refers to Step 1 for the MODEL configuration. Update that reference to identify
the “Pick a model” cell, leaving the surrounding troubleshooting guidance
unchanged.
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py`:
- Around line 462-486: The _launch_serve implementation marks the launch
successful based on touch alone, so immediately failing serve processes are not
detected. Add a short post-launch liveness check using the background process
PID before creating _LAUNCH_MARKER, ensuring the marker is written only when the
process remains alive; update the _launch_serve docstring to describe this
behavior accurately.
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/config.py`:
- Around line 53-113: Update the relevant Pydantic configuration models,
including OpenShellExecutorConfig and the models defining protocol/access/tls
fields, to reject unknown configuration keys with extra="forbid". Replace
enum-like string annotations for protocol, access, and tls with
Literal-constrained values matching their documented supported options, while
preserving the existing landlock_compatibility Literal behavior.
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/skills/deploy-sandbox/SKILL.md`:
- Around line 80-83: Update the docker run command for generate-certs to replace
the mutable ghcr.io/nvidia/openshell/gateway:latest reference with a specific
immutable image tag or digest. Keep the existing generate-certs arguments and
volume configuration unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1f9a5d1f-9916-4e3b-a259-2c8199c1f4f3
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/src/nemo_platform/skills/nemo-build-agent/SKILL.mdis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.github/actions/changes/action.yaml.github/workflows/ci.yamlMakefilepackages/nemo_platform_ext/src/nemo_platform_ext/skills/nemo-build-agent/SKILL.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/discovery.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/sandbox.pypackages/nmp_platform/config/local.yamlplugins/nemo-agents/src/nemo_agents_plugin/cli.pyplugins/nemo-agents/src/nemo_agents_plugin/container/builder.pyplugins/nemo-agents/src/nemo_agents_plugin/container/sandbox.pyplugins/nemo-agents/src/nemo_agents_plugin/container/template.pyplugins/nemo-agents/tests/unit/test_container.pyplugins/nemo-deployments/examples/openshell/DEMO.ipynbplugins/nemo-deployments/examples/openshell/agent/config.yamlplugins/nemo-deployments/examples/openshell/docker-compose.ymlplugins/nemo-deployments/examples/openshell/gateway.tomlplugins/nemo-deployments/examples/openshell/local-sandbox-policy.yamlplugins/nemo-deployments/pyproject.tomlplugins/nemo-deployments/src/nemo_deployments_plugin/backends/base.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/config.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/policy.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/sandbox_profile.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/registry.pyplugins/nemo-deployments/src/nemo_deployments_plugin/skills.pyplugins/nemo-deployments/src/nemo_deployments_plugin/skills/deploy-sandbox/SKILL.mdplugins/nemo-deployments/src/nemo_deployments_plugin/skills/deploy-sandbox/tests.jsonplugins/nemo-deployments/tests/integration/backends/openshell/test_openshell_backend.pyplugins/nemo-deployments/tests/unit/backends/openshell/conftest.pyplugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_executor_config.pyplugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_policy.pyplugins/nemo-deployments/tests/unit/backends/openshell/test_sandbox_profile.pyplugins/nemo-deployments/tests/unit/test_registry.py
|
Thanks @coderabbitai. Triaged everything against the code and Addressing now (pushed to this branch):
Acknowledged, deferred to follow-up work (not this PR):
No redesign required. Thanks for the review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
backend=$(fd -a '^backend\.py$' plugins/nemo-deployments/src | rg '/openshell/')
config=$(fd -a '^config\.py$' plugins/nemo-deployments/src | rg '/openshell/')
tests=$(fd -a -t f . plugins/nemo-deployments/tests/unit/backends/openshell | tr '\n' ' ')
printf '%s\n' '--- backend outline ---'
ast-grep outline "$backend" --items all --type method,function --match 'read_status|_provision|_launch|_marker' --view expanded
printf '%s\n' '--- config outline ---'
ast-grep outline "$config" --items all --type class,method --match 'OpenShell|PlatformEgress|_validate' --view expanded
printf '%s\n' '--- implementation evidence ---'
rg -n -C 8 'RpcError|_provision|read_status|ConfigDict|extra\s*=|Literal\[|protocol:|tls:' \
"$backend" "$config"
printf '%s\n' '--- relevant tests ---'
rg -n -C 4 'RpcError|UNKNOWN|extra|platform_egres|graphql|passthrough|protocol|tls' $testsLength of output: 90125
Would you like follow-up issues for the deferred valid gaps? ✏️ Learnings added
|
|
A few more things from my agent: Findings
The 57 targeted OpenShell unit tests pass, and current CI is green; the live gateway path is skipped without a running gateway. I did not repeat the already-open CodeRabbit findings or post comments to GitHub. |
tylersbray
left a comment
tylersbray
left a comment
There was a problem hiding this comment.
Nonblocking approval, modulo conflicts, happy CI, stripping ticket numbers out, etc etc. Looks good.
Add the `openshell` Python SDK as the nemo-deployments plugin's optional `[openshell]` extra, alongside grpcio/protobuf. The SDK is on PyPI and resolves like any other dependency; no vendored gRPC stubs and no local wheel pin. It is kept out of the dev dependency-group because the wheel is platform-restricted (manylinux_2_39 / macOS 13 arm64, no sdist) and a dev-group entry would break `uv sync --all-packages` on older hosts. Pin as `openshell>=0.0.83` and let `uv.lock` hold the exact resolved version so the lockfile carries the pin without churning pyproject on every SDK bump. AIRCORE-872 Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Build the OpenShell SandboxPolicy for a deployment: a default-deny filesystem/process allowlist plus a mandatory, always-injected platform-egress rule so the sandbox's only outbound path is back to the Inference Gateway. A static-YAML policy override is supported for callers that need to hand-tune the allowlist, validated against the policy proto. AIRCORE-872 Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Implement OpenShellDeploymentBackend against the DeploymentBackend ABC (create / read_status / delete / expose) over the OpenShell gRPC gateway, and register it in the backend registry. The backend applies the generated SandboxPolicy and serve workdir when it stands a deployment up, drives provisioning from read_status, resolves secrets, and only exposes ports once a live serve pid is present, so an agent deployed through the standard deployments API runs inside a policy-governed sandbox. Covered by mocked unit tests, an executor-config test, and a live-gateway integration test. AIRCORE-872 Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Add a provider-neutral `--sandbox-runtime` image profile to `nemo agents package` so an agent can be packaged for a sandbox runtime (openshell) without hand-editing the container build. The nemo-deployments plugin contributes the `openshell` profile via the `nemo.sandbox_profiles` entry point; the agents container builder/template consume the selected profile. build-agent skill docs updated to mention the flag. AIRCORE-872 Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Register an `openshell-local` executor in the local platform config so the OpenShell backend is reachable end-to-end through the deployments API on a developer machine. AIRCORE-872 Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
21ff025 to
a013991
Compare
Add the `deploy-sandbox` skill (with its test manifest) that walks the create -> package -> deploy -> invoke flow for a policy-governed OpenShell sandbox, and a self-contained getting-started notebook under examples/openshell. The demo routes the agent's model traffic through gateway-managed `inference.local`, so egress governance holds with zero sandbox egress rules and no docker-specific host/proxy hacks. A dedicated CI job installs the `[openshell]` extra and covers the backend. AIRCORE-872 Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
a013991 to
d99e6e0
Compare
5 participants
Summary
Adds an OpenShell deployment backend to the
nemo-deploymentsplugin, so an agent deployed through the standard deployments API runs inside an OpenShell sandbox. The backend generates a Landlock filesystem/process policy and a default-deny egress rule that only permits traffic back to the platform Inference Gateway, so the agent's model calls go through the platform and other egress is blocked at the sandbox boundary.The create/package/deploy/invoke flow works end to end. The demo routes model traffic through gateway-managed
inference.local, which drops the docker-specific host and proxy workarounds the earlier version needed. Getting-started notebook:plugins/nemo-deployments/examples/openshell/DEMO.ipynb.What's here (AIRCORE-872)
Twenty commits, ordered so each builds on the last. The first six:
openshellPyPI package as the plugin's optional[openshell]extra and dev group. No vendored gRPC stubs and no wheel pin. Pinned>=0.0.92, with the exact version inuv.lock. The floor matters: the gateway started enforcing a 19-char routable-name limit in v0.0.88, so an older client contract is not compatible with a current gateway.OpenShellDeploymentBackendimplements theDeploymentBackendABC (create, read_status, delete, expose) over the OpenShell gRPC gateway and applies the policy and serve workdir. Covered by mocked unit tests, an executor-config test, and a live-gateway integration test.--sandbox-runtime openshellprofile fornemo agents package, registered through thenemo.sandbox_profilesentry point.openshell-localexecutor tolocal.yamlfor the local end-to-end path.deploy-sandboxskill plus the getting-started notebook.Review round (28 Jul)
Addressing @matthewgrossman's findings:
_sandbox_nameemitted 20 chars against a 19-char limit, so every deployment failed withINVALID_ARGUMENT. Verified live: a 0.0.92 gateway rejects the old name and accepts the new 18-char one. Port names are clamped to the same limit, sinceExposeServicecaps service names identically.get_logsreturned supervisor logs, not the agent's.GetSandboxLogsis fed by supervisor tracing events, never by the workload's redirected stdout, so the serve log is tailed overExecSandboxinstead.json_format.ParseDictagainstSandboxPolicyrather than rebuilt field by field, so an unknown key raises instead of vanishing, and every field OpenShell supports is accepted rather than the subset this backend writes. The two free-form strings the proto cannot type (landlock.compatibility,enforcement) are checked separately, because the supervisor reads anything unrecognised as the weaker setting.created.endpointsafter create went asynchronous; it now pollsread_statusto READY.Status
In review.
openshellis a normal PyPI dependency (>=0.0.92, resolved to0.0.92in the lock), so CI can resolve it anduv syncpicks it up like any other dependency. The example compose pins the gateway image to the matching tag instead of:latest, overridable throughIMAGE_TAG, so client and gateway cannot drift apart. Local checks pass: openshell unit tests (69), ruff check and format, ty. The live integration test passes against a 0.0.92 gateway.Linear: AIRCORE-872
Summary by CodeRabbit
openshell-localexecutor for sandboxed local deployments.--sandbox-runtimetonemo agents package, enabling sandbox-runtime-aware Dockerfile generation.deploy-sandboxrunbook with governed sandbox guidance and troubleshooting notes.make test-deployments-openshelltarget.