When redis-cli received ASK, it didn't handle it (#8930)

When redis-cli received ASK, it used string matching wrong and didn't
handle it. 

When we access a slot which is in migrating state, it maybe
return ASK. After redirect to the new node, we need send ASKING
command before retry the command.  In this PR after redis-cli receives 
ASK, we send a ASKING command before send the origin command 
after reconnecting.

Other changes:
* Make redis-cli -u and -c (unix socket and cluster mode) incompatible 
  with one another.
* When send command fails, we avoid the 2nd reconnect retry and just
  print the error info. Users will decide how to do next. 
  See #9277.
* Add a test faking two redis nodes in TCL to just send ASK and OK in 
  redis protocol to test ASK behavior. 

Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Oran Agra <oran@redislabs.com>

3 files changed, 135 insertions, 17 deletions

diff --git a/src/redis-cli.c b/src/redis-cli.c
index 0ca1809ab..edf695ec4 100644
--- a/src/redis-cli.c
+++ b/src/redis-cli.c
@@ -220,6 +220,7 @@ static struct config {
     long long lru_test_sample_size;
     int cluster_mode;
     int cluster_reissue_command;
+    int cluster_send_asking;
     int slave_mode;
     int pipe_mode;
     int pipe_timeout;
@@ -931,6 +932,29 @@ static int cliConnect(int flags) {
     return REDIS_OK;
 }
 
+/* In cluster, if server replies ASK, we will redirect to a different node.
+ * Before sending the real command, we need to send ASKING command first. */
+static int cliSendAsking() {
+    redisReply *reply;
+
+    config.cluster_send_asking = 0;
+    if (context == NULL) {
+        return REDIS_ERR;
+    }
+    reply = redisCommand(context,"ASKING");
+    if (reply == NULL) {
+        fprintf(stderr, "\nI/O error\n");
+        return REDIS_ERR;
+    }
+    int result = REDIS_OK;
+    if (reply->type == REDIS_REPLY_ERROR) {
+        result = REDIS_ERR;
+        fprintf(stderr,"ASKING failed: %s\n",reply->str);
+    }
+    freeReplyObject(reply);
+    return result;
+}
+
 static void cliPrintContextError(void) {
     if (context == NULL) return;
     fprintf(stderr,"Error: %s\n",context->errstr);
@@ -1306,7 +1330,7 @@ static int cliReadReply(int output_raw_strings) {
     /* Check if we need to connect to a different node and reissue the
      * request. */
     if (config.cluster_mode && reply->type == REDIS_REPLY_ERROR &&
-        (!strncmp(reply->str,"MOVED",5) || !strcmp(reply->str,"ASK")))
+        (!strncmp(reply->str,"MOVED ",6) || !strncmp(reply->str,"ASK ",4)))
     {
         char *p = reply->str, *s;
         int slot;
@@ -1330,6 +1354,9 @@ static int cliReadReply(int output_raw_strings) {
             printf("-> Redirected to slot [%d] located at %s:%d\n",
                 slot, config.hostip, config.hostport);
         config.cluster_reissue_command = 1;
+        if (!strncmp(reply->str,"ASK ",4)) {
+            config.cluster_send_asking = 1;
+        }
         cliRefreshPrompt();
     } else if (!config.interactive && config.set_errcode && 
         reply->type == REDIS_REPLY_ERROR) 
@@ -1819,6 +1846,11 @@ static int parseOptions(int argc, char **argv) {
         }
     }
 
+    if (config.hostsocket && config.cluster_mode) {
+        fprintf(stderr,"Options -c and -s are mutually exclusive.\n");
+        exit(1);
+    }
+
     /* --ldb requires --eval. */
     if (config.eval_ldb && config.eval == NULL) {
         fprintf(stderr,"Options --ldb and --ldb-sync-mode require --eval.\n");
@@ -2037,26 +2069,32 @@ static sds *getSdsArrayFromArgv(int argc, char **argv, int quoted) {
 
 static int issueCommandRepeat(int argc, char **argv, long repeat) {
     while (1) {
+        if (config.cluster_reissue_command || context == NULL ||
+            context->err == REDIS_ERR_IO || context->err == REDIS_ERR_EOF)
+        {
+            if (cliConnect(CC_FORCE) != REDIS_OK) {
+                cliPrintContextError();
+                config.cluster_reissue_command = 0;
+                return REDIS_ERR;
+            }
+        }
         config.cluster_reissue_command = 0;
-        if (cliSendCommand(argc,argv,repeat) != REDIS_OK) {
-            cliConnect(CC_FORCE);
-
-            /* If we still cannot send the command print error.
-             * We'll try to reconnect the next time. */
-            if (cliSendCommand(argc,argv,repeat) != REDIS_OK) {
+        if (config.cluster_send_asking) {
+            if (cliSendAsking() != REDIS_OK) {
                 cliPrintContextError();
                 return REDIS_ERR;
             }
         }
+        if (cliSendCommand(argc,argv,repeat) != REDIS_OK) {
+            cliPrintContextError();
+            return REDIS_ERR;
+        }
 
         /* Issue the command again if we got redirected in cluster mode */
         if (config.cluster_mode && config.cluster_reissue_command) {
-            /* If cliConnect fails, sleep for a while and try again. */
-            if (cliConnect(CC_FORCE) != REDIS_OK)
-                sleep(1);
-        } else {
-            break;
+            continue;
         }
+        break;
     }
     return REDIS_OK;
 }
@@ -8301,6 +8339,7 @@ int main(int argc, char **argv) {
     config.lru_test_mode = 0;
     config.lru_test_sample_size = 0;
     config.cluster_mode = 0;
+    config.cluster_send_asking = 0;
     config.slave_mode = 0;
     config.getrdb_mode = 0;
     config.stat_mode = 0;
diff --git a/tests/helpers/fake_redis_node.tcl b/tests/helpers/fake_redis_node.tcl
new file mode 100644
index 000000000..a12d87fed
--- /dev/null
+++ b/tests/helpers/fake_redis_node.tcl
@@ -0,0 +1,58 @@
+# A fake Redis node for replaying predefined/expected traffic with a client.
+#
+# Usage: tclsh fake_redis_node.tcl PORT COMMAND REPLY [ COMMAND REPLY [ ... ] ]
+#
+# Commands are given as space-separated strings, e.g. "GET foo", and replies as
+# RESP-encoded replies minus the trailing \r\n, e.g. "+OK".
+
+set port [lindex $argv 0];
+set expected_traffic [lrange $argv 1 end];
+
+# Reads and parses a command from a socket and returns it as a space-separated
+# string, e.g. "set foo bar".
+proc read_command {sock} {
+    set char [read $sock 1]
+    switch $char {
+        * {
+            set numargs [gets $sock]
+            set result {}
+            for {set i 0} {$i<$numargs} {incr i} {
+                read $sock 1;       # dollar sign
+                set len [gets $sock]
+                set str [read $sock $len]
+                gets $sock;         # trailing \r\n
+                lappend result $str
+            }
+            return $result
+        }
+        {} {
+            # EOF
+            return {}
+        }
+        default {
+            # Non-RESP command
+            set rest [gets $sock]
+            return "$char$rest"
+        }
+    }
+}
+
+proc accept {sock host port} {
+    global expected_traffic
+    foreach {expect_cmd reply} $expected_traffic {
+        if {[eof $sock]} {break}
+        set cmd [read_command $sock]
+        if {[string equal -nocase $cmd $expect_cmd]} {
+            puts $sock $reply
+            flush $sock
+        } else {
+            puts $sock "-ERR unexpected command $cmd"
+            break
+        }
+    }
+    close $sock
+}
+
+socket -server accept $port
+after 5000 set done timeout
+vwait done
diff --git a/tests/integration/redis-cli.tcl b/tests/integration/redis-cli.tcl
index db525e405..8ffa154a7 100644
--- a/tests/integration/redis-cli.tcl
+++ b/tests/integration/redis-cli.tcl
@@ -85,8 +85,8 @@ start_server {tags {"cli"}} {
         set _ $tmp
     }
 
-    proc _run_cli {opts args} {
-        set cmd [rediscli [srv host] [srv port] [list -n $::dbnum {*}$args]]
+    proc _run_cli {host port db opts args} {
+        set cmd [rediscli $host $port [list -n $db {*}$args]]
         foreach {key value} $opts {
             if {$key eq "pipe"} {
                 set cmd "sh -c \"$value | $cmd\""
@@ -105,15 +105,19 @@ start_server {tags {"cli"}} {
     }
 
     proc run_cli {args} {
-        _run_cli {} {*}$args
+        _run_cli [srv host] [srv port] $::dbnum {} {*}$args
     }
 
     proc run_cli_with_input_pipe {cmd args} {
-        _run_cli [list pipe $cmd] -x {*}$args
+        _run_cli [srv host] [srv port] $::dbnum [list pipe $cmd] -x {*}$args
     }
 
     proc run_cli_with_input_file {path args} {
-        _run_cli [list path $path] -x {*}$args
+        _run_cli [srv host] [srv port] $::dbnum [list path $path] -x {*}$args
+    }
+
+    proc run_cli_host_port_db {host port db args} {
+        _run_cli $host $port $db {} {*}$args
     }
 
     proc test_nontty_cli {name code} {
@@ -230,6 +234,23 @@ start_server {tags {"cli"}} {
         assert_equal "foo\nbar" [run_cli lrange list 0 -1]
     }
 
+    test_nontty_cli "ASK redirect test" {
+        # Set up two fake Redis nodes.
+        set tclsh [info nameofexecutable]
+        set script "tests/helpers/fake_redis_node.tcl"
+        set port1 [find_available_port $::baseport $::portcount]
+        set port2 [find_available_port $::baseport $::portcount]
+        set p1 [exec $tclsh $script $port1 \
+                "SET foo bar" "-ASK 12182 127.0.0.1:$port2" &]
+        set p2 [exec $tclsh $script $port2 \
+                "ASKING" "+OK" \
+                "SET foo bar" "+OK" &]
+        # Sleep to make sure both fake nodes have started listening
+        after 100
+        # Run the cli
+        assert_equal "OK" [run_cli_host_port_db "127.0.0.1" $port1 0 -c SET foo bar]
+    }
+
     test_nontty_cli "Quoted input arguments" {
         r set "\x00\x00" "value"
         assert_equal "value" [run_cli --quoted-input get {"\x00\x00"}]