c-ares DnsResolverImpl implementation (issue #143). (#428) (#474)

This PR adds c-ares support to DnsResolverImpl, breaking the dependency
on glibc getaddrinfo_a(). It also modifies the DnsResolver API to
support queries that resolve immediately without any deferral and
introduces some more extensive asynch DNS resolution testing via a
simple loopback test DNS server.

ci/build_setup.sh

@@ -35,6 +35,7 @@ $EXTRA_CMAKE_FLAGS \
 -DENVOY_GTEST_INCLUDE_DIR:FILEPATH=/thirdparty_build/include \
 -DENVOY_HTTP_PARSER_INCLUDE_DIR:FILEPATH=/thirdparty_build/include \
 -DENVOY_LIBEVENT_INCLUDE_DIR:FILEPATH=/thirdparty_build/include \
+-DENVOY_CARES_INCLUDE_DIR:FILEPATH=/thirdparty_build/include \
 -DENVOY_NGHTTP2_INCLUDE_DIR:FILEPATH=/thirdparty_build/include \
 -DENVOY_SPDLOG_INCLUDE_DIR:FILEPATH=/thirdparty/spdlog-0.11.0/include \
 -DENVOY_TCLAP_INCLUDE_DIR:FILEPATH=/thirdparty/tclap-1.2.1/include \

ci/ci_steps.sh

@@ -6,7 +6,7 @@ set -e
 # Lint travis file.
 travis lint .travis.yml --skip-completion-check
 # Do a build matrix with different types of builds docs, coverage, normal, etc.
-docker run -t -i -v $TRAVIS_BUILD_DIR:/source lyft/envoy-build:4c887e005b129b28d815d59f9983b1ed3eb9568f /bin/bash -c "cd /source && ci/do_ci.sh $TEST_TYPE"
+docker run -t -i -v $TRAVIS_BUILD_DIR:/source lyft/envoy-build:5ba9f93b749aaabdc4e6d16f941f9970a80fcf8e /bin/bash -c "cd /source && ci/do_ci.sh $TEST_TYPE"
 
 # The following scripts are only relevant on a `normal` run.
 # This script build a lyft/envoy image an that image is pushed on merge to master.

docs/install/requirements.rst

@@ -21,6 +21,7 @@ Envoy has the following requirements:
 * `protobuf <https://github.com/google/protobuf>`_ (last tested with 3.0.0)
 * `lightstep-tracer-cpp <https://github.com/lightstep/lightstep-tracer-cpp/>`_ (last tested with 0.19)
 * `rapidjson <https://github.com/miloyip/rapidjson/>`_ (last tested with 1.1.0)
+* `c-ares <https://github.com/c-ares/c-ares>`_ (last tested with 1.12.0)
 
 In order to compile and run the tests the following is required:
 

include/envoy/network/dns.h

@@ -36,9 +36,10 @@ class DnsResolver {
    * Initiate an async DNS resolution.
    * @param dns_name supplies the DNS name to lookup.
    * @param callback supplies the callback to invoke when the resolution is complete.
-   * @return a handle that can be used to cancel the resolution.
+   * @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, ResolveCb callback) PURE;
 };
 
 typedef std::unique_ptr<DnsResolver> DnsResolverPtr;

source/common/CMakeLists.txt

@@ -125,6 +125,7 @@ if (NOT ENVOY_SANITIZE)
   include_directories(${ENVOY_GPERFTOOLS_INCLUDE_DIR})
 endif()
 
+include_directories(${ENVOY_CARES_INCLUDE_DIR})
 include_directories(${ENVOY_LIBEVENT_INCLUDE_DIR})
 include_directories(${ENVOY_NGHTTP2_INCLUDE_DIR})
 include_directories(SYSTEM ${ENVOY_OPENSSL_INCLUDE_DIR})

source/common/network/dns_impl.cc

@@ -1,97 +1,127 @@
 #include "dns_impl.h"
 
 #include "common/common/assert.h"
-#include "common/event/libevent.h"
 #include "common/network/address_impl.h"
 #include "common/network/utility.h"
 
