From b292f3f33034cf136637b7e121e1273361dd6725 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Fri, 19 May 2017 10:46:34 -0400 Subject: [PATCH 01/12] Add Ipv6 dns support --- .../configuration/cluster_manager/cluster.rst | 12 + include/envoy/network/dns.h | 5 +- source/common/json/config_schemas.cc | 4 + source/common/network/dns_impl.cc | 71 ++++-- source/common/network/dns_impl.h | 10 +- source/common/upstream/logical_dns_cluster.cc | 41 +++- source/common/upstream/logical_dns_cluster.h | 1 + source/common/upstream/upstream_impl.cc | 28 ++- source/common/upstream/upstream_impl.h | 1 + test/common/network/BUILD | 2 + test/common/network/dns_impl_test.cc | 226 ++++++++++++++---- .../upstream/cluster_manager_impl_test.cc | 4 +- .../upstream/logical_dns_cluster_test.cc | 107 +++++++-- test/common/upstream/upstream_impl_test.cc | 108 ++++++++- test/mocks/network/mocks.cc | 2 +- test/mocks/network/mocks.h | 4 +- test/test_common/BUILD | 1 + test/test_common/utility.cc | 3 +- 18 files changed, 507 insertions(+), 123 deletions(-) diff --git a/docs/configuration/cluster_manager/cluster.rst b/docs/configuration/cluster_manager/cluster.rst index 117959eb8b0ee..a5f0ebac9f3b9 100644 --- a/docs/configuration/cluster_manager/cluster.rst +++ b/docs/configuration/cluster_manager/cluster.rst @@ -20,6 +20,7 @@ Cluster "features": "...", "http_codec_options": "...", "dns_refresh_rate_ms": "...", + "dns_lookup_ip_version": "...", "outlier_detection": "..." } @@ -144,6 +145,17 @@ dns_refresh_rate_ms the value defaults to 5000. For cluster types other than *strict_dns* and *logical_dns* this setting is ignored. +.. _config_cluster_manager_cluster_dns_refresh_rate_ms: + +dns_loopkup_ip_version + *(optional, string)* The dns IP address version lookup policy. The options are *v4_only*, *v6_only*, + and *auto*. If this setting is not specified, the value defaults to *v4_only*. When *v4_only* is selected, + the dns resolver will only perform a lookup for addresses in the IPv4 family. If *v6_only* is selected, + the dns resolver will only perform a lookup for addresses in the IPv6 family. If *auto* is specified, + the dns resolver will first perform a lookup for addresses in the IPv4 family and fallback to a lookup for + addresses in the IPv6 family. For cluster types other than *strict_dns* and *logical_dns*, this setting + is ignored. + .. _config_cluster_manager_cluster_outlier_detection: outlier_detection diff --git a/include/envoy/network/dns.h b/include/envoy/network/dns.h index 30658930967ac..381b89e394aa7 100644 --- a/include/envoy/network/dns.h +++ b/include/envoy/network/dns.h @@ -41,11 +41,14 @@ class DnsResolver { /** * Initiate an async DNS resolution. * @param dns_name supplies the DNS name to lookup. + * @param dns_lookup_ip_version the DNS IP version lookup policy. * @param callback supplies the callback to invoke when the resolution is complete. * @return if non-null, a handle that can be used to cancel the resolution. * This is only valid until the invocation of callback or ~DnsResolver(). */ - virtual ActiveDnsQuery* resolve(const std::string& dns_name, ResolveCb callback) PURE; + virtual ActiveDnsQuery* resolve(const std::string& dns_name, + const std::string& dns_lookup_ip_version, + ResolveCb callback) PURE; }; typedef std::unique_ptr DnsResolverPtr; diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 48e758386345c..0e9f158db1bfd 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -1210,6 +1210,10 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( "minimum" : 0, "exclusiveMinimum" : true }, + "dns_lookup_ip_version" : { + "type" : "string", + "enum" : ["v4_only", "v6_only", "auto"] + }, "outlier_detection" : { "type" : "object", "properties" : { diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 72567dd1209ab..67b34fa8471e2 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -52,25 +52,41 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, hostent* delete this; return; } + if (status == ARES_SUCCESS || !fallback_if_failed) { + completed_ = true; + } + std::list address_list; - completed_ = true; if (status == ARES_SUCCESS) { - ASSERT(hostent->h_addrtype == AF_INET); - for (int i = 0; hostent->h_addr_list[i] != nullptr; ++i) { - ASSERT(hostent->h_length == sizeof(in_addr)); - sockaddr_in address; - memset(&address, 0, sizeof(address)); - // TODO(mattklein123): IPv6 support. - address.sin_family = AF_INET; - address.sin_port = 0; - address.sin_addr = *reinterpret_cast(hostent->h_addr_list[i]); - address_list.emplace_back(new Address::Ipv4Instance(&address)); + if (hostent->h_addrtype == AF_INET) { + for (int i = 0; hostent->h_addr_list[i] != nullptr; ++i) { + ASSERT(hostent->h_length == sizeof(in_addr)); + sockaddr_in address; + memset(&address, 0, sizeof(address)); + address.sin_family = AF_INET; + address.sin_port = 0; + address.sin_addr = *reinterpret_cast(hostent->h_addr_list[i]); + address_list.emplace_back(new Address::Ipv4Instance(&address)); + } + } else if (hostent->h_addrtype == AF_INET6) { + for (int i = 0; hostent->h_addr_list[i] != nullptr; ++i) { + ASSERT(hostent->h_length == sizeof(in6_addr)); + sockaddr_in6 address; + memset(&address, 0, sizeof(address)); + address.sin6_family = AF_INET6; + address.sin6_port = 0; + address.sin6_addr = *reinterpret_cast(hostent->h_addr_list[i]); + address_list.emplace_back(new Address::Ipv6Instance(address)); + } } + } else if (fallback_if_failed) { + fallback_if_failed = false; + getHostByName(AF_INET6); } - if (!cancelled_) { + if (!cancelled_ && completed_) { callback_(std::move(address_list)); } - if (owned_) { + if (owned_ && completed_) { delete this; } } @@ -115,17 +131,26 @@ void DnsResolverImpl::onAresSocketStateChange(int fd, int read, int write) { (write ? Event::FileReadyType::Write : 0)); } -ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, ResolveCb callback) { +ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, + const std::string& dns_lookup_ip_version, + ResolveCb callback) { std::unique_ptr pending_resolution(new PendingResolution()); pending_resolution->callback_ = callback; + pending_resolution->channel_ = channel_; + pending_resolution->dns_name_ = dns_name; + if (dns_lookup_ip_version == "auto") { + pending_resolution->fallback_if_failed = true; + } - ares_gethostbyname(channel_, dns_name.c_str(), - AF_INET, [](void* arg, int status, int timeouts, hostent* hostent) { - static_cast(arg)->onAresHostCallback(status, hostent); - UNREFERENCED_PARAMETER(timeouts); - }, pending_resolution.get()); + if (dns_lookup_ip_version == "v6_only") { + pending_resolution->getHostByName(AF_INET6); + } else { + pending_resolution->getHostByName(AF_INET); + } if (pending_resolution->completed_) { + // Resolution does not need asynchronous behavior or network events. For + // example, localhost lookup. return nullptr; } else { // The PendingResolution will self-delete when the request completes @@ -135,5 +160,13 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, ResolveCb } } +void DnsResolverImpl::PendingResolution::getHostByName(int family) { + ares_gethostbyname(channel_, dns_name_.c_str(), + family, [](void* arg, int status, int timeouts, hostent* hostent) { + static_cast(arg)->onAresHostCallback(status, hostent); + UNREFERENCED_PARAMETER(timeouts); + }, this); +} + } // Network } // Envoy diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index 7980994cd8072..037ad43b8e137 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -29,7 +29,8 @@ class DnsResolverImpl : public DnsResolver { ~DnsResolverImpl() override; // Network::DnsResolver - ActiveDnsQuery* resolve(const std::string& dns_name, ResolveCb callback) override; + ActiveDnsQuery* resolve(const std::string& dns_name, const std::string& dns_lookup_ip_version, + ResolveCb callback) override; private: friend class DnsResolverImplPeer; @@ -53,6 +54,13 @@ class DnsResolverImpl : public DnsResolver { bool completed_ = false; // Was the query cancelled via cancel()? bool cancelled_ = false; + // If dns_lookup_ip_version is "auto", fallback to v6 address if v4 + // resolution failed. + bool fallback_if_failed = false; + ares_channel channel_; + std::string dns_name_; + + void getHostByName(int family); }; // Callback for events on sockets tracked in events_. diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index f974a50574f82..7c0a6503496ea 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -18,14 +18,18 @@ LogicalDnsCluster::LogicalDnsCluster(const Json::Object& config, Runtime::Loader : ClusterImplBase(config, runtime, stats, ssl_context_manager), dns_resolver_(dns_resolver), dns_refresh_rate_ms_( std::chrono::milliseconds(config.getInteger("dns_refresh_rate_ms", 5000))), - tls_(tls), tls_slot_(tls.allocateSlot()), initialized_(false), + dns_lookup_ip_version_(config.getString("dns_lookup_ip_version", "v4_only")), tls_(tls), + tls_slot_(tls.allocateSlot()), initialized_(false), resolve_timer_(dispatcher.createTimer([this]() -> void { startResolve(); })) { - std::vector hosts_json = config.getObjectArray("hosts"); if (hosts_json.size() != 1) { throw EnvoyException("logical_dns clusters must have a single host"); } - + if (dns_lookup_ip_version_ != "v4_only" && dns_lookup_ip_version_ != "v6_only" && + dns_lookup_ip_version_ != "auto") { + throw EnvoyException( + fmt::format("unknown dns_lookup_ip_version option {}", dns_lookup_ip_version_)); + } dns_url_ = hosts_json[0]->getString("url"); hostname_ = Network::Utility::hostFromTcpUrl(dns_url_); Network::Utility::portFromTcpUrl(dns_url_); @@ -51,18 +55,26 @@ void LogicalDnsCluster::startResolve() { info_->stats().update_attempt_.inc(); active_dns_query_ = dns_resolver_.resolve( - dns_address, [this, dns_address]( - std::list&& address_list) -> void { + dns_address, dns_lookup_ip_version_, + [this, dns_address]( + std::list&& address_list) -> void { active_dns_query_ = nullptr; log_debug("async DNS resolution complete for {}", dns_address); info_->stats().update_success_.inc(); if (!address_list.empty()) { - // TODO(mattklein123): IPv6 support as well as moving port handling into the DNS - // interface. - Network::Address::InstanceConstSharedPtr new_address( - new Network::Address::Ipv4Instance(address_list.front()->ip()->addressAsString(), - Network::Utility::portFromTcpUrl(dns_url_))); + // TODO(mattklein123): Move port handling into the DNS interface. + Network::Address::IpVersion version = address_list.front()->ip()->version(); + Network::Address::InstanceConstSharedPtr new_address; + if (version == Network::Address::IpVersion::v4) { + new_address.reset( + new Network::Address::Ipv4Instance(address_list.front()->ip()->addressAsString(), + Network::Utility::portFromTcpUrl(dns_url_))); + } else { + new_address.reset( + new Network::Address::Ipv6Instance(address_list.front()->ip()->addressAsString(), + Network::Utility::portFromTcpUrl(dns_url_))); + } if (!current_resolved_address_ || !(*new_address == *current_resolved_address_)) { current_resolved_address_ = new_address; // Capture URL to avoid a race with another update. @@ -77,8 +89,13 @@ void LogicalDnsCluster::startResolve() { // to show the friendly DNS name in that output, but currently there is no way to // express a DNS name inside of an Address::Instance. For now this is OK but we might // want to do better again later. - logical_host_.reset(new LogicalHost( - info_, hostname_, Network::Utility::resolveUrl("tcp://0.0.0.0:0"), *this)); + if (version == Network::Address::IpVersion::v4) { + logical_host_.reset( + new LogicalHost(info_, hostname_, Network::Utility::getIpv4AnyAddress(), *this)); + } else { + logical_host_.reset( + new LogicalHost(info_, hostname_, Network::Utility::getIpv6AnyAddress(), *this)); + } HostVectorSharedPtr new_hosts(new std::vector()); new_hosts->emplace_back(logical_host_); updateHosts(new_hosts, createHealthyHostList(*new_hosts), empty_host_lists_, diff --git a/source/common/upstream/logical_dns_cluster.h b/source/common/upstream/logical_dns_cluster.h index 6572ff901412c..bceb26236111d 100644 --- a/source/common/upstream/logical_dns_cluster.h +++ b/source/common/upstream/logical_dns_cluster.h @@ -88,6 +88,7 @@ class LogicalDnsCluster : public ClusterImplBase { Network::DnsResolver& dns_resolver_; const std::chrono::milliseconds dns_refresh_rate_ms_; + const std::string dns_lookup_ip_version_; ThreadLocal::Instance& tls_; uint32_t tls_slot_; std::function initialize_callback_; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 9289f793c119b..78cbe44d96012 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -369,7 +369,13 @@ StrictDnsClusterImpl::StrictDnsClusterImpl(const Json::Object& config, Runtime:: Event::Dispatcher& dispatcher) : BaseDynamicClusterImpl(config, runtime, stats, ssl_context_manager), dns_resolver_(dns_resolver), dns_refresh_rate_ms_(std::chrono::milliseconds( - config.getInteger("dns_refresh_rate_ms", 5000))) { + config.getInteger("dns_refresh_rate_ms", 5000))), + dns_lookup_ip_version_(config.getString("dns_lookup_ip_version", "v4_only")) { + if (dns_lookup_ip_version_ != "v4_only" && dns_lookup_ip_version_ != "v6_only" && + dns_lookup_ip_version_ != "auto") { + throw EnvoyException( + fmt::format("unknown dns_lookup_ip_version option {}", dns_lookup_ip_version_)); + } for (Json::ObjectSharedPtr host : config.getObjectArray("hosts")) { resolve_targets_.emplace_back(new ResolveTarget(*this, dispatcher, host->getString("url"))); } @@ -413,7 +419,7 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { parent_.info_->stats().update_attempt_.inc(); active_query_ = parent_.dns_resolver_.resolve( - dns_address_, + dns_address_, parent_.dns_lookup_ip_version_, [this](std::list&& address_list) -> void { active_query_ = nullptr; log_debug("async DNS resolution complete for {}", dns_address_); @@ -424,11 +430,19 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { // TODO(mattklein123): Currently the DNS interface does not consider port. We need to make // a new address that has port in it. We need to both support IPv6 as well as potentially // move port handling into the DNS interface itself, which would work better for SRV. - new_hosts.emplace_back(new HostImpl( - parent_.info_, dns_address_, - Network::Address::InstanceConstSharedPtr{ - new Network::Address::Ipv4Instance(address->ip()->addressAsString(), port_)}, - false, 1, "")); + if (address->ip()->version() == Network::Address::IpVersion::v4) { + new_hosts.emplace_back(new HostImpl( + parent_.info_, dns_address_, + Network::Address::InstanceConstSharedPtr{ + new Network::Address::Ipv4Instance(address->ip()->addressAsString(), port_)}, + false, 1, "")); + } else { + new_hosts.emplace_back(new HostImpl( + parent_.info_, dns_address_, + Network::Address::InstanceConstSharedPtr{ + new Network::Address::Ipv6Instance(address->ip()->addressAsString(), port_)}, + false, 1, "")); + } } std::vector hosts_added; diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index c340cd0a88b5f..16249451c1924 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -350,6 +350,7 @@ class StrictDnsClusterImpl : public BaseDynamicClusterImpl { Network::DnsResolver& dns_resolver_; std::list resolve_targets_; const std::chrono::milliseconds dns_refresh_rate_ms_; + const std::string dns_lookup_ip_version_; }; } // Upstream diff --git a/test/common/network/BUILD b/test/common/network/BUILD index 96a7fdc6895da..4588070ffd84f 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -63,6 +63,8 @@ envoy_cc_test( "//source/common/network:listen_socket_lib", "//source/common/stats:stats_lib", "//test/mocks/network:network_mocks", + "//test/test_common:environment_lib", + "//test/test_common:network_utility_lib", ], ) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 93a3f92853333..5464027807387 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -20,6 +20,8 @@ #include "common/stats/stats_impl.h" #include "test/mocks/network/mocks.h" +#include "test/test_common/environment.h" +#include "test/test_common/network_utility.h" #include "test/test_common/printers.h" #include "ares.h" @@ -35,13 +37,14 @@ namespace { typedef std::list IpList; // Map from hostname to IpList. typedef std::unordered_map HostMap; - // Represents a single TestDnsServer query state and lifecycle. This implements // just enough of RFC 1035 to handle queries we generate in the tests below. +enum record_type { A, AAAA }; + class TestDnsServerQuery { public: - TestDnsServerQuery(ConnectionPtr connection, const HostMap& hosts) - : connection_(std::move(connection)), hosts_(hosts) { + TestDnsServerQuery(ConnectionPtr connection, const HostMap& hosts_A, const HostMap& hosts_AAAA) + : connection_(std::move(connection)), hosts_A_(hosts_A), hosts_AAAA_(hosts_AAAA) { connection_->addReadFilter(Network::ReadFilterSharedPtr{new ReadFilter(*this)}); } @@ -88,19 +91,33 @@ class TestDnsServerQuery { // The number of bytes the encoded question name takes up in the request. // Useful in the response when generating resource records containing the // name. - long question_len; + long name_len; + // Get host name from query and use the name to lookup a record + // in a host maap. If the query type is of type A, then perform the lookup in + // the hosts_A_ host map. If the query type is of type AAAA, then perform the + // lookup in the hosts_AAAA_ host map. char* name; - ASSERT_EQ(ARES_SUCCESS, ares_expand_name(question, request, size_, &name, &question_len)); - auto it = parent_.hosts_.find(name); + ASSERT_EQ(ARES_SUCCESS, ares_expand_name(question, request, size_, &name, &name_len)); const std::list* ips = nullptr; - if (it != parent_.hosts_.end()) { - ips = &it->second; + // Query type. We only expect for resources of type A or AAAA. + const int q_type = DNS_QUESTION_TYPE(question + name_len); + ASSERT_TRUE(q_type == T_A || q_type == T_AAAA); + if (q_type == T_A) { + auto it = parent_.hosts_A_.find(name); + if (it != parent_.hosts_A_.end()) { + ips = &it->second; + } + } else { + auto it = parent_.hosts_AAAA_.find(name); + if (it != parent_.hosts_AAAA_.end()) { + ips = &it->second; + } } ares_free_string(name); // The response begins with the intial part of the request // (including the question section). - const size_t response_base_len = HFIXEDSZ + question_len + QFIXEDSZ; + const size_t response_base_len = HFIXEDSZ + name_len + QFIXEDSZ; unsigned char response_base[response_base_len]; memcpy(response_base, request, response_base_len); DNS_HEADER_SET_QR(response_base, 1); @@ -110,15 +127,26 @@ class TestDnsServerQuery { DNS_HEADER_SET_NSCOUNT(response_base, 0); DNS_HEADER_SET_ARCOUNT(response_base, 0); - // An A resource record for each IP found in the host map. - const size_t response_rest_len = - ips != nullptr ? ips->size() * (question_len + RRFIXEDSZ + sizeof(in_addr)) : 0; + // Create a resource record for each IP found in the host map. unsigned char response_rr_fixed[RRFIXEDSZ]; - DNS_RR_SET_TYPE(response_rr_fixed, T_A); + if (q_type == T_A) { + DNS_RR_SET_TYPE(response_rr_fixed, T_A); + DNS_RR_SET_LEN(response_rr_fixed, sizeof(in_addr)); + } else { + DNS_RR_SET_TYPE(response_rr_fixed, T_AAAA); + DNS_RR_SET_LEN(response_rr_fixed, sizeof(in6_addr)); + } DNS_RR_SET_CLASS(response_rr_fixed, C_IN); DNS_RR_SET_TTL(response_rr_fixed, 0); - DNS_RR_SET_LEN(response_rr_fixed, sizeof(in_addr)); + size_t response_rest_len; + if (q_type == T_A) { + response_rest_len = + ips != nullptr ? ips->size() * (name_len + RRFIXEDSZ + sizeof(in_addr)) : 0; + } else { + response_rest_len = + ips != nullptr ? ips->size() * (name_len + RRFIXEDSZ + sizeof(in6_addr)) : 0; + } // Send response to client. const uint16_t response_size_n = htons(response_base_len + response_rest_len); Buffer::OwnedImpl write_buffer_; @@ -126,11 +154,17 @@ class TestDnsServerQuery { write_buffer_.add(response_base, response_base_len); if (ips != nullptr) { for (auto it : *ips) { - write_buffer_.add(question, question_len); - write_buffer_.add(response_rr_fixed, 10); - in_addr addr; - ASSERT_EQ(1, inet_pton(AF_INET, it.c_str(), &addr)); - write_buffer_.add(&addr, sizeof(addr)); + write_buffer_.add(question, name_len); + write_buffer_.add(response_rr_fixed, RRFIXEDSZ); + if (q_type == T_A) { + in_addr addr; + ASSERT_EQ(1, inet_pton(AF_INET, it.c_str(), &addr)); + write_buffer_.add(&addr, sizeof(addr)); + } else { + in6_addr addr; + ASSERT_EQ(1, inet_pton(AF_INET6, it.c_str(), &addr)); + write_buffer_.add(&addr, sizeof(addr)); + } } } parent_.connection_->write(write_buffer_); @@ -152,20 +186,29 @@ class TestDnsServerQuery { private: ConnectionPtr connection_; - const HostMap& hosts_; + const HostMap& hosts_A_; + const HostMap& hosts_AAAA_; }; class TestDnsServer : public ListenerCallbacks { public: void onNewConnection(ConnectionPtr&& new_connection) override { - TestDnsServerQuery* query = new TestDnsServerQuery(std::move(new_connection), hosts_); + TestDnsServerQuery* query = + new TestDnsServerQuery(std::move(new_connection), hosts_A_, hosts_AAAA_); queries_.emplace_back(query); } - void addHosts(const std::string& hostname, const IpList& ip) { hosts_[hostname] = ip; } + void addHosts(const std::string& hostname, const IpList& ip, const record_type type) { + if (type == A) { + hosts_A_[hostname] = ip; + } else if (type == AAAA) { + hosts_AAAA_[hostname] = ip; + } + } private: - HostMap hosts_; + HostMap hosts_A_; + HostMap hosts_AAAA_; // All queries are tracked so we can do resource reclamation when the test is // over. std::vector> queries_; @@ -197,7 +240,7 @@ class DnsResolverImplPeer { DnsResolverImpl* resolver_; }; -class DnsImplTest : public testing::Test { +class DnsImplTest : public testing::TestWithParam { public: void SetUp() override { resolver_ = dispatcher_.createDnsResolver(); @@ -205,7 +248,7 @@ class DnsImplTest : public testing::Test { // Instantiate TestDnsServer and listen on a random port on the loopback address. server_.reset(new TestDnsServer()); socket_.reset( - new Network::TcpListenSocket(Network::Utility::getCanonicalIpv4LoopbackAddress(), true)); + new Network::TcpListenSocket(Network::Test::getCanonicalLoopbackAddress(GetParam()), true)); listener_ = dispatcher_.createListener(connection_handler_, *socket_, *server_, stats_store_, {.bind_to_port_ = true, .use_proxy_proto_ = false, @@ -247,13 +290,17 @@ static bool hasAddress(const std::list& results return false; } +// Parameterized test dns server socket address. +INSTANTIATE_TEST_CASE_P(IpVersions, DnsImplTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest())); + // Validate that when DnsResolverImpl is destructed with outstanding requests, // that we don't invoke any callbacks. This is a regression test from // development, where segfaults were encountered due to callback invocations on // destruction. -TEST_F(DnsImplTest, DestructPending) { +TEST_P(DnsImplTest, DestructPending) { EXPECT_NE(nullptr, resolver_->resolve( - "", [&](std::list&& results) -> void { + "", "", [&](std::list&& results) -> void { FAIL(); UNREFERENCED_PARAMETER(results); })); @@ -265,10 +312,10 @@ TEST_F(DnsImplTest, DestructPending) { // Validate basic success/fail lookup behavior. The empty request will connect // to TestDnsServer, but localhost should resolve via the hosts file with no // asynchronous behavior or network events. -TEST_F(DnsImplTest, LocalLookup) { +TEST_P(DnsImplTest, LocalLookup) { std::list address_list; EXPECT_NE(nullptr, resolver_->resolve( - "", [&](std::list&& results) -> void { + "", "", [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); })); @@ -276,19 +323,104 @@ TEST_F(DnsImplTest, LocalLookup) { dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(address_list.empty()); - EXPECT_EQ(nullptr, resolver_->resolve("localhost", - [&](std::list&& results) - -> void { address_list = results; })); - EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); + if (GetParam() == Address::IpVersion::v4) { + EXPECT_EQ(nullptr, resolver_->resolve("localhost", "auto", + [&](std::list&& results) + -> void { address_list = results; })); + EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); + EXPECT_FALSE(hasAddress(address_list, "::1")); + + EXPECT_EQ(nullptr, resolver_->resolve("localhost", "v4_only", + [&](std::list&& results) + -> void { address_list = results; })); + EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); + EXPECT_FALSE(hasAddress(address_list, "::1")); + } + + if (GetParam() == Address::IpVersion::v6) { + EXPECT_EQ(nullptr, resolver_->resolve("localhost", "v6_only", + [&](std::list&& results) + -> void { address_list = results; })); + EXPECT_TRUE(hasAddress(address_list, "::1")); + EXPECT_FALSE(hasAddress(address_list, "127.0.0.1")); + } +} + +TEST_P(DnsImplTest, Fallback) { + std::list address_list; + server_->addHosts("some.good.domain", {"1::2"}, AAAA); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", "auto", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(hasAddress(address_list, "1::2")); + + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", "v4_only", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_FALSE(hasAddress(address_list, "1::2")); + + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", "v6_only", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(hasAddress(address_list, "1::2")); +} + +TEST_P(DnsImplTest, DnsIpAddressVersion) { + std::list address_list; + server_->addHosts("some.good.domain", {"1.2.3.4"}, A); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", "auto", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); + + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", "v4_only", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); + + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", "v6_only", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_FALSE(hasAddress(address_list, "1.2.3.4")); } // Validate success/fail lookup behavior via TestDnsServer. This exercises the // network event handling in DnsResolverImpl. -TEST_F(DnsImplTest, RemoteAsyncLookup) { - server_->addHosts("some.good.domain", {"201.134.56.7"}); +TEST_P(DnsImplTest, RemoteAsyncLookup) { + server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.bad.domain", + resolver_->resolve("some.bad.domain", "auto", [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -298,7 +430,7 @@ TEST_F(DnsImplTest, RemoteAsyncLookup) { EXPECT_TRUE(address_list.empty()); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", + resolver_->resolve("some.good.domain", "auto", [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -309,11 +441,11 @@ TEST_F(DnsImplTest, RemoteAsyncLookup) { } // Validate that multiple A records are correctly passed to the callback. -TEST_F(DnsImplTest, MultiARecordLookup) { - server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}); +TEST_P(DnsImplTest, MultiARecordLookup) { + server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", + resolver_->resolve("some.good.domain", "v4_only", [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -326,15 +458,15 @@ TEST_F(DnsImplTest, MultiARecordLookup) { } // Validate working of cancellation provided by ActiveDnsQuery return. -TEST_F(DnsImplTest, Cancel) { - server_->addHosts("some.good.domain", {"201.134.56.7"}); +TEST_P(DnsImplTest, Cancel) { + server_->addHosts("some.good.domain", {"201.134.56.7"}, A); ActiveDnsQuery* query = resolver_->resolve( - "some.domain", [](std::list && ) -> void { FAIL(); }); + "some.domain", "auto", [](std::list && ) -> void { FAIL(); }); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", + resolver_->resolve("some.good.domain", "auto", [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -353,11 +485,11 @@ class DnsImplZeroTimeoutTest : public DnsImplTest { }; // Validate that timeouts result in an empty callback. -TEST_F(DnsImplZeroTimeoutTest, Timeout) { - server_->addHosts("some.good.domain", {"201.134.56.7"}); +TEST_P(DnsImplZeroTimeoutTest, Timeout) { + server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", + resolver_->resolve("some.good.domain", "v4_only", [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 335eb864204e2..63dbd2f1d9274 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -669,8 +669,8 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemove) { Network::DnsResolver::ResolveCb dns_callback; Event::MockTimer* dns_timer_ = new NiceMock(&factory_.dispatcher_); Network::MockActiveDnsQuery active_dns_query; - EXPECT_CALL(factory_.dns_resolver_, resolve(_, _)) - .WillRepeatedly(DoAll(SaveArg<1>(&dns_callback), Return(&active_dns_query))); + EXPECT_CALL(factory_.dns_resolver_, resolve(_, _, _)) + .WillRepeatedly(DoAll(SaveArg<2>(&dns_callback), Return(&active_dns_query))); create(*loader); // Test for no hosts returning the correct values before we have hosts. diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index 97ff34a999b5b..ffb26857834ad 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -36,13 +36,13 @@ class LogicalDnsClusterTest : public testing::Test { cluster_->setInitializedCb([&]() -> void { initialized_.ready(); }); } - void expectResolve() { - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", _)) - .WillOnce(Invoke([&](const std::string&, Network::DnsResolver::ResolveCb cb) - -> Network::ActiveDnsQuery* { - dns_callback_ = cb; - return &active_dns_query_; - })); + void expectResolve(const std::string& dns_ip_lookup_version) { + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", dns_ip_lookup_version, _)) + .WillOnce(Invoke([&](const std::string&, const std::string&, + Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { + dns_callback_ = cb; + return &active_dns_query_; + })); } Stats::IsolatedStoreImpl stats_store_; @@ -73,6 +73,21 @@ TEST_F(LogicalDnsClusterTest, BadConfig) { EXPECT_THROW(setup(json), EnvoyException); } +TEST_F(LogicalDnsClusterTest, BadIpVersionConfig) { + std::string json = R"EOF( + { + "name": "name", + "connect_timeout_ms": 250, + "type": "logical_dns", + "lb_type": "round_robin", + "dns_lookup_ip_version": "foo", + "hosts": [{"url": "tcp://foo.bar.com:443"}] + } + )EOF"; + + EXPECT_THROW(setup(json), EnvoyException); +} + // Validate that if the DNS resolves immediately, during the LogicalDnsCluster // constructor, we have the expected host state and initialization callback // invocation. @@ -88,13 +103,67 @@ TEST_F(LogicalDnsClusterTest, ImmediateResolve) { )EOF"; EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", _)) - .WillOnce(Invoke([&](const std::string&, Network::DnsResolver::ResolveCb cb) - -> Network::ActiveDnsQuery* { - EXPECT_CALL(*resolve_timer_, enableTimer(_)); - cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); - return nullptr; - })); + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", "v4_only", _)) + .WillOnce(Invoke([&](const std::string&, const std::string&, + Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { + EXPECT_CALL(*resolve_timer_, enableTimer(_)); + cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); + return nullptr; + })); + setup(json); + EXPECT_EQ(1UL, cluster_->hosts().size()); + EXPECT_EQ(1UL, cluster_->healthyHosts().size()); + EXPECT_EQ("foo.bar.com", cluster_->hosts()[0]->hostname()); + tls_.shutdownThread(); +} + +TEST_F(LogicalDnsClusterTest, ImmediateResolveV6Only) { + std::string json = R"EOF( + { + "name": "name", + "connect_timeout_ms": 250, + "type": "logical_dns", + "lb_type": "round_robin", + "dns_lookup_ip_version": "v6_only", + "hosts": [{"url": "tcp://foo.bar.com:443"}] + } + )EOF"; + + EXPECT_CALL(initialized_, ready()); + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", "v6_only", _)) + .WillOnce(Invoke([&](const std::string&, const std::string&, + Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { + EXPECT_CALL(*resolve_timer_, enableTimer(_)); + cb(TestUtility::makeDnsResponse({"::1"})); + return nullptr; + })); + setup(json); + EXPECT_EQ(1UL, cluster_->hosts().size()); + EXPECT_EQ(1UL, cluster_->healthyHosts().size()); + EXPECT_EQ("foo.bar.com", cluster_->hosts()[0]->hostname()); + tls_.shutdownThread(); +} + +TEST_F(LogicalDnsClusterTest, ImmediateResolveAuto) { + std::string json = R"EOF( + { + "name": "name", + "connect_timeout_ms": 250, + "type": "logical_dns", + "lb_type": "round_robin", + "dns_lookup_ip_version": "auto", + "hosts": [{"url": "tcp://foo.bar.com:443"}] + } + )EOF"; + + EXPECT_CALL(initialized_, ready()); + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", "auto", _)) + .WillOnce(Invoke([&](const std::string&, const std::string&, + Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { + EXPECT_CALL(*resolve_timer_, enableTimer(_)); + cb(TestUtility::makeDnsResponse({"::1"})); + return nullptr; + })); setup(json); EXPECT_EQ(1UL, cluster_->hosts().size()); EXPECT_EQ(1UL, cluster_->healthyHosts().size()); @@ -114,7 +183,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { } )EOF"; - expectResolve(); + expectResolve("v4_only"); setup(json); EXPECT_CALL(membership_updated_, ready()); @@ -135,7 +204,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { logical_host->createConnection(dispatcher_); logical_host->outlierDetector().putHttpResponseCode(200); - expectResolve(); + expectResolve("v4_only"); resolve_timer_->callback_(); // Should not cause any changes. @@ -155,7 +224,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { EXPECT_EQ("foo.bar.com", data.host_description_->hostname()); data.host_description_->outlierDetector().putHttpResponseCode(200); - expectResolve(); + expectResolve("v4_only"); resolve_timer_->callback_(); // Should cause a change. @@ -168,7 +237,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { .WillOnce(Return(new NiceMock())); logical_host->createConnection(dispatcher_); - expectResolve(); + expectResolve("v4_only"); resolve_timer_->callback_(); // Empty should not cause any change. @@ -183,7 +252,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { // Make sure we cancel. EXPECT_CALL(active_dns_query_, cancel()); - expectResolve(); + expectResolve("v4_only"); resolve_timer_->callback_(); tls_.shutdownThread(); diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 8f4101751823b..80209496b9f8f 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -47,12 +47,12 @@ struct ResolverData { } void expectResolve(Network::MockDnsResolver& dns_resolver) { - EXPECT_CALL(dns_resolver, resolve(_, _)) - .WillOnce(Invoke([&](const std::string&, Network::DnsResolver::ResolveCb cb) - -> Network::ActiveDnsQuery* { - dns_callback_ = cb; - return &active_dns_query_; - })) + EXPECT_CALL(dns_resolver, resolve(_, _, _)) + .WillOnce(Invoke([&](const std::string&, const std::string&, + Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { + dns_callback_ = cb; + return &active_dns_query_; + })) .RetiresOnSaturation(); } @@ -80,12 +80,80 @@ TEST(StrictDnsClusterImplTest, ImmediateResolve) { )EOF"; EXPECT_CALL(initialized, ready()); - EXPECT_CALL(dns_resolver, resolve("foo.bar.com", _)) - .WillOnce(Invoke([&](const std::string&, Network::DnsResolver::ResolveCb cb) - -> Network::ActiveDnsQuery* { - cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); - return nullptr; - })); + EXPECT_CALL(dns_resolver, resolve("foo.bar.com", _, _)) + .WillOnce(Invoke([&](const std::string&, const std::string&, + Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { + cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); + return nullptr; + })); + Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); + StrictDnsClusterImpl cluster(*loader, runtime, stats, ssl_context_manager, dns_resolver, + dispatcher); + cluster.setInitializedCb([&]() -> void { initialized.ready(); }); + EXPECT_EQ(2UL, cluster.hosts().size()); + EXPECT_EQ(2UL, cluster.healthyHosts().size()); +} + +TEST(StrictDnsClusterImplTest, ImmediateResolveV6Only) { + Stats::IsolatedStoreImpl stats; + Ssl::MockContextManager ssl_context_manager; + NiceMock dns_resolver; + NiceMock dispatcher; + NiceMock runtime; + ReadyWatcher initialized; + + std::string json = R"EOF( + { + "name": "name", + "connect_timeout_ms": 250, + "type": "strict_dns", + "lb_type": "round_robin", + "hosts": [{"url": "tcp://foo.bar.com:443"}], + "dns_lookup_ip_version" : "v6_only" + } + )EOF"; + + EXPECT_CALL(initialized, ready()); + EXPECT_CALL(dns_resolver, resolve("foo.bar.com", "v6_only", _)) + .WillOnce(Invoke([&](const std::string&, const std::string&, + Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { + cb(TestUtility::makeDnsResponse({"::1", "::2"})); + return nullptr; + })); + Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); + StrictDnsClusterImpl cluster(*loader, runtime, stats, ssl_context_manager, dns_resolver, + dispatcher); + cluster.setInitializedCb([&]() -> void { initialized.ready(); }); + EXPECT_EQ(2UL, cluster.hosts().size()); + EXPECT_EQ(2UL, cluster.healthyHosts().size()); +} + +TEST(StrictDnsClusterImplTest, ImmediateResolveAuto) { + Stats::IsolatedStoreImpl stats; + Ssl::MockContextManager ssl_context_manager; + NiceMock dns_resolver; + NiceMock dispatcher; + NiceMock runtime; + ReadyWatcher initialized; + + std::string json = R"EOF( + { + "name": "name", + "connect_timeout_ms": 250, + "type": "strict_dns", + "lb_type": "round_robin", + "hosts": [{"url": "tcp://foo.bar.com:443"}], + "dns_lookup_ip_version" : "auto" + } + )EOF"; + + EXPECT_CALL(initialized, ready()); + EXPECT_CALL(dns_resolver, resolve("foo.bar.com", "auto", _)) + .WillOnce(Invoke([&](const std::string&, const std::string&, + Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { + cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); + return nullptr; + })); Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); StrictDnsClusterImpl cluster(*loader, runtime, stats, ssl_context_manager, dns_resolver, dispatcher); @@ -473,5 +541,21 @@ TEST(ClusterDefinitionTest, BadClusterConfig) { EXPECT_THROW(loader->validateSchema(Json::Schema::CLUSTER_SCHEMA), Json::Exception); } +TEST(ClusterDefinitionTest, BadDnsIpVersionClusterConfig) { + std::string json = R"EOF( + { + "name": "cluster_1", + "connect_timeout_ms": 250, + "type": "static", + "lb_type": "round_robin", + "hosts": [{"url": "tcp://127.0.0.1:11001"}], + "dns_lookup_ip_version" : "foo" + } + )EOF"; + + Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); + EXPECT_THROW(loader->validateSchema(Json::Schema::CLUSTER_SCHEMA), Json::Exception); +} + } // Upstream } // Envoy diff --git a/test/mocks/network/mocks.cc b/test/mocks/network/mocks.cc index 119db644ee862..5bca23db36b65 100644 --- a/test/mocks/network/mocks.cc +++ b/test/mocks/network/mocks.cc @@ -76,7 +76,7 @@ MockActiveDnsQuery::MockActiveDnsQuery() {} MockActiveDnsQuery::~MockActiveDnsQuery() {} MockDnsResolver::MockDnsResolver() { - ON_CALL(*this, resolve(_, _)).WillByDefault(Return(&active_query_)); + ON_CALL(*this, resolve(_, _, _)).WillByDefault(Return(&active_query_)); } MockDnsResolver::~MockDnsResolver() {} diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 170b09896e968..19b7cb6d589a8 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -117,7 +117,9 @@ class MockDnsResolver : public DnsResolver { ~MockDnsResolver(); // Network::DnsResolver - MOCK_METHOD2(resolve, ActiveDnsQuery*(const std::string& dns_name, ResolveCb callback)); + MOCK_METHOD3(resolve, + ActiveDnsQuery*(const std::string& dns_name, + const std::string& dns_lookup_ip_version, ResolveCb callback)); testing::NiceMock active_query_; }; diff --git a/test/test_common/BUILD b/test/test_common/BUILD index c28731ba1ba23..f66fb67d466b8 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -63,5 +63,6 @@ envoy_cc_test_library( "//source/common/common:empty_string", "//source/common/http:header_map_lib", "//source/common/network:address_lib", + "//source/common/network:utility_lib", ], ) diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index f8d2b4934f909..013ab2b90d8fe 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -14,6 +14,7 @@ #include "common/common/empty_string.h" #include "common/network/address_impl.h" +#include "common/network/utility.h" #include "test/test_common/printers.h" @@ -65,7 +66,7 @@ std::list TestUtility::makeDnsResponse(const std::list& addresses) { std::list ret; for (auto address : addresses) { - ret.emplace_back(new Network::Address::Ipv4Instance(address)); + ret.emplace_back(Network::Utility::parseInternetAddress(address)); } return ret; } From 41f290f9c53556a7ba6b687943d1a066db84ecef Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Fri, 19 May 2017 17:49:47 -0400 Subject: [PATCH 02/12] Fix docs --- docs/configuration/cluster_manager/cluster.rst | 10 +++++----- test/common/network/dns_impl_test.cc | 5 +++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/docs/configuration/cluster_manager/cluster.rst b/docs/configuration/cluster_manager/cluster.rst index a5f0ebac9f3b9..a4715fb08dd52 100644 --- a/docs/configuration/cluster_manager/cluster.rst +++ b/docs/configuration/cluster_manager/cluster.rst @@ -145,14 +145,14 @@ dns_refresh_rate_ms the value defaults to 5000. For cluster types other than *strict_dns* and *logical_dns* this setting is ignored. -.. _config_cluster_manager_cluster_dns_refresh_rate_ms: +.. _config_cluster_manager_cluster_dns_lookup_ip_version: dns_loopkup_ip_version - *(optional, string)* The dns IP address version lookup policy. The options are *v4_only*, *v6_only*, + *(optional, string)* The DNS IP address version lookup policy. The options are *v4_only*, *v6_only*, and *auto*. If this setting is not specified, the value defaults to *v4_only*. When *v4_only* is selected, - the dns resolver will only perform a lookup for addresses in the IPv4 family. If *v6_only* is selected, - the dns resolver will only perform a lookup for addresses in the IPv6 family. If *auto* is specified, - the dns resolver will first perform a lookup for addresses in the IPv4 family and fallback to a lookup for + the DNS resolver will only perform a lookup for addresses in the IPv4 family. If *v6_only* is selected, + the DNS resolver will only perform a lookup for addresses in the IPv6 family. If *auto* is specified, + the DNS resolver will first perform a lookup for addresses in the IPv4 family and fallback to a lookup for addresses in the IPv6 family. For cluster types other than *strict_dns* and *logical_dns*, this setting is ignored. diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 5464027807387..a59d4c8d9fe75 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -461,8 +461,9 @@ TEST_P(DnsImplTest, MultiARecordLookup) { TEST_P(DnsImplTest, Cancel) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); - ActiveDnsQuery* query = resolver_->resolve( - "some.domain", "auto", [](std::list && ) -> void { FAIL(); }); + ActiveDnsQuery* query = + resolver_->resolve("some.domain", "auto", + [](std::list && ) -> void { FAIL(); }); std::list address_list; EXPECT_NE(nullptr, From c6ca1618725cd2a619406bb018296e3ffdff7383 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Mon, 22 May 2017 15:48:30 -0400 Subject: [PATCH 03/12] Response to comments --- .../configuration/cluster_manager/cluster.rst | 16 +-- include/envoy/network/dns.h | 6 +- source/common/json/config_schemas.cc | 2 +- source/common/network/dns_impl.cc | 12 +- source/common/network/dns_impl.h | 4 +- source/common/upstream/logical_dns_cluster.cc | 18 +-- source/common/upstream/logical_dns_cluster.h | 2 +- source/common/upstream/upstream_impl.cc | 21 ++-- source/common/upstream/upstream_impl.h | 2 +- test/common/network/dns_impl_test.cc | 103 +++++++++++++----- .../upstream/logical_dns_cluster_test.cc | 47 +++----- test/common/upstream/upstream_impl_test.cc | 22 ++-- test/mocks/network/mocks.h | 2 +- 13 files changed, 149 insertions(+), 108 deletions(-) diff --git a/docs/configuration/cluster_manager/cluster.rst b/docs/configuration/cluster_manager/cluster.rst index a4715fb08dd52..c3176ef1c7067 100644 --- a/docs/configuration/cluster_manager/cluster.rst +++ b/docs/configuration/cluster_manager/cluster.rst @@ -20,7 +20,7 @@ Cluster "features": "...", "http_codec_options": "...", "dns_refresh_rate_ms": "...", - "dns_lookup_ip_version": "...", + "dns_lookup_family": "...", "outlier_detection": "..." } @@ -145,15 +145,15 @@ dns_refresh_rate_ms the value defaults to 5000. For cluster types other than *strict_dns* and *logical_dns* this setting is ignored. -.. _config_cluster_manager_cluster_dns_lookup_ip_version: +.. _config_cluster_manager_cluster_dns_lookup_family: -dns_loopkup_ip_version - *(optional, string)* The DNS IP address version lookup policy. The options are *v4_only*, *v6_only*, - and *auto*. If this setting is not specified, the value defaults to *v4_only*. When *v4_only* is selected, +dns_lookup_family + *(optional, string)* The DNS IP address resolution policy. The options are *v4_only*, *v6_only*, + and *fallback*. If this setting is not specified, the value defaults to *v4_only*. When *v4_only* is selected, the DNS resolver will only perform a lookup for addresses in the IPv4 family. If *v6_only* is selected, - the DNS resolver will only perform a lookup for addresses in the IPv6 family. If *auto* is specified, - the DNS resolver will first perform a lookup for addresses in the IPv4 family and fallback to a lookup for - addresses in the IPv6 family. For cluster types other than *strict_dns* and *logical_dns*, this setting + the DNS resolver will only perform a lookup for addresses in the IPv6 family. If *fallback* is specified, + the DNS resolver will first perform a lookup for addresses in the IPv6 family and fallback to a lookup for + addresses in the IPv4 family. For cluster types other than *strict_dns* and *logical_dns*, this setting is ignored. .. _config_cluster_manager_cluster_outlier_detection: diff --git a/include/envoy/network/dns.h b/include/envoy/network/dns.h index 381b89e394aa7..e900636efc47d 100644 --- a/include/envoy/network/dns.h +++ b/include/envoy/network/dns.h @@ -24,6 +24,8 @@ class ActiveDnsQuery { virtual void cancel() PURE; }; +enum class DnsLookupFamily { v4_only, v6_only, fallback }; + /** * An asynchronous DNS resolver. */ @@ -41,13 +43,13 @@ class DnsResolver { /** * Initiate an async DNS resolution. * @param dns_name supplies the DNS name to lookup. - * @param dns_lookup_ip_version the DNS IP version lookup policy. + * @param dns_lookup_family the DNS IP version lookup policy. * @param callback supplies the callback to invoke when the resolution is complete. * @return if non-null, a handle that can be used to cancel the resolution. * This is only valid until the invocation of callback or ~DnsResolver(). */ virtual ActiveDnsQuery* resolve(const std::string& dns_name, - const std::string& dns_lookup_ip_version, + const DnsLookupFamily& dns_lookup_family, ResolveCb callback) PURE; }; diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 0e9f158db1bfd..c427fd80611b7 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -1210,7 +1210,7 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( "minimum" : 0, "exclusiveMinimum" : true }, - "dns_lookup_ip_version" : { + "dns_lookup_family" : { "type" : "string", "enum" : ["v4_only", "v6_only", "auto"] }, diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 67b34fa8471e2..745033e219863 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -81,7 +81,7 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, hostent* } } else if (fallback_if_failed) { fallback_if_failed = false; - getHostByName(AF_INET6); + getHostByName(AF_INET); } if (!cancelled_ && completed_) { callback_(std::move(address_list)); @@ -132,20 +132,20 @@ void DnsResolverImpl::onAresSocketStateChange(int fd, int read, int write) { } ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, - const std::string& dns_lookup_ip_version, + const DnsLookupFamily& dns_lookup_family, ResolveCb callback) { std::unique_ptr pending_resolution(new PendingResolution()); pending_resolution->callback_ = callback; pending_resolution->channel_ = channel_; pending_resolution->dns_name_ = dns_name; - if (dns_lookup_ip_version == "auto") { + if (dns_lookup_family == DnsLookupFamily::fallback) { pending_resolution->fallback_if_failed = true; } - if (dns_lookup_ip_version == "v6_only") { - pending_resolution->getHostByName(AF_INET6); - } else { + if (dns_lookup_family == DnsLookupFamily::v4_only) { pending_resolution->getHostByName(AF_INET); + } else { + pending_resolution->getHostByName(AF_INET6); } if (pending_resolution->completed_) { diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index 037ad43b8e137..1138268f6942b 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -29,7 +29,7 @@ class DnsResolverImpl : public DnsResolver { ~DnsResolverImpl() override; // Network::DnsResolver - ActiveDnsQuery* resolve(const std::string& dns_name, const std::string& dns_lookup_ip_version, + ActiveDnsQuery* resolve(const std::string& dns_name, const DnsLookupFamily& dns_lookup_family, ResolveCb callback) override; private: @@ -54,7 +54,7 @@ class DnsResolverImpl : public DnsResolver { bool completed_ = false; // Was the query cancelled via cancel()? bool cancelled_ = false; - // If dns_lookup_ip_version is "auto", fallback to v6 address if v4 + // If dns_lookup_family is "fallback", fallback to v4 address if v6 // resolution failed. bool fallback_if_failed = false; ares_channel channel_; diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index 7c0a6503496ea..0c5ee0cffcd3b 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -18,17 +18,21 @@ LogicalDnsCluster::LogicalDnsCluster(const Json::Object& config, Runtime::Loader : ClusterImplBase(config, runtime, stats, ssl_context_manager), dns_resolver_(dns_resolver), dns_refresh_rate_ms_( std::chrono::milliseconds(config.getInteger("dns_refresh_rate_ms", 5000))), - dns_lookup_ip_version_(config.getString("dns_lookup_ip_version", "v4_only")), tls_(tls), - tls_slot_(tls.allocateSlot()), initialized_(false), + tls_(tls), tls_slot_(tls.allocateSlot()), initialized_(false), resolve_timer_(dispatcher.createTimer([this]() -> void { startResolve(); })) { std::vector hosts_json = config.getObjectArray("hosts"); if (hosts_json.size() != 1) { throw EnvoyException("logical_dns clusters must have a single host"); } - if (dns_lookup_ip_version_ != "v4_only" && dns_lookup_ip_version_ != "v6_only" && - dns_lookup_ip_version_ != "auto") { - throw EnvoyException( - fmt::format("unknown dns_lookup_ip_version option {}", dns_lookup_ip_version_)); + + // Resolve string to enum. + std::string dns_lookup_family = config.getString("dns_lookup_family", "v4_only"); + if (dns_lookup_family == "v6_only") { + dns_lookup_family_ = Network::DnsLookupFamily::v6_only; + } else if (dns_lookup_family == "fallback") { + dns_lookup_family_ = Network::DnsLookupFamily::fallback; + } else { + dns_lookup_family_ = Network::DnsLookupFamily::v4_only; } dns_url_ = hosts_json[0]->getString("url"); hostname_ = Network::Utility::hostFromTcpUrl(dns_url_); @@ -55,7 +59,7 @@ void LogicalDnsCluster::startResolve() { info_->stats().update_attempt_.inc(); active_dns_query_ = dns_resolver_.resolve( - dns_address, dns_lookup_ip_version_, + dns_address, dns_lookup_family_, [this, dns_address]( std::list&& address_list) -> void { active_dns_query_ = nullptr; diff --git a/source/common/upstream/logical_dns_cluster.h b/source/common/upstream/logical_dns_cluster.h index bceb26236111d..f3c938743ed4b 100644 --- a/source/common/upstream/logical_dns_cluster.h +++ b/source/common/upstream/logical_dns_cluster.h @@ -88,7 +88,7 @@ class LogicalDnsCluster : public ClusterImplBase { Network::DnsResolver& dns_resolver_; const std::chrono::milliseconds dns_refresh_rate_ms_; - const std::string dns_lookup_ip_version_; + Network::DnsLookupFamily dns_lookup_family_; ThreadLocal::Instance& tls_; uint32_t tls_slot_; std::function initialize_callback_; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 78cbe44d96012..dcbf0947dc2dc 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -369,13 +369,20 @@ StrictDnsClusterImpl::StrictDnsClusterImpl(const Json::Object& config, Runtime:: Event::Dispatcher& dispatcher) : BaseDynamicClusterImpl(config, runtime, stats, ssl_context_manager), dns_resolver_(dns_resolver), dns_refresh_rate_ms_(std::chrono::milliseconds( - config.getInteger("dns_refresh_rate_ms", 5000))), - dns_lookup_ip_version_(config.getString("dns_lookup_ip_version", "v4_only")) { - if (dns_lookup_ip_version_ != "v4_only" && dns_lookup_ip_version_ != "v6_only" && - dns_lookup_ip_version_ != "auto") { - throw EnvoyException( - fmt::format("unknown dns_lookup_ip_version option {}", dns_lookup_ip_version_)); + config.getInteger("dns_refresh_rate_ms", 5000))) { + // Resolve string to enum. + std::string dns_lookup_family = config.getString("dns_lookup_family", "v4_only"); + if (dns_lookup_family == "v6_only") { + dns_lookup_family_ = Network::DnsLookupFamily::v6_only; + } else if (dns_lookup_family == "fallback") { + dns_lookup_family_ = Network::DnsLookupFamily::fallback; + } else { + dns_lookup_family_ = Network::DnsLookupFamily::v4_only; } + //} else { + // throw EnvoyException(fmt::format("unknown dns_lookup_family option {}", dns_lookup_family)); + //} + for (Json::ObjectSharedPtr host : config.getObjectArray("hosts")) { resolve_targets_.emplace_back(new ResolveTarget(*this, dispatcher, host->getString("url"))); } @@ -419,7 +426,7 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { parent_.info_->stats().update_attempt_.inc(); active_query_ = parent_.dns_resolver_.resolve( - dns_address_, parent_.dns_lookup_ip_version_, + dns_address_, parent_.dns_lookup_family_, [this](std::list&& address_list) -> void { active_query_ = nullptr; log_debug("async DNS resolution complete for {}", dns_address_); diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 16249451c1924..1c62a19243edd 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -350,7 +350,7 @@ class StrictDnsClusterImpl : public BaseDynamicClusterImpl { Network::DnsResolver& dns_resolver_; std::list resolve_targets_; const std::chrono::milliseconds dns_refresh_rate_ms_; - const std::string dns_lookup_ip_version_; + Network::DnsLookupFamily dns_lookup_family_; }; } // Upstream diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index a59d4c8d9fe75..dfaae695e15b8 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -299,11 +299,12 @@ INSTANTIATE_TEST_CASE_P(IpVersions, DnsImplTest, // development, where segfaults were encountered due to callback invocations on // destruction. TEST_P(DnsImplTest, DestructPending) { - EXPECT_NE(nullptr, resolver_->resolve( - "", "", [&](std::list&& results) -> void { - FAIL(); - UNREFERENCED_PARAMETER(results); - })); + EXPECT_NE(nullptr, + resolver_->resolve("", DnsLookupFamily::v4_only, + [&](std::list&& results) -> void { + FAIL(); + UNREFERENCED_PARAMETER(results); + })); // Also validate that pending events are around to exercise the resource // reclamation path. EXPECT_GT(peer_->events().size(), 0U); @@ -314,31 +315,32 @@ TEST_P(DnsImplTest, DestructPending) { // asynchronous behavior or network events. TEST_P(DnsImplTest, LocalLookup) { std::list address_list; - EXPECT_NE(nullptr, resolver_->resolve( - "", "", [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("", DnsLookupFamily::v4_only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(address_list.empty()); if (GetParam() == Address::IpVersion::v4) { - EXPECT_EQ(nullptr, resolver_->resolve("localhost", "auto", + EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::v4_only, [&](std::list&& results) -> void { address_list = results; })); EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); EXPECT_FALSE(hasAddress(address_list, "::1")); + } - EXPECT_EQ(nullptr, resolver_->resolve("localhost", "v4_only", + if (GetParam() == Address::IpVersion::v6) { + EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::fallback, [&](std::list&& results) -> void { address_list = results; })); - EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); - EXPECT_FALSE(hasAddress(address_list, "::1")); - } + EXPECT_FALSE(hasAddress(address_list, "127.0.0.1")); + EXPECT_TRUE(hasAddress(address_list, "::1")); - if (GetParam() == Address::IpVersion::v6) { - EXPECT_EQ(nullptr, resolver_->resolve("localhost", "v6_only", + EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::v6_only, [&](std::list&& results) -> void { address_list = results; })); EXPECT_TRUE(hasAddress(address_list, "::1")); @@ -346,11 +348,11 @@ TEST_P(DnsImplTest, LocalLookup) { } } -TEST_P(DnsImplTest, Fallback) { +TEST_P(DnsImplTest, DnsIpAddressVersionV6) { std::list address_list; server_->addHosts("some.good.domain", {"1::2"}, AAAA); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", "auto", + resolver_->resolve("some.good.domain", DnsLookupFamily::fallback, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -360,7 +362,7 @@ TEST_P(DnsImplTest, Fallback) { EXPECT_TRUE(hasAddress(address_list, "1::2")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", "v4_only", + resolver_->resolve("some.good.domain", DnsLookupFamily::v4_only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -370,7 +372,7 @@ TEST_P(DnsImplTest, Fallback) { EXPECT_FALSE(hasAddress(address_list, "1::2")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", "v6_only", + resolver_->resolve("some.good.domain", DnsLookupFamily::v6_only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -384,7 +386,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { std::list address_list; server_->addHosts("some.good.domain", {"1.2.3.4"}, A); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", "auto", + resolver_->resolve("some.good.domain", DnsLookupFamily::fallback, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -394,7 +396,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", "v4_only", + resolver_->resolve("some.good.domain", DnsLookupFamily::v4_only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -404,7 +406,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", "v6_only", + resolver_->resolve("some.good.domain", DnsLookupFamily::v6_only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -420,7 +422,7 @@ TEST_P(DnsImplTest, RemoteAsyncLookup) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.bad.domain", "auto", + resolver_->resolve("some.bad.domain", DnsLookupFamily::fallback, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -430,7 +432,7 @@ TEST_P(DnsImplTest, RemoteAsyncLookup) { EXPECT_TRUE(address_list.empty()); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", "auto", + resolver_->resolve("some.good.domain", DnsLookupFamily::fallback, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -445,7 +447,24 @@ TEST_P(DnsImplTest, MultiARecordLookup) { server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", "v4_only", + resolver_->resolve("some.good.domain", DnsLookupFamily::v4_only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); + EXPECT_TRUE(hasAddress(address_list, "123.4.5.6")); + EXPECT_TRUE(hasAddress(address_list, "6.5.4.3")); +} + +TEST_P(DnsImplTest, MultiARecordLookupWithV6) { + server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}, A); + server_->addHosts("some.good.domain", {"1::2", "1::2:3", "1::2:3:4"}, AAAA); + std::list address_list; + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::v4_only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -455,6 +474,30 @@ TEST_P(DnsImplTest, MultiARecordLookup) { EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); EXPECT_TRUE(hasAddress(address_list, "123.4.5.6")); EXPECT_TRUE(hasAddress(address_list, "6.5.4.3")); + + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::fallback, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(hasAddress(address_list, "1::2")); + EXPECT_TRUE(hasAddress(address_list, "1::2:3")); + EXPECT_TRUE(hasAddress(address_list, "1::2:3:4")); + + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::v6_only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(hasAddress(address_list, "1::2")); + EXPECT_TRUE(hasAddress(address_list, "1::2:3")); + EXPECT_TRUE(hasAddress(address_list, "1::2:3:4")); } // Validate working of cancellation provided by ActiveDnsQuery return. @@ -462,12 +505,12 @@ TEST_P(DnsImplTest, Cancel) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); ActiveDnsQuery* query = - resolver_->resolve("some.domain", "auto", + resolver_->resolve("some.domain", DnsLookupFamily::fallback, [](std::list && ) -> void { FAIL(); }); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", "auto", + resolver_->resolve("some.good.domain", DnsLookupFamily::fallback, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -490,7 +533,7 @@ TEST_P(DnsImplZeroTimeoutTest, Timeout) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", "v4_only", + resolver_->resolve("some.good.domain", DnsLookupFamily::v4_only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index ffb26857834ad..04e31b69eb0ac 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -36,9 +36,9 @@ class LogicalDnsClusterTest : public testing::Test { cluster_->setInitializedCb([&]() -> void { initialized_.ready(); }); } - void expectResolve(const std::string& dns_ip_lookup_version) { - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", dns_ip_lookup_version, _)) - .WillOnce(Invoke([&](const std::string&, const std::string&, + void expectResolve(const Network::DnsLookupFamily& dns_lookup_family) { + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", dns_lookup_family, _)) + .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { dns_callback_ = cb; return &active_dns_query_; @@ -73,21 +73,6 @@ TEST_F(LogicalDnsClusterTest, BadConfig) { EXPECT_THROW(setup(json), EnvoyException); } -TEST_F(LogicalDnsClusterTest, BadIpVersionConfig) { - std::string json = R"EOF( - { - "name": "name", - "connect_timeout_ms": 250, - "type": "logical_dns", - "lb_type": "round_robin", - "dns_lookup_ip_version": "foo", - "hosts": [{"url": "tcp://foo.bar.com:443"}] - } - )EOF"; - - EXPECT_THROW(setup(json), EnvoyException); -} - // Validate that if the DNS resolves immediately, during the LogicalDnsCluster // constructor, we have the expected host state and initialization callback // invocation. @@ -103,8 +88,8 @@ TEST_F(LogicalDnsClusterTest, ImmediateResolve) { )EOF"; EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", "v4_only", _)) - .WillOnce(Invoke([&](const std::string&, const std::string&, + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::v4_only, _)) + .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { EXPECT_CALL(*resolve_timer_, enableTimer(_)); cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); @@ -124,14 +109,14 @@ TEST_F(LogicalDnsClusterTest, ImmediateResolveV6Only) { "connect_timeout_ms": 250, "type": "logical_dns", "lb_type": "round_robin", - "dns_lookup_ip_version": "v6_only", + "dns_lookup_family": "v6_only", "hosts": [{"url": "tcp://foo.bar.com:443"}] } )EOF"; EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", "v6_only", _)) - .WillOnce(Invoke([&](const std::string&, const std::string&, + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::v6_only, _)) + .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { EXPECT_CALL(*resolve_timer_, enableTimer(_)); cb(TestUtility::makeDnsResponse({"::1"})); @@ -151,14 +136,14 @@ TEST_F(LogicalDnsClusterTest, ImmediateResolveAuto) { "connect_timeout_ms": 250, "type": "logical_dns", "lb_type": "round_robin", - "dns_lookup_ip_version": "auto", + "dns_lookup_family": "fallback", "hosts": [{"url": "tcp://foo.bar.com:443"}] } )EOF"; EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", "auto", _)) - .WillOnce(Invoke([&](const std::string&, const std::string&, + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::fallback, _)) + .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { EXPECT_CALL(*resolve_timer_, enableTimer(_)); cb(TestUtility::makeDnsResponse({"::1"})); @@ -183,7 +168,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { } )EOF"; - expectResolve("v4_only"); + expectResolve(Network::DnsLookupFamily::v4_only); setup(json); EXPECT_CALL(membership_updated_, ready()); @@ -204,7 +189,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { logical_host->createConnection(dispatcher_); logical_host->outlierDetector().putHttpResponseCode(200); - expectResolve("v4_only"); + expectResolve(Network::DnsLookupFamily::v4_only); resolve_timer_->callback_(); // Should not cause any changes. @@ -224,7 +209,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { EXPECT_EQ("foo.bar.com", data.host_description_->hostname()); data.host_description_->outlierDetector().putHttpResponseCode(200); - expectResolve("v4_only"); + expectResolve(Network::DnsLookupFamily::v4_only); resolve_timer_->callback_(); // Should cause a change. @@ -237,7 +222,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { .WillOnce(Return(new NiceMock())); logical_host->createConnection(dispatcher_); - expectResolve("v4_only"); + expectResolve(Network::DnsLookupFamily::v4_only); resolve_timer_->callback_(); // Empty should not cause any change. @@ -252,7 +237,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { // Make sure we cancel. EXPECT_CALL(active_dns_query_, cancel()); - expectResolve("v4_only"); + expectResolve(Network::DnsLookupFamily::v4_only); resolve_timer_->callback_(); tls_.shutdownThread(); diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 80209496b9f8f..f761ccdfefdff 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -48,7 +48,7 @@ struct ResolverData { void expectResolve(Network::MockDnsResolver& dns_resolver) { EXPECT_CALL(dns_resolver, resolve(_, _, _)) - .WillOnce(Invoke([&](const std::string&, const std::string&, + .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { dns_callback_ = cb; return &active_dns_query_; @@ -80,8 +80,8 @@ TEST(StrictDnsClusterImplTest, ImmediateResolve) { )EOF"; EXPECT_CALL(initialized, ready()); - EXPECT_CALL(dns_resolver, resolve("foo.bar.com", _, _)) - .WillOnce(Invoke([&](const std::string&, const std::string&, + EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::v4_only, _)) + .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); return nullptr; @@ -109,13 +109,13 @@ TEST(StrictDnsClusterImplTest, ImmediateResolveV6Only) { "type": "strict_dns", "lb_type": "round_robin", "hosts": [{"url": "tcp://foo.bar.com:443"}], - "dns_lookup_ip_version" : "v6_only" + "dns_lookup_family" : "v6_only" } )EOF"; EXPECT_CALL(initialized, ready()); - EXPECT_CALL(dns_resolver, resolve("foo.bar.com", "v6_only", _)) - .WillOnce(Invoke([&](const std::string&, const std::string&, + EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::v6_only, _)) + .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { cb(TestUtility::makeDnsResponse({"::1", "::2"})); return nullptr; @@ -128,7 +128,7 @@ TEST(StrictDnsClusterImplTest, ImmediateResolveV6Only) { EXPECT_EQ(2UL, cluster.healthyHosts().size()); } -TEST(StrictDnsClusterImplTest, ImmediateResolveAuto) { +TEST(StrictDnsClusterImplTest, ImmediateResolveFallback) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock dns_resolver; @@ -143,13 +143,13 @@ TEST(StrictDnsClusterImplTest, ImmediateResolveAuto) { "type": "strict_dns", "lb_type": "round_robin", "hosts": [{"url": "tcp://foo.bar.com:443"}], - "dns_lookup_ip_version" : "auto" + "dns_lookup_family" : "fallback" } )EOF"; EXPECT_CALL(initialized, ready()); - EXPECT_CALL(dns_resolver, resolve("foo.bar.com", "auto", _)) - .WillOnce(Invoke([&](const std::string&, const std::string&, + EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::fallback, _)) + .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); return nullptr; @@ -549,7 +549,7 @@ TEST(ClusterDefinitionTest, BadDnsIpVersionClusterConfig) { "type": "static", "lb_type": "round_robin", "hosts": [{"url": "tcp://127.0.0.1:11001"}], - "dns_lookup_ip_version" : "foo" + "dns_lookup_family" : "foo" } )EOF"; diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 19b7cb6d589a8..421ebcd184b4a 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -119,7 +119,7 @@ class MockDnsResolver : public DnsResolver { // Network::DnsResolver MOCK_METHOD3(resolve, ActiveDnsQuery*(const std::string& dns_name, - const std::string& dns_lookup_ip_version, ResolveCb callback)); + const DnsLookupFamily& dns_lookup_family, ResolveCb callback)); testing::NiceMock active_query_; }; From 536a20430e95ef3579054a37b31b1144b4ae038f Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Mon, 22 May 2017 15:54:07 -0400 Subject: [PATCH 04/12] fix --- source/common/upstream/upstream_impl.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index dcbf0947dc2dc..0b94c4617af5d 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -379,9 +379,6 @@ StrictDnsClusterImpl::StrictDnsClusterImpl(const Json::Object& config, Runtime:: } else { dns_lookup_family_ = Network::DnsLookupFamily::v4_only; } - //} else { - // throw EnvoyException(fmt::format("unknown dns_lookup_family option {}", dns_lookup_family)); - //} for (Json::ObjectSharedPtr host : config.getObjectArray("hosts")) { resolve_targets_.emplace_back(new ResolveTarget(*this, dispatcher, host->getString("url"))); From c17cd013472d11ddf2f7e31f865b46005c95002b Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Mon, 22 May 2017 16:11:27 -0400 Subject: [PATCH 05/12] Fix --- test/common/network/dns_impl_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index dfaae695e15b8..7e020a04eb02d 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -93,13 +93,13 @@ class TestDnsServerQuery { // name. long name_len; // Get host name from query and use the name to lookup a record - // in a host maap. If the query type is of type A, then perform the lookup in + // in a host map. If the query type is of type A, then perform the lookup in // the hosts_A_ host map. If the query type is of type AAAA, then perform the // lookup in the hosts_AAAA_ host map. char* name; ASSERT_EQ(ARES_SUCCESS, ares_expand_name(question, request, size_, &name, &name_len)); const std::list* ips = nullptr; - // Query type. We only expect for resources of type A or AAAA. + // We only expect resources of type A or AAAA. const int q_type = DNS_QUESTION_TYPE(question + name_len); ASSERT_TRUE(q_type == T_A || q_type == T_AAAA); if (q_type == T_A) { @@ -198,7 +198,7 @@ class TestDnsServer : public ListenerCallbacks { queries_.emplace_back(query); } - void addHosts(const std::string& hostname, const IpList& ip, const record_type type) { + void addHosts(const std::string& hostname, const IpList& ip, const record_type& type) { if (type == A) { hosts_A_[hostname] = ip; } else if (type == AAAA) { @@ -290,7 +290,7 @@ static bool hasAddress(const std::list& results return false; } -// Parameterized test dns server socket address. +// Parameterize the DNS test server socket address. INSTANTIATE_TEST_CASE_P(IpVersions, DnsImplTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest())); From 9ba1839d22d807cbc695c1521a079530d0271e48 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Tue, 23 May 2017 16:23:42 -0400 Subject: [PATCH 06/12] Response to comments --- STYLE.md | 1 + .../configuration/cluster_manager/cluster.rst | 4 +- include/envoy/network/dns.h | 5 +-- source/common/network/dns_impl.cc | 41 ++++++++++------- source/common/network/dns_impl.h | 22 +++++++--- source/common/upstream/logical_dns_cluster.cc | 24 ++++++---- source/common/upstream/upstream_impl.cc | 17 ++++--- test/common/network/dns_impl_test.cc | 44 ++++++++++--------- .../upstream/logical_dns_cluster_test.cc | 33 ++++++++++---- test/common/upstream/upstream_impl_test.cc | 8 ++-- test/mocks/network/mocks.h | 5 +-- 11 files changed, 123 insertions(+), 81 deletions(-) diff --git a/STYLE.md b/STYLE.md index 265ba39f36e70..40c624364be1c 100644 --- a/STYLE.md +++ b/STYLE.md @@ -17,6 +17,7 @@ includes both const and non-const references. * Function names using camel case starting with a lower case letter (e.g., "doFoo()"). * Struct/Class member variables have a '\_' postfix (e.g., "int foo\_;"). +* Enum values using all CAPS (e.g., "EXTERNAL_ONLY"). * 100 columns is the line limit. * Use your GitHub name in TODO comments, e.g. `TODO(foobar): blah`. * Smart pointers are type aliased: diff --git a/docs/configuration/cluster_manager/cluster.rst b/docs/configuration/cluster_manager/cluster.rst index c3176ef1c7067..285b4159955c2 100644 --- a/docs/configuration/cluster_manager/cluster.rst +++ b/docs/configuration/cluster_manager/cluster.rst @@ -149,9 +149,9 @@ dns_refresh_rate_ms dns_lookup_family *(optional, string)* The DNS IP address resolution policy. The options are *v4_only*, *v6_only*, - and *fallback*. If this setting is not specified, the value defaults to *v4_only*. When *v4_only* is selected, + and *auto*. If this setting is not specified, the value defaults to *v4_only*. When *v4_only* is selected, the DNS resolver will only perform a lookup for addresses in the IPv4 family. If *v6_only* is selected, - the DNS resolver will only perform a lookup for addresses in the IPv6 family. If *fallback* is specified, + the DNS resolver will only perform a lookup for addresses in the IPv6 family. If *auto* is specified, the DNS resolver will first perform a lookup for addresses in the IPv6 family and fallback to a lookup for addresses in the IPv4 family. For cluster types other than *strict_dns* and *logical_dns*, this setting is ignored. diff --git a/include/envoy/network/dns.h b/include/envoy/network/dns.h index e900636efc47d..2a6e7b2c0e324 100644 --- a/include/envoy/network/dns.h +++ b/include/envoy/network/dns.h @@ -24,7 +24,7 @@ class ActiveDnsQuery { virtual void cancel() PURE; }; -enum class DnsLookupFamily { v4_only, v6_only, fallback }; +enum class DnsLookupFamily { V4_ONLY, V6_ONLY, AUTO }; /** * An asynchronous DNS resolver. @@ -48,8 +48,7 @@ class DnsResolver { * @return if non-null, a handle that can be used to cancel the resolution. * This is only valid until the invocation of callback or ~DnsResolver(). */ - virtual ActiveDnsQuery* resolve(const std::string& dns_name, - const DnsLookupFamily& dns_lookup_family, + virtual ActiveDnsQuery* resolve(const std::string& dns_name, DnsLookupFamily dns_lookup_family, ResolveCb callback) PURE; }; diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 745033e219863..ca1952a9f2fe0 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -79,15 +79,23 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, hostent* address_list.emplace_back(new Address::Ipv6Instance(address)); } } - } else if (fallback_if_failed) { - fallback_if_failed = false; - getHostByName(AF_INET); } - if (!cancelled_ && completed_) { - callback_(std::move(address_list)); + + if (completed_) { + if (!cancelled_) { + callback_(std::move(address_list)); + } + if (owned_) { + delete this; + return; + } } - if (owned_ && completed_) { - delete this; + + if (status != ARES_SUCCESS && fallback_if_failed) { + fallback_if_failed = false; + getHostByName(AF_INET); + // Note: Nothing can follow this call to getHostByName due to deletion of this + // object upon synchronous resolution. } } @@ -132,17 +140,17 @@ void DnsResolverImpl::onAresSocketStateChange(int fd, int read, int write) { } ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, - const DnsLookupFamily& dns_lookup_family, - ResolveCb callback) { - std::unique_ptr pending_resolution(new PendingResolution()); - pending_resolution->callback_ = callback; - pending_resolution->channel_ = channel_; - pending_resolution->dns_name_ = dns_name; - if (dns_lookup_family == DnsLookupFamily::fallback) { + DnsLookupFamily dns_lookup_family, ResolveCb callback) { + // TODO(hennna): Add DNS caching which will allow testing the edge case of a + // failed intial call to getHostbyName followed by a synchronous IPv4 + // resolution. + std::unique_ptr pending_resolution( + new PendingResolution(callback, channel_, dns_name)); + if (dns_lookup_family == DnsLookupFamily::AUTO) { pending_resolution->fallback_if_failed = true; } - if (dns_lookup_family == DnsLookupFamily::v4_only) { + if (dns_lookup_family == DnsLookupFamily::V4_ONLY) { pending_resolution->getHostByName(AF_INET); } else { pending_resolution->getHostByName(AF_INET6); @@ -162,9 +170,8 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, void DnsResolverImpl::PendingResolution::getHostByName(int family) { ares_gethostbyname(channel_, dns_name_.c_str(), - family, [](void* arg, int status, int timeouts, hostent* hostent) { + family, [](void* arg, int status, int, hostent* hostent) { static_cast(arg)->onAresHostCallback(status, hostent); - UNREFERENCED_PARAMETER(timeouts); }, this); } diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index 1138268f6942b..686adcd6bc75d 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -29,24 +29,34 @@ class DnsResolverImpl : public DnsResolver { ~DnsResolverImpl() override; // Network::DnsResolver - ActiveDnsQuery* resolve(const std::string& dns_name, const DnsLookupFamily& dns_lookup_family, + ActiveDnsQuery* resolve(const std::string& dns_name, DnsLookupFamily dns_lookup_family, ResolveCb callback) override; private: friend class DnsResolverImplPeer; struct PendingResolution : public ActiveDnsQuery { // Network::ActiveDnsQuery + PendingResolution(ResolveCb callback, ares_channel channel, const std::string& dns_name) + : callback_(callback), channel_(channel), dns_name_(dns_name) {} + void cancel() override { // c-ares only supports channel-wide cancellation, so we just allow the // network events to continue but don't invoke the callback on completion. cancelled_ = true; } - // c-ares ares_gethostbyname() query callback. + /* c-ares ares_gethostbyname() query callback. + * @param status return status of call to ares_gethostbyname. + * @param hostent structure that stores information about a given host. + */ void onAresHostCallback(int status, hostent* hostent); + /* wrapper function of call to ares_gethostbyname. + * @param family currently AF_INET and AF_INET6 are supported. + */ + void getHostByName(int family); // Caller supplied callback to invoke on query completion or error. - ResolveCb callback_; + const ResolveCb callback_; // Does the object own itself? Resource reclamation occurs via self-deleting // on query completion or error. bool owned_ = false; @@ -57,10 +67,8 @@ class DnsResolverImpl : public DnsResolver { // If dns_lookup_family is "fallback", fallback to v4 address if v6 // resolution failed. bool fallback_if_failed = false; - ares_channel channel_; - std::string dns_name_; - - void getHostByName(int family); + const ares_channel channel_; + const std::string dns_name_; }; // Callback for events on sockets tracked in events_. diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index 0c5ee0cffcd3b..6f34570a32722 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -25,14 +25,14 @@ LogicalDnsCluster::LogicalDnsCluster(const Json::Object& config, Runtime::Loader throw EnvoyException("logical_dns clusters must have a single host"); } - // Resolve string to enum. std::string dns_lookup_family = config.getString("dns_lookup_family", "v4_only"); if (dns_lookup_family == "v6_only") { - dns_lookup_family_ = Network::DnsLookupFamily::v6_only; - } else if (dns_lookup_family == "fallback") { - dns_lookup_family_ = Network::DnsLookupFamily::fallback; + dns_lookup_family_ = Network::DnsLookupFamily::V6_ONLY; + } else if (dns_lookup_family == "auto") { + dns_lookup_family_ = Network::DnsLookupFamily::AUTO; } else { - dns_lookup_family_ = Network::DnsLookupFamily::v4_only; + ASSERT(dns_lookup_family == "v4_only"); + dns_lookup_family_ = Network::DnsLookupFamily::V4_ONLY; } dns_url_ = hosts_json[0]->getString("url"); hostname_ = Network::Utility::hostFromTcpUrl(dns_url_); @@ -70,14 +70,17 @@ void LogicalDnsCluster::startResolve() { // TODO(mattklein123): Move port handling into the DNS interface. Network::Address::IpVersion version = address_list.front()->ip()->version(); Network::Address::InstanceConstSharedPtr new_address; - if (version == Network::Address::IpVersion::v4) { + switch (version) { + case Network::Address::IpVersion::v4: new_address.reset( new Network::Address::Ipv4Instance(address_list.front()->ip()->addressAsString(), Network::Utility::portFromTcpUrl(dns_url_))); - } else { + break; + case Network::Address::IpVersion::v6: new_address.reset( new Network::Address::Ipv6Instance(address_list.front()->ip()->addressAsString(), Network::Utility::portFromTcpUrl(dns_url_))); + break; } if (!current_resolved_address_ || !(*new_address == *current_resolved_address_)) { current_resolved_address_ = new_address; @@ -93,12 +96,15 @@ void LogicalDnsCluster::startResolve() { // to show the friendly DNS name in that output, but currently there is no way to // express a DNS name inside of an Address::Instance. For now this is OK but we might // want to do better again later. - if (version == Network::Address::IpVersion::v4) { + switch (version) { + case Network::Address::IpVersion::v4: logical_host_.reset( new LogicalHost(info_, hostname_, Network::Utility::getIpv4AnyAddress(), *this)); - } else { + break; + case Network::Address::IpVersion::v6: logical_host_.reset( new LogicalHost(info_, hostname_, Network::Utility::getIpv6AnyAddress(), *this)); + break; } HostVectorSharedPtr new_hosts(new std::vector()); new_hosts->emplace_back(logical_host_); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 0b94c4617af5d..5ee92bdde83ae 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -370,14 +370,14 @@ StrictDnsClusterImpl::StrictDnsClusterImpl(const Json::Object& config, Runtime:: : BaseDynamicClusterImpl(config, runtime, stats, ssl_context_manager), dns_resolver_(dns_resolver), dns_refresh_rate_ms_(std::chrono::milliseconds( config.getInteger("dns_refresh_rate_ms", 5000))) { - // Resolve string to enum. std::string dns_lookup_family = config.getString("dns_lookup_family", "v4_only"); if (dns_lookup_family == "v6_only") { - dns_lookup_family_ = Network::DnsLookupFamily::v6_only; - } else if (dns_lookup_family == "fallback") { - dns_lookup_family_ = Network::DnsLookupFamily::fallback; + dns_lookup_family_ = Network::DnsLookupFamily::V6_ONLY; + } else if (dns_lookup_family == "auto") { + dns_lookup_family_ = Network::DnsLookupFamily::AUTO; } else { - dns_lookup_family_ = Network::DnsLookupFamily::v4_only; + ASSERT(dns_lookup_family == "v4_only"); + dns_lookup_family_ = Network::DnsLookupFamily::V4_ONLY; } for (Json::ObjectSharedPtr host : config.getObjectArray("hosts")) { @@ -434,18 +434,21 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { // TODO(mattklein123): Currently the DNS interface does not consider port. We need to make // a new address that has port in it. We need to both support IPv6 as well as potentially // move port handling into the DNS interface itself, which would work better for SRV. - if (address->ip()->version() == Network::Address::IpVersion::v4) { + switch (address->ip()->version()) { + case Network::Address::IpVersion::v4: new_hosts.emplace_back(new HostImpl( parent_.info_, dns_address_, Network::Address::InstanceConstSharedPtr{ new Network::Address::Ipv4Instance(address->ip()->addressAsString(), port_)}, false, 1, "")); - } else { + break; + case Network::Address::IpVersion::v6: new_hosts.emplace_back(new HostImpl( parent_.info_, dns_address_, Network::Address::InstanceConstSharedPtr{ new Network::Address::Ipv6Instance(address->ip()->addressAsString(), port_)}, false, 1, "")); + break; } } diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 7e020a04eb02d..ec37ecdecf72a 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -300,7 +300,7 @@ INSTANTIATE_TEST_CASE_P(IpVersions, DnsImplTest, // destruction. TEST_P(DnsImplTest, DestructPending) { EXPECT_NE(nullptr, - resolver_->resolve("", DnsLookupFamily::v4_only, + resolver_->resolve("", DnsLookupFamily::V4_ONLY, [&](std::list&& results) -> void { FAIL(); UNREFERENCED_PARAMETER(results); @@ -316,7 +316,7 @@ TEST_P(DnsImplTest, DestructPending) { TEST_P(DnsImplTest, LocalLookup) { std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("", DnsLookupFamily::v4_only, + resolver_->resolve("", DnsLookupFamily::V4_ONLY, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -326,7 +326,7 @@ TEST_P(DnsImplTest, LocalLookup) { EXPECT_TRUE(address_list.empty()); if (GetParam() == Address::IpVersion::v4) { - EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::v4_only, + EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::V4_ONLY, [&](std::list&& results) -> void { address_list = results; })); EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); @@ -334,13 +334,13 @@ TEST_P(DnsImplTest, LocalLookup) { } if (GetParam() == Address::IpVersion::v6) { - EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::fallback, + EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::AUTO, [&](std::list&& results) -> void { address_list = results; })); EXPECT_FALSE(hasAddress(address_list, "127.0.0.1")); EXPECT_TRUE(hasAddress(address_list, "::1")); - EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::v6_only, + EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::V6_ONLY, [&](std::list&& results) -> void { address_list = results; })); EXPECT_TRUE(hasAddress(address_list, "::1")); @@ -352,7 +352,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersionV6) { std::list address_list; server_->addHosts("some.good.domain", {"1::2"}, AAAA); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::fallback, + resolver_->resolve("some.good.domain", DnsLookupFamily::AUTO, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -362,7 +362,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersionV6) { EXPECT_TRUE(hasAddress(address_list, "1::2")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::v4_only, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4_ONLY, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -372,7 +372,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersionV6) { EXPECT_FALSE(hasAddress(address_list, "1::2")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::v6_only, + resolver_->resolve("some.good.domain", DnsLookupFamily::V6_ONLY, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -386,7 +386,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { std::list address_list; server_->addHosts("some.good.domain", {"1.2.3.4"}, A); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::fallback, + resolver_->resolve("some.good.domain", DnsLookupFamily::AUTO, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -396,7 +396,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::v4_only, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4_ONLY, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -406,7 +406,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::v6_only, + resolver_->resolve("some.good.domain", DnsLookupFamily::V6_ONLY, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -422,7 +422,7 @@ TEST_P(DnsImplTest, RemoteAsyncLookup) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.bad.domain", DnsLookupFamily::fallback, + resolver_->resolve("some.bad.domain", DnsLookupFamily::AUTO, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -432,7 +432,7 @@ TEST_P(DnsImplTest, RemoteAsyncLookup) { EXPECT_TRUE(address_list.empty()); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::fallback, + resolver_->resolve("some.good.domain", DnsLookupFamily::AUTO, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -447,7 +447,7 @@ TEST_P(DnsImplTest, MultiARecordLookup) { server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::v4_only, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4_ONLY, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -464,7 +464,7 @@ TEST_P(DnsImplTest, MultiARecordLookupWithV6) { server_->addHosts("some.good.domain", {"1::2", "1::2:3", "1::2:3:4"}, AAAA); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::v4_only, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4_ONLY, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -476,7 +476,7 @@ TEST_P(DnsImplTest, MultiARecordLookupWithV6) { EXPECT_TRUE(hasAddress(address_list, "6.5.4.3")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::fallback, + resolver_->resolve("some.good.domain", DnsLookupFamily::AUTO, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -488,7 +488,7 @@ TEST_P(DnsImplTest, MultiARecordLookupWithV6) { EXPECT_TRUE(hasAddress(address_list, "1::2:3:4")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::v6_only, + resolver_->resolve("some.good.domain", DnsLookupFamily::V6_ONLY, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -505,12 +505,12 @@ TEST_P(DnsImplTest, Cancel) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); ActiveDnsQuery* query = - resolver_->resolve("some.domain", DnsLookupFamily::fallback, + resolver_->resolve("some.domain", DnsLookupFamily::AUTO, [](std::list && ) -> void { FAIL(); }); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::fallback, + resolver_->resolve("some.good.domain", DnsLookupFamily::AUTO, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -528,12 +528,16 @@ class DnsImplZeroTimeoutTest : public DnsImplTest { bool zero_timeout() const override { return true; } }; +// Parameterize the DNS test server socket address. +INSTANTIATE_TEST_CASE_P(IpVersions, DnsImplZeroTimeoutTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest())); + // Validate that timeouts result in an empty callback. TEST_P(DnsImplZeroTimeoutTest, Timeout) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::v4_only, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4_ONLY, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index 04e31b69eb0ac..c92058b9ae3d9 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -73,6 +73,21 @@ TEST_F(LogicalDnsClusterTest, BadConfig) { EXPECT_THROW(setup(json), EnvoyException); } +TEST_F(LogicalDnsClusterTest, BadDnsConfig) { + std::string json = R"EOF( + { + "name": "name", + "connect_timeout_ms": 250, + "type": "logical_dns", + "lb_type": "round_robin", + "hosts": [{"url": "tcp://foo.bar.com:443"}], + "dns_lookup_family": "foo" + } + )EOF"; + + EXPECT_DEATH(setup(json), "dns_lookup_family == \"v4_only\""); +} + // Validate that if the DNS resolves immediately, during the LogicalDnsCluster // constructor, we have the expected host state and initialization callback // invocation. @@ -88,7 +103,7 @@ TEST_F(LogicalDnsClusterTest, ImmediateResolve) { )EOF"; EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::v4_only, _)) + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::V4_ONLY, _)) .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { EXPECT_CALL(*resolve_timer_, enableTimer(_)); @@ -115,7 +130,7 @@ TEST_F(LogicalDnsClusterTest, ImmediateResolveV6Only) { )EOF"; EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::v6_only, _)) + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::V6_ONLY, _)) .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { EXPECT_CALL(*resolve_timer_, enableTimer(_)); @@ -136,13 +151,13 @@ TEST_F(LogicalDnsClusterTest, ImmediateResolveAuto) { "connect_timeout_ms": 250, "type": "logical_dns", "lb_type": "round_robin", - "dns_lookup_family": "fallback", + "dns_lookup_family": "auto", "hosts": [{"url": "tcp://foo.bar.com:443"}] } )EOF"; EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::fallback, _)) + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::AUTO, _)) .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { EXPECT_CALL(*resolve_timer_, enableTimer(_)); @@ -168,7 +183,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { } )EOF"; - expectResolve(Network::DnsLookupFamily::v4_only); + expectResolve(Network::DnsLookupFamily::V4_ONLY); setup(json); EXPECT_CALL(membership_updated_, ready()); @@ -189,7 +204,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { logical_host->createConnection(dispatcher_); logical_host->outlierDetector().putHttpResponseCode(200); - expectResolve(Network::DnsLookupFamily::v4_only); + expectResolve(Network::DnsLookupFamily::V4_ONLY); resolve_timer_->callback_(); // Should not cause any changes. @@ -209,7 +224,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { EXPECT_EQ("foo.bar.com", data.host_description_->hostname()); data.host_description_->outlierDetector().putHttpResponseCode(200); - expectResolve(Network::DnsLookupFamily::v4_only); + expectResolve(Network::DnsLookupFamily::V4_ONLY); resolve_timer_->callback_(); // Should cause a change. @@ -222,7 +237,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { .WillOnce(Return(new NiceMock())); logical_host->createConnection(dispatcher_); - expectResolve(Network::DnsLookupFamily::v4_only); + expectResolve(Network::DnsLookupFamily::V4_ONLY); resolve_timer_->callback_(); // Empty should not cause any change. @@ -237,7 +252,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { // Make sure we cancel. EXPECT_CALL(active_dns_query_, cancel()); - expectResolve(Network::DnsLookupFamily::v4_only); + expectResolve(Network::DnsLookupFamily::V4_ONLY); resolve_timer_->callback_(); tls_.shutdownThread(); diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index f761ccdfefdff..560f6140ee800 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -80,7 +80,7 @@ TEST(StrictDnsClusterImplTest, ImmediateResolve) { )EOF"; EXPECT_CALL(initialized, ready()); - EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::v4_only, _)) + EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::V4_ONLY, _)) .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); @@ -114,7 +114,7 @@ TEST(StrictDnsClusterImplTest, ImmediateResolveV6Only) { )EOF"; EXPECT_CALL(initialized, ready()); - EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::v6_only, _)) + EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::V6_ONLY, _)) .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { cb(TestUtility::makeDnsResponse({"::1", "::2"})); @@ -143,12 +143,12 @@ TEST(StrictDnsClusterImplTest, ImmediateResolveFallback) { "type": "strict_dns", "lb_type": "round_robin", "hosts": [{"url": "tcp://foo.bar.com:443"}], - "dns_lookup_family" : "fallback" + "dns_lookup_family" : "auto" } )EOF"; EXPECT_CALL(initialized, ready()); - EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::fallback, _)) + EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::AUTO, _)) .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 421ebcd184b4a..fe78c71474f26 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -117,9 +117,8 @@ class MockDnsResolver : public DnsResolver { ~MockDnsResolver(); // Network::DnsResolver - MOCK_METHOD3(resolve, - ActiveDnsQuery*(const std::string& dns_name, - const DnsLookupFamily& dns_lookup_family, ResolveCb callback)); + MOCK_METHOD3(resolve, ActiveDnsQuery*(const std::string& dns_name, + DnsLookupFamily dns_lookup_family, ResolveCb callback)); testing::NiceMock active_query_; }; From 5c455917e171360c51ca832706b533a709824719 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Tue, 23 May 2017 17:33:05 -0400 Subject: [PATCH 07/12] Test not needed --- test/common/upstream/logical_dns_cluster_test.cc | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index c92058b9ae3d9..578aba5358e5d 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -73,21 +73,6 @@ TEST_F(LogicalDnsClusterTest, BadConfig) { EXPECT_THROW(setup(json), EnvoyException); } -TEST_F(LogicalDnsClusterTest, BadDnsConfig) { - std::string json = R"EOF( - { - "name": "name", - "connect_timeout_ms": 250, - "type": "logical_dns", - "lb_type": "round_robin", - "hosts": [{"url": "tcp://foo.bar.com:443"}], - "dns_lookup_family": "foo" - } - )EOF"; - - EXPECT_DEATH(setup(json), "dns_lookup_family == \"v4_only\""); -} - // Validate that if the DNS resolves immediately, during the LogicalDnsCluster // constructor, we have the expected host state and initialization callback // invocation. From 5588ebe30cb4212434edcb3d4c455a947b6109ac Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 24 May 2017 14:09:08 -0400 Subject: [PATCH 08/12] Response to comments --- STYLE.md | 2 +- .../configuration/cluster_manager/cluster.rst | 2 +- source/common/network/dns_impl.cc | 9 +- source/common/network/dns_impl.h | 2 +- source/common/network/utility.cc | 11 ++ source/common/network/utility.h | 8 ++ source/common/upstream/logical_dns_cluster.cc | 20 +--- source/common/upstream/upstream_impl.cc | 20 +--- .../upstream/logical_dns_cluster_test.cc | 95 +++++++--------- test/common/upstream/upstream_impl_test.cc | 105 ++++++------------ 10 files changed, 113 insertions(+), 161 deletions(-) diff --git a/STYLE.md b/STYLE.md index 40c624364be1c..012b019dfb922 100644 --- a/STYLE.md +++ b/STYLE.md @@ -17,7 +17,7 @@ includes both const and non-const references. * Function names using camel case starting with a lower case letter (e.g., "doFoo()"). * Struct/Class member variables have a '\_' postfix (e.g., "int foo\_;"). -* Enum values using all CAPS (e.g., "EXTERNAL_ONLY"). +* Enum values using all CAPS (e.g., `EXTERNAL_ONLY`). * 100 columns is the line limit. * Use your GitHub name in TODO comments, e.g. `TODO(foobar): blah`. * Smart pointers are type aliased: diff --git a/docs/configuration/cluster_manager/cluster.rst b/docs/configuration/cluster_manager/cluster.rst index 285b4159955c2..b3b6cc4ab4e75 100644 --- a/docs/configuration/cluster_manager/cluster.rst +++ b/docs/configuration/cluster_manager/cluster.rst @@ -21,7 +21,7 @@ Cluster "http_codec_options": "...", "dns_refresh_rate_ms": "...", "dns_lookup_family": "...", - "outlier_detection": "..." + "outlier_detection": "{...}" } .. _config_cluster_manager_cluster_name: diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index ca1952a9f2fe0..6e75504121cd1 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -52,7 +52,7 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, hostent* delete this; return; } - if (status == ARES_SUCCESS || !fallback_if_failed) { + if (status == ARES_SUCCESS || !fallback_if_failed_) { completed_ = true; } @@ -91,11 +91,12 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, hostent* } } - if (status != ARES_SUCCESS && fallback_if_failed) { - fallback_if_failed = false; + if (status != ARES_SUCCESS && fallback_if_failed_) { + fallback_if_failed_ = false; getHostByName(AF_INET); // Note: Nothing can follow this call to getHostByName due to deletion of this // object upon synchronous resolution. + return; } } @@ -147,7 +148,7 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, std::unique_ptr pending_resolution( new PendingResolution(callback, channel_, dns_name)); if (dns_lookup_family == DnsLookupFamily::AUTO) { - pending_resolution->fallback_if_failed = true; + pending_resolution->fallback_if_failed_ = true; } if (dns_lookup_family == DnsLookupFamily::V4_ONLY) { diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index 686adcd6bc75d..50865c67df6be 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -66,7 +66,7 @@ class DnsResolverImpl : public DnsResolver { bool cancelled_ = false; // If dns_lookup_family is "fallback", fallback to v4 address if v6 // resolution failed. - bool fallback_if_failed = false; + bool fallback_if_failed_ = false; const ares_channel channel_; const std::string dns_name_; }; diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 92356a6610a93..14f590ab1ca4d 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -299,6 +299,17 @@ Address::InstanceConstSharedPtr Utility::getIpv6AnyAddress() { return any; } +Address::InstanceConstSharedPtr Utility::getAddressUpdatePort(const Address::Instance& address, + uint32_t port) { + switch (address.ip()->version()) { + case Network::Address::IpVersion::v4: + return std::make_shared(address.ip()->addressAsString(), port); + case Network::Address::IpVersion::v6: + return std::make_shared(address.ip()->addressAsString(), port); + } + NOT_REACHED; +} + Address::InstanceConstSharedPtr Utility::getOriginalDst(int fd) { sockaddr_storage orig_addr; socklen_t addr_len = sizeof(sockaddr_storage); diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 3f105f16f9c1b..a13ab7fc7969d 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -146,6 +146,14 @@ class Utility { */ static Address::InstanceConstSharedPtr getIpv6AnyAddress(); + /** + * @param address IP address instance. + * @param port to update. + * @return Address::InstanceConstSharedPtr a new address instance with updated port. + */ + static Address::InstanceConstSharedPtr getAddressUpdatePort(const Address::Instance& address, + uint32_t port); + /** * Retrieve the original destination address from an accepted fd. * The address (IP and port) may be not local and the port may differ from diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index 6f34570a32722..0667534489bab 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -68,20 +68,10 @@ void LogicalDnsCluster::startResolve() { if (!address_list.empty()) { // TODO(mattklein123): Move port handling into the DNS interface. - Network::Address::IpVersion version = address_list.front()->ip()->version(); - Network::Address::InstanceConstSharedPtr new_address; - switch (version) { - case Network::Address::IpVersion::v4: - new_address.reset( - new Network::Address::Ipv4Instance(address_list.front()->ip()->addressAsString(), - Network::Utility::portFromTcpUrl(dns_url_))); - break; - case Network::Address::IpVersion::v6: - new_address.reset( - new Network::Address::Ipv6Instance(address_list.front()->ip()->addressAsString(), - Network::Utility::portFromTcpUrl(dns_url_))); - break; - } + ASSERT(address_list.front() != nullptr); + Network::Address::InstanceConstSharedPtr new_address = + Network::Utility::getAddressUpdatePort(*address_list.front(), + Network::Utility::portFromTcpUrl(dns_url_)); if (!current_resolved_address_ || !(*new_address == *current_resolved_address_)) { current_resolved_address_ = new_address; // Capture URL to avoid a race with another update. @@ -96,7 +86,7 @@ void LogicalDnsCluster::startResolve() { // to show the friendly DNS name in that output, but currently there is no way to // express a DNS name inside of an Address::Instance. For now this is OK but we might // want to do better again later. - switch (version) { + switch (address_list.front()->ip()->version()) { case Network::Address::IpVersion::v4: logical_host_.reset( new LogicalHost(info_, hostname_, Network::Utility::getIpv4AnyAddress(), *this)); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 5ee92bdde83ae..441388289899b 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -434,22 +434,10 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { // TODO(mattklein123): Currently the DNS interface does not consider port. We need to make // a new address that has port in it. We need to both support IPv6 as well as potentially // move port handling into the DNS interface itself, which would work better for SRV. - switch (address->ip()->version()) { - case Network::Address::IpVersion::v4: - new_hosts.emplace_back(new HostImpl( - parent_.info_, dns_address_, - Network::Address::InstanceConstSharedPtr{ - new Network::Address::Ipv4Instance(address->ip()->addressAsString(), port_)}, - false, 1, "")); - break; - case Network::Address::IpVersion::v6: - new_hosts.emplace_back(new HostImpl( - parent_.info_, dns_address_, - Network::Address::InstanceConstSharedPtr{ - new Network::Address::Ipv6Instance(address->ip()->addressAsString(), port_)}, - false, 1, "")); - break; - } + ASSERT(address != nullptr); + new_hosts.emplace_back( + new HostImpl(parent_.info_, dns_address_, + Network::Utility::getAddressUpdatePort(*address, port_), false, 1, "")); } std::vector hosts_added; diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index 578aba5358e5d..8914502518323 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -1,6 +1,7 @@ #include #include #include +#include #include #include "common/network/utility.h" @@ -59,67 +60,64 @@ class LogicalDnsClusterTest : public testing::Test { NiceMock dispatcher_; }; -TEST_F(LogicalDnsClusterTest, BadConfig) { - std::string json = R"EOF( +typedef std::tuple> DnsConfigTuple; +std::vector generateParamVector() { + std::vector dns_config; { - "name": "name", - "connect_timeout_ms": 250, - "type": "logical_dns", - "lb_type": "round_robin", - "hosts": [{"url": "tcp://foo.bar.com:443"}, {"url": "tcp://foo2.bar.com:443"}] + std::string family_json(""); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V4_ONLY); + std::list dns_response{"127.0.0.1", "127.0.0.2"}; + dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } - )EOF"; - - EXPECT_THROW(setup(json), EnvoyException); + { + std::string family_json(R"EOF("dns_lookup_family": "v4_only",)EOF"); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V4_ONLY); + std::list dns_response{"127.0.0.1", "127.0.0.2"}; + dns_config.push_back(std::make_tuple(family_json, family, dns_response)); + } + { + std::string family_json(R"EOF("dns_lookup_family": "v6_only",)EOF"); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V6_ONLY); + std::list dns_response{"::1", "::2"}; + dns_config.push_back(std::make_tuple(family_json, family, dns_response)); + } + { + std::string family_json(R"EOF("dns_lookup_family": "auto",)EOF"); + Network::DnsLookupFamily family(Network::DnsLookupFamily::AUTO); + std::list dns_response{"::1"}; + dns_config.push_back(std::make_tuple(family_json, family, dns_response)); + } + return dns_config; } +class LogicalDnsParamTest : public LogicalDnsClusterTest, + public testing::WithParamInterface {}; + +INSTANTIATE_TEST_CASE_P(DnsParam, LogicalDnsParamTest, testing::ValuesIn(generateParamVector())); + // Validate that if the DNS resolves immediately, during the LogicalDnsCluster // constructor, we have the expected host state and initialization callback // invocation. -TEST_F(LogicalDnsClusterTest, ImmediateResolve) { +TEST_P(LogicalDnsParamTest, ImmediateResolve) { std::string json = R"EOF( { "name": "name", "connect_timeout_ms": 250, "type": "logical_dns", "lb_type": "round_robin", - "hosts": [{"url": "tcp://foo.bar.com:443"}] - } )EOF"; - - EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::V4_ONLY, _)) - .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, - Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { - EXPECT_CALL(*resolve_timer_, enableTimer(_)); - cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); - return nullptr; - })); - setup(json); - EXPECT_EQ(1UL, cluster_->hosts().size()); - EXPECT_EQ(1UL, cluster_->healthyHosts().size()); - EXPECT_EQ("foo.bar.com", cluster_->hosts()[0]->hostname()); - tls_.shutdownThread(); -} - -TEST_F(LogicalDnsClusterTest, ImmediateResolveV6Only) { - std::string json = R"EOF( - { - "name": "name", - "connect_timeout_ms": 250, - "type": "logical_dns", - "lb_type": "round_robin", - "dns_lookup_family": "v6_only", + json += std::get<0>(GetParam()); + json += R"EOF( "hosts": [{"url": "tcp://foo.bar.com:443"}] } )EOF"; EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::V6_ONLY, _)) + EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", std::get<1>(GetParam()), _)) .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { EXPECT_CALL(*resolve_timer_, enableTimer(_)); - cb(TestUtility::makeDnsResponse({"::1"})); + cb(TestUtility::makeDnsResponse(std::get<2>(GetParam()))); return nullptr; })); setup(json); @@ -129,31 +127,18 @@ TEST_F(LogicalDnsClusterTest, ImmediateResolveV6Only) { tls_.shutdownThread(); } -TEST_F(LogicalDnsClusterTest, ImmediateResolveAuto) { +TEST_F(LogicalDnsClusterTest, BadConfig) { std::string json = R"EOF( { "name": "name", "connect_timeout_ms": 250, "type": "logical_dns", "lb_type": "round_robin", - "dns_lookup_family": "auto", - "hosts": [{"url": "tcp://foo.bar.com:443"}] + "hosts": [{"url": "tcp://foo.bar.com:443"}, {"url": "tcp://foo2.bar.com:443"}] } )EOF"; - EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", Network::DnsLookupFamily::AUTO, _)) - .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, - Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { - EXPECT_CALL(*resolve_timer_, enableTimer(_)); - cb(TestUtility::makeDnsResponse({"::1"})); - return nullptr; - })); - setup(json); - EXPECT_EQ(1UL, cluster_->hosts().size()); - EXPECT_EQ(1UL, cluster_->healthyHosts().size()); - EXPECT_EQ("foo.bar.com", cluster_->hosts()[0]->hostname()); - tls_.shutdownThread(); + EXPECT_THROW(setup(json), EnvoyException); } TEST_F(LogicalDnsClusterTest, Basic) { diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 560f6140ee800..08dc8f4e8d138 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -2,6 +2,7 @@ #include #include #include +#include #include #include "envoy/api/api.h" @@ -61,74 +62,41 @@ struct ResolverData { Network::MockActiveDnsQuery active_dns_query_; }; -TEST(StrictDnsClusterImplTest, ImmediateResolve) { - Stats::IsolatedStoreImpl stats; - Ssl::MockContextManager ssl_context_manager; - NiceMock dns_resolver; - NiceMock dispatcher; - NiceMock runtime; - ReadyWatcher initialized; - - std::string json = R"EOF( +typedef std::tuple> DnsConfigTuple; +std::vector generateParamVector() { + std::vector dns_config; { - "name": "name", - "connect_timeout_ms": 250, - "type": "strict_dns", - "lb_type": "round_robin", - "hosts": [{"url": "tcp://foo.bar.com:443"}] + std::string family_json(""); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V4_ONLY); + std::list dns_response{"127.0.0.1", "127.0.0.2"}; + dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } - )EOF"; - - EXPECT_CALL(initialized, ready()); - EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::V4_ONLY, _)) - .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, - Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { - cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); - return nullptr; - })); - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); - StrictDnsClusterImpl cluster(*loader, runtime, stats, ssl_context_manager, dns_resolver, - dispatcher); - cluster.setInitializedCb([&]() -> void { initialized.ready(); }); - EXPECT_EQ(2UL, cluster.hosts().size()); - EXPECT_EQ(2UL, cluster.healthyHosts().size()); -} - -TEST(StrictDnsClusterImplTest, ImmediateResolveV6Only) { - Stats::IsolatedStoreImpl stats; - Ssl::MockContextManager ssl_context_manager; - NiceMock dns_resolver; - NiceMock dispatcher; - NiceMock runtime; - ReadyWatcher initialized; - - std::string json = R"EOF( { - "name": "name", - "connect_timeout_ms": 250, - "type": "strict_dns", - "lb_type": "round_robin", - "hosts": [{"url": "tcp://foo.bar.com:443"}], - "dns_lookup_family" : "v6_only" + std::string family_json(R"EOF("dns_lookup_family": "v4_only",)EOF"); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V4_ONLY); + std::list dns_response{"127.0.0.1", "127.0.0.2"}; + dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } - )EOF"; - - EXPECT_CALL(initialized, ready()); - EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::V6_ONLY, _)) - .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, - Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { - cb(TestUtility::makeDnsResponse({"::1", "::2"})); - return nullptr; - })); - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); - StrictDnsClusterImpl cluster(*loader, runtime, stats, ssl_context_manager, dns_resolver, - dispatcher); - cluster.setInitializedCb([&]() -> void { initialized.ready(); }); - EXPECT_EQ(2UL, cluster.hosts().size()); - EXPECT_EQ(2UL, cluster.healthyHosts().size()); + { + std::string family_json(R"EOF("dns_lookup_family": "v6_only",)EOF"); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V6_ONLY); + std::list dns_response{"::1", "::2"}; + dns_config.push_back(std::make_tuple(family_json, family, dns_response)); + } + { + std::string family_json(R"EOF("dns_lookup_family": "auto",)EOF"); + Network::DnsLookupFamily family(Network::DnsLookupFamily::AUTO); + std::list dns_response{"127.0.0.1", "127.0.0.2"}; + dns_config.push_back(std::make_tuple(family_json, family, dns_response)); + } + return dns_config; } -TEST(StrictDnsClusterImplTest, ImmediateResolveFallback) { +class StrictDnsParamTest : public testing::TestWithParam {}; + +INSTANTIATE_TEST_CASE_P(DnsParam, StrictDnsParamTest, testing::ValuesIn(generateParamVector())); + +TEST_P(StrictDnsParamTest, ImmediateResolve) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock dns_resolver; @@ -141,17 +109,18 @@ TEST(StrictDnsClusterImplTest, ImmediateResolveFallback) { "name": "name", "connect_timeout_ms": 250, "type": "strict_dns", + )EOF"; + json += std::get<0>(GetParam()); + json += R"EOF( "lb_type": "round_robin", - "hosts": [{"url": "tcp://foo.bar.com:443"}], - "dns_lookup_family" : "auto" + "hosts": [{"url": "tcp://foo.bar.com:443"}] } )EOF"; - EXPECT_CALL(initialized, ready()); - EXPECT_CALL(dns_resolver, resolve("foo.bar.com", Network::DnsLookupFamily::AUTO, _)) + EXPECT_CALL(dns_resolver, resolve("foo.bar.com", std::get<1>(GetParam()), _)) .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { - cb(TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"})); + cb(TestUtility::makeDnsResponse(std::get<2>(GetParam()))); return nullptr; })); Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); @@ -541,7 +510,7 @@ TEST(ClusterDefinitionTest, BadClusterConfig) { EXPECT_THROW(loader->validateSchema(Json::Schema::CLUSTER_SCHEMA), Json::Exception); } -TEST(ClusterDefinitionTest, BadDnsIpVersionClusterConfig) { +TEST(ClusterDefinitionTest, BadDnsClusterConfig) { std::string json = R"EOF( { "name": "cluster_1", From 759c5e4394a2692d7d50a0eef6fe6cb836592f56 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 24 May 2017 14:26:12 -0400 Subject: [PATCH 09/12] Fix --- source/common/network/utility.cc | 4 ++-- source/common/network/utility.h | 4 ++-- source/common/upstream/logical_dns_cluster.cc | 4 ++-- source/common/upstream/upstream_impl.cc | 6 +++--- test/common/upstream/logical_dns_cluster_test.cc | 6 +++--- test/common/upstream/upstream_impl_test.cc | 4 ++-- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 14f590ab1ca4d..38ccf07f95770 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -299,8 +299,8 @@ Address::InstanceConstSharedPtr Utility::getIpv6AnyAddress() { return any; } -Address::InstanceConstSharedPtr Utility::getAddressUpdatePort(const Address::Instance& address, - uint32_t port) { +Address::InstanceConstSharedPtr Utility::getAddressWithPort(const Address::Instance& address, + uint32_t port) { switch (address.ip()->version()) { case Network::Address::IpVersion::v4: return std::make_shared(address.ip()->addressAsString(), port); diff --git a/source/common/network/utility.h b/source/common/network/utility.h index a13ab7fc7969d..244b6b47d7fd6 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -151,8 +151,8 @@ class Utility { * @param port to update. * @return Address::InstanceConstSharedPtr a new address instance with updated port. */ - static Address::InstanceConstSharedPtr getAddressUpdatePort(const Address::Instance& address, - uint32_t port); + static Address::InstanceConstSharedPtr getAddressWithPort(const Address::Instance& address, + uint32_t port); /** * Retrieve the original destination address from an accepted fd. diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index 0667534489bab..73be74602d641 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -70,8 +70,8 @@ void LogicalDnsCluster::startResolve() { // TODO(mattklein123): Move port handling into the DNS interface. ASSERT(address_list.front() != nullptr); Network::Address::InstanceConstSharedPtr new_address = - Network::Utility::getAddressUpdatePort(*address_list.front(), - Network::Utility::portFromTcpUrl(dns_url_)); + Network::Utility::getAddressWithPort(*address_list.front(), + Network::Utility::portFromTcpUrl(dns_url_)); if (!current_resolved_address_ || !(*new_address == *current_resolved_address_)) { current_resolved_address_ = new_address; // Capture URL to avoid a race with another update. diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 441388289899b..0a2fc0deefa7b 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -435,9 +435,9 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { // a new address that has port in it. We need to both support IPv6 as well as potentially // move port handling into the DNS interface itself, which would work better for SRV. ASSERT(address != nullptr); - new_hosts.emplace_back( - new HostImpl(parent_.info_, dns_address_, - Network::Utility::getAddressUpdatePort(*address, port_), false, 1, "")); + new_hosts.emplace_back(new HostImpl(parent_.info_, dns_address_, + Network::Utility::getAddressWithPort(*address, port_), + false, 1, "")); } std::vector hosts_added; diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index 8914502518323..24983bc0b6579 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -37,9 +37,9 @@ class LogicalDnsClusterTest : public testing::Test { cluster_->setInitializedCb([&]() -> void { initialized_.ready(); }); } - void expectResolve(const Network::DnsLookupFamily& dns_lookup_family) { + void expectResolve(Network::DnsLookupFamily dns_lookup_family) { EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", dns_lookup_family, _)) - .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, + .WillOnce(Invoke([&](const std::string&, Network::DnsLookupFamily, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { dns_callback_ = cb; return &active_dns_query_; @@ -114,7 +114,7 @@ TEST_P(LogicalDnsParamTest, ImmediateResolve) { EXPECT_CALL(initialized_, ready()); EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", std::get<1>(GetParam()), _)) - .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, + .WillOnce(Invoke([&](const std::string&, Network::DnsLookupFamily, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { EXPECT_CALL(*resolve_timer_, enableTimer(_)); cb(TestUtility::makeDnsResponse(std::get<2>(GetParam()))); diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 08dc8f4e8d138..acae116d8e82c 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -49,7 +49,7 @@ struct ResolverData { void expectResolve(Network::MockDnsResolver& dns_resolver) { EXPECT_CALL(dns_resolver, resolve(_, _, _)) - .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, + .WillOnce(Invoke([&](const std::string&, Network::DnsLookupFamily, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { dns_callback_ = cb; return &active_dns_query_; @@ -118,7 +118,7 @@ TEST_P(StrictDnsParamTest, ImmediateResolve) { )EOF"; EXPECT_CALL(initialized, ready()); EXPECT_CALL(dns_resolver, resolve("foo.bar.com", std::get<1>(GetParam()), _)) - .WillOnce(Invoke([&](const std::string&, const Network::DnsLookupFamily&, + .WillOnce(Invoke([&](const std::string&, Network::DnsLookupFamily, Network::DnsResolver::ResolveCb cb) -> Network::ActiveDnsQuery* { cb(TestUtility::makeDnsResponse(std::get<2>(GetParam()))); return nullptr; From 2b24ec31c57e4865a975389520d93cab4bf2a4b2 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 24 May 2017 15:05:13 -0400 Subject: [PATCH 10/12] Fix coverage test failure --- test/common/upstream/logical_dns_cluster_test.cc | 12 +++++++----- test/common/upstream/upstream_impl_test.cc | 11 ++++++----- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index 24983bc0b6579..1d53ad0c2406c 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -60,9 +60,10 @@ class LogicalDnsClusterTest : public testing::Test { NiceMock dispatcher_; }; -typedef std::tuple> DnsConfigTuple; -std::vector generateParamVector() { - std::vector dns_config; +typedef std::tuple> + LogicalDnsConfigTuple; +std::vector generateLogicalDnsParams() { + std::vector dns_config; { std::string family_json(""); Network::DnsLookupFamily family(Network::DnsLookupFamily::V4_ONLY); @@ -91,9 +92,10 @@ std::vector generateParamVector() { } class LogicalDnsParamTest : public LogicalDnsClusterTest, - public testing::WithParamInterface {}; + public testing::WithParamInterface {}; -INSTANTIATE_TEST_CASE_P(DnsParam, LogicalDnsParamTest, testing::ValuesIn(generateParamVector())); +INSTANTIATE_TEST_CASE_P(DnsParam, LogicalDnsParamTest, + testing::ValuesIn(generateLogicalDnsParams())); // Validate that if the DNS resolves immediately, during the LogicalDnsCluster // constructor, we have the expected host state and initialization callback diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index acae116d8e82c..6c2d4db597fe8 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -62,9 +62,10 @@ struct ResolverData { Network::MockActiveDnsQuery active_dns_query_; }; -typedef std::tuple> DnsConfigTuple; -std::vector generateParamVector() { - std::vector dns_config; +typedef std::tuple> + StrictDnsConfigTuple; +std::vector generateStrictDnsParams() { + std::vector dns_config; { std::string family_json(""); Network::DnsLookupFamily family(Network::DnsLookupFamily::V4_ONLY); @@ -92,9 +93,9 @@ std::vector generateParamVector() { return dns_config; } -class StrictDnsParamTest : public testing::TestWithParam {}; +class StrictDnsParamTest : public testing::TestWithParam {}; -INSTANTIATE_TEST_CASE_P(DnsParam, StrictDnsParamTest, testing::ValuesIn(generateParamVector())); +INSTANTIATE_TEST_CASE_P(DnsParam, StrictDnsParamTest, testing::ValuesIn(generateStrictDnsParams())); TEST_P(StrictDnsParamTest, ImmediateResolve) { Stats::IsolatedStoreImpl stats; From 554fb2e60ac448952bf8c318d6c9ef4ff484d689 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 24 May 2017 15:27:52 -0400 Subject: [PATCH 11/12] Fix enum style --- STYLE.md | 2 +- include/envoy/network/dns.h | 2 +- source/common/network/dns_impl.cc | 4 +- source/common/upstream/logical_dns_cluster.cc | 6 +-- source/common/upstream/upstream_impl.cc | 6 +-- test/common/network/dns_impl_test.cc | 40 +++++++++---------- .../upstream/logical_dns_cluster_test.cc | 18 ++++----- test/common/upstream/upstream_impl_test.cc | 8 ++-- 8 files changed, 43 insertions(+), 43 deletions(-) diff --git a/STYLE.md b/STYLE.md index 012b019dfb922..fdb344e160152 100644 --- a/STYLE.md +++ b/STYLE.md @@ -17,7 +17,7 @@ includes both const and non-const references. * Function names using camel case starting with a lower case letter (e.g., "doFoo()"). * Struct/Class member variables have a '\_' postfix (e.g., "int foo\_;"). -* Enum values using all CAPS (e.g., `EXTERNAL_ONLY`). +* Enum values using PascalCase (e.g., `RoundRobin`). * 100 columns is the line limit. * Use your GitHub name in TODO comments, e.g. `TODO(foobar): blah`. * Smart pointers are type aliased: diff --git a/include/envoy/network/dns.h b/include/envoy/network/dns.h index 2a6e7b2c0e324..477434940b030 100644 --- a/include/envoy/network/dns.h +++ b/include/envoy/network/dns.h @@ -24,7 +24,7 @@ class ActiveDnsQuery { virtual void cancel() PURE; }; -enum class DnsLookupFamily { V4_ONLY, V6_ONLY, AUTO }; +enum class DnsLookupFamily { V4Only, V6Only, Auto }; /** * An asynchronous DNS resolver. diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 6e75504121cd1..37a6db10ae7d3 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -147,11 +147,11 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, // resolution. std::unique_ptr pending_resolution( new PendingResolution(callback, channel_, dns_name)); - if (dns_lookup_family == DnsLookupFamily::AUTO) { + if (dns_lookup_family == DnsLookupFamily::Auto) { pending_resolution->fallback_if_failed_ = true; } - if (dns_lookup_family == DnsLookupFamily::V4_ONLY) { + if (dns_lookup_family == DnsLookupFamily::V4Only) { pending_resolution->getHostByName(AF_INET); } else { pending_resolution->getHostByName(AF_INET6); diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index 73be74602d641..b3f570f06e855 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -27,12 +27,12 @@ LogicalDnsCluster::LogicalDnsCluster(const Json::Object& config, Runtime::Loader std::string dns_lookup_family = config.getString("dns_lookup_family", "v4_only"); if (dns_lookup_family == "v6_only") { - dns_lookup_family_ = Network::DnsLookupFamily::V6_ONLY; + dns_lookup_family_ = Network::DnsLookupFamily::V6Only; } else if (dns_lookup_family == "auto") { - dns_lookup_family_ = Network::DnsLookupFamily::AUTO; + dns_lookup_family_ = Network::DnsLookupFamily::Auto; } else { ASSERT(dns_lookup_family == "v4_only"); - dns_lookup_family_ = Network::DnsLookupFamily::V4_ONLY; + dns_lookup_family_ = Network::DnsLookupFamily::V4Only; } dns_url_ = hosts_json[0]->getString("url"); hostname_ = Network::Utility::hostFromTcpUrl(dns_url_); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 0a2fc0deefa7b..d22e8c7576514 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -372,12 +372,12 @@ StrictDnsClusterImpl::StrictDnsClusterImpl(const Json::Object& config, Runtime:: config.getInteger("dns_refresh_rate_ms", 5000))) { std::string dns_lookup_family = config.getString("dns_lookup_family", "v4_only"); if (dns_lookup_family == "v6_only") { - dns_lookup_family_ = Network::DnsLookupFamily::V6_ONLY; + dns_lookup_family_ = Network::DnsLookupFamily::V6Only; } else if (dns_lookup_family == "auto") { - dns_lookup_family_ = Network::DnsLookupFamily::AUTO; + dns_lookup_family_ = Network::DnsLookupFamily::Auto; } else { ASSERT(dns_lookup_family == "v4_only"); - dns_lookup_family_ = Network::DnsLookupFamily::V4_ONLY; + dns_lookup_family_ = Network::DnsLookupFamily::V4Only; } for (Json::ObjectSharedPtr host : config.getObjectArray("hosts")) { diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index ec37ecdecf72a..ba5c12646b4a2 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -300,7 +300,7 @@ INSTANTIATE_TEST_CASE_P(IpVersions, DnsImplTest, // destruction. TEST_P(DnsImplTest, DestructPending) { EXPECT_NE(nullptr, - resolver_->resolve("", DnsLookupFamily::V4_ONLY, + resolver_->resolve("", DnsLookupFamily::V4Only, [&](std::list&& results) -> void { FAIL(); UNREFERENCED_PARAMETER(results); @@ -316,7 +316,7 @@ TEST_P(DnsImplTest, DestructPending) { TEST_P(DnsImplTest, LocalLookup) { std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("", DnsLookupFamily::V4_ONLY, + resolver_->resolve("", DnsLookupFamily::V4Only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -326,7 +326,7 @@ TEST_P(DnsImplTest, LocalLookup) { EXPECT_TRUE(address_list.empty()); if (GetParam() == Address::IpVersion::v4) { - EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::V4_ONLY, + EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::V4Only, [&](std::list&& results) -> void { address_list = results; })); EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); @@ -334,13 +334,13 @@ TEST_P(DnsImplTest, LocalLookup) { } if (GetParam() == Address::IpVersion::v6) { - EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::AUTO, + EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::Auto, [&](std::list&& results) -> void { address_list = results; })); EXPECT_FALSE(hasAddress(address_list, "127.0.0.1")); EXPECT_TRUE(hasAddress(address_list, "::1")); - EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::V6_ONLY, + EXPECT_EQ(nullptr, resolver_->resolve("localhost", DnsLookupFamily::V6Only, [&](std::list&& results) -> void { address_list = results; })); EXPECT_TRUE(hasAddress(address_list, "::1")); @@ -352,7 +352,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersionV6) { std::list address_list; server_->addHosts("some.good.domain", {"1::2"}, AAAA); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::AUTO, + resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -362,7 +362,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersionV6) { EXPECT_TRUE(hasAddress(address_list, "1::2")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V4_ONLY, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -372,7 +372,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersionV6) { EXPECT_FALSE(hasAddress(address_list, "1::2")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V6_ONLY, + resolver_->resolve("some.good.domain", DnsLookupFamily::V6Only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -386,7 +386,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { std::list address_list; server_->addHosts("some.good.domain", {"1.2.3.4"}, A); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::AUTO, + resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -396,7 +396,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V4_ONLY, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -406,7 +406,7 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V6_ONLY, + resolver_->resolve("some.good.domain", DnsLookupFamily::V6Only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -422,7 +422,7 @@ TEST_P(DnsImplTest, RemoteAsyncLookup) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.bad.domain", DnsLookupFamily::AUTO, + resolver_->resolve("some.bad.domain", DnsLookupFamily::Auto, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -432,7 +432,7 @@ TEST_P(DnsImplTest, RemoteAsyncLookup) { EXPECT_TRUE(address_list.empty()); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::AUTO, + resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -447,7 +447,7 @@ TEST_P(DnsImplTest, MultiARecordLookup) { server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V4_ONLY, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -464,7 +464,7 @@ TEST_P(DnsImplTest, MultiARecordLookupWithV6) { server_->addHosts("some.good.domain", {"1::2", "1::2:3", "1::2:3:4"}, AAAA); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V4_ONLY, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -476,7 +476,7 @@ TEST_P(DnsImplTest, MultiARecordLookupWithV6) { EXPECT_TRUE(hasAddress(address_list, "6.5.4.3")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::AUTO, + resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -488,7 +488,7 @@ TEST_P(DnsImplTest, MultiARecordLookupWithV6) { EXPECT_TRUE(hasAddress(address_list, "1::2:3:4")); EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V6_ONLY, + resolver_->resolve("some.good.domain", DnsLookupFamily::V6Only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -505,12 +505,12 @@ TEST_P(DnsImplTest, Cancel) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); ActiveDnsQuery* query = - resolver_->resolve("some.domain", DnsLookupFamily::AUTO, + resolver_->resolve("some.domain", DnsLookupFamily::Auto, [](std::list && ) -> void { FAIL(); }); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::AUTO, + resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); @@ -537,7 +537,7 @@ TEST_P(DnsImplZeroTimeoutTest, Timeout) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V4_ONLY, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, [&](std::list&& results) -> void { address_list = results; dispatcher_.exit(); diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index 1d53ad0c2406c..8d7f2798f80ad 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -66,25 +66,25 @@ std::vector generateLogicalDnsParams() { std::vector dns_config; { std::string family_json(""); - Network::DnsLookupFamily family(Network::DnsLookupFamily::V4_ONLY); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V4Only); std::list dns_response{"127.0.0.1", "127.0.0.2"}; dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } { std::string family_json(R"EOF("dns_lookup_family": "v4_only",)EOF"); - Network::DnsLookupFamily family(Network::DnsLookupFamily::V4_ONLY); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V4Only); std::list dns_response{"127.0.0.1", "127.0.0.2"}; dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } { std::string family_json(R"EOF("dns_lookup_family": "v6_only",)EOF"); - Network::DnsLookupFamily family(Network::DnsLookupFamily::V6_ONLY); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V6Only); std::list dns_response{"::1", "::2"}; dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } { std::string family_json(R"EOF("dns_lookup_family": "auto",)EOF"); - Network::DnsLookupFamily family(Network::DnsLookupFamily::AUTO); + Network::DnsLookupFamily family(Network::DnsLookupFamily::Auto); std::list dns_response{"::1"}; dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } @@ -155,7 +155,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { } )EOF"; - expectResolve(Network::DnsLookupFamily::V4_ONLY); + expectResolve(Network::DnsLookupFamily::V4Only); setup(json); EXPECT_CALL(membership_updated_, ready()); @@ -176,7 +176,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { logical_host->createConnection(dispatcher_); logical_host->outlierDetector().putHttpResponseCode(200); - expectResolve(Network::DnsLookupFamily::V4_ONLY); + expectResolve(Network::DnsLookupFamily::V4Only); resolve_timer_->callback_(); // Should not cause any changes. @@ -196,7 +196,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { EXPECT_EQ("foo.bar.com", data.host_description_->hostname()); data.host_description_->outlierDetector().putHttpResponseCode(200); - expectResolve(Network::DnsLookupFamily::V4_ONLY); + expectResolve(Network::DnsLookupFamily::V4Only); resolve_timer_->callback_(); // Should cause a change. @@ -209,7 +209,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { .WillOnce(Return(new NiceMock())); logical_host->createConnection(dispatcher_); - expectResolve(Network::DnsLookupFamily::V4_ONLY); + expectResolve(Network::DnsLookupFamily::V4Only); resolve_timer_->callback_(); // Empty should not cause any change. @@ -224,7 +224,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { // Make sure we cancel. EXPECT_CALL(active_dns_query_, cancel()); - expectResolve(Network::DnsLookupFamily::V4_ONLY); + expectResolve(Network::DnsLookupFamily::V4Only); resolve_timer_->callback_(); tls_.shutdownThread(); diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 6c2d4db597fe8..0e5e42afab35c 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -68,25 +68,25 @@ std::vector generateStrictDnsParams() { std::vector dns_config; { std::string family_json(""); - Network::DnsLookupFamily family(Network::DnsLookupFamily::V4_ONLY); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V4Only); std::list dns_response{"127.0.0.1", "127.0.0.2"}; dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } { std::string family_json(R"EOF("dns_lookup_family": "v4_only",)EOF"); - Network::DnsLookupFamily family(Network::DnsLookupFamily::V4_ONLY); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V4Only); std::list dns_response{"127.0.0.1", "127.0.0.2"}; dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } { std::string family_json(R"EOF("dns_lookup_family": "v6_only",)EOF"); - Network::DnsLookupFamily family(Network::DnsLookupFamily::V6_ONLY); + Network::DnsLookupFamily family(Network::DnsLookupFamily::V6Only); std::list dns_response{"::1", "::2"}; dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } { std::string family_json(R"EOF("dns_lookup_family": "auto",)EOF"); - Network::DnsLookupFamily family(Network::DnsLookupFamily::AUTO); + Network::DnsLookupFamily family(Network::DnsLookupFamily::Auto); std::list dns_response{"127.0.0.1", "127.0.0.2"}; dns_config.push_back(std::make_tuple(family_json, family, dns_response)); } From 405fd798e542cb822cf8f36ea92f86c2c74a4129 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 24 May 2017 17:54:17 -0400 Subject: [PATCH 12/12] Fix --- source/common/network/dns_impl.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index 50865c67df6be..7ff4d8c1f7d44 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -45,12 +45,14 @@ class DnsResolverImpl : public DnsResolver { cancelled_ = true; } - /* c-ares ares_gethostbyname() query callback. + /** + * c-ares ares_gethostbyname() query callback. * @param status return status of call to ares_gethostbyname. * @param hostent structure that stores information about a given host. */ void onAresHostCallback(int status, hostent* hostent); - /* wrapper function of call to ares_gethostbyname. + /** + * wrapper function of call to ares_gethostbyname. * @param family currently AF_INET and AF_INET6 are supported. */ void getHostByName(int family);