-
-
Notifications
You must be signed in to change notification settings - Fork 931
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
Add support for setting redis username #1351
Add support for setting redis username #1351
Conversation
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.
👍 Have requested 1 additional test, and I had to use a slightly modified command to set up the testuser
, but this works great, thank you @gabor-boros !
acl setuser testuser on >password allkeys allcommands
- I tested this using the instructions in the PR description.
- I read through the code
-
Includes documentationN/A -- standard connection string
@@ -944,6 +944,7 @@ def _connparams(self, asynchronous=False): | |||
'host': conninfo.hostname or '127.0.0.1', | |||
'port': conninfo.port or self.connection.default_port, | |||
'virtual_host': conninfo.virtual_host, | |||
'username': conninfo.userid, |
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.
should we also update the related docs?
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.
@auvipy Actually, this is a good question. I'd say yes if the documentation explicitly says that the username part of the connection URL won't be parsed (before this change), otherwise, I'd say that this should be "mainstream" that a connection URL can contain a username.
If you would like me, I'm happy to find the relevant section in the documentation and explicitly state that, from now on, usernames will be parsed too.
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.
Please do.
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.
@thedrow @auvipy I'm happy to extend any documentation, if you could advise a bit. The documentation has no mention at all about usernames in terms of the Redis transport. In fact, the documentation is defined as
.. code-block::
redis://REDIS_ADDRESS[:PORT][/VIRTUALHOST]
rediss://REDIS_ADDRESS[:PORT][/VIRTUALHOST]
To use sentinel for dynamic Redis discovery,
the connection string has the following format:
.. code-block::
sentinel://SENTINEL_ADDRESS[:PORT]
I do believe -- but correct me if I'm wrong -- the reason behind this is that the host parameter (REDIS_ADDRESS
) may contain username and password as per RFC 2617. If you would like me, I can describe that in the inline documentation, though that would be a bit verbose.
If you would like me to extend another documentation that I did not find (yet), please help me with the link.
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 documentation is not 100% complete. I am not 100% whether is good idea to refer to some standard because URL parsing is done by urllib.parse.urlparse
but some magic is involved during the parsing - see kombu/utils/url.py:url_to_parts()
. Let it keep simple. Please extend the documentation as follows:
redis://[USER:PASSWORD@]REDIS_ADDRESS[:PORT][/VIRTUALHOST]
rediss://[USER:PASSWORD@]REDIS_ADDRESS[:PORT][/VIRTUALHOST]
Maybe we can also add some examples to the documentation.
Otherwise, the PR seems to be fine for me.
@gabor-boros thank you for our PR. Let me review this PR. I try to do it in next few days. |
Thank you @gabor-boros for your PR. I have accepted the PR. The documentation update is committed in next commit. |
* feat: add support for setting redis username * tests: add redis connparams credentials tests (cherry picked from commit 2d036f5)
* feat: add support for setting redis username * tests: add redis connparams credentials tests (cherry picked from commit 2d036f5)
Although the underlying Redis package supports setting username for connections, the connection params for username are not passed to the
Redis
client.This PR extracts the
userid
from connection info (or host) if and passes to Redis client ensuring the connection can be established for other users than the defaultdefault
user (set when username isNone
).Dependencies: None
Testing instructions:
docker run --name kombu_redis --rm -it -p 6379:6379 redis:latest
)docker exec -it kombu_redis redis-cli
)ACL LIST
) and validate onlydefault
user is listeddefault
by runningacl setuser default on >securepass
ACL LIST
) and validatedefault
user now has a password hash setacl setuser testuser on >password allcommands
ACL LIST
) and validatetestuser
is created with a password hashExample 1
Example 2
Author notes and concerns:
Redis' "new" ACL features released as part of Redis 6 introduced the possibility of defining additional users identified by passwords. Therefore it would be a great achievement if Kombu (and Celery) would support connecting to Redis using an ACL limited user.