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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/nc_message.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,6 @@ _msg_get(void)
msg->token = NULL;

msg->parser = NULL;
msg->add_auth = NULL;
msg->result = MSG_PARSE_OK;

msg->fragment = NULL;
Expand Down Expand Up @@ -293,7 +292,6 @@ msg_get(struct conn *conn, bool request, bool redis)
} else {
msg->parser = redis_parse_rsp;
}
msg->add_auth = redis_add_auth;
msg->fragment = redis_fragment;
msg->reply = redis_reply;
msg->failure = redis_failure;
Expand All @@ -305,7 +303,6 @@ msg_get(struct conn *conn, bool request, bool redis)
} else {
msg->parser = memcache_parse_rsp;
}
msg->add_auth = memcache_add_auth;
msg->fragment = memcache_fragment;
msg->failure = memcache_failure;
msg->pre_coalesce = memcache_pre_coalesce;
Expand Down
2 changes: 0 additions & 2 deletions src/nc_message.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#include <nc_core.h>

typedef void (*msg_parse_t)(struct msg *);
typedef rstatus_t (*msg_add_auth_t)(struct context *ctx, struct conn *c_conn, struct conn *s_conn);
typedef rstatus_t (*msg_fragment_t)(struct msg *, uint32_t, struct msg_tqh *);
typedef void (*msg_coalesce_t)(struct msg *r);
typedef rstatus_t (*msg_reply_t)(struct msg *r);
Expand Down Expand Up @@ -223,7 +222,6 @@ struct msg {

msg_fragment_t fragment; /* message fragment */
msg_reply_t reply; /* generate message reply (example: ping) */
msg_add_auth_t add_auth; /* add auth message when we forward msg */
msg_failure_t failure; /* transient failure response? */

msg_coalesce_t pre_coalesce; /* message pre-coalesce */
Expand Down
9 changes: 0 additions & 9 deletions src/nc_request.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,15 +593,6 @@ req_forward(struct context *ctx, struct conn *c_conn, struct msg *msg)
}
}

if (!conn_authenticated(s_conn)) {
status = msg->add_auth(ctx, c_conn, s_conn);
if (status != NC_OK) {
req_forward_error(ctx, c_conn, msg);
s_conn->err = errno;
return;
}
}

s_conn->enqueue_inq(ctx, s_conn, msg);

req_forward_stats(ctx, s_conn->owner, msg);
Expand Down
7 changes: 0 additions & 7 deletions src/proto/nc_memcache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1545,13 +1545,6 @@ memcache_swallow_msg(struct conn *conn, struct msg *pmsg, struct msg *msg)
{
}

rstatus_t
memcache_add_auth(struct context *ctx, struct conn *c_conn, struct conn *s_conn)
{
NOT_REACHED();
return NC_OK;
}

rstatus_t
memcache_reply(struct msg *r)
{
Expand Down
2 changes: 0 additions & 2 deletions src/proto/nc_proto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ void memcache_parse_rsp(struct msg *r);
bool memcache_failure(struct msg *r);
void memcache_pre_coalesce(struct msg *r);
void memcache_post_coalesce(struct msg *r);
rstatus_t memcache_add_auth(struct context *ctx, struct conn *c_conn, struct conn *s_conn);
rstatus_t memcache_fragment(struct msg *r, uint32_t ncontinuum, struct msg_tqh *frag_msgq);
rstatus_t memcache_reply(struct msg *r);
void memcache_post_connect(struct context *ctx, struct conn *conn, struct server *server);
Expand All @@ -154,7 +153,6 @@ void redis_parse_rsp(struct msg *r);
bool redis_failure(struct msg *r);
void redis_pre_coalesce(struct msg *r);
void redis_post_coalesce(struct msg *r);
rstatus_t redis_add_auth(struct context *ctx, struct conn *c_conn, struct conn *s_conn);
rstatus_t redis_fragment(struct msg *r, uint32_t ncontinuum, struct msg_tqh *frag_msgq);
rstatus_t redis_reply(struct msg *r);
void redis_post_connect(struct context *ctx, struct conn *conn, struct server *server);
Expand Down
59 changes: 43 additions & 16 deletions src/proto/nc_redis.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2837,20 +2837,23 @@ redis_handle_auth_req(struct msg *req, struct msg *rsp)
}

