c-ares: make qcache_max_ttl configurable

api/envoy/extensions/network/dns_resolver/cares/v3/cares_dns_resolver.proto

@@ -21,7 +21,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 // [#extension: envoy.network.dns_resolver.cares]
 
 // Configuration for c-ares DNS resolver.
-// [#next-free-field: 12]
+// [#next-free-field: 13]
 message CaresDnsResolverConfig {
   // A list of DNS resolver addresses.
   // :ref:`use_resolvers_as_fallback <envoy_v3_api_field_extensions.network.dns_resolver.cares.v3.CaresDnsResolverConfig.use_resolvers_as_fallback>`
@@ -113,4 +113,14 @@ message CaresDnsResolverConfig {
   //
   // Default is false.
   bool reinit_channel_on_timeout = 11;
+
+  // The maximum duration (in seconds) for which DNS responses will be cached by c-ares.
+  //
+  // If set to a non-zero value, the query cache is enabled and will respect the
+  // TTL provided in the DNS response, up to this maximum limit.
+  //
+  // .. note::
+  //   While the underlying c-ares library defaults to 1 hour, Envoy's default
+  //   for this field is 0, which disables the query cache entirely.
+  google.protobuf.UInt32Value qcache_max_ttl = 12 [(validate.rules).uint32 = {gte: 0}];
 }

changelogs/current/minor_behavior_changes/dns__shared-dnsresolover.rst

@@ -0,0 +1,3 @@
+Share DNSResolver if c-ares config is the same. This is so that qcache can be enabled and shared
+across clusters. This behavior can be reverted by setting the runtime guard
+``envoy.restart_features.shared_cares_dns_resolver`` to ``false``.

changelogs/current/new_features/cares__expose-cares-cache-setting.rst

@@ -0,0 +1 @@
+Added qcache_max_ttl to CaresDnsResolverConfig. Default is 0 which means disabled to preserve existing behavior.

source/common/runtime/runtime_features.cc

@@ -127,6 +127,7 @@ RUNTIME_GUARD(envoy_reloadable_features_websocket_enable_timeout_on_upgrade_resp
 RUNTIME_GUARD(envoy_reloadable_features_xds_failover_to_primary_enabled);
 RUNTIME_GUARD(envoy_reloadable_features_xds_legacy_delta_skip_subsequent_node);
 RUNTIME_GUARD(envoy_restart_features_raise_file_limits);
+RUNTIME_GUARD(envoy_restart_features_shared_cares_dns_resolver);
 RUNTIME_GUARD(envoy_restart_features_validate_http3_pseudo_headers);
 RUNTIME_GUARD(envoy_restart_features_worker_threads_watchdog_fix);
 // Begin false flags. Most of them should come with a TODO to flip true.

source/extensions/network/dns_resolver/cares/dns_impl.cc

@@ -23,6 +23,7 @@
 #include "source/common/protobuf/utility.h"
 #include "source/common/runtime/runtime_features.h"
 
+#include "absl/container/flat_hash_map.h"
 #include "absl/strings/str_join.h"
 #include "ares.h"
 
@@ -36,7 +37,17 @@ namespace {
 // to their original values: 5 second timeout and 4 retry attempts.
 // Ref: https://github.com/envoyproxy/envoy/issues/35117
 constexpr uint32_t DEFAULT_QUERY_TIMEOUT_SECONDS = 5;
+constexpr uint32_t DEFAULT_QCACHE_MAX_TTL = 0;
 constexpr uint32_t DEFAULT_QUERY_TRIES = 4;
+
+uint32_t getQcacheMaxTtl(
+    const envoy::extensions::network::dns_resolver::cares::v3::CaresDnsResolverConfig& config) {
+  if (!config.has_qcache_max_ttl()) {
+    return DEFAULT_QCACHE_MAX_TTL;
+  }
+
+  return config.qcache_max_ttl().value();
+}
 } // namespace
 
 DnsResolverImpl::DnsResolverImpl(
@@ -63,7 +74,8 @@ DnsResolverImpl::DnsResolverImpl(
               : std::chrono::milliseconds::zero()),
       reinit_channel_on_timeout_(config.reinit_channel_on_timeout()), resolvers_csv_(resolvers_csv),
       filter_unroutable_families_(config.filter_unroutable_families()),
-      scope_(root_scope.createScope("dns.cares.")), stats_(generateCaresDnsResolverStats(*scope_)) {
+      scope_(root_scope.createScope("dns.cares.")), stats_(generateCaresDnsResolverStats(*scope_)),
+      max_cache_ttl_(getQcacheMaxTtl(config)) {
   AresOptions options = defaultAresOptions();
   initializeChannel(&options.options_, options.optmask_);
 
@@ -151,9 +163,11 @@ DnsResolverImpl::AresOptions DnsResolverImpl::defaultAresOptions() {
     options.options_.ednspsz = edns0_max_payload_size_;
   }
 
-  // Disable query cache by default.
   options.optmask_ |= ARES_OPT_QUERY_CACHE;
-  options.options_.qcache_max_ttl = 0;
+  if (max_cache_ttl_) {
+    ENVOY_LOG(debug, "c-ares query cached enabled: max ttl {}", max_cache_ttl_);
+  }
+  options.options_.qcache_max_ttl = max_cache_ttl_;
 
   return options;
 }
@@ -667,6 +681,19 @@ class CaresDnsResolverFactory : public DnsResolverFactory,
     // Only c-ares DNS factory will call into this function.
     // Directly unpack the typed config to a c-ares object.
     RETURN_IF_NOT_OK(Envoy::MessageUtil::unpackTo(typed_dns_resolver_config.typed_config(), cares));
+    std::size_t key = 0;
+    if (Runtime::runtimeFeatureEnabled("envoy.restart_features.shared_cares_dns_resolver")) {
+      key = MessageUtil::hash(cares);
+      const auto it = resolver_map_.find(key);
+      if (it != resolver_map_.end()) {
+        auto resolver = it->second.lock();
+        if (resolver) {
+          ENVOY_LOG(trace, "found existing resolvers: {}", key);
+          return resolver;
+        }
+      }
+    }
+
     if (!cares.resolvers().empty()) {
       const auto& resolver_addrs = cares.resolvers();
       resolvers.reserve(resolver_addrs.size());
@@ -678,8 +705,25 @@ class CaresDnsResolverFactory : public DnsResolverFactory,
     }
     auto csv_or_error = DnsResolverImpl::maybeBuildResolversCsv(resolvers);
     RETURN_IF_NOT_OK(csv_or_error.status());
-    return std::make_shared<Network::DnsResolverImpl>(cares, dispatcher, csv_or_error.value(),
-                                                      api.rootScope());
+
+    auto resolver = std::make_shared<Network::DnsResolverImpl>(
+        cares, dispatcher, csv_or_error.value(), api.rootScope());
+    if (Runtime::runtimeFeatureEnabled("envoy.restart_features.shared_cares_dns_resolver")) {
+      // clean up any nil resolver in the map so it doesn't keep growing
+      auto original_size = resolver_map_.size();
+      absl::erase_if(
+          resolver_map_,
+          [](const std::pair<const std::size_t, std::weak_ptr<Network::DnsResolver>>& entry) {
+            return entry.second.lock() == nullptr;
+          });
+      if (resolver_map_.size() < original_size) {
+        ENVOY_LOG(trace, "cleaned up {} entries in resolver_map_",
+                  original_size - resolver_map_.size());
+      }
+      resolver_map_[key] = resolver;
+      ENVOY_LOG(trace, "resolver_map_ size after adding: {}", resolver_map_.size());
+    }
+    return resolver;
   }
 
   void initialize() override {
@@ -704,6 +748,7 @@ class CaresDnsResolverFactory : public DnsResolverFactory,
 private:
   bool ares_library_initialized_ ABSL_GUARDED_BY(mutex_){false};
   absl::Mutex mutex_;
+  mutable absl::flat_hash_map<std::size_t, std::weak_ptr<Network::DnsResolver>> resolver_map_;
 };
 
 // Register the CaresDnsResolverFactory

source/extensions/network/dns_resolver/cares/dns_impl.h

@@ -211,6 +211,7 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
   const bool filter_unroutable_families_;
   Stats::ScopeSharedPtr scope_;
   CaresDnsResolverStats stats_;
+  const uint32_t max_cache_ttl_; // in seconds
 };
 
 DECLARE_FACTORY(CaresDnsResolverFactory);

test/extensions/network/dns_resolver/cares/BUILD

@@ -34,6 +34,7 @@ envoy_cc_test(
         "//test/mocks/network:network_mocks",
         "//test/test_common:environment_lib",
         "//test/test_common:network_utility_lib",
+        "//test/test_common:test_runtime_lib",
         "//test/test_common:threadsafe_singleton_injector_lib",
         "//test/test_common:utility_lib",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",

test/extensions/network/dns_resolver/cares/dns_impl_test.cc

@@ -1,4 +1,5 @@
 #include <ares.h>
+#include <sys/types.h>
 
 #include <list>
 #include <memory>
@@ -32,6 +33,7 @@
 #include "test/test_common/environment.h"
 #include "test/test_common/network_utility.h"
 #include "test/test_common/printers.h"
+#include "test/test_common/test_runtime.h"
 #include "test/test_common/threadsafe_singleton_injector.h"
 #include "test/test_common/utility.h"
 
@@ -438,6 +440,17 @@ class DnsImplConstructor : public testing::Test {
   Api::ApiPtr api_;
   Event::DispatcherPtr dispatcher_;
   envoy::config::core::v3::DnsResolverOptions dns_resolver_options_;
+
+  envoy::config::core::v3::TypedExtensionConfig getCaresDnsResolverConfig(uint32_t qcache_max_ttl) {
+    envoy::extensions::network::dns_resolver::cares::v3::CaresDnsResolverConfig cares;
+    cares.mutable_dns_resolver_options()->MergeFrom(dns_resolver_options_);
+    cares.mutable_qcache_max_ttl()->set_value(qcache_max_ttl);
+
+    envoy::config::core::v3::TypedExtensionConfig typed_dns_resolver_config;
+    typed_dns_resolver_config.mutable_typed_config()->PackFrom(cares);
+    typed_dns_resolver_config.set_name(std::string(Network::CaresDnsResolver));
+    return typed_dns_resolver_config;
+  }
 };
 
 TEST_F(DnsImplConstructor, SupportsCustomResolvers) {
@@ -2264,6 +2277,113 @@ TEST_F(DnsImplConstructor, VerifyCustomTimeoutAndTries) {
   ares_destroy_options(&opts);
 }
 
+TEST_F(DnsImplConstructor, VerifyCustomQcacheMaxTtl) {
+  auto typed_dns_resolver_config = getCaresDnsResolverConfig(123);
+
+  Network::DnsResolverFactory& dns_resolver_factory =
+      createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config);
+  auto resolver =
+      dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config)
+          .value();
+
+  auto peer = std::make_unique<DnsResolverImplPeer>(dynamic_cast<DnsResolverImpl*>(resolver.get()));
+  ares_options opts{};
+  int optmask = 0;
+  EXPECT_EQ(ARES_SUCCESS, ares_save_options(peer->channel(), &opts, &optmask));
+  EXPECT_TRUE((optmask & ARES_OPT_QUERY_CACHE) == ARES_OPT_QUERY_CACHE);
+  EXPECT_EQ(123, opts.qcache_max_ttl);
+  ares_destroy_options(&opts);
+}
+
+TEST_F(DnsImplConstructor, ReusesResolverForIdenticalConfig) {
+  auto typed_dns_resolver_config = getCaresDnsResolverConfig(0);
+  Network::DnsResolverFactory& dns_resolver_factory =
+      createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config);
+
+  auto resolver1 =
+      dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config)
+          .value();
+  auto resolver2 =
+      dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config)
+          .value();
+
+  EXPECT_EQ(resolver1.get(), resolver2.get());
+}
+
+TEST_F(DnsImplConstructor, DoesNotReuseResolverForIdenticalConfigWhenFeatureDisabled) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({{"envoy.restart_features.shared_cares_dns_resolver", "false"}});
+
+  auto typed_dns_resolver_config = getCaresDnsResolverConfig(0);
+  Network::DnsResolverFactory& dns_resolver_factory =
+      createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config);
+
+  auto resolver1 =
+      dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config)
+          .value();
+  auto resolver2 =
+      dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config)
+          .value();
+
+  EXPECT_NE(resolver1.get(), resolver2.get());
+}
+
+TEST_F(DnsImplConstructor, DoesNotReuseResolverForDifferentConfig) {
+  auto typed_dns_resolver_config1 = getCaresDnsResolverConfig(67);
+  auto typed_dns_resolver_config2 = getCaresDnsResolverConfig(123);
+
+  Network::DnsResolverFactory& dns_resolver_factory =
+      createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config1);
+
+  auto resolver1 =
+      dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config1)
+          .value();
+  auto resolver2 =
+      dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config2)
+          .value();
+
+  EXPECT_NE(resolver1.get(), resolver2.get());
+}
+
+TEST_F(DnsImplConstructor, CleansExpiredResolverBeforeReinsertingIdenticalConfig) {
+  auto typed_dns_resolver_config = getCaresDnsResolverConfig(1234);
+
+  Network::DnsResolverFactory& dns_resolver_factory =
+      createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config);
+
+  DnsResolver* first_resolver = nullptr;
+  {
+    auto resolver1 =
+        dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config)
+            .value();
+    // Save the pointer only for identity comparison after resolver1 is destroyed.
+    first_resolver = resolver1.get();
+  }
+
+  auto typed_dns_resolver_config2 = getCaresDnsResolverConfig(5678);
+  // Create another resolver with a different config to trigger eviction of the first resolver from
+  // the resolver map.
+  auto resolver2 =
+      dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config2)
+          .value();
+
+  // This is a dummy resolver so if memory is immediately reused, this will take the memory released
+  // by the first resolver.
+  auto typed_dns_resolver_config3 = getCaresDnsResolverConfig(890);
+  auto resolver3 =
+      dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config3)
+          .value();
+
+  // Create a forth resolver with the same config as the first resolver and verify the first
+  // resolver is not reused, which proves that the first resolver was evicted from the resolver map.
+  auto resolver4 =
+      dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config)
+          .value();
+
+  EXPECT_NE(resolver2.get(), resolver4.get());
+  EXPECT_NE(first_resolver, resolver4.get());
+}
+
 class DnsImplAresFlagsForMaxUdpQueriesinTest : public DnsImplTest {
 protected:
   bool tcpOnly() const override { return false; }