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 d05d073da4e35..077ba1e9ecab2 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: 12] +// [#next-free-field: 13] message CaresDnsResolverConfig { // A list of DNS resolver addresses. // :ref:`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}]; } diff --git a/changelogs/current/minor_behavior_changes/dns__shared-dnsresolover.rst b/changelogs/current/minor_behavior_changes/dns__shared-dnsresolover.rst new file mode 100644 index 0000000000000..c1400143e3723 --- /dev/null +++ b/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``. diff --git a/changelogs/current/new_features/cares__expose-cares-cache-setting.rst b/changelogs/current/new_features/cares__expose-cares-cache-setting.rst new file mode 100644 index 0000000000000..171cda3a01a9e --- /dev/null +++ b/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. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index aed651b960d09..56cbe8dc3c125 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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. diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index a3d61b0efbd4b..b01515fe7a101 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/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(cares, dispatcher, csv_or_error.value(), - api.rootScope()); + + 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; } 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> 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 ad5a91cb2c6d3..62dfd50b98e53 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.h +++ b/source/extensions/network/dns_resolver/cares/dns_impl.h @@ -211,6 +211,7 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable +#include #include #include @@ -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(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; }