From 30e1bdd290aee8d0f7b815c385188b35f7f08a41 Mon Sep 17 00:00:00 2001 From: Wang Yuan Date: Wed, 14 Jul 2021 21:00:07 +0800 Subject: [PATCH] Use CLUSTERX command to set cluster topology (#324) Currently, we use cluster command to set cluster topology, as we know, we must allow users use cluster command to get nodes and slots, and we only can rename command name instead of subcommand, that is to say, users have a chance to change cluster topology, it is dangerous. Now, we use CLUSTERX command to set cluster topology, you can rename it if you don't want to expose this command to users. --- src/redis_cmd.cc | 78 +++++++++++++++++-------- src/redis_connection.cc | 2 +- tests/tcl/tests/integration/cluster.tcl | 30 +++++----- tests/tcl/tests/unit/command.tcl | 4 +- tools/try_cluster/try_cluster.sh | 4 +- 5 files changed, 75 insertions(+), 43 deletions(-) diff --git a/src/redis_cmd.cc b/src/redis_cmd.cc index 173b90d6a61..8d19ed5b578 100644 --- a/src/redis_cmd.cc +++ b/src/redis_cmd.cc @@ -4140,24 +4140,11 @@ class CommandCluster : public Commander { Status Parse(const std::vector &args) override { subcommand_ = Util::ToLower(args[1]); - if (args.size() == 2 && (subcommand_ == "nodes" || subcommand_ == "version" || - subcommand_ == "slots" || subcommand_ == "info")) return Status::OK(); + if (args.size() == 2 && (subcommand_ == "nodes" || subcommand_ == "slots" + || subcommand_ == "info")) return Status::OK(); if (subcommand_ == "keyslot" && args_.size() == 3) return Status::OK(); - if (subcommand_ == "setnodeid" && args_.size() == 3 && - args_[2].size() == kClusetNodeIdLen) return Status::OK(); - if (subcommand_ == "setnodes" && args_.size() >= 4) { - nodes_str_ = args_[2]; - set_version_ = atoll(args_[3].c_str()); - if (set_version_ < 0) return Status(Status::RedisParseErr, "Invalid version"); - if (args_.size() == 4) return Status::OK(); - if (args_.size() == 5 && strcasecmp(args_[4].c_str(), "force") == 0) { - force_ = true; - return Status::OK(); - } - return Status(Status::RedisParseErr, "Invalid setnodes options"); - } return Status(Status::RedisParseErr, - "CLUSTER command, CLUSTER VERSION|INFO|NODES|SLOTS|KEYSLOT|SETNODEID|SETNODES"); + "CLUSTER command, CLUSTER INFO|NODES|SLOTS|KEYSLOT"); } Status Execute(Server *svr, Connection *conn, std::string *output) override { @@ -4174,13 +4161,6 @@ class CommandCluster : public Commander { if (subcommand_ == "keyslot") { auto slot_id = GetSlotNumFromKey(args_[2]); *output = Redis::Integer(slot_id); - } else if (subcommand_ == "setnodes") { - Status s = svr->cluster_->SetClusterNodes(nodes_str_, set_version_, force_); - if (s.IsOK()) { - *output = Redis::SimpleString("OK"); - } else { - *output = Redis::Error(s.Msg()); - } } else if (subcommand_ == "slots") { std::vector infos; Status s = svr->cluster_->GetSlotsInfo(&infos); @@ -4216,6 +4196,57 @@ class CommandCluster : public Commander { } else { *output = Redis::Error(s.Msg()); } + } else { + *output = Redis::Error("Invalid cluster command options"); + } + return Status::OK(); + } + + private: + std::string subcommand_; +}; + +class CommandClusterX : public Commander { + public: + Status Parse(const std::vector &args) override { + subcommand_ = Util::ToLower(args[1]); + + if (args.size() == 2 && (subcommand_ == "version")) return Status::OK(); + if (subcommand_ == "setnodeid" && args_.size() == 3 && + args_[2].size() == kClusetNodeIdLen) return Status::OK(); + if (subcommand_ == "setnodes" && args_.size() >= 4) { + nodes_str_ = args_[2]; + set_version_ = atoll(args_[3].c_str()); + if (set_version_ < 0) return Status(Status::RedisParseErr, "Invalid version"); + if (args_.size() == 4) return Status::OK(); + if (args_.size() == 5 && strcasecmp(args_[4].c_str(), "force") == 0) { + force_ = true; + return Status::OK(); + } + return Status(Status::RedisParseErr, "Invalid setnodes options"); + } + return Status(Status::RedisParseErr, + "CLUSTERX command, CLUSTERX VERSION|SETNODEID|SETNODES"); + } + + Status Execute(Server *svr, Connection *conn, std::string *output) override { + if (svr->GetConfig()->cluster_enable_ == false) { + *output = Redis::Error("Cluster mode is not enabled"); + return Status::OK(); + } + + if (!conn->IsAdmin()) { + *output = Redis::Error(errAdministorPermissionRequired); + return Status::OK(); + } + + if (subcommand_ == "setnodes") { + Status s = svr->cluster_->SetClusterNodes(nodes_str_, set_version_, force_); + if (s.IsOK()) { + *output = Redis::SimpleString("OK"); + } else { + *output = Redis::Error(s.Msg()); + } } else if (subcommand_ == "setnodeid") { Status s = svr->cluster_->SetNodeId(args_[2]); if (s.IsOK()) { @@ -4406,6 +4437,7 @@ CommandAttributes redisCommandTable[] = { ADD_CMD("sirevrangebyvalue", -4, "read-only", 1, 1, 1, CommandSortedintRevRangeByValue), ADD_CMD("cluster", -2, "cluster", 0, 0, 0, CommandCluster), + ADD_CMD("clusterx", -2, "cluster", 0, 0, 0, CommandClusterX), ADD_CMD("compact", 1, "read-only", 0, 0, 0, CommandCompact), ADD_CMD("bgsave", 1, "read-only", 0, 0, 0, CommandBGSave), diff --git a/src/redis_connection.cc b/src/redis_connection.cc index 2a9d2b3ae94..050a8dc8480 100644 --- a/src/redis_connection.cc +++ b/src/redis_connection.cc @@ -316,7 +316,7 @@ void Connection::ExecuteCommands(const std::vector &to_pro // No lock guard, because 'exec' command has acquired 'WorkExclusivityGuard' } else if (attributes->is_exclusive() || (cmd_name == "config" && cmd_tokens.size() == 2 && !strcasecmp(cmd_tokens[1].c_str(), "set")) || - (config->cluster_enable_ && cmd_name == "cluster" && cmd_tokens.size() >= 2 + (config->cluster_enable_ && cmd_name == "clusterx" && cmd_tokens.size() >= 2 && Cluster::SubCommandIsExecExclusive(cmd_tokens[1]))) { exclusivity = svr_->WorkExclusivityGuard(); } else { diff --git a/tests/tcl/tests/integration/cluster.tcl b/tests/tcl/tests/integration/cluster.tcl index aecf6e2f1d8..fbb9da56d2d 100644 --- a/tests/tcl/tests/integration/cluster.tcl +++ b/tests/tcl/tests/integration/cluster.tcl @@ -18,7 +18,7 @@ start_server {tags {"cluster"} overrides {cluster-enable yes}} { start_server {tags {"cluster"} overrides {cluster-enable yes}} { set nodeid "07c37dfeb235213a872192d90877d0cd55635b91" - r cluster SETNODEID $nodeid + r clusterx SETNODEID $nodeid test {basic function of cluster} { # Cluster is not initialized @@ -28,8 +28,8 @@ start_server {tags {"cluster"} overrides {cluster-enable yes}} { # Set cluster nodes info set port [srv port] set nodes_str "$nodeid 127.0.0.1 $port master - 0-100" - r cluster setnodes $nodes_str 2 - assert_equal 2 [r cluster version] + r clusterx setnodes $nodes_str 2 + assert_equal 2 [r clusterx version] # Get and check cluster nodes info set output_nodes [r cluster nodes] @@ -48,8 +48,8 @@ start_server {tags {"cluster"} overrides {cluster-enable yes}} { # Set cluster nodes info set port [srv port] set nodes_str "07c37dfeb235213a872192d90877d0cd55635b91 127.0.0.1 $port master - 0-200" - r cluster setnodes $nodes_str 1 force - assert_equal 1 [r cluster version] + r clusterx setnodes $nodes_str 1 force + assert_equal 1 [r clusterx version] set output_nodes [r cluster nodes] assert_equal "0-200\n" [lindex [split $output_nodes " "] 8] @@ -59,19 +59,19 @@ start_server {tags {"cluster"} overrides {cluster-enable yes}} { catch {[r cluster no-subcommand]} err assert_match "*CLUSTER*" $err - catch {[r cluster version a]} err + catch {[r clusterx version a]} err assert_match "*CLUSTER*" $err catch {[r cluster nodes a]} err assert_match "*CLUSTER*" $err - catch {[r cluster setnodeid a]} err + catch {[r clusterx setnodeid a]} err assert_match "*CLUSTER*" $err - catch {[r cluster setnodes a]} err + catch {[r clusterx setnodes a]} err assert_match "*CLUSTER*" $err - catch {[r cluster setnodes a -1]} err + catch {[r clusterx setnodes a -1]} err assert_match "*Invalid version*" $err } } @@ -83,8 +83,8 @@ start_server {tags {"cluster"} overrides {cluster-enable yes}} { set port [srv port] set cluster_nodes "$nodeid $host $port master -" set cluster_nodes "${cluster_nodes} 0-1 2 4-8191 8192 8193 10000 10002-11002 16381 16382-16383" - r cluster setnodes "$cluster_nodes" 1 - r cluster setnodeid $nodeid + r clusterx setnodes "$cluster_nodes" 1 + r clusterx setnodeid $nodeid set ret [r cluster slots] assert_equal 5 [llength $ret] @@ -137,10 +137,10 @@ start_server {tags {"cluster"} overrides {cluster-enable yes}} { set cluster_nodes "$cluster_nodes\n$node3_id $node3_host $node3_port slave $node2_id" # node0 doesn't serve any slot, just like a router - $r0 cluster setnodes $cluster_nodes 1 - $r1 cluster setnodes $cluster_nodes 1 - $r2 cluster setnodes $cluster_nodes 1 - $r3 cluster setnodes $cluster_nodes 1 + $r0 clusterx setnodes $cluster_nodes 1 + $r1 clusterx setnodes $cluster_nodes 1 + $r2 clusterx setnodes $cluster_nodes 1 + $r3 clusterx setnodes $cluster_nodes 1 test {cluster info command} { set ret [$r1 cluster info] diff --git a/tests/tcl/tests/unit/command.tcl b/tests/tcl/tests/unit/command.tcl index e5db0ff3224..369aafdc4fe 100644 --- a/tests/tcl/tests/unit/command.tcl +++ b/tests/tcl/tests/unit/command.tcl @@ -1,7 +1,7 @@ start_server {tags {"command"}} { - test {kvrocks has 159 commands currently} { + test {kvrocks has 160 commands currently} { r command count - } {159} + } {160} test {acquire GET command info by COMMAND INFO} { set e [lindex [r command info get] 0] diff --git a/tools/try_cluster/try_cluster.sh b/tools/try_cluster/try_cluster.sh index 53b7ffb74f7..9d59bbd04fb 100755 --- a/tools/try_cluster/try_cluster.sh +++ b/tools/try_cluster/try_cluster.sh @@ -49,8 +49,8 @@ then sed -i.bak "s|dir.*|dir "node_${PORT}"|g" ${conf_file} && rm ${conf_file}.bak $BIN_PATH/kvrocks -c ${conf_file} sleep 0.5 - redis-cli -h 127.0.0.1 -p $PORT cluster setnodeid ${node_id[$index]} - redis-cli -h 127.0.0.1 -p $PORT cluster setnodes "${cluster_nodes}" 1 + redis-cli -h 127.0.0.1 -p $PORT clusterx setnodeid ${node_id[$index]} + redis-cli -h 127.0.0.1 -p $PORT clusterx setnodes "${cluster_nodes}" 1 if [ `expr $index % 2` == 1 ] then redis-cli -h 127.0.0.1 -p $PORT slaveof 127.0.0.1 $((PORT-1))