Skip to content

redis: fix hostname resolution bug introduced in #19541 #20217

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

Merged
mattklein123 merged 8 commits into from
Mar 8, 2022
Merged

redis: fix hostname resolution bug introduced in #19541 #20217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
598e0ea
add debug statements
huntsman90 Mar 3, 2022
dea11d9
change redis cluster
huntsman90 Mar 4, 2022
ed81964
Revert "add debug statements"
huntsman90 Mar 4, 2022
1338dff
add a test
huntsman90 Mar 4, 2022
ceea432
add a test
huntsman90 Mar 4, 2022
dcc53af
Revert "add a test"
huntsman90 Mar 4, 2022
0493c35
modify test to check for parallel DNS resolution
huntsman90 Mar 8, 2022
9ffbd4d
remove log statements
huntsman90 Mar 8, 2022
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
8 changes: 4 additions & 4 deletions source/extensions/clusters/redis/redis_cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,8 @@ void RedisCluster::RedisDiscoverySession::resolveClusterHostnames(
parent_.dns_resolver_->resolve(
slot.primary_hostname_, parent_.dns_lookup_family_,
[this, slot_idx, slots,
&hostname_resolution_required_cnt](Network::DnsResolver::ResolutionStatus status,
std::list<Network::DnsResponse>&& response) -> void {
hostname_resolution_required_cnt](Network::DnsResolver::ResolutionStatus status,
std::list<Network::DnsResponse>&& response) -> void {
auto& slot = (*slots)[slot_idx];
ENVOY_LOG(debug, "async DNS resolution complete for {}", slot.primary_hostname_);
updateDnsStats(status, response.empty());
Expand Down Expand Up @@ -393,8 +393,8 @@ void RedisCluster::RedisDiscoverySession::resolveReplicas(
parent_.dns_resolver_->resolve(
replica.first, parent_.dns_lookup_family_,
[this, index, slots, replica_idx,
&hostname_resolution_required_cnt](Network::DnsResolver::ResolutionStatus status,
std::list<Network::DnsResponse>&& response) -> void {
hostname_resolution_required_cnt](Network::DnsResolver::ResolutionStatus status,
std::list<Network::DnsResponse>&& response) -> void {
auto& slot = (*slots)[index];
auto& replica = slot.replicas_to_resolve_[replica_idx];
ENVOY_LOG(debug, "async DNS resolution complete for {}", replica.first);
Expand Down
78 changes: 78 additions & 0 deletions test/extensions/clusters/redis/redis_cluster_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <bitset>
#include <chrono>
#include <memory>
#include <utility>
#include <vector>

#include "envoy/common/callback.h"
Expand Down Expand Up @@ -36,6 +37,7 @@ using testing::Eq;
using testing::NiceMock;
using testing::Ref;
using testing::Return;
using testing::SaveArg;

namespace Envoy {
namespace Extensions {
Expand Down Expand Up @@ -400,6 +402,50 @@ class RedisClusterTest : public testing::Test,
return response;
}

NetworkFilters::Common::Redis::RespValuePtr
twoSlotsPrimariesHostnames(const std::string& primary1, const std::string& primary2,
int64_t port) const {
std::vector<NetworkFilters::Common::Redis::RespValue> primary_1(2);
primary_1[0].type(NetworkFilters::Common::Redis::RespType::BulkString);
primary_1[0].asString() = primary1;
primary_1[1].type(NetworkFilters::Common::Redis::RespType::Integer);
primary_1[1].asInteger() = port;

std::vector<NetworkFilters::Common::Redis::RespValue> primary_2(2);
primary_2[0].type(NetworkFilters::Common::Redis::RespType::BulkString);
primary_2[0].asString() = primary2;
primary_2[1].type(NetworkFilters::Common::Redis::RespType::Integer);
primary_2[1].asInteger() = port;

std::vector<NetworkFilters::Common::Redis::RespValue> slot_1(3);
slot_1[0].type(NetworkFilters::Common::Redis::RespType::Integer);
slot_1[0].asInteger() = 0;
slot_1[1].type(NetworkFilters::Common::Redis::RespType::Integer);
slot_1[1].asInteger() = 9999;
slot_1[2].type(NetworkFilters::Common::Redis::RespType::Array);
slot_1[2].asArray().swap(primary_1);

std::vector<NetworkFilters::Common::Redis::RespValue> slot_2(3);
slot_2[0].type(NetworkFilters::Common::Redis::RespType::Integer);
slot_2[0].asInteger() = 10000;
slot_2[1].type(NetworkFilters::Common::Redis::RespType::Integer);
slot_2[1].asInteger() = 16383;
slot_2[2].type(NetworkFilters::Common::Redis::RespType::Array);
slot_2[2].asArray().swap(primary_2);

std::vector<NetworkFilters::Common::Redis::RespValue> slots(2);
slots[0].type(NetworkFilters::Common::Redis::RespType::Array);
slots[0].asArray().swap(slot_1);
slots[1].type(NetworkFilters::Common::Redis::RespType::Array);
slots[1].asArray().swap(slot_2);

NetworkFilters::Common::Redis::RespValuePtr response(
new NetworkFilters::Common::Redis::RespValue());
response->type(NetworkFilters::Common::Redis::RespType::Array);
response->asArray().swap(slots);
return response;
}

NetworkFilters::Common::Redis::RespValue
createStringField(bool is_correct_type, const std::string& correct_value) const {
NetworkFilters::Common::Redis::RespValue respValue;
Expand Down Expand Up @@ -778,6 +824,38 @@ TEST_F(RedisClusterTest, AddressAsHostname) {
EXPECT_EQ(0U, cluster_->info()->stats().update_failure_.value());
}

TEST_F(RedisClusterTest, AddressAsHostnameParallelResolution) {
// This test specifically ensures that DNS resolution of different hostnames running parallel
// works as expected.
setupFromV3Yaml(BasicConfig);
const std::list<std::string> resolved_addresses{"127.0.0.1", "127.0.0.2"};
expectResolveDiscovery(Network::DnsLookupFamily::V4Only, "foo.bar.com", resolved_addresses);

Network::DnsResolver::ResolveCb primary1_resolve_cb;
Network::DnsResolver::ResolveCb primary2_resolve_cb;
EXPECT_CALL(*dns_resolver_, resolve("primary1.com", Network::DnsLookupFamily::V4Only, _))
.WillOnce(DoAll(SaveArg<2>(&primary1_resolve_cb), Return(&dns_resolver_->active_query_)));
EXPECT_CALL(*dns_resolver_, resolve("primary2.com", Network::DnsLookupFamily::V4Only, _))
.WillOnce(DoAll(SaveArg<2>(&primary2_resolve_cb), Return(&dns_resolver_->active_query_)));

expectRedisResolve(true);

EXPECT_CALL(membership_updated_, ready());
EXPECT_CALL(initialized_, ready());
EXPECT_CALL(*cluster_callback_, onClusterSlotUpdate(_, _));
cluster_->initialize([&]() -> void { initialized_.ready(); });
expectClusterSlotResponse(twoSlotsPrimariesHostnames("primary1.com", "primary2.com", 22120));
primary1_resolve_cb(Network::DnsResolver::ResolutionStatus::Success,
TestUtility::makeDnsResponse(std::list<std::string>{"127.0.1.1"}));
primary2_resolve_cb(Network::DnsResolver::ResolutionStatus::Success,
TestUtility::makeDnsResponse(std::list<std::string>{"127.0.1.2"}));
expectHealthyHosts(std::list<std::string>({
"127.0.1.1:22120",
"127.0.1.2:22120",
}));
EXPECT_EQ(0U, cluster_->info()->stats().update_failure_.value());
}

TEST_F(RedisClusterTest, AddressAsHostnameFailure) {
setupFromV3Yaml(BasicConfig);
const std::list<std::string> resolved_addresses{"127.0.0.1", "127.0.0.2"};
Expand Down