From d20706b04ea479638a22636ea08d805c8d218a04 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 24 Jun 2022 19:55:10 +0000 Subject: [PATCH 01/17] Only call ares_library_init()/cleanup() routines in process_wide() if ares extension is linked in. Signed-off-by: Yanjun Xiang --- envoy/network/dns_resolver.h | 2 ++ source/exe/BUILD | 1 + source/exe/process_wide.cc | 14 ++++++++++++-- .../network/dns_resolver/cares/dns_impl.cc | 3 +++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/envoy/network/dns_resolver.h b/envoy/network/dns_resolver.h index 2a326f152fe83..e498614c3f8ec 100644 --- a/envoy/network/dns_resolver.h +++ b/envoy/network/dns_resolver.h @@ -26,6 +26,8 @@ class DnsResolverFactory : public Config::TypedFactory { const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) const PURE; std::string category() const override { return std::string(DnsResolverCategory); } + virtual void init() {} + virtual void cleanup() {} }; } // namespace Network diff --git a/source/exe/BUILD b/source/exe/BUILD index 994bc24231636..cf2b9462e6aca 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -143,6 +143,7 @@ envoy_cc_library( hdrs = ["process_wide.h"], external_deps = ["ares"], deps = [ + "//envoy/network:dns_resolver_interface", "//source/common/common:assert_lib", "//source/common/event:libevent_lib", "//source/common/http/http2:nghttp2_lib", diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index df421084a21b1..a0a66fd30c990 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -1,5 +1,6 @@ #include "source/exe/process_wide.h" +#include "envoy/network/dns_resolver.h" #include "source/common/common/assert.h" #include "source/common/event/libevent.h" #include "source/common/http/http2/nghttp2.h" @@ -30,7 +31,12 @@ ProcessWide::ProcessWide() { if (init_data.count_++ == 0) { // TODO(mattklein123): Audit the following as not all of these have to be re-initialized in the // edge case where something does init/destroy/init/destroy. - ares_library_init(ARES_LIB_INIT_ALL); + + // Initialize c-ares library if it is linked in. + if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName + (std::string(Network::CaresDnsResolver), true)) { + dns_factory->init(); + } Event::Libevent::Global::initialize(); Envoy::Server::validateProtoDescriptors(); Http::Http2::initializeNghttp2Logging(); @@ -58,7 +64,11 @@ ProcessWide::~ProcessWide() { ASSERT(init_data.count_ > 0); if (--init_data.count_ == 0) { - ares_library_cleanup(); + // Cleanup c-ares library if it is linked in. + if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName + (std::string(Network::CaresDnsResolver), true)) { + dns_factory->cleanup(); + } } } diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index 35c147083be25..de0745c129c5c 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -521,6 +521,9 @@ class CaresDnsResolverFactory : public DnsResolverFactory { } return std::make_shared(cares, dispatcher, resolvers); } + + void init() override { ares_library_init(ARES_LIB_INIT_ALL); } + void cleanup() override { ares_library_cleanup(); } }; // Register the CaresDnsResolverFactory From f9751b7b15ba147bc5a26ed652745dc82fed4274 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sat, 25 Jun 2022 12:19:34 +0000 Subject: [PATCH 02/17] remove ares dependancy from process_wide Signed-off-by: Yanjun Xiang --- source/exe/BUILD | 1 - source/exe/process_wide.cc | 2 -- 2 files changed, 3 deletions(-) diff --git a/source/exe/BUILD b/source/exe/BUILD index cf2b9462e6aca..7059a446017ff 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -141,7 +141,6 @@ envoy_cc_library( name = "process_wide_lib", srcs = ["process_wide.cc"], hdrs = ["process_wide.h"], - external_deps = ["ares"], deps = [ "//envoy/network:dns_resolver_interface", "//source/common/common:assert_lib", diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index a0a66fd30c990..d55bc8edbf7fd 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -6,8 +6,6 @@ #include "source/common/http/http2/nghttp2.h" #include "source/server/proto_descriptors.h" -#include "ares.h" - namespace Envoy { namespace { From c4aaa4bb05ce9d4bea1437a87d77d0c04b3a72b9 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sun, 26 Jun 2022 16:22:40 +0000 Subject: [PATCH 03/17] fix format error Signed-off-by: Yanjun Xiang --- source/exe/process_wide.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index d55bc8edbf7fd..343644a8e4eb5 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -1,6 +1,7 @@ #include "source/exe/process_wide.h" #include "envoy/network/dns_resolver.h" + #include "source/common/common/assert.h" #include "source/common/event/libevent.h" #include "source/common/http/http2/nghttp2.h" @@ -31,8 +32,8 @@ ProcessWide::ProcessWide() { // edge case where something does init/destroy/init/destroy. // Initialize c-ares library if it is linked in. - if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName - (std::string(Network::CaresDnsResolver), true)) { + if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName( + std::string(Network::CaresDnsResolver), true)) { dns_factory->init(); } Event::Libevent::Global::initialize(); @@ -63,8 +64,8 @@ ProcessWide::~ProcessWide() { ASSERT(init_data.count_ > 0); if (--init_data.count_ == 0) { // Cleanup c-ares library if it is linked in. - if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName - (std::string(Network::CaresDnsResolver), true)) { + if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName( + std::string(Network::CaresDnsResolver), true)) { dns_factory->cleanup(); } } From 95d221628db253d4a05371439e122286587ae062 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sun, 26 Jun 2022 17:01:08 +0000 Subject: [PATCH 04/17] addressing comments Signed-off-by: Yanjun Xiang --- envoy/network/dns_resolver.h | 1 - source/exe/process_wide.cc | 6 ------ .../network/dns_resolver/cares/dns_impl.cc | 17 +++++++++++++++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/envoy/network/dns_resolver.h b/envoy/network/dns_resolver.h index e498614c3f8ec..f9ad71a79cf1e 100644 --- a/envoy/network/dns_resolver.h +++ b/envoy/network/dns_resolver.h @@ -26,7 +26,6 @@ class DnsResolverFactory : public Config::TypedFactory { const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) const PURE; std::string category() const override { return std::string(DnsResolverCategory); } - virtual void init() {} virtual void cleanup() {} }; diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index 343644a8e4eb5..082e30f9b9802 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -30,12 +30,6 @@ ProcessWide::ProcessWide() { if (init_data.count_++ == 0) { // TODO(mattklein123): Audit the following as not all of these have to be re-initialized in the // edge case where something does init/destroy/init/destroy. - - // Initialize c-ares library if it is linked in. - if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName( - std::string(Network::CaresDnsResolver), true)) { - dns_factory->init(); - } Event::Libevent::Global::initialize(); Envoy::Server::validateProtoDescriptors(); Http::Http2::initializeNghttp2Logging(); diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index de0745c129c5c..5289ad08c9c16 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -495,6 +495,7 @@ DnsResolverImpl::AddrInfoPendingResolution::availableInterfaces() { // c-ares DNS resolver factory class CaresDnsResolverFactory : public DnsResolverFactory { public: + CaresDnsResolverFactory() : ares_library_initialized_(false) {} std::string name() const override { return std::string(CaresDnsResolver); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { @@ -507,6 +508,11 @@ class CaresDnsResolverFactory : public DnsResolverFactory { typed_dns_resolver_config) const override { envoy::extensions::network::dns_resolver::cares::v3::CaresDnsResolverConfig cares; std::vector resolvers; + // Initialize c-ares library in case first time. + if (!ares_library_initialized_) { + ares_library_init(ARES_LIB_INIT_ALL); + ares_library_initialized_ = true; + } ASSERT(dispatcher.isThreadSafe()); // Only c-ares DNS factory will call into this function. @@ -522,8 +528,15 @@ class CaresDnsResolverFactory : public DnsResolverFactory { return std::make_shared(cares, dispatcher, resolvers); } - void init() override { ares_library_init(ARES_LIB_INIT_ALL); } - void cleanup() override { ares_library_cleanup(); } + void cleanup() override { + // Cleanup c-ares library if initialized. + if (ares_library_initialized_) { + ares_library_cleanup(); + } + } + +private: + mutable bool ares_library_initialized_; }; // Register the CaresDnsResolverFactory From d425d911eb940d16c34f6c5f987714eed20ce619 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sun, 26 Jun 2022 19:36:23 +0000 Subject: [PATCH 05/17] addressing comments Signed-off-by: Yanjun Xiang --- envoy/network/dns_resolver.h | 1 + .../common/network/dns_resolver/dns_factory_util.cc | 7 +++++-- .../network/dns_resolver/cares/dns_impl.cc | 12 +++++++----- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/envoy/network/dns_resolver.h b/envoy/network/dns_resolver.h index f9ad71a79cf1e..e498614c3f8ec 100644 --- a/envoy/network/dns_resolver.h +++ b/envoy/network/dns_resolver.h @@ -26,6 +26,7 @@ class DnsResolverFactory : public Config::TypedFactory { const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) const PURE; std::string category() const override { return std::string(DnsResolverCategory); } + virtual void init() {} virtual void cleanup() {} }; diff --git a/source/common/network/dns_resolver/dns_factory_util.cc b/source/common/network/dns_resolver/dns_factory_util.cc index d8d1712cab956..d030d1e0fe111 100644 --- a/source/common/network/dns_resolver/dns_factory_util.cc +++ b/source/common/network/dns_resolver/dns_factory_util.cc @@ -79,8 +79,11 @@ void handleLegacyDnsResolverData( Network::DnsResolverFactory& createDnsResolverFactoryFromTypedConfig( const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) { ENVOY_LOG_MISC(debug, "create DNS resolver type: {}", typed_dns_resolver_config.name()); - return Config::Utility::getAndCheckFactory( - typed_dns_resolver_config); + auto& factory = + Config::Utility::getAndCheckFactory(typed_dns_resolver_config); + // Perform factory initialization. + factory.init(); + return factory; } // Create the default DNS resolver factory. apple for MacOS or c-ares for all others. diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index 5289ad08c9c16..7bb820fde8305 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -508,11 +508,6 @@ class CaresDnsResolverFactory : public DnsResolverFactory { typed_dns_resolver_config) const override { envoy::extensions::network::dns_resolver::cares::v3::CaresDnsResolverConfig cares; std::vector resolvers; - // Initialize c-ares library in case first time. - if (!ares_library_initialized_) { - ares_library_init(ARES_LIB_INIT_ALL); - ares_library_initialized_ = true; - } ASSERT(dispatcher.isThreadSafe()); // Only c-ares DNS factory will call into this function. @@ -528,6 +523,13 @@ class CaresDnsResolverFactory : public DnsResolverFactory { return std::make_shared(cares, dispatcher, resolvers); } + void init() override { + // Initialize c-ares library in case first time. + if (!ares_library_initialized_) { + ares_library_initialized_ = true; + ares_library_init(ARES_LIB_INIT_ALL); + } + } void cleanup() override { // Cleanup c-ares library if initialized. if (ares_library_initialized_) { From d22cb618ea8e5acb70e0ee8ae4f40282bf359e7e Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Mon, 27 Jun 2022 13:27:18 +0000 Subject: [PATCH 06/17] fixing CI error Signed-off-by: Yanjun Xiang --- source/extensions/network/dns_resolver/cares/dns_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index 7bb820fde8305..ad9c7ba22f318 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -495,7 +495,6 @@ DnsResolverImpl::AddrInfoPendingResolution::availableInterfaces() { // c-ares DNS resolver factory class CaresDnsResolverFactory : public DnsResolverFactory { public: - CaresDnsResolverFactory() : ares_library_initialized_(false) {} std::string name() const override { return std::string(CaresDnsResolver); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { @@ -538,7 +537,7 @@ class CaresDnsResolverFactory : public DnsResolverFactory { } private: - mutable bool ares_library_initialized_; + mutable bool ares_library_initialized_{false}; }; // Register the CaresDnsResolverFactory From f37c515b1c334005fa89f1ad73b76d432a3617f9 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Mon, 27 Jun 2022 23:45:45 +0000 Subject: [PATCH 07/17] adding mutex to protect ares_library_init() operation race Signed-off-by: Yanjun Xiang --- source/extensions/network/dns_resolver/cares/dns_impl.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index ad9c7ba22f318..dca6c3fbff86c 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -524,6 +524,8 @@ class CaresDnsResolverFactory : public DnsResolverFactory { void init() override { // Initialize c-ares library in case first time. + absl::Mutex mutex; + absl::MutexLock lock(&mutex); if (!ares_library_initialized_) { ares_library_initialized_ = true; ares_library_init(ARES_LIB_INIT_ALL); From 838aa9cae0e997ef20f6d5ce0d526829d9d2e799 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 28 Jun 2022 20:05:51 +0000 Subject: [PATCH 08/17] addressing comments Signed-off-by: Yanjun Xiang --- envoy/network/dns_resolver.h | 12 ++++++++++++ source/exe/process_wide.cc | 3 ++- .../network/dns_resolver/cares/dns_impl.cc | 8 +++++--- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/envoy/network/dns_resolver.h b/envoy/network/dns_resolver.h index e498614c3f8ec..96c5b7975948f 100644 --- a/envoy/network/dns_resolver.h +++ b/envoy/network/dns_resolver.h @@ -26,7 +26,19 @@ class DnsResolverFactory : public Config::TypedFactory { const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) const PURE; std::string category() const override { return std::string(DnsResolverCategory); } + + /* + * Initialize the related data for this type of DNS resolver. + * For some DNS resolvers, like c-ares, there are some specific data structure + * needs to be initialized before using it to resolve target. + */ virtual void init() {} + + /* + * Cleanup the related data for this type of DNS resolver. + * For some DNS resolvers, like c-ares, there are some specific data structure + * needs to be cleaned up before terminates Envoy. + */ virtual void cleanup() {} }; diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index 082e30f9b9802..bb5a8f451cc2a 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -57,7 +57,8 @@ ProcessWide::~ProcessWide() { ASSERT(init_data.count_ > 0); if (--init_data.count_ == 0) { - // Cleanup c-ares library if it is linked in. + // The c-ares library init is done in a thread-safe way when it is actually used for resolving. + // Clean it up here in case it is ever used. if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName( std::string(Network::CaresDnsResolver), true)) { dns_factory->cleanup(); diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index dca6c3fbff86c..0ccd114b090db 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -524,8 +524,7 @@ class CaresDnsResolverFactory : public DnsResolverFactory { void init() override { // Initialize c-ares library in case first time. - absl::Mutex mutex; - absl::MutexLock lock(&mutex); + absl::MutexLock lock(&mutex_); if (!ares_library_initialized_) { ares_library_initialized_ = true; ares_library_init(ARES_LIB_INIT_ALL); @@ -533,13 +532,16 @@ class CaresDnsResolverFactory : public DnsResolverFactory { } void cleanup() override { // Cleanup c-ares library if initialized. + absl::MutexLock lock(&mutex_); if (ares_library_initialized_) { + ares_library_initialized_ = false; ares_library_cleanup(); } } private: - mutable bool ares_library_initialized_{false}; + bool ares_library_initialized_ ABSL_GUARDED_BY(mutex_){false}; + absl::Mutex mutex_; }; // Register the CaresDnsResolverFactory From e4576ac96b7f56e1695ee4ae610f1d36a63fcd97 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 28 Jun 2022 23:06:43 +0000 Subject: [PATCH 09/17] addressing comments Signed-off-by: Yanjun Xiang --- envoy/network/dns_resolver.h | 10 +++++----- source/common/network/dns_resolver/dns_factory_util.cc | 2 +- source/exe/process_wide.cc | 2 +- .../extensions/network/dns_resolver/cares/dns_impl.cc | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/envoy/network/dns_resolver.h b/envoy/network/dns_resolver.h index 96c5b7975948f..b7ff32002c447 100644 --- a/envoy/network/dns_resolver.h +++ b/envoy/network/dns_resolver.h @@ -15,7 +15,7 @@ constexpr absl::string_view DnsResolverCategory = "envoy.network.dns_resolver"; class DnsResolverFactory : public Config::TypedFactory { public: - /* + /** * @returns a DnsResolver object. * @param dispatcher: the local dispatcher thread * @param api: API interface to interact with system resources @@ -27,19 +27,19 @@ class DnsResolverFactory : public Config::TypedFactory { std::string category() const override { return std::string(DnsResolverCategory); } - /* + /** * Initialize the related data for this type of DNS resolver. * For some DNS resolvers, like c-ares, there are some specific data structure * needs to be initialized before using it to resolve target. */ - virtual void init() {} + virtual void initialize() {} - /* + /** * Cleanup the related data for this type of DNS resolver. * For some DNS resolvers, like c-ares, there are some specific data structure * needs to be cleaned up before terminates Envoy. */ - virtual void cleanup() {} + virtual void terminate() {} }; } // namespace Network diff --git a/source/common/network/dns_resolver/dns_factory_util.cc b/source/common/network/dns_resolver/dns_factory_util.cc index d030d1e0fe111..a88f163ab1170 100644 --- a/source/common/network/dns_resolver/dns_factory_util.cc +++ b/source/common/network/dns_resolver/dns_factory_util.cc @@ -82,7 +82,7 @@ Network::DnsResolverFactory& createDnsResolverFactoryFromTypedConfig( auto& factory = Config::Utility::getAndCheckFactory(typed_dns_resolver_config); // Perform factory initialization. - factory.init(); + factory.initialize(); return factory; } diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index bb5a8f451cc2a..6100396ad83bd 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -61,7 +61,7 @@ ProcessWide::~ProcessWide() { // Clean it up here in case it is ever used. if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName( std::string(Network::CaresDnsResolver), true)) { - dns_factory->cleanup(); + dns_factory->terminate(); } } } diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index 0ccd114b090db..cd739286ce7ca 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -522,7 +522,7 @@ class CaresDnsResolverFactory : public DnsResolverFactory { return std::make_shared(cares, dispatcher, resolvers); } - void init() override { + void initialize() override { // Initialize c-ares library in case first time. absl::MutexLock lock(&mutex_); if (!ares_library_initialized_) { @@ -530,7 +530,7 @@ class CaresDnsResolverFactory : public DnsResolverFactory { ares_library_init(ARES_LIB_INIT_ALL); } } - void cleanup() override { + void terminate() override { // Cleanup c-ares library if initialized. absl::MutexLock lock(&mutex_); if (ares_library_initialized_) { From 8a3d24e8f143aceb7db15c8e4b2810fcfe9610c3 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 29 Jun 2022 01:40:07 +0000 Subject: [PATCH 10/17] addressing comments Signed-off-by: Yanjun Xiang --- source/common/network/dns_resolver/dns_factory_util.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/network/dns_resolver/dns_factory_util.cc b/source/common/network/dns_resolver/dns_factory_util.cc index a88f163ab1170..462d23467dd75 100644 --- a/source/common/network/dns_resolver/dns_factory_util.cc +++ b/source/common/network/dns_resolver/dns_factory_util.cc @@ -81,7 +81,6 @@ Network::DnsResolverFactory& createDnsResolverFactoryFromTypedConfig( ENVOY_LOG_MISC(debug, "create DNS resolver type: {}", typed_dns_resolver_config.name()); auto& factory = Config::Utility::getAndCheckFactory(typed_dns_resolver_config); - // Perform factory initialization. factory.initialize(); return factory; } From ca388d03a84cc34338f5e3d2dd52d66479ccbcd1 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Thu, 30 Jun 2022 03:10:38 +0000 Subject: [PATCH 11/17] perform factory init for c-ares only Signed-off-by: Yanjun Xiang --- source/common/network/dns_resolver/dns_factory_util.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/network/dns_resolver/dns_factory_util.cc b/source/common/network/dns_resolver/dns_factory_util.cc index 462d23467dd75..50ad5bb745ae5 100644 --- a/source/common/network/dns_resolver/dns_factory_util.cc +++ b/source/common/network/dns_resolver/dns_factory_util.cc @@ -81,7 +81,9 @@ Network::DnsResolverFactory& createDnsResolverFactoryFromTypedConfig( ENVOY_LOG_MISC(debug, "create DNS resolver type: {}", typed_dns_resolver_config.name()); auto& factory = Config::Utility::getAndCheckFactory(typed_dns_resolver_config); - factory.initialize(); + if (factory.name() == std::string(CaresDnsResolver)) { + factory.initialize(); + } return factory; } From 09f15d793bb51551ee2320e8e69504f86bb72680 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Thu, 30 Jun 2022 22:01:16 +0000 Subject: [PATCH 12/17] make the factory init/terminate generic Signed-off-by: Yanjun Xiang --- source/common/config/utility.h | 11 +++++++++++ .../common/network/dns_resolver/dns_factory_util.cc | 4 +--- source/exe/process_wide.cc | 8 +++----- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 311a4b75d7dca..2c3d729e78d8e 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -241,6 +241,17 @@ class Utility { POOL_HISTOGRAM(scope))}; } + + + /** + * Get the hash map for this factory. + */ + template + static absl::flat_hash_map& getFactoryMap() { + return Registry::FactoryRegistry::factories(); + } + + /** * Get a Factory from the registry with a particular name (and templated type) with error checking * to ensure the name and factory are valid. diff --git a/source/common/network/dns_resolver/dns_factory_util.cc b/source/common/network/dns_resolver/dns_factory_util.cc index 50ad5bb745ae5..462d23467dd75 100644 --- a/source/common/network/dns_resolver/dns_factory_util.cc +++ b/source/common/network/dns_resolver/dns_factory_util.cc @@ -81,9 +81,7 @@ Network::DnsResolverFactory& createDnsResolverFactoryFromTypedConfig( ENVOY_LOG_MISC(debug, "create DNS resolver type: {}", typed_dns_resolver_config.name()); auto& factory = Config::Utility::getAndCheckFactory(typed_dns_resolver_config); - if (factory.name() == std::string(CaresDnsResolver)) { - factory.initialize(); - } + factory.initialize(); return factory; } diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index 6100396ad83bd..77d14b71d6b82 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -57,11 +57,9 @@ ProcessWide::~ProcessWide() { ASSERT(init_data.count_ > 0); if (--init_data.count_ == 0) { - // The c-ares library init is done in a thread-safe way when it is actually used for resolving. - // Clean it up here in case it is ever used. - if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName( - std::string(Network::CaresDnsResolver), true)) { - dns_factory->terminate(); + for (const auto& dns_factory : + Config::Utility::getFactoryMap()) { + dns_factory.second->terminate(); } } } From 6a26ee90d6705981f65681cea98083cfbf66c8c9 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 1 Jul 2022 13:47:49 +0000 Subject: [PATCH 13/17] addressing comments Signed-off-by: Yanjun Xiang --- envoy/network/dns_resolver.h | 9 +++++++++ source/common/config/utility.h | 11 ----------- source/exe/process_wide.cc | 5 +---- .../extensions/network/dns_resolver/cares/dns_impl.cc | 5 ++++- 4 files changed, 14 insertions(+), 16 deletions(-) diff --git a/envoy/network/dns_resolver.h b/envoy/network/dns_resolver.h index b7ff32002c447..b6a9d260a3453 100644 --- a/envoy/network/dns_resolver.h +++ b/envoy/network/dns_resolver.h @@ -42,5 +42,14 @@ class DnsResolverFactory : public Config::TypedFactory { virtual void terminate() {} }; +/** + * Terminate the DNS resolver factories. + */ +static inline void terminateDnsResolverFactories() { + auto& factories = Registry::FactoryRegistry::factories(); + std::for_each(factories.begin(), factories.end(), + [](auto& factory_it) { factory_it.second->terminate(); }); +} + } // namespace Network } // namespace Envoy diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 2c3d729e78d8e..311a4b75d7dca 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -241,17 +241,6 @@ class Utility { POOL_HISTOGRAM(scope))}; } - - - /** - * Get the hash map for this factory. - */ - template - static absl::flat_hash_map& getFactoryMap() { - return Registry::FactoryRegistry::factories(); - } - - /** * Get a Factory from the registry with a particular name (and templated type) with error checking * to ensure the name and factory are valid. diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index 77d14b71d6b82..74d6a944fb00e 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -57,10 +57,7 @@ ProcessWide::~ProcessWide() { ASSERT(init_data.count_ > 0); if (--init_data.count_ == 0) { - for (const auto& dns_factory : - Config::Utility::getFactoryMap()) { - dns_factory.second->terminate(); - } + Network::terminateDnsResolverFactories(); } } diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index cd739286ce7ca..e210153851b61 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -493,7 +493,8 @@ DnsResolverImpl::AddrInfoPendingResolution::availableInterfaces() { } // c-ares DNS resolver factory -class CaresDnsResolverFactory : public DnsResolverFactory { +class CaresDnsResolverFactory : public DnsResolverFactory, + public Logger::Loggable{ public: std::string name() const override { return std::string(CaresDnsResolver); } @@ -527,6 +528,7 @@ class CaresDnsResolverFactory : public DnsResolverFactory { absl::MutexLock lock(&mutex_); if (!ares_library_initialized_) { ares_library_initialized_ = true; + ENVOY_LOG(info, "c-ares library initialized."); ares_library_init(ARES_LIB_INIT_ALL); } } @@ -535,6 +537,7 @@ class CaresDnsResolverFactory : public DnsResolverFactory { absl::MutexLock lock(&mutex_); if (ares_library_initialized_) { ares_library_initialized_ = false; + ENVOY_LOG(info, "c-ares library cleaned up."); ares_library_cleanup(); } } From 76faa30f27897155148fa8f341d6127dce755431 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 1 Jul 2022 15:16:47 +0000 Subject: [PATCH 14/17] addressing comments Signed-off-by: Yanjun Xiang --- envoy/network/dns_resolver.h | 22 +++++++++++-------- .../network/dns_resolver/dns_factory_util.cc | 19 ++++++++++++---- source/exe/process_wide.cc | 2 +- .../network/dns_resolver/cares/dns_impl.cc | 2 +- 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/envoy/network/dns_resolver.h b/envoy/network/dns_resolver.h index b6a9d260a3453..65a62d84836c8 100644 --- a/envoy/network/dns_resolver.h +++ b/envoy/network/dns_resolver.h @@ -40,16 +40,20 @@ class DnsResolverFactory : public Config::TypedFactory { * needs to be cleaned up before terminates Envoy. */ virtual void terminate() {} -}; -/** - * Terminate the DNS resolver factories. - */ -static inline void terminateDnsResolverFactories() { - auto& factories = Registry::FactoryRegistry::factories(); - std::for_each(factories.begin(), factories.end(), - [](auto& factory_it) { factory_it.second->terminate(); }); -} + /** + * Find the DNS resolver factory based on the typed config and initialize it. + * @returns the DNS Resolver factory. + * @param typed_dns_resolver_config: the typed DNS resolver config + */ + static Network::DnsResolverFactory& initializeDnsResolverFactory( + const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config); + + /** + * Terminate the DNS resolver factories. + */ + static void terminateDnsResolverFactories(); +}; } // namespace Network } // namespace Envoy diff --git a/source/common/network/dns_resolver/dns_factory_util.cc b/source/common/network/dns_resolver/dns_factory_util.cc index 462d23467dd75..51554ca2fd09e 100644 --- a/source/common/network/dns_resolver/dns_factory_util.cc +++ b/source/common/network/dns_resolver/dns_factory_util.cc @@ -79,10 +79,7 @@ void handleLegacyDnsResolverData( Network::DnsResolverFactory& createDnsResolverFactoryFromTypedConfig( const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) { ENVOY_LOG_MISC(debug, "create DNS resolver type: {}", typed_dns_resolver_config.name()); - auto& factory = - Config::Utility::getAndCheckFactory(typed_dns_resolver_config); - factory.initialize(); - return factory; + return DnsResolverFactory::initializeDnsResolverFactory(typed_dns_resolver_config); } // Create the default DNS resolver factory. apple for MacOS or c-ares for all others. @@ -94,5 +91,19 @@ Network::DnsResolverFactory& createDefaultDnsResolverFactory( return createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config); } +Network::DnsResolverFactory& DnsResolverFactory::initializeDnsResolverFactory( + const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) { + auto& factory = + Config::Utility::getAndCheckFactory(typed_dns_resolver_config); + factory.initialize(); + return factory; +} + +void DnsResolverFactory::terminateDnsResolverFactories() { + auto& factories = Registry::FactoryRegistry::factories(); + std::for_each(factories.begin(), factories.end(), + [](auto& factory_it) { factory_it.second->terminate(); }); +} + } // namespace Network } // namespace Envoy diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index 74d6a944fb00e..e577d7f7724c2 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -57,7 +57,7 @@ ProcessWide::~ProcessWide() { ASSERT(init_data.count_ > 0); if (--init_data.count_ == 0) { - Network::terminateDnsResolverFactories(); + Network::DnsResolverFactory::terminateDnsResolverFactories(); } } diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index e210153851b61..6d14b89fbe48d 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -494,7 +494,7 @@ DnsResolverImpl::AddrInfoPendingResolution::availableInterfaces() { // c-ares DNS resolver factory class CaresDnsResolverFactory : public DnsResolverFactory, - public Logger::Loggable{ + public Logger::Loggable { public: std::string name() const override { return std::string(CaresDnsResolver); } From 8f2cb43c66193fe6871bf27dde2531c19b371bdf Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 1 Jul 2022 16:21:17 +0000 Subject: [PATCH 15/17] addressing comments Signed-off-by: Yanjun Xiang --- envoy/network/dns_resolver.h | 8 ++++---- source/common/network/dns_resolver/dns_factory_util.cc | 6 +++--- source/exe/BUILD | 2 +- source/exe/process_wide.cc | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/envoy/network/dns_resolver.h b/envoy/network/dns_resolver.h index 65a62d84836c8..8cdfda1e64840 100644 --- a/envoy/network/dns_resolver.h +++ b/envoy/network/dns_resolver.h @@ -42,17 +42,17 @@ class DnsResolverFactory : public Config::TypedFactory { virtual void terminate() {} /** - * Find the DNS resolver factory based on the typed config and initialize it. + * Create the DNS resolver factory based on the typed config and initialize it. * @returns the DNS Resolver factory. * @param typed_dns_resolver_config: the typed DNS resolver config */ - static Network::DnsResolverFactory& initializeDnsResolverFactory( + static Network::DnsResolverFactory& createFactory( const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config); /** - * Terminate the DNS resolver factories. + * Terminate the DNS resolver factory. */ - static void terminateDnsResolverFactories(); + static void terminateFactory(); }; } // namespace Network diff --git a/source/common/network/dns_resolver/dns_factory_util.cc b/source/common/network/dns_resolver/dns_factory_util.cc index 51554ca2fd09e..ce0e1ed708d74 100644 --- a/source/common/network/dns_resolver/dns_factory_util.cc +++ b/source/common/network/dns_resolver/dns_factory_util.cc @@ -79,7 +79,7 @@ void handleLegacyDnsResolverData( Network::DnsResolverFactory& createDnsResolverFactoryFromTypedConfig( const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) { ENVOY_LOG_MISC(debug, "create DNS resolver type: {}", typed_dns_resolver_config.name()); - return DnsResolverFactory::initializeDnsResolverFactory(typed_dns_resolver_config); + return DnsResolverFactory::createFactory(typed_dns_resolver_config); } // Create the default DNS resolver factory. apple for MacOS or c-ares for all others. @@ -91,7 +91,7 @@ Network::DnsResolverFactory& createDefaultDnsResolverFactory( return createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config); } -Network::DnsResolverFactory& DnsResolverFactory::initializeDnsResolverFactory( +Network::DnsResolverFactory& DnsResolverFactory::createFactory( const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) { auto& factory = Config::Utility::getAndCheckFactory(typed_dns_resolver_config); @@ -99,7 +99,7 @@ Network::DnsResolverFactory& DnsResolverFactory::initializeDnsResolverFactory( return factory; } -void DnsResolverFactory::terminateDnsResolverFactories() { +void DnsResolverFactory::terminateFactory() { auto& factories = Registry::FactoryRegistry::factories(); std::for_each(factories.begin(), factories.end(), [](auto& factory_it) { factory_it.second->terminate(); }); diff --git a/source/exe/BUILD b/source/exe/BUILD index 7059a446017ff..cc0796ed99007 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -142,10 +142,10 @@ envoy_cc_library( srcs = ["process_wide.cc"], hdrs = ["process_wide.h"], deps = [ - "//envoy/network:dns_resolver_interface", "//source/common/common:assert_lib", "//source/common/event:libevent_lib", "//source/common/http/http2:nghttp2_lib", + "//source/common/network/dns_resolver:dns_factory_util_lib", "//source/server:proto_descriptors_lib", ], ) diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index e577d7f7724c2..c588a84181c57 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -57,7 +57,7 @@ ProcessWide::~ProcessWide() { ASSERT(init_data.count_ > 0); if (--init_data.count_ == 0) { - Network::DnsResolverFactory::terminateDnsResolverFactories(); + Network::DnsResolverFactory::terminateFactory(); } } From 10a71edacbfa3efadbdb2c25434799c967b39489 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 1 Jul 2022 16:43:21 +0000 Subject: [PATCH 16/17] fixing CI format error Signed-off-by: Yanjun Xiang --- envoy/network/dns_resolver.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/envoy/network/dns_resolver.h b/envoy/network/dns_resolver.h index 8cdfda1e64840..e62d4dcf628cc 100644 --- a/envoy/network/dns_resolver.h +++ b/envoy/network/dns_resolver.h @@ -46,8 +46,8 @@ class DnsResolverFactory : public Config::TypedFactory { * @returns the DNS Resolver factory. * @param typed_dns_resolver_config: the typed DNS resolver config */ - static Network::DnsResolverFactory& createFactory( - const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config); + static Network::DnsResolverFactory& + createFactory(const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config); /** * Terminate the DNS resolver factory. From 629e580a8ae19dab92eb7c3a5eeae8707c298d0f Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 1 Jul 2022 17:38:54 +0000 Subject: [PATCH 17/17] addressing comments Signed-off-by: Yanjun Xiang --- envoy/network/dns_resolver.h | 4 ++-- source/common/network/dns_resolver/dns_factory_util.cc | 2 +- source/exe/process_wide.cc | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/envoy/network/dns_resolver.h b/envoy/network/dns_resolver.h index e62d4dcf628cc..3562f8756e94f 100644 --- a/envoy/network/dns_resolver.h +++ b/envoy/network/dns_resolver.h @@ -50,9 +50,9 @@ class DnsResolverFactory : public Config::TypedFactory { createFactory(const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config); /** - * Terminate the DNS resolver factory. + * Call the terminate method on all the registered DNS resolver factories. */ - static void terminateFactory(); + static void terminateFactories(); }; } // namespace Network diff --git a/source/common/network/dns_resolver/dns_factory_util.cc b/source/common/network/dns_resolver/dns_factory_util.cc index ce0e1ed708d74..d06f1186fb561 100644 --- a/source/common/network/dns_resolver/dns_factory_util.cc +++ b/source/common/network/dns_resolver/dns_factory_util.cc @@ -99,7 +99,7 @@ Network::DnsResolverFactory& DnsResolverFactory::createFactory( return factory; } -void DnsResolverFactory::terminateFactory() { +void DnsResolverFactory::terminateFactories() { auto& factories = Registry::FactoryRegistry::factories(); std::for_each(factories.begin(), factories.end(), [](auto& factory_it) { factory_it.second->terminate(); }); diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index c588a84181c57..9e93e9c48f4b4 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -57,7 +57,7 @@ ProcessWide::~ProcessWide() { ASSERT(init_data.count_ > 0); if (--init_data.count_ == 0) { - Network::DnsResolverFactory::terminateFactory(); + Network::DnsResolverFactory::terminateFactories(); } }