Authenticate to Redis with Microsoft Entra ID using Token Cache #372
Conversation
LofhJann
left a comment
LofhJann
left a comment
There was a problem hiding this comment.
Works.
Requesting some clarifications and changes, some smaller some bigger but main logic is fine.
Co-authored-by: Janne Löfhjelm <janne.lofhjelm@gmail.com>
…uthentication' into feat/microsoft-entra-for-redis-authentication
| int timeOutMs = connTimeOutSecs * 1000; | ||
| Jedis jedis = new Jedis(redisHost, port, timeOutMs); | ||
| jedis.connect(); | ||
| protected Jedis createRedisClient(@NotNull String redisHost, int port) { |
There was a problem hiding this comment.
I prefer having the opportunity to use both our own Redis or Redis managed by Azure. Please rather create a separate method or class.
There was a problem hiding this comment.
Also, sorry, I have to write this somewhere:
Some of the methods in this class do not really use private instance variables of the object, apart from the logger. Would a cleaner approach aim for static methods, perhaps split over several classes? Why is something named PulsarApplication creating Jedis clients with intricate code and health checks?
If possible, I would rather follow "clean as you code" instead of waiting for separate refactoring issues that might not get prioritized high enough, unless we need a total rewrite issue.
| .build()); | ||
| } | ||
|
|
||
| public static String extractUsernameFromToken(String token) { |
There was a problem hiding this comment.
This method looks nasty (and is from the Azure code samples). I would bury these Azure-specific things in a separate Azure kludge class if we choose to keep both kinds of Jedis clients.
| public boolean checkResponse(@Nullable final Long response) { | ||
| return response != null && response == 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
For all code copied from the Azure code samples, please refer to the exact commit and mention the license under which it is copied in a comment. E.g. The following method is copied or adapted from https://github.com/Azure/azure-sdk-for-java/blob/e7103f9019669032c7ffc3b51f1bd30c6ad8655f/sdk/identity/azure-identity/src/samples/Azure-Cache-For-Redis/Jedis/Azure-AAD-Authentication-With-Jedis.md under the MIT license.
Fixes AB#57327