From baef43e44d44ff87667e4baeb708d2ceb67bb106 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Wed, 30 Jun 2021 21:09:32 +0000 Subject: [PATCH 1/2] dns cache: add DNS query timeout option Signed-off-by: Matt Klein --- .../dynamic_forward_proxy/v3/dns_cache.proto | 8 +- .../v4alpha/dns_cache.proto | 8 +- docs/root/version_history/current.rst | 2 + .../dynamic_forward_proxy/v3/dns_cache.proto | 8 +- .../v4alpha/dns_cache.proto | 8 +- .../dynamic_forward_proxy/dns_cache_impl.cc | 45 +++++++---- .../dynamic_forward_proxy/dns_cache_impl.h | 8 +- .../dns_cache_impl_test.cc | 74 +++++++++++++++++++ 8 files changed, 143 insertions(+), 18 deletions(-) diff --git a/api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto b/api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto index 5c35e80d591fd..b1b1550874fd9 100644 --- a/api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto +++ b/api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto @@ -30,7 +30,7 @@ message DnsCacheCircuitBreakers { // Configuration for the dynamic forward proxy DNS cache. See the :ref:`architecture overview // ` for more information. -// [#next-free-field: 11] +// [#next-free-field: 12] message DnsCacheConfig { option (udpa.annotations.versioning).previous_message_type = "envoy.config.common.dynamic_forward_proxy.v2alpha.DnsCacheConfig"; @@ -114,4 +114,10 @@ message DnsCacheConfig { // performance improvement, in the form of cache hits, for hostnames that are going to be // resolved during steady state and are known at config load time. repeated config.core.v3.SocketAddress preresolve_hostnames = 10; + + // The timeout used for DNS queries. This timeout is independent of any timeout and retry policy + // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque. + // Setting this timeout will ensure that queries succeed or fail within the specified time frame + // and are then retried using the standard refresh rates. Defaults to 5s if not set. + google.protobuf.Duration dns_query_timeout = 11; } diff --git a/api/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto b/api/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto index 08b78d3fa45ca..89cf5e0362949 100644 --- a/api/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto +++ b/api/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto @@ -32,7 +32,7 @@ message DnsCacheCircuitBreakers { // Configuration for the dynamic forward proxy DNS cache. See the :ref:`architecture overview // ` for more information. -// [#next-free-field: 11] +// [#next-free-field: 12] message DnsCacheConfig { option (udpa.annotations.versioning).previous_message_type = "envoy.extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig"; @@ -111,4 +111,10 @@ message DnsCacheConfig { // performance improvement, in the form of cache hits, for hostnames that are going to be // resolved during steady state and are known at config load time. repeated config.core.v4alpha.SocketAddress preresolve_hostnames = 10; + + // The timeout used for DNS queries. This timeout is independent of any timeout and retry policy + // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque. + // Setting this timeout will ensure that queries succeed or fail within the specified time frame + // and are then retried using the standard refresh rates. Defaults to 5s if not set. + google.protobuf.Duration dns_query_timeout = 11; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 099b8b61817fb..fee3c0bca9c37 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 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 ``envoy.reloadable_features.experimental_matching_api`` to true. @@ -89,6 +90,7 @@ New Features * connection_limit: added new :ref:`Network connection limit filter `. * crash support: restore crash context when continuing to processing requests or responses as a result of an asynchronous callback that invokes a filter directly. This is unlike the call stacks that go through the various network layers, to eventually reach the filter. For a concrete example see: ``Envoy::Extensions::HttpFilters::Cache::CacheFilter::getHeaders`` which posts a callback on the dispatcher that will invoke the filter directly. * dns cache: added :ref:`preresolve_hostnames ` option to the DNS cache config. This option allows hostnames to be preresolved into the cache upon cache creation. This might provide performance improvement, in the form of cache hits, for hostnames that are going to be resolved during steady state and are known at config load time. +* dns cache: added :ref:`dns_query_timeout ` option to the DNS cache config. This option allows explicitly controlling the timeout of underlying queries independently of the underlying DNS platform implementation. Coupled with success and failure retry policies the use of this timeout will lead to more deterministic DNS resolution times. * dns resolver: added ``DnsResolverOptions`` protobuf message to reconcile all of the DNS lookup option flags. By setting the configuration option :ref:`use_tcp_for_dns_lookups ` as true we can make the underlying dns resolver library to make only TCP queries to the DNS servers and by setting the configuration option :ref:`no_default_search_domain ` as true the DNS resolver library will not use the default search domains. * dns resolver: added ``DnsResolutionConfig`` to combine :ref:`dns_resolver_options ` and :ref:`resolvers ` in a single protobuf message. The field ``resolvers`` can be specified with a list of DNS resolver addresses. If specified, DNS client library will perform resolution via the underlying DNS resolvers. Otherwise, the default system resolvers (e.g., /etc/resolv.conf) will be used. * dns_filter: added :ref:`dns_resolution_config ` to aggregate all of the DNS resolver configuration in a single message. By setting the configuration option ``use_tcp_for_dns_lookups`` to true we can make dns filter's external resolvers to answer queries using TCP only, by setting the configuration option ``no_default_search_domain`` as true the DNS resolver will not use the default search domains. And by setting the configuration ``resolvers`` we can specify the external DNS servers to be used for external DNS query which replaces the pre-existing alpha api field ``upstream_resolvers``. diff --git a/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto b/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto index 5c35e80d591fd..b1b1550874fd9 100644 --- a/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto +++ b/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto @@ -30,7 +30,7 @@ message DnsCacheCircuitBreakers { // Configuration for the dynamic forward proxy DNS cache. See the :ref:`architecture overview // ` for more information. -// [#next-free-field: 11] +// [#next-free-field: 12] message DnsCacheConfig { option (udpa.annotations.versioning).previous_message_type = "envoy.config.common.dynamic_forward_proxy.v2alpha.DnsCacheConfig"; @@ -114,4 +114,10 @@ message DnsCacheConfig { // performance improvement, in the form of cache hits, for hostnames that are going to be // resolved during steady state and are known at config load time. repeated config.core.v3.SocketAddress preresolve_hostnames = 10; + + // The timeout used for DNS queries. This timeout is independent of any timeout and retry policy + // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque. + // Setting this timeout will ensure that queries succeed or fail within the specified time frame + // and are then retried using the standard refresh rates. Defaults to 5s if not set. + google.protobuf.Duration dns_query_timeout = 11; } diff --git a/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto b/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto index dde756a1608a2..72e75d39b4956 100644 --- a/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto +++ b/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto @@ -33,7 +33,7 @@ message DnsCacheCircuitBreakers { // Configuration for the dynamic forward proxy DNS cache. See the :ref:`architecture overview // ` for more information. -// [#next-free-field: 11] +// [#next-free-field: 12] message DnsCacheConfig { option (udpa.annotations.versioning).previous_message_type = "envoy.extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig"; @@ -117,4 +117,10 @@ message DnsCacheConfig { // performance improvement, in the form of cache hits, for hostnames that are going to be // resolved during steady state and are known at config load time. repeated config.core.v4alpha.SocketAddress preresolve_hostnames = 10; + + // The timeout used for DNS queries. This timeout is independent of any timeout and retry policy + // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque. + // Setting this timeout will ensure that queries succeed or fail within the specified time frame + // and are then retried using the standard refresh rates. Defaults to 5s if not set. + google.protobuf.Duration dns_query_timeout = 11; } diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc index 312c920c27bb6..9cde22b39ee5a 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc @@ -26,6 +26,7 @@ DnsCacheImpl::DnsCacheImpl( stats_(generateDnsCacheStats(*scope_)), resource_manager_(*scope_, loader, config.name(), config.dns_cache_circuit_breaker()), refresh_interval_(PROTOBUF_GET_MS_OR_DEFAULT(config, dns_refresh_rate, 60000)), + timeout_interval_(PROTOBUF_GET_MS_OR_DEFAULT(config, dns_query_timeout, 5000)), failure_backoff_strategy_( Config::Utility::prepareDnsRefreshStrategy< envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig>( @@ -200,13 +201,31 @@ void DnsCacheImpl::startCacheLoad(const std::string& host, uint16_t default_port *this, std::string(host_attributes.host_), host_attributes.port_.value_or(default_port), host_attributes.is_ip_address_, - [this, host]() { onReResolve(host); })) + [this, host]() { onReResolve(host); }, + [this, host]() { onResolveTimeout(host); })) .first->second.get(); } startResolve(host, *primary_host); } +DnsCacheImpl::PrimaryHostInfo& DnsCacheImpl::getPrimaryHost(const std::string& host) { + absl::ReaderMutexLock reader_lock{&primary_hosts_lock_}; + const auto primary_host_it = primary_hosts_.find(host); + ASSERT(primary_host_it != primary_hosts_.end()); + return *(primary_host_it->second.get()); +} + +void DnsCacheImpl::onResolveTimeout(const std::string& host) { + ASSERT(main_thread_dispatcher_.isThreadSafe()); + + auto& primary_host = getPrimaryHost(host); + ENVOY_LOG(debug, "host='{}' resolution timeout", host); + stats_.dns_query_timeout_.inc(); + primary_host.active_query_->cancel(); + finishResolve(host, Network::DnsResolver::ResolutionStatus::Failure, {}); +} + void DnsCacheImpl::onReResolve(const std::string& host) { ASSERT(main_thread_dispatcher_.isThreadSafe()); // If we need to erase the host, hold onto the PrimaryHostInfo object that owns this callback. @@ -216,16 +235,11 @@ void DnsCacheImpl::onReResolve(const std::string& host) { // Functions like this one that modify primary_hosts_ are only called in the main thread so we // know it is safe to use the PrimaryHostInfo pointers outside of the lock. - auto* primary_host = [&]() { - absl::ReaderMutexLock reader_lock{&primary_hosts_lock_}; - const auto primary_host_it = primary_hosts_.find(host); - ASSERT(primary_host_it != primary_hosts_.end()); - return primary_host_it->second.get(); - }(); + auto& primary_host = getPrimaryHost(host); const std::chrono::steady_clock::duration now_duration = main_thread_dispatcher_.timeSource().monotonicTime().time_since_epoch(); - auto last_used_time = primary_host->host_info_->lastUsedTime(); + auto last_used_time = primary_host.host_info_->lastUsedTime(); ENVOY_LOG(debug, "host='{}' TTL check: now={} last_used={}", host, now_duration.count(), last_used_time.count()); if ((now_duration - last_used_time) > host_ttl_) { @@ -233,7 +247,7 @@ void DnsCacheImpl::onReResolve(const std::string& host) { // If the host has no address then that means that the DnsCacheImpl has never // runAddUpdateCallbacks for this host, and thus the callback targets are not aware of it. // Therefore, runRemoveCallbacks should only be ran if the host's address != nullptr. - if (primary_host->host_info_->address()) { + if (primary_host.host_info_->address()) { runRemoveCallbacks(host); } { @@ -243,9 +257,9 @@ void DnsCacheImpl::onReResolve(const std::string& host) { host_to_erase = std::move(host_it->second); primary_hosts_.erase(host_it); } - notifyThreads(host, primary_host->host_info_); + notifyThreads(host, primary_host.host_info_); } else { - startResolve(host, *primary_host); + startResolve(host, primary_host); } } @@ -256,6 +270,7 @@ void DnsCacheImpl::startResolve(const std::string& host, PrimaryHostInfo& host_i stats_.dns_query_attempt_.inc(); + host_info.timeout_timer_->enableTimer(timeout_interval_, nullptr); host_info.active_query_ = resolver_->resolve(host_info.host_info_->resolvedHost(), dns_lookup_family_, [this, host](Network::DnsResolver::ResolutionStatus status, @@ -280,6 +295,7 @@ void DnsCacheImpl::finishResolve(const std::string& host, }(); const bool first_resolve = !primary_host_info->host_info_->firstResolveComplete(); + primary_host_info->timeout_timer_->disableTimer(); primary_host_info->active_query_ = nullptr; // If the DNS resolver successfully resolved with an empty response list, the dns cache does not @@ -379,9 +395,12 @@ void DnsCacheImpl::ThreadLocalHostInfo::onHostMapUpdate( DnsCacheImpl::PrimaryHostInfo::PrimaryHostInfo(DnsCacheImpl& parent, absl::string_view host_to_resolve, uint16_t port, - bool is_ip_address, const Event::TimerCb& timer_cb) + bool is_ip_address, + const Event::TimerCb& refresh_timer_cb, + const Event::TimerCb& timeout_timer_cb) : parent_(parent), port_(port), - refresh_timer_(parent.main_thread_dispatcher_.createTimer(timer_cb)), + refresh_timer_(parent.main_thread_dispatcher_.createTimer(refresh_timer_cb)), + timeout_timer_(parent.main_thread_dispatcher_.createTimer(timeout_timer_cb)), host_info_(std::make_shared(parent.main_thread_dispatcher_.timeSource(), host_to_resolve, is_ip_address)) { parent_.stats_.host_added_.inc(); diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h index 1e668c9847f20..461b31ec1e424 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h @@ -24,6 +24,7 @@ namespace DynamicForwardProxy { COUNTER(dns_query_attempt) \ COUNTER(dns_query_failure) \ COUNTER(dns_query_success) \ + COUNTER(dns_query_timeout) \ COUNTER(host_added) \ COUNTER(host_address_changed) \ COUNTER(host_overflow) \ @@ -139,12 +140,14 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable(&resolve_cb), Return(&resolver_->active_query_))); auto result = dns_cache_->loadDnsCacheEntry("foo.com", 80, callbacks); @@ -152,6 +154,7 @@ TEST_F(DnsCacheImplTest, ResolveSuccess) { checkStats(1 /* attempt */, 0 /* success */, 0 /* failure */, 0 /* address changed */, 1 /* added */, 0 /* removed */, 1 /* num hosts */); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate("foo.com", DnsHostInfoEquals("10.0.0.1:80", "foo.com", false))); EXPECT_CALL(callbacks, @@ -164,6 +167,7 @@ TEST_F(DnsCacheImplTest, ResolveSuccess) { 1 /* added */, 0 /* removed */, 1 /* num hosts */); // Re-resolve timer. + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); resolve_timer->invokeCallback(); @@ -172,6 +176,7 @@ TEST_F(DnsCacheImplTest, ResolveSuccess) { 1 /* added */, 0 /* removed */, 1 /* num hosts */); // Address does not change. + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _)); resolve_cb(Network::DnsResolver::ResolutionStatus::Success, TestUtility::makeDnsResponse({"10.0.0.1"})); @@ -180,6 +185,7 @@ TEST_F(DnsCacheImplTest, ResolveSuccess) { 1 /* added */, 0 /* removed */, 1 /* num hosts */); // Re-resolve timer. + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); resolve_timer->invokeCallback(); @@ -188,6 +194,7 @@ TEST_F(DnsCacheImplTest, ResolveSuccess) { 1 /* added */, 0 /* removed */, 1 /* num hosts */); // Address does change. + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate("foo.com", DnsHostInfoEquals("10.0.0.2:80", "foo.com", false))); EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _)); @@ -206,6 +213,8 @@ TEST_F(DnsCacheImplTest, Ipv4Address) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("127.0.0.1", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); auto result = dns_cache_->loadDnsCacheEntry("127.0.0.1", 80, callbacks); @@ -213,6 +222,7 @@ TEST_F(DnsCacheImplTest, Ipv4Address) { EXPECT_NE(result.handle_, nullptr); EXPECT_EQ(absl::nullopt, result.host_info_); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL( update_callbacks_, onDnsHostAddOrUpdate("127.0.0.1", DnsHostInfoEquals("127.0.0.1:80", "127.0.0.1", true))); @@ -231,6 +241,8 @@ TEST_F(DnsCacheImplTest, Ipv4AddressWithPort) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("127.0.0.1", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); auto result = dns_cache_->loadDnsCacheEntry("127.0.0.1:10000", 80, callbacks); @@ -238,6 +250,7 @@ TEST_F(DnsCacheImplTest, Ipv4AddressWithPort) { EXPECT_NE(result.handle_, nullptr); EXPECT_EQ(absl::nullopt, result.host_info_); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate("127.0.0.1:10000", DnsHostInfoEquals("127.0.0.1:10000", "127.0.0.1", true))); @@ -256,6 +269,8 @@ TEST_F(DnsCacheImplTest, Ipv6Address) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("::1", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); auto result = dns_cache_->loadDnsCacheEntry("[::1]", 80, callbacks); @@ -263,6 +278,7 @@ TEST_F(DnsCacheImplTest, Ipv6Address) { EXPECT_NE(result.handle_, nullptr); EXPECT_EQ(absl::nullopt, result.host_info_); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate("[::1]", DnsHostInfoEquals("[::1]:80", "::1", true))); EXPECT_CALL(callbacks, onLoadDnsCacheComplete(DnsHostInfoEquals("[::1]:80", "::1", true))); @@ -279,6 +295,8 @@ TEST_F(DnsCacheImplTest, Ipv6AddressWithPort) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("::1", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); auto result = dns_cache_->loadDnsCacheEntry("[::1]:10000", 80, callbacks); @@ -286,6 +304,7 @@ TEST_F(DnsCacheImplTest, Ipv6AddressWithPort) { EXPECT_NE(result.handle_, nullptr); EXPECT_EQ(absl::nullopt, result.host_info_); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate("[::1]:10000", DnsHostInfoEquals("[::1]:10000", "::1", true))); EXPECT_CALL(callbacks, onLoadDnsCacheComplete(DnsHostInfoEquals("[::1]:10000", "::1", true))); @@ -302,6 +321,8 @@ TEST_F(DnsCacheImplTest, TTL) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); auto result = dns_cache_->loadDnsCacheEntry("foo.com", 80, callbacks); @@ -312,6 +333,7 @@ TEST_F(DnsCacheImplTest, TTL) { checkStats(1 /* attempt */, 0 /* success */, 0 /* failure */, 0 /* address changed */, 1 /* added */, 0 /* removed */, 1 /* num hosts */); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate("foo.com", DnsHostInfoEquals("10.0.0.1:80", "foo.com", false))); EXPECT_CALL(callbacks, @@ -325,12 +347,14 @@ TEST_F(DnsCacheImplTest, TTL) { // Re-resolve with ~60s passed. TTL should still be OK at default of 5 minutes. simTime().advanceTimeWait(std::chrono::milliseconds(60001)); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); resolve_timer->invokeCallback(); checkStats(2 /* attempt */, 1 /* success */, 0 /* failure */, 1 /* address changed */, 1 /* added */, 0 /* removed */, 1 /* num hosts */); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _)); resolve_cb(Network::DnsResolver::ResolutionStatus::Success, TestUtility::makeDnsResponse({"10.0.0.1"})); @@ -347,6 +371,8 @@ TEST_F(DnsCacheImplTest, TTL) { // Make sure we don't get a cache hit the next time the host is requested. resolve_timer = new Event::MockTimer(&dispatcher_); + timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); result = dns_cache_->loadDnsCacheEntry("foo.com", 80, callbacks); @@ -361,12 +387,15 @@ TEST_F(DnsCacheImplTest, TTL) { TEST_F(DnsCacheImplTest, TTLWithCustomParameters) { *config_.mutable_dns_refresh_rate() = Protobuf::util::TimeUtil::SecondsToDuration(30); *config_.mutable_host_ttl() = Protobuf::util::TimeUtil::SecondsToDuration(60); + *config_.mutable_dns_query_timeout() = Protobuf::util::TimeUtil::SecondsToDuration(1); initialize(); InSequence s; MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(1000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); auto result = dns_cache_->loadDnsCacheEntry("foo.com", 80, callbacks); @@ -374,6 +403,7 @@ TEST_F(DnsCacheImplTest, TTLWithCustomParameters) { EXPECT_NE(result.handle_, nullptr); EXPECT_EQ(absl::nullopt, result.host_info_); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate("foo.com", DnsHostInfoEquals("10.0.0.1:80", "foo.com", false))); EXPECT_CALL(callbacks, @@ -384,9 +414,11 @@ TEST_F(DnsCacheImplTest, TTLWithCustomParameters) { // Re-resolve with ~30s passed. TTL should still be OK at 60s. simTime().advanceTimeWait(std::chrono::milliseconds(30001)); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(1000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); resolve_timer->invokeCallback(); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(30000), _)); resolve_cb(Network::DnsResolver::ResolutionStatus::Success, TestUtility::makeDnsResponse({"10.0.0.1"})); @@ -411,6 +443,8 @@ TEST_F(DnsCacheImplTest, InlineResolve) { EXPECT_EQ(absl::nullopt, result.host_info_); Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("localhost", _, _)) .WillOnce(Invoke([](const std::string&, Network::DnsLookupFamily, Network::DnsResolver::ResolveCb callback) { @@ -418,6 +452,7 @@ TEST_F(DnsCacheImplTest, InlineResolve) { TestUtility::makeDnsResponse({"127.0.0.1"})); return nullptr; })); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL( update_callbacks_, onDnsHostAddOrUpdate("localhost", DnsHostInfoEquals("127.0.0.1:80", "localhost", false))); @@ -427,6 +462,36 @@ TEST_F(DnsCacheImplTest, InlineResolve) { post_cb(); } +// Resolve timeout. +TEST_F(DnsCacheImplTest, ResolveTimeout) { + initialize(); + InSequence s; + + MockLoadDnsCacheEntryCallbacks callbacks; + Network::DnsResolver::ResolveCb resolve_cb; + Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); + EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) + .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); + auto result = dns_cache_->loadDnsCacheEntry("foo.com", 80, callbacks); + EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::Loading, result.status_); + EXPECT_NE(result.handle_, nullptr); + EXPECT_EQ(absl::nullopt, result.host_info_); + checkStats(1 /* attempt */, 0 /* success */, 0 /* failure */, 0 /* address changed */, + 1 /* added */, 0 /* removed */, 1 /* num hosts */); + + EXPECT_CALL(resolver_->active_query_, cancel()); + EXPECT_CALL(*timeout_timer, disableTimer()); + EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate(_, _)).Times(0); + EXPECT_CALL(callbacks, onLoadDnsCacheComplete(DnsHostInfoAddressIsNull())); + EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _)); + timeout_timer->invokeCallback(); + checkStats(1 /* attempt */, 0 /* success */, 1 /* failure */, 0 /* address changed */, + 1 /* added */, 0 /* removed */, 1 /* num hosts */); + EXPECT_EQ(1, TestUtility::findCounter(store_, "dns_cache.foo.dns_query_timeout")->value()); +} + // Resolve failure that returns no addresses. TEST_F(DnsCacheImplTest, ResolveFailure) { initialize(); @@ -435,6 +500,8 @@ TEST_F(DnsCacheImplTest, ResolveFailure) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); auto result = dns_cache_->loadDnsCacheEntry("foo.com", 80, callbacks); @@ -444,6 +511,7 @@ TEST_F(DnsCacheImplTest, ResolveFailure) { checkStats(1 /* attempt */, 0 /* success */, 0 /* failure */, 0 /* address changed */, 1 /* added */, 0 /* removed */, 1 /* num hosts */); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate(_, _)).Times(0); EXPECT_CALL(callbacks, onLoadDnsCacheComplete(DnsHostInfoAddressIsNull())); EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _)); @@ -481,6 +549,8 @@ TEST_F(DnsCacheImplTest, ResolveFailureWithFailureRefreshRate) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); auto result = dns_cache_->loadDnsCacheEntry("foo.com", 80, callbacks); @@ -490,6 +560,7 @@ TEST_F(DnsCacheImplTest, ResolveFailureWithFailureRefreshRate) { checkStats(1 /* attempt */, 0 /* success */, 0 /* failure */, 0 /* address changed */, 1 /* added */, 0 /* removed */, 1 /* num hosts */); + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate(_, _)).Times(0); EXPECT_CALL(callbacks, onLoadDnsCacheComplete(DnsHostInfoAddressIsNull())); ON_CALL(random_, random()).WillByDefault(Return(8000)); @@ -524,6 +595,8 @@ TEST_F(DnsCacheImplTest, ResolveSuccessWithEmptyResult) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); auto result = dns_cache_->loadDnsCacheEntry("foo.com", 80, callbacks); @@ -534,6 +607,7 @@ TEST_F(DnsCacheImplTest, ResolveSuccessWithEmptyResult) { 1 /* added */, 0 /* removed */, 1 /* num hosts */); // A successful empty resolution DOES NOT update the host information. + EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate(_, _)).Times(0); EXPECT_CALL(callbacks, onLoadDnsCacheComplete(DnsHostInfoAddressIsNull())); EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _)); From e822fdf8de360d0274709f8595f4d83dd99f1135 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 1 Jul 2021 17:20:10 +0000 Subject: [PATCH 2/2] comments Signed-off-by: Matt Klein --- .../dynamic_forward_proxy/v3/dns_cache.proto | 2 +- .../v4alpha/dns_cache.proto | 2 +- .../dynamic_forward_proxy_filter.rst | 1 + envoy/network/dns.h | 12 +++++++++- .../dynamic_forward_proxy/v3/dns_cache.proto | 2 +- .../v4alpha/dns_cache.proto | 2 +- source/common/network/apple_dns_impl.cc | 4 +++- source/common/network/apple_dns_impl.h | 2 +- source/common/network/dns_impl.h | 3 ++- source/common/upstream/logical_dns_cluster.cc | 2 +- source/common/upstream/strict_dns_cluster.cc | 2 +- .../clusters/redis/redis_cluster.cc | 2 +- .../dynamic_forward_proxy/dns_cache_impl.cc | 11 +++++---- test/common/network/apple_dns_impl_test.cc | 4 ++-- test/common/network/dns_impl_test.cc | 4 ++-- .../upstream/logical_dns_cluster_test.cc | 2 +- test/common/upstream/upstream_impl_test.cc | 24 ++++++++++++------- .../clusters/redis/redis_cluster_test.cc | 3 +-- .../dns_cache_impl_test.cc | 5 ++-- test/mocks/network/mocks.h | 2 +- 20 files changed, 57 insertions(+), 34 deletions(-) diff --git a/api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto b/api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto index b1b1550874fd9..b5925637a8e5a 100644 --- a/api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto +++ b/api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto @@ -119,5 +119,5 @@ message DnsCacheConfig { // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque. // Setting this timeout will ensure that queries succeed or fail within the specified time frame // and are then retried using the standard refresh rates. Defaults to 5s if not set. - google.protobuf.Duration dns_query_timeout = 11; + google.protobuf.Duration dns_query_timeout = 11 [(validate.rules).duration = {gt {}}]; } diff --git a/api/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto b/api/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto index 89cf5e0362949..fd5a296bc0600 100644 --- a/api/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto +++ b/api/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto @@ -116,5 +116,5 @@ message DnsCacheConfig { // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque. // Setting this timeout will ensure that queries succeed or fail within the specified time frame // and are then retried using the standard refresh rates. Defaults to 5s if not set. - google.protobuf.Duration dns_query_timeout = 11; + google.protobuf.Duration dns_query_timeout = 11 [(validate.rules).duration = {gt {}}]; } diff --git a/docs/root/configuration/http/http_filters/dynamic_forward_proxy_filter.rst b/docs/root/configuration/http/http_filters/dynamic_forward_proxy_filter.rst index 3699fb96d3efc..d2c9530207ee5 100644 --- a/docs/root/configuration/http/http_filters/dynamic_forward_proxy_filter.rst +++ b/docs/root/configuration/http/http_filters/dynamic_forward_proxy_filter.rst @@ -49,6 +49,7 @@ namespace. dns_query_attempt, Counter, Number of DNS query attempts. dns_query_success, Counter, Number of DNS query successes. dns_query_failure, Counter, Number of DNS query failures. + dns_query_timeout, Counter, Number of DNS query :ref:`timeouts `. host_address_changed, Counter, Number of DNS queries that resulted in a host address change. host_added, Counter, Number of hosts that have been added to the cache. host_removed, Counter, Number of hosts that have been removed from the cache. diff --git a/envoy/network/dns.h b/envoy/network/dns.h index 29cef98b104fa..d2f7e23f5bae9 100644 --- a/envoy/network/dns.h +++ b/envoy/network/dns.h @@ -19,10 +19,20 @@ class ActiveDnsQuery { public: virtual ~ActiveDnsQuery() = default; + enum class CancelReason { + // The caller no longer needs the answer to the query. + QueryAbandoned, + // The query timed out from the perspective of the caller. The DNS implementation may take + // a different action in this case (e.g., destroying existing DNS connections) in an effort + // to get an answer to future queries. + Timeout + }; + /** * Cancel an outstanding DNS request. + * @param reason supplies the cancel reason. */ - virtual void cancel() PURE; + virtual void cancel(CancelReason reason) PURE; }; /** diff --git a/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto b/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto index b1b1550874fd9..b5925637a8e5a 100644 --- a/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto +++ b/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto @@ -119,5 +119,5 @@ message DnsCacheConfig { // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque. // Setting this timeout will ensure that queries succeed or fail within the specified time frame // and are then retried using the standard refresh rates. Defaults to 5s if not set. - google.protobuf.Duration dns_query_timeout = 11; + google.protobuf.Duration dns_query_timeout = 11 [(validate.rules).duration = {gt {}}]; } diff --git a/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto b/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto index 72e75d39b4956..ec3bad19d7510 100644 --- a/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto +++ b/generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto @@ -122,5 +122,5 @@ message DnsCacheConfig { // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque. // Setting this timeout will ensure that queries succeed or fail within the specified time frame // and are then retried using the standard refresh rates. Defaults to 5s if not set. - google.protobuf.Duration dns_query_timeout = 11; + google.protobuf.Duration dns_query_timeout = 11 [(validate.rules).duration = {gt {}}]; } diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index 1325914a3dfb0..a991a99b0977c 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -265,7 +265,9 @@ AppleDnsResolverImpl::PendingResolution::~PendingResolution() { } } -void AppleDnsResolverImpl::PendingResolution::cancel() { +void AppleDnsResolverImpl::PendingResolution::cancel(Network::ActiveDnsQuery::CancelReason) { + // TODO(mattklein123): If cancel reason is timeout, do something more aggressive about destroying + // 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_) { diff --git a/source/common/network/apple_dns_impl.h b/source/common/network/apple_dns_impl.h index d22bc2c7c87e2..b4b48a528d2a2 100644 --- a/source/common/network/apple_dns_impl.h +++ b/source/common/network/apple_dns_impl.h @@ -90,7 +90,7 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggablecancel(); + active_dns_query_->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned); } } diff --git a/source/common/upstream/strict_dns_cluster.cc b/source/common/upstream/strict_dns_cluster.cc index 6663f63cd9e5a..f270b6ea5300e 100644 --- a/source/common/upstream/strict_dns_cluster.cc +++ b/source/common/upstream/strict_dns_cluster.cc @@ -97,7 +97,7 @@ StrictDnsClusterImpl::ResolveTarget::ResolveTarget( StrictDnsClusterImpl::ResolveTarget::~ResolveTarget() { if (active_query_) { - active_query_->cancel(); + active_query_->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned); } } diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index 18a0a79fda211..25ae1c8c7c66d 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -149,7 +149,7 @@ RedisCluster::DnsDiscoveryResolveTarget::DnsDiscoveryResolveTarget(RedisCluster& RedisCluster::DnsDiscoveryResolveTarget::~DnsDiscoveryResolveTarget() { if (active_query_) { - active_query_->cancel(); + active_query_->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned); } // Disable timer for mock tests. if (resolve_timer_) { diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc index 9cde22b39ee5a..e4d8204687893 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc @@ -58,7 +58,8 @@ DnsCacheImpl::DnsCacheImpl( DnsCacheImpl::~DnsCacheImpl() { for (const auto& primary_host : primary_hosts_) { if (primary_host.second->active_query_ != nullptr) { - primary_host.second->active_query_->cancel(); + primary_host.second->active_query_->cancel( + Network::ActiveDnsQuery::CancelReason::QueryAbandoned); } } @@ -210,6 +211,9 @@ void DnsCacheImpl::startCacheLoad(const std::string& host, uint16_t default_port } DnsCacheImpl::PrimaryHostInfo& DnsCacheImpl::getPrimaryHost(const std::string& host) { + // Functions modify primary_hosts_ are only called in the main thread so we + // know it is safe to use the PrimaryHostInfo pointers outside of the lock. + ASSERT(main_thread_dispatcher_.isThreadSafe()); absl::ReaderMutexLock reader_lock{&primary_hosts_lock_}; const auto primary_host_it = primary_hosts_.find(host); ASSERT(primary_host_it != primary_hosts_.end()); @@ -222,7 +226,7 @@ void DnsCacheImpl::onResolveTimeout(const std::string& host) { auto& primary_host = getPrimaryHost(host); ENVOY_LOG(debug, "host='{}' resolution timeout", host); stats_.dns_query_timeout_.inc(); - primary_host.active_query_->cancel(); + primary_host.active_query_->cancel(Network::ActiveDnsQuery::CancelReason::Timeout); finishResolve(host, Network::DnsResolver::ResolutionStatus::Failure, {}); } @@ -233,10 +237,7 @@ void DnsCacheImpl::onReResolve(const std::string& host) { // use-after-free issues PrimaryHostInfoPtr host_to_erase; - // Functions like this one that modify primary_hosts_ are only called in the main thread so we - // know it is safe to use the PrimaryHostInfo pointers outside of the lock. auto& primary_host = getPrimaryHost(host); - const std::chrono::steady_clock::duration now_duration = main_thread_dispatcher_.timeSource().monotonicTime().time_since_epoch(); auto last_used_time = primary_host.host_info_->lastUsedTime(); diff --git a/test/common/network/apple_dns_impl_test.cc b/test/common/network/apple_dns_impl_test.cc index d2f00c15cdb29..5763e4b52e8dc 100644 --- a/test/common/network/apple_dns_impl_test.cc +++ b/test/common/network/apple_dns_impl_test.cc @@ -137,7 +137,7 @@ TEST_F(AppleDnsImplTest, InvalidConfigOptions) { TEST_F(AppleDnsImplTest, DestructPending) { ActiveDnsQuery* query = resolveWithUnreferencedParameters("", DnsLookupFamily::V4Only, 0); ASSERT_NE(nullptr, query); - query->cancel(); + query->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned); } TEST_F(AppleDnsImplTest, LocalLookup) { @@ -194,7 +194,7 @@ TEST_F(AppleDnsImplTest, Cancel) { DnsResolver::ResolutionStatus::Success, true)); ASSERT_NE(nullptr, query); - query->cancel(); + query->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned); dispatcher_->run(Event::Dispatcher::RunType::Block); } diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index e3c1401c673b2..11a6cfb2001fb 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -572,7 +572,7 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplTest, TEST_P(DnsImplTest, DestructPending) { ActiveDnsQuery* query = resolveWithUnreferencedParameters("", DnsLookupFamily::V4Only, false); ASSERT_NE(nullptr, query); - query->cancel(); + query->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned); // Also validate that pending events are around to exercise the resource // reclamation path. EXPECT_GT(peer_->events().size(), 0U); @@ -806,7 +806,7 @@ TEST_P(DnsImplTest, Cancel) { {"201.134.56.7"}, {}, absl::nullopt)); ASSERT_NE(nullptr, query); - query->cancel(); + query->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned); dispatcher_->run(Event::Dispatcher::RunType::Block); } diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index ce94bf6e2184a..407a7f2f86bee 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -188,7 +188,7 @@ class LogicalDnsClusterTest : public Event::TestUsingSimulatedTime, public testi logical_host->createConnection(dispatcher_, nullptr, nullptr); // Make sure we cancel. - EXPECT_CALL(active_dns_query_, cancel()); + EXPECT_CALL(active_dns_query_, cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); expectResolve(Network::DnsLookupFamily::V4Only, expected_address); resolve_timer_->invokeCallback(); diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index ed7586497ae4a..4b7c0791ede63 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -457,8 +457,10 @@ TEST_F(StrictDnsClusterImplTest, Basic) { resolver2.expectResolve(*dns_resolver_); resolver2.timer_->invokeCallback(); - EXPECT_CALL(resolver1.active_dns_query_, cancel()); - EXPECT_CALL(resolver2.active_dns_query_, cancel()); + EXPECT_CALL(resolver1.active_dns_query_, + cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); + EXPECT_CALL(resolver2.active_dns_query_, + cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); } // Verifies that host removal works correctly when hosts are being health checked @@ -854,9 +856,12 @@ TEST_F(StrictDnsClusterImplTest, LoadAssignmentBasic) { resolver3.expectResolve(*dns_resolver_); resolver3.timer_->invokeCallback(); - EXPECT_CALL(resolver1.active_dns_query_, cancel()); - EXPECT_CALL(resolver2.active_dns_query_, cancel()); - EXPECT_CALL(resolver3.active_dns_query_, cancel()); + EXPECT_CALL(resolver1.active_dns_query_, + cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); + EXPECT_CALL(resolver2.active_dns_query_, + cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); + EXPECT_CALL(resolver3.active_dns_query_, + cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); } TEST_F(StrictDnsClusterImplTest, LoadAssignmentBasicMultiplePriorities) { @@ -993,9 +998,12 @@ TEST_F(StrictDnsClusterImplTest, LoadAssignmentBasicMultiplePriorities) { resolver3.expectResolve(*dns_resolver_); resolver3.timer_->invokeCallback(); - EXPECT_CALL(resolver1.active_dns_query_, cancel()); - EXPECT_CALL(resolver2.active_dns_query_, cancel()); - EXPECT_CALL(resolver3.active_dns_query_, cancel()); + EXPECT_CALL(resolver1.active_dns_query_, + cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); + EXPECT_CALL(resolver2.active_dns_query_, + cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); + EXPECT_CALL(resolver3.active_dns_query_, + cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); } // Verifies that specifying a custom resolver when using STRICT_DNS fails diff --git a/test/extensions/clusters/redis/redis_cluster_test.cc b/test/extensions/clusters/redis/redis_cluster_test.cc index 4bc85dd7150fe..526c10a3039f2 100644 --- a/test/extensions/clusters/redis/redis_cluster_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_test.cc @@ -544,10 +544,9 @@ class RedisClusterTest : public testing::Test, Network::DnsResolver::ResolveCb) -> Network::ActiveDnsQuery* { return &active_dns_query_; })); - ; resolver_target.startResolveDns(); - EXPECT_CALL(active_dns_query_, cancel()); + EXPECT_CALL(active_dns_query_, cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); } Stats::TestUtil::TestStore stats_store_; diff --git a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc index 1f539269208b1..397b87bc1adb2 100644 --- a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc +++ b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc @@ -481,7 +481,7 @@ TEST_F(DnsCacheImplTest, ResolveTimeout) { checkStats(1 /* attempt */, 0 /* success */, 0 /* failure */, 0 /* address changed */, 1 /* added */, 0 /* removed */, 1 /* num hosts */); - EXPECT_CALL(resolver_->active_query_, cancel()); + EXPECT_CALL(resolver_->active_query_, cancel(Network::ActiveDnsQuery::CancelReason::Timeout)); EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate(_, _)).Times(0); EXPECT_CALL(callbacks, onLoadDnsCacheComplete(DnsHostInfoAddressIsNull())); @@ -783,7 +783,8 @@ TEST_F(DnsCacheImplTest, CancelActiveQueriesOnDestroy) { EXPECT_NE(result.handle_, nullptr); EXPECT_EQ(absl::nullopt, result.host_info_); - EXPECT_CALL(resolver_->active_query_, cancel()); + EXPECT_CALL(resolver_->active_query_, + cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned)); dns_cache_.reset(); } diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index b112ccf480d0f..b15bb1abdf5f7 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -37,7 +37,7 @@ class MockActiveDnsQuery : public ActiveDnsQuery { ~MockActiveDnsQuery() override; // Network::ActiveDnsQuery - MOCK_METHOD(void, cancel, ()); + MOCK_METHOD(void, cancel, (CancelReason reason)); }; class MockDnsResolver : public DnsResolver {