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

Add support for a credential provider #246

Closed
trentbitterman opened this issue Nov 14, 2024 · 9 comments
Closed

Add support for a credential provider #246

trentbitterman opened this issue Nov 14, 2024 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@trentbitterman
Copy link
Contributor

trentbitterman commented Nov 14, 2024

Currently, coredis allows authenticating with Redis clusters by supplying a username and password. This works in most situations, but in some cases, such as an Elasticache cluster using IAM for auth, this can't be used as the "password" will rotate and there's no good way in coredis to update the password when it does. Other Redis libraries, such as redis-py, allow the specification of a credential provider, which is basically just an object with a get_credential method that will return the current password when called. It would be useful if coredis had something similar.

Expected Behaviour

In addition to supporting a password string parameter, the various coredis clients should also allow passing in a credential provider object that coredis can use to query the password/credential whenever it is needed.

Thanks for all of your work on this library. It's really great!

@trentbitterman trentbitterman added the enhancement New feature or request label Nov 14, 2024
@alisaifee alisaifee self-assigned this Nov 26, 2024
@alisaifee
Copy link
Owner

Apologies for the late response. This makes a lot of sense and would be a pretty straightforward addition to the API. I'm open to a PR for this as I've had limited bandwidth recently for maintenance, however, if not - I'd appreciate any suggestions regarding which providers make sense as builtins versus just being documented as recipes or suggestions in the documentation.

@trentbitterman
Copy link
Contributor Author

No worries. I'll work on putting something together and hopefully have a PR up sometime next week.

@alisaifee
Copy link
Owner

No worries. I'll work on putting something together and hopefully have a PR up sometime next week.

Thank you! The state of master/CI is a lot more stable now (and python 3.13 compatible) so hopefully that should make things a little easier to work with.

@trentbitterman
Copy link
Contributor Author

Great, thanks! I've put up a PR at #253 that I think should be ready for review. It looks like you'll need to trigger the gates for it. So far I only added a basic username and password provider and documented that plus the abstract class, so I can more if you think that would be better. Let me know what you'd prefer and if there are any changes you'd like to my PR. Thanks!

@alisaifee
Copy link
Owner

Great work, and I think the basic username/password support built in is fine (especially with the recipe for IAM being shipped with the package and well documented).

@alisaifee
Copy link
Owner

@trentbitterman: I made a few changes to the IAM recipe. Specifically:

  • use aiobotocore instead of botocore
  • use asyncache to decorate the get_credentials method (the cachetools decorator was raising an error upon subsequent calls as it was returning an already awaiting coroutine)

Additionally I've added the dependencies needed for recipes in requirements/recipes.txt which can be used during installation by doing pip install coredis[recipes].

I've done some basic testing to verify that this works functionally - but not much in terms of how this changes any performance characteristics. Perhaps you can take a look before I cut a release?

@alisaifee alisaifee reopened this Dec 6, 2024
@trentbitterman
Copy link
Contributor Author

Sure, I can take a look. Thanks for making those improvements!

@alisaifee
Copy link
Owner

Just FYI, I've cut the 4.18.0 release with the recipe using aiobotocore. If you find any issues or better approaches in your environment please do let me know since at the moment I have no real use case to test it out.

@trentbitterman
Copy link
Contributor Author

Thanks. Sorry, I got busy with other things and haven't gotten around to testing it. I'll hopefully test it out later this week and let you know if I run into any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants