Revert #45073 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: 13]
+// [#next-free-field: 12]
 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,14 +113,4 @@ 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}];
 }

source/common/runtime/runtime_features.cc

@@ -151,7 +151,6 @@ 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,7 +23,6 @@
 #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"
 
@@ -37,17 +36,7 @@ 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(
@@ -74,8 +63,7 @@ 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_)),
-      max_cache_ttl_(getQcacheMaxTtl(config)) {
+      scope_(root_scope.createScope("dns.cares.")), stats_(generateCaresDnsResolverStats(*scope_)) {
   AresOptions options = defaultAresOptions();
   initializeChannel(&options.options_, options.optmask_);
 
@@ -163,11 +151,9 @@ DnsResolverImpl::AresOptions DnsResolverImpl::defaultAresOptions() {
     options.options_.ednspsz = edns0_max_payload_size_;
   }
 
+  // Disable query cache by default.
   options.optmask_ |= ARES_OPT_QUERY_CACHE;
-  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_;
+  options.options_.qcache_max_ttl = 0;
 
   return options;
 }
@@ -681,19 +667,6 @@ 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());
@@ -705,25 +678,8 @@ class CaresDnsResolverFactory : public DnsResolverFactory,
     }
     auto csv_or_error = DnsResolverImpl::maybeBuildResolversCsv(resolvers);
     RETURN_IF_NOT_OK(csv_or_error.status());
-
-    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;
+    return std::make_shared<Network::DnsResolverImpl>(cares, dispatcher, csv_or_error.value(),
+                                                      api.rootScope());
   }
 
   void initialize() override {
@@ -748,7 +704,6 @@ 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,7 +211,6 @@ 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,7 +34,6 @@ 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,5 +1,4 @@
 #include <ares.h>
-#include <sys/types.h>
 
 #include <list>
 #include <memory>
@@ -33,7 +32,6 @@
 #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"
 
@@ -440,17 +438,6 @@ 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;
-    std::ignore = 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) {
@@ -2277,113 +2264,6 @@ 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; }