Add possibility to use different passwords for Sentinel and Redis host (#1698) - #3140
Add possibility to use different passwords for Sentinel and Redis host (#1698)#3140doosterkamp wants to merge 9 commits into
Conversation
| Append(sb, OptionKeys.User, user); | ||
| Append(sb, OptionKeys.SentinelUser, sentinelUser); | ||
| Append(sb, OptionKeys.Password, (includePassword || string.IsNullOrEmpty(password)) ? password : "*****"); | ||
| Append(sb, OptionKeys.SentinelPassword, (includePassword || string.IsNullOrEmpty(SentinelPassword)) ? sentinelPassword : "*****"); |
There was a problem hiding this comment.
this should be checking the field, not the property; i.e. if I assign just .Password, I do not expect it to start showing in the config-string as the SentinelPassword=... value
There was a problem hiding this comment.
(actually, this is only used for the empty check, but the point remains - the check should be consistent in the IsNullOrEmpty and value part)
There was a problem hiding this comment.
Again, wanted to go to fast ... sorry.
Has been corrected.
By the way; is there a reason you use IsNullOrEmpty and not IsNullOrWhitespace?
I always liked IsNullOrWhitpace a little more, as it also checks for (unwanted?) spaces, tabs, etc.
| { | ||
| var sentinelConnection = SentinelConnect(configuration, log); | ||
| // Use separate sentinel credentials when provided on the configuration | ||
| var sentinelConfig = configuration.Clone(); |
There was a problem hiding this comment.
this feels ... weird; I wonder whether what we should be doing is making sure that ConnectionMultiplexer stores the serverType (which is already passed in), or (preferable) just a private readonly bool _isSentinel, and using that during handshake to read from the appropriate fields properties of the non-mutated options; thoughts?
There was a problem hiding this comment.
I like the thought of private readonly bool _isSentinel!
It makes the intend more clear and it should make it more robust and threadsafe, avoiding issues with the handshake.
It looked like a small change, so I took the liberty to apply this (I've put it in a separate commit, so it's an easy roll-back if you don't like it)
Also, I have rolled back the cloning part and took a better look at how and where the connection was set up.
It turns out to be a small change also, so I moved the assignment of the (Sentinel) user/pw to the HandshakeAsync method.
mgravell
left a comment
mgravell
left a comment
There was a problem hiding this comment.
Concept is great, +1; a few nits in the ConfigurationOptions piece just because that type is super weird (not your fault), and I'm not a huge fan of the clone usage - I welcome your thoughts there.
Thanks! |
…nOptions - fixed typos, so fields, not properties, are used on the correct places
|
@mgravell is there a nice/correct way to clear the CodeQL security failure or do I need to add an inline suppression message? |
2 participants
Adding the possibility to use different passwords for Sentinel and Redis host.
Fixing the long standing issue #1698
Checklist