-
Notifications
You must be signed in to change notification settings - Fork 31
feat: Allow Redis password to be loaded from a file, enabling Kubernetes secret integration #117
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
base: main
Are you sure you want to change the base?
Conversation
…tes secret integration Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @yashisrani, 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 enhances the security and operational flexibility for managing Redis passwords within the application. By introducing the capability to load the Redis password from a file, it enables seamless integration with Kubernetes secrets, allowing sensitive credentials to be managed more securely and compliantly. The changes involve updates to both the application's core Redis connection logic and its Kubernetes deployment configuration to adopt this new, more secure method. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 useful feature to load the Redis password from a file, which is great for Kubernetes integration with secrets. The implementation is mostly solid. I've pointed out a security concern with a hardcoded password in the example Kubernetes manifest, which should be addressed or at least documented for users. I also suggested a small improvement in the Go code to make the password trimming from the file more robust. Overall, good work.
| stringData: | ||
| password: "SuperSecretRedisPassword" |
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.
The Redis password is hardcoded in this Secret definition. While this may be convenient for local development or demonstration, it poses a significant security risk if used in production environments. It's strongly recommended to not check in plaintext secrets into version control. For production, secrets should be created out-of-band (e.g., via kubectl create secret or a CI/CD pipeline) or managed by a secrets management system like HashiCorp Vault. Consider adding a comment here to warn users about this.
pkg/store/store_redis.go
Outdated
| // Trim possible whitespace/newlines from the file content | ||
| redisPassword = string(data) | ||
| // Handle cases where the file might have trailing newlines | ||
| if len(redisPassword) > 0 && redisPassword[len(redisPassword)-1] == '\n' { | ||
| redisPassword = redisPassword[:len(redisPassword)-1] | ||
| } |
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.
The current implementation for trimming the password read from the file only handles a single trailing newline (\n). This is not robust enough as the file could contain other whitespace (like spaces, tabs, or carriage returns \r), or could have leading whitespace. Using strings.TrimSpace is a better approach as it handles all leading and trailing whitespace characters, making the password retrieval more reliable.
Note: You'll need to add "strings" to your imports for this change.
| // Trim possible whitespace/newlines from the file content | |
| redisPassword = string(data) | |
| // Handle cases where the file might have trailing newlines | |
| if len(redisPassword) > 0 && redisPassword[len(redisPassword)-1] == '\n' { | |
| redisPassword = redisPassword[:len(redisPassword)-1] | |
| } | |
| // Trim possible whitespace/newlines from the file content | |
| redisPassword = strings.TrimSpace(string(data)) |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
=======================================
Coverage ? 29.68%
=======================================
Files ? 29
Lines ? 2749
Branches ? 0
=======================================
Hits ? 816
Misses ? 1799
Partials ? 134
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:
|
k8s/agentcube-router.yaml
Outdated
| - name: REDIS_PASSWORD | ||
| value: "" | ||
| - name: REDIS_PASSWORD_FILE | ||
| value: "/etc/agentcube/secrets/password" |
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.
Our intention is to use downward api FYI https://kubernetes.io/docs/tasks/inject-data-application/distribute-credentials-secure/#define-a-container-environment-variable-with-data-from-a-single-secret
Donot store the passwd in the filesystem, which is not allowed from security view
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
| namespace: agentcube | ||
| type: Opaque | ||
| stringData: | ||
| password: "SuperSecretRedisPassword" |
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.
Please wait until #99, adapt to helm chart too
|
@yashisrani We can move forward now with all the dependencies merged |
Fix: #104
Key Changes:
Store:
REDIS_PASSWORD_FILEenvironment variable.REDIS_PASSWORDis not set.Deployment:
/etc/agentcube/secretsand configured the application to read the password from this location.