Skip to content

refactor(helm): externalize chart credentials via SecretRef only (FLPATH-4806) - #66

Merged
chadcrum merged 6 commits into
dcm-project:mainfrom
chadcrum:flpath-4806-helm-externalize-secrets
Sep 21, 2026
Merged

chadcrum merged 6 commits into
dcm-project:mainfrom
chadcrum:flpath-4806-helm-externalize-secrets

Conversation

@chadcrum

@chadcrum chadcrum commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Require pre-created Kubernetes SecretRefs for Helm database and authentication credentials instead of inline values or chart-managed credential Secrets.
  • Update the bundled Keycloak realm import to use ${AUTH_PROXY_SECRET} and ${DCM_DEV_USER_PASSWORD} placeholders.
  • Document the required database, pull, auth, and kubeconfig Secrets and update schema verification cases.

Related PRs

Expected CI failure

The Helm chart job is expected to fail on this PR until the companion changes land. PR #60 updates deploy/keycloak/realm-export.json (the Compose source) but intentionally leaves deploy/helm/dcm/files/realm-export.json unchanged; this PR updates the Helm copy. While either PR is tested alone, helm-chart-verify-sync reports a stale or mismatched realm file. Once PRs #60 and #46 are merged, the Helm CI test is expected to pass.

Issue

https://redhat.atlassian.net/browse/FLPATH-4806

@chadcrum
chadcrum marked this pull request as ready for review September 13, 2026 18:31
@chadcrum
chadcrum requested a review from a team as a code owner September 13, 2026 18:31
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Require external SecretRefs for Helm chart credentials

✨ Enhancement ⚙️ Configuration changes 📝 Documentation 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Require pre-created SecretRefs for database, authentication, pull, and kubeconfig credentials.
• Import Keycloak realm credentials from external Secret-backed environment variables.
• Align chart documentation, schema validation, and template verification with external secrets.
Diagram

graph TD
  V["Helm Values"] --> H["Ref Validation"] --> D["Database Workloads"]
  H --> K["Keycloak"]
  H --> P["Service Providers"]
  S[("External Secrets")] --> D
  S --> K
  S --> P
  R["Realm Import"] --> K
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. External Secrets Operator integration
  • ➕ Automates synchronization from cloud or enterprise secret stores.
  • ➕ Supports centralized rotation without manually recreating Kubernetes Secrets.
  • ➖ Introduces CRDs, controller dependencies, and backend-specific configuration.
  • ➖ Reduces chart portability across clusters without the operator.
2. Secrets Store CSI Driver
  • ➕ Can mount credentials directly from external stores.
  • ➕ May avoid persisting secret material in Kubernetes Secrets.
  • ➖ Existing envFrom and secretKeyRef consumers require Secret synchronization or application changes.
  • ➖ Adds CSI provider dependencies and more complex workload configuration.

Recommendation: Keep the chart’s generic pre-created SecretRef contract. It cleanly separates credential lifecycle from Helm without imposing a secret-management platform; deployment automation can provision those Secrets through External Secrets Operator, CSI synchronization, or another approved mechanism.

Files changed (14) +141 / -272

Enhancement (1) +10 / -11
keycloak.yamlImport realm with Secret-backed placeholders +10/-11

Import realm with Secret-backed placeholders

• Starts Keycloak directly with realm import arguments, exposes the proxy secret alongside other auth variables, and mounts the realm file at Keycloak’s import path. Removes shell-based password substitution and chart-managed Secret checksum handling.

deploy/helm/dcm/templates/keycloak.yaml

Refactor (7) +48 / -110
_helpers.tplCentralize required database and auth Secret names +18/-6

Centralize required database and auth Secret names

• Adds a database SecretRef helper and changes authentication Secret resolution to fail when no external reference is configured. PostgreSQL readiness initialization now obtains its user from the database Secret.

deploy/helm/dcm/templates/_helpers.tpl

acm-cluster-service-provider.yamlRemove inline ACM access credentials +7/-39

Remove inline ACM access credentials

• Requires an external pull Secret and removes chart-generated pull and kubeconfig Secrets. External kubeconfig references now control volume mounting, while omission retains in-cluster ServiceAccount authentication.

deploy/helm/dcm/templates/acm-cluster-service-provider.yaml

control-plane.yamlReference the external database Secret +1/-4

Reference the external database Secret

• Points control-plane database environment loading at the configured database SecretRef. Removes rollout checksums tied to the deleted chart-managed authentication Secret.

deploy/helm/dcm/templates/control-plane.yaml

k8s-container-service-provider.yamlMount referenced Kubernetes provider kubeconfig +6/-17