-#include "event2/event.h"
+#include "ares.h"
 
 namespace Network {
 
-DnsResolverImpl::DnsResolverImpl(Event::DispatcherImpl& dispatcher) : dispatcher_(dispatcher) {
-  // This sets us up to receive signals on an fd when async DNS resolved are completed.
-  sigset_t mask;
-  sigemptyset(&mask);
-  sigaddset(&mask, Event::Libevent::Global::DNS_SIGNAL_ID);
-  signal_fd_ = signalfd(-1, &mask, SFD_NONBLOCK);
-  RELEASE_ASSERT(-1 != signal_fd_);
-
-  event_assign(&signal_read_event_, &dispatcher_.base(), signal_fd_,
-               EV_READ | EV_PERSIST, [](evutil_socket_t, short, void* arg) -> void {
-                 static_cast<DnsResolverImpl*>(arg)->onSignal();
-               }, this);
-
-  event_add(&signal_read_event_, nullptr);
+DnsResolverImpl::DnsResolverImpl(Event::Dispatcher& dispatcher)
+    : dispatcher_(dispatcher),
+      timer_(dispatcher.createTimer([this] { onEventCallback(ARES_SOCKET_BAD, 0); })) {
+  // This is also done in main(), to satisfy the requirement that c-ares is
+  // initialized prior to threading. The additional call to ares_library_init()
+  // here is a nop in normal execution, but exists for testing where we don't
+  // launch via main().
+  ares_library_init(ARES_LIB_INIT_ALL);
+  ares_options options;
+  initializeChannel(&options, 0);
 }
 
 DnsResolverImpl::~DnsResolverImpl() {
-  close(signal_fd_);
-  event_del(&signal_read_event_);
+  timer_->disableTimer();
+  ares_destroy(channel_);
+  ares_library_cleanup();
 }
 
-void DnsResolverImpl::onSignal() {
-  while (true) {
-    signalfd_siginfo signal_info;
-    ssize_t rc = read(signal_fd_, &signal_info, sizeof(signal_info));
-    if (rc == -1 && errno == EAGAIN) {
-      break;
-    }
-
-    RELEASE_ASSERT(rc == sizeof(signal_info));
-    PendingResolution* pending_resolution =
-        reinterpret_cast<PendingResolution*>(signal_info.ssi_ptr);
+void DnsResolverImpl::initializeChannel(ares_options* options, int optmask) {
+  options->sock_state_cb = [](void* arg, int 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);
+}
 
-    std::list<Address::InstancePtr> address_list;
-    addrinfo* result = pending_resolution->async_cb_data_.ar_result;
-    while (result != nullptr) {
+void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, hostent* hostent) {
+  // We receive ARES_EDESTRUCTION when destructing with pending queries.
+  if (status == ARES_EDESTRUCTION) {
+    ASSERT(owned_);
+    delete this;
+    return;
+  }
+  std::list<Address::InstancePtr> 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: IPv6 support.
-      ASSERT(result->ai_family == AF_INET);
-      sockaddr_in* address = reinterpret_cast<sockaddr_in*>(result->ai_addr);
-      address_list.emplace_back(new Address::Ipv4Instance(address));
-      result = result->ai_next;
+      address.sin_family = AF_INET;
+      address.sin_port = 0;
+      address.sin_addr = *reinterpret_cast<in_addr*>(hostent->h_addr_list[i]);
+      address_list.emplace_back(new Address::Ipv4Instance(&address));
     }
+  }
+  if (!cancelled_) {
+    callback_(std::move(address_list));
+  }
+  if (owned_) {
+    delete this;
+  }
+}
 
-    freeaddrinfo(pending_resolution->async_cb_data_.ar_result);
-    if (!pending_resolution->cancelled_) {
-      // TODO: There is no good way to cancel a DNS request with the terrible getaddrinfo_a() API.
-      //       We just mark it cancelled and ignore raising a callback. In the future when we switch
-      //       this out for a better library we can actually cancel.
-      pending_resolution->callback_(std::move(address_list));
-    }
-    pending_resolution->removeFromList(pending_resolutions_);
+void DnsResolverImpl::updateAresTimer() {
+  // Update the timeout for events.
+  timeval timeout;
+  timeval* timeout_result = ares_timeout(channel_, nullptr, &timeout);
+  if (timeout_result != nullptr) {
+    timer_->enableTimer(
+        std::chrono::milliseconds(timeout_result->tv_sec * 1000 + timeout_result->tv_usec / 1000));
+  } else {
+    timer_->disableTimer();
   }
 }
 
-ActiveDnsQuery& DnsResolverImpl::resolve(const std::string& dns_name, ResolveCb callback) {
-  // This initializes the getaddrinfo_a callback data.
-  PendingResolutionPtr pending_resolution(new PendingResolution());
-  ActiveDnsQuery& ret = *pending_resolution;
-  pending_resolution->host_ = dns_name;
-  pending_resolution->async_cb_data_.ar_name = pending_resolution->host_.c_str();
-  pending_resolution->async_cb_data_.ar_service = nullptr;
-  pending_resolution->async_cb_data_.ar_request = &pending_resolution->hints_;
-  pending_resolution->callback_ = callback;
+void DnsResolverImpl::onEventCallback(int fd, uint32_t events) {
+  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);
+  updateAresTimer();
+}
 
-  // This initializes the hints for the lookup.
-  memset(&pending_resolution->hints_, 0, sizeof(pending_resolution->hints_));
-  pending_resolution->hints_.ai_family = AF_INET;
-  pending_resolution->hints_.ai_socktype = SOCK_STREAM;
+void DnsResolverImpl::onAresSocketStateChange(int fd, int read, int write) {
+  updateAresTimer();
+  auto it = events_.find(fd);
+  // Stop tracking events for fd if no more state change events.
+  if (read == 0 && write == 0) {
+    if (it != events_.end()) {
+      events_.erase(it);
+    }
+    return;
+  }
 
-  // This initializes the async signal data.
-  sigevent signal_info;
-  signal_info.sigev_notify = SIGEV_SIGNAL;
-  signal_info.sigev_signo = Event::Libevent::Global::DNS_SIGNAL_ID;
-  signal_info.sigev_value.sival_ptr = pending_resolution.get();
+  // If we weren't tracking the fd before, create a new FileEvent.
+  if (it == events_.end()) {
+    events_[fd] = dispatcher_.createFileEvent(fd, [this, fd](uint32_t events) {
+      onEventCallback(fd, events);
+    }, Event::FileTriggerType::Level);
+  }
+  events_[fd]->setEnabled((read ? Event::FileReadyType::Read : 0) |
+                          (write ? Event::FileReadyType::Write : 0));
+}
 
-  gaicb* list[1];
-  list[0] = &pending_resolution->async_cb_data_;
-  pending_resolution->moveIntoList(std::move(pending_resolution), pending_resolutions_);
-  int rc = getaddrinfo_a(GAI_NOWAIT, list, 1, &signal_info);
-  RELEASE_ASSERT(0 == rc);
-  UNREFERENCED_PARAMETER(rc);
+ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, ResolveCb callback) {
+  std::unique_ptr<PendingResolution> pending_resolution(new PendingResolution());
+  pending_resolution->callback_ = callback;
 
-  return ret;
+  ares_gethostbyname(channel_, dns_name.c_str(),
+                     AF_INET, [](void* arg, int status, int timeouts, hostent* hostent) {
+                       static_cast<PendingResolution*>(arg)->onAresHostCallback(status, hostent);
+                       UNREFERENCED_PARAMETER(timeouts);
+                     }, pending_resolution.get());
+
+  if (pending_resolution->completed_) {
+    return nullptr;
+  } else {
+    // The PendingResolution will self-delete when the request completes
+    // (including if cancelled or if ~DnsResolverImpl() happens).
+    pending_resolution->owned_ = true;
+    return pending_resolution.release();
+  }
 }
 
 } // Network

