diff --git a/api/envoy/extensions/network/dns_resolver/cares/v3/cares_dns_resolver.proto b/api/envoy/extensions/network/dns_resolver/cares/v3/cares_dns_resolver.proto index 077ba1e9ecab2..d05d073da4e35 100644 --- a/api/envoy/extensions/network/dns_resolver/cares/v3/cares_dns_resolver.proto +++ b/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 ` @@ -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}]; } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 3c54d5ee5ca68..7171b3d50b28a 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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. diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index f5668650ca146..eb1557f3c2f61 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/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( - 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>& 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(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> resolver_map_; }; // Register the CaresDnsResolverFactory diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.h b/source/extensions/network/dns_resolver/cares/dns_impl.h index 4950d7a1396e1..bd06d0bf41766 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.h +++ b/source/extensions/network/dns_resolver/cares/dns_impl.h @@ -211,7 +211,6 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable -#include #include #include @@ -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(dynamic_cast(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; }