Skip to content

Conversation

@shohamy7
Copy link
Contributor

@shohamy7 shohamy7 commented Jan 3, 2024


Redis 6 and above introduced an option to authenticate with username in addition to password.
This PR aims to add support for this kind of authentication.
For more info, see here

@eladkal
Copy link
Contributor

eladkal commented Jan 3, 2024

Redis 6 and above introduced an option to authenticate with username in addition to password.

We currently don't support redis 6 due to

# We limit redis to <5.0.0 because of incompatibility with celery. Both Celery and Kombu limited it
# and deferred fixing it for later, we should bump the limit once they do. Also !=4.5.5 matches celery
# limits and prevents installing 4.5.5 which is broken.
# https://github.com/celery/celery/pull/8442, https://github.com/celery/kombu/pull/1776
- redis>=4.5.2,<5.0.0,!=4.5.5

If this is not relevant any more then we should update the provider.yaml if its still relevant than we need to handle the version support

@shohamy7
Copy link
Contributor Author

shohamy7 commented Jan 3, 2024

I saw this limitation, but this limitation is for the redis client library.
It looks like the client library in the corresponding version can work with redis 6 as specified here

@eladkal
Copy link
Contributor

eladkal commented Jan 3, 2024

It looks like the client library in the corresponding version can work with redis 6 as specified here

cool

@shohamy7 shohamy7 force-pushed the allow-username-auth-with-redis-hook branch from 6695f1b to 4423bbf Compare January 3, 2024 19:27
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this new parameter compatible with the older versions?

@shohamy7
Copy link
Contributor Author

shohamy7 commented Jan 3, 2024

@hussein-awala
Currently, in the redis provider we are requiring at least redis>=4.5.2 which support Redis versions 5.0 to 7.0.
From the redis docs, it looks like there is backward compatibility (when we are not specifying the username, the default user will be used).
From the redis client perspective, the username field was always there - we just did not set it (so it was None).
If you won't set the login field in the connection form - the username will be None and the RedisHook will act the same as before.
When using old Redis versions (e.g. 5.0) which not support username authentication - it should be handle by the redis client, as they are saying that it is supported :)

@shohamy7 shohamy7 force-pushed the allow-username-auth-with-redis-hook branch from 4423bbf to 6f64c0f Compare January 3, 2024 20:14
@hussein-awala
Copy link
Member

Indeed, I just checked and found that the username argument is accepted in the old versions.

@hussein-awala hussein-awala merged commit 66f9b38 into apache:main Jan 3, 2024
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 4, 2024
The two related PRs #3656i1 and apache#36562 were merged without rebasing
and it caused wrong expectation in the test after username had been
added in the client in one of them.
potiuk added a commit that referenced this pull request Jan 4, 2024
The two related PRs #36561 and #36562 were merged without rebasing
and it caused wrong expectation in the test after username had been
added in the client in one of them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants