From 13f64545c6e0640aeb7ccbafb3ddd8ce8265fd65 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 31 Jan 2020 14:01:27 -0800 Subject: [PATCH 1/8] wip Signed-off-by: Jose Nino --- source/common/network/dns_impl.cc | 27 +++++++++++++++++++++++---- source/common/network/dns_impl.h | 7 +++++-- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index c134872b71e74..98437ae519caa 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -24,11 +24,11 @@ DnsResolverImpl::DnsResolverImpl( const std::vector& resolvers, const bool use_tcp_for_dns_lookups) : dispatcher_(dispatcher), - timer_(dispatcher.createTimer([this] { onEventCallback(ARES_SOCKET_BAD, 0); })) { + timer_(dispatcher.createTimer([this] { onEventCallback(ARES_SOCKET_BAD, 0); })), use_tcp_for_dns_lookups_(use_tcp_for_dns_lookups) { ares_options options{}; int optmask = 0; - if (use_tcp_for_dns_lookups) { + if (use_tcp_for_dns_lookups_) { optmask |= ARES_OPT_FLAGS; options.flags |= ARES_FLAG_USEVC; } @@ -70,11 +70,15 @@ void DnsResolverImpl::initializeChannel(ares_options* options, int optmask) { static_cast(arg)->onAresSocketStateChange(fd, read, write); }; options->sock_state_cb_data = this; - ares_init_options(&channel_, options, optmask | ARES_OPT_SOCK_STATE_CB); + int status = ares_init_options(&channel_, options, optmask | ARES_OPT_SOCK_STATE_CB); + ENVOY_LOG(error, "ARES CHANNEL INIT {}", status); } void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, int timeouts, ares_addrinfo* addrinfo) { + if (status == ARES_ECONNREFUSED) { + parent_.dirty_channel_ = true; + } // We receive ARES_EDESTRUCTION when destructing with pending queries. if (status == ARES_EDESTRUCTION) { ASSERT(owned_); @@ -171,6 +175,7 @@ void DnsResolverImpl::updateAresTimer() { } void DnsResolverImpl::onEventCallback(int fd, uint32_t events) { + ENVOY_LOG(error, "onEventCallback"); const ares_socket_t read_fd = events & Event::FileReadyType::Read ? fd : ARES_SOCKET_BAD; const ares_socket_t write_fd = events & Event::FileReadyType::Write ? fd : ARES_SOCKET_BAD; ares_process_fd(channel_, read_fd, write_fd); @@ -203,8 +208,22 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, // TODO(hennna): Add DNS caching which will allow testing the edge case of a // failed initial call to getHostByName followed by a synchronous IPv4 // resolution. + if (dirty_channel_) { + ares_destroy(channel_); + + ares_options options{}; + int optmask = 0; + + if (use_tcp_for_dns_lookups_) { + optmask |= ARES_OPT_FLAGS; + options.flags |= ARES_FLAG_USEVC; + } + + initializeChannel(&options, optmask); + + } std::unique_ptr pending_resolution( - new PendingResolution(callback, dispatcher_, channel_, dns_name)); + new PendingResolution(*this, callback, dispatcher_, channel_, dns_name)); if (dns_lookup_family == DnsLookupFamily::Auto) { pending_resolution->fallback_if_failed_ = true; } diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index cecad8a2e7280..1f28ab7aa9e55 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -39,9 +39,9 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable events_; }; From f4ed3cdf4ae38922b794dbc727689104f5d26dc2 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 31 Jan 2020 14:55:23 -0800 Subject: [PATCH 2/8] comments Signed-off-by: Jose Nino --- source/common/network/dns_impl.cc | 30 +++++++++++++++++++----------- source/common/network/dns_impl.h | 7 ++++--- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 98437ae519caa..7902fc0d62a05 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -24,7 +24,8 @@ DnsResolverImpl::DnsResolverImpl( const std::vector& resolvers, const bool use_tcp_for_dns_lookups) : dispatcher_(dispatcher), - timer_(dispatcher.createTimer([this] { onEventCallback(ARES_SOCKET_BAD, 0); })), use_tcp_for_dns_lookups_(use_tcp_for_dns_lookups) { + timer_(dispatcher.createTimer([this] { onEventCallback(ARES_SOCKET_BAD, 0); })), + use_tcp_for_dns_lookups_(use_tcp_for_dns_lookups) { ares_options options{}; int optmask = 0; @@ -76,8 +77,14 @@ void DnsResolverImpl::initializeChannel(ares_options* options, int optmask) { void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, int timeouts, ares_addrinfo* addrinfo) { - if (status == ARES_ECONNREFUSED) { + // If c-ares returns ARES_ECONNREFUSED and there is no fallback we assume that the channel_ is + // broken. Mark the channel dirty so that it is destroyed and reinitialized on a subsequest call + // to DnsResolver::resolve(). The optimal solution would be for c-ares to reinitialize the + // channel, and not have Envoy track side effects. + // context: https://github.com/envoyproxy/envoy/issues/4543. + if (status == ARES_ECONNREFUSED && !fallback_if_failed_) { parent_.dirty_channel_ = true; + return; } // We receive ARES_EDESTRUCTION when destructing with pending queries. if (status == ARES_EDESTRUCTION) { @@ -208,19 +215,20 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, // TODO(hennna): Add DNS caching which will allow testing the edge case of a // failed initial call to getHostByName followed by a synchronous IPv4 // resolution. - if (dirty_channel_) { - ares_destroy(channel_); - ares_options options{}; - int optmask = 0; + // @see DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback for why this is done. + if (dirty_channel_) { + ares_destroy(channel_); - if (use_tcp_for_dns_lookups_) { - optmask |= ARES_OPT_FLAGS; - options.flags |= ARES_FLAG_USEVC; - } + ares_options options{}; + int optmask = 0; - initializeChannel(&options, optmask); + if (use_tcp_for_dns_lookups_) { + optmask |= ARES_OPT_FLAGS; + options.flags |= ARES_FLAG_USEVC; + } + initializeChannel(&options, optmask); } std::unique_ptr pending_resolution( new PendingResolution(*this, callback, dispatcher_, channel_, dns_name)); diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index 1f28ab7aa9e55..55b793084833d 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -39,9 +39,10 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable Date: Fri, 31 Jan 2020 15:25:07 -0800 Subject: [PATCH 3/8] comment Signed-off-by: Jose Nino --- source/common/network/dns_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 7902fc0d62a05..e135114aa7ab2 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -81,7 +81,8 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i // broken. Mark the channel dirty so that it is destroyed and reinitialized on a subsequest call // to DnsResolver::resolve(). The optimal solution would be for c-ares to reinitialize the // channel, and not have Envoy track side effects. - // context: https://github.com/envoyproxy/envoy/issues/4543. + // context: https://github.com/envoyproxy/envoy/issues/4543 and + // https://github.com/c-ares/c-ares/issues/301. if (status == ARES_ECONNREFUSED && !fallback_if_failed_) { parent_.dirty_channel_ = true; return; From 0db2c6bbe9f6dd58b3c4503f01d24e9aa6ae1753 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 31 Jan 2020 15:31:24 -0800 Subject: [PATCH 4/8] fmt Signed-off-by: Jose Nino --- source/common/network/dns_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index e135114aa7ab2..362ef76088982 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -78,7 +78,7 @@ void DnsResolverImpl::initializeChannel(ares_options* options, int optmask) { void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, int timeouts, ares_addrinfo* addrinfo) { // If c-ares returns ARES_ECONNREFUSED and there is no fallback we assume that the channel_ is - // broken. Mark the channel dirty so that it is destroyed and reinitialized on a subsequest call + // broken. Mark the channel dirty so that it is destroyed and reinitialized on a subsequent call // to DnsResolver::resolve(). The optimal solution would be for c-ares to reinitialize the // channel, and not have Envoy track side effects. // context: https://github.com/envoyproxy/envoy/issues/4543 and From de58b0143ac2dc74f90870c92a46bccf3ca84503 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 31 Jan 2020 16:51:42 -0800 Subject: [PATCH 5/8] comments Signed-off-by: Jose Nino --- source/common/network/dns_impl.cc | 35 +++++++++++++------------------ source/common/network/dns_impl.h | 6 +++--- 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 362ef76088982..991ee1e97fa75 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -26,15 +26,8 @@ DnsResolverImpl::DnsResolverImpl( : dispatcher_(dispatcher), timer_(dispatcher.createTimer([this] { onEventCallback(ARES_SOCKET_BAD, 0); })), use_tcp_for_dns_lookups_(use_tcp_for_dns_lookups) { - ares_options options{}; - int optmask = 0; - - if (use_tcp_for_dns_lookups_) { - optmask |= ARES_OPT_FLAGS; - options.flags |= ARES_FLAG_USEVC; - } - initializeChannel(&options, optmask); + initializeChannel(); if (!resolvers.empty()) { std::vector resolver_addrs; @@ -66,7 +59,15 @@ DnsResolverImpl::~DnsResolverImpl() { ares_destroy(channel_); } -void DnsResolverImpl::initializeChannel(ares_options* options, int optmask) { +void DnsResolverImpl::initializeChannel() { + ares_options options{}; + int optmask = 0; + + if (use_tcp_for_dns_lookups_) { + optmask |= ARES_OPT_FLAGS; + options.flags |= ARES_FLAG_USEVC; + } + options->sock_state_cb = [](void* arg, int fd, int read, int write) { static_cast(arg)->onAresSocketStateChange(fd, read, write); }; @@ -83,6 +84,9 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i // channel, and not have Envoy track side effects. // context: https://github.com/envoyproxy/envoy/issues/4543 and // https://github.com/c-ares/c-ares/issues/301. + // + // n.b: the channel cannot be destroyed and reinitialized here because that leads to a c-ares + // segfault. if (status == ARES_ECONNREFUSED && !fallback_if_failed_) { parent_.dirty_channel_ = true; return; @@ -183,7 +187,6 @@ void DnsResolverImpl::updateAresTimer() { } void DnsResolverImpl::onEventCallback(int fd, uint32_t events) { - ENVOY_LOG(error, "onEventCallback"); const ares_socket_t read_fd = events & Event::FileReadyType::Read ? fd : ARES_SOCKET_BAD; const ares_socket_t write_fd = events & Event::FileReadyType::Write ? fd : ARES_SOCKET_BAD; ares_process_fd(channel_, read_fd, write_fd); @@ -219,17 +222,9 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, // @see DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback for why this is done. if (dirty_channel_) { + dirty_channel_ = false; ares_destroy(channel_); - - ares_options options{}; - int optmask = 0; - - if (use_tcp_for_dns_lookups_) { - optmask |= ARES_OPT_FLAGS; - options.flags |= ARES_FLAG_USEVC; - } - - initializeChannel(&options, optmask); + initializeChannel(); } std::unique_ptr pending_resolution( new PendingResolution(*this, callback, dispatcher_, channel_, dns_name)); diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index 55b793084833d..1af98a0203076 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -87,8 +87,8 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable events_; }; From 84c5a3c733f47e64ecb1a8e820645979eda00c5e Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 4 Feb 2020 16:36:49 -0800 Subject: [PATCH 6/8] update Signed-off-by: Jose Nino --- source/common/network/dns_impl.cc | 49 ++++++++++--------- source/common/network/dns_impl.h | 9 +++- test/common/network/dns_impl_test.cc | 72 ++++++++++++++++++++++++++-- 3 files changed, 103 insertions(+), 27 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 991ee1e97fa75..505b1bea7dc14 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -27,7 +27,8 @@ DnsResolverImpl::DnsResolverImpl( timer_(dispatcher.createTimer([this] { onEventCallback(ARES_SOCKET_BAD, 0); })), use_tcp_for_dns_lookups_(use_tcp_for_dns_lookups) { - initializeChannel(); + AresOptions options = defaultAresOptions(); + initializeChannel(&options.options_, options.optmask_); if (!resolvers.empty()) { std::vector resolver_addrs; @@ -59,38 +60,27 @@ DnsResolverImpl::~DnsResolverImpl() { ares_destroy(channel_); } -void DnsResolverImpl::initializeChannel() { - ares_options options{}; - int optmask = 0; +DnsResolverImpl::AresOptions DnsResolverImpl::defaultAresOptions() { + AresOptions options{}; if (use_tcp_for_dns_lookups_) { - optmask |= ARES_OPT_FLAGS; - options.flags |= ARES_FLAG_USEVC; + options.optmask_ |= ARES_OPT_FLAGS; + options.options_.flags |= ARES_FLAG_USEVC; } + return options; +} + +void DnsResolverImpl::initializeChannel(ares_options* options, int optmask) { options->sock_state_cb = [](void* arg, int fd, int read, int write) { static_cast(arg)->onAresSocketStateChange(fd, read, write); }; options->sock_state_cb_data = this; - int status = ares_init_options(&channel_, options, optmask | ARES_OPT_SOCK_STATE_CB); - ENVOY_LOG(error, "ARES CHANNEL INIT {}", status); + ares_init_options(&channel_, options, optmask | ARES_OPT_SOCK_STATE_CB); } void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, int timeouts, ares_addrinfo* addrinfo) { - // If c-ares returns ARES_ECONNREFUSED and there is no fallback we assume that the channel_ is - // broken. Mark the channel dirty so that it is destroyed and reinitialized on a subsequent call - // to DnsResolver::resolve(). The optimal solution would be for c-ares to reinitialize the - // channel, and not have Envoy track side effects. - // context: https://github.com/envoyproxy/envoy/issues/4543 and - // https://github.com/c-ares/c-ares/issues/301. - // - // n.b: the channel cannot be destroyed and reinitialized here because that leads to a c-ares - // segfault. - if (status == ARES_ECONNREFUSED && !fallback_if_failed_) { - parent_.dirty_channel_ = true; - return; - } // We receive ARES_EDESTRUCTION when destructing with pending queries. if (status == ARES_EDESTRUCTION) { ASSERT(owned_); @@ -99,6 +89,19 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i } if (!fallback_if_failed_) { completed_ = true; + + // If c-ares returns ARES_ECONNREFUSED and there is no fallback we assume that the channel_ is + // broken. Mark the channel dirty so that it is destroyed and reinitialized on a subsequent call + // to DnsResolver::resolve(). The optimal solution would be for c-ares to reinitialize the + // channel, and not have Envoy track side effects. + // context: https://github.com/envoyproxy/envoy/issues/4543 and + // https://github.com/c-ares/c-ares/issues/301. + // + // The channel cannot be destroyed and reinitialized here because that leads to a c-ares + // segfault. + if (status == ARES_ECONNREFUSED) { + parent_.dirty_channel_ = true; + } } std::list address_list; @@ -224,7 +227,9 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, if (dirty_channel_) { dirty_channel_ = false; ares_destroy(channel_); - initializeChannel(); + + AresOptions options = defaultAresOptions(); + initializeChannel(&options.options_, options.optmask_); } std::unique_ptr pending_resolution( new PendingResolution(*this, callback, dispatcher_, channel_, dns_name)); diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index 1af98a0203076..79a140dea6cf9 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -82,15 +82,22 @@ class DnsResolverImpl : public DnsResolver, protected Logger::LoggableaddReadFilter(Network::ReadFilterSharedPtr{new ReadFilter(*this)}); } @@ -171,7 +171,11 @@ class TestDnsServerQuery { memcpy(response_base, request, response_base_len); DNS_HEADER_SET_QR(response_base, 1); DNS_HEADER_SET_AA(response_base, 0); - DNS_HEADER_SET_RCODE(response_base, answer_size > 0 ? NOERROR : NXDOMAIN); + if (parent_.refused_) { + DNS_HEADER_SET_RCODE(response_base, REFUSED); + } else { + DNS_HEADER_SET_RCODE(response_base, answer_size > 0 ? NOERROR : NXDOMAIN); + } DNS_HEADER_SET_ANCOUNT(response_base, answer_size); DNS_HEADER_SET_NSCOUNT(response_base, 0); DNS_HEADER_SET_ARCOUNT(response_base, 0); @@ -253,6 +257,7 @@ class TestDnsServerQuery { const HostMap& hosts_aaaa_; const CNameMap& cnames_; const std::chrono::seconds& record_ttl_; + bool refused_{}; }; class TestDnsServer : public ListenerCallbacks { @@ -263,7 +268,7 @@ class TestDnsServer : public ListenerCallbacks { Network::ConnectionPtr new_connection = dispatcher_.createServerConnection( std::move(socket), Network::Test::createRawBufferSocket()); TestDnsServerQuery* query = new TestDnsServerQuery(std::move(new_connection), hosts_a_, - hosts_aaaa_, cnames_, record_ttl_); + hosts_aaaa_, cnames_, record_ttl_, refused_); queries_.emplace_back(query); } @@ -280,6 +285,7 @@ class TestDnsServer : public ListenerCallbacks { } void setRecordTtl(const std::chrono::seconds& ttl) { record_ttl_ = ttl; } + void setRefused(bool refused) { refused_ = refused; } private: Event::Dispatcher& dispatcher_; @@ -288,6 +294,7 @@ class TestDnsServer : public ListenerCallbacks { HostMap hosts_aaaa_; CNameMap cnames_; std::chrono::seconds record_ttl_; + bool refused_{}; // All queries are tracked so we can do resource reclamation when the test is // over. std::vector> queries_; @@ -300,6 +307,7 @@ class DnsResolverImplPeer { DnsResolverImplPeer(DnsResolverImpl* resolver) : resolver_(resolver) {} ares_channel channel() const { return resolver_->channel_; } + bool isChannelDirty() const { return resolver_->dirty_channel_; } const std::unordered_map& events() { return resolver_->events_; } // Reset the channel state for a DnsResolverImpl such that it will only use // TCP and optionally has a zero timeout (for validating timeout behavior). @@ -467,6 +475,62 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); +TEST_P(DnsImplTest, DestroyChannelOnRefused) { + ASSERT_FALSE(peer_->isChannelDirty()); + server_->addHosts("some.good.domain", {"201.134.56.7"}, RecordType::A); + server_->setRefused(true); + + std::list address_list; + EXPECT_NE(nullptr, resolver_->resolve("", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = getAddressList(results); + dispatcher_->exit(); + })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); + // The c-ares channel should be dirty because the TestDnsServer replied with return code REFUSED; + // This test, and the way the TestDnsServerQuery is setup, relies on the fact that Envoy's + // c-ares channel is configured **without** the ARES_FLAG_NOCHECKRESP flag. This causes c-ares to + // discard packets with REFUSED, and thus Envoy receives ARES_ECONNREFUSED due to the code here: + // https://github.com/c-ares/c-ares/blob/d7e070e7283f822b1d2787903cce3615536c5610/ares_process.c#L654 + // If that flag needs to be set, or c-ares changes its handling this test will need to be updated + // to create another condition where c-ares invokes onAresGetAddrInfoCallback with status == + // ARES_ECONNREFUSED. + EXPECT_TRUE(peer_->isChannelDirty()); + EXPECT_TRUE(address_list.empty()); + + server_->setRefused(false); + + // Resolve will destroy the original channel and create a new one. + EXPECT_NE(nullptr, resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = getAddressList(results); + dispatcher_->exit(); + })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); + // However, the fresh channel initialized by production code does not point to the TestDnsServer. + // This means that resolution will return ARES_ENOTFOUND. This should not dirty the channel. + EXPECT_FALSE(peer_->isChannelDirty()); + EXPECT_TRUE(address_list.empty()); + + // Reset the channel to point to the TestDndServer, and make sure resolution is healthy. + if (tcp_only()) { + peer_->resetChannelTcpOnly(zero_timeout()); + } + ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str()); + + EXPECT_NE(nullptr, resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, + [&](std::list&& results) -> void { + address_list = getAddressList(results); + dispatcher_->exit(); + })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); + EXPECT_FALSE(peer_->isChannelDirty()); + EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); +} + // 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 From 49b7c29e48c43341593dce5c1798d31dbf730ec5 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 4 Feb 2020 16:39:49 -0800 Subject: [PATCH 7/8] spelling Signed-off-by: Jose Nino --- test/common/network/dns_impl_test.cc | 2 +- tools/spelling_dictionary.txt | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index b90dcce4f2c32..41748e21ea283 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -514,7 +514,7 @@ TEST_P(DnsImplTest, DestroyChannelOnRefused) { EXPECT_FALSE(peer_->isChannelDirty()); EXPECT_TRUE(address_list.empty()); - // Reset the channel to point to the TestDndServer, and make sure resolution is healthy. + // Reset the channel to point to the TestDnsServer, and make sure resolution is healthy. if (tcp_only()) { peer_->resetChannelTcpOnly(zero_timeout()); } diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index aafa29d7f61f3..27e007dda358d 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -76,6 +76,7 @@ EDESTRUCTION EDF EINVAL ELB +ENOTFOUND ENOTSUP ENV EOF @@ -181,6 +182,7 @@ NDEBUG NEXTHDR NGHTTP NOAUTH +NOCHECKRESP NODELAY NOLINT NOLINTNEXTLINE From 037f8a40b2f5d589cfbda6db4afc2c96cf4857ad Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 4 Feb 2020 16:50:13 -0800 Subject: [PATCH 8/8] reorder test Signed-off-by: Jose Nino --- test/common/network/dns_impl_test.cc | 114 ++++++++++++++------------- 1 file changed, 58 insertions(+), 56 deletions(-) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 41748e21ea283..65bc03c10dc97 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -475,62 +475,6 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); -TEST_P(DnsImplTest, DestroyChannelOnRefused) { - ASSERT_FALSE(peer_->isChannelDirty()); - server_->addHosts("some.good.domain", {"201.134.56.7"}, RecordType::A); - server_->setRefused(true); - - std::list address_list; - EXPECT_NE(nullptr, resolver_->resolve("", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - address_list = getAddressList(results); - dispatcher_->exit(); - })); - - dispatcher_->run(Event::Dispatcher::RunType::Block); - // The c-ares channel should be dirty because the TestDnsServer replied with return code REFUSED; - // This test, and the way the TestDnsServerQuery is setup, relies on the fact that Envoy's - // c-ares channel is configured **without** the ARES_FLAG_NOCHECKRESP flag. This causes c-ares to - // discard packets with REFUSED, and thus Envoy receives ARES_ECONNREFUSED due to the code here: - // https://github.com/c-ares/c-ares/blob/d7e070e7283f822b1d2787903cce3615536c5610/ares_process.c#L654 - // If that flag needs to be set, or c-ares changes its handling this test will need to be updated - // to create another condition where c-ares invokes onAresGetAddrInfoCallback with status == - // ARES_ECONNREFUSED. - EXPECT_TRUE(peer_->isChannelDirty()); - EXPECT_TRUE(address_list.empty()); - - server_->setRefused(false); - - // Resolve will destroy the original channel and create a new one. - EXPECT_NE(nullptr, resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - address_list = getAddressList(results); - dispatcher_->exit(); - })); - - dispatcher_->run(Event::Dispatcher::RunType::Block); - // However, the fresh channel initialized by production code does not point to the TestDnsServer. - // This means that resolution will return ARES_ENOTFOUND. This should not dirty the channel. - EXPECT_FALSE(peer_->isChannelDirty()); - EXPECT_TRUE(address_list.empty()); - - // Reset the channel to point to the TestDnsServer, and make sure resolution is healthy. - if (tcp_only()) { - peer_->resetChannelTcpOnly(zero_timeout()); - } - ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str()); - - EXPECT_NE(nullptr, resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = getAddressList(results); - dispatcher_->exit(); - })); - - dispatcher_->run(Event::Dispatcher::RunType::Block); - EXPECT_FALSE(peer_->isChannelDirty()); - EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); -} - // 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 @@ -646,6 +590,64 @@ TEST_P(DnsImplTest, CallbackException) { "unknown"); } +// Validate that the c-ares channel is destroyed and re-initialized when c-ares returns +// ARES_ECONNREFUSED as its callback status. +TEST_P(DnsImplTest, DestroyChannelOnRefused) { + ASSERT_FALSE(peer_->isChannelDirty()); + server_->addHosts("some.good.domain", {"201.134.56.7"}, RecordType::A); + server_->setRefused(true); + + std::list address_list; + EXPECT_NE(nullptr, resolver_->resolve("", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = getAddressList(results); + dispatcher_->exit(); + })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); + // The c-ares channel should be dirty because the TestDnsServer replied with return code REFUSED; + // This test, and the way the TestDnsServerQuery is setup, relies on the fact that Envoy's + // c-ares channel is configured **without** the ARES_FLAG_NOCHECKRESP flag. This causes c-ares to + // discard packets with REFUSED, and thus Envoy receives ARES_ECONNREFUSED due to the code here: + // https://github.com/c-ares/c-ares/blob/d7e070e7283f822b1d2787903cce3615536c5610/ares_process.c#L654 + // If that flag needs to be set, or c-ares changes its handling this test will need to be updated + // to create another condition where c-ares invokes onAresGetAddrInfoCallback with status == + // ARES_ECONNREFUSED. + EXPECT_TRUE(peer_->isChannelDirty()); + EXPECT_TRUE(address_list.empty()); + + server_->setRefused(false); + + // Resolve will destroy the original channel and create a new one. + EXPECT_NE(nullptr, resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = getAddressList(results); + dispatcher_->exit(); + })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); + // However, the fresh channel initialized by production code does not point to the TestDnsServer. + // This means that resolution will return ARES_ENOTFOUND. This should not dirty the channel. + EXPECT_FALSE(peer_->isChannelDirty()); + EXPECT_TRUE(address_list.empty()); + + // Reset the channel to point to the TestDnsServer, and make sure resolution is healthy. + if (tcp_only()) { + peer_->resetChannelTcpOnly(zero_timeout()); + } + ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str()); + + EXPECT_NE(nullptr, resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, + [&](std::list&& results) -> void { + address_list = getAddressList(results); + dispatcher_->exit(); + })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); + EXPECT_FALSE(peer_->isChannelDirty()); + EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); +} + TEST_P(DnsImplTest, DnsIpAddressVersion) { std::list address_list; server_->addHosts("some.good.domain", {"1.2.3.4"}, RecordType::A);