envoyproxy · alyssawilk · Jan 5, 2022
diff --git a/api/envoy/extensions/filters/http/dynamic_forward_proxy/v3/dynamic_forward_proxy.proto b/api/envoy/extensions/filters/http/dynamic_forward_proxy/v3/dynamic_forward_proxy.proto
@@ -4,6 +4,8 @@ package envoy.extensions.filters.http.dynamic_forward_proxy.v3;
 
 import "envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto";
 
+import "google/protobuf/duration.proto";
+
 import "udpa/annotations/status.proto";
 import "udpa/annotations/versioning.proto";
 import "validate/validate.proto";
@@ -34,6 +36,10 @@ message FilterConfig {
   // `envoy.stream.upstream_address` (See
   // :repo:`upstream_address.h<source/common/stream_info/upstream_address.h>`).
   bool save_upstream_address = 2;
+
+  // The duration to wait for a DNS resolution before sending a local response.
+  // If this is not set it defaults to 5s.
+  google.protobuf.Duration resolution_timeout = 3;
 }
 
 // Per route Configuration for the dynamic forward proxy HTTP filter.

diff --git a/...y/extensions/filters/network/sni_dynamic_forward_proxy/v3/sni_dynamic_forward_proxy.proto b/...y/extensions/filters/network/sni_dynamic_forward_proxy/v3/sni_dynamic_forward_proxy.proto
@@ -4,6 +4,8 @@ package envoy.extensions.filters.network.sni_dynamic_forward_proxy.v3;
 
 import "envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto";
 
+import "google/protobuf/duration.proto";
+
 import "udpa/annotations/status.proto";
 import "validate/validate.proto";
 
@@ -33,4 +35,8 @@ message FilterConfig {
     // The port number to connect to the upstream.
     uint32 port_value = 2 [(validate.rules).uint32 = {lte: 65535 gt: 0}];
   }
+
+  // The duration to wait for a DNS resolution before failing with a downstream disconnect.
+  // If this is not set it defaults to 5s.
+  google.protobuf.Duration resolution_timeout = 3;
 }
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -13,14 +13,17 @@ Minor Behavior Changes
 
 * bandwidth_limit: added :ref:`response trailers <envoy_v3_api_field_extensions.filters.http.bandwidth_limit.v3.BandwidthLimit.enable_response_trailers>` when request or response delay are enforced.
 * bandwidth_limit: added :ref:`bandwidth limit stats <config_http_filters_bandwidth_limit>` *request_enforced* and *response_enforced*.
+* dns: fixing a bug where the dns cache sent updates to threads on resolution failure. This behavior can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.disallow_empty_resolutions`` to false.
 * dns: now respecting the returned DNS TTL for resolved hosts, rather than always relying on the hard-coded :ref:`dns_refresh_rate. <envoy_v3_api_field_config.cluster.v3.Cluster.dns_refresh_rate>` This behavior can be temporarily reverted by setting the runtime guard ``envoy.reloadable_features.use_dns_ttl`` to false.
+* dynamic_proxy_filter: adding a default 5s timeout configured by :ref:`resolution_timeout <envoy_v3_api_field_extensions.filters.http.dynamic_forward_proxy.v3.FilterConfig.resolution_timeout>`.  Previously when DNS resolution failed, the DNS cache would update with an empty host list after the initial resolution timeout, the filter would unblock the request, and upstream communication would fail with "no unhealthy hosts". To preserve prior behavior now that the DNS cache will not send null updates, the filter now has a resolution timer defaulting to 5s which will fail the request clearly indicating DNS resolution as the failure mode. This behavioral change can be reverted either by setting the resolution timeout to 0, or setting ``envoy.reloadable_features.disallow_empty_resolutions`` to false.
 * http: envoy will now proxy 102 and 103 headers from upstream, though as with 100s only the first 1xx response headers will be sent. This behavioral change by can temporarily reverted by setting runtime guard ``envoy.reloadable_features.proxy_102_103`` to false.
 * http: usage of the experimental matching API is no longer guarded behind a feature flag, as the corresponding protobuf fields have been marked as WIP.
 * http: when envoy run out of ``max_requests_per_connection``, it will send an HTTP/2 "shutdown nofitication" (GOAWAY frame with max stream ID) and go to a default grace period of 5000 milliseconds (5 seconds) if ``drain_timeout`` is not specified. During this grace period, envoy will continue to accept new streams. After the grace period, a final GOAWAY is sent and envoy will start refusing new streams. However before bugfix, during the grace period, every time a new stream is received, old envoy will always send a "shutdown notification" and restart drain again which actually causes the grace period to be extended and is no longer equal to ``drain_timeout``.
 * json: switching from rapidjson to nlohmann/json. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.remove_legacy_json`` to false.
 * listener: destroy per network filter chain stats when a network filter chain is removed during the listener in place update.
 * quic: add back the support for IETF draft 29 which is guarded via ``envoy.reloadable_features.FLAGS_quic_reloadable_flag_quic_disable_version_draft_29``. It is off by default so Envoy only supports RFCv1 without flipping this runtime guard explicitly. Draft 29 is not recommended for use.
 * router: take elapsed time into account when setting the x-envoy-expected-rq-timeout-ms header for retries, and never send a value that's longer than the request timeout. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.update_expected_rq_timeout_on_retry`` to false.
