Extension of Redis Session Store to Support Redis Cluster #363
Conversation
yan-dblinf
requested review from
JoelSpeed,
steakunderscore and
loshz
as code owners
|
Hi @JoelSpeed, I made an initial commit regarding your comment in #340 . Please let me know if we are on the right track from your perspective. Later on, I can add more commits for test code and update documents. |
There was a problem hiding this comment.
This looks good for the most part, though I have added one suggestion below
One thing that came to mind while reading this, what happens if I set redis-use-sentinel and redis-use-cluster? I assume these should be mutually exclusive looking at the code you've added. Can we make sure there is some validation on the options so that people don't set both, expecting that to work?
I would also please ask that you update the documentation before we merge this
|
Hey @JoelSpeed, thank you for the valuable comments! I made some updates about the function name, conflict options check, and docs. Hope it covers all your points. My concern is about the unit test for which there is no available mock modules like miniredis or minisentinel. Since we don't have a unit test for sentinel, I wondering if I can skip it for cluster or it's required. WDYT? |
| CookieCipher: opts.Cipher, | ||
| CookieOptions: cookieOpts, | ||
| } | ||
| return rs, nil | ||
|
|
||
| } | ||
|
|
||
| func newRedisClient(opts options.RedisStoreOptions) (*redis.Client, error) { | ||
| func newRedisCmdable(opts options.RedisStoreOptions) (redis.Cmdable, error) { | ||
| if opts.UseSentinel && opts.UseCluster { |
There was a problem hiding this comment.
There is option validation earlier in the process than this, we should be able to validate the options there and fail earlier, maybe add it in here instead? https://github.com/pusher/oauth2_proxy/blob/9670f54dd00fd75ffd0fb765f8fd60aa64c1fabd/options.go#L208
There was a problem hiding this comment.
The function call stack for this validation is:
(1) options.Validate() ->
(2) sessions.NewSessionStore() ->
(3) redis_store.NewRedisSessionStore() ->
(4) redis_store.newRedisCmdable(),
which reuses the validation of session type in (2), and gathers validations related to Redis in (4). Since (4) is called indirectly by (1), has it already followed the fail-fast principle?
An alternative way to struct validations against session options is to put this check into a function like func (rso *RedisStoreOptions) Validate(), and let the parent struct options call it. WDYT?
There was a problem hiding this comment.
Thanks for the explanation of the stack, you are correct, it is verified at the same point. I think it's fine as is, though what I thought was happening was your suggestion of func (rso *RedisStoreOptions) Validate() 😂
I'm not sure which I prefer tbh, any second opinions welcome
There was a problem hiding this comment.
If you are okay with the current implementation, how about going with it as it follows the existing convention? The alternative way I suggest actually seems a little bit deviated from the main goal of this PR, i.e. support RedisCluster. IMHO, it deserves to open another PR for code refactoring in the future when we get a strong motivation. WDYT?
There was a problem hiding this comment.
I'm happy to keep this as is now you've clarified this for me 👍
Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>
Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>
| CookieCipher: opts.Cipher, | ||
| CookieOptions: cookieOpts, | ||
| } | ||
| return rs, nil | ||
|
|
||
| } | ||
|
|
||
| func newRedisClient(opts options.RedisStoreOptions) (*redis.Client, error) { | ||
| func newRedisCmdable(opts options.RedisStoreOptions) (redis.Cmdable, error) { | ||
| if opts.UseSentinel && opts.UseCluster { |
There was a problem hiding this comment.
I'm happy to keep this as is now you've clarified this for me 👍
|
@JoelSpeed Appreciate your helpful review! Hope to work with you in the open source community again. 👍 |
Description
Extend the redis session store to support redis cluster.
Motivation and Context
#340
How Has This Been Tested?
Tested on staging environment of Houzz for internal websites.
Checklist: