DEVOPS-3406 - helm: declared secretsMode with ESO ExternalSecret support - #2
Conversation
Replace implicit secret wiring (secret.create + secret.existingSecret inference) with an explicit secret.secretsMode: inline | existingSecret | externalSecrets, enforced at render time with fail-fast validation for every invalid combination (unknown/emptied mode, missing or blank existingSecret, secretStoreRef.name, or pathPrefix, wrong create/ existingSecret per mode, invalid store kind). externalSecrets mode renders an ExternalSecret (ESO) whose spec.data is generated from secret.keys against <pathPrefix>/<KEY_NAME>, so the pulled key list can never drift from what workloads consume, and no secret material lands in git or rendered manifests — the GitOps/ArgoCD-safe path. apiVersion is selected from cluster capabilities (v1 else v1beta1) and target.deletionPolicy defaults to Retain. Also stop the migration Job from inlining DATABASE_URL and DEN_DB_ENCRYPTION_KEY as plaintext env outside inline mode; it now uses secretKeyRef for existingSecret/externalSecrets flows. Documented in README (per-mode subsections) and values.yaml REQUIRED comments; covered by tests/external-secrets.sh (render assertions for all three modes plus 12 negative validation cases).
There was a problem hiding this comment.
🟡 Changes recommended
secretsMode validation currently permits inline with secret.create: false, which can accidentally re-inline secrets and should be blocked with fail-fast validation plus a regression test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the packaging/helm/openwork-ee Helm chart to make secret management explicit via secret.secretsMode and adds native External Secrets Operator (ESO) support so GitOps renders do not inline secret values into manifests.
Changes:
- Introduces
secret.secretsMode: inline | existingSecret | externalSecretswith render-time validation and updated secret name resolution. - Adds ESO rendering via a new
ExternalSecrettemplate that generatesspec.datafromsecret.keysand supports capability-basedv1/v1beta1apiVersion selection. - Updates migration Job secret wiring to inline only in
inlinemode and adds a chart test script covering the new modes and validation failures.
File summaries
| File | Description |
|---|---|
| packaging/helm/openwork-ee/values.yaml | Documents secretsMode and adds the externalSecrets configuration block. |
| packaging/helm/openwork-ee/tests/external-secrets.sh | Adds chart render/validation assertions for inline, existingSecret, and externalSecrets modes. |
| packaging/helm/openwork-ee/templates/secret.yaml | Renders the v1 Secret only in inline mode and runs secrets-mode validation. |
| packaging/helm/openwork-ee/templates/migration-job.yaml | Stops inlining sensitive env vars outside inline mode (uses secretKeyRef otherwise). |
| packaging/helm/openwork-ee/templates/externalsecret.yaml | New template to render an ESO ExternalSecret from secret.keys + externalSecrets.*. |
| packaging/helm/openwork-ee/templates/configmap.yaml | Ensures secrets-mode validation runs during template rendering. |
| packaging/helm/openwork-ee/templates/_helpers.tpl | Adds secrets-mode + externalSecrets validators and capability-based ESO apiVersion helper. |
| packaging/helm/openwork-ee/README.md | Reworks documentation around the three explicit secret modes with ESO examples. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The migration Job template should invoke openwork-ee.secretsMode.validate before branching on secretsMode to ensure the legacy shim/validation always runs and never unexpectedly inlines secret values when the Job is rendered.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packaging/helm/openwork-ee/templates/_helpers.tpl:73
- The validation error message for an invalid secrets mode is
"secretsMode must be one of ...", but the values key is actuallysecret.secretsMode. Other validators in this chart consistently include the full values path (e.g.config.public.allowPrivateMcpUrls ...,observability.backend ...), so this message is harder to action when the render fails.
Consider changing it to "secret.secretsMode must be one of inline, existingSecret, externalSecrets" for consistency and quicker debugging.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The mode validation and templating changes are consistent across the chart, prevent plaintext secret rendering outside inline mode, and are backed by targeted render/negative-case tests.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
…esource in Helm chart
There was a problem hiding this comment.
🟡 Changes recommended
Two operational/correctness gaps were found (Namespace deletion risk on uninstall, and secretsMode validation not guaranteed under --show-only for templates that don’t include the validator) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packaging/helm/openwork-ee/templates/_helpers.tpl:66
openwork-ee.secretsMode.validate(and the legacy create=false shim) only runs in the templates that explicitlyincludeit. Several other templates referenceopenwork-ee.secretName(e.g. den-api/den-web/inference/env-probe), and rendering them viahelm template --show-only ...can currently bypass secretsMode validation/shimming entirely, contradicting the “render-enforced” contract.
Consider invoking the validator inside openwork-ee.secretName so any template that consumes the secret name (the common case) also triggers the validation/shim.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| metadata: | ||
| name: {{ include "openwork-ee.namespace" . }} | ||
| labels: | ||
| {{- include "openwork-ee.labels" . | nindent 4 }} |
There was a problem hiding this comment.
🟡 Changes recommended
A helper bug can incorrectly collapse fullname for some release/chart name combinations, and some validated ESO/name fields can still render with leading/trailing whitespace, producing invalid or incorrect Kubernetes references.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packaging/helm/openwork-ee/templates/_helpers.tpl:66
secret.existingSecretis validated withtrim, butopenwork-ee.secretNamereturns the raw value. If a user provides leading/trailing whitespace, validation will pass but rendered resource references will include spaces and be rejected by Kubernetes (or fail to match the intended Secret). Trimming at the point of use keeps behavior consistent with validation.
packaging/helm/openwork-ee/templates/externalsecret.yaml:29externalSecrets.*fields are validated usingtrim, but the template renders the untrimmed values (e.g.secretStoreRef.name,pathPrefix). This means values like"store "or"trunk "will pass validation but render invalid/incorrect references (spaces in names or remote key paths). Trim these strings before rendering to match the validator’s intent.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| {{- else -}} | ||
| {{- $name := default .Chart.Name .Values.nameOverride -}} | ||
| {{- if contains $name .Release.Name -}} | ||
| {{- if or (contains $name .Release.Name) (hasPrefix .Release.Name $name) -}} |
…esource in Helm chart
96af0fb to
5a5e775
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are verified issues that can break upgrades or intended validation behavior (fullname naming change risk, nondeterministic ExternalSecret output ordering, and required ESO config not actually enforced due to non-empty defaults, plus outdated namespace docs).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packaging/helm/openwork-ee/templates/_helpers.tpl:10
- The added
hasPrefix .Release.Name $namebranch changesfullnameresolution when the release name is a prefix of the chart name (e.g. releaseopenworkwith chartopenwork-ee). That would rename most resources on upgrade (fromopenwork-openwork-ee-*toopenwork-*), which can orphan old resources and break Helm upgrades unexpectedly.
{{- if or (contains $name .Release.Name) (hasPrefix .Release.Name $name) -}}
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
| {{- $prefix := .Values.externalSecrets.pathPrefix | trimSuffix "/" }} | ||
| {{- range $name, $envKey := .Values.secret.keys }} | ||
| - secretKey: {{ $envKey }} | ||
| remoteRef: | ||
| key: {{ printf "%s/%s" $prefix $envKey | quote }} | ||
| conversionStrategy: {{ $.Values.externalSecrets.conversionStrategy }} | ||
| decodingStrategy: {{ $.Values.externalSecrets.decodingStrategy }} | ||
| metadataPolicy: {{ $.Values.externalSecrets.metadataPolicy }} | ||
| {{- end }} |
| # blank or empty (the render fails otherwise), and must match the name of | ||
| # your SecretStore/ClusterSecretStore. Without it ESO has no provider to | ||
| # read from. | ||
| name: "external-secrets" |
| # Render a Namespace object for the namespace above, so GitOps flows (ArgoCD | ||
| # runs helm template and does not create namespaces unless CreateNamespace=true) | ||
| # manage it as a resource from git. Set false only when the platform team | ||
| # provisions namespaces out of band. | ||
| createNamespace: true |
…RBAC, init containers, and hook annotations
There was a problem hiding this comment.
🔵 Needs a closer look
createNamespace currently won’t prevent first-time install/sync failures when migrations.hook is enabled because Helm/ArgoCD hooks run before the Namespace manifest is applied.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packaging/helm/openwork-ee/templates/namespace.yaml:15
createNamespacerenders a Namespace as a normal manifest, but the chart’s default migrations run aspre-install,pre-upgradehooks (migrations.hook: true). Helm/ArgoCD applies hooks before normal manifests, so a first-time install into a non-existent namespace will still fail because the hook Job/RBAC resources are created before the Namespace exists. If the intention is to make the chart self-contained for GitOps/Helm installs without pre-provisioning the namespace, the Namespace needs to be created in an earlier hook phase (or hooks must be disabled when relying on createNamespace).
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The Namespace template performs an unconditional cluster-scoped lookup that can break restricted-RBAC Helm renders, and secretKeyRef keys should be quoted to avoid Helm type-coercion producing invalid manifests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packaging/helm/openwork-ee/templates/migration-job.yaml:87
- The
secretKeyRef.keyvalues come fromsecret.keys.*and are currently unquoted, so--set secret.keys.databaseUrl=123(or other Helm-typed values) would render a non-string YAML scalar and be rejected by Kubernetes schema validation. Quoting these keys makes overrides robust and matches the chart’s approach of quoting other user-provided string fields.
This issue also appears on line 92 of the same file.
packaging/helm/openwork-ee/templates/migration-job.yaml:96
- Same issue as DATABASE_URL:
secretKeyRef.keyshould be quoted to avoid Helm type-coercion producing a non-string YAML scalar when users overridesecret.keys.denDbEncryptionKeyvia--set.
valueFrom:
secretKeyRef:
name: {{ include "openwork-ee.secretName" . }}
key: {{ .Values.secret.keys.denDbEncryptionKey }}
{{- end }}
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
| {{- $namespaceName := include "openwork-ee.namespace" . | trimAll "\"" -}} | ||
| {{- $existing := lookup "v1" "Namespace" "" $namespaceName -}} | ||
| {{- if and .Values.createNamespace (not $existing) }} | ||
| apiVersion: v1 | ||
| kind: Namespace | ||
| metadata: | ||
| name: {{ include "openwork-ee.namespace" . }} | ||
| labels: | ||
| {{- include "openwork-ee.labels" . | nindent 4 }} | ||
| {{- if .Values.migrations.hook }} | ||
| annotations: | ||
| "helm.sh/hook": pre-install,pre-upgrade | ||
| "helm.sh/hook-weight": "-11" | ||
| "helm.sh/hook-delete-policy": before-hook-creation | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
🟡 Changes recommended
The Namespace hook uses hook-delete-policy: before-hook-creation, which can cause a Namespace delete/recreate cascade if the hook re-runs in hook-aware GitOps/templating flows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Two issues should be addressed before approval: a regex bug in tests/namespace.sh that can make the hook-safety check scan the wrong documents, and an overly-permissive legacy shim condition in _helpers.tpl that can silently reroute partially real inline values instead of failing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packaging/helm/openwork-ee/templates/_helpers.tpl:111
- The legacy
create: falseshim currently triggers if any placeholder marker is present (or (...)). That means a partially-edited values file (e.g. realdatabaseUrlbut still-CHANGE_MEauth/encryption) will be silently auto-routed toexistingSecretinstead of failing, which contradicts the "untouched placeholder values" intent and thevalues.yamlcomment about real values still failing.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Some newly added templates emit Secret names unquoted, which can produce invalid manifests if an existingSecret name is YAML-scalar-like (e.g. true/1234) and be rejected at apply time.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
packaging/helm/openwork-ee/templates/externalsecret.yaml:7
metadata.namerenders the Secret name without quoting. If a user supplies anexistingSecretname that looks like a YAML scalar (e.g.true,1234), YAML->JSON decoding can fail because it becomes non-string. Quote the name to guarantee a string.
This issue also appears on line 29 of the same file.
packaging/helm/openwork-ee/templates/migration-job.yaml:85
secretKeyRef.nameis emitted unquoted; if the resolved Secret name is a YAML keyword/number (possible insecretsMode=existingSecret), the manifest can fail to apply due to type mismatch. Quote the rendered name.
This issue also appears on line 94 of the same file.
packaging/helm/openwork-ee/templates/migration-job.yaml:94
secretKeyRef.nameis emitted unquoted; scalar-like Secret names (e.g.true,1234) can be decoded as non-strings and rejected. Quote the rendered name.
name: {{ include "openwork-ee.secretName" . }}
packaging/helm/openwork-ee/templates/externalsecret.yaml:29
spec.target.namerenders without quoting; scalar-like Secret names (e.g.true,1234) can cause apply-time type errors due to YAML coercion. Quote this value to force a string.
name: {{ include "openwork-ee.secretName" . }}
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new tests/external-secrets.sh assertions expect unquoted name: fields, but openwork-ee.secretName renders quoted names, so the test suite will fail even when templates are correct.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
packaging/helm/openwork-ee/tests/external-secrets.sh:142
- Same quoting issue as above:
secretNamerendersname: "padded-secret", so this assertion will not match and will fail even when the output is correct.
padded_existing_rendered="$tmp_dir/padded-existing.yaml"
helm template openwork-ee "$chart_dir" -f "$padded_existing_values" > "$padded_existing_rendered"
assert_count "$padded_existing_rendered" 'name: padded-secret' 5
assert_not_contains "$padded_existing_rendered" ' padded-secret'
assert_count "$enabled_rendered" 'helm.sh/hook-weight": "-10"' 1
packaging/helm/openwork-ee/tests/external-secrets.sh:400
- Same quoting issue as above:
secretNamerendersname: "manually-managed", so the current needle (without quotes) won't match the rendered manifests.
helm template openwork-ee "$chart_dir" -f "$tmp_dir/existing-values.yaml" > "$existing_rendered"
assert_count "$existing_rendered" 'kind: Secret' 0
assert_count "$existing_rendered" 'kind: ExternalSecret' 0
assert_count "$existing_rendered" 'name: manually-managed' 5
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new Helm validation/legacy-shim logic has a few concrete edge cases (notably scalar existingSecret handling and ESO deletionPolicy vs hook behavior) that should be tightened to preserve the stated fail-fast and safety guarantees.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
packaging/helm/openwork-ee/templates/_helpers.tpl:105
secretsMode.validatecheckssecret.existingSecretusing Go-template truthiness (if .Values.secret.existingSecret), so scalar-looking values like--set secret.existingSecret=falseor--set secret.existingSecret=0bypass the intended fail-fast guard in non-existingSecretmodes. UsetoString(optionally with/without trim) so any non-empty value is rejected outsidesecretsMode=existingSecret.
packaging/helm/openwork-ee/templates/_helpers.tpl:124- The legacy create=false shim treats a DSN as a placeholder when it merely contains
******, which can accidentally reroute a real DSN if it includes that substring. Since the documented placeholder uses a******prefix, preferhasPrefix "******"to reduce false positives while preserving the legacy behavior.
packaging/helm/openwork-ee/templates/_helpers.tpl:164 - In
externalSecretsmode,externalsecret.yamlis rendered as a Helm hook withbefore-hook-creation, which deletes and recreates the ExternalSecret on upgrades. If a user setsexternalSecrets.target.deletionPolicy: Delete, ESO may delete the materialized Secret during upgrades (brief outage / crashloop risk). Consider validating and failing fast whenmigrations.hook=trueanddeletionPolicy=Delete.
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
secret.create+secret.existingSecretinference) with an explicit, render-enforcedsecret.secretsMode: inline | existingSecret | externalSecrets.secretsMode: externalSecretsrenders anExternalSecretwhosespec.datais generated fromsecret.keysagainst<pathPrefix>/<KEY_NAME>— git holds only store references and remote paths, never secret values (GitOps/ArgoCD-safe).DATABASE_URL/DEN_DB_ENCRYPTION_KEYas plaintext env outsideinlinemode;secretKeyRefis used forexistingSecret/externalSecretsflows.metadata.namespacefrom a quotednamespacevalue defaulting toopenwork.)Why
helm template, so anything insecret.values(e.g. a real MySQL DSN) lands in git, the rendered Job manifest, and etcd — Jobs get no redaction. The previous "docs say don't do it" stance doesn't stop anyone.create: true+existingSecretset,existingSecretwith no name, etc.), failing only at pod start.spec.dataremoteRef per key, unchanged sinceexternal-secrets.io/v1beta1).Issue
Scope
values.yaml:secret.secretsMode(documented, REQUIRED annotations on every must-populate line),externalSecretsblock (secretStoreRef,pathPrefix,refreshInterval,targetpolicies, uniform strategy fields); real-shapeddatabaseUrlplaceholder.templates/_helpers.tpl:secretsMode.validate+externalSecrets.validate(fail-fast, blank-rejecting),externalSecrets.apiVersioncapability cascade,secretNameresolution by mode.templates/externalsecret.yaml(new): rendersspec.datafromsecret.keys; capability-selectedv1/v1beta1.templates/secret.yaml,migration-job.yaml,configmap.yaml: mode-gated rendering/validation.README.md: Secrets section restructured around the three declared modes with AWS Secrets Manager examples.tests/external-secrets.sh(new): render assertions for all three modes + 12 negative validation cases.namespacevalue + quoted helper, all 11 resources namespaced,tests/namespace.sh.Out of scope
secretsMode: inline(the chart fails only on incoherent config, not on value content) — deliberate;inlineis documented as local-eval-only.customCa/observabilityvalidators.dataFrom.extract/rewritesupport — can be added if a real consumer needs whole-JSON trunks.Testing
Ran
for test in packaging/helm/openwork-ee/tests/*.sh; do bash "$test"; done(helm v3.16.3)helm templateacross all threesecretsModevalues, plus invalid combinations (secretsMode: vault, emptied mode, whitespace-only names/prefix,create: truein ESO mode)data[].remoteRefResult
CI status
publish-ee-imagesworkflow viapackaging/helm/openwork-ee/tests/*.shloop)Manual verification
helm template openwork-ee ./packaging/helm/openwork-ee— default renders 8 resources in namespaceopenwork, one Secret from inline values, no ExternalSecret.helm template ... --set secret.secretsMode=externalSecrets --set secret.create=false --set externalSecrets.secretStoreRef.name=external-secrets --set externalSecrets.pathPrefix=eks/openwork/prod/den— renders one ExternalSecret with 20spec.dataentries keyed<pathPrefix>/<KEY_NAME>, no v1/Secret, no inline values anywhere in output.helm template ... --set secret.secretsMode=externalSecrets(withoutcreate: false) — fails withsecret.create must be false when secretsMode is not inline; repeat for the other 11 negative cases intests/external-secrets.sh.Evidence
Risk
secretsMode: inlinereproduces the previous render byte-for-byte (same Secret, same migration Job env). Breaking only for values files that setsecret.create: false/existingSecretwithout the newsecretsModekey — those now fail at render with an explicit message telling the user exactly what to set.Rollback
secret.create/existingSecretbehavior and hook-based migration Job env.