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

Fix Bug: redis_auth don't work with redis_db #458

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

charsyam
Copy link
Contributor

@charsyam charsyam commented Mar 9, 2016

currently, twemproxy ignore "redis_db" when "redis_auth" is set.
select command is set as noforward because of auth.

this patch fix this.
and move add_auth to post_connected.

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2019

CLA assistant check
All committers have signed the CLA.

@Hemant-Mann
Copy link

thanks for the fix @charsyam

void
redis_post_connect(struct context *ctx, struct conn *conn, struct server *server)
{
int sended = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
int sended = 0;
int sent = 0;

nit: https://www.merriam-webster.com/dictionary/sent seems more appropriate for a variable name here and elsewhere.

I had a lot of other changes planned for 0.6.0 (including merging in the heartbeat, failover, sentinel patches #608) and it may or may not be necessary to refactor this after that

Unrelated: Is it possible to rebase and add an integration test of this

At a glance, it seems like the right approach? I assume calling SELECT before AUTH is allowed if it worked for you, not familliar with redis auth

Choose a reason for hiding this comment

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

Hey @TysonAndre

So basically calling SELECT before AUTH does not work in redis but the issue is what @charsyam mentioned

currently, twemproxy ignore "redis_db" when "redis_auth" is set.
select command is set as noforward because of auth.

once the connection is authenticated we can change the redis db using the SELECT command and this PR does fix that, I've verified in testing

Copy link
Collaborator

Choose a reason for hiding this comment

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

currently, twemproxy ignore "redis_db" when "redis_auth" is set.
select command is set as noforward because of auth.

Oh, this part. Makes sense.

// src/nc_request.c
static bool
req_filter(struct conn *conn, struct msg *msg)
{
/* ... */
    /*
     * If this conn is not authenticated, we will mark it as noforward,
     * and handle it in the redis_reply handler.
     */
    if (!conn_authenticated(conn)) {
        msg->noforward = 1;
    }

redis_post_connect(struct context *ctx, struct conn *conn, struct server *server)
{
int sended = 0;
redis_select_db(ctx, conn, server, &sended);
Copy link
Collaborator

Choose a reason for hiding this comment

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

    /* enqueue as head and send */
    req_server_enqueue_imsgq_head(ctx, conn, msg);

Oh, I see, both of these will set the new message to be the head of the queue. So I misread it at first, it's actually sending the command to add auth before selecting the db number.

(though I assume moving the auth check out of req_forward was the actual fix)

Choose a reason for hiding this comment

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

it's actually sending the command to add auth before selecting the db number.

Exactly

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

Successfully merging this pull request may close these issues.

4 participants