feat: add ghproxy valkey/redis ACL support #433
Conversation
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @kairoaraujo! |
|
Hi @kairoaraujo. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-ci-robot
added
the
area/ghproxy
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kairoaraujo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/S
✅ Deploy Preview for k8s-prow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
cncf-cla: yes
cmd/ghproxy/ghproxy.go
Outdated
| @@ -143,6 +145,8 @@ func flagOptions() *options { | |||
| flag.IntVar(&o.sizeGB, "cache-sizeGB", 0, "Cache size in GB per unique token if using a disk cache.") | |||
| flag.BoolVar(&o.diskCacheDisableAuthHeaderPartitioning, "legacy-disable-disk-cache-partitions-by-auth-header", true, "Whether to disable partitioning a disk cache by auth header. Disabling this will start a new cache at $cache_dir/$sha256sum_of_authorization_header for each unique authorization header. Bigger setups are advise to manually warm this up from an existing cache. This option will be removed and set to `false` in the future") | |||
| flag.StringVar(&o.redisAddress, "redis-address", "", "Redis address if using a redis cache e.g. localhost:6379.") | |||
| flag.StringVar(&o.redisUsername, "redis-usersname", "", "Redis username") | |||
There was a problem hiding this comment.
| flag.StringVar(&o.redisUsername, "redis-usersname", "", "Redis username") | |
| flag.StringVar(&o.redisUsername, "redis-username", "", "Redis username") |
| @@ -143,6 +145,8 @@ func flagOptions() *options { | |||
| flag.IntVar(&o.sizeGB, "cache-sizeGB", 0, "Cache size in GB per unique token if using a disk cache.") | |||
| flag.BoolVar(&o.diskCacheDisableAuthHeaderPartitioning, "legacy-disable-disk-cache-partitions-by-auth-header", true, "Whether to disable partitioning a disk cache by auth header. Disabling this will start a new cache at $cache_dir/$sha256sum_of_authorization_header for each unique authorization header. Bigger setups are advise to manually warm this up from an existing cache. This option will be removed and set to `false` in the future") | |||
| flag.StringVar(&o.redisAddress, "redis-address", "", "Redis address if using a redis cache e.g. localhost:6379.") | |||
There was a problem hiding this comment.
I would add some validation that states that redis-address must be supplied if either of the 2 new fields are
pkg/ghcache/ghcache.go
Outdated
| var conn redis.Conn | ||
| var err error | ||
| if redisUsername != "" { | ||
| conn, err = redis.Dial("tcp", redisAddress, redis.DialUsername(redisUsername), redis.DialPassword(redisPassword)) | ||
| } else if redisPassword != "" { | ||
| conn, err = redis.Dial("tcp", redisAddress, redis.DialPassword(redisPassword)) | ||
| } else { | ||
| conn, err = redis.Dial("tcp", redisAddress) | ||
| } |
There was a problem hiding this comment.
Rather than creating the connection in three separate statements, can we just always pass the two new options? it seems like passing empty strings for either of the options would have the same result as not including them, but that should be verified.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
There was a problem hiding this comment.
Pull Request Overview
This PR adds ACL support for valkey/redis in ghproxy by enabling the use of a username and password when connecting to Redis.
- Modified the NewRedisCache function in pkg/ghcache/ghcache.go to support ACL by conditionally passing the username and password.
- Extended the ghproxy command-line options with redisUsername and redisPassword flags.
- Noted a typographical error in the flag name for redisUsername.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/ghcache/ghcache.go | Updated NewRedisCache to support ACL connection using username and password |
| cmd/ghproxy/ghproxy.go | Added new flags for redis credentials; detected a typo in the username flag |
cmd/ghproxy/ghproxy.go
Outdated
| @@ -143,6 +145,8 @@ func flagOptions() *options { | |||
| flag.IntVar(&o.sizeGB, "cache-sizeGB", 0, "Cache size in GB per unique token if using a disk cache.") | |||
| flag.BoolVar(&o.diskCacheDisableAuthHeaderPartitioning, "legacy-disable-disk-cache-partitions-by-auth-header", true, "Whether to disable partitioning a disk cache by auth header. Disabling this will start a new cache at $cache_dir/$sha256sum_of_authorization_header for each unique authorization header. Bigger setups are advise to manually warm this up from an existing cache. This option will be removed and set to `false` in the future") | |||
| flag.StringVar(&o.redisAddress, "redis-address", "", "Redis address if using a redis cache e.g. localhost:6379.") | |||
| flag.StringVar(&o.redisUsername, "redis-usersname", "", "Redis username") | |||
| flag.StringVar(&o.redisPassword, "redis-password", "", "Redis password") | |||
There was a problem hiding this comment.
I was not able to a full review but I see we add a sensitive value parameter.
- It should not be passed as a value; a typical pattern in Prow is to pass a path to a file with the secret as content
- Internally we should work with secrets through the agent. It handles reading the secret from the file, reloading (maybe not that useful here) and log censoring. Whatever needs the secret will fetch it from the agent.
add ghproxy support to valkey/redis using ACL (username and password) when using --redis-address the user can pass the username and password if the redis server has auth --redis-username=username --redis-password=password Signed-off-by: Kairo Araujo <kairo@dearaujo.nl>
Signed-off-by: Kairo Araujo <kairo@dearaujo.nl>
kairoaraujo
force-pushed
the
ghproxy-valkey-redis-credentials
branch
from
ad8a96e to
92f64ea
Compare
k8s-ci-robot
added
size/M
add ghproxy support to valkey/redis using ACL (username and password)
when using
--redis-addressthe user can pass the username and password if the redis server has authNew parameters:
--redis-username=username--redis-password=password