dfp: Add a feature to disable DNS query timeout

api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto

@@ -136,8 +136,9 @@ message DnsCacheConfig {
   // 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 {}}];
+  // and are then retried using the standard refresh rates. Setting it to 0 will disable the DNS
+  // query timeout. Defaults to 5s if not set.
+  google.protobuf.Duration dns_query_timeout = 11 [(validate.rules).duration = {gte {}}];
 
   // Configuration to flush the DNS cache to long term storage.
   config.common.key_value.v3.KeyValueStoreConfig key_value_config = 13;

changelogs/current.yaml

@@ -8,6 +8,12 @@ minor_behavior_changes:
 - area: http2
   change: |
     Sets runtime guard ``envoy.reloadable_features.http2_use_oghttp2`` to true by default.
+- area: dfp
+  change: |
+    Setting :ref:`dns_query_timeout
+    <envoy_v3_api_field_extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig.dns_query_timeout>`
+    to 0 will disable the DNS query timeout. This behavior can be disabled by setting
+    ``envoy.restart_features.allow_disabling_dns_query_timeout`` flag to ``false``.
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*

mobile/library/cc/engine_builder.cc

@@ -861,6 +861,7 @@ std::unique_ptr<envoy::config::bootstrap::v3::Bootstrap> EngineBuilder::generate
       *(*runtime_values.mutable_fields())["restart_features"].mutable_struct_value();
   (*runtime_values.mutable_fields())["disallow_global_stats"].set_bool_value(true);
   (*runtime_values.mutable_fields())["enable_dfp_dns_trace"].set_bool_value(true);
+  (*runtime_values.mutable_fields())["allow_disabling_dns_query_timeout"].set_bool_value(true);
   for (auto& guard_and_value : restart_runtime_guards_) {
     (*restart_features.mutable_fields())[guard_and_value.first].set_bool_value(
         guard_and_value.second);

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

@@ -70,6 +70,9 @@ DnsCacheImpl::DnsCacheImpl(
   }
   enable_dfp_dns_trace_ = context.serverFactoryContext().runtime().snapshot().getBoolean(
       "envoy.enable_dfp_dns_trace", false);
+  allow_disabling_dns_query_timeout_ =
+      context.serverFactoryContext().runtime().snapshot().getBoolean(
+          "envoy.allow_disabling_dns_query_timeout", true);
 }
 
 DnsCacheImpl::~DnsCacheImpl() {
@@ -326,10 +329,14 @@ void DnsCacheImpl::forceRefreshHosts() {
       primary_host.second->active_query_->cancel(
           Network::ActiveDnsQuery::CancelReason::QueryAbandoned);
       primary_host.second->active_query_ = nullptr;
-      primary_host.second->timeout_timer_->disableTimer();
+      if (allow_disabling_dns_query_timeout_ && timeout_interval_.count() > 0) {
+        primary_host.second->timeout_timer_->disableTimer();
+      }
     }
 
-    ASSERT(!primary_host.second->timeout_timer_->enabled());
+    if (allow_disabling_dns_query_timeout_ && timeout_interval_.count() > 0) {
+      ASSERT(!primary_host.second->timeout_timer_->enabled());
+    }
     primary_host.second->refresh_timer_->enableTimer(std::chrono::milliseconds(0), nullptr);
     ENVOY_LOG_EVENT(debug, "force_refresh_host", "force refreshing host='{}'", primary_host.first);
   }
@@ -354,8 +361,10 @@ void DnsCacheImpl::stop() {
       primary_host.second->active_query_ = nullptr;
     }
 
-    primary_host.second->timeout_timer_->disableTimer();
-    ASSERT(!primary_host.second->timeout_timer_->enabled());
+    if (allow_disabling_dns_query_timeout_ && timeout_interval_.count() > 0) {
+      primary_host.second->timeout_timer_->disableTimer();
+      ASSERT(!primary_host.second->timeout_timer_->enabled());
+    }
     primary_host.second->refresh_timer_->disableTimer();
     ENVOY_LOG_EVENT(debug, "stop_host", "stop host='{}'", primary_host.first);
   }
