DNS: officially (previously only unofficially) allowing null resolutions

api/envoy/data/accesslog/v3/accesslog.proto

@@ -192,7 +192,7 @@ message AccessLogCommon {
 }
 
 // Flags indicating occurrences during request/response processing.
-// [#next-free-field: 27]
+// [#next-free-field: 28]
 message ResponseFlags {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.data.accesslog.v2.ResponseFlags";
@@ -290,6 +290,9 @@ message ResponseFlags {
 
   // Indicates overload manager terminated the request.
   bool overload_manager = 26;
+
+  // Indicates a DNS resolution failed.
+  bool dns_resolution_failure = 27;
 }
 
 // Properties of a negotiated TLS connection.

docs/root/configuration/observability/access_log/usage.rst

@@ -391,6 +391,7 @@ The following command operators are supported:
     * **UPE**: The upstream response had an HTTP protocol error.
     * **UMSDR**: The upstream request reached max stream duration.
     * **OM**: Overload Manager terminated the request.
+    * **DF**: The request was terminated due to DNS resolution failure.
 
 %ROUTE_NAME%
   Name of the route.

docs/root/version_history/current.rst

@@ -11,6 +11,7 @@ Minor Behavior Changes
 ----------------------
 *Changes that may cause incompatibilities for some users, but should not for most*
 
+* dynamic_forward_proxy: if a DNS resolution fails, failing immediately with a specific resolution error, rather than finishing up all local filters and failing to select an upstream host.
 * ext_authz: added requested server name in ext_authz network filter for auth review.
 * grpc: flip runtime guard ``envoy.reloadable_features.enable_grpc_async_client_cache`` to be default enabled. async grpc client created through getOrCreateRawAsyncClient will be cached by default.
 * http: now the max concurrent streams of http2 connection can not only be adjusted down according to the SETTINGS frame but also can be adjusted up, of course, it can not exceed the configured upper bounds. This fix is guarded by ``envoy.reloadable_features.http2_allow_capacity_increase_by_settings``.

envoy/stream_info/stream_info.h

@@ -89,8 +89,10 @@ enum ResponseFlag {
   NoClusterFound = 0x1000000,
   // Overload Manager terminated the stream.
   OverloadManager = 0x2000000,
+  // DNS resolution failed.
+  DnsResolutionFailed = 0x4000000,
   // ATTENTION: MAKE SURE THIS REMAINS EQUAL TO THE LAST FLAG.
-  LastFlag = OverloadManager,
+  LastFlag = DnsResolutionFailed,
 };
 
 /**

source/common/stream_info/utility.cc

@@ -21,7 +21,7 @@ const std::string ResponseFlagUtils::toShortString(const StreamInfo& stream_info
 }
 
 absl::flat_hash_map<std::string, ResponseFlag> ResponseFlagUtils::getFlagMap() {
-  static_assert(ResponseFlag::LastFlag == 0x2000000,
+  static_assert(ResponseFlag::LastFlag == 0x4000000,
                 "A flag has been added. Add the new flag to ALL_RESPONSE_STRING_FLAGS.");
   absl::flat_hash_map<std::string, ResponseFlag> res;
   for (auto [str, flag] : ResponseFlagUtils::ALL_RESPONSE_STRING_FLAGS) {

source/common/stream_info/utility.h

@@ -45,6 +45,7 @@ class ResponseFlagUtils {
   constexpr static absl::string_view UPSTREAM_PROTOCOL_ERROR = "UPE";
   constexpr static absl::string_view NO_CLUSTER_FOUND = "NC";
   constexpr static absl::string_view OVERLOAD_MANAGER = "OM";
+  constexpr static absl::string_view DNS_FAIL = "DF";
 
   static constexpr std::array ALL_RESPONSE_STRING_FLAGS{
       FlagStringAndEnum{FAILED_LOCAL_HEALTH_CHECK, ResponseFlag::FailedLocalHealthCheck},

source/extensions/access_loggers/grpc/grpc_access_log_utils.cc

@@ -39,7 +39,7 @@ void Utility::responseFlagsToAccessLogResponseFlags(
     envoy::data::accesslog::v3::AccessLogCommon& common_access_log,
     const StreamInfo::StreamInfo& stream_info) {
 
-  static_assert(StreamInfo::ResponseFlag::LastFlag == 0x2000000,
+  static_assert(StreamInfo::ResponseFlag::LastFlag == 0x4000000,
                 "A flag has been added. Fix this code.");
 
   if (stream_info.hasResponseFlag(StreamInfo::ResponseFlag::FailedLocalHealthCheck)) {
@@ -146,6 +146,10 @@ void Utility::responseFlagsToAccessLogResponseFlags(
   if (stream_info.hasResponseFlag(StreamInfo::ResponseFlag::OverloadManager)) {
     common_access_log.mutable_response_flags()->set_overload_manager(true);
   }
+
+  if (stream_info.hasResponseFlag(StreamInfo::ResponseFlag::DnsResolutionFailed)) {
+    common_access_log.mutable_response_flags()->set_dns_resolution_failure(true);
+  }
 }
 
 void Utility::extractCommonAccessLogProperties(

source/extensions/common/dynamic_forward_proxy/dns_cache.h

@@ -21,7 +21,7 @@ class DnsHostInfo {
 
   /**
    * Returns the host's currently resolved address. This address may change periodically due to
-   * async re-resolution.
+   * async re-resolution. This address may be null in the case of failed resolution.
    */
   virtual Network::Address::InstanceConstSharedPtr address() const PURE;
 

source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc

@@ -24,11 +24,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 DnsResolutionFailure = "DNS resolution failure";
 };
 
 struct RcDetailsValues {
   const std::string DnsCacheOverflow = "dns_cache_overflow";
   const std::string PendingRequestOverflow = "dynamic_forward_proxy_pending_request_overflow";
+  const std::string DnsResolutionFailure = "dns_resolution_failure";
 };
 
 using CustomClusterType = envoy::config::cluster::v3::Cluster::CustomClusterType;
@@ -143,11 +145,15 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea
     ENVOY_STREAM_LOG(debug, "DNS cache entry already loaded, continuing", *decoder_callbacks_);
 
     auto const& host = config_->cache().getHost(headers.Host()->value().getStringView());
+    latchTime(decoder_callbacks_, DNS_END);
     if (host.has_value()) {
+      if (!host.value()->address()) {
+        onDnsResolutionFail();
+        return Http::FilterHeadersStatus::StopIteration;
+      }
       addHostAddressToFilterState(host.value()->address());
     }
 
-    latchTime(decoder_callbacks_, DNS_END);
     return Http::FilterHeadersStatus::Continue;
   }
   case LoadDnsCacheEntryStatus::Loading:
@@ -167,20 +173,12 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea
 
 void ProxyFilter::addHostAddressToFilterState(
     const Network::Address::InstanceConstSharedPtr& address) {
+  ASSERT(address); // null pointer checks must be done before calling this function.
 
   if (!config_->saveUpstreamAddress()) {
     return;
   }
 
-  // `onLoadDnsCacheComplete` is called by DNS cache on first resolution even if there was a
-  // resolution failure (null address). This check makes sure that we do not add null address to
-  // FilterState when this happens.
-  if (!address) {
-    ENVOY_STREAM_LOG(debug, "Cannot add address to filter state: invalid address",
-                     *decoder_callbacks_);
-    return;
-  }
-
   ENVOY_STREAM_LOG(trace, "Adding resolved host {} to filter state", *decoder_callbacks_,
                    address->asString());
 
@@ -195,6 +193,13 @@ void ProxyFilter::addHostAddressToFilterState(
                         StreamInfo::FilterState::LifeSpan::Request);
 }
 