rstatus_t
redis_add_auth(struct context *ctx, struct conn *c_conn, struct conn *s_conn)
redis_add_auth(struct context *ctx, struct conn *conn, struct server *server, int *sended)
{
rstatus_t status;
struct msg *msg;
struct server_pool *pool;

ASSERT(!s_conn->client && !s_conn->proxy);
ASSERT(!conn_authenticated(s_conn));
ASSERT(!conn->client && !conn->proxy && conn->connected && conn->redis);
ASSERT(sended);

pool = c_conn->owner;
pool = server->owner;
if (!pool->require_auth) {
return NC_OK;
}

msg = msg_get(c_conn, true, c_conn->redis);
msg = msg_get(conn, true, conn->redis);
if (msg == NULL) {
c_conn->err = errno;
conn->err = errno;
return NC_ENOMEM;
}

Expand All @@ -2861,23 +2864,33 @@ redis_add_auth(struct context *ctx, struct conn *c_conn, struct conn *s_conn)
return status;
}

msg->type = MSG_REQ_REDIS_AUTH;
msg->result = MSG_PARSE_OK;
msg->swallow = 1;
s_conn->enqueue_inq(ctx, s_conn, msg);
s_conn->authenticated = 1;
msg->owner = NULL;

conn->authenticated = 1;

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

log_debug(LOG_NOTICE, "sent 'AUTH %s' to %s | %s", pool->redis_auth.data,
pool->name.data, server->name.data);

return NC_OK;
}

void
redis_post_connect(struct context *ctx, struct conn *conn, struct server *server)
rstatus_t
redis_select_db(struct context *ctx, struct conn *conn, struct server *server, int *sended)
{
rstatus_t status;
struct server_pool *pool = server->owner;
struct msg *msg;
int digits;

ASSERT(!conn->client && conn->connected);
ASSERT(conn->redis);
ASSERT(!conn->client && conn->connected && conn->redis);
ASSERT(sended);

/*
* By default, every connection to redis uses the database DB 0. You
Expand All @@ -2886,7 +2899,7 @@ redis_post_connect(struct context *ctx, struct conn *conn, struct server *server
* on a per pool basis in the configuration
*/
if (pool->redis_db <= 0) {
return;
return NC_OK;
}

/*
Expand All @@ -2896,14 +2909,15 @@ redis_post_connect(struct context *ctx, struct conn *conn, struct server *server
*/
msg = msg_get(conn, true, conn->redis);
if (msg == NULL) {
return;
conn->err = errno;
return NC_ENOMEM;
}

digits = (pool->redis_db >= 10) ? (int)log10(pool->redis_db) + 1 : 1;
status = msg_prepend_format(msg, "*2\r\n$6\r\nSELECT\r\n$%d\r\n%d\r\n", digits, pool->redis_db);
if (status != NC_OK) {
msg_put(msg);
return;
return status;
}
msg->type = MSG_REQ_REDIS_SELECT;
msg->result = MSG_PARSE_OK;
Expand All @@ -2912,10 +2926,23 @@ redis_post_connect(struct context *ctx, struct conn *conn, struct server *server

/* enqueue as head and send */
req_server_enqueue_imsgq_head(ctx, conn, msg);
msg_send(ctx, conn);
*sended = 1;

log_debug(LOG_NOTICE, "sent 'SELECT %d' to %s | %s", pool->redis_db,
pool->name.data, server->name.data);

return NC_OK;
}

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

Copy link

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_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)

Copy link

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

redis_add_auth(ctx, conn, server, &sended);
if (sended) {
msg_send(ctx, conn);
}
}

void
Expand Down