From d48690d455bb182fdff15c21da9e04ba59581f7c Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 12 Jul 2022 14:21:10 -0300 Subject: [PATCH] Make CaresPTRResolver a singleton through DNSPTRResolverProvider, add comments and address minor comments --- src/Access/Common/AllowedClientHosts.cpp | 13 ++-- src/CMakeLists.txt | 2 +- src/Common/CARESPTRResolver.cpp | 77 ------------------- src/Common/CARESPTRResolver.h | 25 ------- src/Common/CaresPTRResolver.cpp | 95 ++++++++++++++++++++++++ src/Common/CaresPTRResolver.h | 40 ++++++++++ src/Common/DNSPTRResolver.h | 2 + src/Common/DNSPTRResolverProvider.cpp | 8 +- src/Common/DNSPTRResolverProvider.h | 1 - src/Common/DNSResolver.cpp | 8 +- 10 files changed, 155 insertions(+), 116 deletions(-) delete mode 100644 src/Common/CARESPTRResolver.cpp delete mode 100644 src/Common/CARESPTRResolver.h create mode 100644 src/Common/CaresPTRResolver.cpp create mode 100644 src/Common/CaresPTRResolver.h diff --git a/src/Access/Common/AllowedClientHosts.cpp b/src/Access/Common/AllowedClientHosts.cpp index 09058a3c045..e32b218c948 100644 --- a/src/Access/Common/AllowedClientHosts.cpp +++ b/src/Access/Common/AllowedClientHosts.cpp @@ -110,18 +110,18 @@ namespace } /// Returns the host name by its address. - std::vector getHostsByAddress(const IPAddress & address) + Strings getHostsByAddress(const IPAddress & address) { - std::vector hosts = DNSResolver::instance().reverseResolve(address); + auto hosts = DNSResolver::instance().reverseResolve(address); if (hosts.empty()) - throw Exception(address.toString() + " could not be resolved", ErrorCodes::DNS_ERROR); + throw Exception(ErrorCodes::DNS_ERROR, "{} could not be resolved", address.toString()); - for (auto & host : hosts) { + for (const auto & host : hosts) { /// Check that PTR record is resolved back to client address if (!isAddressOfHost(address, host)) - throw Exception("Host " + String(host) + " isn't resolved back to " + address.toString(), ErrorCodes::DNS_ERROR); + throw Exception(ErrorCodes::DNS_ERROR, "Host {} isn't resolved back to {}", host, address.toString()); } return hosts; @@ -532,7 +532,8 @@ bool AllowedClientHosts::contains(const IPAddress & client_address) const { if (boost::iequals(name_regexp_, "localhost")) return is_client_local(); - if (!resolved_hosts) { + if (!resolved_hosts) + { resolved_hosts = getHostsByAddress(client_address); } diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index c04d679f67b..8e709acd43e 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -439,7 +439,7 @@ if (TARGET ch_contrib::avrocpp) dbms_target_link_libraries(PRIVATE ch_contrib::avrocpp) endif () -set_source_files_properties(Common/CARESPTRResolver.cpp PROPERTIES COMPILE_FLAGS -Wno-reserved-identifier) +set_source_files_properties(Common/CaresPTRResolver.cpp PROPERTIES COMPILE_FLAGS -Wno-reserved-identifier) target_link_libraries (clickhouse_common_io PRIVATE ch_contrib::c-ares) if (TARGET OpenSSL::Crypto) diff --git a/src/Common/CARESPTRResolver.cpp b/src/Common/CARESPTRResolver.cpp deleted file mode 100644 index 1977f5cbb7a..00000000000 --- a/src/Common/CARESPTRResolver.cpp +++ /dev/null @@ -1,77 +0,0 @@ -#include "CARESPTRResolver.h" -#include -#include -#include "netdb.h" -#include "ares.h" - -namespace DB { - - namespace ErrorCodes - { - extern const int DNS_ERROR; - } - - static void callback(void * arg, int status, int, struct hostent * host) { - auto * ptr_records = reinterpret_cast*>(arg); - if (status == ARES_SUCCESS) { - int i = 0; - while (auto * ptr_record = host->h_aliases[i]) { - ptr_records->emplace_back(ptr_record); - i++; - } - } - } - - CARESPTRResolver::CARESPTRResolver() : channel(std::make_unique()) { - init(); - } - - CARESPTRResolver::~CARESPTRResolver() { - deinit(); - } - - std::vector CARESPTRResolver::resolve(const std::string & ip) { - std::vector ptr_records; - - resolve(ip, ptr_records); - wait(); - - return ptr_records; - } - - void CARESPTRResolver::init() { - if (ares_init(channel.get()) != ARES_SUCCESS) { - throw DB::Exception("Failed to initialize c-ares", DB::ErrorCodes::DNS_ERROR); - } - } - - void CARESPTRResolver::deinit() { - ares_destroy(*channel); - ares_library_cleanup(); - } - - void CARESPTRResolver::resolve(const std::string & ip, std::vector & response) { - in_addr addr; - inet_aton(ip.c_str(), &addr); - - ares_gethostbyaddr(*channel, reinterpret_cast(&addr), sizeof(addr), AF_INET, callback, &response); - } - - void CARESPTRResolver::wait() { - for(;;) { - timeval * tvp, tv; - fd_set read_fds, write_fds; - int nfds; - - FD_ZERO(&read_fds); - FD_ZERO(&write_fds); - nfds = ares_fds(*channel, &read_fds, &write_fds); - if(nfds == 0) { - break; - } - tvp = ares_timeout(*channel, nullptr, &tv); - select(nfds, &read_fds, &write_fds, nullptr, tvp); - ares_process(*channel, &read_fds, &write_fds); - } - } -} diff --git a/src/Common/CARESPTRResolver.h b/src/Common/CARESPTRResolver.h deleted file mode 100644 index cbbbed017cf..00000000000 --- a/src/Common/CARESPTRResolver.h +++ /dev/null @@ -1,25 +0,0 @@ -#pragma once - -#include "DNSPTRResolver.h" - -using ares_channel = struct ares_channeldata *; - -namespace DB { - class CARESPTRResolver : public DNSPTRResolver { - public: - CARESPTRResolver(); - ~CARESPTRResolver() override; - - std::vector resolve(const std::string & ip) override; - - private: - void init(); - void deinit(); - void wait(); - - void resolve(const std::string & ip, std::vector & response); - - std::unique_ptr channel; - }; -} - diff --git a/src/Common/CaresPTRResolver.cpp b/src/Common/CaresPTRResolver.cpp new file mode 100644 index 00000000000..6160c2fb3a7 --- /dev/null +++ b/src/Common/CaresPTRResolver.cpp @@ -0,0 +1,95 @@ +#include "CaresPTRResolver.h" +#include +#include +#include "ares.h" +#include "netdb.h" + +namespace DB { + + namespace ErrorCodes + { + extern const int DNS_ERROR; + } + + static void callback(void * arg, int status, int, struct hostent * host) { + auto * ptr_records = reinterpret_cast*>(arg); + if (status == ARES_SUCCESS && host->h_aliases) { + int i = 0; + while (auto * ptr_record = host->h_aliases[i]) { + ptr_records->emplace_back(ptr_record); + i++; + } + } + } + + CaresPTRResolver::CaresPTRResolver(CaresPTRResolver::provider_token) : channel(nullptr) { + /* + * ares_library_init is not thread safe. Currently, the only other usage of c-ares seems to be in grpc. + * In grpc, ares_library_init seems to be called only in Windows. + * See https://github.com/grpc/grpc/blob/master/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc#L1187 + * That means it's safe to init it here, but we should be cautious when introducing new code that depends on c-ares and even updates + * to grpc. As discussed in https://github.com/ClickHouse/ClickHouse/pull/37827#discussion_r919189085, c-ares should be adapted to be atomic + * */ + if (ares_library_init(ARES_LIB_INIT_ALL) != ARES_SUCCESS || ares_init(&channel) != ARES_SUCCESS) { + throw DB::Exception("Failed to initialize c-ares", DB::ErrorCodes::DNS_ERROR); + } + } + + CaresPTRResolver::~CaresPTRResolver() { + ares_destroy(channel); + ares_library_cleanup(); + } + + std::vector CaresPTRResolver::resolve(const std::string & ip) { + std::vector ptr_records; + + resolve(ip, ptr_records); + wait(); + + return ptr_records; + } + + std::vector CaresPTRResolver::resolve_v6(const std::string & ip) + { + std::vector ptr_records; + + resolve_v6(ip, ptr_records); + wait(); + + return ptr_records; + } + + void CaresPTRResolver::resolve(const std::string & ip, std::vector & response) { + in_addr addr; + + inet_pton(AF_INET, ip.c_str(), &addr); + + ares_gethostbyaddr(channel, reinterpret_cast(&addr), sizeof(addr), AF_INET, callback, &response); + } + + void CaresPTRResolver::resolve_v6(const std::string & ip, std::vector & response) { + in6_addr addr; + inet_pton(AF_INET6, ip.c_str(), &addr); + + ares_gethostbyaddr(channel, reinterpret_cast(&addr), sizeof(addr), AF_INET6, callback, &response); + } + + void CaresPTRResolver::wait() { + for(;;) { + timeval * tvp, tv; + fd_set read_fds; + fd_set write_fds; + int nfds; + + FD_ZERO(&read_fds); + FD_ZERO(&write_fds); + nfds = ares_fds(channel, &read_fds, &write_fds); + if(nfds == 0) { + break; + } + tvp = ares_timeout(channel, nullptr, &tv); + select(nfds, &read_fds, &write_fds, nullptr, tvp); + ares_process(channel, &read_fds, &write_fds); + } + } +} diff --git a/src/Common/CaresPTRResolver.h b/src/Common/CaresPTRResolver.h new file mode 100644 index 00000000000..23c529f4947 --- /dev/null +++ b/src/Common/CaresPTRResolver.h @@ -0,0 +1,40 @@ +#pragma once + +#include "DNSPTRResolver.h" + +using ares_channel = struct ares_channeldata *; + +namespace DB { + + /* + * Implements reverse DNS resolution using c-ares lib. System reverse DNS resolution via + * gethostbyaddr or getnameinfo does not work reliably as in some systems + * it returns all PTR records for a given IP and in others it returns only one. + * */ + class CaresPTRResolver : public DNSPTRResolver { + friend class DNSPTRResolverProvider; + + /* + * Allow only DNSPTRProvider to instantiate this class + * */ + struct provider_token {}; + + public: + CaresPTRResolver(provider_token); + ~CaresPTRResolver() override; + + std::vector resolve(const std::string & ip) override; + + std::vector resolve_v6(const std::string & ip) override; + + private: + void wait(); + + void resolve(const std::string & ip, std::vector & response); + + void resolve_v6(const std::string & ip, std::vector & response); + + ares_channel channel; + }; +} + diff --git a/src/Common/DNSPTRResolver.h b/src/Common/DNSPTRResolver.h index 20a5422767a..7cb78c15567 100644 --- a/src/Common/DNSPTRResolver.h +++ b/src/Common/DNSPTRResolver.h @@ -10,5 +10,7 @@ namespace DB { virtual std::vector resolve(const std::string & ip) = 0; + virtual std::vector resolve_v6(const std::string & ip) = 0; + }; } diff --git a/src/Common/DNSPTRResolverProvider.cpp b/src/Common/DNSPTRResolverProvider.cpp index ef19024495f..faf1980977c 100644 --- a/src/Common/DNSPTRResolverProvider.cpp +++ b/src/Common/DNSPTRResolverProvider.cpp @@ -1,12 +1,12 @@ #include "DNSPTRResolverProvider.h" -#include "CARESPTRResolver.h" +#include "CaresPTRResolver.h" namespace DB { - std::shared_ptr DNSPTRResolverProvider::get() { - static auto cares_resolver = std::make_shared(); + static auto cares_resolver = std::make_shared( + CaresPTRResolver::provider_token {} + ); return cares_resolver; } - } diff --git a/src/Common/DNSPTRResolverProvider.h b/src/Common/DNSPTRResolverProvider.h index 17764d8c91d..b4fa8ab77a4 100644 --- a/src/Common/DNSPTRResolverProvider.h +++ b/src/Common/DNSPTRResolverProvider.h @@ -10,4 +10,3 @@ namespace DB { static std::shared_ptr get(); }; } - diff --git a/src/Common/DNSResolver.cpp b/src/Common/DNSResolver.cpp index 4c32dbf3341..d8967e9661f 100644 --- a/src/Common/DNSResolver.cpp +++ b/src/Common/DNSResolver.cpp @@ -12,7 +12,6 @@ #include #include #include -#include #include "DNSPTRResolverProvider.h" namespace ProfileEvents @@ -143,7 +142,12 @@ static DNSResolver::IPAddresses resolveIPAddressImpl(const std::string & host) static std::vector reverseResolveImpl(const Poco::Net::IPAddress & address) { auto ptr_resolver = DB::DNSPTRResolverProvider::get(); - return ptr_resolver->resolve(address.toString()); + + if (address.family() == Poco::Net::IPAddress::Family::IPv4) { + return ptr_resolver->resolve(address.toString()); + } else { + return ptr_resolver->resolve_v6(address.toString()); + } } struct DNSResolver::Impl