From e6bf020b9e1f77d633660da8a45dc99e1be10e6f Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Thu, 15 Aug 2019 15:37:46 +0300 Subject: [PATCH] Continue name resolution fixing, add unit test name_lookup() now returns all results given by getnameinfo(). When searching for a server, finding one matching address in the lookup-results is enough for a match. Also, added a test for name_lookup(). The test is minimal on its own, as hardcoded test cases are not generally valid. --- maxutils/maxbase/include/maxbase/host.hh | 8 +- maxutils/maxbase/src/host.cc | 50 +++++++----- maxutils/maxbase/src/test/CMakeLists.txt | 4 + maxutils/maxbase/src/test/test_name_lookup.cc | 80 +++++++++++++++++++ .../monitor/mariadbmon/cluster_discovery.cc | 18 ++--- .../modules/monitor/mariadbmon/mariadbmon.cc | 20 +++-- .../modules/monitor/mariadbmon/mariadbmon.hh | 5 +- 7 files changed, 142 insertions(+), 43 deletions(-) create mode 100644 maxutils/maxbase/src/test/test_name_lookup.cc diff --git a/maxutils/maxbase/include/maxbase/host.hh b/maxutils/maxbase/include/maxbase/host.hh index a290c7f55..365ad4a9d 100644 --- a/maxutils/maxbase/include/maxbase/host.hh +++ b/maxutils/maxbase/include/maxbase/host.hh @@ -16,6 +16,7 @@ #include #include +#include /** Host is a streamable class that represents an address and port, or a unix domain socket. */ @@ -110,14 +111,15 @@ inline bool operator!=(const Host& l, const Host& r) } /** - * Perform DNS resolution on a hostname or text-form IP address. + * Perform domain name resolution on a hostname or text-form IP address. * * @param host Hostname to convert. - * @param addr_out Output buffer. The output is in IPv6-form as returned by "inet_ntop(AF_INET6, ...)". + * @param addresses_out Output buffer. The output is in IPv6-form as returned by "inet_ntop(AF_INET6, ...)". * @param error_out Error output * @return True if successful */ -bool name_lookup(const std::string& host, std::string* addr_out, std::string* error_out = nullptr); +bool name_lookup(const std::string& host, std::unordered_set* addresses_out, + std::string* error_out = nullptr); /** * Perform reverse DNS on an IP address. This may involve network communication so can be slow. diff --git a/maxutils/maxbase/src/host.cc b/maxutils/maxbase/src/host.cc index f8f12f622..7a14ecb01 100644 --- a/maxutils/maxbase/src/host.cc +++ b/maxutils/maxbase/src/host.cc @@ -227,42 +227,50 @@ std::istream& operator>>(std::istream& is, Host& host) return is; } -bool name_lookup(const std::string& host, std::string* addr_out, std::string* error_out) +bool name_lookup(const std::string& host, std::unordered_set* addresses_out, std::string* error_out) { addrinfo hints; memset(&hints, 0, sizeof(hints)); - hints.ai_family = AF_INET6; /* Only return IPv6-addresses, possibly mapped from IPv4 */ - hints.ai_socktype = SOCK_DGRAM; /* Datagram socket */ - hints.ai_flags = AI_V4MAPPED; /* Mapped IPv4 */ - hints.ai_protocol = 0; /* Any protocol */ - hints.ai_canonname = NULL; - hints.ai_addr = NULL; - hints.ai_next = NULL; + hints.ai_family = AF_UNSPEC; /* Return any address type */ + hints.ai_socktype = SOCK_DGRAM; /* Datagram socket */ + hints.ai_flags = 0; /* Mapped IPv4 */ + hints.ai_protocol = 0; /* Any protocol */ + hints.ai_canonname = nullptr; + hints.ai_addr = nullptr; + hints.ai_next = nullptr; addrinfo* results = nullptr; bool success = false; std::string error_msg; + int rv_addrinfo = getaddrinfo(host.c_str(), nullptr, &hints, &results); if (rv_addrinfo == 0) { - mxb_assert(results); - if (results) + // getaddrinfo may return multiple result addresses. Save all of them. + for (auto iter = results; iter; iter = iter->ai_next) { - // getaddrinfo may return multiple result addresses. Only consider the first. - char buf[INET6_ADDRSTRLEN]; - in6_addr* addr = &((sockaddr_in6*)results->ai_addr)->sin6_addr; + int address_family = iter->ai_family; + void* addr = nullptr; + char buf[INET6_ADDRSTRLEN]; // Enough for both address types + if (iter->ai_family == AF_INET) + { + auto sa_in = (sockaddr_in*)(iter->ai_addr); + addr = &sa_in->sin_addr; + } + else if (iter->ai_family == AF_INET6) + { + auto sa_in = (sockaddr_in6*)(iter->ai_addr); + addr = &sa_in->sin6_addr; + } - if (inet_ntop(AF_INET6, addr, buf, sizeof(buf))) + inet_ntop(address_family, addr, buf, sizeof(buf)); + if (buf[0]) { - *addr_out = buf; - success = true; + addresses_out->insert(buf); + success = true; // One successful lookup is enough. } - else - { - error_msg = mxb::string_printf("inet_ntop() failed: '%s'.", strerror(errno)); - } - freeaddrinfo(results); } + freeaddrinfo(results); } else { diff --git a/maxutils/maxbase/src/test/CMakeLists.txt b/maxutils/maxbase/src/test/CMakeLists.txt index 5016e866b..3af01eae1 100644 --- a/maxutils/maxbase/src/test/CMakeLists.txt +++ b/maxutils/maxbase/src/test/CMakeLists.txt @@ -24,3 +24,7 @@ add_test(test_worker test_worker) add_executable(test_host_class test_host_class.cc) target_link_libraries(test_host_class maxbase pthread rt) add_test(test_host_class test_host_class) + +add_executable(test_name_lookup test_name_lookup.cc) +target_link_libraries(test_name_lookup maxbase) +add_test(test_name_lookup test_name_lookup) diff --git a/maxutils/maxbase/src/test/test_name_lookup.cc b/maxutils/maxbase/src/test/test_name_lookup.cc new file mode 100644 index 000000000..f1ccd861e --- /dev/null +++ b/maxutils/maxbase/src/test/test_name_lookup.cc @@ -0,0 +1,80 @@ +/* + * Copyright (c) 2019 MariaDB Corporation Ab + * + * Use of this software is governed by the Business Source License included + * in the LICENSE.TXT file and at www.mariadb.com/bsl11. + * + * Change Date: 2023-01-01 + * + * On the date above, in accordance with the Business Source License, use + * of this software will be governed by version 2 or later of the General + * Public License. + */ +#include + +#include + +int main() +{ + using std::string; + using std::unordered_set; + + struct Test + { + string host; + string ip; + string result; + }; + + Test tests[] = { + {"localhost", "127.0.0.1", "127.0.0.1"}, + }; + /* + * Here are some additional test case examples. They may not work + * as is. + * + * Test tests[] = { + * {"yle.fi", "13.32.43.102", "13.32.43.102"}, + * {"mariadb.com", "35.235.124.140", "35.235.124.140"}, + * {"reddit.com", "151.101.1.140", "151.101.1.140"}, + * {"max-tst-02.mariadb.com", "94.23.248.118", "94.23.248.118"}, + * {"wikipedia.org", "2620:0:862:ed1a::1", "2620:0:862:ed1a::1"}, + * {"one.one.one.one", "2606:4700:4700::1111", "2606:4700:4700::1111"}, + * }; + */ + + bool ok = true; + for (auto& item : tests) + { + string error; + auto& host = item.host; + auto& ip = item.ip; + auto& expected_result = item.result; + for (auto& subitem : {host, ip}) + { + unordered_set lookup_result; + bool lookup_ok = mxb::name_lookup(subitem, &lookup_result, &error); + if (!lookup_ok) + { + printf("Lookup of '%s' failed: %s\n", subitem.c_str(), error.c_str()); + ok = false; + } + else if (lookup_result.count(expected_result) == 0) + { + string results_conc; + string sep; + for (auto result_elem : lookup_result) + { + results_conc += sep + result_elem; + sep = ", "; + } + printf("Lookup of '%s' gave incorrect results. Expected '%s', got '%s'.\n", + subitem.c_str(), expected_result.c_str(), results_conc.c_str()); + ok = false; + } + } + } + + return ok ? EXIT_SUCCESS : EXIT_FAILURE; +} + diff --git a/server/modules/monitor/mariadbmon/cluster_discovery.cc b/server/modules/monitor/mariadbmon/cluster_discovery.cc index 9d5ff30b6..677870634 100644 --- a/server/modules/monitor/mariadbmon/cluster_discovery.cc +++ b/server/modules/monitor/mariadbmon/cluster_discovery.cc @@ -1005,34 +1005,32 @@ bool MariaDBMonitor::is_candidate_valid(MariaDBServer* cand, RequireRunning req_ return is_valid; }; -string MariaDBMonitor::DNSResolver::resolve_server(const string& host) +MariaDBMonitor::DNSResolver::StringSet MariaDBMonitor::DNSResolver::resolve_server(const string& host) { auto now = mxb::Clock::now(); const auto MAX_AGE = mxb::Duration((double)5*60); // Refresh interval for cache entries. auto recent_time = now - MAX_AGE; + DNSResolver::StringSet rval; - string rval; auto iter = m_mapping.find(host); - if (iter == m_mapping.end() || iter->second.timestamp < recent_time) { // Map did not have a record, or it was too old. In either case, generate a new one. - string addr; + DNSResolver::StringSet addresses; string error_msg; - bool dns_success = mxb::name_lookup(host, &addr, &error_msg); + bool dns_success = mxb::name_lookup(host, &addresses, &error_msg); if (!dns_success) { MXB_ERROR("Could not resolve host '%s'. %s", host.c_str(), error_msg.c_str()); } - // If dns failed, addr will be empty. Add the element anyway to prevent repeated lookups. - MapElement newelem = {addr, now}; - m_mapping[host] = newelem; - rval = addr; + // If dns failed, the array will be empty. Add/replace the element anyway to prevent repeated lookups. + m_mapping[host] = {addresses, now}; + rval = std::move(addresses); } else { // Return recent value. - rval = iter->second.address; + rval = iter->second.addresses; } return rval; } diff --git a/server/modules/monitor/mariadbmon/mariadbmon.cc b/server/modules/monitor/mariadbmon/mariadbmon.cc index 51d24b8e8..8cccdd03f 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.cc +++ b/server/modules/monitor/mariadbmon/mariadbmon.cc @@ -124,21 +124,27 @@ MariaDBServer* MariaDBMonitor::get_server(const EndPoint& search_ep) { // Phase 2: Was not found with simple string compare. Try DNS resolving for endpoints with // matching ports. - string target_addr = m_resolver.resolve_server(search_ep.host()); - if (!target_addr.empty()) + DNSResolver::StringSet target_addresses = m_resolver.resolve_server(search_ep.host()); + if (!target_addresses.empty()) { for (auto server : m_servers) { - if (server->m_server_base->server->port == search_ep.port()) + SERVER* srv = server->m_server_base->server; + if (srv->port == search_ep.port()) { - string server_addr = m_resolver.resolve_server(server->m_server_base->server->address); - if (server_addr == target_addr) + auto server_addresses = m_resolver.resolve_server(srv->address); + // The number of elements in the arrays is rarely over 1. + for (auto& address : server_addresses) { - found = server; - break; + if (target_addresses.count(address) > 0) + { + found = server; + goto breakout; + } } } } + breakout:; } } return found; diff --git a/server/modules/monitor/mariadbmon/mariadbmon.hh b/server/modules/monitor/mariadbmon/mariadbmon.hh index 6303296ee..dcbe6d551 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.hh +++ b/server/modules/monitor/mariadbmon/mariadbmon.hh @@ -172,12 +172,13 @@ private: class DNSResolver { public: - std::string resolve_server(const std::string& host); + using StringSet = std::unordered_set; + StringSet resolve_server(const std::string& host); private: struct MapElement { - std::string address; + StringSet addresses; // A hostname can map to multiple addresses mxb::TimePoint timestamp; };