Skip to content
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

Don't send AUTH and/or SELECT command after connecting to sentinels #346

Merged
merged 1 commit into from
Jun 14, 2016
Merged

Don't send AUTH and/or SELECT command after connecting to sentinels #346

merged 1 commit into from
Jun 14, 2016

Conversation

pascal-hofmann
Copy link

Connecting to sentinels is broken with v1.1. Bad commit: 5a0dfc3#diff-e55f11b907debe0b57a114d0c38dc563R244.

AUTH and/or SELECT commands are sent to sentinels when password or database is specified. Sentinels don't understand these commands and thus master discovery fails and a Predis\ClientException with message "No sentinel server available for autodiscovery." is thrown.

We need to explicitly specify a null password and database.

@nrk nrk added the bug label Jun 14, 2016
@nrk nrk added this to the v1.1.1 milestone Jun 14, 2016
@nrk nrk merged commit ce8b3fb into predis:master Jun 14, 2016
@nrk
Copy link
Contributor

nrk commented Jun 14, 2016

My bad, it was actually the intended behavior but I failed to notice that stripping the password and database keys from the user-supplied parameters array using unset() would not have prevented the connection factory from applying the password and database parameters coming from the global parameters client option. Having both keys defined but set to NULL takes the precedence over the default parameters applied by the connection factory, fixing the issue. Thanks!

I was never really satisfied with the code I wrote for the configuration of the redis-sentinel backend but it was the only way to provide an easy configuration for users without pushing any breaking change, now I'm definitely convinced that this needs to be reworked for v2.0 (BCs allowed).

@nrk nrk added the redis-sentinel Replication (managed by redis-sentinel) label Jun 14, 2016
@AaronJan
Copy link

Just updated my project and got this error.. 😭

@pascal-hofmann
Copy link
Author

@AaronJan For the time being this works for me in composer.json:
"predis/predis": "^1.1.1@dev",

@pascal-hofmann pascal-hofmann deleted the master branch June 15, 2016 12:20
@nrk
Copy link
Contributor

nrk commented Jun 15, 2016

@AaronJan this fix will be available starting with v1.1.1 but it's still unreleased (I plan to ship it tomorrow), if you need it ASAP you can tweak your composer.json accordingly.

@AaronJan
Copy link

Thank you guys! I'll try that. 😄

@nrk
Copy link
Contributor

nrk commented Jun 16, 2016

@AaronJan JFYI Predis v1.1.1 is now available.

@AaronJan
Copy link

@nrk 👍 Copy that!

nrk added a commit that referenced this pull request Sep 5, 2020
Password-based authentication for sentinels has been added in Redis 5.
Predis was actively ignoring any "password" parameter for sentinels when
creating connections to them to avoid issues when this parameter is set
in the default "parameters" array passed via client options, as they are
applied to **every** connection created by Predis (see #346).

We need to find a better way to specify a common password for sentinels
to be handled in a different way than the ones for Redis nodes. For now
each sentinel node protected by password must have an explicit password
set in its parameters list even if this password, by design, is the same
for all sentinels. Since we cannot use default "parameters" as explained
above but we still need to pass a common value for all sentinels an idea
could be using a dedicated client option like we did with "service", but
we will see later.

In this commit we also explicitly reset any "username" parameter as it
would trigger an `AUTH $username $password` but sentinels do not support
ACL authentication.

Fixes #594.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug redis-sentinel Replication (managed by redis-sentinel)
Development

Successfully merging this pull request may close these issues.

None yet

3 participants