Mount referenced Kubernetes provider kubeconfig

• Replaces inline kubeconfig Secret generation with an optional pre-existing Secret mount. The provider continues using chart-created ServiceAccount RBAC when no reference is supplied.

deploy/helm/dcm/templates/k8s-container-service-provider.yaml

kubevirt-service-provider.yamlReplace inline KubeVirt kubeconfig +4/-16

Replace inline KubeVirt kubeconfig

• Removes generated kubeconfig Secrets and mounts an optional pre-existing Secret identified by kubeconfigRef.

deploy/helm/dcm/templates/kubevirt-service-provider.yaml

postgres.yamlLoad PostgreSQL credentials from SecretRef +7/-11

Load PostgreSQL credentials from SecretRef

• Loads PostgreSQL credentials from the configured external database Secret. Readiness and liveness probes now use POSTGRES_USER from that Secret instead of an inline Helm value.

deploy/helm/dcm/templates/postgres.yaml

three-tier-demo-service-provider.yamlExternalize demo provider credentials +5/-17

Externalize demo provider credentials

• Uses the shared external database Secret and replaces inline kubeconfig generation with an optional referenced Secret mount.

deploy/helm/dcm/templates/three-tier-demo-service-provider.yaml

Tests (2) +22 / -10
verify-schema.shValidate required external SecretRefs +10/-6

Validate required external SecretRefs

• Updates positive and negative lint cases for the required database and authentication SecretRefs. Removes coverage for the retired inline authentication credential path.

deploy/helm/dcm/scripts/verify-schema.sh

verify-template.shVerify templates never render credential Secrets +12/-4

Verify templates never render credential Secrets

• Checks that database and authentication credentials remain externally referenced and that all expected secretKeyRef entries are rendered. Adds validation for a missing ACM pull Secret reference.

deploy/helm/dcm/scripts/verify-template.sh

Documentation (1) +43 / -71
README.mdDocument pre-created credential Secrets +43/-71

Document pre-created credential Secrets

• Replaces inline credential examples with commands for creating database, pull, authentication, and kubeconfig Secrets. Documents SecretRef defaults, rotation behavior, and the externally supplied Keycloak realm credentials.

deploy/helm/dcm/README.md

Other (3) +18 / -70
realm-export.jsonParameterize Keycloak realm credentials +2/-2

Parameterize Keycloak realm credentials

• Replaces the embedded proxy client secret and development-user password marker with Keycloak environment-variable placeholders sourced from the authentication Secret.

deploy/helm/dcm/files/realm-export.json

values.schema.jsonRestrict credential configuration to SecretRefs +8/-52

Restrict credential configuration to SecretRefs

• Removes inline database, authentication, pull-secret, and kubeconfig fields from the chart schema. Requires a database SecretRef globally and an authentication SecretRef whenever authentication is enabled.

deploy/helm/dcm/values.schema.json

values.yamlReplace inline credentials with SecretRef defaults +8/-16

Replace inline credentials with SecretRef defaults

• Removes embedded database and authentication credentials and replaces provider kubeconfig values with reference fields. Adds default database, authentication, and ACM pull Secret names and updates the Keycloak image to 26.0.1.

deploy/helm/dcm/values.yaml

@qodo-code-review

qodo-code-review Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Chart checks fail before installation ✗ Dismissed 🐞 Bug ☼ Reliability
Description
files/realm-export.json changes the client-secret field while the canonical
deploy/keycloak/realm-export.json remains different, despite the chart sync verifier requiring
byte-for-byte equality. Every Helm validation target depends on that verifier, so the Helm CI
workflow exits before schema, lint, or template tests run.
Code

deploy/helm/dcm/files/realm-export.json[15]

+      "secret": "${AUTH_PROXY_SECRET}",
Evidence
The canonical realm source still contains the old literal client secret, whereas the changed Helm
copy contains a placeholder. The chart Make targets define the source and destination and make
helm-chart-check depend on a cmp -s verification; the Helm workflow invokes that check.

deploy/keycloak/realm-export.json[9-16]
make/helm.mk[6-16]
make/helm.mk[27-41]
.github/workflows/helm.yaml[26-26]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The bundled Helm realm export no longer matches its canonical source. `make helm-chart-verify-sync` compares these files byte-for-byte, and the Helm workflow runs `make helm-chart-check`, so this branch cannot pass chart validation until both copies contain the same realm definition.

## Fix Focus Areas
- deploy/helm/dcm/files/realm-export.json[15-15]
- deploy/keycloak/realm-export.json[9-15]

## Recommended Fix
Apply the same placeholder-based realm changes to `deploy/keycloak/realm-export.json`, then regenerate or copy the bundled Helm file with `make helm-chart-sync`. If the canonical-source change must land in another PR, remove the Helm-copy change from this PR and merge the changes only once both files can be updated together.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Schema consumers see stale defaults ✗ Dismissed 📘 Rule violation ≡ Correctness
Description
dbSecretRef lacks its dcm-db default, while authSecretRef and pullSecretRef retain
empty-string defaults and the updated Keycloak image has no schema default. Tools that inspect
values.schema.json therefore receive values different from the defaults used by installations
through values.yaml.
Code

deploy/helm/dcm/values.yaml[43]

+  authSecretRef: dcm-auth  # pre-created Secret when auth.enabled=true (see README)
Evidence
Rule 2949318 requires every changed non-null Helm value default to be mirrored exactly in the
schema. The changed values use dcm-db, dcm-auth, dcm-acm-pull-secret, and Keycloak image
26.0.1, while the cited schema nodes omit those defaults or still declare an empty string.

Rule 2949318: Keep Helm values.schema.json in sync with values.yaml structure, types, and defaults
deploy/helm/dcm/values.yaml[8-9]
deploy/helm/dcm/values.yaml[40-47]
deploy/helm/dcm/values.yaml[99-109]
deploy/helm/dcm/values.schema.json[117-141]
deploy/helm/dcm/values.schema.json[201-209]
deploy/helm/dcm/values.schema.json[266-269]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Several defaults changed or were introduced in `values.yaml` without matching defaults in `values.schema.json`.

## Fix Focus Areas
- deploy/helm/dcm/values.schema.json[117-141]
- deploy/helm/dcm/values.schema.json[201-209]
- deploy/helm/dcm/values.schema.json[266-269]

## Recommended Fix
Add schema defaults matching `values.yaml`: `dcm-db` for the database Secret reference, `dcm-auth` for the authentication Secret reference, `dcm-acm-pull-secret` for the pull Secret reference, and `quay.io/keycloak/keycloak:26.0.1` for the Keycloak image.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. External kubeconfig paths go untested ✗ Dismissed 📘 Rule violation ▣ Testability
Description
verify-template.sh adds checks for external database and authentication Secrets and the ACM pull
Secret, but never renders any provider with a non-empty kubeconfigRef. Regressions in the newly
changed volume, mount, environment-variable, and Secret-name branches for the ACM, Kubernetes,
KubeVirt, and three-tier providers can therefore pass template verification.
Code

deploy/helm/dcm/scripts/verify-template.sh[R76-83]

+db_ref_out="$(helm_out)"
+if printf '%s' "$db_ref_out" | awk 'BEGIN{RS="---"} /kind: Secret/ && /name: dcm-db/ && /stringData/ {found=1} END{exit !found}'; then
+	fail "chart db Secret must not render; use postgres.dbSecretRef"
+fi
+db_ref_count="$(printf '%s' "$db_ref_out" | grep -c 'name: dcm-db')"
+[ "$db_ref_count" -ge 2 ] || fail "workloads must reference postgres.dbSecretRef (found $db_ref_count)"
+
+require_template_failure "acm without pullSecretRef" "acmClusterServiceProvider.pullSecretRef is required when enabled" --set acmClusterServiceProvider.enabled=true --set acmClusterServiceProvider.pullSecretRef=
Evidence
Rule 2949358 requires verification coverage for every changed render-time branch. The provider
templates now branch on kubeconfigRef, while the updated verification script only checks
database/auth references and the missing ACM pull reference.

Rule 2949358: Keep template verification script in sync with render-time behavior changes
deploy/helm/dcm/scripts/verify-template.sh[69-83]
deploy/helm/dcm/templates/acm-cluster-service-provider.yaml[134-148]
deploy/helm/dcm/templates/k8s-container-service-provider.yaml[82-98]
deploy/helm/dcm/templates/kubevirt-service-provider.yaml[33-47]
deploy/helm/dcm/templates/three-tier-demo-service-provider.yaml[51-65]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The template verification changes do not exercise the new Secret-reference kubeconfig branches across the affected service providers.

## Fix Focus Areas
- deploy/helm/dcm/scripts/verify-template.sh[76-83]
- deploy/helm/dcm/templates/acm-cluster-service-provider.yaml[134-148]
- deploy/helm/dcm/templates/k8s-container-service-provider.yaml[82-98]
- deploy/helm/dcm/templates/kubevirt-service-provider.yaml[33-47]
- deploy/helm/dcm/templates/three-tier-demo-service-provider.yaml[51-65]

