Fix Bug: redis_auth don't work with redis_db #458
Conversation
|
|
|
thanks for the fix @charsyam |
| void | ||
| redis_post_connect(struct context *ctx, struct conn *conn, struct server *server) | ||
| { | ||
| int sended = 0; |
There was a problem hiding this comment.
| 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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
/* 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)
There was a problem hiding this comment.
it's actually sending the command to add auth before selecting the db number.
Exactly
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.