dns cache: add DNS query timeout option (envoyproxy#17207)

Signed-off-by: Matt Klein <[email protected]>

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 [(validate.rules).duration = {gt {}}];
 }

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 <envoy_v3_api_field_extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig.dns_query_timeout>`.
   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.

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.
@@ -92,6 +93,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``.

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;
 };
 
 /**

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_) {

source/common/network/apple_dns_impl.h

@@ -90,7 +90,7 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logg
     ~PendingResolution();
 
     // Network::ActiveDnsQuery
-    void cancel() override;
+    void cancel(Network::ActiveDnsQuery::CancelReason reason) override;
 
     static DnsResponse buildDnsResponse(const struct sockaddr* address, uint32_t ttl);
     // Wrapper for the API call.

source/common/network/dns_impl.h

@@ -44,9 +44,10 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
         : parent_(parent), callback_(callback), dispatcher_(dispatcher), channel_(channel),
           dns_name_(dns_name) {}
 
-    void cancel() override {
+    void cancel(CancelReason) override {
       // c-ares only supports channel-wide cancellation, so we just allow the
       // network events to continue but don't invoke the callback on completion.
+      // TODO(mattklein123): Potentially use timeout to destroy and recreate the channel.
       cancelled_ = true;
     }
 

source/common/upstream/logical_dns_cluster.cc

@@ -98,7 +98,7 @@ void LogicalDnsCluster::startPreInit() {
 
 LogicalDnsCluster::~LogicalDnsCluster() {
   if (active_dns_query_) {
-    active_dns_query_->cancel();
+    active_dns_query_->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned);
   }
 }
 

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);
   }
 }
 

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_) {

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>(
@@ -57,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);
     }
   }
 
@@ -200,40 +202,53 @@ 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) {
+  // 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());
+  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(Network::ActiveDnsQuery::CancelReason::Timeout);
+  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.
   // This is defined at function scope so that it is only erased on function exit to avoid
   // 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 = [&]() {
-    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 +258,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 +271,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 +296,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 +396,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();