pal-e-docs

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

review-1118-2026-05-01-v2 Doc

review approved
Project

Verdict: APPROVED

Third review of board item #1118 after the second body refinement landed. Lucas approved Option A on 2026-05-01. The previous reviewer (review-1118-2026-05-01) flagged a one-word body fix (container name westside-admin → app in the snippet) plus a recommended new AC. Both have landed cleanly. All caller-specified verification points pass. All previously-passing checks remain clean. Ticket is ready to advance to next_up.

Caller-Specified Verifications (Second Refinement)

  • [x] 1. Container name in snippet is - name: app — CONFIRMED. Body's "Expected Behavior" snippet at the strategic-merge stage uses - name: app. Matches base container name; will merge correctly into the single existing container.
  • [x] 2. Inline comment explaining post-merge JSON-patch rename — CONFIRMED. Three-line inline comment in the snippet: "IMPORTANT: targets BASE container name; the overlay's JSON patch renames it to 'westside-admin' AFTER strategic merge applies. Using 'westside-admin' here would create a second container instead of patching the existing one." Plus a dedicated "Why container name app not westside-admin" paragraph below the snippet that walks through the kustomization patch order. Both verbose and self-documenting.
  • [x] 3. New AC: kustomize build ... | yq '.spec.template.spec.containers | length' returns 1 — CONFIRMED. Listed as AC #5: "kustomize build overlays/westside-admin/prod/ | yq '.spec.template.spec.containers | length' returns 1 (catches the wrong-container-name failure mode pre-cluster)". Catches the strategic-merge container-append regression before sync.
  • [x] 4. Pod-level superset matches streamlit pattern — CONFIRMED. Snippet has runAsUser: 1000, runAsGroup: 1000, fsGroup: 1000 at the pod-level securityContext. Cross-checked against overlays/westside-streamlit/prod/deployment-patch.yaml: identical superset (runAsNonRoot: true, runAsUser: 1000, runAsGroup: 1000, fsGroup: 1000, seccompProfile.type: RuntimeDefault). Cross-overlay consistency achieved.
  • [x] 5. Pre-merge Dockerfile USER verification updated to "(CONFIRMED)" — CONFIRMED. Body now reads: "Pre-merge Dockerfile verification (CONFIRMED): westside-admin's Dockerfile@main line 31 has USER node per PR #10 — the image actually runs as non-root, so the manifest assertion will hold at runtime." Independently verified: fetched forgejo_admin/westside-admin@main Dockerfile via API, line 31 reads USER node. Hidden runtime prerequisite is satisfied.

Regression Checks (Previously Passing)

  • [x] 6. Per-overlay scope (no base change) — CONFIRMED. Snippet targets overlays/westside-admin/prod/deployment-patch.yaml exclusively. "Why per-overlay rather than base" paragraph explains the 9-of-14-services Dockerfile-USER prerequisite. Base untouched.
  • [x] 7. Out-of-scope items explicit and pointing to filed spike tickets — CONFIRMED. "Scope Boundary (narrow)" section names both deferrals. Verified via Forgejo API:
  • [x] 8. AC list does NOT include streamlit-recovery — CONFIRMED. All 6 ACs (the original 5 + the newly-added kustomize-build check) are scoped to westside-admin only. Zero references to streamlit, basketball-api, or any sibling overlay.
  • [x] 9. Misleading "valid in any namespace" sentence still removed — CONFIRMED. Full-text scan of refined body returns zero matches for "valid in any namespace" or analogous PSA-misframings.

Independent Verifications

  • [x] File path exists — overlays/westside-admin/prod/deployment-patch.yaml verified at exact path on forgejo_admin/pal-e-deployments@main via API. Returns 200, valid YAML, container name app, no securityContext block currently. Confirms the fix is needed and the file is the right target.
  • [x] Kustomize patch order — overlays/westside-admin/prod/kustomization.yaml verified: patches: list applies deployment-patch.yaml (strategic merge) FIRST, then JSON patches that replace /spec/template/spec/containers/0/name from app → westside-admin. Inline comment line 9 confirms: "targets base name 'app' before rename". Snippet's - name: app will merge correctly.
  • [x] Streamlit reference pattern — overlays/westside-streamlit/prod/deployment-patch.yaml uses identical pod-level superset. Snippet's pod-level fields match exactly. Container-level fields match the security-relevant subset (snippet additionally adds runAsNonRoot: true at container-level, which is harmless redundancy with the pod-level setting).
  • [x] Base unchanged — bases/standard/deployment.yaml remains free of any securityContext block (verified locally). Option A scope honored.

