manifests: load REDIS_PASSWORD from Secret via secretKeyRef#372
Conversation
|
@kubeboiii: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Welcome @kubeboiii! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR moves Redis password configuration in the Helm chart from plain-text env values to Kubernetes Secrets with support for referencing a pre-existing Secret, and updates docs accordingly.
Changes:
- Inject
REDIS_PASSWORDviasecretKeyRefinstead of a plainvalue. - Add an optional chart-managed Redis Secret and helpers to derive Secret name/key.
- Document chart-managed vs. existing Secret setup in getting started guide.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| manifests/charts/base/values.yaml | Adds values for using an existing Redis Secret and configuring the password key name. |
| manifests/charts/base/templates/workloadmanager.yaml | Switches REDIS_PASSWORD to valueFrom.secretKeyRef. |
| manifests/charts/base/templates/agentcube-router.yaml | Switches REDIS_PASSWORD to valueFrom.secretKeyRef. |
| manifests/charts/base/templates/redis-secret.yaml | Adds chart-managed Redis password Secret when existingSecret is unset. |
| manifests/charts/base/templates/_helpers.tpl | Adds helper templates for Redis Secret name and password key. |
| docs/getting-started.md | Documents new Secret-based configuration and new Helm values. |
| @@ -0,0 +1,13 @@ | |||
| {{- if not .Values.redis.existingSecret }} | |||
| stringData: | ||
| {{ include "agentcube.redis.passwordKey" . }}: {{ .Values.redis.password | quote }} | ||
| {{- end }} |
| {{- define "agentcube.redis.secretName" -}} | ||
| {{- if .Values.redis.existingSecret }} | ||
| {{- .Values.redis.existingSecret }} | ||
| {{- else }} | ||
| {{- printf "%s-redis" .Release.Name }} | ||
| {{- end }} | ||
| {{- end }} |
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "agentcube.redis.secretName" . }} | ||
| key: {{ include "agentcube.redis.passwordKey" . }} |
| --set router.serviceAccountName="agentcube-router" | ||
| ``` | ||
|
|
||
| The Redis password is stored in a Kubernetes Secret and injected via `secretKeyRef` (not plain text in Deployment manifests). With the command above, Helm creates a chart-managed Secret from `redis.password`. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves security by injecting the Redis password via a Kubernetes Secret using secretKeyRef instead of plain text in the deployment manifests. It adds support for both chart-managed secrets and referencing an existing external secret, updating the documentation, helper templates, and deployment configurations accordingly. The review feedback suggests avoiding the creation of an empty Secret when no password is provided, and conditionally injecting the REDIS_PASSWORD environment variable in the router and workload manager deployments to prevent failures in environments where a password is not required.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @@ -0,0 +1,13 @@ | |||
| {{- if not .Values.redis.existingSecret }} | |||
There was a problem hiding this comment.
To avoid creating an empty Secret when no Redis password is provided (e.g., in local development or test environments where REDIS_PASSWORD_REQUIRED=false is used), we should only create this Secret if redis.password is actually set.
{{- if and (not .Values.redis.existingSecret) .Values.redis.password }}| - name: REDIS_PASSWORD | ||
| value: {{ .Values.redis.password | quote }} | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "agentcube.redis.secretName" . }} | ||
| key: {{ include "agentcube.redis.passwordKey" . }} |
There was a problem hiding this comment.
To prevent the deployment from failing when no Redis password is provided (and thus no Secret is created), we should conditionally inject the REDIS_PASSWORD environment variable only when either redis.password or redis.existingSecret is configured.
{{- if or .Values.redis.password .Values.redis.existingSecret }}
- name: REDIS_PASSWORD
valueFrom:
secretKeyRef:
name: {{ include "agentcube.redis.secretName" . }}
key: {{ include "agentcube.redis.passwordKey" . }}
{{- end }}| - name: REDIS_PASSWORD | ||
| value: {{ .Values.redis.password | quote }} | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "agentcube.redis.secretName" . }} | ||
| key: {{ include "agentcube.redis.passwordKey" . }} |
There was a problem hiding this comment.
To prevent the deployment from failing when no Redis password is provided (and thus no Secret is created), we should conditionally inject the REDIS_PASSWORD environment variable only when either redis.password or redis.existingSecret is configured.
{{- if or .Values.redis.password .Values.redis.existingSecret }}
- name: REDIS_PASSWORD
valueFrom:
secretKeyRef:
name: {{ include "agentcube.redis.secretName" . }}
key: {{ include "agentcube.redis.passwordKey" . }}
{{- end }}
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
===========================================
+ Coverage 47.57% 57.98% +10.41%
===========================================
Files 30 34 +4
Lines 2819 3180 +361
===========================================
+ Hits 1341 1844 +503
+ Misses 1338 1151 -187
- Partials 140 185 +45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Store Redis credentials in a Kubernetes Secret and inject them with secretKeyRef in workloadmanager and agentcube-router Deployments. Add redis.existingSecret for production installs. Skip Secret and REDIS_PASSWORD env when no password is configured. Signed-off-by: Himanshu <144804569+kubeboiii@users.noreply.github.com>
kubeboiii
force-pushed
the
fix/redis-password-secret-ref
branch
from
e953f3e to
dfde24c
Compare
| # For production, prefer creating a Secret yourself and set existingSecret instead. | ||
| password: "" | ||
| # Name of an existing Secret containing the Redis password (skips chart Secret creation). | ||
| existingSecret: "" |
There was a problem hiding this comment.
Renamed to redis.secretName — clearer than existingSecret. When set, the chart does not create {release}-redis and both Deployments use that Secret via secretKeyRef.
| # Name of an existing Secret containing the Redis password (skips chart Secret creation). | ||
| existingSecret: "" | ||
| # Key within existingSecret or chart-managed Secret (default: password). | ||
| existingSecretPasswordKey: "password" |
There was a problem hiding this comment.
Renamed to redis.secretKey (default password). Used directly in templates with | default "password" instead of a helper.
There was a problem hiding this comment.
Removed _helpers.tpl and inlined the logic in the three templates as suggested.
| {{/* | ||
| Key within the Redis Secret that holds the password. | ||
| */}} | ||
| {{- define "agentcube.redis.passwordKey" -}} |
There was a problem hiding this comment.
I think redefining another var makes it harder to understand
There was a problem hiding this comment.
Dropped the passwordKey helper; templates now use .Values.redis.secretKey | default "password" directly.
Rename redis.existingSecret to redis.secretName and redis.existingSecretPasswordKey to redis.secretKey. Inline secret name/key in templates and remove _helpers.tpl. Signed-off-by: Himanshu <144804569+kubeboiii@users.noreply.github.com>
Document mutual exclusivity of redis.password vs redis.secretName, add upgrade note, and remove invalid router.rbac.create from examples. Signed-off-by: Himanshu <144804569+kubeboiii@users.noreply.github.com>
|
Thanks @hzxuzhonghu for the review — addressed in the latest commits:
Also added a short note in |
| {{- if and (not .Values.redis.secretName) .Values.redis.password }} | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: {{ printf "%s-redis" .Release.Name }} | ||
| namespace: {{ .Release.Namespace }} | ||
| labels: | ||
| app.kubernetes.io/name: agentcube | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| type: Opaque | ||
| stringData: | ||
| {{ .Values.redis.secretKey | default "password" }}: {{ .Values.redis.password | quote }} | ||
| {{- end }} |
| {{- if or .Values.redis.password .Values.redis.secretName }} | ||
| - name: REDIS_PASSWORD | ||
| value: {{ .Values.redis.password | quote }} | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.redis.secretName | default (printf "%s-redis" .Release.Name) | quote }} | ||
| key: {{ .Values.redis.secretKey | default "password" | quote }} | ||
| {{- end }} |
| --create-namespace \ | ||
| --set redis.addr="redis.agentcube.svc.cluster.local:6379" \ | ||
| --set redis.password="''''" \ | ||
| --set router.rbac.create=true \ | ||
| --set router.serviceAccountName="agentcube-router" |
| helm install agentcube ./manifests/charts/base \ | ||
| --namespace agentcube \ | ||
| --create-namespace \ | ||
| --set redis.addr="redis.agentcube.svc.cluster.local:6379" \ | ||
| --set redis.secretName="agentcube-redis" \ | ||
| --set router.serviceAccountName="agentcube-router" |
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
I think we can also update the e2e with password
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
Security fix for Helm chart credential handling.
What this PR does / why we need it:
The Helm chart embedded the Redis password directly in Deployment env
valuefields for workloadmanager and agentcube-router. Anyone with read access to Deployments in the namespace could read the credential fromkubectl get deployment -o yaml.This PR stores the password in a Kubernetes Secret and references it with
secretKeyRefforREDIS_PASSWORDin both components. Whenredis.secretNameis unset andredis.passwordis set, the chart creates a Secret named{release}-redis. Whenredis.secretNameis set, the chart skips Secret creation and uses the provided Secret name and key. When neither is set, no Secret orREDIS_PASSWORDenv is rendered (supports e2e and dev installs withREDIS_PASSWORD_REQUIRED=false).No changes to application code. Components continue to read
REDIS_PASSWORDfrom the environment.Which issue(s) this PR fixes:
Fixes #368
Special notes for your reviewer:
manifests/charts/base/plusdocs/getting-started.md.helm install --set redis.password=...still works; the value is written to a Secret instead of the Deployment manifest.redis.secretName,redis.secretKey(default key:password). Use eitherredis.passwordorredis.secretName, not both.--set redis.password=""withREDIS_PASSWORD_REQUIRED=falseare unchanged in behavior.--set redis.passwordstill places the value in Helm release metadata. Production installs should prefer a pre-created Secret viaredis.secretName(documented in getting-started).Does this PR introduce a user-facing change?:
Yes. Helm installs now create or reference a Redis Secret for credentials instead of inlining the password in Deployment specs.