Allow Module authentication to succeed when cluster is down (#693)

Module Authentication using a blocking implementation currently gets rejected when the "cluster is down" from the client timeout cron job (`clientsCronHandleTimeout`). This PR exempts clients blocked on Module Authentication from being rejected here. --------- Signed-off-by: KarthikSubbarao <[email protected]>
valkey-io · Jul 1, 2024 · fa01a29 · fa01a29
1 parent 9f4f603
commit fa01a29
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 11 deletions.
diff --git a/src/cluster.c b/src/cluster.c
@@ -1218,6 +1218,10 @@ int clusterRedirectBlockedClientIfNeeded(client *c) {
         dictEntry *de;
         dictIterator *di;
 
+        /* If the client is blocked on module, but not on a specific key,
+         * don't unblock it. */
+        if (c->bstate.btype == BLOCKED_MODULE && !moduleClientIsBlockedOnKeys(c)) return 0;
+
         /* If the cluster is down, unblock the client with the right error.
          * If the cluster is configured to allow reads on cluster down, we
          * still want to emit this error since a write will be required
@@ -1227,10 +1231,6 @@ int clusterRedirectBlockedClientIfNeeded(client *c) {
             return 1;
         }
 
-        /* If the client is blocked on module, but not on a specific key,
-         * don't unblock it (except for the CLUSTER_FAIL case above). */
-        if (c->bstate.btype == BLOCKED_MODULE && !moduleClientIsBlockedOnKeys(c)) return 0;
-
         /* All keys must belong to the same slot, so check first key only. */
         di = dictGetIterator(c->bstate.keys);
         if ((de = dictNext(di)) != NULL) {

diff --git a/tests/unit/moduleapi/cluster.tcl b/tests/unit/moduleapi/cluster.tcl
@@ -8,8 +8,9 @@ tags {tls:skip external:skip cluster modules} {
 set testmodule_nokey [file normalize tests/modules/blockonbackground.so]
 set testmodule_blockedclient [file normalize tests/modules/blockedclient.so]
 set testmodule [file normalize tests/modules/blockonkeys.so]
+set testmodule_auth [file normalize tests/modules/auth.so]
 
-set modules [list loadmodule $testmodule loadmodule $testmodule_nokey loadmodule $testmodule_blockedclient]
+set modules [list loadmodule $testmodule loadmodule $testmodule_nokey loadmodule $testmodule_blockedclient loadmodule $testmodule_auth]
 start_cluster 3 0 [list config_lines $modules] {
 
     set node1 [srv 0 client]
@@ -146,18 +147,27 @@ start_cluster 3 0 [list config_lines $modules] {
         assert_error {*CLUSTERDOWN*} {$node1_rd read}
     }
 
-    test "Verify command (no keys) got unblocked after cluster failure" {
-        assert_error {*CLUSTERDOWN*} {$node2_rd read}
-
-        # verify there are no blocked clients
-        assert_equal [s 0 blocked_clients]  {0}
-        assert_equal [s -1 blocked_clients]  {0}
+    test "Verify command (with no keys) is not unblocked after cluster failure" {
+        assert_no_match {*CLUSTERDOWN*} {$node2_rd read}
+        # verify there are blocked clients
+        assert_equal [s -1 blocked_clients]  {1}
     }
 
     test "Verify command RM_Call is rejected when cluster is down" {
         assert_error "ERR Can not execute a command 'set' while the cluster is down" {$node1 do_rm_call set x 1}
     }
 
+    test "Verify Module Auth Succeeds when cluster is down" {
+        r acl setuser foo >pwd on ~* &* +@all
+        assert_error "*CLUSTERDOWN*" {r set x 1}
+        # Non Blocking Module Auth
+        assert_equal {OK} [r testmoduleone.rm_register_auth_cb]
+        assert_equal {OK} [r AUTH foo allow]
+        # Blocking Module Auth
+        assert_equal {OK} [r testmoduleone.rm_register_blocking_auth_cb]
+        assert_equal {OK} [r AUTH foo block_allow]
+    }
+
     resume_process $node3_pid
     $node1_rd close
     $node2_rd close