[zoroyihan7]

What

A generic per-workspace sandbox environment Secret, named sandbox-env.

• sandbox.env_secret (chart value, .env key sandbox_env_secret) accepts sandbox-env or empty. Empty (the default) renders the sandbox template exactly as before. Any other value makes the chart (and upgrade.sh / install.sh) refuse.
• When set, the sandbox template's main container gets envFrom: [{secretRef: {name: sandbox-env, optional: true}}]. Every key of that Secret in the workspace namespace becomes an environment variable of every sandbox in that workspace.
  • By reference: values stay in the Secret. They are not in the Workload, its env, or the Pod spec.
  • Per workspace: a Pod resolves Secrets only in its own namespace, which is the workspace's.
  • Optional: a workspace without the Secret starts its sandboxes unchanged.
  • Precedence: explicit env entries (the template's own, or a Workload's env) keep their value; the Secret only adds variables.
  • Adding a variable = adding a key to the Secret. Keys must be valid environment variable names; the kubelet skips others with an InvalidEnvironmentVariableNames event. An API PATCH with params replaces every key of a general Secret (unchanged API contract), so through the API send the full set (GET, add the key, PATCH all); the docs show a kubectl patch --type=json that adds one key and keeps the others.
• Reserved name, admin-only: every key reaches every sandbox in the workspace, and the default role lets any workspace member create a Secret bound to their workspace. The apiserver refuses create / update / delete of a Secret named sandbox-env unless the caller is a system admin or an admin of every workspace it is (or will be) bound to (403). The same check covers binding an image Secret of that name through the workspace image-secret API (which takes image Secrets only, so a general sandbox-env gets 400 there before any role check), is repeated on each conflict retry of an update, delete or binding, and treats a missing user as no admin. Once the Secret carries a workspace selector (primus-safe.workspace.selector, or a non-empty selector.applied record), only a system admin may change, delete or rebind it, because the selector reaches workspaces the ids list does not name. No 403 for sandbox-env names its workspaces: neither the guard's nor the ordinary role check's on get, patch and delete, which is replaced for that name only (the workspaces are logged server-side). The role check's message for other names is unchanged. The name is reserved whether or not the feature is on and needs no apiserver configuration, so turning the feature on opens no window in which an apiserver pod would let a member take it. That is why the name is fixed: a site-chosen name could only be guarded after the apiserver restarted with it, while the sandbox template may already read it.
• Mirror: resource-manager never takes over a sandbox-env already in a workspace namespace without the mirror label, even for an explicit primus-safe.workspace.ids grant. It leaves it as it is, so it is neither overwritten nor deleted when the grant is withdrawn, and makes the conflict visible: the conflicting workspaces in the new primus-safe.workspace.mirror.conflicts annotation on the source Secret (written for any Secret, removed when there are none), and a MirrorConflict Warning event raised once for each workspace that enters it, not on every reconcile. A conflict is not a reconcile error for any name (the Secret in the namespace winning is a supported state); a sandbox-env with one is checked again every 10 minutes (RequeueAfter), so its copy is made once the namespace's own Secret is removed, which changes nothing the controller watches. Other names are not rechecked on a timer, as before. Copies the mirror made itself are synced and removed as before. Other Secret names keep today's ids-grant adoption.
• Delete: the delete of the reserved Secret is pinned to the checked resourceVersion; a lost precondition is retried on the re-read, re-checked Secret; a Secret deleted concurrently during that retry is success; a persistent conflict returns a message saying so. Every other name is deleted exactly as on main (one read, no precondition, no retry).
• Image-secret binding changes the workspace ids on a copy, kept only once the patch succeeded, and on a conflict re-reads the Secret for every name, so the retry writes onto the stored bindings and keeps one added meanwhile. sandbox-env is also patched with an optimistic lock and re-authorized on each retry; other names are patched without a version, as before. A binding that keeps conflicting is a 409 (Primus.00012) that says to try again, as the reserved delete is.
• Docs: docs/apis/secret.md#sandbox-environment-secret (operator how-to: the two ways to provide the Secret, kubectl in the workspace namespace or the API mirror, must not be mixed in one workspace; which one wins if they are), docs/installation/upgrade.md (on a site running an older version, turn it on in an upgrade after the one that installs this version), docs/installation/install.md (a fresh install can turn it on: sandbox_env_secret=sandbox-env ./install.sh).
• Also: authSecretUpdate returns the user it resolved, so the patch handler resolves it once. The patch handler's failure log no longer dereferences a Secret the conflict path may have cleared. .github/workflows/block-sensitive-secrets.yml gains a line-level exemption, used only for the Password: password, line of resources/secret.go; the rest of that file is still scanned. install.sh, which renders its own values and never read either sandbox setting, takes sandbox_env_secret and sandbox_extra_ca_secret from its environment, checks the first before installing anything, renders both as upgrade.sh does, and writes both to the .env it generates so the next upgrade.sh keeps them.

