Skip to content

feat(helm): expose sandbox_uid and sandbox_gid as chart values - #3021

Open
natedemoss wants to merge 1 commit into
NVIDIA:mainfrom
natedemoss:feat/helm-sandbox-uid-gid
Open

feat(helm): expose sandbox_uid and sandbox_gid as chart values#3021
natedemoss wants to merge 1 commit into
NVIDIA:mainfrom
natedemoss:feat/helm-sandbox-uid-gid

Conversation

@natedemoss

@natedemoss natedemoss commented Aug 29, 2026

Copy link
Copy Markdown

Summary

The Kubernetes driver has accepted sandbox_uid / sandbox_gid as first-class config for a while, but the chart's gateway ConfigMap never rendered them, so a Helm-installed gateway had no way to reach them. Today the only lever is the openshift.io/sa.scc.uid-range namespace annotation, which is awkward on non-OpenShift clusters and lives on a resource the chart doesn't own. This adds server.sandboxUid / server.sandboxGid and renders them into [openshell.drivers.kubernetes].

Related Issue

Closes #2697

Changes

  • templates/gateway-config.yaml — render sandbox_uid / sandbox_gid when set, using the same {{- if .Values.server.X }} idiom as the sibling optional fields immediately above (app_armor_profile, default_runtime_class_name, workspace_storage_class, etc.).
  • values.yaml — both default to "", so nothing renders unless you opt in. Existing installs get a byte-identical ConfigMap.
  • Both are emitted unquoted. The driver deserializes them as Option<u32>, so a quoted scalar would fail at gateway startup rather than at render time. That's what the trailing $ in the test regexes is guarding.
  • Three unittest cases: absent by default, both rendered under the right table, and UID-set-with-GID-unset. The last one matters because resolve_sandbox_gid() already falls back to the configured UID, so an empty sandbox_gid must not leak into the config.
  • Docs — the UID resolution section of sandbox-compute-drivers.mdx only mentioned the SCC annotation, so I pointed it at the chart values too. gateway-config.mdx already documents the underlying TOML keys, so I left that page alone.

No Rust changed; the driver side already works.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

I ran the checks that this change can actually affect, on Windows:

  • helm:lint — defaults plus all 16 ci/values-*.yaml variants, 17/17 clean.
  • helm:docs:check — README regenerated with helm-docs and in sync.
  • helm:test — 112 tests / 10 suites pass.
  • fern check — 0 errors (the 2 warnings are pre-existing: the unauthenticated redirect check and a theme contrast ratio).
  • markdownlint-cli2 — 0 issues across all 132 files.

I did not run the full mise run pre-commit, so I've left that box unchecked rather than claim it: the Rust half of lint doesn't build on Windows (openssh is unix-only), and this PR touches no Rust, Python, or proto. I also can't run the Kubernetes e2e lane locally. Since both values default to "" and render nothing, I verified with helm template that a default install produces zero sandbox_uid / sandbox_gid lines, and that setting them yields sandbox_uid = 1500 / sandbox_gid = 1600 as bare integers.

As a sanity check that the new tests actually bite, I stashed just the template change and re-ran the suite — the two rendering assertions fail without it.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable) — n/a, no architectural change

The Kubernetes compute driver already accepts sandbox_uid and sandbox_gid
as first-class config, but the chart's gateway ConfigMap never rendered
them. On a Helm-installed gateway the only lever for pinning a sandbox UID
was the openshift.io/sa.scc.uid-range namespace annotation, which is
OpenShift-branded and lives on a resource the chart does not own.

Render both keys into [openshell.drivers.kubernetes] using the same
{{- if .Values.server.X }} idiom as the sibling optional fields. Both
values default to "" and render nothing, so existing installs are
byte-identical to before.

Both are emitted unquoted because the driver parses them as TOML integers;
a quoted scalar would be rejected at gateway startup rather than at render
time.

Closes NVIDIA#2697

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kcgm4MbvdCsDRPWbiZ4mDu
Signed-off-by: natedemoss <ndemoss28@gmail.com>
Copilot AI lite review requested due to automatic review settings August 29, 2026 15:16
@copy-pr-bot

copy-pr-bot Bot commented Aug 29, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(helm): expose sandbox_uid / sandbox_gid as first-class Helm values

2 participants