DR-2672 - Dummy stub for client setinfo and ignoring private client a…

…uth message in pubsub response (#39) * DR-2672 - Dummy stub for client setinfo * DR-2674 - Ignoring private client auth resp in pubsub flow * Adding comment for the additional condition
freshworks · Nov 15, 2024 · 3ac8acf · 3ac8acf
1 parent f435e3a
commit 3ac8acf
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 7 deletions.
diff --git a/source/extensions/filters/network/common/redis/supported_commands.h b/source/extensions/filters/network/common/redis/supported_commands.h
@@ -81,6 +81,10 @@ struct SupportedCommands {
     CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set<std::string>, "subscribe", "psubscribe", "unsubscribe", "punsubscribe","quit", "ping");
   }
 
+  static const absl::flat_hash_set<std::string>& subcrStateNoArgallowedCommands() {
+    CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set<std::string>, "unsubscribe", "punsubscribe","quit", "ping");
+  }
+
   /**
    * @return commands that make a client to enter subscribed state.
    */
@@ -113,7 +117,7 @@ struct SupportedCommands {
   * @return client sub commands thats supported 
   */
   static const absl::flat_hash_set<std::string>& clientSubCommands() {
-    CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set<std::string>, "getname","setname");
+    CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set<std::string>, "getname","setname", "setinfo");
   }
 
     /**

diff --git a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc
@@ -936,9 +936,13 @@ SplitRequestPtr PubSubRequest::create(Router& router, Common::Redis::RespValuePt
   int32_t redisShardsCount=0;
 
   if (Common::Redis::SupportedCommands::subscriptionCommands().contains(command_name) && incoming_request->asArray().size() < 2) {
-    ENVOY_LOG(debug, "Invalid request: '{}'", incoming_request->toString());
-    callbacks.onResponse(Common::Redis::Utility::makeError(Response::get().InvalidRequest));
-    return nullptr;
+    if (!Common::Redis::SupportedCommands::subcrStateNoArgallowedCommands().contains(command_name)) {
+      ENVOY_LOG(debug, "Invalid request: '{}'", incoming_request->toString());
+      callbacks.onResponse(Common::Redis::Utility::makeError(Response::get().InvalidRequest));
+      return nullptr;
+    } else {
+      ENVOY_LOG(debug, "No arguments acceptable for command: '{}'", command_name);
+    }
   }
 
 
@@ -1118,9 +1122,21 @@ void PubSubMessageHandler::handleChannelMessageCustom(Common::Redis::RespValuePt
   ENVOY_LOG(debug, "message received on channel '{}' on shardIndex : '{}'", value->toString(),shardIndex);
 
   if (value->type() != Common::Redis::RespType::Array ||
-      value->asArray()[0].type() != Common::Redis::RespType::BulkString) {
-    ENVOY_LOG(error, "unexpected message format: '{}'", value->toString());
-    return;
+      value->asArray()[0].type() != Common::Redis::RespType::BulkString) {        
+        // In pubsub flow, commands like subscribe or unsubscribe are dealt with private client and during creation of new private client, Envoy
+        // sends auth requests along with the actual command, This was verified in packet capture and that is the reason for this check and thereby 
+        // ignoring the OK response in this flow. 
+        // Sample packet capture:
+        // Command was subscribe a but below is the request and response captured in packet capture
+        // Request: 55	15.416512	127.0.0.1	127.0.0.1	RESP	122	Request: auth default pwd@123 subscribe a
+        // Response: 57	15.416650	127.0.0.1	127.0.0.1	RESP	91	Response: OK Array(3)
+        if (value->type() == Common::Redis::RespType::SimpleString && value->asString() == "OK") {
+          ENVOY_LOG(debug, "This is a private client creation auth command response - '{}'", value->toString());
+          return;
+        } else {
+          ENVOY_LOG(error, "unexpected message format: '{}'", value->toString());
+          return;
+        }
   }
 
   std::string message_type = value->asArray()[0].asString();
@@ -2007,6 +2023,9 @@ SplitRequestPtr InstanceImpl::makeRequest(Common::Redis::RespValuePtr&& request,
         ClientResp->type(Common::Redis::RespType::BulkString);
         ClientResp->asString() = callbacks.getClientname();
       }
+    } else if (sub_command == "setinfo") {
+      ClientResp->type(Common::Redis::RespType::SimpleString);
+      ClientResp->asString() = "OK";
     }
     callbacks.onResponse(std::move(ClientResp));
     return nullptr;