diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 4b5cdae997692..4fc24fea145d9 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -23,6 +23,7 @@ Minor Behavior Changes be now be disabled in favor of using unsigned payloads with compatible services via the new ``use_unsigned_payload`` filter option (default false). * cluster: added default value of 5 seconds for :ref:`connect_timeout `. +* dns: changed apple resolver implementation to not reuse the UDS to the local DNS daemon. * dns cache: the new :ref:`dns_query_timeout ` option has a default of 5s. See below for more information. * http: disable the integration between :ref:`ExtensionWithMatcher ` and HTTP filters by default to reflects its experimental status. This feature can be enabled by seting diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 0140b28d3a021..ff1c855977b80 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -173,8 +173,7 @@ Network::DnsResolverSharedPtr DispatcherImpl::createDnsResolver( "using TCP for DNS lookups is not possible when using Apple APIs for DNS " "resolution. Apple' API only uses UDP for DNS resolution. Use UDP or disable " "the envoy.restart_features.use_apple_api_for_dns_lookups runtime feature."); - return std::make_shared(*this, api_.randomGenerator(), - api_.rootScope()); + return std::make_shared(*this, api_.rootScope()); } #endif return std::make_shared(*this, resolvers, dns_resolver_options); diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index a991a99b0977c..4d363ddc0bdf6 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -44,104 +44,14 @@ DNSServiceErrorType DnsService::dnsServiceGetAddrInfo(DNSServiceRef* sdRef, DNSS return DNSServiceGetAddrInfo(sdRef, flags, interfaceIndex, protocol, hostname, callBack, context); } -// Parameters of the jittered backoff strategy. -static constexpr std::chrono::milliseconds RetryInitialDelayMilliseconds(30); -static constexpr std::chrono::milliseconds RetryMaxDelayMilliseconds(30000); - -AppleDnsResolverImpl::AppleDnsResolverImpl(Event::Dispatcher& dispatcher, - Random::RandomGenerator& random, - Stats::Scope& root_scope) - : dispatcher_(dispatcher), initialize_failure_timer_(dispatcher.createTimer( - [this]() -> void { initializeMainSdRef(); })), - backoff_strategy_(std::make_unique( - RetryInitialDelayMilliseconds.count(), RetryMaxDelayMilliseconds.count(), random)), - scope_(root_scope.createScope("dns.apple.")), stats_(generateAppleDnsResolverStats(*scope_)) { - ENVOY_LOG(debug, "Constructing DNS resolver"); - initializeMainSdRef(); -} - -AppleDnsResolverImpl::~AppleDnsResolverImpl() { deallocateMainSdRef(); } +AppleDnsResolverImpl::AppleDnsResolverImpl(Event::Dispatcher& dispatcher, Stats::Scope& root_scope) + : dispatcher_(dispatcher), scope_(root_scope.createScope("dns.apple.")), + stats_(generateAppleDnsResolverStats(*scope_)) {} AppleDnsResolverStats AppleDnsResolverImpl::generateAppleDnsResolverStats(Stats::Scope& scope) { return {ALL_APPLE_DNS_RESOLVER_STATS(POOL_COUNTER(scope))}; } -void AppleDnsResolverImpl::deallocateMainSdRef() { - ENVOY_LOG(debug, "DNSServiceRefDeallocate main sd ref"); - // dns_sd.h says: - // If the reference's underlying socket is used in a run loop or select() call, it should - // be removed BEFORE DNSServiceRefDeallocate() is called, as this function closes the - // reference's socket. - sd_ref_event_.reset(); - DnsServiceSingleton::get().dnsServiceRefDeallocate(main_sd_ref_); -} - -void AppleDnsResolverImpl::initializeMainSdRef() { - // This implementation uses a shared connection for three main reasons: - // 1. Efficiency of concurrent resolutions by sharing the same underlying UDS to the DNS - // server. - // 2. An error on a connection to the DNS server is good indication that other connections, - // even if not shared, would not succeed. So it is better to share one connection and - // promptly cancel all outstanding queries, rather than individually wait for all - // connections to error out. - // 3. It follows the precedent set in dns_impl with the c-ares library, for consistency of - // style, performance, and expectations between the two implementations. - // However, using a shared connection brings some complexities detailed in the inline comments - // for kDNSServiceFlagsShareConnection in dns_sd.h, and copied (and edited) in this implementation - // where relevant. - // - // When error occurs while the main_sd_ref_ is initialized, the initialize_failure_timer_ will be - // enabled to retry initialization. Retries can also be triggered via query submission, @see - // AppleDnsResolverImpl::resolve(...) for details. - auto error = DnsServiceSingleton::get().dnsServiceCreateConnection(&main_sd_ref_); - if (error != kDNSServiceErr_NoError) { - stats_.connection_failure_.inc(); - initialize_failure_timer_->enableTimer( - std::chrono::milliseconds(backoff_strategy_->nextBackOffMs())); - return; - } - - auto fd = DnsServiceSingleton::get().dnsServiceRefSockFD(main_sd_ref_); - // According to dns_sd.h: DnsServiceRefSockFD returns "The DNSServiceRef's underlying socket - // descriptor, or -1 on error.". Although it gives no detailed description on when/why this call - // would fail. - if (fd == -1) { - stats_.socket_failure_.inc(); - initialize_failure_timer_->enableTimer( - std::chrono::milliseconds(backoff_strategy_->nextBackOffMs())); - return; - } - - sd_ref_event_ = dispatcher_.createFileEvent( - fd, - // note: Event::FileTriggerType::Level is used here to closely resemble the c-ares - // implementation in dns_impl.cc. - [this](uint32_t events) { onEventCallback(events); }, Event::FileTriggerType::Level, - Event::FileReadyType::Read); - sd_ref_event_->setEnabled(Event::FileReadyType::Read); - - // Disable the failure timer and reset the backoff strategy because the main_sd_ref_ was - // successfully initialized. Note that these actions will be no-ops if the timer was not armed to - // begin with. - initialize_failure_timer_->disableTimer(); - backoff_strategy_->reset(); -} - -void AppleDnsResolverImpl::onEventCallback(uint32_t events) { - ENVOY_LOG(debug, "DNS resolver file event ({})", events); - RELEASE_ASSERT(events & Event::FileReadyType::Read, - fmt::format("invalid FileReadyType event={}", events)); - DNSServiceErrorType error = DnsServiceSingleton::get().dnsServiceProcessResult(main_sd_ref_); - if (error != kDNSServiceErr_NoError) { - ENVOY_LOG(warn, "DNS resolver error ({}) in DNSServiceProcessResult", error); - stats_.processing_failure_.inc(); - // Similar to receiving an error in onDNSServiceGetAddrInfoReply, an error while processing fd - // events indicates that the sd_ref state is broken. - // Therefore, flush queries with_error == true. - flushPendingQueries(true /* with_error */); - } -} - ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, DnsLookupFamily dns_lookup_family, ResolveCb callback) { @@ -162,28 +72,8 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, // Resolution via Apple APIs ENVOY_LOG(trace, "DNS resolver local resolution failed with: {}", e.what()); - // First check that the main_sd_ref is alive by checking if the resolver is currently trying to - // initialize its main_sd_ref. - if (initialize_failure_timer_->enabled()) { - // No queries should be accumulating while the main_sd_ref_ is not alive. Either they were - // flushed when the error that deallocated occurred, or they have all failed in this branch of - // the code synchronously due to continuous inability to initialize the main_sd_ref_. - ASSERT(queries_with_pending_cb_.empty()); - - // Short-circuit the pending retry to initialize the main_sd_ref_ and try now. - initializeMainSdRef(); - - // If the timer is still enabled, that means the initialization failed. Synchronously fail the - // resolution, the callback target should retry. - if (initialize_failure_timer_->enabled()) { - callback(DnsResolver::ResolutionStatus::Failure, {}); - return nullptr; - } - } - - // Proceed with resolution after establishing that the resolver has a live main_sd_ref_. auto pending_resolution = - std::make_unique(*this, callback, dispatcher_, main_sd_ref_, dns_name); + std::make_unique(*this, callback, dispatcher_, dns_name); DNSServiceErrorType error = pending_resolution->dnsServiceGetAddrInfo(dns_lookup_family); if (error != kDNSServiceErr_NoError) { @@ -197,6 +87,12 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, return nullptr; } + // Otherwise, hook up the query's UDS socket to the event loop to process updates. + if (!pending_resolution->dnsServiceRefSockFD()) { + ENVOY_LOG(warn, "DNS resolver error in dnsServiceRefSockFD for {}", dns_name); + return nullptr; + } + pending_resolution->owned_ = true; return pending_resolution.release(); } @@ -209,59 +105,30 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, return nullptr; } -void AppleDnsResolverImpl::addPendingQuery(PendingResolution* query) { - ASSERT(queries_with_pending_cb_.count(query) == 0); - queries_with_pending_cb_.insert(query); -} - -void AppleDnsResolverImpl::removePendingQuery(PendingResolution* query) { - auto erased = queries_with_pending_cb_.erase(query); - ASSERT(erased == 1); -} - -void AppleDnsResolverImpl::flushPendingQueries(const bool with_error) { - ENVOY_LOG(debug, "DNS Resolver flushing {} queries", queries_with_pending_cb_.size()); - for (std::set::iterator it = queries_with_pending_cb_.begin(); - it != queries_with_pending_cb_.end(); ++it) { - auto query = *it; - - ASSERT(query->pending_cb_); - query->callback_(query->pending_cb_->status_, std::move(query->pending_cb_->responses_)); - - if (query->owned_) { - ENVOY_LOG(debug, "Resolution for {} completed (async)", query->dns_name_); - delete *it; - } else { - ENVOY_LOG(debug, "Resolution for {} completed (synchronously)", query->dns_name_); - query->synchronously_completed_ = true; - } - } - - // Purge the contents so no one tries to delete them again. - queries_with_pending_cb_.clear(); - - if (with_error) { - // The main sd ref is destroyed here because a callback with an error is good indication that - // the connection to the DNS server is faulty and needs to be torn down. - // - // Deallocation of the MainSdRef __has__ to happen __after__ flushing queries. Flushing queries - // de-allocates individual refs, so deallocating the main ref ahead would cause deallocation of - // invalid individual refs per dns_sd.h - deallocateMainSdRef(); - initializeMainSdRef(); - } -} +AppleDnsResolverImpl::PendingResolution::PendingResolution(AppleDnsResolverImpl& parent, + ResolveCb callback, + Event::Dispatcher& dispatcher, + const std::string& dns_name) + : parent_(parent), callback_(callback), dispatcher_(dispatcher), dns_name_(dns_name), + pending_cb_({ResolutionStatus::Success, {}}) {} AppleDnsResolverImpl::PendingResolution::~PendingResolution() { ENVOY_LOG(debug, "Destroying PendingResolution for {}", dns_name_); + + // dns_sd.h says: + // If the reference's underlying socket is used in a run loop or select() call, it should + // be removed BEFORE DNSServiceRefDeallocate() is called, as this function closes the + // reference's socket. + sd_ref_event_.reset(); + // It is possible that DNSServiceGetAddrInfo returns a synchronous error, with a NULLed // DNSServiceRef, in AppleDnsResolverImpl::resolve. // Additionally, it is also possible that the query is cancelled before resolution starts, and // thus the DNSServiceRef is null. // Therefore, only deallocate if the ref is not null. - if (individual_sd_ref_) { + if (sd_ref_) { ENVOY_LOG(debug, "DNSServiceRefDeallocate individual sd ref"); - DnsServiceSingleton::get().dnsServiceRefDeallocate(individual_sd_ref_); + DnsServiceSingleton::get().dnsServiceRefDeallocate(sd_ref_); } } @@ -270,94 +137,35 @@ void AppleDnsResolverImpl::PendingResolution::cancel(Network::ActiveDnsQuery::Ca // and recreating the DNS system to maximize the chance of success in following queries. ENVOY_LOG(debug, "Cancelling PendingResolution for {}", dns_name_); ASSERT(owned_); - if (pending_cb_) { - /* (taken and edited from dns_sd.h) - * Canceling operations and kDNSServiceFlagsMoreComing - * Whenever you cancel any operation for which you had deferred [resolution] - * because of a kDNSServiceFlagsMoreComing flag, you should [flush]. This is because, after - * cancelling the operation, you can no longer wait for a callback *without* MoreComing set, to - * tell you [to flush] (the operation has been canceled, so there will be no more callbacks). - * - * [FURTHER] An implication of the collective - * kDNSServiceFlagsMoreComing flag for shared connections is that this - * guideline applies more broadly -- any time you cancel an operation on - * a shared connection, you should perform all deferred updates for all - * operations sharing that connection. This is because the MoreComing flag - * might have been referring to events coming for the operation you canceled, - * which will now not be coming because the operation has been canceled. - */ - // First, get rid of the current query, because if it is canceled, its callback should not be - // executed during the subsequent flush. - parent_.removePendingQuery(this); - // Then, flush all other queries. - parent_.flushPendingQueries(false /* with_error */); - } // Because the query is self-owned, delete now. delete this; } -void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply( - DNSServiceFlags flags, uint32_t interface_index, DNSServiceErrorType error_code, - const char* hostname, const struct sockaddr* address, uint32_t ttl) { - ENVOY_LOG(debug, - "DNS for {} resolved with: flags={}[MoreComing={}, Add={}], interface_index={}, " - "error_code={}, hostname={}", - dns_name_, flags, flags & kDNSServiceFlagsMoreComing ? "yes" : "no", - flags & kDNSServiceFlagsAdd ? "yes" : "no", interface_index, error_code, hostname); - - if (!pending_cb_) { - pending_cb_ = {ResolutionStatus::Success, {}}; - parent_.addPendingQuery(this); - } - - // Generic error handling. - if (error_code != kDNSServiceErr_NoError) { - // TODO(junr03): consider creating stats for known error types (timeout, refused connection, - // etc.). Currently a bit challenging because there is no scope access wired through. Current - // query gets a failure status - - pending_cb_->status_ = ResolutionStatus::Failure; - pending_cb_->responses_.clear(); - - ENVOY_LOG(warn, "[Error path] DNS Resolver flushing queries pending callback"); - parent_.flushPendingQueries(true /* with_error */); - // Note: Nothing can follow this call to flushPendingQueries due to deletion of this - // object upon resolution. - return; +void AppleDnsResolverImpl::PendingResolution::onEventCallback(uint32_t events) { + ENVOY_LOG(debug, "DNS resolver file event ({})", events); + RELEASE_ASSERT(events & Event::FileReadyType::Read, + fmt::format("invalid FileReadyType event={}", events)); + DNSServiceErrorType error = DnsServiceSingleton::get().dnsServiceProcessResult(sd_ref_); + if (error != kDNSServiceErr_NoError) { + ENVOY_LOG(warn, "DNS resolver error ({}) in DNSServiceProcessResult", error); + parent_.stats_.processing_failure_.inc(); + // Similar to receiving an error in onDNSServiceGetAddrInfoReply, an error while processing fd + // events indicates that the sd_ref state is broken. + // Therefore, finish resolving with an error. + pending_cb_.status_ = ResolutionStatus::Failure; + finishResolve(); } +} - // Only add this address to the list if kDNSServiceFlagsAdd is set. Callback targets are only - // additive. - if (flags & kDNSServiceFlagsAdd) { - ASSERT(address, "invalid to add null address"); - auto dns_response = buildDnsResponse(address, ttl); - ENVOY_LOG(debug, "Address to add address={}, ttl={}", - dns_response.address_->ip()->addressAsString(), ttl); - pending_cb_->responses_.push_back(dns_response); - } +void AppleDnsResolverImpl::PendingResolution::finishResolve() { + callback_(pending_cb_.status_, std::move(pending_cb_.responses_)); - if (!(flags & kDNSServiceFlagsMoreComing)) { - /* (taken and edited from dns_sd.h) - * Collective kDNSServiceFlagsMoreComing flag: - * When [DNSServiceGetAddrInfoReply] are invoked using a shared DNSServiceRef, the - * kDNSServiceFlagsMoreComing flag applies collectively to *all* active - * operations sharing the same [main_sd_ref]. If the MoreComing flag is - * set it means that there are more results queued on this parent DNSServiceRef, - * but not necessarily more results for this particular callback function. - * The implication of this for client programmers is that when a callback - * is invoked with the MoreComing flag set, the code should update its - * internal data structures with the new result (as is done above when calling - * parent_.addPendingQuery(this))...Then, later when a callback is eventually invoked with the - * MoreComing flag not set, the code should update *all* [pending queries] related to that - * shared parent DNSServiceRef that need updating (i.e that have had DNSServiceGetAddrInfoReply - * called on them since the last flush), not just the [queries] related to the particular - * callback that happened to be the last one to be invoked. - */ - ENVOY_LOG(debug, "DNS Resolver flushing queries pending callback"); - parent_.flushPendingQueries(false /* with_error */); - // Note: Nothing can follow this call to flushPendingQueries due to deletion of this - // object upon resolution. - return; + if (owned_) { + ENVOY_LOG(debug, "Resolution for {} completed (async)", dns_name_); + delete this; + } else { + ENVOY_LOG(debug, "Resolution for {} completed (synchronously)", dns_name_); + synchronously_completed_ = true; } } @@ -380,8 +188,7 @@ AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrInfo(DnsLookupFamily d // from the cache? // TODO: explore validation via `DNSSEC`? return DnsServiceSingleton::get().dnsServiceGetAddrInfo( - &individual_sd_ref_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, protocol, - dns_name_.c_str(), + &sd_ref_, kDNSServiceFlagsTimeout, 0, protocol, dns_name_.c_str(), /* * About Thread Safety (taken from inline documentation there): * The dns_sd.h API does not presuppose any particular threading model, and consequently @@ -404,6 +211,70 @@ AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrInfo(DnsLookupFamily d this); } +void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply( + DNSServiceFlags flags, uint32_t interface_index, DNSServiceErrorType error_code, + const char* hostname, const struct sockaddr* address, uint32_t ttl) { + ENVOY_LOG(debug, + "DNS for {} resolved with: flags={}[MoreComing={}, Add={}], interface_index={}, " + "error_code={}, hostname={}", + dns_name_, flags, flags & kDNSServiceFlagsMoreComing ? "yes" : "no", + flags & kDNSServiceFlagsAdd ? "yes" : "no", interface_index, error_code, hostname); + + // Generic error handling. + if (error_code != kDNSServiceErr_NoError) { + // TODO(junr03): consider creating stats for known error types (timeout, refused connection, + // etc.). Currently a bit challenging because there is no scope access wired through. Current + // query gets a failure status + + pending_cb_.status_ = ResolutionStatus::Failure; + pending_cb_.responses_.clear(); + + finishResolve(); + // Note: Nothing can follow this call to flushPendingQueries due to deletion of this + // object upon resolution. + return; + } + + // dns_sd.h does not call out behavior where callbacks to DNSServiceGetAddrInfoReply + // would respond without the flag. However, Envoy's API is solely additive. + // Therefore, only add this address to the list if kDNSServiceFlagsAdd is set. + if (flags & kDNSServiceFlagsAdd) { + ASSERT(address, "invalid to add null address"); + auto dns_response = buildDnsResponse(address, ttl); + ENVOY_LOG(debug, "Address to add address={}, ttl={}", + dns_response.address_->ip()->addressAsString(), ttl); + pending_cb_.responses_.push_back(dns_response); + } + + if (!(flags & kDNSServiceFlagsMoreComing)) { + ENVOY_LOG(debug, "DNS Resolver flushing queries pending callback"); + finishResolve(); + // Note: Nothing can follow this call to finishResolve due to deletion of this + // object upon resolution. + return; + } +} + +bool AppleDnsResolverImpl::PendingResolution::dnsServiceRefSockFD() { + auto fd = DnsServiceSingleton::get().dnsServiceRefSockFD(sd_ref_); + // According to dns_sd.h: DnsServiceRefSockFD returns "The DNSServiceRef's underlying socket + // descriptor, or -1 on error.". Although it gives no detailed description on when/why this call + // would fail. + if (fd == -1) { + parent_.stats_.socket_failure_.inc(); + return false; + } + + sd_ref_event_ = dispatcher_.createFileEvent( + fd, + // note: Event::FileTriggerType::Level is used here to closely resemble the c-ares + // implementation in dns_impl.cc. + [this](uint32_t events) { onEventCallback(events); }, Event::FileTriggerType::Level, + Event::FileReadyType::Read); + sd_ref_event_->setEnabled(Event::FileReadyType::Read); + return true; +} + DnsResponse AppleDnsResolverImpl::PendingResolution::buildDnsResponse(const struct sockaddr* address, uint32_t ttl) { diff --git a/source/common/network/apple_dns_impl.h b/source/common/network/apple_dns_impl.h index b4b48a528d2a2..81647737f3741 100644 --- a/source/common/network/apple_dns_impl.h +++ b/source/common/network/apple_dns_impl.h @@ -60,9 +60,8 @@ struct AppleDnsResolverStats { */ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable { public: - AppleDnsResolverImpl(Event::Dispatcher& dispatcher, Random::RandomGenerator& random, - Stats::Scope& root_scope); - ~AppleDnsResolverImpl() override; + AppleDnsResolverImpl(Event::Dispatcher& dispatcher, Stats::Scope& root_scope); + static AppleDnsResolverStats generateAppleDnsResolverStats(Stats::Scope& scope); // Network::DnsResolver @@ -72,33 +71,24 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable pending_cb_{}; + FinalResponse pending_cb_; }; - void initializeMainSdRef(); - void deallocateMainSdRef(); - void onEventCallback(uint32_t events); - void addPendingQuery(PendingResolution* query); - void removePendingQuery(PendingResolution* query); - void flushPendingQueries(const bool with_error); - Event::Dispatcher& dispatcher_; Event::TimerPtr initialize_failure_timer_; BackOffStrategyPtr backoff_strategy_; - DNSServiceRef main_sd_ref_{nullptr}; - Event::FileEventPtr sd_ref_event_; Stats::ScopePtr scope_; AppleDnsResolverStats stats_; - // When using a shared sd ref via DNSServiceCreateConnection, the DNSServiceGetAddrInfoReply - // callback with the kDNSServiceFlagsMoreComing flag might refer to addresses for various - // PendingResolutions. Therefore, the resolver needs to have a container of queries pending - // calling their own callback_s until a DNSServiceGetAddrInfoReply is called with - // kDNSServiceFlagsMoreComing not set or an error status is received in - // DNSServiceGetAddrInfoReply. - std::set queries_with_pending_cb_; }; } // namespace Network diff --git a/test/common/network/apple_dns_impl_test.cc b/test/common/network/apple_dns_impl_test.cc index 5763e4b52e8dc..ff25252c98078 100644 --- a/test/common/network/apple_dns_impl_test.cc +++ b/test/common/network/apple_dns_impl_test.cc @@ -10,7 +10,6 @@ #include "envoy/network/address.h" #include "envoy/network/dns.h" -#include "source/common/common/random_generator.h" #include "source/common/network/address_impl.h" #include "source/common/network/apple_dns_impl.h" #include "source/common/network/utility.h" @@ -45,7 +44,6 @@ class MockDnsService : public Network::DnsService { ~MockDnsService() = default; MOCK_METHOD(void, dnsServiceRefDeallocate, (DNSServiceRef sdRef)); - MOCK_METHOD(DNSServiceErrorType, dnsServiceCreateConnection, (DNSServiceRef * sdRef)); MOCK_METHOD(dnssd_sock_t, dnsServiceRefSockFD, (DNSServiceRef sdRef)); MOCK_METHOD(DNSServiceErrorType, dnsServiceProcessResult, (DNSServiceRef sdRef)); MOCK_METHOD(DNSServiceErrorType, dnsServiceGetAddrInfo, @@ -69,7 +67,8 @@ class AppleDnsImplTest : public testing::Test { ActiveDnsQuery* resolveWithExpectations(const std::string& address, const DnsLookupFamily lookup_family, const DnsResolver::ResolutionStatus expected_status, - const bool expected_results) { + const bool expected_results, + const bool exit_dispatcher = true) { return resolver_->resolve( address, lookup_family, [=](DnsResolver::ResolutionStatus status, std::list&& results) -> void { @@ -84,7 +83,9 @@ class AppleDnsImplTest : public testing::Test { } } } - dispatcher_->exit(); + if (exit_dispatcher) { + dispatcher_->exit(); + } }); } @@ -160,6 +161,29 @@ TEST_F(AppleDnsImplTest, DnsIpAddressVersion) { dispatcher_->run(Event::Dispatcher::RunType::Block); } +// dns_sd is very opaque and does not explicitly call out the state that is kept across queries. +// The following two tests make sure that two consecutive queries for the same domain result in +// successful resolution. This is implicitly testing the behavior of kDNSServiceFlagsAdd across +// queries. +TEST_F(AppleDnsImplTest, DoubleLookup) { + EXPECT_NE(nullptr, resolveWithExpectations("google.com", DnsLookupFamily::V4Only, + DnsResolver::ResolutionStatus::Success, true)); + dispatcher_->run(Event::Dispatcher::RunType::Block); + + EXPECT_NE(nullptr, resolveWithExpectations("google.com", DnsLookupFamily::V4Only, + DnsResolver::ResolutionStatus::Success, true)); + dispatcher_->run(Event::Dispatcher::RunType::Block); +} + +TEST_F(AppleDnsImplTest, DoubleLookupInOneLoop) { + EXPECT_NE(nullptr, resolveWithExpectations("google.com", DnsLookupFamily::V4Only, + DnsResolver::ResolutionStatus::Success, true, false)); + + EXPECT_NE(nullptr, resolveWithExpectations("google.com", DnsLookupFamily::V4Only, + DnsResolver::ResolutionStatus::Success, true)); + dispatcher_->run(Event::Dispatcher::RunType::Block); +} + TEST_F(AppleDnsImplTest, DnsIpAddressVersionInvalid) { EXPECT_NE(nullptr, resolveWithExpectations("invalidDnsName", DnsLookupFamily::Auto, DnsResolver::ResolutionStatus::Failure, false)); @@ -222,28 +246,8 @@ TEST_F(AppleDnsImplTest, LocalResolution) { // error conditions, and callback firing. class AppleDnsImplFakeApiTest : public testing::Test { public: - AppleDnsImplFakeApiTest() : initialize_failure_timer_(new Event::MockTimer) {} - - ~AppleDnsImplFakeApiTest() override { - if (resolver_) { - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)); - } - } - - void createResolver() { - file_event_ = new NiceMock; - - EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Return(initialize_failure_timer_)); - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)) - .WillOnce(DoAll( - WithArgs<0>(Invoke([](DNSServiceRef* ref) -> void { *ref = new _DNSServiceRef_t{}; })), - Return(kDNSServiceErr_NoError))); - EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); - EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) - .WillOnce(DoAll(SaveArg<1>(&file_ready_cb_), Return(file_event_))); - EXPECT_CALL(*initialize_failure_timer_, disableTimer()); - - resolver_ = std::make_unique(dispatcher_, random_, stats_store_); + void SetUp() override { + resolver_ = std::make_unique(dispatcher_, stats_store_); } protected: @@ -252,60 +256,84 @@ class AppleDnsImplFakeApiTest : public testing::Test { Stats::IsolatedStoreImpl stats_store_; std::unique_ptr resolver_{}; NiceMock dispatcher_; - Event::MockTimer* initialize_failure_timer_; NiceMock* file_event_; Event::FileReadyCb file_ready_cb_; - Random::RandomGeneratorImpl random_; }; -TEST_F(AppleDnsImplFakeApiTest, ErrorInConnectionCreation) { - EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Return(initialize_failure_timer_)); - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)).WillOnce(Return(kDNSServiceErr_Unknown)); - EXPECT_CALL(*initialize_failure_timer_, enableTimer(_, _)); +TEST_F(AppleDnsImplFakeApiTest, ErrorInSocketAccess) { + const std::string hostname = "foo.com"; + sockaddr_in addr4; + addr4.sin_family = AF_INET; + EXPECT_EQ(1, inet_pton(AF_INET, "1.2.3.4", &addr4.sin_addr)); + addr4.sin_port = htons(6502); - resolver_ = std::make_unique(dispatcher_, random_, stats_store_); + Network::Address::Ipv4Instance address(&addr4); + DNSServiceGetAddrInfoReply reply_callback; + absl::Notification dns_callback_executed; - EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "dns.apple.connection_failure")->value()); -} + EXPECT_CALL(dns_service_, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, + kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, + StrEq(hostname.c_str()), _, _)) + .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); -TEST_F(AppleDnsImplFakeApiTest, ErrorInSocketAccess) { - EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Return(initialize_failure_timer_)); - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)).WillOnce(Return(kDNSServiceErr_NoError)); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(-1)); - EXPECT_CALL(*initialize_failure_timer_, enableTimer(_, _)); - resolver_ = std::make_unique(dispatcher_, random_, stats_store_); + auto query = + resolver_->resolve(hostname, Network::DnsLookupFamily::Auto, + [&dns_callback_executed](DnsResolver::ResolutionStatus status, + std::list&& response) -> void { + // Status is success because it isn't possible to attach a file event + // error to a specific query. + EXPECT_EQ(DnsResolver::ResolutionStatus::Failure, status); + EXPECT_EQ(0, response.size()); + dns_callback_executed.Notify(); + }); + + EXPECT_EQ(nullptr, query); EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "dns.apple.socket_failure")->value()); } TEST_F(AppleDnsImplFakeApiTest, InvalidFileEvent) { - createResolver(); + file_event_ = new NiceMock; - EXPECT_DEATH(file_ready_cb_(2), "invalid FileReadyType event=2"); -} + const std::string hostname = "foo.com"; + sockaddr_in addr4; + addr4.sin_family = AF_INET; + EXPECT_EQ(1, inet_pton(AF_INET, "1.2.3.4", &addr4.sin_addr)); + addr4.sin_port = htons(6502); -TEST_F(AppleDnsImplFakeApiTest, ErrorInProcessResult) { - createResolver(); + Network::Address::Ipv4Instance address(&addr4); + DNSServiceGetAddrInfoReply reply_callback; + absl::Notification dns_callback_executed; + + EXPECT_CALL(dns_service_, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, + kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, + StrEq(hostname.c_str()), _, _)) + .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); - // Error in processing will cause the connection to the DNS server to be reset. - EXPECT_CALL(dns_service_, dnsServiceProcessResult(_)).WillOnce(Return(kDNSServiceErr_Unknown)); - // Kill the old connection. - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)); - // Create a new one. - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)).WillOnce(Return(kDNSServiceErr_NoError)); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) - .WillOnce(Return(new NiceMock)); - EXPECT_CALL(*initialize_failure_timer_, disableTimer()); + .WillOnce(DoAll(SaveArg<1>(&file_ready_cb_), Return(file_event_))); - file_ready_cb_(Event::FileReadyType::Read); + auto query = + resolver_->resolve(hostname, Network::DnsLookupFamily::Auto, + [&dns_callback_executed](DnsResolver::ResolutionStatus status, + std::list&& response) -> void { + EXPECT_EQ(DnsResolver::ResolutionStatus::Failure, status); + EXPECT_EQ(0, response.size()); + dns_callback_executed.Notify(); + }); - EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "dns.apple.processing_failure")->value()); + EXPECT_NE(nullptr, query); + + EXPECT_DEATH(file_ready_cb_(2), "invalid FileReadyType event=2"); } -TEST_F(AppleDnsImplFakeApiTest, ErrorInProcessResultWithPendingQueries) { - createResolver(); +TEST_F(AppleDnsImplFakeApiTest, ErrorInProcessResult) { + file_event_ = new NiceMock; const std::string hostname = "foo.com"; sockaddr_in addr4; @@ -318,56 +346,37 @@ TEST_F(AppleDnsImplFakeApiTest, ErrorInProcessResultWithPendingQueries) { absl::Notification dns_callback_executed; EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); + + EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); + EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) + .WillOnce(DoAll(SaveArg<1>(&file_ready_cb_), Return(file_event_))); auto query = resolver_->resolve(hostname, Network::DnsLookupFamily::Auto, [&dns_callback_executed](DnsResolver::ResolutionStatus status, std::list&& response) -> void { - // Status is success because it isn't possible to attach a file event - // error to a specific query. - EXPECT_EQ(DnsResolver::ResolutionStatus::Success, status); - EXPECT_EQ(1, response.size()); - EXPECT_EQ("1.2.3.4:0", response.front().address_->asString()); - EXPECT_EQ(std::chrono::seconds(30), response.front().ttl_); + EXPECT_EQ(DnsResolver::ResolutionStatus::Failure, status); + EXPECT_EQ(0, response.size()); dns_callback_executed.Notify(); }); - ASSERT_NE(nullptr, query); - - // Fill the query with one address, and promise more addresses are coming. Meaning the query will - // be pending. - reply_callback(nullptr, kDNSServiceFlagsAdd | kDNSServiceFlagsMoreComing, 0, - kDNSServiceErr_NoError, hostname.c_str(), address.sockAddr(), 30, query); + EXPECT_NE(nullptr, query); + // Error in processing will cause the connection to the DNS server to be reset. EXPECT_CALL(dns_service_, dnsServiceProcessResult(_)).WillOnce(Return(kDNSServiceErr_Unknown)); - // The query's ref is going to be deallocated when the query is destroyed. The main ref is going - // to be deallocated due to the error. - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)).Times(2); - // A new main ref is created on error. - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)).WillOnce(Return(kDNSServiceErr_NoError)); - EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); - EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) - .WillOnce(Return(new NiceMock)); - EXPECT_CALL(*initialize_failure_timer_, disableTimer()); file_ready_cb_(Event::FileReadyType::Read); - dns_callback_executed.WaitForNotification(); + EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "dns.apple.processing_failure")->value()); } TEST_F(AppleDnsImplFakeApiTest, SynchronousErrorInGetAddrInfo) { - createResolver(); - - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, _, _, _, _, _, _)) .WillOnce(Return(kDNSServiceErr_Unknown)); - // The Query's sd ref will be deallocated. - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)); EXPECT_EQ(nullptr, resolver_->resolve("foo.com", Network::DnsLookupFamily::Auto, @@ -378,8 +387,6 @@ TEST_F(AppleDnsImplFakeApiTest, SynchronousErrorInGetAddrInfo) { } TEST_F(AppleDnsImplFakeApiTest, QuerySynchronousCompletion) { - createResolver(); - const std::string hostname = "foo.com"; sockaddr_in addr4; addr4.sin_family = AF_INET; @@ -389,11 +396,8 @@ TEST_F(AppleDnsImplFakeApiTest, QuerySynchronousCompletion) { Network::Address::Ipv4Instance address(&addr4); absl::Notification dns_callback_executed; - // The query's ref is going to be deallocated when the query is destroyed. - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)); - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll( @@ -420,8 +424,6 @@ TEST_F(AppleDnsImplFakeApiTest, QuerySynchronousCompletion) { } TEST_F(AppleDnsImplFakeApiTest, QueryCompletedWithError) { - createResolver(); - const std::string hostname = "foo.com"; sockaddr_in addr4; addr4.sin_family = AF_INET; @@ -431,21 +433,8 @@ TEST_F(AppleDnsImplFakeApiTest, QueryCompletedWithError) { Network::Address::Ipv4Instance address(&addr4); absl::Notification dns_callback_executed; - // The query's ref is going to be deallocated when the query is destroyed. The main ref is going - // to be deallocated due to the error. - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)).Times(2); - // A new main ref is created on error. - EXPECT_CALL(*initialize_failure_timer_, disableTimer()); - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)) - .WillOnce(DoAll( - WithArgs<0>(Invoke([](DNSServiceRef* ref) -> void { *ref = new _DNSServiceRef_t{}; })), - Return(kDNSServiceErr_NoError))); - EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); - EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) - .WillOnce(Return(new NiceMock)); - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll( @@ -469,8 +458,6 @@ TEST_F(AppleDnsImplFakeApiTest, QueryCompletedWithError) { } TEST_F(AppleDnsImplFakeApiTest, MultipleAddresses) { - createResolver(); - const std::string hostname = "foo.com"; sockaddr_in addr4; addr4.sin_family = AF_INET; @@ -487,13 +474,16 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleAddresses) { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); + EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); + EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) + .WillOnce(Return(new NiceMock)); + auto query = resolver_->resolve(hostname, Network::DnsLookupFamily::Auto, [&dns_callback_executed](DnsResolver::ResolutionStatus status, @@ -509,8 +499,6 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleAddresses) { reply_callback(nullptr, kDNSServiceFlagsAdd | kDNSServiceFlagsMoreComing, 0, kDNSServiceErr_NoError, hostname.c_str(), address.sockAddr(), 30, query); - // The query's ref is going to be deallocated when the query is destroyed. - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)); reply_callback(nullptr, kDNSServiceFlagsAdd, 0, kDNSServiceErr_NoError, hostname.c_str(), address2.sockAddr(), 30, query); @@ -518,8 +506,6 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleAddresses) { } TEST_F(AppleDnsImplFakeApiTest, MultipleAddressesSecondOneFails) { - createResolver(); - const std::string hostname = "foo.com"; sockaddr_in addr4; addr4.sin_family = AF_INET; @@ -530,13 +516,16 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleAddressesSecondOneFails) { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); + EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); + EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) + .WillOnce(Return(new NiceMock)); + auto query = resolver_->resolve(hostname, Network::DnsLookupFamily::Auto, [&dns_callback_executed](DnsResolver::ResolutionStatus status, @@ -552,23 +541,12 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleAddressesSecondOneFails) { reply_callback(nullptr, kDNSServiceFlagsAdd | kDNSServiceFlagsMoreComing, 0, kDNSServiceErr_NoError, hostname.c_str(), address.sockAddr(), 30, query); - // The query's ref is going to be deallocated when the query is destroyed. - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)).Times(2); - // A new main ref is created on error. - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)).WillOnce(Return(kDNSServiceErr_NoError)); - EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); - EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) - .WillOnce(Return(new NiceMock)); - EXPECT_CALL(*initialize_failure_timer_, disableTimer()); - reply_callback(nullptr, 0, 0, kDNSServiceErr_Unknown, hostname.c_str(), nullptr, 30, query); dns_callback_executed.WaitForNotification(); } TEST_F(AppleDnsImplFakeApiTest, MultipleQueries) { - createResolver(); - const std::string hostname = "foo.com"; sockaddr_in addr4; addr4.sin_family = AF_INET; @@ -588,13 +566,16 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueries) { absl::Notification dns_callback_executed2; // Start first query. - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); + EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); + EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) + .WillOnce(Return(new NiceMock)); + auto query = resolver_->resolve(hostname, Network::DnsLookupFamily::Auto, [&dns_callback_executed](DnsResolver::ResolutionStatus status, @@ -608,12 +589,15 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueries) { ASSERT_NE(nullptr, query); // Start second query. - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4, StrEq(hostname2.c_str()), _, _)) + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4, + StrEq(hostname2.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback2), Return(kDNSServiceErr_NoError))); + EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); + EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) + .WillOnce(Return(new NiceMock)); + auto query2 = resolver_->resolve(hostname2, Network::DnsLookupFamily::V4Only, [&dns_callback_executed2](DnsResolver::ResolutionStatus status, @@ -628,11 +612,9 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueries) { // Fill the query with one address, and promise more addresses are coming. Meaning the query will // be pending. - reply_callback(nullptr, kDNSServiceFlagsAdd | kDNSServiceFlagsMoreComing, 0, - kDNSServiceErr_NoError, hostname.c_str(), address.sockAddr(), 30, query); + reply_callback(nullptr, kDNSServiceFlagsAdd, 0, kDNSServiceErr_NoError, hostname.c_str(), + address.sockAddr(), 30, query); - // The two query's ref is going to be deallocated when the query is destroyed. - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)).Times(2); reply_callback2(nullptr, kDNSServiceFlagsAdd, 0, kDNSServiceErr_NoError, hostname2.c_str(), address2.sockAddr(), 30, query2); @@ -641,8 +623,6 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueries) { } TEST_F(AppleDnsImplFakeApiTest, MultipleQueriesOneFails) { - createResolver(); - const std::string hostname = "foo.com"; sockaddr_in addr4; addr4.sin_family = AF_INET; @@ -657,13 +637,16 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueriesOneFails) { absl::Notification dns_callback_executed2; // Start first query. - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); + EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); + EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) + .WillOnce(Return(new NiceMock)); + auto query = resolver_->resolve(hostname, Network::DnsLookupFamily::Auto, [&dns_callback_executed](DnsResolver::ResolutionStatus status, @@ -679,12 +662,15 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueriesOneFails) { ASSERT_NE(nullptr, query); // Start second query. - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4, StrEq(hostname2.c_str()), _, _)) + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4, + StrEq(hostname2.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback2), Return(kDNSServiceErr_NoError))); + EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); + EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) + .WillOnce(Return(new NiceMock)); + auto query2 = resolver_->resolve(hostname2, Network::DnsLookupFamily::V4Only, [&dns_callback_executed2](DnsResolver::ResolutionStatus status, @@ -695,19 +681,8 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueriesOneFails) { }); ASSERT_NE(nullptr, query2); - // Fill the query with one address, and promise more addresses are coming. Meaning the query will - // be pending. - reply_callback(nullptr, kDNSServiceFlagsAdd | kDNSServiceFlagsMoreComing, 0, - kDNSServiceErr_NoError, hostname.c_str(), address.sockAddr(), 30, query); - - // The two query's ref is going to be deallocated when the query is destroyed. - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)).Times(3); - // A new main ref is created on error. - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)).WillOnce(Return(kDNSServiceErr_NoError)); - EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); - EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) - .WillOnce(Return(new NiceMock)); - EXPECT_CALL(*initialize_failure_timer_, disableTimer()); + reply_callback(nullptr, kDNSServiceFlagsAdd, 0, kDNSServiceErr_NoError, hostname.c_str(), + address.sockAddr(), 30, query); // The second query fails. reply_callback2(nullptr, 0, 0, kDNSServiceErr_Unknown, hostname2.c_str(), nullptr, 30, query2); @@ -717,8 +692,6 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueriesOneFails) { } TEST_F(AppleDnsImplFakeApiTest, ResultWithOnlyNonAdditiveReplies) { - createResolver(); - const std::string hostname = "foo.com"; sockaddr_in addr4; addr4.sin_family = AF_INET; @@ -728,13 +701,16 @@ TEST_F(AppleDnsImplFakeApiTest, ResultWithOnlyNonAdditiveReplies) { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); + EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); + EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) + .WillOnce(Return(new NiceMock)); + auto query = resolver_->resolve(hostname, Network::DnsLookupFamily::Auto, [&dns_callback_executed](DnsResolver::ResolutionStatus status, @@ -745,8 +721,6 @@ TEST_F(AppleDnsImplFakeApiTest, ResultWithOnlyNonAdditiveReplies) { }); ASSERT_NE(nullptr, query); - // The query's sd ref will be deallocated on completion. - EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)); // Reply _without_ add and _without_ more coming flags. This should cause a flush with an empty // response. reply_callback(nullptr, 0, 0, kDNSServiceErr_NoError, hostname.c_str(), nullptr, 30, query); @@ -754,8 +728,6 @@ TEST_F(AppleDnsImplFakeApiTest, ResultWithOnlyNonAdditiveReplies) { } TEST_F(AppleDnsImplFakeApiTest, ResultWithNullAddress) { - createResolver(); - const std::string hostname = "foo.com"; sockaddr_in addr4; addr4.sin_family = AF_INET; @@ -764,13 +736,16 @@ TEST_F(AppleDnsImplFakeApiTest, ResultWithNullAddress) { Network::Address::Ipv4Instance address(&addr4); DNSServiceGetAddrInfoReply reply_callback; - EXPECT_CALL(*initialize_failure_timer_, enabled()).WillOnce(Return(false)); EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); + EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); + EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) + .WillOnce(Return(new NiceMock)); + auto query = resolver_->resolve( hostname, Network::DnsLookupFamily::Auto, [](DnsResolver::ResolutionStatus, std::list&&) -> void { FAIL(); }); @@ -781,79 +756,51 @@ TEST_F(AppleDnsImplFakeApiTest, ResultWithNullAddress) { "invalid to add null address"); } -TEST_F(AppleDnsImplFakeApiTest, ErrorInConnectionCreationImmediateCallback) { - EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Return(initialize_failure_timer_)); - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)).WillOnce(Return(kDNSServiceErr_Unknown)); - EXPECT_CALL(*initialize_failure_timer_, enableTimer(_, _)); - - resolver_ = std::make_unique(dispatcher_, random_, stats_store_); +TEST_F(AppleDnsImplFakeApiTest, DeallocateOnDestruction) { + const std::string hostname = "foo.com"; + sockaddr_in addr4; + addr4.sin_family = AF_INET; + EXPECT_EQ(1, inet_pton(AF_INET, "1.2.3.4", &addr4.sin_addr)); + addr4.sin_port = htons(6502); + Network::Address::Ipv4Instance address(&addr4); - EXPECT_CALL(*initialize_failure_timer_, enabled()).Times(2).WillRepeatedly(Return(true)); - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)).WillOnce(Return(kDNSServiceErr_NoError)); - EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(-1)); - EXPECT_CALL(*initialize_failure_timer_, enableTimer(_, _)); + sockaddr_in addr4_2; + addr4_2.sin_family = AF_INET; + EXPECT_EQ(1, inet_pton(AF_INET, "5.6.7.8", &addr4_2.sin_addr)); + addr4_2.sin_port = htons(6502); + Network::Address::Ipv4Instance address2(&addr4); + DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_EQ(nullptr, - resolver_->resolve("foo.com", Network::DnsLookupFamily::Auto, - [&dns_callback_executed](DnsResolver::ResolutionStatus status, - std::list&& response) -> void { - EXPECT_EQ(DnsResolver::ResolutionStatus::Failure, status); - EXPECT_TRUE(response.empty()); - dns_callback_executed.Notify(); - })); - dns_callback_executed.WaitForNotification(); -} -TEST_F(AppleDnsImplFakeApiTest, ErrorInConnectionCreationQueryDispatched) { - EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Return(initialize_failure_timer_)); - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)).WillOnce(Return(kDNSServiceErr_Unknown)); - EXPECT_CALL(*initialize_failure_timer_, enableTimer(_, _)); - - resolver_ = std::make_unique(dispatcher_, random_, stats_store_); - - file_event_ = new NiceMock; - EXPECT_CALL(*initialize_failure_timer_, enabled()) - .Times(2) - .WillOnce(Return(true)) - .WillOnce(Return(false)); - EXPECT_CALL(dns_service_, dnsServiceCreateConnection(_)) + EXPECT_CALL(dns_service_, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, + kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll( + SaveArg<5>(&reply_callback), WithArgs<0>(Invoke([](DNSServiceRef* ref) -> void { *ref = new _DNSServiceRef_t{}; })), Return(kDNSServiceErr_NoError))); + EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _)) - .WillOnce(DoAll(SaveArg<1>(&file_ready_cb_), Return(file_event_))); - EXPECT_CALL(*initialize_failure_timer_, disableTimer()); - - absl::Notification dns_callback_executed; - const std::string hostname = "foo.com"; - DNSServiceGetAddrInfoReply reply_callback; - sockaddr_in addr4; - addr4.sin_family = AF_INET; - EXPECT_EQ(1, inet_pton(AF_INET, "1.2.3.4", &addr4.sin_addr)); - addr4.sin_port = htons(6502); - Network::Address::Ipv4Instance address(&addr4); - - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4, StrEq(hostname.c_str()), _, _)) - .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); + .WillOnce(Return(new NiceMock)); auto query = - resolver_->resolve(hostname, Network::DnsLookupFamily::V4Only, + resolver_->resolve(hostname, Network::DnsLookupFamily::Auto, [&dns_callback_executed](DnsResolver::ResolutionStatus status, std::list&& response) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Success, status); EXPECT_EQ(1, response.size()); dns_callback_executed.Notify(); }); - EXPECT_NE(nullptr, query); + ASSERT_NE(nullptr, query); - // The query's sd ref will be deallocated on completion. + // The query's ref is going to be deallocated when the query is destroyed. EXPECT_CALL(dns_service_, dnsServiceRefDeallocate(_)); + reply_callback(nullptr, kDNSServiceFlagsAdd, 0, kDNSServiceErr_NoError, hostname.c_str(), - address.sockAddr(), 30, query); + address2.sockAddr(), 30, query); dns_callback_executed.WaitForNotification(); }