From 0487adba5001fa3ff5fd2b51f82892a43a97f7b0 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 29 Oct 2020 09:59:42 -0700 Subject: [PATCH 01/10] test Signed-off-by: Jose Nino --- test/common/network/dns_impl_test.cc | 43 ++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 339cfb4abc89e..a95a97a7b6cf7 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -434,14 +434,18 @@ class DnsImplTest : public testing::TestWithParam { : api_(Api::createApiForTest()), dispatcher_(api_->allocateDispatcher("test_thread")) {} void SetUp() override { - resolver_ = dispatcher_->createDnsResolver({}, use_tcp_for_dns_lookups()); - // Instantiate TestDnsServer and listen on a random port on the loopback address. server_ = std::make_unique(*dispatcher_); socket_ = std::make_shared( Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, true); listener_ = dispatcher_->createListener(socket_, *server_, true, ENVOY_TCP_BACKLOG_SIZE); + if (set_resolver_in_constructor()) { + resolver_ = dispatcher_->createDnsResolver({socket_->localAddress()}, use_tcp_for_dns_lookups()); + } else { + resolver_ = dispatcher_->createDnsResolver({}, use_tcp_for_dns_lookups()); + } + // Point c-ares at the listener with no search domains and TCP-only. peer_ = std::make_unique(dynamic_cast(resolver_.get())); if (tcp_only()) { @@ -542,6 +546,7 @@ class DnsImplTest : public testing::TestWithParam { virtual bool zero_timeout() const { return false; } virtual bool tcp_only() const { return true; } virtual bool use_tcp_for_dns_lookups() const { return false; } + virtual bool set_resolver_in_constructor() const { return false; } std::unique_ptr server_; std::unique_ptr peer_; Network::MockConnectionHandler connection_handler_; @@ -945,5 +950,39 @@ TEST_P(DnsImplAresFlagsForUdpTest, UdpLookupsEnabled) { ares_destroy_options(&opts); } +class DnsImplCustomResolverTest : public DnsImplTest { + bool tcp_only() const override { return false; } + bool use_tcp_for_dns_lookups() const override { return true; } + bool set_resolver_in_constructor() const override { return true; } +}; + +TEST_P(DnsImplCustomResolverTest, CustomResolverValidAfterChannelDestruction) { + ASSERT_FALSE(peer_->isChannelDirty()); + server_->addHosts("some.good.domain", {"201.134.56.7"}, RecordType::A); + server_->setRefused(true); + + EXPECT_NE(nullptr, + resolveWithExpectations("", DnsLookupFamily::V4Only, + DnsResolver::ResolutionStatus::Failure, {}, {}, absl::nullopt)); + 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()); + + server_->setRefused(false); + + EXPECT_NE(nullptr, resolveWithExpectations("some.good.domain", DnsLookupFamily::Auto, + DnsResolver::ResolutionStatus::Success, + {"201.134.56.7"}, {}, absl::nullopt)); + dispatcher_->run(Event::Dispatcher::RunType::Block); + EXPECT_FALSE(peer_->isChannelDirty()); +} + } // namespace Network } // namespace Envoy From ccdf449fa41ebd0dc6a4836b80f7f3adb347a212 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 29 Oct 2020 10:03:15 -0700 Subject: [PATCH 02/10] parametrized test Signed-off-by: Jose Nino --- test/common/network/dns_impl_test.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index a95a97a7b6cf7..f5f585c31c32f 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -956,6 +956,11 @@ class DnsImplCustomResolverTest : public DnsImplTest { bool set_resolver_in_constructor() const override { return true; } }; +// Parameterize the DNS test server socket address. +INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplCustomResolverTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + TEST_P(DnsImplCustomResolverTest, CustomResolverValidAfterChannelDestruction) { ASSERT_FALSE(peer_->isChannelDirty()); server_->addHosts("some.good.domain", {"201.134.56.7"}, RecordType::A); From 3e0f6bbde8eec99026ebdbf2d5a87d80d055b378 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 29 Oct 2020 10:17:07 -0700 Subject: [PATCH 03/10] test Signed-off-by: Jose Nino --- test/common/network/dns_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index f5f585c31c32f..0e038521d6ccc 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -967,7 +967,7 @@ TEST_P(DnsImplCustomResolverTest, CustomResolverValidAfterChannelDestruction) { server_->setRefused(true); EXPECT_NE(nullptr, - resolveWithExpectations("", DnsLookupFamily::V4Only, + resolveWithExpectations("some.good.domain", DnsLookupFamily::V4Only, DnsResolver::ResolutionStatus::Failure, {}, {}, absl::nullopt)); dispatcher_->run(Event::Dispatcher::RunType::Block); // The c-ares channel should be dirty because the TestDnsServer replied with return code REFUSED; From 6bd6f810f92efd303c194008228295b1324291cf Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 29 Oct 2020 10:27:57 -0700 Subject: [PATCH 04/10] fmt Signed-off-by: Jose Nino --- test/common/network/dns_impl_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 0e038521d6ccc..7388dca842d28 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -441,7 +441,8 @@ class DnsImplTest : public testing::TestWithParam { listener_ = dispatcher_->createListener(socket_, *server_, true, ENVOY_TCP_BACKLOG_SIZE); if (set_resolver_in_constructor()) { - resolver_ = dispatcher_->createDnsResolver({socket_->localAddress()}, use_tcp_for_dns_lookups()); + resolver_ = + dispatcher_->createDnsResolver({socket_->localAddress()}, use_tcp_for_dns_lookups()); } else { resolver_ = dispatcher_->createDnsResolver({}, use_tcp_for_dns_lookups()); } From 490bd9756379c30d6ca6a700417d5fd1e0a4d048 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 29 Oct 2020 10:28:05 -0700 Subject: [PATCH 05/10] fix Signed-off-by: Jose Nino --- source/common/network/dns_impl.cc | 65 ++++++++++++++++++------------- source/common/network/dns_impl.h | 4 ++ 2 files changed, 41 insertions(+), 28 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index d44d53c70f39e..590aef2048f3e 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -25,34 +25,10 @@ DnsResolverImpl::DnsResolverImpl( 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) { - + use_tcp_for_dns_lookups_(use_tcp_for_dns_lookups), + resolvers_csv_(maybeBuildResolversCsv(resolvers)) { AresOptions options = defaultAresOptions(); initializeChannel(&options.options_, options.optmask_); - - if (!resolvers.empty()) { - std::vector resolver_addrs; - resolver_addrs.reserve(resolvers.size()); - for (const auto& resolver : resolvers) { - // This should be an IP address (i.e. not a pipe). - if (resolver->ip() == nullptr) { - ares_destroy(channel_); - throw EnvoyException( - fmt::format("DNS resolver '{}' is not an IP address", resolver->asString())); - } - // Note that the ip()->port() may be zero if the port is not fully specified by the - // Address::Instance. - // resolver->asString() is avoided as that format may be modified by custom - // Address::Instance implementations in ways that make the not a simple - // integer. See https://github.com/envoyproxy/envoy/pull/3366. - resolver_addrs.push_back(fmt::format(resolver->ip()->ipv6() ? "[{}]:{}" : "{}:{}", - resolver->ip()->addressAsString(), - resolver->ip()->port())); - } - const std::string resolvers_csv = absl::StrJoin(resolver_addrs, ","); - int result = ares_set_servers_ports_csv(channel_, resolvers_csv.c_str()); - RELEASE_ASSERT(result == ARES_SUCCESS, ""); - } } DnsResolverImpl::~DnsResolverImpl() { @@ -60,6 +36,32 @@ DnsResolverImpl::~DnsResolverImpl() { ares_destroy(channel_); } +absl::optional DnsResolverImpl::maybeBuildResolversCsv( + const std::vector& resolvers) { + if (resolvers.empty()) { + return absl::nullopt; + } + + std::vector resolver_addrs; + resolver_addrs.reserve(resolvers.size()); + for (const auto& resolver : resolvers) { + // This should be an IP address (i.e. not a pipe). + if (resolver->ip() == nullptr) { + throw EnvoyException( + fmt::format("DNS resolver '{}' is not an IP address", resolver->asString())); + } + // Note that the ip()->port() may be zero if the port is not fully specified by the + // Address::Instance. + // resolver->asString() is avoided as that format may be modified by custom + // Address::Instance implementations in ways that make the not a simple + // integer. See https://github.com/envoyproxy/envoy/pull/3366. + resolver_addrs.push_back(fmt::format(resolver->ip()->ipv6() ? "[{}]:{}" : "{}:{}", + resolver->ip()->addressAsString(), + resolver->ip()->port())); + } + return {absl::StrJoin(resolver_addrs, ",")}; +} + DnsResolverImpl::AresOptions DnsResolverImpl::defaultAresOptions() { AresOptions options{}; @@ -72,11 +74,19 @@ DnsResolverImpl::AresOptions DnsResolverImpl::defaultAresOptions() { } void DnsResolverImpl::initializeChannel(ares_options* options, int optmask) { + dirty_channel_ = false; + options->sock_state_cb = [](void* arg, os_fd_t fd, int read, int write) { static_cast(arg)->onAresSocketStateChange(fd, read, write); }; options->sock_state_cb_data = this; ares_init_options(&channel_, options, optmask | ARES_OPT_SOCK_STATE_CB); + + // Ensure that the channel points to custom resolvers, if they exist. + if (resolvers_csv_.has_value()) { + int result = ares_set_servers_ports_csv(channel_, resolvers_csv_->c_str()); + RELEASE_ASSERT(result == ARES_SUCCESS, ""); + } } void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, int timeouts, @@ -236,12 +246,11 @@ 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_); - AresOptions options = defaultAresOptions(); initializeChannel(&options.options_, options.optmask_); } + std::unique_ptr pending_resolution( new PendingResolution(*this, callback, dispatcher_, channel_, dns_name)); if (dns_lookup_family == DnsLookupFamily::Auto) { diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index dc62e06adb112..ace150273f31a 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -87,6 +87,9 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable + maybeBuildResolversCsv(const std::vector& resolvers); + // Callback for events on sockets tracked in events_. void onEventCallback(os_fd_t fd, uint32_t events); // c-ares callback when a socket state changes, indicating that libevent @@ -105,6 +108,7 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable events_; + const absl::optional resolvers_csv_; }; } // namespace Network From 025a0d6d08d7f5c2ff909d97aa31bdc5d43aa393 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 29 Oct 2020 10:37:14 -0700 Subject: [PATCH 06/10] comment on test Signed-off-by: Jose Nino --- test/common/network/dns_impl_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 7388dca842d28..1318cbaadf531 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -983,6 +983,9 @@ TEST_P(DnsImplCustomResolverTest, CustomResolverValidAfterChannelDestruction) { server_->setRefused(false); + // The next query destroys, and reinitializes the channel. Furthermore, because the test dns + // server's address was passed as a custom resolver on construction, the new channel should still + // point to the test dns server, and the query should succeed. EXPECT_NE(nullptr, resolveWithExpectations("some.good.domain", DnsLookupFamily::Auto, DnsResolver::ResolutionStatus::Success, {"201.134.56.7"}, {}, absl::nullopt)); From 2e1c6351b95d6ea0addfea8c7e6af69313472412 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 29 Oct 2020 10:37:52 -0700 Subject: [PATCH 07/10] comment on test Signed-off-by: Jose Nino --- test/common/network/dns_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 1318cbaadf531..3b1d7b60579bb 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -983,7 +983,7 @@ TEST_P(DnsImplCustomResolverTest, CustomResolverValidAfterChannelDestruction) { server_->setRefused(false); - // The next query destroys, and reinitializes the channel. Furthermore, because the test dns + // The next query destroys, and re-initializes the channel. Furthermore, because the test dns // server's address was passed as a custom resolver on construction, the new channel should still // point to the test dns server, and the query should succeed. EXPECT_NE(nullptr, resolveWithExpectations("some.good.domain", DnsLookupFamily::Auto, From 9096ad8142c1b1e1906dffab0fe256423b046194 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 29 Oct 2020 10:44:40 -0700 Subject: [PATCH 08/10] release notes Signed-off-by: Jose Nino --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 763c1f98b11b6..29dd5603b4b3b 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -20,6 +20,7 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* +* dns: fix a bug where custom resolvers provided in configuration were not preserved after network issues. * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. * tls: fix read resumption after triggering buffer high-watermark and all remaining request/response bytes are stored in the SSL connection's internal buffers. From 07f46aae8dbdf75829e4a1ee1ea0e40a01610e95 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 29 Oct 2020 13:55:39 -0700 Subject: [PATCH 09/10] clang tidy Signed-off-by: Jose Nino --- test/common/network/dns_impl_test.cc | 38 ++++++++++++++-------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 3b1d7b60579bb..814472c34b857 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -440,17 +440,17 @@ class DnsImplTest : public testing::TestWithParam { Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, true); listener_ = dispatcher_->createListener(socket_, *server_, true, ENVOY_TCP_BACKLOG_SIZE); - if (set_resolver_in_constructor()) { + if (setResolverInConstructor()) { resolver_ = - dispatcher_->createDnsResolver({socket_->localAddress()}, use_tcp_for_dns_lookups()); + dispatcher_->createDnsResolver({socket_->localAddress()}, useTcpForDnsLookups()); } else { - resolver_ = dispatcher_->createDnsResolver({}, use_tcp_for_dns_lookups()); + resolver_ = dispatcher_->createDnsResolver({}, useTcpForDnsLookups()); } // Point c-ares at the listener with no search domains and TCP-only. peer_ = std::make_unique(dynamic_cast(resolver_.get())); - if (tcp_only()) { - peer_->resetChannelTcpOnly(zero_timeout()); + if (tcpOnly()) { + peer_->resetChannelTcpOnly(zeroTimeout()); } ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str()); } @@ -544,10 +544,10 @@ class DnsImplTest : public testing::TestWithParam { protected: // Should the DnsResolverImpl use a zero timeout for c-ares queries? - virtual bool zero_timeout() const { return false; } - virtual bool tcp_only() const { return true; } - virtual bool use_tcp_for_dns_lookups() const { return false; } - virtual bool set_resolver_in_constructor() const { return false; } + virtual bool zeroTimeout() const { return false; } + virtual bool tcpOnly() const { return true; } + virtual bool useTcpForDnsLookups() const { return false; } + virtual bool setResolverInConstructor() const { return false; } std::unique_ptr server_; std::unique_ptr peer_; Network::MockConnectionHandler connection_handler_; @@ -585,7 +585,7 @@ TEST_P(DnsImplTest, DestructCallback) { // This simulates destruction thanks to another query setting the dirty_channel_ bit, thus causing // a subsequent result to call ares_destroy. - peer_->resetChannelTcpOnly(zero_timeout()); + peer_->resetChannelTcpOnly(zeroTimeout()); ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str()); dispatcher_->run(Event::Dispatcher::RunType::Block); @@ -710,8 +710,8 @@ TEST_P(DnsImplTest, DestroyChannelOnRefused) { EXPECT_FALSE(peer_->isChannelDirty()); // Reset the channel to point to the TestDnsServer, and make sure resolution is healthy. - if (tcp_only()) { - peer_->resetChannelTcpOnly(zero_timeout()); + if (tcpOnly()) { + peer_->resetChannelTcpOnly(zeroTimeout()); } ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str()); @@ -884,7 +884,7 @@ TEST_P(DnsImplTest, PendingTimerEnable) { class DnsImplZeroTimeoutTest : public DnsImplTest { protected: - bool zero_timeout() const override { return true; } + bool zeroTimeout() const override { return true; } }; // Parameterize the DNS test server socket address. @@ -904,8 +904,8 @@ TEST_P(DnsImplZeroTimeoutTest, Timeout) { class DnsImplAresFlagsForTcpTest : public DnsImplTest { protected: - bool tcp_only() const override { return false; } - bool use_tcp_for_dns_lookups() const override { return true; } + bool tcpOnly() const override { return false; } + bool useTcpForDnsLookups() const override { return true; } }; // Parameterize the DNS test server socket address. @@ -929,7 +929,7 @@ TEST_P(DnsImplAresFlagsForTcpTest, TcpLookupsEnabled) { class DnsImplAresFlagsForUdpTest : public DnsImplTest { protected: - bool tcp_only() const override { return false; } + bool tcpOnly() const override { return false; } }; // Parameterize the DNS test server socket address. @@ -952,9 +952,9 @@ TEST_P(DnsImplAresFlagsForUdpTest, UdpLookupsEnabled) { } class DnsImplCustomResolverTest : public DnsImplTest { - bool tcp_only() const override { return false; } - bool use_tcp_for_dns_lookups() const override { return true; } - bool set_resolver_in_constructor() const override { return true; } + bool tcpOnly() const override { return false; } + bool useTcpForDnsLookups() const override { return true; } + bool setResolverInConstructor() const override { return true; } }; // Parameterize the DNS test server socket address. From ef642f68c0bddbad23bbd085bfdf841d38a98337 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 29 Oct 2020 13:58:05 -0700 Subject: [PATCH 10/10] fmt Signed-off-by: Jose Nino --- test/common/network/dns_impl_test.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 814472c34b857..0489bc067d399 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -441,8 +441,7 @@ class DnsImplTest : public testing::TestWithParam { listener_ = dispatcher_->createListener(socket_, *server_, true, ENVOY_TCP_BACKLOG_SIZE); if (setResolverInConstructor()) { - resolver_ = - dispatcher_->createDnsResolver({socket_->localAddress()}, useTcpForDnsLookups()); + resolver_ = dispatcher_->createDnsResolver({socket_->localAddress()}, useTcpForDnsLookups()); } else { resolver_ = dispatcher_->createDnsResolver({}, useTcpForDnsLookups()); }