-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
CredentialsProvider class added to support password rotation #2261
Conversation
Codecov ReportBase: 92.04% // Head: 92.12% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2261 +/- ##
==========================================
+ Coverage 92.04% 92.12% +0.07%
==========================================
Files 110 113 +3
Lines 28746 29063 +317
==========================================
+ Hits 26460 26773 +313
- Misses 2286 2290 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
3affd60
to
7b958e0
Compare
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.
Except these tiny comments - it looks great!
Added type hints + moved CredentialsProvider to a separate file |
ef24d8d
to
f85dbbc
Compare
@chayim @dvora-h - Finished round, ready for review :). Tests failed here from some internal error, in my fork it seems that tests are passing: |
@barshaul Thanks as always. I left a tonne of comments - and will give it another once over. As part of this, I suggest a MockCredentialProvider or similar. It would be nice to provide an example of a standard provider, perhaps implemented by a single (useless) function. This is how @dvora-h initially looked at this PR in fact. |
@chayim Finished round. I left some of the reviews open with questions |
533a887
to
dbb16f4
Compare
…to enable backward compatibility with changing the members value on existing connection.
… and 'password' inside on_connect function
c68a715
to
abe6137
Compare
4385dfc
to
4205350
Compare
4205350
to
ba91b0f
Compare
@barshaul I think that after adding async support we are good to go. |
LGTM now! |
References: 1. #1602 and related PRs. Current PR is probably better than handling in JedisFactory 2. redis/redis-py#2261 - main reason of this PR 3. redis/lettuce#1774 4. #632 --- * Introduce credentials provider * use volatile * Test in Sentineled mode * Support CharSequence in DefaultRedisCredentials * Added doc for prepare() and cleanUp() * Test the provider interface * Added example * Removed deprecations
Pull Request check-list
Please make sure to review and check all of these items:
$ tox
pass with this change (including linting)?NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
Instead of providing a simple pair of username+password, the users can create a CredentialsProvider object with their own credentials supplier function, and have redis-py call it whenever a new connection is created.
By doing so, users will be able to fetch the current password and rotate credentials without having to create a new client.
See related feature request in Lettuce: redis/lettuce#1774