From 52b82fa7d8bbf62d5d544b5f8bb59dd63d2ca95e Mon Sep 17 00:00:00 2001 From: proost Date: Sat, 9 Nov 2024 23:23:06 +0900 Subject: [PATCH 1/2] fix: inconsistency moved readonly client Signed-off-by: proost --- src/script.c | 27 ++++++++++++++++++++++++ tests/unit/cluster/scripting.tcl | 36 ++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/src/script.c b/src/script.c index f1d0a8fb79..6cd87913a4 100644 --- a/src/script.c +++ b/src/script.c @@ -373,6 +373,33 @@ static int scriptVerifyWriteCommandAllow(scriptRunCtx *run_ctx, char **err) { * of this script. */ int deny_write_type = writeCommandsDeniedByDiskError(); + /* + * If client is readonly and server is a replica, we should not allow write-commands. + * we should redirect the client. + */ + if (run_ctx->original_client->flag.readonly && server.primary_host) { + client *c = run_ctx->c; + client *original_c = run_ctx->original_client; + + /* + * Duplicate relevant flags in the script client. + */ + c->flag.readonly = original_c->flag.readonly; + c->flag.asking = original_c->flag.asking; + + int error_code; + int hashslot = -1; + clusterNode *n = getNodeByQuery(c, c->cmd, c->argv, c->argc, &hashslot, &error_code); + if (n == NULL || !clusterNodeIsMyself(n)) { + if (error_code == CLUSTER_REDIR_MOVED || error_code == CLUSTER_REDIR_ASK) { + int port = clusterNodeClientPort(n, connIsTLS(original_c->conn)); + *err = sdscatprintf(sdsempty(), "-%s %d %s:%d", (error_code == CLUSTER_REDIR_ASK) ? "ASK" : "MOVED", + hashslot, clusterNodePreferredEndpoint(n, c), port); + return C_ERR; + } + } + } + if (server.primary_host && server.repl_replica_ro && !mustObeyClient(run_ctx->original_client)) { *err = sdsdup(shared.roreplicaerr->ptr); return C_ERR; diff --git a/tests/unit/cluster/scripting.tcl b/tests/unit/cluster/scripting.tcl index 88e158afc5..c753416476 100644 --- a/tests/unit/cluster/scripting.tcl +++ b/tests/unit/cluster/scripting.tcl @@ -97,3 +97,39 @@ start_cluster 1 0 {tags {external:skip cluster}} { assert_equal [lsort [r 0 cluster shards]] [lsort [r 0 eval "return redis.call('cluster', 'shards')" 0]] } } + +start_cluster 2 2 {tags {external:skip cluster}} { + test "Cluster should start ok" { + wait_for_cluster_state ok + } + + set primary1 [srv 0 "client"] + set primary2 [srv -1 "client"] + set replica1 [srv -2 "client"] + set replica2 [srv -3 "client"] + + set slot_for_foo [$primary2 cluster keyslot foo] + + test "Read-only client that sends lua script which has write command on replica get MOVED error" { + assert_equal {OK} [$replica2 READONLY] + + catch { + $replica2 eval "redis.call('set', 'foo', 'bar')" 0 + } e + + assert_match {*MOVED*} $e + } + + test "Read-only client that sends lua script which has write command on replica get ASK error during migration" { + assert_equal {OK} [$primary1 cluster setslot $slot_for_foo importing [$primary2 cluster myid]] + assert_equal {OK} [$primary2 cluster setslot $slot_for_foo migrating [$primary1 cluster myid]] + + assert_equal {OK} [$replica2 READONLY] + + catch { + $replica2 eval "redis.call('set', 'foo', 'bar')" 0 + } e + + assert_match {*ASK*} $e + } +} From 39df99036adbae905efeccece7e3d84b68d288b9 Mon Sep 17 00:00:00 2001 From: proost Date: Sun, 29 Dec 2024 18:56:37 +0900 Subject: [PATCH 2/2] fix: change error message Signed-off-by: proost --- src/script.c | 6 ++---- tests/unit/cluster/scripting.tcl | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/script.c b/src/script.c index 6cd87913a4..503e58d484 100644 --- a/src/script.c +++ b/src/script.c @@ -375,7 +375,6 @@ static int scriptVerifyWriteCommandAllow(scriptRunCtx *run_ctx, char **err) { /* * If client is readonly and server is a replica, we should not allow write-commands. - * we should redirect the client. */ if (run_ctx->original_client->flag.readonly && server.primary_host) { client *c = run_ctx->c; @@ -392,9 +391,8 @@ static int scriptVerifyWriteCommandAllow(scriptRunCtx *run_ctx, char **err) { clusterNode *n = getNodeByQuery(c, c->cmd, c->argv, c->argc, &hashslot, &error_code); if (n == NULL || !clusterNodeIsMyself(n)) { if (error_code == CLUSTER_REDIR_MOVED || error_code == CLUSTER_REDIR_ASK) { - int port = clusterNodeClientPort(n, connIsTLS(original_c->conn)); - *err = sdscatprintf(sdsempty(), "-%s %d %s:%d", (error_code == CLUSTER_REDIR_ASK) ? "ASK" : "MOVED", - hashslot, clusterNodePreferredEndpoint(n, c), port); + *err = sdsnew("Script attempted to access a non local key in a " + "cluster node"); return C_ERR; } } diff --git a/tests/unit/cluster/scripting.tcl b/tests/unit/cluster/scripting.tcl index c753416476..e7c5e3d262 100644 --- a/tests/unit/cluster/scripting.tcl +++ b/tests/unit/cluster/scripting.tcl @@ -117,7 +117,7 @@ start_cluster 2 2 {tags {external:skip cluster}} { $replica2 eval "redis.call('set', 'foo', 'bar')" 0 } e - assert_match {*MOVED*} $e + assert_match {*Script attempted to access a non local ke*} $e } test "Read-only client that sends lua script which has write command on replica get ASK error during migration" { @@ -130,6 +130,6 @@ start_cluster 2 2 {tags {external:skip cluster}} { $replica2 eval "redis.call('set', 'foo', 'bar')" 0 } e - assert_match {*ASK*} $e + assert_match {*Script attempted to access a non local ke*} $e } }