From 73ff248f41020612e9031e4bd7ce529ef8c5263a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Svensson?= <bjorn.a.svensson@est.tech>
Date: Tue, 14 Jan 2025 13:17:59 +0100
Subject: [PATCH] Handle NIL addresses in CLUSTER SLOTS responses
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

"A NULL value for the endpoint indicates the node has an unknown endpoint and
 the client should connect to the same endpoint it used to send the CLUSTER SLOTS
 command but with the port returned from the command."

Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
---
 hircluster.c                                  | 12 ++++++--
 .../connect-during-cluster-startup-test.sh    | 30 ++++++++++++++++---
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/hircluster.c b/hircluster.c
index bfcf32a..95c11c0 100644
--- a/hircluster.c
+++ b/hircluster.c
@@ -804,8 +804,11 @@ static dict *parse_cluster_slots(redisClusterContext *cc, redisContext *c,
                 elem_ip = elem_nodes->element[0];
                 elem_port = elem_nodes->element[1];
 
+                /* Validate ip and port elements. Accept a NULL value ip (NIL type)
+                 * since we will handle the unknown endpoint special. */
                 if (elem_ip == NULL || elem_port == NULL ||
-                    elem_ip->type != REDIS_REPLY_STRING ||
+                    (elem_ip->type != REDIS_REPLY_STRING &&
+                     elem_ip->type != REDIS_REPLY_NIL) ||
                     elem_port->type != REDIS_REPLY_INTEGER ||
                     !hi_valid_port((int)elem_port->integer)) {
                     __redisClusterSetError(
@@ -815,8 +818,11 @@ static dict *parse_cluster_slots(redisClusterContext *cc, redisContext *c,
                     goto error;
                 }
 
-                /* Get the received ip/host. According to the docs an empty string can
-                 * be treated as it means the same address we sent this command to. */
+                /* Get the received ip/host. According to the docs an unknown
+                 * endpoint or an empty string can be treated as it means
+                 * the same address as we sent this command to.
+                 * An unknown endpoint has the type REDIS_REPLY_NIL and its
+                 * length is initiated to zero. */
                 char *host = (elem_ip->len > 0) ? elem_ip->str : c->tcp.host;
                 if (host == NULL) {
                     goto oom;
diff --git a/tests/scripts/connect-during-cluster-startup-test.sh b/tests/scripts/connect-during-cluster-startup-test.sh
index a9ef3fa..31bd2bc 100755
--- a/tests/scripts/connect-during-cluster-startup-test.sh
+++ b/tests/scripts/connect-during-cluster-startup-test.sh
@@ -4,6 +4,7 @@
 #
 # The first attempt to get the slotmap will receive a reply without any
 # slot information and this should result in a retry.
+# The following slotmap updates tests the handling of an nil/empty IP address.
 #
 # The client is configured to use the CLUSTER SLOTS command.
 #
@@ -18,16 +19,22 @@ syncpid=$!
 
 # Start simulated server.
 timeout 5s ./simulated-redis.pl -p 7400 -d --sigcont $syncpid <<'EOF' &
-# The initial slotmap is not covering any slots, expect a retry.
+# The initial slotmap is not covering any slots, expect a retry since it's not accepted.
 EXPECT CONNECT
 EXPECT ["CLUSTER", "SLOTS"]
 SEND []
 
-# The node has now been delegated slots.
+# The node has now been delegated a few slots and should be accepted.
+# Respond with an unknown endpoint (nil) to test that current connection IP is used instead.
+EXPECT ["CLUSTER", "SLOTS"]
+SEND *1\r\n*3\r\n:0\r\n:10\r\n*3\r\n$-1\r\n:7400\r\n$40\r\nf5378fa2ad1fbd569f01ba2fe29fa8feb36cdfb8\r\n
+
+# The node has now been delegated all slots.
+# Use empty address to test that current connection IP is used instead.
 EXPECT ["CLUSTER", "SLOTS"]
 SEND [[0, 16383, ["", 7400, "f5378fa2ad1fbd569f01ba2fe29fa8feb36cdfb8"]]]
 
-EXPECT ["SET", "foo", "bar"]
+EXPECT ["SET", "foo", "bar3"]
 SEND +OK
 EXPECT CLOSE
 EOF
@@ -38,7 +45,19 @@ wait $syncpid;
 
 # Run client which will fetch the initial slotmap asynchronously.
 timeout 3s "$clientprog" --events --async-initial-update 127.0.0.1:7400 > "$testname.out" <<'EOF'
-SET foo bar
+# Slot not yet handled, will trigger a slotmap update which will be throttled.
+SET foo bar1
+
+# Wait to avoid slotmap update throttling.
+!sleep
+
+# A command will fail direcly, but a slotmap update is scheduled.
+SET foo bar2
+
+# Allow slotmap update to finish.
+!sleep
+
+SET foo bar3
 EOF
 clientexit=$?
 
@@ -59,6 +78,9 @@ fi
 # Check the output from the client.
 expected="Event: slotmap-updated
 Event: ready
+error: slot not served by any node
+error: slot not served by any node
+Event: slotmap-updated
 OK
 Event: free-context"