-
Notifications
You must be signed in to change notification settings - Fork 501
[Feat] Enable redis password in helm chart #1586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feat] Enable redis password in helm chart #1586
Conversation
Summary of ChangesHello @lx1036, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the security of the application's Redis integration within the Kubernetes environment. It introduces a robust mechanism to enable and manage Redis passwords via the Helm chart, ensuring that all dependent services can securely authenticate with Redis using credentials stored in a Kubernetes Secret. This change aligns with best practices for securing sensitive data in a production cluster. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable security enhancement by enabling password protection for Redis within the Helm chart. The implementation correctly propagates the Redis password to various components. My review includes suggestions to further improve security by changing how the password is managed in values.yaml and generated in the secret. I've also pointed out a couple of minor issues in the Helm templates to improve maintainability and correctness, such as fixing a cross-component value reference and an indentation error.
| {{- if .Values.metadata.redis.enablePassword }} | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: aibrix-redis | ||
| namespace: {{ .Release.Namespace }} | ||
| type: Opaque | ||
| data: | ||
| redis-password: {{ .Values.metadata.redis.password }} | ||
| {{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve security and user experience, it's better to handle password encoding within the template. This change will take a plaintext password from values.yaml, base64 encode it, and fail the deployment if enablePassword is true but no password is provided. This complements the change I suggested for values.yaml.
{{- if .Values.metadata.redis.enablePassword }}
{{- if not .Values.metadata.redis.password -}}
{{- fail "A password is required when .Values.metadata.redis.enablePassword is true" -}}
{{- end }}
apiVersion: v1
kind: Secret
metadata:
name: aibrix-redis
namespace: {{ .Release.Namespace }}
type: Opaque
data:
redis-password: {{ .Values.metadata.redis.password | b64enc }}
{{- end }}
dist/chart/values.yaml
Outdated
| enablePassword: false | ||
| password: "YTFiMmMz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing a default, weak, base64-encoded password in values.yaml is a security risk. It's better to leave this empty and require the user to provide a strong password when enablePassword is set to true. Also, for better user experience, the password should be stored in plaintext in values.yaml and be base64 encoded within the Secret template. I'll leave a corresponding suggestion on dist/chart/templates/metadata-service/redis.yaml.
enablePassword: false
# When enablePassword is true, a plaintext password must be provided.
# It will be Base64 encoded automatically.
password: ""| value: "{{ .Values.gatewayPlugin.dependencies.redis.port }}" | ||
| {{- if .Values.metadata.redis.enablePassword }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of issues here:
- The
REDIS_PORTfor the GPU optimizer should be sourced from its own configuration section invalues.yaml(.Values.gpuOptimizer.dependencies.redis.port) instead of from thegatewayPluginsection. This coupling makes the chart less modular and harder to maintain. - The indentation for the
ifblock on line 37 is incorrect. It should be aligned with the- name:blocks (12 spaces) for consistency and readability.
value: "{{ .Values.gpuOptimizer.dependencies.redis.port }}"
{{- if .Values.metadata.redis.enablePassword }}96b9ffe to
6744184
Compare
|
can you fix the DCO with |
| value: "{{ $val }}" | ||
| {{- end }} | ||
| resources: {{ toYaml .Values.gatewayPlugin.resources | nindent 12 }} | ||
| resources: {{ toYaml .Values.gatewayPlugin.container.resources | nindent 12 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. thanks
googs1025
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good improvement. Thanks.
plz fix DCO. other LGTM
Signed-off-by: LiuXiang <[email protected]>
6744184 to
23bb205
Compare
Has fixed DCO. |
|
final ack for @Jeffwan |
Signed-off-by: LiuXiang <[email protected]> Signed-off-by: ChethanUK <[email protected]>
Pull Request Description
In our k8s cluster, install components like redis, mysql and so on, it must enable password for security.