+void ProxyFilter::onDnsResolutionFail() {
+  decoder_callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::DnsResolutionFailed);
+  decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable,
+                                     ResponseStrings::get().DnsResolutionFailure, nullptr,
+                                     absl::nullopt, RcDetails::get().DnsResolutionFailure);
+}
+
 void ProxyFilter::onLoadDnsCacheComplete(
     const Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) {
   ENVOY_STREAM_LOG(debug, "load DNS cache complete, continuing after adding resolved host: {}",
@@ -203,6 +208,10 @@ void ProxyFilter::onLoadDnsCacheComplete(
   ASSERT(circuit_breaker_ != nullptr);
   circuit_breaker_.reset();
 
+  if (!host_info->address()) {
+    onDnsResolutionFail();
+    return;
+  }
   addHostAddressToFilterState(host_info->address());
 
   decoder_callbacks_->continueDecoding();

source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h

@@ -65,6 +65,7 @@ class ProxyFilter
 
 private:
   void addHostAddressToFilterState(const Network::Address::InstanceConstSharedPtr& address);
+  void onDnsResolutionFail();
 
   const ProxyFilterConfigSharedPtr config_;
   Upstream::ClusterInfoConstSharedPtr cluster_info_;

test/extensions/access_loggers/grpc/grpc_access_log_utils_test.cc

@@ -48,6 +48,7 @@ TEST(UtilityResponseFlagsToAccessLogResponseFlagsTest, All) {
   common_access_log_expected.mutable_response_flags()->set_upstream_protocol_error(true);
   common_access_log_expected.mutable_response_flags()->set_no_cluster_found(true);
   common_access_log_expected.mutable_response_flags()->set_overload_manager(true);
+  common_access_log_expected.mutable_response_flags()->set_dns_resolution_failure(true);
 
   EXPECT_EQ(common_access_log_expected.DebugString(), common_access_log.DebugString());
 }

test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc

@@ -9,6 +9,8 @@
 #include "test/integration/http_integration.h"
 #include "test/integration/ssl_utility.h"
 
+using testing::HasSubstr;
+
 namespace Envoy {
 namespace {
 
@@ -195,6 +197,7 @@ TEST_P(ProxyFilterIntegrationTest, RequestWithBody) {
 // Currently if the first DNS resolution fails, the filter will continue with
 // a null address. Make sure this mode fails gracefully.
 TEST_P(ProxyFilterIntegrationTest, RequestWithUnknownDomain) {
+  useAccessLog("%RESPONSE_CODE_DETAILS%");
   initializeWithArgs();
   codec_client_ = makeHttpConnection(lookupPort("http"));
   const Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"},
@@ -205,6 +208,7 @@ TEST_P(ProxyFilterIntegrationTest, RequestWithUnknownDomain) {
   auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
   ASSERT_TRUE(response->waitForEndStream());
   EXPECT_EQ("503", response->headers().getStatusValue());
+  EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("dns_resolution_failure"));
 }
 
 // Verify that after we populate the cache and reload the cluster we reattach to the cache with

test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc

@@ -11,6 +11,7 @@
 #include "test/mocks/upstream/cluster_manager.h"
 #include "test/mocks/upstream/transport_socket_match.h"
 
+using testing::AnyNumber;
 using testing::AtLeast;
 using testing::Eq;
 using testing::InSequence;
@@ -380,7 +381,8 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, AddResolvedHostFilterStateMetadata
   Upstream::ResourceAutoIncDec* circuit_breakers_(
       new Upstream::ResourceAutoIncDec(pending_requests_));
 
-  EXPECT_CALL(callbacks_, streamInfo());
+  EXPECT_CALL(callbacks_, streamInfo()).Times(AnyNumber());
+  EXPECT_CALL(callbacks_, dispatcher()).Times(AnyNumber());
   auto& filter_state = callbacks_.streamInfo().filterState();
 
   InSequence s;
@@ -409,11 +411,7 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, AddResolvedHostFilterStateMetadata
             return host_info;
           }));
 
-  EXPECT_CALL(*host_info, address());
-
-  EXPECT_CALL(callbacks_, streamInfo());
-  EXPECT_CALL(callbacks_, streamInfo());
-  EXPECT_CALL(callbacks_, dispatcher());
+  EXPECT_CALL(*host_info, address()).Times(2).WillRepeatedly(Return(host_info->address_));
 
   // Host was resolved successfully, so continue filter iteration.
   EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));
@@ -431,7 +429,8 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetad
   Upstream::ResourceAutoIncDec* circuit_breakers_(
       new Upstream::ResourceAutoIncDec(pending_requests_));
 
-  EXPECT_CALL(callbacks_, streamInfo()).Times(testing::AnyNumber());
+  EXPECT_CALL(callbacks_, streamInfo()).Times(AnyNumber());
+  EXPECT_CALL(callbacks_, dispatcher()).Times(AnyNumber());
 
   // Pre-populate the filter state with an address.
   auto& filter_state = callbacks_.streamInfo().filterState();
@@ -468,11 +467,7 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetad
             return host_info;
           }));
 
