Resolve external secrets by convention

[maor-rozenfeld]

Fixes CI-177.

[Copilot]

Pull request overview

Updates the Helm chart’s ExternalSecret generation to derive secret entries automatically from configured environment maps (by convention), and introduces a dedicated Superset secret key separate from OPS_ENCRYPTION_KEY.

Changes:

• Add SUPERSET_SECRET_KEY to openopsEnv and wire Superset to use it.
• Replace the hardcoded ExternalSecret spec.data list with convention-based generation across component env maps.
• Add helper templates to resolve remote secret properties for “derived” keys and to emit ExternalSecret data entries.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
chart/values.yaml	Adds SUPERSET_SECRET_KEY and updates analytics env mapping to use it.
chart/templates/external-secret.yaml	Refactors ExternalSecret data to be generated from merged env maps by convention.
chart/templates/_helpers.tpl	Adds helper templates for mapping derived env keys to AWS Secrets Manager properties and emitting ExternalSecret entries.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[linear]

CI-177 Auto-generate ExternalSecret entries from env var definitions

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

SUPERSET_SECRET_KEY is introduced with an empty default but it isn’t validated as required anywhere (unlike OPS_ENCRYPTION_KEY, OPS_JWT_SECRET, etc). Since analytics.env.SUPERSET_SECRET_KEY now reads from this value, non-external-secret installs can render/deploy with an empty Superset secret key and fail at runtime. Consider adding a chart-level validation for openopsEnv.SUPERSET_SECRET_KEY when analytics is deployed (and validation isn’t being skipped).

Suggested change

	SUPERSET_SECRET_KEY: ""
	# NOTE: Override this with a strong, unique value in your own values file for production.
	SUPERSET_SECRET_KEY: "CHANGE_ME_SUPERSET_SECRET_KEY"

[Copilot]

data: is followed by template actions starting at column 0 using {{- ... -}}. The leading - trims the newline/whitespace before the action, which can cause data: and the first rendered list item to end up on the same line (invalid YAML) or otherwise mis-indented. Keep the newline after data: and indent the rendered list items under it (e.g., avoid left-trimming here and/or nindent the included output).