## Recommended Fix
Render each affected provider with a distinct non-empty `kubeconfigRef`, then assert that the expected environment variable, volume mount, and Secret name appear and that no chart-managed kubeconfig Secret is rendered.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Two kubeconfig hints are overlong ✗ Dismissed 📘 Rule violation ⚙ Maintainability
Description
The end-of-line comments on the Kubernetes and ACM provider kubeconfigRef values combine
optionality with in-cluster authentication behavior in clauses longer than the approximate limit.
Their detailed behavior is compressed beside editable values instead of being placed in preceding
comments or the schema documentation.
Code

deploy/helm/dcm/values.yaml[97]

+  kubeconfigRef: ""  # optional; omit for in-cluster ServiceAccount auth with chart RBAC
Evidence
Rule 2949335 limits end-of-line comments to short, single-clause edit-time hints. The added comments
at lines 97 and 109 each use a semicolon to combine two explanations and exceed the approximate
phrase-length threshold.

Rule 2949335: Restrict end-of-line comments in values.yaml to short edit-time hints only
deploy/helm/dcm/values.yaml[97-97]
deploy/helm/dcm/values.yaml[109-109]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Two newly added `kubeconfigRef` end-of-line comments exceed the phrase-length convention and contain multiple pieces of behavioral guidance.

## Fix Focus Areas
- deploy/helm/dcm/values.yaml[97-97]
- deploy/helm/dcm/values.yaml[109-109]

## Recommended Fix
Keep only a short edit-time hint beside each value, such as `# optional external kubeconfig Secret`, and move the in-cluster ServiceAccount behavior to a preceding standalone comment or the existing schema description.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials
✅ Compliance rules (platform): 17 rules
Review mode: 🧠 Deep: This is a security-sensitive Helm credential refactor spanning many templates, schema rules, SecretRefs, and Keycloak initialization paths, with numerous independent behavior changes likely to harbor subtle defects.

Grey Divider

Tip of the day
💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread deploy/helm/dcm/values.yaml
Comment thread deploy/helm/dcm/scripts/verify-template.sh
Comment thread deploy/helm/dcm/values.yaml Outdated
Comment thread deploy/helm/dcm/files/realm-export.json
Comment thread deploy/helm/dcm/README.md
# External kubeconfig mode (pre-existing Secret):
kubectl create secret generic my-kubeconfig-secret \
--from-file=kubeconfig=/path/to/kubeconfig
helm upgrade dcm deploy/helm/dcm --reuse-values \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are old keys (postgres.user, auth.proxySecret, inline kubeconfig/pullSecret) exists helm upgrade --reuse-values on an existing install will fail or attach the wrong Secret.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't had a real release yet, so there shouldn't be existing installs that need an upgrade migration. Given that, are you okay with us proceeding as-is, or do you have another approach you'd prefer?

@LinskId LinskId left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after resolving comments

Comment thread deploy/helm/dcm/README.md Outdated
chadcrum and others added 5 commits September 14, 2026 19:02
…ATH-4806)

Remove inline DB and auth credentials from values/schema and delete
chart-managed secret templates. Require pre-created Kubernetes Secrets,
update Keycloak realm import for env placeholders, and align verify scripts.

Compose realm source intentionally unchanged; pairs with compose split PR.

https: //redhat.atlassian.net/browse/FLPATH-4806
Signed-off-by: Chad Crum <ccrum@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Chad Crum <ccrum@redhat.com>
Signed-off-by: Chad Crum <ccrum@redhat.com>
Signed-off-by: Chad Crum <ccrum@redhat.com>
Signed-off-by: Chad Crum <ccrum@redhat.com>
Comment thread deploy/helm/dcm/values.schema.json
Comment thread deploy/helm/dcm/README.md
Signed-off-by: Chad Crum <ccrum@redhat.com>
@chadcrum
chadcrum force-pushed the flpath-4806-helm-externalize-secrets branch from 89dd2b1 to f870d32 Compare September 16, 2026 21:14

@gabriel-farache gabriel-farache left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jordigilh jordigilh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion

The current checks are green. One compatibility consideration is worth documenting before merge: this change removes several chart values and Secret templates, which could affect helm upgrade --reuse-values if existing deployments use those keys. I am not assuming that such deployments exist.

If this chart is intentionally greenfield-only, please state that explicitly. If existing deployments are supported, consider adding a migration note or an upgrade test covering the removed values. This is a suggestion, not a confirmed blocker.

Current review position: No blocking finding from this suggestion.

@chadcrum
chadcrum merged commit 5fee57f into dcm-project:main Sep 21, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants