dns cache: add DNS query timeout option

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
 // <arch_overview_http_dynamic_forward_proxy>` 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;
 }

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 <envoy_v3_api_field_config.cluster.v3.Cluster.connect_timeout>`.
+* dns cache: the new :ref:`dns_query_timeout <envoy_v3_api_field_extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig.dns_query_timeout>` option has a default of 5s. See below for more information.
 * http: disable the integration between :ref:`ExtensionWithMatcher <envoy_v3_api_msg_extensions.common.matching.v3.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 <config_network_filters_connection_limit>`.
 * 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 <envoy_v3_api_field_extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig.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 <envoy_v3_api_field_extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig.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 <envoy_v3_api_field_config.core.v3.DnsResolverOptions.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 <envoy_v3_api_field_config.core.v3.DnsResolverOptions.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 <envoy_v3_api_field_config.core.v3.DnsResolutionConfig.dns_resolver_options>` and :ref:`resolvers <envoy_v3_api_field_config.core.v3.DnsResolutionConfig.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 <envoy_v3_api_field_extensions.filters.udp.dns_filter.v3alpha.DnsFilterConfig.ClientContextConfig.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``.

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,24 +235,19 @@ 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_) {
     ENVOY_LOG(debug, "host='{}' TTL expired, removing", 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<DnsHostInfoImpl>(parent.main_thread_dispatcher_.timeSource(),
                                                    host_to_resolve, is_ip_address)) {
   parent_.stats_.host_added_.inc();

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<Logger::Id::forward_proxy
   // Primary host information that accounts for TTL, re-resolution, etc.
   struct 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);
     ~PrimaryHostInfo();
 
     DnsCacheImpl& parent_;
     const uint16_t port_;
     const Event::TimerPtr refresh_timer_;
+    const Event::TimerPtr timeout_timer_;
     const DnsHostInfoImplSharedPtr host_info_;
     Network::ActiveDnsQuery* active_query_{};
   };
@@ -172,6 +175,8 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable<Logger::Id::forward_proxy
   void runRemoveCallbacks(const std::string& host);
   void notifyThreads(const std::string& host, const DnsHostInfoImplSharedPtr& resolved_info);
   void onReResolve(const std::string& host);
+  void onResolveTimeout(const std::string& host);
+  PrimaryHostInfo& getPrimaryHost(const std::string& host);
 
   Event::Dispatcher& main_thread_dispatcher_;
   const Network::DnsLookupFamily dns_lookup_family_;
@@ -185,6 +190,7 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable<Logger::Id::forward_proxy
       primary_hosts_ ABSL_GUARDED_BY(primary_hosts_lock_);
   DnsCacheResourceManagerImpl resource_manager_;
   const std::chrono::milliseconds refresh_interval_;
+  const std::chrono::milliseconds timeout_interval_;
   const BackOffStrategyPtr failure_backoff_strategy_;
   const std::chrono::milliseconds host_ttl_;
   const uint32_t max_hosts_;