-
Notifications
You must be signed in to change notification settings - Fork 270
feat(redis): add REDIS_SSL_CERT_REQS and TLS config support #471
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,31 +20,42 @@ var ( | |
| ErrNotFound = errors.New("key not found") | ||
| ) | ||
|
|
||
| func getRedisOptions(addr, username, password string, useSsl bool, db int) *redis.Options { | ||
| func getRedisOptions(addr, username, password string, useSsl bool, db int, tlsConf *tls.Config) *redis.Options { | ||
| opts := &redis.Options{ | ||
| Addr: addr, | ||
| Username: username, | ||
| Password: password, | ||
| DB: db, | ||
| } | ||
| if useSsl { | ||
| opts.TLSConfig = &tls.Config{} | ||
| // Use provided TLS config (encodes CERT_NONE / REQUIRED policy). If nil, default to system roots. | ||
| if tlsConf != nil { | ||
| opts.TLSConfig = tlsConf | ||
| } else { | ||
| opts.TLSConfig = &tls.Config{} | ||
| } | ||
| } | ||
| return opts | ||
| } | ||
|
|
||
| func InitRedisClient(addr, username, password string, useSsl bool, db int) error { | ||
| opts := getRedisOptions(addr, username, password, useSsl, db) | ||
| func InitRedisClient(addr, username, password string, useSsl bool, db int, tlsConf *tls.Config) error { | ||
| opts := getRedisOptions(addr, username, password, useSsl, db, tlsConf) | ||
| client = redis.NewClient(opts) | ||
|
|
||
| if _, err := client.Ping(ctx).Result(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func InitRedisSentinelClient(sentinels []string, masterName, username, password, sentinelUsername, sentinelPassword string, useSsl bool, db int, socketTimeout float64) error { | ||
| func InitRedisSentinelClient( | ||
| sentinels []string, | ||
| masterName, username, password, sentinelUsername, sentinelPassword string, | ||
| useSsl bool, | ||
| db int, | ||
| socketTimeout float64, | ||
| tlsConf *tls.Config, | ||
| ) error { | ||
| opts := &redis.FailoverOptions{ | ||
| MasterName: masterName, | ||
| SentinelAddrs: sentinels, | ||
|
|
@@ -56,7 +67,12 @@ func InitRedisSentinelClient(sentinels []string, masterName, username, password, | |
| } | ||
|
|
||
| if useSsl { | ||
| opts.TLSConfig = &tls.Config{} | ||
| // go-redis v9 uses TLSConfig for both Sentinel discovery and data connections | ||
| if tlsConf != nil { | ||
| opts.TLSConfig = tlsConf | ||
| } else { | ||
| opts.TLSConfig = &tls.Config{} | ||
| } | ||
| } | ||
|
|
||
| if socketTimeout > 0 { | ||
|
|
@@ -68,7 +84,6 @@ func InitRedisSentinelClient(sentinels []string, masterName, username, password, | |
| if _, err := client.Ping(ctx).Result(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
|
|
@@ -77,15 +92,13 @@ func Close() error { | |
| if client == nil { | ||
| return ErrDBNotInit | ||
| } | ||
|
|
||
| return client.Close() | ||
| } | ||
|
|
||
| func getCmdable(context ...redis.Cmdable) redis.Cmdable { | ||
| if len(context) > 0 { | ||
| return context[0] | ||
| } | ||
|
|
||
| return client | ||
| } | ||
|
|
||
|
|
@@ -365,13 +378,12 @@ func ScanMap[V any](key string, match string, context ...redis.Cmdable) (map[str | |
|
|
||
| result := make(map[string]V) | ||
|
|
||
| ScanMapAsync[V](key, match, func(m map[string]V) error { | ||
| _ = ScanMapAsync[V](key, match, func(m map[string]V) error { | ||
| for k, v := range m { | ||
| result[k] = v | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| }, context...) | ||
|
|
||
| return result, nil | ||
|
Comment on lines
+381
to
388
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change correctly passes the The The function signature of if err := ScanMapAsync[V](key, match, func(m map[string]V) error {
for k, v := range m {
result[k] = v
}
return nil
}, context...); err != nil {
return nil, err
}
return result, nil |
||
| } | ||
|
|
@@ -388,7 +400,6 @@ func ScanMapAsync[V any](key string, match string, fn func(map[string]V) error, | |
| kvs, newCursor, err := getCmdable(context...). | ||
| HScan(ctx, serialKey(key), cursor, match, 32). | ||
| Result() | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -399,7 +410,6 @@ func ScanMapAsync[V any](key string, match string, fn func(map[string]V) error, | |
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| result[kvs[i]] = value | ||
| } | ||
|
|
||
|
|
@@ -410,7 +420,6 @@ func ScanMapAsync[V any](key string, match string, fn func(map[string]V) error, | |
| if newCursor == 0 { | ||
| break | ||
| } | ||
|
|
||
| cursor = newCursor | ||
| } | ||
|
|
||
|
|
@@ -533,7 +542,6 @@ func Subscribe[T any](channel string) (<-chan T, func()) { | |
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| ch <- v | ||
| case *redis.Pong: | ||
| default: | ||
|
|
||
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 handling of
REDIS_SSL_CERT_REQShas a couple of issues:CERT_OPTIONALis implemented identically toCERT_REQUIRED(InsecureSkipVerify = false). This is misleading, asCERT_OPTIONALtypically implies that certificate verification is attempted but not enforced. Go'scrypto/tlspackage does not have a direct equivalent for this behavior. To avoid confusion, it would be better to explicitly disallow this value.defaultcase silently treats any invalid value asCERT_REQUIRED. This can hide misconfigurations. It would be safer to return an error for unknown values.I suggest refactoring this
switchstatement to be stricter and clearer.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.
Hi @Oak10, I'd like to follow the suggestions, Let's me know your thoughts