DEVOPS-3406 - inject required secret keys only - #4
DEVOPS-3406 - inject required secret keys only#4raul-gherman-modaoperandi wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Journey-level verification is missing, and the operator documentation contains misleading key-selection guidance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the Helm chart to inject only boot-critical secrets by default while allowing explicit optional keys.
Changes:
- Adds and validates
externalSecrets.optionalKeys. - Limits default ESO rendering to three required keys.
- Updates chart tests and documentation.
File summaries
| File | Description |
|---|---|
values.yaml |
Defines optional key configuration. |
tests/external-secrets.sh |
Tests required and optional rendering. |
templates/externalsecret.yaml |
Selectively renders secret entries. |
templates/_helpers.tpl |
Validates optional key names. |
README.md |
Documents selective secret injection. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 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
Optional-key updates are not propagated safely to running workloads, and documentation issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The rollout can race ESO reconciliation, and renamed Secret keys do not affect the checksum.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
| optionalKeys) instead: adding/removing an optionalKeys entry changes the pod | ||
| template and rolls the workloads, so envFrom picks up the new keys that ESO | ||
| materializes out of band. existingSecret mode is operator-managed — no chart |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Workload rollouts can race ESO synchronization, and source configuration changes do not affect the checksum.
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:98
- This rollout can race ESO reconciliation. On upgrade the retained target Secret already exists, Helm does not wait for this ExternalSecret hook to become Ready, and the migration init container checks only Secret existence. The Deployments can therefore restart before an added key is present (or before a removed key disappears), and
envFromremains stale after ESO eventually syncs. Gate the rollout on the expected target-key state/ExternalSecret readiness, or perform a post-sync restart.
optionalKeys) instead: adding/removing an optionalKeys entry changes the pod
template and rolls the workloads, so envFrom picks up the new keys that ESO
materializes out of band. existingSecret mode is operator-managed — no chart
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The rollout can race ESO reconciliation, leaving restarted pods with stale secret keys.
Review details
Suppressed comments (1)
packaging/helm/openwork-ee/templates/_helpers.tpl:109
- This checksum changes the Deployments in the same Helm upgrade that updates the ExternalSecret, but it does not wait for ESO to reconcile the target Secret. On an existing installation the old Secret already satisfies the migration hook's existence check, so pods can restart before an added key is present (or before a removed key disappears) and keep the stale environment until another rollout. The rollout must be triggered by the reconciled Secret itself, or sequenced behind a check for the expected key set rather than directly from
optionalKeys.
{{- range $name := concat $requiredKeys $optionalKeys | uniq | sortAlpha -}}
{{- $resolvedKeys = append $resolvedKeys (index $.Values.secret.keys $name) -}}
{{- end -}}
{{- $resolvedKeys | uniq | sortAlpha | toJson | sha256sum -}}
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
f396400 to
9e65fcb
Compare
There was a problem hiding this comment.
🟡 Changes recommended
ESO reconciliation can race workload rollouts, and upgrade and metadata-policy handling remain incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packaging/helm/openwork-ee/README.md:340
- This is a breaking default for existing ESO releases: older chart versions rendered all
secret.keys, while an existing values file has nooptionalKeys, so reconciliation can remove every non-core key. Add an upgrade warning requiring operators to list all currently consumed non-core keys before upgrading, otherwise SMTP, Daytona, and similar integrations can fail after reconciliation or restart.
the provider. Only the three boot-critical keys (`DATABASE_URL`,
`BETTER_AUTH_SECRET`, `DEN_DB_ENCRYPTION_KEY`) are rendered by default; add
more by name via `optionalKeys`:
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The checksum omits a content-changing ESO option, and the documented zero-second wait is not honored.
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:119
metadataPolicyis part of the remote value semantics: switching fromNonetoFetchcan make ESO materialize provider metadata instead of the secret value. Because it is omitted from this checksum, that supported values change updates the Secret but does not roll existing pods, leaving theirenvFromsnapshot stale. Include it in the hashed input.
"conversionStrategy" .Values.externalSecrets.conversionStrategy
"decodingStrategy" .Values.externalSecrets.decodingStrategy -}}
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Existing ESO upgrades can silently remove non-core secrets, and previously raised reconciliation/checksum concerns remain unresolved.
Review details
Suppressed comments (1)
packaging/helm/openwork-ee/README.md:340
- This selective default is a breaking upgrade for existing ESO releases: older values files have no
optionalKeys, so reconciliation drops every previously rendered non-core key and restarted pods can lose SMTP, Daytona, or other integrations. Add an upgrade warning requiring operators to list all currently consumed non-core keys before upgrading.
The chart renders `spec.data` — the oldest stable ESO shape, unchanged since
`external-secrets.io/v1beta1` — pulling keys from `<pathPrefix>/<KEY_NAME>` in
the provider. Only the three boot-critical keys (`DATABASE_URL`,
`BETTER_AUTH_SECRET`, `DEN_DB_ENCRYPTION_KEY`) are rendered by default; add
more by name via `optionalKeys`:
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
No description provided.