envoyproxy · mattklein123 · Dec 6, 2019 · Oct 14, 2019 · Oct 14, 2019 · Oct 14, 2019
diff --git a/source/extensions/clusters/redis/redis_cluster.h b/source/extensions/clusters/redis/redis_cluster.h
@@ -229,7 +229,10 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl {
     void onResponse(NetworkFilters::Common::Redis::RespValuePtr&& value) override;
     void onFailure() override;
     // Note: Below callback isn't used in topology updates
-    bool onRedirection(NetworkFilters::Common::Redis::RespValuePtr&&) override { return true; }
+    bool onRedirection(NetworkFilters::Common::Redis::RespValuePtr&&, const std::string&,
+                       bool) override {
+      return true;
+    }
     void onUnexpectedResponse(const NetworkFilters::Common::Redis::RespValuePtr&);
 
     Network::Address::InstanceConstSharedPtr

diff --git a/source/extensions/filters/network/common/redis/client.h b/source/extensions/filters/network/common/redis/client.h
@@ -48,9 +48,12 @@ class ClientCallbacks {
   /**
    * Called when a MOVED or ASK redirection error is received, and the request must be retried.
    * @param value supplies the MOVED error response
+   * @param host_address supplies the redirection host address and port
+   * @param ask_redirection indicates if this is a ASK redirection
    * @return bool true if the request is successfully redirected, false otherwise
    */
-  virtual bool onRedirection(RespValuePtr&& value) PURE;
+  virtual bool onRedirection(RespValuePtr&& value, const std::string& host_address,
+                             bool ask_redirection) PURE;
 };
 
 /**
@@ -62,7 +65,9 @@ class DoNothingPoolCallbacks : public ClientCallbacks {
   // ClientCallbacks
   void onResponse(Common::Redis::RespValuePtr&&) override {}
   void onFailure() override {}
-  bool onRedirection(Common::Redis::RespValuePtr&&) override { return false; }
+  bool onRedirection(Common::Redis::RespValuePtr&&, const std::string&, bool) override {
+    return false;
+  }
 };
 
 /**

diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc
@@ -224,17 +224,26 @@ void ClientImpl::onRespValue(RespValuePtr&& value) {
   } else if (config_.enableRedirection() && (value->type() == Common::Redis::RespType::Error)) {
     std::vector<absl::string_view> err = StringUtil::splitToken(value->asString(), " ", false);
     bool redirected = false;
+    bool redirect_succeeded = false;
     if (err.size() == 3) {
-      if (err[0] == RedirectionResponse::get().MOVED || err[0] == RedirectionResponse::get().ASK) {
-        redirected = callbacks.onRedirection(std::move(value));
-        if (redirected) {
-          host_->cluster().stats().upstream_internal_redirect_succeeded_total_.inc();
-        } else {
-          host_->cluster().stats().upstream_internal_redirect_failed_total_.inc();
-        }
+      // MOVED and ASK redirection errors have the following substrings: MOVED or ASK (err[0]), hash
+      // key slot (err[1]), and IP address and TCP port separated by a colon (err[2])
+      if (err[0] == RedirectionResponse::get().MOVED) {
+        redirected = true;
+        redirect_succeeded = callbacks.onRedirection(std::move(value), std::string(err[2]), false);
+      } else if (err[0] == RedirectionResponse::get().ASK) {
+        redirected = true;
+        redirect_succeeded = callbacks.onRedirection(std::move(value), std::string(err[2]), true);
       }
-    } else {
+    }
+    if (!redirected) {
       callbacks.onResponse(std::move(value));
+    } else {
+      if (redirect_succeeded) {
+        host_->cluster().stats().upstream_internal_redirect_succeeded_total_.inc();
+      } else {
+        host_->cluster().stats().upstream_internal_redirect_failed_total_.inc();
+      }
     }
   } else {
     callbacks.onResponse(std::move(value));

diff --git a/source/extensions/filters/network/common/redis/utility.cc b/source/extensions/filters/network/common/redis/utility.cc
@@ -19,29 +19,6 @@ AuthRequest::AuthRequest(const std::string& password) {
   asArray().swap(values);
 }
 
-bool redirectionArgsInvalid(const Common::Redis::RespValue* original_request,
-                            const Common::Redis::RespValue& error_response,
-                            std::vector<absl::string_view>& error_substrings,
-                            bool& ask_redirection) {
-  if ((original_request == nullptr) || (error_response.type() != Common::Redis::RespType::Error)) {
-    return true;
-  }
-  error_substrings = StringUtil::splitToken(error_response.asString(), " ", false);
-  if (error_substrings.size() != 3) {
-    return true;
-  }
-  if (error_substrings[0] == "ASK") {
-    ask_redirection = true;
-  } else if (error_substrings[0] == "MOVED") {
-    ask_redirection = false;
-  } else {
-    // The first substring must be MOVED or ASK.
-    return true;
-  }
-  // Other validation done later to avoid duplicate processing.
-  return false;
-}
-
 RespValuePtr makeError(const std::string& error) {
   Common::Redis::RespValuePtr response(new RespValue());
   response->type(Common::Redis::RespType::Error);

diff --git a/source/extensions/filters/network/common/redis/utility.h b/source/extensions/filters/network/common/redis/utility.h
@@ -16,21 +16,6 @@ class AuthRequest : public Redis::RespValue {
   AuthRequest(const std::string& password);
 };
 
-/**
- * Validate the received moved/ask redirection error and the original redis request.
- * @param[in] original_request supplies the incoming request associated with the command splitter
- * request.
- * @param[in] error_response supplies the moved/ask redirection response from the upstream Redis
- * server.
- * @param[out] error_substrings the non-whitespace substrings of error_response.
- * @param[out] ask_redirection true if error_response is an ASK redirection error, false otherwise.
- * @return bool true if the original_request or error_response are not valid, false otherwise.
- */
-bool redirectionArgsInvalid(const Common::Redis::RespValue* original_request,
-                            const Common::Redis::RespValue& error_response,
-                            std::vector<absl::string_view>& error_substrings,
-                            bool& ask_redirection);
-
 RespValuePtr makeError(const std::string& error);
 
 class ReadOnlyRequest : public Redis::RespValue {

diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc
@@ -376,19 +376,9 @@ void InstanceImpl::PendingRequest::onFailure() {
   parent_.onRequestCompleted();
 }
 
-bool InstanceImpl::PendingRequest::onRedirection(Common::Redis::RespValuePtr&& value) {
-  std::vector<absl::string_view> err;
-  bool ask_redirection = false;
-  if (Common::Redis::Utility::redirectionArgsInvalid(&getRequest(incoming_request_), *value, err,
-                                                     ask_redirection)) {
-    onResponse(std::move(value));
-    return false;
-  }
-
-  // MOVED and ASK redirection errors have the following substrings: MOVED or ASK (err[0]), hash key
-  // slot (err[1]), and IP address and TCP port separated by a colon (err[2]).
-  const std::string host_address = std::string(err[2]);
-
+bool InstanceImpl::PendingRequest::onRedirection(Common::Redis::RespValuePtr&& value,
+                                                 const std::string& host_address,
+                                                 bool ask_redirection) {
   // Prepend request with an asking command if redirected via an ASK error. The returned handle is
   // not important since there is no point in being able to cancel the request. The use of
   // null_pool_callbacks ensures the transparent filtering of the Redis server's response to the

diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h
@@ -110,7 +110,8 @@ class InstanceImpl : public Instance {
     // Common::Redis::Client::ClientCallbacks
     void onResponse(Common::Redis::RespValuePtr&& response) override;
     void onFailure() override;
-    bool onRedirection(Common::Redis::RespValuePtr&& value) override;
+    bool onRedirection(Common::Redis::RespValuePtr&& value, const std::string& host_address,
+                       bool ask_redirection) override;
 
     // PoolRequest
     void cancel() override;

diff --git a/source/extensions/health_checkers/redis/redis.cc b/source/extensions/health_checkers/redis/redis.cc
@@ -113,7 +113,7 @@ void RedisHealthChecker::RedisActiveHealthCheckSession::onFailure() {
 }
 
 bool RedisHealthChecker::RedisActiveHealthCheckSession::onRedirection(
-    NetworkFilters::Common::Redis::RespValuePtr&&) {
+    NetworkFilters::Common::Redis::RespValuePtr&&, const std::string&, bool) {
   // Treat any redirection error response from a Redis server as success.
   current_request_ = nullptr;
   handleSuccess();

diff --git a/source/extensions/health_checkers/redis/redis.h b/source/extensions/health_checkers/redis/redis.h
@@ -88,7 +88,8 @@ class RedisHealthChecker : public Upstream::HealthCheckerImplBase {
     // Extensions::NetworkFilters::Common::Redis::Client::ClientCallbacks
     void onResponse(NetworkFilters::Common::Redis::RespValuePtr&& value) override;
     void onFailure() override;
-    bool onRedirection(NetworkFilters::Common::Redis::RespValuePtr&& value) override;
+    bool onRedirection(NetworkFilters::Common::Redis::RespValuePtr&&, const std::string&,
+                       bool) override;
 
     // Network::ConnectionCallbacks
     void onEvent(Network::ConnectionEvent event) override;

diff --git a/test/extensions/clusters/redis/redis_cluster_test.cc b/test/extensions/clusters/redis/redis_cluster_test.cc
@@ -510,7 +510,7 @@ class RedisClusterTest : public testing::Test,
         new NetworkFilters::Common::Redis::RespValue()};
     dummy_value->type(NetworkFilters::Common::Redis::RespType::Error);
     dummy_value->asString() = "dummy text";
-    EXPECT_TRUE(discovery_session.onRedirection(std::move(dummy_value)));
+    EXPECT_TRUE(discovery_session.onRedirection(std::move(dummy_value), "dummy ip", false));
 
     RedisCluster::RedisDiscoveryClient discovery_client(discovery_session);
     EXPECT_NO_THROW(discovery_client.onAboveWriteBufferHighWatermark());

diff --git a/test/extensions/filters/network/common/redis/client_impl_test.cc b/test/extensions/filters/network/common/redis/client_impl_test.cc
@@ -666,7 +666,8 @@ TEST_F(RedisClientImplTest, AskRedirection) {
     // The exact values of the hash slot and IP info are not important.
     response1->asString() = "ASK 1111 10.1.2.3:4321";
     // Simulate redirection failure.
-    EXPECT_CALL(callbacks1, onRedirection_(Ref(response1))).WillOnce(Return(false));
+    EXPECT_CALL(callbacks1, onRedirection_(Ref(response1), "10.1.2.3:4321", true))
+        .WillOnce(Return(false));
     EXPECT_CALL(*connect_or_op_timer_, enableTimer(_, _));
     EXPECT_CALL(host_->outlier_detector_,
                 putResult(Upstream::Outlier::Result::ExtOriginRequestSuccess, _));
@@ -678,7 +679,8 @@ TEST_F(RedisClientImplTest, AskRedirection) {
     response2->type(Common::Redis::RespType::Error);
     // The exact values of the hash slot and IP info are not important.
     response2->asString() = "ASK 2222 10.1.2.4:4321";
-    EXPECT_CALL(callbacks2, onRedirection_(Ref(response2))).WillOnce(Return(true));
+    EXPECT_CALL(callbacks2, onRedirection_(Ref(response2), "10.1.2.4:4321", true))
+        .WillOnce(Return(true));
     EXPECT_CALL(*connect_or_op_timer_, disableTimer());
     EXPECT_CALL(host_->outlier_detector_,
                 putResult(Upstream::Outlier::Result::ExtOriginRequestSuccess, _));
@@ -727,7 +729,8 @@ TEST_F(RedisClientImplTest, MovedRedirection) {
     // The exact values of the hash slot and IP info are not important.
     response1->asString() = "MOVED 1111 10.1.2.3:4321";
     // Simulate redirection failure.
-    EXPECT_CALL(callbacks1, onRedirection_(Ref(response1))).WillOnce(Return(false));
+    EXPECT_CALL(callbacks1, onRedirection_(Ref(response1), "10.1.2.3:4321", false))
+        .WillOnce(Return(false));
     EXPECT_CALL(*connect_or_op_timer_, enableTimer(_, _));
     EXPECT_CALL(host_->outlier_detector_,
                 putResult(Upstream::Outlier::Result::ExtOriginRequestSuccess, _));
@@ -739,7 +742,8 @@ TEST_F(RedisClientImplTest, MovedRedirection) {
     response2->type(Common::Redis::RespType::Error);
     // The exact values of the hash slot and IP info are not important.
     response2->asString() = "MOVED 2222 10.1.2.4:4321";
-    EXPECT_CALL(callbacks2, onRedirection_(Ref(response2))).WillOnce(Return(true));
+    EXPECT_CALL(callbacks2, onRedirection_(Ref(response2), "10.1.2.4:4321", false))
+        .WillOnce(Return(true));
     EXPECT_CALL(*connect_or_op_timer_, disableTimer());
     EXPECT_CALL(host_->outlier_detector_,
                 putResult(Upstream::Outlier::Result::ExtOriginRequestSuccess, _));
@@ -754,6 +758,71 @@ TEST_F(RedisClientImplTest, MovedRedirection) {
   client_->close();
 }
 
+TEST_F(RedisClientImplTest, RedirectionFailure) {
+  InSequence s;
+
+  setup();
+
+  Common::Redis::RespValue request1;
+  MockClientCallbacks callbacks1;
+  EXPECT_CALL(*encoder_, encode(Ref(request1), _));
+  EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false));
+  PoolRequest* handle1 = client_->makeRequest(request1, callbacks1);
+  EXPECT_NE(nullptr, handle1);
+
+  onConnected();
+
+  Common::Redis::RespValue request2;
+  MockClientCallbacks callbacks2;
+  EXPECT_CALL(*encoder_, encode(Ref(request2), _));
+  EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false));
+  PoolRequest* handle2 = client_->makeRequest(request2, callbacks2);
+  EXPECT_NE(nullptr, handle2);
+
+  EXPECT_EQ(2UL, host_->cluster_.stats_.upstream_rq_total_.value());
+  EXPECT_EQ(2UL, host_->cluster_.stats_.upstream_rq_active_.value());
+  EXPECT_EQ(2UL, host_->stats_.rq_total_.value());
+  EXPECT_EQ(2UL, host_->stats_.rq_active_.value());
+
+  Buffer::OwnedImpl fake_data;
+  EXPECT_CALL(*decoder_, decode(Ref(fake_data))).WillOnce(Invoke([&](Buffer::Instance&) -> void {
+    InSequence s;
+
+    // Test an error that looks like it might be a MOVED or ASK redirection error except for the
+    // first non-whitespace substring.
+    Common::Redis::RespValuePtr response1{new Common::Redis::RespValue()};
+    response1->type(Common::Redis::RespType::Error);
+    response1->asString() = "NOTMOVEDORASK 1111 1.1.1.1:1";
+
+    EXPECT_CALL(callbacks1, onResponse_(Ref(response1)));
+    EXPECT_CALL(*connect_or_op_timer_, enableTimer(_, _));
+    EXPECT_CALL(host_->outlier_detector_,
+                putResult(Upstream::Outlier::Result::ExtOriginRequestSuccess, _));
+    callbacks_->onRespValue(std::move(response1));
+
+    EXPECT_EQ(0UL, host_->cluster_.stats_.upstream_internal_redirect_succeeded_total_.value());
+    EXPECT_EQ(0UL, host_->cluster_.stats_.upstream_internal_redirect_failed_total_.value());
+
+    // Test a truncated MOVED error response that cannot be parsed properly.
+    Common::Redis::RespValuePtr response2(new Common::Redis::RespValue());
+    response2->type(Common::Redis::RespType::Error);
+    response2->asString() = "MOVED 1111";
+    EXPECT_CALL(callbacks2, onResponse_(Ref(response2)));
+    EXPECT_CALL(*connect_or_op_timer_, enableTimer(_, _));
+    EXPECT_CALL(host_->outlier_detector_,
+                putResult(Upstream::Outlier::Result::ExtOriginRequestSuccess, _));
+    callbacks_->onRespValue(std::move(response2));
+
+    EXPECT_EQ(0UL, host_->cluster_.stats_.upstream_internal_redirect_succeeded_total_.value());
+    EXPECT_EQ(0UL, host_->cluster_.stats_.upstream_internal_redirect_failed_total_.value());
+  }));
+  upstream_read_filter_->onData(fake_data, false);
+
+  EXPECT_CALL(*upstream_connection_, close(Network::ConnectionCloseType::NoFlush));
+  EXPECT_CALL(*connect_or_op_timer_, disableTimer());
+  client_->close();
+}
+
 TEST_F(RedisClientImplTest, AskRedirectionNotEnabled) {
   InSequence s;
 

diff --git a/test/extensions/filters/network/common/redis/mocks.h b/test/extensions/filters/network/common/redis/mocks.h
@@ -99,11 +99,15 @@ class MockClientCallbacks : public ClientCallbacks {
   ~MockClientCallbacks() override;
 
   void onResponse(Common::Redis::RespValuePtr&& value) override { onResponse_(value); }
-  bool onRedirection(Common::Redis::RespValuePtr&& value) override { return onRedirection_(value); }
+  bool onRedirection(Common::Redis::RespValuePtr&& value, const std::string& host_address,
+                     bool ask_redirection) override {
+    return onRedirection_(value, host_address, ask_redirection);
+  }
 
   MOCK_METHOD1(onResponse_, void(Common::Redis::RespValuePtr& value));
   MOCK_METHOD0(onFailure, void());
-  MOCK_METHOD1(onRedirection_, bool(Common::Redis::RespValuePtr& value));
+  MOCK_METHOD3(onRedirection_, bool(Common::Redis::RespValuePtr& value,
+                                    const std::string& host_address, bool ask_redirection));
 };
 
 } // namespace Client