Skip to content

feat(helm): support existing token Secret (SOPS) and scope token RBAC to namespace - #497

Merged
sandipanpanda merged 1 commit into
mainfrom
zxp-chart-sp
Jul 10, 2026
Merged

feat(helm): support existing token Secret (SOPS) and scope token RBAC to namespace#497
sandipanpanda merged 1 commit into
mainfrom
zxp-chart-sp

Conversation

@sandipanpanda

@sandipanpanda sandipanpanda commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary by Gitar

  • Helm configuration:
    • Added support for existingSecret to source credentials from pre-managed Secrets, enabling SOPS/GitOps workflows.
    • Added zxporter.validateConfig logic to prevent naming collisions with the internal tokenSecretName.
  • Documentation:
    • Added docs/existing-secret-sops.md guide for using SOPS-encrypted secrets with the chart.
  • Deployment:
    • Updated deployment.yaml to inject CLUSTER_TOKEN and PAT_TOKEN from external secrets when existingSecret is configured.
  • RBAC cleanup:
    • Removed zxporter-rbac.yaml content, shifting to namespace-scoped token management.

This will update automatically on new commits.

 namespace

- add zxporter.existingSecret to read the token from a user-managed Secret
  instead of values; chart skips creating its own credentials Secret
- guard against existingSecret.name colliding with tokenSecretName
- drop redundant cluster-wide Secret grant (namespace Role already
covers it)
- docs: docs/existing-secret-sops.md
Comment on lines +55 to 57
{{- if and (empty .Values.zxporter.clusterToken) (empty .Values.zxporter.patToken) (empty (dig "existingSecret" "name" "" .Values.zxporter)) -}}
{{- fail "ERROR: A token must be provided. Set one of zxporter.clusterToken, zxporter.patToken, or zxporter.existingSecret.name (a pre-existing Secret holding the token)." -}}
{{- end -}}

@gitar-bot gitar-bot Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Edge Case: existingSecret satisfies validation but is ignored when useSecretForToken=false

zxporter.validateConfig now treats a non-empty existingSecret.name as satisfying the "a token is required" check, regardless of useSecretForToken.

However, existingSecret is only consumed by the deployment token block, which is entirely gated by {{- if .Values.zxporter.useSecretForToken }} (deployment.yaml:62). When useSecretForToken=false, tokens instead come from the ConfigMap (configmap.yaml:4-7), which only reads clusterToken/patToken — it never reads existingSecret.

So a user who sets existingSecret.name but has useSecretForToken=false (and leaves clusterToken/patToken empty) will pass helm install validation, but the rendered ConfigMap will have empty CLUSTER_TOKEN/PAT_TOKEN and no env vars are injected. The controller starts with no token and fails at runtime ("no URL or token was configured") — the exact silent failure the validation is meant to prevent.

Suggest making the validation reflect the actual usage: only accept existingSecret.name as a valid token source when useSecretForToken is true, or fail fast if existingSecret.name is set while useSecretForToken=false.

Only accept existingSecret.name when useSecretForToken is true, and fail fast on the incompatible combination.:

{{- $existingName := dig "existingSecret" "name" "" .Values.zxporter -}}
{{- $existingUsable := and $existingName .Values.zxporter.useSecretForToken -}}
{{- if and (empty .Values.zxporter.clusterToken) (empty .Values.zxporter.patToken) (not $existingUsable) -}}
  {{- fail "ERROR: A token must be provided. Set zxporter.clusterToken, zxporter.patToken, or (with useSecretForToken=true) zxporter.existingSecret.name." -}}
{{- end -}}
{{- if and $existingName (not .Values.zxporter.useSecretForToken) -}}
  {{- fail "ERROR: zxporter.existingSecret.name only works with useSecretForToken=true." -}}
{{- end -}}

Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown
Code Review ⚠️ Changes requested 0 resolved / 1 findings

Implements support for existing SOPS-encrypted secrets and scopes RBAC to the local namespace. The configuration is currently fragile, as existingSecret is ignored when useSecretForToken is disabled.

⚠️ Edge Case: existingSecret satisfies validation but is ignored when useSecretForToken=false

📄 helm-chart/zxporter/templates/_helpers.tpl:55-57 📄 helm-chart/zxporter/templates/deployment.yaml:62-64

zxporter.validateConfig now treats a non-empty existingSecret.name as satisfying the "a token is required" check, regardless of useSecretForToken.

