Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Read_policy is not set correctly. #8034

Merged
merged 1 commit into from
Aug 26, 2019
Merged

Read_policy is not set correctly. #8034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions source/extensions/filters/network/common/redis/client_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/network/redis_proxy/conn_pool_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
49 changes: 46 additions & 3 deletions test/extensions/clusters/redis/redis_cluster_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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.

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand Down
18 changes: 13 additions & 5 deletions test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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));
Expand Down Expand Up @@ -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) {
Expand Down