Read_policy is not set correctly. (#8034)

Add more integration test and additional checks in the unit tests. Signed-off-by: Henry Yang <hyang@lyft.com>
envoyproxy · Aug 26, 2019 · fc32b64 · fc32b64
1 parent b0aca30
commit fc32b64
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 13 deletions.
diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc
@@ -31,14 +31,14 @@ ConfigImpl::ConfigImpl(
     break;
   case envoy::config::filter::network::redis_proxy::v2::
       RedisProxy_ConnPoolSettings_ReadPolicy_REPLICA:
-    read_policy_ = ReadPolicy::PreferMaster;
+    read_policy_ = ReadPolicy::Replica;
     break;
   case envoy::config::filter::network::redis_proxy::v2::
       RedisProxy_ConnPoolSettings_ReadPolicy_PREFER_REPLICA:
-    read_policy_ = ReadPolicy::PreferMaster;
+    read_policy_ = ReadPolicy::PreferReplica;
     break;
   case envoy::config::filter::network::redis_proxy::v2::RedisProxy_ConnPoolSettings_ReadPolicy_ANY:
-    read_policy_ = ReadPolicy::PreferMaster;
+    read_policy_ = ReadPolicy::Any;
     break;
   default:
     NOT_REACHED_GCOVR_EXCL_LINE;

diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc
@@ -225,8 +225,8 @@ InstanceImpl::ThreadLocalPool::makeRequest(const std::string& key,
   }
 
   const bool use_crc16 = is_redis_cluster_;
-  Clusters::Redis::RedisLoadBalancerContextImpl lb_context(key, parent_.config_.enableHashtagging(),
-                                                           use_crc16, request);
+  Clusters::Redis::RedisLoadBalancerContextImpl lb_context(
+      key, parent_.config_.enableHashtagging(), use_crc16, request, parent_.config_.readPolicy());
   Upstream::HostConstSharedPtr host = cluster_->loadBalancer().chooseHost(&lb_context);
   if (!host) {
     return nullptr;

diff --git a/test/extensions/clusters/redis/redis_cluster_integration_test.cc b/test/extensions/clusters/redis/redis_cluster_integration_test.cc
@@ -15,8 +15,7 @@ namespace {
 // This is a basic redis_proxy configuration with a single host
 // in the cluster. The load balancing policy must be set
 // to random for proper test operation.
-
-const std::string& testConfig() {
+const std::string& listenerConfig() {
   CONSTRUCT_ON_FIRST_USE(std::string, R"EOF(
 admin:
   access_log_path: /dev/null
@@ -40,7 +39,11 @@ const std::string& testConfig() {
             catch_all_route:
               cluster: cluster_0
           settings:
-            op_timeout: 5s
+            op_timeout: 5s)EOF");
+}
+
+const std::string& clusterConfig() {
+  CONSTRUCT_ON_FIRST_USE(std::string, R"EOF(
   clusters:
     - name: cluster_0
       lb_policy: CLUSTER_PROVIDED
@@ -58,6 +61,16 @@ const std::string& testConfig() {
 )EOF");
 }
 
+const std::string& testConfig() {
+  CONSTRUCT_ON_FIRST_USE(std::string, listenerConfig() + clusterConfig());
+}
+
+const std::string& testConfigWithReadPolicy() {
+  CONSTRUCT_ON_FIRST_USE(std::string, listenerConfig() + R"EOF(
+            read_policy: REPLICA
+)EOF" + clusterConfig());
+}
+
 // This is the basic redis_proxy configuration with an upstream
 // authentication password specified.
 
@@ -278,6 +291,13 @@ class RedisClusterWithAuthIntegrationTest : public RedisClusterIntegrationTest {
       : RedisClusterIntegrationTest(config, num_upstreams) {}
 };
 
+class RedisClusterWithReadPolicyIntegrationTest : public RedisClusterIntegrationTest {
+public:
+  RedisClusterWithReadPolicyIntegrationTest(const std::string& config = testConfigWithReadPolicy(),
+                                            int num_upstreams = 3)
+      : RedisClusterIntegrationTest(config, num_upstreams) {}
+};
+
 INSTANTIATE_TEST_SUITE_P(IpVersions, RedisClusterIntegrationTest,
                          testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
                          TestUtility::ipTestParamsToString);
@@ -333,6 +353,29 @@ TEST_P(RedisClusterIntegrationTest, TwoSlot) {
   simpleRequestAndResponse(1, makeBulkStringArray({"get", "foo"}), "$3\r\nbar\r\n");
 }
 
+// This test sends simple "set foo" and "get foo" command from a fake
+// downstream client through the proxy to a fake upstream
+// Redis cluster with a single slot with master and replica.
+// The envoy proxy is set with read_policy to read from replica, the expected result
+// is that the set command will be sent to the master and the get command will be sent
+// to the replica
+
+TEST_P(RedisClusterWithReadPolicyIntegrationTest, SingleSlotMasterReplicaReadReplica) {
+  random_index_ = 0;
+
+  on_server_init_function_ = [this]() {
+    std::string cluster_slot_response = singleSlotMasterReplica(
+        fake_upstreams_[0]->localAddress()->ip(), fake_upstreams_[1]->localAddress()->ip());
+    expectCallClusterSlot(random_index_, cluster_slot_response);
+  };
+
+  initialize();
+
+  // foo hashes to slot 12182 which has master node in upstream 0 and replica in upstream 1
+  simpleRequestAndResponse(0, makeBulkStringArray({"set", "foo", "bar"}), ":1\r\n");
+  simpleRequestAndResponse(1, makeBulkStringArray({"get", "foo"}), "$3\r\nbar\r\n");
+}
+
 // This test sends a simple "get foo" command from a fake
 // downstream client through the proxy to a fake upstream
 // Redis cluster with a single slot with master and replica.

diff --git a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc
@@ -161,7 +161,8 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client
 
   void testReadPolicy(
       envoy::config::filter::network::redis_proxy::v2::RedisProxy::ConnPoolSettings::ReadPolicy
-          read_policy) {
+          read_policy,
+      NetworkFilters::Common::Redis::Client::ReadPolicy expected_read_policy) {
     InSequence s;
 
     read_policy_ = read_policy;
@@ -178,6 +179,9 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client
               EXPECT_EQ(context->computeHashKey().value(), MurmurHash::murmurHash2_64("hash_key"));
               EXPECT_EQ(context->metadataMatchCriteria(), nullptr);
               EXPECT_EQ(context->downstreamConnection(), nullptr);
+              auto redis_context =
+                  dynamic_cast<Clusters::Redis::RedisLoadBalancerContext*>(context);
+              EXPECT_EQ(redis_context->readPolicy(), expected_read_policy);
               return cm_.thread_local_cluster_.lb_.host_;
             }));
     EXPECT_CALL(*this, create_(_)).WillOnce(Return(client));
@@ -279,13 +283,17 @@ TEST_F(RedisConnPoolImplTest, BasicWithAuthPassword) {
 
 TEST_F(RedisConnPoolImplTest, BasicWithReadPolicy) {
   testReadPolicy(envoy::config::filter::network::redis_proxy::v2::
-                     RedisProxy_ConnPoolSettings_ReadPolicy_PREFER_MASTER);
+                     RedisProxy_ConnPoolSettings_ReadPolicy_PREFER_MASTER,
+                 NetworkFilters::Common::Redis::Client::ReadPolicy::PreferMaster);
   testReadPolicy(envoy::config::filter::network::redis_proxy::v2::
-                     RedisProxy_ConnPoolSettings_ReadPolicy_REPLICA);
+                     RedisProxy_ConnPoolSettings_ReadPolicy_REPLICA,
+                 NetworkFilters::Common::Redis::Client::ReadPolicy::Replica);
   testReadPolicy(envoy::config::filter::network::redis_proxy::v2::
-                     RedisProxy_ConnPoolSettings_ReadPolicy_PREFER_REPLICA);
+                     RedisProxy_ConnPoolSettings_ReadPolicy_PREFER_REPLICA,
+                 NetworkFilters::Common::Redis::Client::ReadPolicy::PreferReplica);
   testReadPolicy(
-      envoy::config::filter::network::redis_proxy::v2::RedisProxy_ConnPoolSettings_ReadPolicy_ANY);
+      envoy::config::filter::network::redis_proxy::v2::RedisProxy_ConnPoolSettings_ReadPolicy_ANY,
+      NetworkFilters::Common::Redis::Client::ReadPolicy::Any);
 };
 
 TEST_F(RedisConnPoolImplTest, Hashtagging) {