From c9f5c7e50159895666e1a44f79ba766c0c718b11 Mon Sep 17 00:00:00 2001 From: htuch Date: Fri, 10 Feb 2017 16:39:05 -0500 Subject: [PATCH] c-ares DnsResolverImpl implementation (issue #143). (#428) 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. --- .travis.yml | 2 +- ci/build_setup.sh | 1 + docs/install/requirements.rst | 1 + include/envoy/network/dns.h | 5 +- source/common/CMakeLists.txt | 1 + source/common/network/dns_impl.cc | 168 +++++---- source/common/network/dns_impl.h | 62 ++-- source/common/upstream/logical_dns_cluster.cc | 2 +- source/common/upstream/upstream_impl.cc | 2 +- source/exe/CMakeLists.txt | 1 + source/exe/main.cc | 4 + test/CMakeLists.txt | 2 + test/common/network/dns_impl_test.cc | 340 ++++++++++++++++-- .../upstream/cluster_manager_impl_test.cc | 2 +- .../upstream/logical_dns_cluster_test.cc | 4 +- test/common/upstream/upstream_impl_test.cc | 4 +- test/mocks/network/mocks.cc | 2 +- test/mocks/network/mocks.h | 2 +- thirdparty.cmake | 4 + 19 files changed, 473 insertions(+), 136 deletions(-) diff --git a/.travis.yml b/.travis.yml index 661bbdd96812..39b781fabdaf 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,6 +14,6 @@ env: - TEST_TYPE=docs script: - travis lint .travis.yml --skip-completion-check - - 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" - ./ci/docker_push.sh - ./ci/verify_examples.sh diff --git a/ci/build_setup.sh b/ci/build_setup.sh index a6710bf0207c..e11043a84756 100755 --- a/ci/build_setup.sh +++ b/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 \ diff --git a/docs/install/requirements.rst b/docs/install/requirements.rst index 68209d5c148a..5dad636cf0d0 100644 --- a/docs/install/requirements.rst +++ b/docs/install/requirements.rst @@ -21,6 +21,7 @@ Envoy has the following requirements: * `protobuf `_ (last tested with 3.0.0) * `lightstep-tracer-cpp `_ (last tested with 0.19) * `rapidjson `_ (last tested with 1.1.0) +* `c-ares `_ (last tested with 1.12.0) In order to compile and run the tests the following is required: diff --git a/include/envoy/network/dns.h b/include/envoy/network/dns.h index 3da0a84ad595..74327df544ab 100644 --- a/include/envoy/network/dns.h +++ b/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 DnsResolverPtr; diff --git a/source/common/CMakeLists.txt b/source/common/CMakeLists.txt index c203e8820113..28db7d968dfd 100644 --- a/source/common/CMakeLists.txt +++ b/source/common/CMakeLists.txt @@ -124,6 +124,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}) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 00cd7f2729d4..63126bc762a9 100644 --- a/source/common/network/dns_impl.cc +++ b/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(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(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(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_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_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(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(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 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(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 diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index f187560aad29..1863d5481fa8 100644 --- a/source/common/network/dns_impl.h +++ b/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, 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 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 pending_resolutions_; + Event::Dispatcher& dispatcher_; + Event::TimerPtr timer_; + ares_channel channel_; + std::unordered_map events_; }; } // Network diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index 100a19f54216..4d3323a46ba8 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -41,7 +41,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&& address_list) -> void { active_dns_query_ = nullptr; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 7e176e90cf1f..8571ddfbb872 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -403,7 +403,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&& address_list) -> void { active_query_ = nullptr; log_debug("async DNS resolution complete for {}", dns_address_); diff --git a/source/exe/CMakeLists.txt b/source/exe/CMakeLists.txt index 6d84bde9aa11..04ced0fef305 100644 --- a/source/exe/CMakeLists.txt +++ b/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) diff --git a/source/exe/main.cc b/source/exe/main.cc index feb62cd5df60..edc3c40ab15a 100644 --- a/source/exe/main.cc +++ b/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; } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 79dbbdc2a943..f22f12029a8d 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -19,6 +19,7 @@ include_directories(${PROJECT_SOURCE_DIR}) include_directories(${PROJECT_BINARY_DIR}) include_directories(SYSTEM ${ENVOY_OPENSSL_INCLUDE_DIR}) include_directories(${ENVOY_NGHTTP2_INCLUDE_DIR}) +include_directories(${ENVOY_CARES_INCLUDE_DIR}) include_directories(SYSTEM ${ENVOY_LIGHTSTEP_TRACER_INCLUDE_DIR}) include_directories(${ENVOY_LIBEVENT_INCLUDE_DIR}) @@ -168,6 +169,7 @@ target_link_libraries(envoy-test ssl) target_link_libraries(envoy-test crypto) target_link_libraries(envoy-test nghttp2) target_link_libraries(envoy-test lightstep_core_cxx11) +target_link_libraries(envoy-test cares) target_link_libraries(envoy-test protobuf) target_link_libraries(envoy-test gmock) target_link_libraries(envoy-test pthread) diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 79adeebbca9c..c956012b3c08 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -1,10 +1,218 @@ #include "envoy/event/dispatcher.h" #include "envoy/network/dns.h" -#include "common/api/api_impl.h" +#include "common/buffer/buffer_impl.h" +#include "common/event/dispatcher_impl.h" +#include "common/network/dns_impl.h" +#include "common/network/filter_impl.h" +#include "common/network/listen_socket_impl.h" +#include "common/stats/stats_impl.h" + +#include "test/mocks/network/mocks.h" + +#include +#include + +#include "ares.h" +#include "ares_dns.h" namespace Network { +namespace { + +// List of IP address (in human readable format). +typedef std::list IpList; +// Map from hostname to IpList. +typedef std::unordered_map HostMap; + +// Represents a single TestDnsServer query state and lifecycle. This implements +// just enough of RFC 1035 to handle queries we generate in the tests below. +class TestDnsServerQuery { +public: + TestDnsServerQuery(ConnectionPtr connection, const HostMap& hosts) + : connection_(std::move(connection)), hosts_(hosts) { + connection_->addReadFilter(Network::ReadFilterPtr{new ReadFilter(*this)}); + } + +private: + struct ReadFilter : public Network::ReadFilterBaseImpl { + ReadFilter(TestDnsServerQuery& parent) : parent_(parent) {} + + // Network::ReadFilter + Network::FilterStatus onData(Buffer::Instance& data) override { + onDataInternal(data); + return Network::FilterStatus::StopIteration; + } + + // Hack: void returning variation of onData to allow gtest assertions. + void onDataInternal(Buffer::Instance& data) { + buffer_.add(data); + while (true) { + if (size_ == 0) { + uint16_t size_n; + if (buffer_.length() < sizeof(size_n)) { + // If we don't have enough bytes to determine size, wait until we do. + return; + } + size_n = *static_cast(buffer_.linearize(sizeof(size_n))); + buffer_.drain(sizeof(size_n)); + size_ = ntohs(size_n); + } + + if (buffer_.length() < size_) { + // If we don't have enough bytes to read the complete query, wait until + // we do. + return; + } + + // Expect requests to be small, so stack allocation is fine for test code. + unsigned char* request = static_cast(buffer_.linearize(size_)); + // Only expecting a single question. + ASSERT_EQ(1, DNS_HEADER_QDCOUNT(request)); + // Decode the question and perform lookup. + const unsigned char* question = request + HFIXEDSZ; + // The number of bytes the encoded question name takes up in the request. + // Useful in the response when generating resource records containing the + // name. + long question_len; + char* name; + ASSERT_EQ(ARES_SUCCESS, ares_expand_name(question, request, size_, &name, &question_len)); + auto it = parent_.hosts_.find(name); + const std::list* ips = nullptr; + if (it != parent_.hosts_.end()) { + ips = &it->second; + } + ares_free_string(name); + + // The response begins with the intial part of the request + // (including the question section). + const size_t response_base_len = HFIXEDSZ + question_len + QFIXEDSZ; + unsigned char response_base[response_base_len]; + 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, ips != nullptr ? NOERROR : NXDOMAIN); + DNS_HEADER_SET_ANCOUNT(response_base, ips != nullptr ? ips->size() : 0); + DNS_HEADER_SET_NSCOUNT(response_base, 0); + DNS_HEADER_SET_ARCOUNT(response_base, 0); + + // An A resource record for each IP found in the host map. + const size_t response_rest_len = + ips != nullptr ? ips->size() * (question_len + RRFIXEDSZ + sizeof(in_addr)) : 0; + unsigned char response_rr_fixed[RRFIXEDSZ]; + DNS_RR_SET_TYPE(response_rr_fixed, T_A); + DNS_RR_SET_CLASS(response_rr_fixed, C_IN); + DNS_RR_SET_TTL(response_rr_fixed, 0); + DNS_RR_SET_LEN(response_rr_fixed, sizeof(in_addr)); + + // Send response to client. + const uint16_t response_size_n = htons(response_base_len + response_rest_len); + Buffer::OwnedImpl write_buffer_; + write_buffer_.add(&response_size_n, sizeof(response_size_n)); + write_buffer_.add(response_base, response_base_len); + if (ips != nullptr) { + for (auto it : *ips) { + write_buffer_.add(question, question_len); + write_buffer_.add(response_rr_fixed, 10); + in_addr addr; + ASSERT_EQ(1, inet_pton(AF_INET, it.c_str(), &addr)); + write_buffer_.add(&addr, sizeof(addr)); + } + } + parent_.connection_->write(write_buffer_); + + // Reset query state, time for the next one. + buffer_.drain(size_); + size_ = 0; + } + return; + } + + TestDnsServerQuery& parent_; + // The expected size of the current DNS query to read. If zero, indicates that + // no DNS query is in progress and that a 2 byte size is expected from the + // client to indicate the next DNS query size. + uint16_t size_ = 0; + Buffer::OwnedImpl buffer_; + }; + +private: + ConnectionPtr connection_; + const HostMap& hosts_; +}; + +class TestDnsServer : public ListenerCallbacks { +public: + void onNewConnection(ConnectionPtr&& new_connection) override { + TestDnsServerQuery* query = new TestDnsServerQuery(std::move(new_connection), hosts_); + queries_.emplace_back(query); + } + + void addHosts(const std::string& hostname, const IpList& ip) { hosts_[hostname] = ip; } + +private: + HostMap hosts_; + // All queries are tracked so we can do resource reclamation when the test is + // over. + std::vector> queries_; +}; +} // namespace + +class DnsResolverImplPeer { +public: + DnsResolverImplPeer(DnsResolverImpl* resolver) : resolver_(resolver) {} + ares_channel channel() const { return resolver_->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). + void resetChannelTcpOnly(bool zero_timeout) { + ares_destroy(resolver_->channel_); + ares_options options; + // TCP-only connections to TestDnsServer, since even loopback UDP can be + // lossy with a server under load. + options.flags = ARES_FLAG_USEVC; + // Avoid host-specific domain search behavior when testing to improve + // determinism. + options.ndomains = 0; + options.timeout = 0; + resolver_->initializeChannel(&options, ARES_OPT_FLAGS | ARES_OPT_DOMAINS | + (zero_timeout ? ARES_OPT_TIMEOUTMS : 0)); + } + +private: + DnsResolverImpl* resolver_; +}; + +class DnsImplTest : public testing::Test { +public: + void SetUp() override { + resolver_ = dispatcher_.createDnsResolver(); + + // Point c-ares at 127.0.0.1:10000 with no search domains and TCP-only. + peer_.reset(new DnsResolverImplPeer(dynamic_cast(resolver_.get()))); + peer_->resetChannelTcpOnly(zero_timeout()); + ares_set_servers_ports_csv(peer_->channel(), "127.0.0.1:10000"); + + // Instantiate TestDnsServer and listen on 127.0.0.1:10000. + server_.reset(new TestDnsServer()); + socket_.reset(new Network::TcpListenSocket(uint32_t(10000), true)); + listener_ = dispatcher_.createListener(connection_handler_, *socket_, *server_, stats_store_, + true, false, false); + } + +protected: + // Should the DnsResolverImpl use a zero timeout for c-ares queries? + virtual bool zero_timeout() const { return false; } + std::unique_ptr server_; + std::unique_ptr peer_; + Network::MockConnectionHandler connection_handler_; + Network::TcpListenSocketPtr socket_; + Stats::IsolatedStoreImpl stats_store_; + std::unique_ptr listener_; + Event::DispatcherImpl dispatcher_; + DnsResolverPtr resolver_; +}; + static bool hasAddress(const std::list& results, const std::string& address) { for (auto result : results) { if (result->ip()->addressAsString() == address) { @@ -14,52 +222,116 @@ static bool hasAddress(const std::list& results, const std return false; } -TEST(DnsImplTest, LocalAsyncLookup) { - Api::Impl api(std::chrono::milliseconds(10000)); - Event::DispatcherPtr dispatcher = api.allocateDispatcher(); - DnsResolverPtr resolver = dispatcher->createDnsResolver(); +// 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 +// destruction. +TEST_F(DnsImplTest, DestructPending) { + EXPECT_NE(nullptr, resolver_->resolve("", [&](std::list&& results) -> void { + FAIL(); + UNREFERENCED_PARAMETER(results); + })); + // Also validate that pending events are around to exercise the resource + // reclamation path. + EXPECT_GT(peer_->events().size(), 0U); +} +// Validate basic success/fail lookup behavior. The empty request will connect +// to TestDnsServer, but localhost should resolve via the hosts file with no +// asynchronous behavior or network events. +TEST_F(DnsImplTest, LocalLookup) { std::list address_list; - resolver->resolve("", [&](std::list&& results) -> void { + EXPECT_NE(nullptr, resolver_->resolve("", [&](std::list&& results) -> void { address_list = results; - dispatcher->exit(); - }); + dispatcher_.exit(); + })); - dispatcher->run(Event::Dispatcher::RunType::Block); + dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(address_list.empty()); - resolver->resolve("localhost", [&](std::list&& results) -> void { - address_list = results; - dispatcher->exit(); - }); - - dispatcher->run(Event::Dispatcher::RunType::Block); + EXPECT_EQ(nullptr, resolver_->resolve("localhost", [&](std::list&& results) + -> void { address_list = results; })); EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); } -TEST(DnsImplTest, Cancel) { - Api::Impl api(std::chrono::milliseconds(10000)); - Event::DispatcherPtr dispatcher = api.allocateDispatcher(); - DnsResolverPtr resolver = dispatcher->createDnsResolver(); - Event::TimerPtr stop_timer = dispatcher->createTimer([&]() -> void { - // TODO: This is an absurd hack, but right now the DNS resolver uses signalfd, which means - // that we can get delivery when a new resolver comes up later in the test. We will - // get rid of all of this when we switch this out for c-ares. - dispatcher->exit(); - }); +// Validate success/fail lookup behavior via TestDnsServer. This exercises the +// network event handling in DnsResolverImpl. +TEST_F(DnsImplTest, RemoteAsyncLookup) { + server_->addHosts("some.good.domain", {"201.134.56.7"}); + std::list address_list; + EXPECT_NE(nullptr, resolver_->resolve("some.bad.domain", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(address_list.empty()); - ActiveDnsQuery& query = - resolver->resolve("localhost", [](std::list && ) -> void { FAIL(); }); + EXPECT_NE(nullptr, resolver_->resolve("some.good.domain", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); +} + +// Validate that multiple A records are correctly passed to the callback. +TEST_F(DnsImplTest, MultiARecordLookup) { + server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}); std::list address_list; - resolver->resolve("localhost", [&](std::list&& results) -> void { - address_list = results; - stop_timer->enableTimer(std::chrono::milliseconds(250)); - }); + EXPECT_NE(nullptr, resolver_->resolve("some.good.domain", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); - query.cancel(); - dispatcher->run(Event::Dispatcher::RunType::Block); - EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); + EXPECT_TRUE(hasAddress(address_list, "123.4.5.6")); + EXPECT_TRUE(hasAddress(address_list, "6.5.4.3")); +} + +// Validate working of cancellation provided by ActiveDnsQuery return. +TEST_F(DnsImplTest, Cancel) { + server_->addHosts("some.good.domain", {"201.134.56.7"}); + + ActiveDnsQuery* query = resolver_->resolve("some.domain", [](std::list && ) + -> void { FAIL(); }); + + std::list address_list; + EXPECT_NE(nullptr, resolver_->resolve("some.good.domain", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + ASSERT_NE(nullptr, query); + query->cancel(); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); +} + +class DnsImplZeroTimeoutTest : public DnsImplTest { +protected: + bool zero_timeout() const override { return true; } +}; + +// Validate that timeouts result in an empty callback. +TEST_F(DnsImplZeroTimeoutTest, Timeout) { + server_->addHosts("some.good.domain", {"201.134.56.7"}); + std::list address_list; + EXPECT_NE(nullptr, resolver_->resolve("some.good.domain", + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + EXPECT_TRUE(address_list.empty()); } } // Network diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 0088c8637c18..351d273999d5 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -584,7 +584,7 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemove) { Event::MockTimer* dns_timer_ = new NiceMock(&factory_.dispatcher_); Network::MockActiveDnsQuery active_dns_query; EXPECT_CALL(factory_.dns_resolver_, resolve(_, _)) - .WillRepeatedly(DoAll(SaveArg<1>(&dns_callback), ReturnRef(active_dns_query))); + .WillRepeatedly(DoAll(SaveArg<1>(&dns_callback), Return(&active_dns_query))); create(*loader); // Test for no hosts returning the correct values before we have hosts. diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index 1f5e6929f17f..579f0b32e6f4 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -29,9 +29,9 @@ class LogicalDnsClusterTest : public testing::Test { void expectResolve() { EXPECT_CALL(dns_resolver_, resolve("foo.bar.com", _)) .WillOnce(Invoke([&](const std::string&, Network::DnsResolver::ResolveCb cb) - -> Network::ActiveDnsQuery& { + -> Network::ActiveDnsQuery* { dns_callback_ = cb; - return active_dns_query_; + return &active_dns_query_; })); } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index b0c684ae0728..35576f947754 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -38,9 +38,9 @@ struct ResolverData { void expectResolve(Network::MockDnsResolver& dns_resolver) { EXPECT_CALL(dns_resolver, resolve(_, _)) .WillOnce(Invoke([&](const std::string&, Network::DnsResolver::ResolveCb cb) - -> Network::ActiveDnsQuery& { + -> Network::ActiveDnsQuery* { dns_callback_ = cb; - return active_dns_query_; + return &active_dns_query_; })) .RetiresOnSaturation(); } diff --git a/test/mocks/network/mocks.cc b/test/mocks/network/mocks.cc index b85495566bdc..bedef03218be 100644 --- a/test/mocks/network/mocks.cc +++ b/test/mocks/network/mocks.cc @@ -68,7 +68,7 @@ MockActiveDnsQuery::MockActiveDnsQuery() {} MockActiveDnsQuery::~MockActiveDnsQuery() {} MockDnsResolver::MockDnsResolver() { - ON_CALL(*this, resolve(_, _)).WillByDefault(ReturnRef(active_query_)); + ON_CALL(*this, resolve(_, _)).WillByDefault(Return(&active_query_)); } MockDnsResolver::~MockDnsResolver() {} diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 6fdae7cd43b2..c8ef7dab40ef 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -107,7 +107,7 @@ class MockDnsResolver : public DnsResolver { ~MockDnsResolver(); // Network::DnsResolver - MOCK_METHOD2(resolve, ActiveDnsQuery&(const std::string& dns_name, ResolveCb callback)); + MOCK_METHOD2(resolve, ActiveDnsQuery*(const std::string& dns_name, ResolveCb callback)); testing::NiceMock active_query_; }; diff --git a/thirdparty.cmake b/thirdparty.cmake index 0d9dc3c5c689..8bd88e0e5177 100644 --- a/thirdparty.cmake +++ b/thirdparty.cmake @@ -34,6 +34,10 @@ set(ENVOY_GPERFTOOLS_INCLUDE_DIR "" CACHE FILEPATH "location of gperftools inclu # Last tested with sha b87c80300647c2c0311c1489a104470e099f1531 set(ENVOY_OPENSSL_INCLUDE_DIR "" CACHE FILEPATH "location of openssl includes") +# https://github.com/c-ares/c-ares +# Last tested with 1.12.0 +set(ENVOY_CARES_INCLUDE_DIR "" CACHE FILEPATH "location of c-ares includes") + # https://github.com/google/protobuf # Last tested with 3.0.0 set(ENVOY_PROTOBUF_INCLUDE_DIR "" CACHE FILEPATH "location of protobuf includes")