fix(helm): close blocker/real-gap findings from Helm chart best-practices audit#5555
Conversation
…ices audit Verified every finding against the official Helm docs and Kubernetes Pod Security Standards docs before fixing, and validated each fix with helm lint/template plus the chart's own helm-unittest suite (65 -> 79 tests, all new tests confirmed to fail on the pre-fix code): - Blocker: values.schema.json documented "minimum 32/8 characters" on BETTER_AUTH_SECRET/ENCRYPTION_KEY/postgresql.auth.password but never enforced it. Added anyOf minLength-or-empty constraints (empty stays legal for existingSecret/ESO modes) — verified negative/positive cases live, no regression for any secret-delivery mode. - Real gap: copilot didn't support the External Secrets Operator mode the rest of the chart offers (app/postgresql/externalDatabase). Added external-secret-copilot.yaml, remoteRefs.copilot, and extended sim.copilot.validate with the same "map it or remove it" fail-fast guard app.env/realtime.env already have. Verified byte-identical rendering for the existing non-ESO path. - Real gap: the OpenTelemetry Collector was the only workload missing the shared Restricted-profile securityContext helpers (no container-level hardening at all). Wired sim.podSecurityContext/containerSecurityContext in, preserving the collector's original UID/GID/fsGroup. - Real gap: copilot templates hand-rolled label/selector blocks instead of using the chart's established sim.<component>.labels/selectorLabels pattern. Added sim.copilot.*/sim.copilotPostgresql.* helpers and refactored every consumer — confirmed byte-identical helm template output before/after (selector labels are immutable on upgrade, so this was verified, not assumed). - Documented (README): the ingressFrom default and readOnlyRootFilesystem posture, both real but intentional tradeoffs the audit flagged as underdocumented. Added extraVolumes/extraVolumeMounts to copilot's Deployment (realtime/pii already had it) so the readOnlyRootFilesystem guidance is actually actionable for all three stateless services. Deferred (nice-to-have, not blocking): pinning the two floating Postgres image tags, values.schema.json stubs for ~13 uncovered top-level sections, and an OTel collector image version bump — none are correctness issues.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview
Copilot gains the same secret routing model as app: new Telemetry OTel collector pod/container security contexts are wired through README documents default permissive Reviewed by Cursor Bugbot for commit fd364c2. Configure here. |
Greptile SummaryThis PR hardens the Helm chart and expands copilot secret handling. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "fix(helm): checksum/secret annotation on..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile caught a real bug: copilot.server.env shipped with non-empty static defaults (PORT, SERVICE_NAME, ENVIRONMENT, LOG_LEVEL), unlike app.env/realtime.env which ship fully empty. The new ESO validation correctly required every non-empty env key to be mapped in externalSecrets.remoteRefs.copilot — but that meant a default install with copilot + ESO enabled failed demanding secret-store paths for values that were never secrets. Fixed by applying the chart's own existing pattern for this exact problem: moved the 4 static keys into copilot.server.envDefaults (mirroring app.envDefaults) and inlined them as plain container env, bypassing the Secret/ExternalSecret system entirely — same rationale already documented for app.envDefaults. Verified live that Greptile's exact repro (default copilot env + ESO enabled, only the 7 real secrets mapped) now renders cleanly and the four values still reach the container. Added a regression test that fails on the pre-fix code.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile and Cursor Bugbot both independently caught this: in copilot.server.secret.create=false (existingSecret) mode, the chart still unconditionally inlined copilot.server.envDefaults as explicit container env. Kubernetes gives explicit env precedence over envFrom, so a pre-existing Secret's PORT/LOG_LEVEL/etc values were silently overridden by the chart defaults — the exact shadowing bug app.envDefaults already guards against via its own $useExistingSecret skip, which I forgot to mirror when copying the pattern to copilot. Skip envDefaults entirely in existingSecret mode (matching app's existing behavior — the pre-created Secret is the sole source of truth), while still rendering extraEnv. Verified live: existingSecret mode now renders no env: block at all when extraEnv is unset, and still renders extraEnv without envDefaults leaking in when it is set. Added two regression tests, confirmed both fail on the pre-fix code.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 27d7743. Configure here.
…te, not the global ESO flag Round 2's fix (which I copied nearly verbatim from Greptile's own suggested diff) used $useExistingSecret := and (not externalSecrets.enabled) (not copilot.server.secret.create) — Greptile caught its own suggestion's remaining bug on round 3: when externalSecrets.enabled=true globally (for app/postgresql) but copilot itself uses copilot.server.secret.create=false with its own pre-created Secret, that condition evaluated to non-existingSecret mode, so envDefaults still inlined and shadowed the user's Secret values — same bug, different trigger condition. envFrom always points at the user-provided Secret name whenever secret.create=false, independent of what other components do with ESO, so the check should key on that alone. Verified live: global ESO enabled + copilot's own existingSecret now renders no env: block and envFrom correctly points at the pre-created secret name; the two scenarios that should still inline (copilot itself on ESO, plain inline mode) still work. Added a regression test, confirmed it fails against round 2's guard.
|
@cursor review |
…secret Cursor Bugbot caught a real bug: checksum/secret only hashed secrets-copilot.yaml's rendered output, but under externalSecrets.enabled=true that template renders nothing (env credentials come from external-secret-copilot.yaml instead). Result: changing externalSecrets.remoteRefs.copilot mappings wouldn't change the pod template hash, so Kubernetes would never restart the copilot pod to pick up the new mapping — stale envFrom values until a manual restart. Fixed by hashing the concatenation of both templates' rendered output: whichever mode is active, only one renders non-empty content, but the concatenated hash still changes on a mode switch or a remoteRefs change. This can't reach into the live secret store value ESO syncs (Helm only sees the ExternalSecret manifest at render time) — that's an inherent ESO limitation, not something a checksum annotation can close; documented as such in the template comment. Note: deployment-app.yaml and deployment-realtime.yaml have this same latent limitation in ESO mode (checksum/secret only hashes secrets-app.yaml), but that's pre-existing code outside this PR's diff — not fixed here to stay scoped to what Cursor actually flagged. Verified live: the checksum differs across two different remoteRefs.copilot.LICENSE_KEY mappings, and still changes correctly in plain inline mode. Added a regression test.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fd364c2. Configure here.
1 participant
Summary
values.schema.jsonclaimed "minimum 32/8 characters required" onBETTER_AUTH_SECRET/ENCRYPTION_KEY/postgresql.auth.passwordbut never enforced it — a 5-character secret passed validation. AddedanyOfminLength-or-empty constraints (empty stays legal for existingSecret/ESO modes)external-secret-copilot.yaml,remoteRefs.copilot, and the same "map it or remove it" fail-fast validation the rest of the chart hasallowPrivilegeEscalation: false, no capability drop, no seccomp). Wired in the shared security-context helpers, preserving its original UID/GID/fsGroupsim.<component>.labelspattern. Added the missing helpers and refactored every consumer — verified byte-identicalhelm templateoutput before/after (selector labels are immutable on upgrade, so I checked this, not assumed it)networkPolicy.ingressFromdefault andreadOnlyRootFilesystemposture in the README (both real, intentional tradeoffs the audit flagged as underdocumented), and addedextraVolumes/extraVolumeMountsto copilot's Deployment so that guidance is actually actionable (realtime/pii already had it, copilot didn't)Type of Change
Testing
helm lint --strictandhelm templateagainst the base chart, every example overlay, and multiple hand-built ESO scenarios (positive and negative)helm unittestsuite: 65 → 79 tests, all passing; all 14 new tests confirmed to fail against the pre-fix code (reverted fixes, re-ran, confirmed failure, restored)Checklist