+* sni_dynamic_proxy_filter: adding a default 5s timeout configured by :ref:`resolution_timeout <envoy_v3_api_field_extensions.filters.network.sni_dynamic_forward_proxy.v3alpha.FilterConfig.resolution_timeout>`.  Previously when DNS resolution failed, the DNS cache would update with an empty host list after the initial resolution timeout, the sni dfp filter would unblock the request, and upstream communication would fail resulting in a disconnect. To preserve prior behavior now that the DNS cache will not send null updates, the filter now has a resolution timer defaulting to 5s which will fail the request clearly indicating DNS resolution as the failure mode. This behavioral change can be reverted either by setting the resolution timeout to 0, or setting ``envoy.reloadable_features.disallow_empty_resolutions`` to false.
 
 Bug Fixes
 ---------

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -60,6 +60,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.conn_pool_delete_when_idle",
     "envoy.reloadable_features.correct_scheme_and_xfp",
     "envoy.reloadable_features.disable_tls_inspector_injection",
+    "envoy.reloadable_features.disallow_empty_resolutions",
     "envoy.reloadable_features.fix_added_trailers",
     "envoy.reloadable_features.grpc_bridge_stats_disabled",
     "envoy.reloadable_features.handle_stream_reset_during_hcm_encoding",

@@ -96,6 +96,21 @@ class DnsCache {
      * @param host_info the DnsHostInfo for the resolved host.
      */
     virtual void onLoadDnsCacheComplete(const DnsHostInfoSharedPtr& host_info) PURE;
+
+    /**
+     * Called if as asychronous DNS lookup times out.
+     */
+    virtual void onResolutionTimeout() PURE;
+
+    /**
+     * Returns the resolution time out for this lookup.
+     */
+    virtual std::chrono::milliseconds resolutionTimeout() const PURE;
+
+    /**
+     * Returns the worker thread dispatcher this look up is for.
+     */
+    virtual Event::Dispatcher& dispatcher() PURE;
   };
 
   /**

@@ -213,7 +213,7 @@ void DnsCacheImpl::onResolveTimeout(const std::string& host) {
   ASSERT(main_thread_dispatcher_.isThreadSafe());
 
   auto& primary_host = getPrimaryHost(host);
-  ENVOY_LOG_EVENT(debug, "dns_cache_resolve_timeout", "host='{}' resolution timeout", host);
+  ENVOY_LOG_EVENT(debug, "dns_cache_resolution_timeout", "host='{}' resolution timeout", host);
   stats_.dns_query_timeout_.inc();
   primary_host.active_query_->cancel(Network::ActiveDnsQuery::CancelReason::Timeout);
   finishResolve(host, Network::DnsResolver::ResolutionStatus::Failure, {});
@@ -372,7 +372,9 @@ void DnsCacheImpl::finishResolve(const std::string& host,
   if (first_resolve) {
     primary_host_info->host_info_->setFirstResolveComplete();
   }
-  if (first_resolve || (address_changed && !primary_host_info->host_info_->isStale())) {
+  if ((new_address ||
+       !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.disallow_empty_resolutions")) &&
+      (first_resolve || (address_changed && !primary_host_info->host_info_->isStale()))) {
     notifyThreads(host, primary_host_info->host_info_);
   }
 
@@ -412,6 +414,24 @@ void DnsCacheImpl::notifyThreads(const std::string& host,
   });
 }
 
+DnsCacheImpl::LoadDnsCacheEntryHandleImpl::LoadDnsCacheEntryHandleImpl(
+    absl::flat_hash_map<std::string, std::list<LoadDnsCacheEntryHandleImpl*>>& parent,
+    absl::string_view host, LoadDnsCacheEntryCallbacks& callbacks)
+    : RaiiMapOfListElement<std::string, LoadDnsCacheEntryHandleImpl*>(parent, host, this),
+      callbacks_(callbacks) {
+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.disallow_empty_resolutions")) {
+    resolution_timer_ =
+        callbacks_.dispatcher().createTimer([this]() -> void { callbacks_.onResolutionTimeout(); });
+    resolution_timer_->enableTimer(callbacks_.resolutionTimeout());
+  }
+}
+
+DnsCacheImpl::LoadDnsCacheEntryHandleImpl::~LoadDnsCacheEntryHandleImpl() {
+  if (resolution_timer_ && resolution_timer_->enabled()) {
+    resolution_timer_->disableTimer();
+  }
+}
+
 DnsCacheImpl::ThreadLocalHostInfo::~ThreadLocalHostInfo() {
   // Make sure we cancel any handles that still exist.
   for (const auto& per_host_list : pending_resolutions_) {
@@ -427,6 +447,9 @@ void DnsCacheImpl::ThreadLocalHostInfo::onHostMapUpdate(
   if (host_it != pending_resolutions_.end()) {
     for (auto* resolution : host_it->second) {
       auto& callbacks = resolution->callbacks_;
+      if (resolution->resolution_timer_) {
+        resolution->resolution_timer_->disableTimer();
+      }
       resolution->cancel();
       callbacks.onLoadDnsCacheComplete(resolved_host->info_);
     }

@@ -70,11 +70,11 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable<Logger::Id::forward_proxy
         RaiiMapOfListElement<std::string, LoadDnsCacheEntryHandleImpl*> {
     LoadDnsCacheEntryHandleImpl(
         absl::flat_hash_map<std::string, std::list<LoadDnsCacheEntryHandleImpl*>>& parent,
-        absl::string_view host, LoadDnsCacheEntryCallbacks& callbacks)
-        : RaiiMapOfListElement<std::string, LoadDnsCacheEntryHandleImpl*>(parent, host, this),
-          callbacks_(callbacks) {}
+        absl::string_view host, LoadDnsCacheEntryCallbacks& callbacks);
+    ~LoadDnsCacheEntryHandleImpl() override;
 
     LoadDnsCacheEntryCallbacks& callbacks_;
+    Event::TimerPtr resolution_timer_;
   };
 
   class DnsHostInfoImpl;

@@ -5,6 +5,7 @@
 #include "envoy/extensions/filters/http/dynamic_forward_proxy/v3/dynamic_forward_proxy.pb.h"
 
 #include "source/common/http/utility.h"
+#include "source/common/runtime/runtime_features.h"
 #include "source/common/stream_info/upstream_address.h"
 #include "source/extensions/common/dynamic_forward_proxy/dns_cache.h"
 
@@ -24,11 +25,13 @@ void latchTime(Http::StreamDecoderFilterCallbacks* decoder_callbacks, absl::stri
 struct ResponseStringValues {
   const std::string DnsCacheOverflow = "DNS cache overflow";
   const std::string PendingRequestOverflow = "Dynamic forward proxy pending request overflow";
+  const std::string DnsResolutionTimeout = "DNS resolution timeout";
 };
 
 struct RcDetailsValues {
   const std::string DnsCacheOverflow = "dns_cache_overflow";
   const std::string PendingRequestOverflow = "dynamic_forward_proxy_pending_request_overflow";
+  const std::string DnsResolutionTimeout = "dns_resolution_timeout";
 };
 
 using CustomClusterType = envoy::config::cluster::v3::Cluster::CustomClusterType;
@@ -45,7 +48,8 @@ ProxyFilterConfig::ProxyFilterConfig(
     : dns_cache_manager_(cache_manager_factory.get()),
       dns_cache_(dns_cache_manager_->getCache(proto_config.dns_cache_config())),
       cluster_manager_(cluster_manager),
-      save_upstream_address_(proto_config.save_upstream_address()) {}
+      save_upstream_address_(proto_config.save_upstream_address()),
+      resolution_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(proto_config, resolution_timeout, 5000)) {}
 
 ProxyPerRouteConfig::ProxyPerRouteConfig(
     const envoy::extensions::filters::http::dynamic_forward_proxy::v3::PerRouteConfig& config)
@@ -167,7 +171,6 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea
 
 void ProxyFilter::addHostAddressToFilterState(
     const Network::Address::InstanceConstSharedPtr& address) {
-
   if (!config_->saveUpstreamAddress()) {
     return;
   }
@@ -195,6 +198,12 @@ void ProxyFilter::addHostAddressToFilterState(
                         StreamInfo::FilterState::LifeSpan::Request);
 }
 
+void ProxyFilter::onResolutionTimeout() {
+  decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable,
+                                     ResponseStrings::get().DnsResolutionTimeout, nullptr,
+                                     absl::nullopt, RcDetails::get().DnsResolutionTimeout);
+}
+
 void ProxyFilter::onLoadDnsCacheComplete(
     const Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) {
   ENVOY_STREAM_LOG(debug, "load DNS cache complete, continuing after adding resolved host: {}",

@@ -21,12 +21,14 @@ class ProxyFilterConfig {
   Extensions::Common::DynamicForwardProxy::DnsCache& cache() { return *dns_cache_; }
   Upstream::ClusterManager& clusterManager() { return cluster_manager_; }
   bool saveUpstreamAddress() const { return save_upstream_address_; };
+  std::chrono::milliseconds& resolveTimeout() { return resolution_timeout_; }
 
 private:
   const Extensions::Common::DynamicForwardProxy::DnsCacheManagerSharedPtr dns_cache_manager_;
   const Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr dns_cache_;
   Upstream::ClusterManager& cluster_manager_;
   const bool save_upstream_address_;
+  std::chrono::milliseconds resolution_timeout_;
 };
 
 using ProxyFilterConfigSharedPtr = std::shared_ptr<ProxyFilterConfig>;
@@ -62,6 +64,9 @@ class ProxyFilter
   // Extensions::Common::DynamicForwardProxy::DnsCache::LoadDnsCacheEntryCallbacks
   void onLoadDnsCacheComplete(
       const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) override;
+  void onResolutionTimeout() override;
+  std::chrono::milliseconds resolutionTimeout() const override { return config_->resolveTimeout(); }
+  Event::Dispatcher& dispatcher() override { return decoder_callbacks_->dispatcher(); }
 
 private:
   void addHostAddressToFilterState(const Network::Address::InstanceConstSharedPtr& address);

diff --git a/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc b/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc
@@ -18,7 +18,8 @@ ProxyFilterConfig::ProxyFilterConfig(
     Upstream::ClusterManager&)
     : port_(static_cast<uint16_t>(proto_config.port_value())),
       dns_cache_manager_(cache_manager_factory.get()),
-      dns_cache_(dns_cache_manager_->getCache(proto_config.dns_cache_config())) {}
+      dns_cache_(dns_cache_manager_->getCache(proto_config.dns_cache_config())),
+      resolution_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(proto_config, resolution_timeout, 5000)) {}
 
 ProxyFilter::ProxyFilter(ProxyFilterConfigSharedPtr config) : config_(std::move(config)) {}
 
@@ -70,6 +71,11 @@ Network::FilterStatus ProxyFilter::onNewConnection() {
   NOT_REACHED_GCOVR_EXCL_LINE;
 }
 
+void ProxyFilter::onResolutionTimeout() {
+  ENVOY_CONN_LOG(debug, "DNS resolution timeout", read_callbacks_->connection());
+  read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
+}
+
 void ProxyFilter::onLoadDnsCacheComplete(const Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) {
   ENVOY_CONN_LOG(debug, "load DNS cache complete, continuing", read_callbacks_->connection());
   ASSERT(circuit_breaker_ != nullptr);

diff --git a/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.h b/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.h
@@ -24,11 +24,13 @@ class ProxyFilterConfig {
 
   Extensions::Common::DynamicForwardProxy::DnsCache& cache() { return *dns_cache_; }
   uint32_t port() { return port_; }
+  std::chrono::milliseconds resolutionTimeout() { return resolution_timeout_; }
 
 private:
   const uint32_t port_;
   const Extensions::Common::DynamicForwardProxy::DnsCacheManagerSharedPtr dns_cache_manager_;
   const Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr dns_cache_;
+  const std::chrono::milliseconds resolution_timeout_;
 };
 
 using ProxyFilterConfigSharedPtr = std::shared_ptr<ProxyFilterConfig>;
@@ -52,6 +54,11 @@ class ProxyFilter
   // Extensions::Common::DynamicForwardProxy::DnsCache::LoadDnsCacheEntryCallbacks
   void onLoadDnsCacheComplete(
       const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) override;
+  void onResolutionTimeout() override;
+  std::chrono::milliseconds resolutionTimeout() const override {
+    return config_->resolutionTimeout();
+  }
+  Event::Dispatcher& dispatcher() override { return read_callbacks_->connection().dispatcher(); }
 
 private:
   const ProxyFilterConfigSharedPtr config_;

@@ -57,6 +57,7 @@ envoy_cc_mock(
     hdrs = ["mocks.h"],
     deps = [
         "//source/extensions/common/dynamic_forward_proxy:dns_cache_impl",
+        "//test/mocks/event:event_mocks",
         "//test/mocks/upstream:basic_resource_limit_mocks",
         "@envoy_api//envoy/extensions/common/dynamic_forward_proxy/v3:pkg_cc_proto",
     ],