source/common/network/dns_impl.h

@@ -1,47 +1,67 @@
 #pragma once
 
+#include "envoy/event/dispatcher.h"
+#include "envoy/event/file_event.h"
 #include "envoy/network/dns.h"
 
 #include "common/common/linked_object.h"
-#include "common/event/dispatcher_impl.h"
 
-#include "event2/event_struct.h"
+#include "ares.h"
 
 namespace Network {
 
+class DnsResolverImplPeer;
+
 /**
- * Implementation of DnsResolver that uses getaddrinfo_a. All calls and callbacks are assumed to
- * happen on the thread that owns the creating dispatcher. Also, since results come in via signal
- * only one of these can exist at a time.
+ * Implementation of DnsResolver that uses c-ares. All calls and callbacks are assumed to
+ * happen on the thread that owns the creating dispatcher.
  */
 class DnsResolverImpl : public DnsResolver {
 public:
-  DnsResolverImpl(Event::DispatcherImpl& dispatcher);
-  ~DnsResolverImpl();
+  DnsResolverImpl(Event::Dispatcher& dispatcher);
+  ~DnsResolverImpl() override;
 
   // Network::DnsResolver
-  ActiveDnsQuery& resolve(const std::string& dns_name, ResolveCb callback) override;
+  ActiveDnsQuery* resolve(const std::string& dns_name, ResolveCb callback) override;
 
 private:
-  struct PendingResolution : LinkedObject<PendingResolution>, public ActiveDnsQuery {
+  friend class DnsResolverImplPeer;
+  struct PendingResolution : public ActiveDnsQuery {
     // Network::ActiveDnsQuery
-    void cancel() override { cancelled_ = true; }
+    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.
+    void onAresHostCallback(int status, hostent* hostent);
 
-    std::string host_;
-    addrinfo hints_;
-    gaicb async_cb_data_;
+    // Caller supplied callback to invoke on query completion or error.
     ResolveCb callback_;
-    bool cancelled_{};
+    // Does the object own itself? Resource reclamation occurs via self-deleting
+    // on query completion or error.
+    bool owned_ = false;
+    // Has the query completed? Only meaningful if !owned_;
+    bool completed_ = false;
+    // Was the query cancelled via cancel()?
+    bool cancelled_ = false;
   };
 
-  typedef std::unique_ptr<PendingResolution> PendingResolutionPtr;
-
-  void onSignal();
+  // Callback for events on sockets tracked in events_.
+  void onEventCallback(int fd, uint32_t events);
+  // c-ares callback when a socket state changes, indicating that libevent
+  // should listen for read/write events.
+  void onAresSocketStateChange(int fd, int read, int write);
+  // Initialize the channel with given ares_init_options().
+  void initializeChannel(ares_options* options, int optmask);
+  // Update timer for c-ares timeouts.
+  void updateAresTimer();
 
-  Event::DispatcherImpl& dispatcher_;
-  int signal_fd_;
-  event signal_read_event_;
-  std::list<PendingResolutionPtr> pending_resolutions_;
+  Event::Dispatcher& dispatcher_;
+  Event::TimerPtr timer_;
+  ares_channel channel_;
+  std::unordered_map<int, Event::FileEventPtr> events_;
 };
 
 } // Network

source/common/upstream/logical_dns_cluster.cc

@@ -44,7 +44,7 @@ void LogicalDnsCluster::startResolve() {
   log_debug("starting async DNS resolution for {}", dns_address);
   info_->stats().update_attempt_.inc();
 
-  active_dns_query_ = &dns_resolver_.resolve(
+  active_dns_query_ = dns_resolver_.resolve(
       dns_address,
       [this, dns_address](std::list<Network::Address::InstancePtr>&& address_list) -> void {
         active_dns_query_ = nullptr;

source/common/upstream/upstream_impl.cc

@@ -406,7 +406,7 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() {
   log_debug("starting async DNS resolution for {}", dns_address_);
   parent_.info_->stats().update_attempt_.inc();
 
-  active_query_ = &parent_.dns_resolver_.resolve(
+  active_query_ = parent_.dns_resolver_.resolve(
       dns_address_, [this](std::list<Network::Address::InstancePtr>&& address_list) -> void {
         active_query_ = nullptr;
         log_debug("async DNS resolution complete for {}", dns_address_);

source/exe/CMakeLists.txt

@@ -11,6 +11,7 @@ target_link_libraries(envoy ssl)
 target_link_libraries(envoy crypto)
 target_link_libraries(envoy nghttp2)
 target_link_libraries(envoy lightstep_core_cxx11)
+target_link_libraries(envoy cares)
 target_link_libraries(envoy protobuf)
 target_link_libraries(envoy pthread)
 target_link_libraries(envoy anl)
@@ -19,6 +20,7 @@ target_link_libraries(envoy dl)
 
 include_directories(SYSTEM ${ENVOY_OPENSSL_INCLUDE_DIR})
 include_directories(SYSTEM ${ENVOY_LIGHTSTEP_TRACER_INCLUDE_DIR})
+include_directories(SYSTEM ${ENVOY_CARES_INCLUDE_DIR})
 
 set_target_properties(envoy PROPERTIES COTIRE_CXX_PREFIX_HEADER_INIT
                       "../precompiled/precompiled.h")

source/exe/main.cc

@@ -10,6 +10,8 @@
 #include "server/server.h"
 #include "server/test_hooks.h"
 
+#include "ares.h"
+
 namespace Server {
 
 class ProdComponentFactory : public ComponentFactory {
@@ -28,6 +30,7 @@ class ProdComponentFactory : public ComponentFactory {
 } // Server
 
 int main(int argc, char** argv) {
+  ares_library_init(ARES_LIB_INIT_ALL);
   Event::Libevent::Global::initialize();
   Ssl::OpenSsl::initialize();
   OptionsImpl options(argc, argv, Server::SharedMemory::version(), spdlog::level::warn);
@@ -49,5 +52,6 @@ int main(int argc, char** argv) {
   Server::InstanceImpl server(options, default_test_hooks, *restarter, stats_store,
                               restarter->accessLogLock(), component_factory, local_info);
   server.run();
+  ares_library_cleanup();
   return 0;
 }