dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED

source/common/network/dns_impl.cc

@@ -24,11 +24,12 @@ DnsResolverImpl::DnsResolverImpl(
     const std::vector<Network::Address::InstanceConstSharedPtr>& 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 +71,22 @@ void DnsResolverImpl::initializeChannel(ares_options* options, int optmask) {
     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);
+  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 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.
+  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_);
@@ -171,6 +183,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 +216,23 @@ 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.
+
+  // @see DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback for why this is done.
+  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<PendingResolution> 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;
   }

source/common/network/dns_impl.h

@@ -39,9 +39,10 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
   friend class DnsResolverImplPeer;
   struct PendingResolution : public ActiveDnsQuery {
     // Network::ActiveDnsQuery
-    PendingResolution(ResolveCb callback, Event::Dispatcher& dispatcher, ares_channel channel,
-                      const std::string& dns_name)
-        : callback_(callback), dispatcher_(dispatcher), channel_(channel), dns_name_(dns_name) {}
+    PendingResolution(DnsResolverImpl& parent, ResolveCb callback, Event::Dispatcher& dispatcher,
+                      ares_channel channel, const std::string& dns_name)
+        : parent_(parent), callback_(callback), dispatcher_(dispatcher), channel_(channel),
+          dns_name_(dns_name) {}
 
     void cancel() override {
       // c-ares only supports channel-wide cancellation, so we just allow the
@@ -62,6 +63,7 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
      */
     void getAddrInfo(int family);
 
+    DnsResolverImpl& parent_;
     // Caller supplied callback to invoke on query completion or error.
     const ResolveCb callback_;
     // Dispatcher to post any callback_ exceptions to.
@@ -93,6 +95,8 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
   Event::Dispatcher& dispatcher_;
   Event::TimerPtr timer_;
   ares_channel channel_;
+  bool dirty_channel_{};
+  bool use_tcp_for_dns_lookups_;
   std::unordered_map<int, Event::FileEventPtr> events_;
 };