Skip to content

Conversation

@rawkode
Copy link

@rawkode rawkode commented Sep 30, 2025

This adds support for generating passwords or using a pre-existing secret, as well as test coverage for all changes.

Copilot AI review requested due to automatic review settings September 30, 2025 21:12
@rawkode rawkode force-pushed the add-acl-authentication branch from 4196475 to 2787b1e Compare September 30, 2025 21:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds ACL authentication support to the Valkey Helm chart with three different configuration methods: auto-generated secrets, existing secrets, and inline configuration. It includes comprehensive validation logic and extensive test coverage to ensure only one authentication method is used at a time.

  • Adds three authentication methods with priority-based selection and mutual exclusion validation
  • Implements auto-generation of passwords and ACL configurations via Kubernetes secrets
  • Provides comprehensive test coverage for all authentication scenarios and edge cases

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
valkey/values.yaml Added authentication configuration options with detailed comments
valkey/templates/secret.yaml New template for generating authentication secrets
valkey/templates/init_config.yaml Updated to handle different authentication methods
valkey/templates/deploy_valkey.yaml Added auth volume mounts and validation
valkey/templates/_helpers.tpl Added authentication configuration validation logic
valkey/templates/tests/auth.yaml New test pods for authentication verification
valkey/tests/*.yaml Comprehensive test suites for all components
Justfile Development task automation
.github/workflows/test.yml CI/CD pipeline for testing
valkey/.helmignore Updated ignore patterns

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@mk-raven mk-raven self-assigned this Oct 3, 2025
@mk-raven
Copy link
Collaborator

mk-raven commented Oct 3, 2025

Need to check it firstly

@mk-raven
Copy link
Collaborator

mk-raven commented Oct 16, 2025

@rawkode Please, fix password generation block:

data:
{{- $password := .Values.auth.generateDefaultUser.password }}
  {{- if not $password }}
    {{- $password = randAlphaNum 32 }}

as suggestion writed before
The random password generation using randAlphaNum will create a new password on every template rendering, which could cause authentication failures during upgrades.

rawkode and others added 2 commits October 16, 2025 09:14
Co-authored-by: Copilot <[email protected]>
Signed-off-by: David Flanagan <[email protected]>
@rawkode rawkode requested review from Copilot and mk-raven October 16, 2025 08:17
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

rawkode and others added 2 commits October 16, 2025 09:18
Co-authored-by: Copilot <[email protected]>
Signed-off-by: David Flanagan <[email protected]>
@rawkode rawkode requested a review from Copilot October 16, 2025 08:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

{{- /* Check if aclConfig has actual content (not just comments/whitespace) */}}
{{- if .Values.auth.aclConfig }}
{{- $trimmed := .Values.auth.aclConfig | trim }}
{{- /* Use regex to check for any non-empty, non-comment line */}}
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

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

[nitpick] The regex pattern uses (?m) multiline flag but could be simplified. Consider using a more readable approach or add a comment explaining the regex logic for maintainability.

Suggested change
{{- /* Use regex to check for any non-empty, non-comment line */}}
{{- /* Use regex to check for any non-empty, non-comment line */}}
{{- /*
The regex pattern "(?m)^(\s*[^#\s].*)$" uses the multiline flag (?m) so that ^ and $ match the start/end of each line.
It matches any line that is not just whitespace and does not start with a '#' (comment).
This ensures aclConfig contains at least one meaningful, non-comment line.
*/}}

Copilot uses AI. Check for mistakes.
# If password key exists in secret, test with it
if [ -f /valkey-auth/password ]; then
PASSWORD=$(cat /valkey-auth/password)
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

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

The USERNAME variable is used but never defined in this context. This appears to be a shell variable that should either be defined or the fallback logic should be clarified.

Suggested change
PASSWORD=$(cat /valkey-auth/password)
PASSWORD=$(cat /valkey-auth/password)
# Extract the first username from the ACL file, fallback to "default" if not found
USERNAME=$(awk '/^user / {print $2; exit}' /valkey-auth/users.acl)

Copilot uses AI. Check for mistakes.
@mk-raven mk-raven requested a review from sgissi October 24, 2025 14:42
@mk-raven mk-raven added enhancement New feature or request security labels Oct 24, 2025
@mk-raven
Copy link
Collaborator

mk-raven commented Oct 24, 2025

@rawkode template needs to be fixed:

Run helm lint ./valkey
Error: 1 chart(s) linted, 1 chart(s) failed
==> Linting ./valkey
Error:  templates/: parse error at (valkey/templates/_helpers.tpl:79): invalid syntax

Error: Process completed with exit code 1.

https://github.com/valkey-io/valkey-helm/actions/runs/18554956365/job/53594604893?pr=14

@davgia
Copy link

davgia commented Oct 29, 2025

Any update on this? This is a deal breaker for me (and I think for most users too). Thanks!

@jonkerj
Copy link

jonkerj commented Oct 29, 2025

I need this as well, let's wait just a bit longer for @rawkode to fix this, otherwise I'll submit a PR that passes the checks.

@sgissi
Copy link
Collaborator

sgissi commented Oct 31, 2025

@rawkode Thanks for the PR, excellent work!! Test units look really good!

My 2c, I rather not having auto-generated passwords. This introduces run-time dependencies and cannot be tested. Instead I would introduce an existingSecret/existingSecretKey under generateDefaultUser that would take precedence over the plaintext password.

PS: I noticed there are RegEx gymnastics over aclConfig to detect if there is actual content or not. I would default it to "", and document the format as a values.yaml comment and/or README. Then it can be tested by if .Values.aclConfig directly.

Thoughts?

@OnekO
Copy link

OnekO commented Nov 10, 2025

Here is a working copy, updated to main just now:
https://github.com/OnekO/valkey-helm/tree/add-acl-authentication

I've made a PR to rawcode's repo, just ask for it if you want to merge my branch, in case @rawkode dissapears.

@rawkode
Copy link
Author

rawkode commented Nov 10, 2025

Sorry, I've not had time to clean this up. Happy to see someone else push this forward 😍

@mk-raven
Copy link
Collaborator

Sorry, I've not had time to clean this up. Happy to see someone else push this forward 😍

Amm, ok. I could try to do it by myself or if @OnekO already had some fixes, maybe should make in another PR.

@chrisjaimon2012
Copy link

ah, waiting for this...
@OnekO can you create another PR?
Thanks

@sgissi
Copy link
Collaborator

sgissi commented Nov 13, 2025

Hi all, I have updated this PR with fixes and a few changes. I'll push them here tomorrow.

@sgissi sgissi self-assigned this Nov 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants