dns: preserve custom resolver after channel destruction

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: fixed URL parsing for HTTP/1.1 fully qualified URLs and connect requests containing IPv6 addresses.
 * 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.

source/common/network/dns_impl.cc

@@ -25,41 +25,43 @@ 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<std::string> 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 <port> 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() {
   timer_->disableTimer();
   ares_destroy(channel_);
 }
 
+absl::optional<std::string> DnsResolverImpl::maybeBuildResolversCsv(
+    const std::vector<Network::Address::InstanceConstSharedPtr>& resolvers) {
+  if (resolvers.empty()) {
+    return absl::nullopt;
+  }
+
+  std::vector<std::string> 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 <port> 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<DnsResolverImpl*>(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<PendingResolution> pending_resolution(
       new PendingResolution(*this, callback, dispatcher_, channel_, dns_name));
   if (dns_lookup_family == DnsLookupFamily::Auto) {

source/common/network/dns_impl.h

@@ -87,6 +87,9 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
     int optmask_;
   };
 
+  static absl::optional<std::string>
+  maybeBuildResolversCsv(const std::vector<Network::Address::InstanceConstSharedPtr>& 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<Logger::I
   bool dirty_channel_{};
   const bool use_tcp_for_dns_lookups_;
   absl::node_hash_map<int, Event::FileEventPtr> events_;
+  const absl::optional<std::string> resolvers_csv_;
 };
 
 } // namespace Network

test/common/network/dns_impl_test.cc

@@ -434,18 +434,22 @@ class DnsImplTest : public testing::TestWithParam<Address::IpVersion> {
       : 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<TestDnsServer>(*dispatcher_);
     socket_ = std::make_shared<Network::TcpListenSocket>(
         Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, true);
     listener_ = dispatcher_->createListener(socket_, *server_, true, ENVOY_TCP_BACKLOG_SIZE);
 
+    if (setResolverInConstructor()) {
+      resolver_ = dispatcher_->createDnsResolver({socket_->localAddress()}, useTcpForDnsLookups());
+    } else {
+      resolver_ = dispatcher_->createDnsResolver({}, useTcpForDnsLookups());
+    }
+
     // Point c-ares at the listener with no search domains and TCP-only.
     peer_ = std::make_unique<DnsResolverImplPeer>(dynamic_cast<DnsResolverImpl*>(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());
   }
@@ -539,9 +543,10 @@ class DnsImplTest : public testing::TestWithParam<Address::IpVersion> {
 
 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 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<TestDnsServer> server_;
   std::unique_ptr<DnsResolverImplPeer> peer_;
   Network::MockConnectionHandler connection_handler_;
@@ -579,7 +584,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);
@@ -704,8 +709,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());
 
@@ -878,7 +883,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.
@@ -898,8 +903,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.
@@ -923,7 +928,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.
@@ -945,5 +950,47 @@ TEST_P(DnsImplAresFlagsForUdpTest, UdpLookupsEnabled) {
   ares_destroy_options(&opts);
 }
 
+class DnsImplCustomResolverTest : public DnsImplTest {
+  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.
+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);
+  server_->setRefused(true);
+
+  EXPECT_NE(nullptr,
+            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;
+  // 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);
+
+  // 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,
+                                             DnsResolver::ResolutionStatus::Success,
+                                             {"201.134.56.7"}, {}, absl::nullopt));
+  dispatcher_->run(Event::Dispatcher::RunType::Block);
+  EXPECT_FALSE(peer_->isChannelDirty());
+}
+
 } // namespace Network
 } // namespace Envoy