@@ -367,8 +376,9 @@ void DnsCacheImpl::startResolve(const std::string& host, PrimaryHostInfo& host_i
   ASSERT(host_info.active_query_ == nullptr);
 
   stats_.dns_query_attempt_.inc();
-
-  host_info.timeout_timer_->enableTimer(timeout_interval_, nullptr);
+  if (allow_disabling_dns_query_timeout_ && timeout_interval_.count() > 0) {
+    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, absl::string_view details,
@@ -436,7 +446,9 @@ void DnsCacheImpl::finishResolve(const std::string& host,
 
   if (!from_cache) {
     first_resolve = !primary_host_info->host_info_->firstResolveComplete();
-    primary_host_info->timeout_timer_->disableTimer();
+    if (allow_disabling_dns_query_timeout_ && timeout_interval_.count() > 0) {
+      primary_host_info->timeout_timer_->disableTimer();
+    }
     primary_host_info->active_query_ = nullptr;
 
     if (status == Network::DnsResolver::ResolutionStatus::Failure) {

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h

@@ -273,6 +273,7 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable<Logger::Id::forward_proxy
   absl::optional<Network::Address::IpVersion>
       ip_version_to_remove_ ABSL_GUARDED_BY(ip_version_to_remove_lock_) = absl::nullopt;
   bool enable_dfp_dns_trace_;
+  bool allow_disabling_dns_query_timeout_{true};
 };
 
 } // namespace DynamicForwardProxy

test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc

@@ -53,6 +53,9 @@ class DnsCacheImplTest : public testing::Test, public Event::TestUsingSimulatedT
       }
     }
 
+    ON_CALL(context_.server_factory_context_.runtime_loader_.snapshot_,
+            getBoolean("envoy.allow_disabling_dns_query_timeout", true))
+        .WillByDefault(Return(allow_disabling_dns_query_timeout_));
     EXPECT_CALL(context_.server_factory_context_.dispatcher_, isThreadSafe)
         .WillRepeatedly(Return(true));
 
@@ -98,6 +101,7 @@ class DnsCacheImplTest : public testing::Test, public Event::TestUsingSimulatedT
   Registry::InjectFactory<Network::DnsResolverFactory> registered_dns_factory_;
   std::chrono::milliseconds configured_ttl_ = std::chrono::milliseconds(60000);
   std::chrono::milliseconds dns_ttl_ = std::chrono::milliseconds(6000);
+  bool allow_disabling_dns_query_timeout_{true};
 };
 
 MATCHER_P3(DnsHostInfoEquals, address, resolved_host, is_ip_address, "") {
@@ -710,13 +714,13 @@ TEST_F(DnsCacheImplTest, InlineResolve) {
 }
 
 // Resolve timeout.
-TEST_F(DnsCacheImplTest, ResolveTimeout) {
+TEST_F(DnsCacheImplTest, EnableResolveTimeout) {
   initialize();
   InSequence s;
 
   MockLoadDnsCacheEntryCallbacks callbacks;
   Network::DnsResolver::ResolveCb resolve_cb;
-  Event::MockTimer* resolve_timer =
+  Event::MockTimer* refresh_timer =
       new Event::MockTimer(&context_.server_factory_context_.dispatcher_);
   Event::MockTimer* timeout_timer =
       new Event::MockTimer(&context_.server_factory_context_.dispatcher_);
@@ -737,16 +741,41 @@ TEST_F(DnsCacheImplTest, ResolveTimeout) {
   EXPECT_CALL(update_callbacks_,
               onDnsResolutionComplete("foo.com:80", DnsHostInfoAddressIsNull(),
                                       Network::DnsResolver::ResolutionStatus::Failure));
-  // The resolve timeout will be the default TTL as there was no specific TTL
-  // overriding.
-  EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(configured_ttl_), _));
+  // The refresh timeout will be the default TTL as there was no specific TTL overriding.
+  EXPECT_CALL(*refresh_timer, enableTimer(std::chrono::milliseconds(configured_ttl_), _));
   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(context_.store_, "dns_cache.foo.dns_query_timeout")->value());
 }
 
