Waleed 896d15b666 fix(helm): close blocker/real-gap findings from Helm chart best-practices audit (#5555) ...

* fix(helm): close blocker/real-gap findings from Helm chart best-practices 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.

* fix(helm): move copilot's static config out of the ESO-required Secret

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.

* fix(helm): don't shadow copilot's existingSecret with envDefaults

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.

* fix(helm): key copilot's existingSecret check off its own secret.create, 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.

* fix(helm): checksum/secret annotation on copilot ignores ESO-sourced 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.

2026-07-09 20:00:52 -07:00

..

sim

fix(helm): close blocker/real-gap findings from Helm chart best-practices audit (#5555)

2026-07-09 20:00:52 -07:00