Template Completeness

  • [x] Type (Bug)
  • [x] Lineage (full audit trail: validation note → first review → second review → this re-review)
  • [x] Repo (forgejo_admin/pal-e-deployments)
  • [x] What Broke
  • [x] Scope Boundary (narrow) — explicit OUT-OF-SCOPE list with both deferrals
  • [x] Repro Steps (3 steps, all kubectl commands)
  • [x] Expected Behavior (with corrected snippet + three "Why" justification paragraphs + pre-merge Dockerfile note)
  • [x] Environment
  • [x] Acceptance Criteria (6 ACs — manifest fields, deployment ready, FailedCreate=0, CreateContainerConfigError=0, kustomize-build container count, no-regression)
  • [x] Related (cross-references to parent issue, validation note, both reviews)

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 reviews).
  • [x] arch:k8s-deploy label — arch-k8s-deploy note still missing platform-wide. Per established precedent (review-933, review-935, review-972, review-283, review-1118-2026-04-30, review-1118-2026-05-01) 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, container name app matches snippet target.
  • [x] bases/standard/deployment.yaml — verified untouched scope (Option A).
  • [x] overlays/westside-admin/prod/kustomization.yaml — verified patch ordering compatible with snippet.

Repo Placement

Correct — forgejo_admin/pal-e-deployments. Single repo, single overlay file, single targeted change. No cross-repo dependencies for this ticket.

Dependencies

No upstream board blockers. Westside-admin Dockerfile (PR #10) is merged on forgejo_admin/westside-admin@main, satisfying the runtime non-root prerequisite. ArgoCD will pick up the change on next sync after merge. Two follow-up tickets (#139 streamlit spike, #140 system-wide PSA audit) are tracked separately and explicitly out of scope.

Acceptance Criteria

6 ACs, all verifiable:
  • AC1 — manifest field check via inspection of merged YAML.
  • AC2 — kubectl get deployment 1/1 ready after ArgoCD sync.
  • AC3 — zero FailedCreate from PSA admission via kubectl describe rs.
  • AC4 — zero CreateContainerConfigError events (catches Dockerfile-runs-as-root failure mode).
  • AC5 — kustomize build | yq '.spec.template.spec.containers | length' returns 1 (catches container-name strategic-merge bug pre-cluster). Newly added in this refinement.
  • AC6 — no regression to other services (trivially true, no base change).
AC list is appropriately scoped and cleanly testable.

Blast Radius

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

Decomposition Assessment

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

Recommendation

No action needed. Approved — advance to next_up.
Optional polish (non-blocking, dev agent can ignore):
  • [BODY] OPTIONAL — In the "Scope Boundary (narrow)" section, append explicit issue numbers (e.g., "… tracked as separate spike tickets in this repo: #139 (streamlit), #140 (system-wide PSA audit)") for one-click navigation. The current prose is unambiguous; this is a navigability nicety only.
  • [SCOPE] arch:k8s-deploy backing note still missing platform-wide. Carries forward as platform-wide tracked debt, not a per-ticket blocker per established precedent.

Summary

Two-line fix from the second review (container name app + new kustomize-build AC) landed cleanly, with a robust three-line inline comment and a dedicated justification paragraph for posterity. Pod-level superset now matches streamlit canonical. Pre-merge Dockerfile note upgraded to "(CONFIRMED)" with line-number citation. Both out-of-scope spikes are filed and verified open. All 9 caller-specified verification points pass. Ticket is ready for a dev agent — the snippet can be copy-pasted verbatim and will produce a correct kustomize merge with one container, PSA-restricted, matching streamlit's pattern.