From 5f3d4962f44e8e7cdea875b1eee6c72f8c3710cf Mon Sep 17 00:00:00 2001 From: Henry Yang Date: Tue, 20 Aug 2019 15:46:48 -0700 Subject: [PATCH 1/3] Handle invalid ip address from cluster slots and added tests Signed-off-by: Henry Yang --- .../clusters/redis/redis_cluster.cc | 16 ++++--- .../extensions/clusters/redis/redis_cluster.h | 3 ++ .../clusters/redis/redis_cluster_test.cc | 48 ++++++++++++------- 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index 9e70ceaf7cf3..ecf54168bac5 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -170,11 +170,11 @@ RedisCluster::RedisDiscoverySession::RedisDiscoverySession( resolve_timer_(parent.dispatcher_.createTimer([this]() -> void { startResolveRedis(); })), client_factory_(client_factory), buffer_timeout_(0) {} -namespace { // Convert the cluster slot IP/Port response to and address, return null if the response does not // match the expected type. Network::Address::InstanceConstSharedPtr -ProcessCluster(const NetworkFilters::Common::Redis::RespValue& value) { +RedisCluster::RedisDiscoverySession::RedisDiscoverySession::ProcessCluster( + const NetworkFilters::Common::Redis::RespValue& value) { if (value.type() != NetworkFilters::Common::Redis::RespType::Array) { return nullptr; } @@ -187,12 +187,16 @@ ProcessCluster(const NetworkFilters::Common::Redis::RespValue& value) { std::string address = array[0].asString(); bool ipv6 = (address.find(':') != std::string::npos); - if (ipv6) { - return std::make_shared(address, array[1].asInteger()); + try { + if (ipv6) { + return std::make_shared(address, array[1].asInteger()); + } + return std::make_shared(address, array[1].asInteger()); + } catch (const EnvoyException& ex) { + ENVOY_LOG(warn, "Invalid ip address in CLUSTER SLOTS response: {}", ex.what()); + return nullptr; } - return std::make_shared(address, array[1].asInteger()); } -} // namespace RedisCluster::RedisDiscoverySession::~RedisDiscoverySession() { if (current_request_) { diff --git a/source/extensions/clusters/redis/redis_cluster.h b/source/extensions/clusters/redis/redis_cluster.h index 9117fa7daccc..f4038b7629f1 100644 --- a/source/extensions/clusters/redis/redis_cluster.h +++ b/source/extensions/clusters/redis/redis_cluster.h @@ -227,6 +227,9 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { bool onRedirection(const NetworkFilters::Common::Redis::RespValue&) override { return true; } void onUnexpectedResponse(const NetworkFilters::Common::Redis::RespValuePtr&); + Network::Address::InstanceConstSharedPtr + ProcessCluster(const NetworkFilters::Common::Redis::RespValue& value); + RedisCluster& parent_; Event::Dispatcher& dispatcher_; std::string current_host_address_; diff --git a/test/extensions/clusters/redis/redis_cluster_test.cc b/test/extensions/clusters/redis/redis_cluster_test.cc index 7d6a4967cc8a..a3190debb640 100644 --- a/test/extensions/clusters/redis/redis_cluster_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_test.cc @@ -55,6 +55,8 @@ const std::string BasicConfig = R"EOF( )EOF"; } +static const int ResponseFlagSize = 11; +static const int ResponseReplicaFlagSize = 4; class RedisClusterTest : public testing::Test, public Extensions::NetworkFilters::Common::Redis::Client::ClientFactory { public: @@ -192,7 +194,7 @@ class RedisClusterTest : public testing::Test, replica_1[1].type(NetworkFilters::Common::Redis::RespType::Integer); replica_1[1].asInteger() = port; - std::vector slot_1(4); + std::vector slot_1(ResponseReplicaFlagSize); slot_1[0].type(NetworkFilters::Common::Redis::RespType::Integer); slot_1[0].asInteger() = 0; slot_1[1].type(NetworkFilters::Common::Redis::RespType::Integer); @@ -280,7 +282,7 @@ class RedisClusterTest : public testing::Test, replica_2[1].type(NetworkFilters::Common::Redis::RespType::Integer); replica_2[1].asInteger() = 22120; - std::vector slot_1(4); + std::vector slot_1(ResponseReplicaFlagSize); slot_1[0].type(NetworkFilters::Common::Redis::RespType::Integer); slot_1[0].asInteger() = 0; slot_1[1].type(NetworkFilters::Common::Redis::RespType::Integer); @@ -290,7 +292,7 @@ class RedisClusterTest : public testing::Test, slot_1[3].type(NetworkFilters::Common::Redis::RespType::Array); slot_1[3].asArray().swap(replica_1); - std::vector slot_2(4); + std::vector slot_2(ResponseReplicaFlagSize); slot_2[0].type(NetworkFilters::Common::Redis::RespType::Integer); slot_2[0].asInteger() = 10000; slot_2[1].type(NetworkFilters::Common::Redis::RespType::Integer); @@ -321,7 +323,7 @@ class RedisClusterTest : public testing::Test, respValue.asString() = correct_value; } else { respValue.type(NetworkFilters::Common::Redis::RespType::Integer); - respValue.asInteger() = 10; + respValue.asInteger() = ResponseFlagSize; } return respValue; } @@ -355,8 +357,9 @@ class RedisClusterTest : public testing::Test, // Create a redis cluster slot response. If a bit is set in the bitset, then that part of // of the response is correct, otherwise it's incorrect. - NetworkFilters::Common::Redis::RespValuePtr createResponse(std::bitset<10> flags, - std::bitset<3> replica_flags) const { + NetworkFilters::Common::Redis::RespValuePtr + createResponse(std::bitset flags, + std::bitset replica_flags) const { int64_t idx(0); int64_t slots_type = idx++; int64_t slots_size = idx++; @@ -367,16 +370,22 @@ class RedisClusterTest : public testing::Test, int64_t master_type = idx++; int64_t master_size = idx++; int64_t master_ip_type = idx++; + int64_t master_ip_value = idx++; int64_t master_port_type = idx++; idx = 0; int64_t replica_size = idx++; int64_t replica_ip_type = idx++; + int64_t replica_ip_value = idx++; int64_t replica_port_type = idx++; std::vector master_1_array; if (flags.test(master_size)) { // Ip field. - master_1_array.push_back(createStringField(flags.test(master_ip_type), "127.0.0.1")); + if (flags.test(master_ip_value)) { + master_1_array.push_back(createStringField(flags.test(master_ip_type), "127.0.0.1")); + } else { + master_1_array.push_back(createStringField(flags.test(master_ip_type), "bad ip foo")); + } // Port field. master_1_array.push_back(createIntegerField(flags.test(master_port_type), 22120)); } @@ -384,8 +393,13 @@ class RedisClusterTest : public testing::Test, std::vector replica_1_array; if (replica_flags.any()) { // Ip field. - replica_1_array.push_back( - createStringField(replica_flags.test(replica_ip_type), "127.0.0.2")); + if (replica_flags.test(replica_ip_value)) { + replica_1_array.push_back( + createStringField(replica_flags.test(replica_ip_type), "127.0.0.2")); + } else { + replica_1_array.push_back( + createStringField(replica_flags.test(replica_ip_type), "bad ip bar")); + } // Port field. replica_1_array.push_back(createIntegerField(replica_flags.test(replica_port_type), 22120)); } @@ -771,8 +785,8 @@ TEST_F(RedisClusterTest, RedisErrorResponse) { EXPECT_CALL(membership_updated_, ready()); EXPECT_CALL(initialized_, ready()); EXPECT_CALL(*cluster_callback_, onClusterSlotUpdate(_, _)).Times(1); - std::bitset<10> single_slot_master(0x7ff); - std::bitset<3> no_replica(0); + std::bitset single_slot_master(0xfff); + std::bitset no_replica(0); expectClusterSlotResponse(createResponse(single_slot_master, no_replica)); expectHealthyHosts(std::list({"127.0.0.1:22120"})); @@ -780,8 +794,8 @@ TEST_F(RedisClusterTest, RedisErrorResponse) { uint64_t update_attempt = 2; uint64_t update_failure = 1; // Test every combination the cluster slots response. - for (uint64_t i = 0; i < (1 << 10); i++) { - std::bitset<10> flags(i); + for (uint64_t i = 0; i < (1 << ResponseFlagSize); i++) { + std::bitset flags(i); expectRedisResolve(); resolve_timer_->invokeCallback(); if (flags.all()) { @@ -807,8 +821,8 @@ TEST_F(RedisClusterTest, RedisReplicaErrorResponse) { EXPECT_CALL(membership_updated_, ready()); EXPECT_CALL(initialized_, ready()); EXPECT_CALL(*cluster_callback_, onClusterSlotUpdate(_, _)).Times(1); - std::bitset<10> single_slot_master(0x7ff); - std::bitset<3> no_replica(0); + std::bitset single_slot_master(0xfff); + std::bitset no_replica(0); expectClusterSlotResponse(createResponse(single_slot_master, no_replica)); expectHealthyHosts(std::list({"127.0.0.1:22120"})); @@ -816,8 +830,8 @@ TEST_F(RedisClusterTest, RedisReplicaErrorResponse) { uint64_t update_attempt = 1; uint64_t update_failure = 0; // Test every combination the replica error response. - for (uint64_t i = 1; i < (1 << 3); i++) { - std::bitset<3> replica_flags(i); + for (uint64_t i = 1; i < (1 << ResponseReplicaFlagSize); i++) { + std::bitset replica_flags(i); expectRedisResolve(); resolve_timer_->invokeCallback(); if (replica_flags.all()) { From e75ffabd22e8aaf9b688481b8cf1a78e59f15077 Mon Sep 17 00:00:00 2001 From: Henry Yang Date: Wed, 21 Aug 2019 04:44:18 -0700 Subject: [PATCH 2/3] Fix review feedback Signed-off-by: Henry Yang --- source/extensions/clusters/redis/redis_cluster.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index ecf54168bac5..8eb3489bf019 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -193,7 +193,7 @@ RedisCluster::RedisDiscoverySession::RedisDiscoverySession::ProcessCluster( } return std::make_shared(address, array[1].asInteger()); } catch (const EnvoyException& ex) { - ENVOY_LOG(warn, "Invalid ip address in CLUSTER SLOTS response: {}", ex.what()); + ENVOY_LOG(debug, "Invalid ip address in CLUSTER SLOTS response: {}", ex.what()); return nullptr; } } From e7c19b4c46999b9910efae7a2de970eebd1a2030 Mon Sep 17 00:00:00 2001 From: Henry Yang Date: Wed, 21 Aug 2019 11:28:11 -0700 Subject: [PATCH 3/3] Use parseInternetAddress Signed-off-by: Henry Yang --- source/extensions/clusters/redis/redis_cluster.cc | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index 8eb3489bf019..dc41651b0ca1 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -185,13 +185,8 @@ RedisCluster::RedisDiscoverySession::RedisDiscoverySession::ProcessCluster( return nullptr; } - std::string address = array[0].asString(); - bool ipv6 = (address.find(':') != std::string::npos); try { - if (ipv6) { - return std::make_shared(address, array[1].asInteger()); - } - return std::make_shared(address, array[1].asInteger()); + return Network::Utility::parseInternetAddress(array[0].asString(), array[1].asInteger(), false); } catch (const EnvoyException& ex) { ENVOY_LOG(debug, "Invalid ip address in CLUSTER SLOTS response: {}", ex.what()); return nullptr;