-  EXPECT_CALL(*host_info, address());
-
-  EXPECT_CALL(callbacks_, dispatcher());
-  EXPECT_CALL(callbacks_, streamInfo());
-  EXPECT_CALL(callbacks_, streamInfo());
+  EXPECT_CALL(*host_info, address()).Times(2).WillRepeatedly(Return(host_info->address_));
 
   // Host was resolved successfully, so continue filter iteration.
   EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));
@@ -500,9 +495,8 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, IgnoreFilterStateMetadataNullAddre
   Upstream::ResourceAutoIncDec* circuit_breakers_(
       new Upstream::ResourceAutoIncDec(pending_requests_));
 
-  EXPECT_CALL(callbacks_, streamInfo());
-  auto& filter_state = callbacks_.streamInfo().filterState();
-
+  EXPECT_CALL(callbacks_, streamInfo()).Times(AnyNumber());
+  EXPECT_CALL(callbacks_, dispatcher()).Times(AnyNumber());
   InSequence s;
 
   // Setup test host
@@ -514,8 +508,6 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, IgnoreFilterStateMetadataNullAddre
   EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_())
       .WillOnce(Return(circuit_breakers_));
   EXPECT_CALL(*transport_socket_factory_, implementsSecureTransport()).WillOnce(Return(false));
-  EXPECT_CALL(callbacks_, streamInfo());
-  EXPECT_CALL(callbacks_, dispatcher());
   EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 80, _))
       .WillOnce(Invoke([&](absl::string_view, uint16_t, ProxyFilter::LoadDnsCacheEntryCallbacks&) {
         return MockLoadDnsCacheEntryResult{LoadDnsCacheEntryStatus::InCache, nullptr, host_info};
@@ -529,14 +521,13 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, IgnoreFilterStateMetadataNullAddre
           }));
 
   EXPECT_CALL(*host_info, address());
-  EXPECT_CALL(callbacks_, streamInfo());
-  EXPECT_CALL(callbacks_, dispatcher());
-
-  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));
-
-  // We do not expect FilterState to be populated
-  EXPECT_FALSE(
-      filter_state->hasData<StreamInfo::UpstreamAddress>(StreamInfo::UpstreamAddress::key()));
+  EXPECT_CALL(callbacks_,
+              sendLocalReply(Http::Code::ServiceUnavailable, Eq("DNS resolution failure"), _, _,
+                             Eq("dns_resolution_failure")));
+  EXPECT_CALL(callbacks_, encodeHeaders_(_, false));
+  EXPECT_CALL(callbacks_, encodeData(_, true));
+  EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
+            filter_->decodeHeaders(request_headers_, false));
 
   filter_->onDestroy();
 }