From 34991fcebfa654d272f10afc3f6b01b4d0dd3c8c Mon Sep 17 00:00:00 2001 From: Henry Yang Date: Sat, 24 Aug 2019 20:26:23 -0700 Subject: [PATCH] Read_policy is not set correctly. Add more integration test and additional checks in the unit tests. Signed-off-by: Henry Yang --- .../network/common/redis/client_impl.cc | 6 +-- .../network/redis_proxy/conn_pool_impl.cc | 4 +- .../redis/redis_cluster_integration_test.cc | 49 +++++++++++++++++-- .../redis_proxy/conn_pool_impl_test.cc | 18 +++++-- 4 files changed, 64 insertions(+), 13 deletions(-) diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc index a2610417a873..6f941d223ca0 100644 --- 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 index 2e1414e0252c..a097924ea734 100644 --- 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 index 000928ef2441..bb1bfbd53d98 100644 --- 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 index 22161d0e00fb..9dd249ec6002 100644 --- 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(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) {