Tests

• common/pkg/config: the default renders no envFrom. sandbox-env renders envFrom with the name, optional: true, and the explicit env is identical to the unset render. Every other value (other names, 0, false, invalid or over-long names) makes the chart refuse. The apiserver Deployment and ConfigMap render identically with and without the feature. helm template with defaults is identical to the merge base.
• job-manager/pkg/dispatcher: dispatching through the chart's own template keeps envFrom on the main container, in the workspace namespace. A Workload-set variable stays an explicit env entry. A sentinel value stored in the workspace Secret appears nowhere in the marshalled Workload or dispatched object. The default template references no Secret.
• resource-manager/pkg/resource: a sandbox-env conflict in ws-a does not fail the reconcile, asks for RequeueAfter 10m, records ["ws-a"] in the conflicts annotation, raises one MirrorConflict, and still serves the other workspaces; reconciling again raises nothing; a new conflict in ws-c raises one event, for ws-c only; once both foreign Secrets are removed the copies are made, the annotation is removed and no requeue is asked. A selector conflict on another name is recorded, raised once over two reconciles, and not rechecked. A sandbox-env made in ws-a is not adopted when an API Secret of that name is bound to ws-a (data and labels unchanged, MirrorConflict raised), and survives the unbind. Reverse proof: the same sequence with another name still adopts and then deletes. The mirror's own sandbox-env copy is still synced and removed on unbind.
• apiserver/pkg/handlers/resources: uses the roles the chart installs, not hand-written ones.
  • A member can create, update and delete Secrets of any other name; the guard (asserted by its own message) refuses their create, update and delete of sandbox-env, even when they own it, with no configuration set, and nothing is written.
  • A workspace admin may write it for their own workspace and is refused when it is (or would become) bound to a workspace they do not administer. A system admin may write it. A nil user is refused, including through image-secret binding.
  • A patch or delete that loses a conflict is re-checked against the re-read Secret: a concurrent rebind to a workspace the caller does not administer turns the retry into a 403. A delete that keeps conflicting returns the explained 409. A patch whose Secret vanished during a conflict returns 404. A reserved delete whose retry finds the Secret gone returns nil; one missing on the first read is still 404.
  • A non-reserved delete that conflicts makes one read and one delete with no precondition and returns the conflict as it came; NotFound on delete is ignored (as on main).
  • Image-secret binding of sandbox-env: the patch carries the read resourceVersion; on a conflict with a concurrent rebind to a workspace the caller does not administer the retry is refused and not written; with one they do administer, the retry carries the new resourceVersion and keeps the concurrent binding. Another image Secret's patch carries no resourceVersion.
  • Any image Secret whose first binding patch returns 409 (after a concurrent bind to ws-c) is written again from the re-read Secret: through PATCH /workspaces/ws-b, 200, two patches, stored ids ["ws-a","ws-c","ws-b"] (red on the previous head: one patch, stored ["ws-a","ws-c"], still 200). A patch that fails leaves the caller's Secret unbound. A binding that keeps conflicting returns, for either name, a 409 Primus.00012 that says to try again, also through the HTTP error mapping.
  • A workspace admin of every id-bound workspace is refused patch, delete and image binding of a sandbox-env with a selector, a blank selector, or a selector.applied record; a system admin is not; without a selector, or with an empty record, the admin is let through.
  • The guard's 403, through the API and directly, contains no workspace id; nor does the role check's 403 to a non-member on get, patch and delete of sandbox-env. Reverse proof: for another name the role check's 403 still names them.
  • An API PATCH with only NEW_VAR leaves only NEW_VAR (the documented replace semantics).
• Mutations, each turning the named tests red: ignoring the selector in the guard; dropping the optimistic lock on binding; returning NotFound on a concurrent delete during retry; routing other names through the reserved delete retry (the previous head's behaviour: 5 reads, "kept changing"); putting the workspace list back in the guard's 403; not redacting the role check's 403; re-reading only sandbox-env after a binding conflict (loses the concurrent binding); changing the caller's Secret instead of a copy; not recording conflicts; allowing the mirror to adopt sandbox-env; keying the guard on an (empty) config value; deleting without retry; dropping the nil-user check (panic); logging secret.Name after a vanished Secret (panic).
• Secret scan: replayed locally on the real secret.go (passes), on a copy with an extra Password: literal (caught; the old whole-file exemption let it pass), and with no exemption (the existing line is flagged); replayed again from the workflow file on this head.
• install.sh: with stub kubectl/helm, other, SANDBOX-ENV and sandbox-env2 exit 1 before any kubectl call; its two seds applied to the chart's values.yaml render envFrom and the extra-CA volume with helm template; no new shellcheck warning.

Known limitations (deliberately not built)

• The sandbox env Secret should not be broadcast with the workspace selector: it is a credential, and anyone who can edit a workspace's scopes can make a workspace match. The docs say so. The API cannot set the selector (it takes no annotations); a selector set with kubectl makes the Secret system-admin-only through the API.
• The SaFE API response to a bind does not report a later mirror conflict; it is visible on the source Secret's annotation, the Warning event and the resource-manager log.
• Anyone with direct Kubernetes RBAC write on Secrets in a workspace namespace (or in primus-safe) can still write the Secret. This PR adds no webhook or RBAC change; only the SaFE API is guarded.
• A Secret named sandbox-env written by a non-admin before this version is installed is still mirrored, and its keys would reach the sandboxes of its workspaces once the feature is on. The docs give the commands that show who made each one; check before enabling. A code-level guard (an approval annotation the apiserver stamps on admitted writes and the mirror requires) is proposed as a follow-up.
• Workspaces whose sandboxes are placed on external/virtual nodes need that node to resolve a Secret envFrom. If it cannot, the Pod may be refused even with optional: true. Verify per site before setting the value there; this is why the default stays empty.
• Running sandboxes keep the environment they started with. A rotated value reaches sandboxes created afterwards.

Not verified

No deployment. Not yet run on a live site: arbitrary key injection, user-override precedence, the cross-workspace boundary, rotation, the 403 through the real API path, the mirror refusal against a real data plane, and the external-node behaviour. All rely on Kubernetes envFrom/namespace semantics and the unit tests above.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 91.08280% with 14 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...resource-manager/pkg/resource/secret_controller.go	82.22%	4 Missing and 4 partials ⚠️
SaFE/apiserver/pkg/handlers/resources/workspace.go	86.66%	2 Missing and 2 partials ⚠️
SaFE/apiserver/pkg/handlers/resources/secret.go	97.56%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[zoroyihan7]

Second review round addressed in 81ea08d and d019016:

1. Docs now say an API PATCH of params replaces every key (send the full set) and show a kubectl patch --type=json that adds one key; the general API contract is unchanged.
2. A sandbox-env carrying a workspace selector (or a non-empty selector.applied record) can be changed, deleted or rebound only by a system admin (403 otherwise); tested with reverse proof.
3. Image-secret binding of sandbox-env patches with an optimistic lock and on conflict re-reads and re-authorizes; conflict paths (refused and kept-binding) tested, other names unchanged.
4. A MirrorConflict on sandbox-env now fails the reconcile (backoff retry), records the workspaces in primus-safe.workspace.mirror.conflicts (cleared when resolved), and keeps the Warning event; tested.
5. A sandbox-env deleted concurrently during a delete retry returns nil; first-read NotFound is still 404; tested.
6. The retry loop and the "kept changing" message apply to sandbox-env only; a non-reserved delete is byte-for-byte the main code path, tested (one read, one delete, no precondition, conflict returned as is).
7. No 403 for sandbox-env names its workspaces (guard and ordinary role check on get/patch/delete); they are logged server-side; tested with reverse proof.
8. authSecretUpdate returns the resolved user; the second getAndSetUsername is gone.

[zoroyihan7]

Third review round addressed in e90e17a, b413405 and 1821b01 (head 1821b018). The PR description is updated to match.

1 (blocking): an image binding that lost a conflict reported success. Fixed in bindSecretToWorkspace (e90e17a). The workspace ids are changed on a copy, which replaces the caller's Secret only once the patch succeeded. A conflict now re-reads the Secret for every name, so the retry writes onto the stored bindings and keeps one added meanwhile. sandbox-env still carries the optimistic lock and is re-authorized on each retry.

• TestImageBindingAfterAConflictIsWritten goes through PATCH /workspaces/ws-b (the HTTP handler). The first Secret patch returns 409 after a concurrent bind to ws-c. The test asserts 200, exactly two patches, a stored annotation of ["ws-a","ws-c","ws-b"], and ws-b listing the Secret. The clientset reads the same store the patch writes, so the re-read sees the real state. Before the fix it was red with exactly the reported symptom: one patch, ["ws-a","ws-c"] stored, still 200.
• TestImageBindingFailureLeavesTheSecretUnbound: a patch that fails leaves the caller's Secret as it was read. Red before the fix.
• Mutations: re-reading only sandbox-env again (with the copy kept) turns the first test red, because ws-c is lost. Changing the caller's Secret instead of a copy turns the second test red.

2: a sandbox-env mirror conflict was a reconcile error. Changed in b413405. A mirror conflict is no longer a reconcile error for any name. The warning log and the MirrorConflict event moved from syncSecretToWorkspace to recordMirrorConflicts. They are raised only for a workspace that primus-safe.workspace.mirror.conflicts does not list yet, the same pattern InvalidWorkspaceSelector uses. One addition beyond the suggestion: a sandbox-env with a conflict returns RequeueAfter: 10m and no error. Removing the namespace's own Secret changes nothing the controller watches, so without the requeue the copy would be made only at the Secret's next unrelated change. Other names are not rechecked on a timer, as before.

• TestSandboxEnvMirrorConflictIsRecordedOnceAndRechecked:
  • The first reconcile returns no error, RequeueAfter set, and one event.
  • Reconciling again raises nothing.
  • A new conflict in ws-c raises one event, for ws-c only.
  • Once the foreign Secrets are removed, the copies are made, the annotation is removed and no requeue is asked.
• TestOtherMirrorConflictIsRecordedButNotRechecked (one event over two reconciles, no requeue) and TestMirrorNeverAdoptsAHandMadeSandboxEnvSecret are updated. All three are red on the previous controller: it returned the error, and two reconciles raised two events.

3: 400 rather than 403 on the image-secret API. The type check stays as defense in depth, and docs/apis/secret.md is corrected (1821b01). The workspace image-secret API binds image Secrets only, so a general sandbox-env gets 400 there before any role check. Only an image Secret of that name reaches the admin check (403).

4: an exhausted retry on the reserved binding. It is now commonerrors.NewConflict ("the secret ... kept changing while it was being bound to workspace ...; try again"), for every name, the same as deleteSandboxEnvSecret. Measured before the change: the wrapped conflict did reach the client as 409, not 500, because convertToErrResponse unwraps the StatusError with errors.As. But it carried errorCode Conflict and the raw Kubernetes message.

• TestImageBindingConflictIsExplained covers sandbox-env and another name. It checks 5 attempts, IsConflict, the "try again" text, and, through apiutils.AbortWithApiError, HTTP 409 with Primus.00012.
• Before the change it was red: for sandbox-env on the code and message, for the other name on a nil error (item 1).

5: install.sh ignored both sandbox settings. Fixed in 1821b01. install.sh has no .env to read: it prompts, then writes .env at the end. So it now takes sandbox_env_secret and sandbox_extra_ca_secret from its environment (sandbox_env_secret=sandbox-env ./install.sh). It checks sandbox_env_secret before anything is installed, case-sensitively, ahead of shopt -s nocasematch. It applies the same seds as upgrade.sh and writes both settings to the generated .env, so the next upgrade.sh keeps them.

• Verified: applied to the chart's values.yaml, the two seds render envFrom and the extra-CA volume with helm template.
• With stub kubectl/helm, other, SANDBOX-ENV and sandbox-env2 exit 1 before any kubectl call.
• No new shellcheck warning.
• install.md, upgrade.md and the values.yaml comments are updated.

6: a sandbox-env written by a non-admin before this version. Not done in this PR; I propose it as a follow-up.

Why not here:

• It only concerns Secrets written before this version. From this version on, no non-admin write gets through the API.
• The feature is off by default, and turning it on is an explicit operator step.
• A code-level guard needs a trust marker shared by both components, and it changes what happens to existing copies on upgrade. That deserves its own review.

Proposed design:

• The apiserver stamps an annotation (for example primus-safe.secret.sandbox-env.approved) on every write that authorizeSandboxEnvSecret admits. The API takes no annotations, so only an admitted write or kubectl can set it.
• resource-manager mirrors sandbox-env only when it carries the marker. Without it, resource-manager removes the copies it made, records this on the Secret and raises a Warning once.
• resource-manager watches the marker, so approving the Secret (an admin write, or kubectl annotate) triggers a reconcile.

Trade-offs:

• A sandbox-env made with kubectl in primus-safe needs the annotation added by hand.
• On upgrade, copies of an unapproved sandbox-env are removed even with the feature off. The name meant nothing before, but a site that used it for something else would lose those copies.
• Secrets made by hand in workspace namespaces stay out of scope: writing them needs data-plane RBAC, which this PR already treats as admin.

Meanwhile, docs/apis/secret.md gives the commands that show who made an existing sandbox-env: its owner label and bindings in primus-safe, and kubectl get secrets -A --field-selector metadata.name=sandbox-env -L primus-safe/secret-mirror on each data plane. The values.yaml comment and the limitations section say the gap is open.

Local runs on 1821b018:

• go vet passes on the changed packages.
• These tests pass: apiserver/pkg/handlers/resources with -race, resource-manager/pkg/resource with -gcflags=all=-l, common/pkg/config, common/pkg/secret and job-manager/pkg/dispatcher.
• I replayed the secret-scan step from the workflow file against this head, and it passes. With one more unquoted Password: value in secret.go it is caught. With the line exemption removed, the existing line is flagged.