However, existingSecret is only consumed by the deployment token block, which is entirely gated by {{- if .Values.zxporter.useSecretForToken }} (deployment.yaml:62). When useSecretForToken=false, tokens instead come from the ConfigMap (configmap.yaml:4-7), which only reads clusterToken/patToken — it never reads existingSecret.

So a user who sets existingSecret.name but has useSecretForToken=false (and leaves clusterToken/patToken empty) will pass helm install validation, but the rendered ConfigMap will have empty CLUSTER_TOKEN/PAT_TOKEN and no env vars are injected. The controller starts with no token and fails at runtime ("no URL or token was configured") — the exact silent failure the validation is meant to prevent.

Suggest making the validation reflect the actual usage: only accept existingSecret.name as a valid token source when useSecretForToken is true, or fail fast if existingSecret.name is set while useSecretForToken=false.

Only accept existingSecret.name when useSecretForToken is true, and fail fast on the incompatible combination.
{{- $existingName := dig "existingSecret" "name" "" .Values.zxporter -}}
{{- $existingUsable := and $existingName .Values.zxporter.useSecretForToken -}}
{{- if and (empty .Values.zxporter.clusterToken) (empty .Values.zxporter.patToken) (not $existingUsable) -}}
  {{- fail "ERROR: A token must be provided. Set zxporter.clusterToken, zxporter.patToken, or (with useSecretForToken=true) zxporter.existingSecret.name." -}}
{{- end -}}
{{- if and $existingName (not .Values.zxporter.useSecretForToken) -}}
  {{- fail "ERROR: zxporter.existingSecret.name only works with useSecretForToken=true." -}}
{{- end -}}
🤖 Prompt for agents
Code Review: Implements support for existing SOPS-encrypted secrets and scopes RBAC to the local namespace. The configuration is currently fragile, as existingSecret is ignored when useSecretForToken is disabled.

1. ⚠️ Edge Case: existingSecret satisfies validation but is ignored when useSecretForToken=false
   Files: helm-chart/zxporter/templates/_helpers.tpl:55-57, helm-chart/zxporter/templates/deployment.yaml:62-64

   `zxporter.validateConfig` now treats a non-empty `existingSecret.name` as satisfying the "a token is required" check, regardless of `useSecretForToken`.
   
   However, `existingSecret` is only consumed by the deployment token block, which is entirely gated by `{{- if .Values.zxporter.useSecretForToken }}` (deployment.yaml:62). When `useSecretForToken=false`, tokens instead come from the ConfigMap (configmap.yaml:4-7), which only reads `clusterToken`/`patToken` — it never reads `existingSecret`.
   
   So a user who sets `existingSecret.name` but has `useSecretForToken=false` (and leaves clusterToken/patToken empty) will pass `helm install` validation, but the rendered ConfigMap will have empty `CLUSTER_TOKEN`/`PAT_TOKEN` and no env vars are injected. The controller starts with no token and fails at runtime ("no URL or token was configured") — the exact silent failure the validation is meant to prevent.
   
   Suggest making the validation reflect the actual usage: only accept `existingSecret.name` as a valid token source when `useSecretForToken` is true, or fail fast if `existingSecret.name` is set while `useSecretForToken=false`.

   Fix (Only accept existingSecret.name when useSecretForToken is true, and fail fast on the incompatible combination.):
   {{- $existingName := dig "existingSecret" "name" "" .Values.zxporter -}}
   {{- $existingUsable := and $existingName .Values.zxporter.useSecretForToken -}}
   {{- if and (empty .Values.zxporter.clusterToken) (empty .Values.zxporter.patToken) (not $existingUsable) -}}
     {{- fail "ERROR: A token must be provided. Set zxporter.clusterToken, zxporter.patToken, or (with useSecretForToken=true) zxporter.existingSecret.name." -}}
   {{- end -}}
   {{- if and $existingName (not .Values.zxporter.useSecretForToken) -}}
     {{- fail "ERROR: zxporter.existingSecret.name only works with useSecretForToken=true." -}}
   {{- end -}}

Was this helpful? React with 👍 / 👎 | Gitar

@sandipanpanda
sandipanpanda merged commit 415806a into main Jul 10, 2026
21 checks passed
@sandipanpanda
sandipanpanda deleted the zxp-chart-sp branch July 10, 2026 10:36
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.

2 participants