Make CaresPTRResolver a singleton through DNSPTRResolverProvider, add comments and address minor comments

src/Access/Common/AllowedClientHosts.cpp

@@ -110,18 +110,18 @@ namespace
     }
 
     /// Returns the host name by its address.
-    std::vector<String> getHostsByAddress(const IPAddress & address)
+    Strings getHostsByAddress(const IPAddress & address)
     {
-        std::vector<String> 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);
             }
 

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)

src/Common/CARESPTRResolver.cpp

@@ -1,77 +0,0 @@
-#include "CARESPTRResolver.h"
-#include <Common/Exception.h>
-#include <arpa/inet.h>
-#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<std::vector<std::string>*>(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<ares_channel>()) {
-        init();
-    }
-
-    CARESPTRResolver::~CARESPTRResolver() {
-        deinit();
-    }
-
-    std::vector<std::string> CARESPTRResolver::resolve(const std::string & ip) {
-        std::vector<std::string> 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<std::string> & response) {
-        in_addr addr;
-        inet_aton(ip.c_str(), &addr);
-
-        ares_gethostbyaddr(*channel, reinterpret_cast<const char*>(&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);
-        }
-    }
-}

src/Common/CARESPTRResolver.h

@@ -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<std::string> resolve(const std::string & ip) override;
-
-    private:
-        void init();
-        void deinit();
-        void wait();
-
-        void resolve(const std::string & ip, std::vector<std::string> & response);
-
-        std::unique_ptr<ares_channel> channel;
-    };
-}
-

src/Common/CaresPTRResolver.cpp

@@ -0,0 +1,95 @@
+#include "CaresPTRResolver.h"
+#include <arpa/inet.h>
+#include <Common/Exception.h>
+#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<std::vector<std::string>*>(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<std::string> CaresPTRResolver::resolve(const std::string & ip) {
+        std::vector<std::string> ptr_records;
+
+        resolve(ip, ptr_records);
+        wait();
+
+        return ptr_records;
+    }
+
+    std::vector<std::string> CaresPTRResolver::resolve_v6(const std::string & ip)
+    {
+        std::vector<std::string> ptr_records;
+
+        resolve_v6(ip, ptr_records);
+        wait();
+
+        return ptr_records;
+    }
+
+    void CaresPTRResolver::resolve(const std::string & ip, std::vector<std::string> & response) {
+        in_addr addr;
+
+        inet_pton(AF_INET, ip.c_str(), &addr);
+
+        ares_gethostbyaddr(channel, reinterpret_cast<const void*>(&addr), sizeof(addr), AF_INET, callback, &response);
+    }
+
+    void CaresPTRResolver::resolve_v6(const std::string & ip, std::vector<std::string> & response) {
+        in6_addr addr;
+        inet_pton(AF_INET6, ip.c_str(), &addr);
+
+        ares_gethostbyaddr(channel, reinterpret_cast<const void*>(&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);
+        }
+    }
+}

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<std::string> resolve(const std::string & ip) override;
+
+        std::vector<std::string> resolve_v6(const std::string & ip) override;
+
+    private:
+        void wait();
+
+        void resolve(const std::string & ip, std::vector<std::string> & response);
+
+        void resolve_v6(const std::string & ip, std::vector<std::string> & response);
+
+        ares_channel channel;
+    };
+}
+

src/Common/DNSPTRResolver.h

@@ -10,5 +10,7 @@ namespace DB {
 
         virtual std::vector<std::string> resolve(const std::string & ip) = 0;
 
+        virtual std::vector<std::string> resolve_v6(const std::string & ip) = 0;
+
     };
 }

src/Common/DNSPTRResolverProvider.cpp

@@ -1,12 +1,12 @@
 #include "DNSPTRResolverProvider.h"
-#include "CARESPTRResolver.h"
+#include "CaresPTRResolver.h"
 
 namespace DB {
-
     std::shared_ptr<DNSPTRResolver> DNSPTRResolverProvider::get()
     {
-        static auto cares_resolver = std::make_shared<CARESPTRResolver>();
+        static auto cares_resolver = std::make_shared<CaresPTRResolver>(
+            CaresPTRResolver::provider_token {}
+        );
         return cares_resolver;
     }
-
 }

src/Common/DNSPTRResolverProvider.h

@@ -10,4 +10,3 @@ namespace DB {
         static std::shared_ptr<DNSPTRResolver> get();
     };
 }
-

src/Common/DNSResolver.cpp

@@ -12,7 +12,6 @@
 #include <atomic>
 #include <optional>
 #include <string_view>
-#include <boost/asio.hpp>
 #include "DNSPTRResolverProvider.h"
 
 namespace ProfileEvents
@@ -143,7 +142,12 @@ static DNSResolver::IPAddresses resolveIPAddressImpl(const std::string & host)
 static std::vector<String> 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