Suggested change

	{{- $allEnv := dict -}}
	{{- range $k, $v := .Values.openopsEnv }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}
	{{- range $k, $v := .Values.tables.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{- range $k, $v := .Values.analytics.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{- range $k, $v := .Values.postgres.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{- if .Values.engine }}{{- if .Values.engine.env }}
	{{- range $k, $v := .Values.engine.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{- end }}{{- end }}
	{{- include "openops.collectSecretEntries" (dict "env" $allEnv "secretName" .Values.externalSecrets.secretName) }}
	{{ $allEnv := dict -}}
	{{ range $k, $v := .Values.openopsEnv }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}
	{{ range $k, $v := .Values.tables.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{ range $k, $v := .Values.analytics.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{ range $k, $v := .Values.postgres.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{ if .Values.engine }}{{ if .Values.engine.env }}
	{{ range $k, $v := .Values.engine.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{ end }}{{ end }}
	{{ include "openops.collectSecretEntries" (dict "env" $allEnv "secretName" .Values.externalSecrets.secretName) }}

[Copilot]

The env merge into $allEnv currently uses if not (hasKey $allEnv $k) which prevents later env maps (e.g. analytics.env) from overriding keys defined earlier (e.g. tables.env). This differs from templates/secret-env.yaml, which overwrites on collisions, and it can block legitimate per-component overrides (notably DATABASE_PASSWORD exists in both tables and analytics env by default). Consider using overwrite semantics (later sources win) to match existing behavior.

Suggested change

	{{- range $k, $v := .Values.tables.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{- range $k, $v := .Values.analytics.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{- range $k, $v := .Values.postgres.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{- if .Values.engine }}{{- if .Values.engine.env }}
	{{- range $k, $v := .Values.engine.env }}{{ if not (hasKey $allEnv $k) }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}{{ end }}
	{{- range $k, $v := .Values.tables.env }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}
	{{- range $k, $v := .Values.analytics.env }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}
	{{- range $k, $v := .Values.postgres.env }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}
	{{- if .Values.engine }}{{- if .Values.engine.env }}
	{{- range $k, $v := .Values.engine.env }}{{ $_ := set $allEnv $k ($v | toString) }}{{ end }}

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

validateSecrets now reads required secrets from .Values.openopsEnvSecrets (e.g., OPS_ENCRYPTION_KEY, OPS_JWT_SECRET). The repo’s example overlays still document/set these under openopsEnv (not openopsEnvSecrets), so following the provided examples will now fail chart rendering. Update the example values files (and/or add a temporary backward-compat shim) to keep the bundled overlays/docs consistent with this new convention.

[Copilot]

openopsEnvSecrets.SUPERSET_SECRET_KEY has a hard-coded default value ("thisISaSECRET_1234"). Shipping a real secret (even a placeholder) in default values is insecure and also defeats the goal of forcing explicit configuration. Make this empty by default and add validation (similar to the other required secrets) so installs fail fast unless a unique key is provided.

Suggested change

	SUPERSET_SECRET_KEY: "thisISaSECRET_1234"
	SUPERSET_SECRET_KEY: "" # REQUIRED: Random secret string

[Copilot]

Rendering .Values.openopsEnv and .Values.openopsEnvSecrets as two separate env: blocks can produce duplicate environment variable names if a user (or an overlay file) sets the same key in both maps, which Kubernetes rejects. Consider rendering a single merged map with a deterministic precedence (e.g., secrets overriding non-secrets) or filtering keys from openopsEnv that also exist in openopsEnvSecrets before rendering.

Suggested change

	{{ include "openops.renderEnv" (dict "root" . "env" .Values.openopsEnv) | nindent 12 }}
	{{ include "openops.renderEnv" (dict "root" . "env" .Values.openopsEnvSecrets) | nindent 12 }}
	{{- $openopsEnv := .Values.openopsEnv | default (dict) }}
	{{- $openopsEnvSecrets := .Values.openopsEnvSecrets | default (dict) }}
	{{- $mergedEnv := merge $openopsEnv $openopsEnvSecrets }}
	{{ include "openops.renderEnv" (dict "root" . "env" $mergedEnv) | nindent 12 }}

[Copilot]

Rendering .Values.openopsEnv and .Values.openopsEnvSecrets separately can lead to duplicate env entries when the same key is defined in both maps (common during migrations/overrides). Kubernetes requires env var names to be unique, so this can break deployments. Prefer rendering a single merged env map (with openopsEnvSecrets taking precedence) or exclude overlapping keys from openopsEnv.

Suggested change

	{{ include "openops.renderEnv" (dict "root" . "env" .Values.openopsEnv) | nindent 12 }}
	{{ include "openops.renderEnv" (dict "root" . "env" .Values.openopsEnvSecrets) | nindent 12 }}
	{{- $mergedEnv := dict -}}
	{{- if .Values.openopsEnv }}
	{{- $mergedEnv = merge $mergedEnv .Values.openopsEnv }}
	{{- end }}
	{{- if .Values.openopsEnvSecrets }}
	{{- $mergedEnv = merge $mergedEnv .Values.openopsEnvSecrets }}
	{{- end }}
	{{ include "openops.renderEnv" (dict "root" . "env" $mergedEnv) | nindent 12 }}

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

SUPERSET_SECRET_KEY is referenced in analytics.env, but openops.validateSecrets doesn’t validate that it’s set (and ideally not equal to any placeholder/default). Consider adding a required check here so installs fail fast instead of starting analytics with an empty/known secret key.

Suggested change

	{{- end -}}
	{{- end -}}
	{{- end -}}
	{{- if not .Values.openopsEnvSecrets.SUPERSET_SECRET_KEY -}}
	{{- fail "ERROR: SUPERSET_SECRET_KEY is required. Generate with: openssl rand -hex 32" -}}
	{{- end -}}
	{{- end -}}

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In openops.validateSecrets, the error message recommends openssl rand -hex 32, but the validation enforces len(OPS_ENCRYPTION_KEY) == 32. openssl rand -hex 32 produces 64 hex chars, so users following the error message will still fail validation. Update the guidance (e.g., -hex 16) or adjust the required length to match the recommended command.

[Copilot]

The guidance to generate OPS_ENCRYPTION_KEY with openssl rand -hex 32 doesn’t match the documented/validated requirement of a 32-character hex string. openssl rand -hex 32 outputs 64 hex characters (32 bytes). Either update the docs to openssl rand -hex 16 or change the validation/requirement to 64 chars so users don’t follow instructions that will immediately fail validation.

Suggested change

	# Generate secure values with: openssl rand -hex 32
	openopsEnvSecrets:
	OPS_ENCRYPTION_KEY: "" # REQUIRED: 32-character hex string
	# Generate secure values with: openssl rand -hex 16
	openopsEnvSecrets:
	OPS_ENCRYPTION_KEY: "" # REQUIRED: 32-character (16-byte) hex string, e.g. from `openssl rand -hex 16`

[Copilot]

openops.secretPropertyName is not robust for the actual value strings used in values.yaml (they are typically quoted, e.g. "{{ .Values.openopsEnvSecrets.X }}"). Because the function only trims {{/}} and spaces, it will often fail to strip leading quotes/whitespace and may emit an invalid remoteRef.property like the full template string. Consider extracting the referenced key with a regex (capture group on \.Values\.openopsEnvSecrets\.([A-Z0-9_]+)) and falling back to $key when there is no match.

Suggested change

	{{- if contains ".Values.openopsEnvSecrets." $value -}}
	{{- $value | trimPrefix "{{" | trimPrefix " " | trimSuffix "}}" | trimSuffix " " | trimPrefix ".Values.openopsEnvSecrets." -}}
	{{- else -}}
	{{- $key -}}
	{{- end -}}
	{{- end }}
	{{- $extracted := regexReplaceAll ".*\\.Values\\.openopsEnvSecrets\\.([A-Z0-9_]+).*" $value "$1" -}}
	{{- if ne $extracted "" -}}
	{{- $extracted -}}
	{{- else -}}
	{{- $key -}}
	{{- end }}
	{{- end }}

[Copilot]

The templating whitespace trimming here is likely to generate invalid YAML under spec.data:. In particular, {{- $allEnv := dict -}} trims both the preceding and following newlines, which can collapse data: and the first list item onto the same line. Remove the trailing - (and consider using include ... | nindent 2/nindent 4 for the emitted list) to ensure data: is followed by a newline and properly indented - secretKey entries.

[maor-rozenfeld]

Addressed the relevant review comment:

openssl rand -hex mismatch (✅ Fixed): Changed openssl rand -hex 32 → openssl rand -hex 16 in error messages and docs since -hex 32 produces 64 chars but validation expects 32.

Other comments (not applicable):

• SUPERSET_SECRET_KEY validation: Already has non-empty default thisISaSECRET_1234, so the empty-check would never trigger.
• secretPropertyName robustness: Current trim approach handles all actual value formats correctly (verified).
• external-secret.yaml whitespace: Renders valid YAML (verified with helm template).
• Merge renderEnv calls: Two separate calls for openopsEnv/openopsEnvSecrets is intentional for clarity.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

With the new openopsEnvSecrets convention, the shipped overlay/example files and docs that still instruct placing secrets under openopsEnv (e.g. values.production.yaml) are now misleading. Update those examples/docs to reference openopsEnvSecrets for secret inputs so users don’t accidentally render secrets as plain env values or omit them entirely.