+TEST_F(DnsCacheImplTest, DisableResolveTimeout) {
+  // Setting the DNS query timeout to 0 will disable the resolve timeout.
+  *config_.mutable_dns_query_timeout() = Protobuf::util::TimeUtil::SecondsToDuration(0);
+  initialize();
+  InSequence s;
+
+  MockLoadDnsCacheEntryCallbacks callbacks;
+  Network::DnsResolver::ResolveCb resolve_cb;
+  Event::MockTimer* refresh_timer =
+      new Event::MockTimer(&context_.server_factory_context_.dispatcher_);
+  (void)refresh_timer; // Silent unused warning.
+  Event::MockTimer* timeout_timer =
+      new Event::MockTimer(&context_.server_factory_context_.dispatcher_);
+  (void)timeout_timer; // Silent unused warning.
+  EXPECT_CALL(*resolver_, resolve("foo.com", _, _))
+      .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_)));
+  auto result = dns_cache_->loadDnsCacheEntry("foo.com", 80, false, 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_EQ(0,
+            TestUtility::findCounter(context_.store_, "dns_cache.foo.dns_query_timeout")->value());
+}
+
 // Resolve failure that returns no addresses.
 TEST_F(DnsCacheImplTest, ResolveFailure) {
   initialize();

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

@@ -463,7 +463,7 @@ TEST_P(ProxyFilterIntegrationTest, GetAddrInfoResolveTimeoutWithTrace) {
       name: envoy.network.dns_resolver.getaddrinfo
       typed_config:
         "@type": type.googleapis.com/envoy.extensions.network.dns_resolver.getaddrinfo.v3.GetAddrInfoDnsResolverConfig)EOF";
-  initializeWithArgs(1024, 1024, "", resolver_config, false, 0.000000001);
+  initializeWithArgs(1024, 1024, "", resolver_config, false, 0.001);
   codec_client_ = makeHttpConnection(lookupPort("http"));
 
   auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
@@ -494,7 +494,7 @@ TEST_P(ProxyFilterIntegrationTest, GetAddrInfoResolveTimeoutWithoutTrace) {
       name: envoy.network.dns_resolver.getaddrinfo
       typed_config:
         "@type": type.googleapis.com/envoy.extensions.network.dns_resolver.getaddrinfo.v3.GetAddrInfoDnsResolverConfig)EOF";
-  initializeWithArgs(1024, 1024, "", resolver_config, false, 0.000000001);
+  initializeWithArgs(1024, 1024, "", resolver_config, false, 0.001);
   codec_client_ = makeHttpConnection(lookupPort("http"));
 
   auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
@@ -505,6 +505,32 @@ TEST_P(ProxyFilterIntegrationTest, GetAddrInfoResolveTimeoutWithoutTrace) {
               HasSubstr("dns_resolution_failure{resolve_timeout}"));
 }
 
+TEST_P(ProxyFilterIntegrationTest, DisableResolveTimeout) {
+  useAccessLog("%RESPONSE_CODE_DETAILS%");
+
+  setDownstreamProtocol(Http::CodecType::HTTP2);
+  setUpstreamProtocol(Http::CodecType::HTTP2);
+
+  config_helper_.prependFilter(fmt::format(R"EOF(
+  name: stream-info-to-headers-filter
+)EOF"));
+
+  upstream_tls_ = false; // upstream creation doesn't handle autonomous_upstream_
+  autonomous_upstream_ = true;
+  std::string resolver_config = R"EOF(
+    typed_dns_resolver_config:
+      name: envoy.network.dns_resolver.getaddrinfo
+      typed_config:
+        "@type": type.googleapis.com/envoy.extensions.network.dns_resolver.getaddrinfo.v3.GetAddrInfoDnsResolverConfig)EOF";
+  initializeWithArgs(1024, 1024, "", resolver_config, false, /* dns_query_timeout= */ 0);
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+
+  ASSERT_TRUE(response->waitForEndStream());
+  EXPECT_EQ("200", response->headers().getStatusValue());
+}
+
 // TODO(yanavlasov) Enable per #26642
 #ifndef ENVOY_ENABLE_UHV
 TEST_P(ProxyFilterIntegrationTest, FailOnEmptyHostHeader) {

test/extensions/filters/http/dynamic_forward_proxy/test_resolver.h

@@ -48,7 +48,8 @@ class TestResolver : public GetAddrInfoDnsResolver {
           }
           // Add a dummy trace for test coverage.
           query->addTrace(100);
-          pending_queries_.push_back(PendingQueryInfo{std::move(query), absl::nullopt});
+          pending_queries_.push_back(
+              PendingQueryInfo{std::move(query), config_.num_retries().value() + 1});
         });
     return raw_new_query;
   }