Don't use DNS resolution when finding local ip addresses.

jtc3161 · srh · srh · commit 9ca0d3fc681d · 2018-01-07T16:18:00.000-08:00
Removes use of DNS to find local ip addresses to bind to, except on Windows. Also removes mistaken "Ignore loopback addresses" logic, which happened to get its conditional flipped the wrong way. The flipped "Ignore loopback addresses" logic meant that we would remove all non-loopback addresses that got returned by the DNS lookup. So removing the DNS lookup entirely won't break things. The behavior on Windows never used getifaddrs, it just used DNS lookup. We keep the old behavior on Windows because limited manpower prevents us from using the Windows alternative to getifaddrs. Fixes #4600. Co-authored-by: Sam Hughes <sam@samuelhughes.com>
diff --git a/src/arch/address.cc b/src/arch/address.cc
@@ -155,28 +155,19 @@ std::set<ip_address_t> hostname_to_ips(const std::string &host) {
 std::set<ip_address_t> get_local_ips(const std::set<ip_address_t> &filter,
                                      local_ip_filter_t filter_type) {
     std::set<ip_address_t> all_ips;
-    std::set<ip_address_t> filtered_ips;
-
+#ifdef _WIN32
+    // NOTE: We used to use hostname_to_ips for all OSes, we stopped using it because
+    // DNS resolution could be slow.  Now we only use it for Windows because nobody's
+    // fixed the Windows implementation to get the network interfaces more sensibly.
     try {
         all_ips = hostname_to_ips(str_gethostname());
     } catch (const host_lookup_exc_t &ex) {
         // Continue on, this probably means there's no DNS entry for this host
     }
 
-#ifdef _WIN32
     // TODO WINDOWS: is this enough?
     all_ips.emplace("127.0.0.1");
 #else
-    // Ignore loopback addresses - those will be returned by getifaddrs, and
-    // getaddrinfo is not so trustworthy.
-    // See https://github.com/rethinkdb/rethinkdb/issues/2405
-    for (auto it = all_ips.begin(); it != all_ips.end();) {
-        auto curr = it++;
-        if (!curr->is_loopback()) {
-            all_ips.erase(curr);
-        }
-    }
-
     struct ifaddrs *addrs;
     int res = getifaddrs(&addrs);
     guarantee_err(res == 0,
@@ -195,6 +186,7 @@ std::set<ip_address_t> get_local_ips(const std::set<ip_address_t> &filter,
     freeifaddrs(addrs);
 #endif
 
+    std::set<ip_address_t> filtered_ips;
     // Remove any addresses that don't fit the filter
     for (auto const &ip : all_ips) {
         if (filter_type == local_ip_filter_t::ALL ||
diff --git a/src/clustering/administration/main/serve.cc b/src/clustering/administration/main/serve.cc
@@ -55,7 +55,7 @@ peer_address_set_t look_up_peers_addresses(const std::vector<host_and_port_t> &n
 }
 
 std::string service_address_ports_t::get_addresses_string(
-    std::set<ip_address_t> actual_addresses) const {
+    std::set<ip_address_t> actual_addresses) {
 
     bool first = true;
     std::string result;
@@ -74,8 +74,7 @@ std::string service_address_ports_t::get_addresses_string(
     return result;
 }
 
-bool service_address_ports_t::is_bind_all(
-    std::set<ip_address_t> addresses) const {
+bool service_address_ports_t::is_bind_all(const std::set<ip_address_t> &addresses) {
     // If the set is empty, it means we're listening on all addresses.
     return addresses.empty();
 }
@@ -611,7 +610,7 @@ bool do_serve(io_backender_t *io_backender,
                            serve_info.ports.local_addresses_http.size() == 1 ? "" : "es",
                            addresses_string.c_str());
 
-                    if (!serve_info.ports.is_bind_all(serve_info.ports.local_addresses)) {
+                    if (!service_address_ports_t::is_bind_all(serve_info.ports.local_addresses)) {
                         if(serve_info.config_file) {
                             logNTC("To fully expose RethinkDB on the network, bind to "
                                    "all addresses by adding `bind=all' to the config "
diff --git a/src/clustering/administration/main/serve.hpp b/src/clustering/administration/main/serve.hpp
@@ -80,10 +80,9 @@ struct service_address_ports_t {
             sanitize_port(reql_port, "reql_port", port_offset);
     }
 
-    std::string get_addresses_string(
-        std::set<ip_address_t> actual_addresses) const;
+    static std::string get_addresses_string(std::set<ip_address_t> actual_addresses);
 
-    bool is_bind_all(std::set<ip_address_t> addresses) const;
+    static bool is_bind_all(const std::set<ip_address_t> &addresses);
 
     std::set<ip_address_t> local_addresses;
     std::set<ip_address_t> local_addresses_cluster;
