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

Redis driver fork update v9.6.1 #47049

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

greedy52
Copy link
Contributor

@greedy52 greedy52 commented Oct 1, 2024

@greedy52 greedy52 added the no-changelog Indicates that a PR does not require a changelog entry label Oct 1, 2024
@greedy52 greedy52 self-assigned this Oct 1, 2024
Comment on lines -112 to -116
OnConnect: onConnect,

// Auth should be done by the `OnConnect` callback here. So disable
// "automatic" auth by the client.
DisableAuthOnConnect: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the previous driver, we disabled auth on connect and performed auth during OnConnect callback.

We are switching to use CredentialsProviderContext now for auth and we set Protocol to v2 so the driver won't do RESP3 HELLO

Copy link
Contributor

Choose a reason for hiding this comment

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

looking at drive code it will always send HELLO 2 now instead of not doing HELLO at all (on connection), right?

Copy link
Contributor Author

@greedy52 greedy52 Oct 4, 2024

Choose a reason for hiding this comment

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

yes, and the auth will be done using HELLO 2 AUTH <user> <password> instead of just AUTH. In my opinion, it's a win for newer servers. I haven't tested older version (<6.2) tho. The driver suppose to fallback to regular AUTH. We can test that during release testing.

func authWithPasswordOnConnect(username, password string) onClientConnectFunc {
return func(ctx context.Context, conn *redis.Conn) error {
return authConnection(ctx, conn, username, password)
func authWithPasswordOnConnect(username, password string) fetchCredentialsFunc {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simple refactor to change the onConnect functions to credentials provider.

@greedy52 greedy52 marked this pull request as ready for review October 2, 2024 15:41
@github-actions github-actions bot added database-access Database access related issues and PRs size/sm labels Oct 2, 2024
// but Teleport currently only supports RESP2. The client generally
// fallbacks to RESP2 when they receive an unknown command error for the
// HELLO message.
e.maybeHandleFirstHello()
Copy link
Contributor Author

@greedy52 greedy52 Oct 2, 2024

Choose a reason for hiding this comment

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

This logic is caught and verified by TestAccessRedis test (which is basically a bug in our code that we haven't caught before with "older" clients)

lib/srv/db/redis/client.go Outdated Show resolved Hide resolved
Comment on lines -112 to -116
OnConnect: onConnect,

// Auth should be done by the `OnConnect` callback here. So disable
// "automatic" auth by the client.
DisableAuthOnConnect: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

looking at drive code it will always send HELLO 2 now instead of not doing HELLO at all (on connection), right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database-access Database access related issues and PRs no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants