-
Notifications
You must be signed in to change notification settings - Fork 115
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 Credentials Provider to allow short(er) lived credentials. #376
base: 4.x
Are you sure you want to change the base?
Conversation
8159205
to
5733fa7
Compare
…lled RedisCredentialsProvider. This allows to use short-lived credentials. Credentials are updated for new connections.
5733fa7
to
26a7d52
Compare
@holomekc actually we would like to do it in more generic fashion, we would like to be able to create a Redis client using a java |
Hi @vietj, I can try ;) . I would need some guidance though. I understand what you want to achieve I think. So for the client creation it would start with something like this? static Redis createClient(Vertx vertx, RedisOptions options) {
return createClient(vertx, () -> Future.succeededFuture(options));
}
static Redis createClient(Vertx vertx, Supplier<Future<RedisOptions>> options) { So that the creator is able to adjust the options during runtime? On the one side I think this is a great idea, but this also creates some issues. Changing the existing RedisOptions is only suitable for a subset of the options, which can be specified. E.g. for the credentials it make sense to me. But there are also some values, which needs to be available at client creation time and are probably not changable. E.g. type, poolName, poolSize, netClientOptions, etc. Two examples during client creation:
This would force me to resolve the options in a blocking manner. Isn't it then also confusing to the user that some values are adjustable during runtime and some are not? So I guess I am missing something. Can you provide some guidance here? Br, |
I do agree with you that some options cannot be changed and we should maybe create a specific Options class for this that would have the subset of options that can be changed before. |
Could you provide a code sample how to connect to a AWS ElasticCache Redis server with the IAM Module? How to do ? Thank you |
Hi @geniusit. See answer in #356 (comment) |
Motivation:
At the moment the implementation only allows one hard coded user and password. In theory it would be possible to override the getPassword method of RedisOptions, but this is pretty hacky and also blocks the thread in case providing the credentials (or here just the password) takes a longer time.
With the provided changes it is possible to dynamically adjust the credentials used for a new connection. The rule is: credentials provider > URI > RedisOptions (if available). This allows to use short lived credentials or allow to rotate credentials and update them without reconfiguration. In the Issues section I saw that somebody asked for AWS credentials. This should be possible here in multiple ways. E.g. using IAM auth added for ElastiCache recently, or extracing credentials from AWS SecretsManager (with rotation), etc.
Questions:
I hope I managed to create some useful tests. I was ok with creating the adjustments for RedisConnectionManager, but I am a bit confused regarding the RedisClusterConnection class. I basically searched for getPassword() usage and these are the only two locations. But the later one is never considering the username. This looks a bit strange to me. I just adjusted it so that it still just considers the username and skips the username information there.