pal-e-docs

Re-Review: Add PSA-restricted securityContext to westside-admin overlay (#137)

review-1118-2026-05-01 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review of refined issue body (Lucas approved Option A on 2026-05-01). The refinement resolves all three original blockers cleanly and the scope is now appropriately narrow. However, the verification matrix the caller asked me to run also surfaced a new blocker in the canonical YAML snippet under "Expected Behavior" — the container name in the example does not match the base. A dev agent that copy-pastes the snippet verbatim will produce a broken kustomize merge. One-line fix; re-review will be near-instant.

Original Blocker Resolution (re-verification)

Each of the three blockers from review-1118-2026-04-30, plus the six caller-specified verification items:
  • [x] Original Blocker 1 (root crash on 9 sibling services) — RESOLVED. New "Expected Behavior" targets only overlays/westside-admin/prod/deployment-patch.yaml. Base untouched. Zero impact on the 9 root-running consumers. (Caller verification 1 PASS.)
  • [x] Original Blocker 2 (streamlit's existing securityContext insufficient) — RESOLVED via OUT-OF-SCOPE deferral. AC list no longer contains any westside-streamlit recovery condition. Streamlit spike explicitly called out as a separate ticket in the new "Scope Boundary (narrow)" section. (Caller verifications 2 + 5 PASS.)
  • [x] Original Blocker 3 (basketball-api initContainer) — RESOLVED via OUT-OF-SCOPE deferral. The system-wide PSA hardening initiative explicitly carries this forward as separate scope. (Caller verification 5 PASS.)
  • [x] Caller verification 3 — AC list includes CreateContainerConfigError zero-events check. Verified at AC #4: "Zero CreateContainerConfigError events on westside-admin pods (proves the Dockerfile actually runs as non-root, not just the manifest)." PASS.
  • [x] Caller verification 4 — Pre-merge Dockerfile USER verification note IS present, located in the "Expected Behavior" section: "Pre-merge verification of westside-admin's Dockerfile: confirm the image actually runs as non-root before merging the manifest…" PASS.
  • [x] Caller verification 6 — Misleading "fields are valid in any namespace" sentence has been removed from the body. Confirmed by full-text scan of refined body. PASS.
All six caller-specified verifications pass. The refinement work is solid.

Fresh Verifications (caller-specified)

  • [x] File path exists — overlays/westside-admin/prod/deployment-patch.yaml verified at that exact path on forgejo_admin/pal-e-deployments@main via curl /api/v1/repos/.../raw/.... Returns 200, valid YAML, 56 lines. Container name in current file: app.
  • [x] Dockerfile USER directive — verified forgejo_admin/westside-admin@main Dockerfile contains USER node (line 31). PR #10 is merged (state=closed, merged=true, title "docker: multi-stage Dockerfile for adapter-node runtime"). Hidden prerequisite is satisfied — no flag needed.
  • [ ] Kustomize merge correctness — FAILS. See "New Blocker" below. The base has a containers array with one container named app. Strategic merge keys on container name. The issue body's snippet uses - name: westside-admin, which will NOT merge with - name: app in the base — it will append a SECOND container, leaving the original one without securityContext.

New Blocker: Container Name Mismatch in Snippet

The snippet under "Expected Behavior" specifies:
Why this breaks:
  • Kustomize processes patches: in order. The overlays/westside-admin/prod/kustomization.yaml applies deployment-patch.yaml (strategic merge) FIRST, then JSON patches that rename containers/0/name from app → westside-admin.
  • The existing deployment-patch.yaml correctly uses - name: app at the strategic-merge stage. The kustomization comment even calls this out: "targets base name 'app' before rename."
  • If the dev agent copy-pastes the issue snippet verbatim, the patch will contain - name: westside-admin. Strategic merge keys on container name. With no container named westside-admin in the base, kustomize appends a second container. The pod manifest will then have:
  • Even if kustomize doesn't error, the resulting manifest is broken and PSA admission will reject the second container for being incomplete (no image), or the pod will hit InvalidImageName.
Reference pattern (correct): overlays/westside-streamlit/prod/deployment-patch.yaml — which DOES work in PSA admission terms (its failure is a separate kustomize-rename issue, deferred to the spike) — uses - name: app at the strategic-merge stage. Snippet from streamlit:

Secondary Finding: Snippet vs. Streamlit Canonical

The issue snippet's pod-level securityContext omits runAsUser: 1000, runAsGroup: 1000, and fsGroup: 1000. Streamlit (the only other working PSA-restricted-targeting overlay in this repo) sets all three. Two views:
  • Strict PSA-restricted requirement: only runAsNonRoot: true, seccompProfile.type: RuntimeDefault, allowPrivilegeEscalation: false, capabilities.drop: ["ALL"] are required. The snippet is sufficient for admission.
  • Project canonical pattern: matches streamlit's superset (adds the explicit UID/GID + fsGroup). Lower drift risk if other PSA-restricted overlays follow.
Not a blocker — the snippet IS valid PSA-restricted shape — but flagging because westside-admin's Dockerfile sets USER node (UID 1000 on node:alpine) and an explicit runAsUser: 1000 would document that contract in the manifest. Recommend matching streamlit's canonical superset for consistency, or explicitly annotate why this overlay diverges. Not strictly required for AC pass.

Template Completeness

  • [x] Type (Bug)
  • [x] Lineage (validation-12-2026-04-30, with refinement audit trail)
  • [x] Repo (forgejo_admin/pal-e-deployments)
  • [x] What Broke
  • [x] Scope Boundary (narrow) — new section, explicit OUT OF SCOPE list. Excellent addition.
  • [x] Repro Steps
  • [x] Expected Behavior (with snippet — see container-name issue)
  • [x] Environment
  • [x] Acceptance Criteria (5 ACs, all testable)
  • [x] Related

Traceability

  • [x] story:admin-row-crud label — story note story-westside-admin-admin-row-crud verified on project-westside-admin user-stories table (carried from prior review).
  • [x] arch:k8s-deploy label — arch note still MISSING: search_notes(query="arch-k8s-deploy") returns zero results. Per platform-wide precedent (review-933, review-935, review-972, review-283, review-1118-2026-04-30) treated as known waiver, not per-ticket blocker. Tracked debt.
  • [x] type:bug — correctly classified.
  • [x] blocks:deploy-chain — correct.
  • [x] Forgejo issue #137 — open, accessible, refined body confirmed.

File Targets

  • [x] overlays/westside-admin/prod/deployment-patch.yaml — verified exists at exact path. Currently has zero securityContext. Existing container name: app (matches base). Existing strategic merge correctly targets pre-rename container name.
  • [x] bases/standard/deployment.yaml — verified, container named app, no securityContext. Untouched by this fix per Option A.
  • [x] overlays/westside-admin/prod/kustomization.yaml — verified patch order: strategic-merge FIRST, JSON-patch container rename SECOND. Comment at line 9 confirms: "targets base name 'app' before rename."

Repo Placement

Correct — forgejo_admin/pal-e-deployments. Single repo, single file, single overlay. Refinement collapsed multi-service ambition into a clean single-overlay change.

Dependencies

Board context: #1117 (validation surface) is in qa. Two follow-up tickets explicitly named in the body (streamlit PSA spike + system-wide PSA hardening) — both intentionally out of scope for #1118 and to be filed separately. No upstream board blockers. Westside-admin Dockerfile (PR #10) is merged, satisfying the runtime-USER prerequisite.

Acceptance Criteria

5 ACs, all verifiable:
  • AC1 — manifest field check (visual inspection of merged YAML or kubectl get deploy -o yaml | yq).
  • AC2 — kubectl get deployment 1/1 ready after ArgoCD sync. Standard rollout check.
  • AC3 — zero FailedCreate from PSA admission via kubectl describe rs. Standard PSA failure-mode check.
  • AC4 — zero CreateContainerConfigError events. Catches the Dockerfile-runs-as-root failure mode. Smart, narrowly scoped check.
  • AC5 — no regression elsewhere. Trivially true (no base change). Could even be omitted.
AC list is appropriately scoped. One implicit AC missing: verify the kustomize build before merge — kustomize build overlays/westside-admin/prod/ should produce exactly ONE container in the output. This catches the container-name mismatch blocker before it reaches the cluster. Recommend adding as AC #6.

Blast Radius

Per refinement: zero impact on the 13 sibling overlays. Westside-admin only. Streamlit and base-hardening explicitly carved out. Confirmed clean.

Decomposition Assessment

One file, one overlay, 5 ACs (4 substantive + 1 trivial), single agent pass under the 5-minute rule. No decomposition needed.

Recommendation

Refinement is 95% there. Two body fixes get this to APPROVED:
  • [BODY] BLOCKER — In the "Expected Behavior" snippet, change - name: westside-admin to - name: app. The strategic-merge patch must target the base container name (app); the kustomization's JSON patches will rename it to westside-admin AFTER the merge. Add a one-line comment in the snippet: # container name 'app' matches base (renamed to 'westside-admin' by kustomization JSON patch).
  • [BODY] RECOMMENDED — Add AC #6: kustomize build overlays/westside-admin/prod/ | yq '.spec.template.spec.containers | length' returns 1. Verifies the strategic merge produced one container, not two — catches the container-name mistake before cluster sync.
  • [BODY] OPTIONAL — Consider matching streamlit's canonical pod-level superset (runAsUser: 1000, runAsGroup: 1000, fsGroup: 1000) for cross-overlay consistency. Not required for PSA admission. If diverging intentionally, add a one-line "why" comment.
  • [SCOPE] arch:k8s-deploy backing note still missing platform-wide. Not blocking per precedent.

Summary

Refinement work resolved all three original blockers cleanly: per-overlay scope, streamlit deferred, AC-list narrowed, misleading sentence removed, Dockerfile pre-merge note added. All six caller-specified verifications pass. New blocker introduced by the canonical YAML snippet's container-name mismatch — fix is one word change (westside-admin → app) plus a clarifying comment. Re-review on the corrected body will be a fast pass.