common/pick_address: prefer non-loopback addresses

instead of filtering out loopback ifaces, check for loopback addresses,
and prefer non-loopback addresses over loopback addresses.

before this change, iface named "lo" is filtered out by default,
and "lo" is allowed if `ms_bind_exclude_lo_iface` is false.

after this change, iface with address out of 127/8 is prefered.
the iface marked down is not considered.

the option of "ms_bind_exclude_lo_iface" is removed. the tests are
updated accordingly.

Fixes: https://tracker.ceph.com/issues/50456
Signed-off-by: Kefu Chai <kchai@redhat.com>

diff --git a/src/common/options/global.yaml.in b/src/common/options/global.yaml.in
index ebb9ca3fe7d98a520d065160d038cd8a9eea22db..5849b9b7e6e61bbc13792f5bbf7887680efa7c8b 100644 (file)
--- a/src/common/options/global.yaml.in
+++ b/src/common/options/global.yaml.in
@@ -1173,13 +1173,6 @@ options:
   fmt_desc: Throttles total size of messages waiting to be dispatched.
   default: 100_M
   with_legacy: true
-- name: ms_bind_exclude_lo_iface
-  type: bool
-  level: advanced
-  desc: Allow servers to bind loopback network interfaces (lo)
-  default: true
-  flags:
-  - startup
 - name: ms_bind_ipv4
   type: bool
   level: advanced

diff --git a/src/common/pick_address.cc b/src/common/pick_address.cc
index 47f8875e0cedf7bd87192498d65c3c7e9918afbf..45066a836cdecda7602957ca0446db2426e75b07 100644 (file)
--- a/src/common/pick_address.cc
+++ b/src/common/pick_address.cc
@@ -15,6 +15,7 @@
 #include "common/pick_address.h"
 
 #include <netdb.h>
+#include <netinet/in.h>
 #include <string>
 #include <string.h>
 #include <vector>
@@ -45,15 +46,42 @@ bool matches_with_name(const ifaddrs& ifa, const std::string& if_name)
   return if_name.compare(ifa.ifa_name) == 0;
 }
 
-bool is_addr_allowed(const ifaddrs& ifa, bool exclude_lo_iface)
+static int is_loopback_addr(sockaddr* addr)
 {
-  if (exclude_lo_iface && strcmp(ifa.ifa_name, "lo") == 0) {
-    return false;
+  if (addr->sa_family == AF_INET) {
+    const sockaddr_in* sin = (struct sockaddr_in *)(addr);
+    const in_addr_t net = ntohl(sin->sin_addr.s_addr) >> IN_CLASSA_NSHIFT;
+    return net == IN_LOOPBACKNET ? 1 : 0;
+  } else if (addr->sa_family == AF_INET6) {
+    sockaddr_in6* sin6 = (struct sockaddr_in6 *)(addr);
+    return IN6_IS_ADDR_LOOPBACK(&sin6->sin6_addr) ? 1 : 0;
+  } else {
+    return -1;
   }
-  if (boost::starts_with(ifa.ifa_name, "lo:")) {
-    return false;
+}
+
+static int grade_addr(const ifaddrs& ifa)
+{
+  if (ifa.ifa_addr == nullptr) {
+    return -1;
   }
-  return true;
+  int score = 0;
+  if (ifa.ifa_flags & IFF_UP) {
+    score += 4;
+  }
+  switch (is_loopback_addr(ifa.ifa_addr)) {
+  case 0:
+    // prefer non-loopback addresses
+    score += 2;
+    break;
+  case 1:
+    score += 0;
+    break;
+  default:
+    score = -1;
+    break;
+  }
+  return score;
 }
 
 bool matches_with_net(const ifaddrs& ifa,
@@ -107,13 +135,13 @@ bool matches_with_numa_node(const ifaddrs& ifa, int numa_node)
 #endif
 }
 }
+
 const struct sockaddr *find_ip_in_subnet_list(
   CephContext *cct,
   const struct ifaddrs *ifa,
   unsigned ipv,
   const std::string &networks,
   const std::string &interfaces,
-  bool exclude_lo_iface,
   int numa_node)
 {
   const auto ifs = get_str_list(interfaces);
@@ -123,25 +151,32 @@ const struct sockaddr *find_ip_in_subnet_list(
       exit(1);
   }
 
+  int best_score = -1;
+  const sockaddr* best_addr = nullptr;
   for (const auto* addr = ifa; addr != nullptr; addr = addr->ifa_next) {
-    if (!is_addr_allowed(*addr, exclude_lo_iface)) {
-      continue;
-    }
-    if ((ifs.empty() ||
-         std::any_of(std::begin(ifs), std::end(ifs),
+    if (!ifs.empty() &&
+       std::none_of(std::begin(ifs), std::end(ifs),
                      [&](const auto& if_name) {
                        return matches_with_name(*addr, if_name);
-                     })) &&
-        (nets.empty() ||
-         std::any_of(std::begin(nets), std::end(nets),
+                     })) {
+      continue;
+    }
+    if (!nets.empty() &&
+       std::none_of(std::begin(nets), std::end(nets),
                      [&](const auto& net) {
                        return matches_with_net(cct, *addr, net, ipv);
-                     })) &&
-        matches_with_numa_node(*addr, numa_node)) {
-      return addr->ifa_addr;
+                     })) {
+      continue;
+    }
+    if (!matches_with_numa_node(*addr, numa_node)) {
+      continue;
+    }
+    if (int score = grade_addr(*addr); score > best_score) {
+      best_score = score;
+      best_addr = addr->ifa_addr;
     }
   }
-  return nullptr;
+  return best_addr;
 }
 
 #ifndef WITH_SEASTAR
@@ -166,7 +201,6 @@ static void fill_in_one_address(CephContext *cct,
                                const struct ifaddrs *ifa,
                                const string &networks,
                                const string &interfaces,
-                               bool exclude_lo_iface,
                                const char *conf_var,
                                int numa_node = -1)
 {
@@ -176,7 +210,6 @@ static void fill_in_one_address(CephContext *cct,
     CEPH_PICK_ADDRESS_IPV4|CEPH_PICK_ADDRESS_IPV6,
     networks,
     interfaces,
-    exclude_lo_iface,
     numa_node);
   if (!found) {
     lderr(cct) << "unable to find any IP address in networks '" << networks
@@ -229,27 +262,21 @@ void pick_addresses(CephContext *cct, int needs)
     exit(1);
   }
   auto free_ifa = make_scope_guard([ifa] { freeifaddrs(ifa); });
-
-  const bool exclude_lo_iface =
-    cct->_conf.get_val<bool>("ms_bind_exclude_lo_iface");
   if ((needs & CEPH_PICK_ADDRESS_PUBLIC) &&
     public_addr.is_blank_ip() && !public_network.empty()) {
     fill_in_one_address(cct, ifa, public_network, public_network_interface,
-                       exclude_lo_iface,
                        "public_addr");
   }
 
   if ((needs & CEPH_PICK_ADDRESS_CLUSTER) && cluster_addr.is_blank_ip()) {
     if (!cluster_network.empty()) {
       fill_in_one_address(cct, ifa, cluster_network, cluster_network_interface,
-                         exclude_lo_iface,
                          "cluster_addr");
     } else {
       if (!public_network.empty()) {
         lderr(cct) << "Public network was set, but cluster network was not set " << dendl;
         lderr(cct) << "    Using public network also for cluster network" << dendl;
         fill_in_one_address(cct, ifa, public_network, public_network_interface,
-                           exclude_lo_iface,
                            "cluster_addr");
       }
     }
@@ -263,14 +290,12 @@ static int fill_in_one_address(
   unsigned ipv,
   const string &networks,
   const string &interfaces,
-  bool exclude_lo_iface,
   entity_addrvec_t *addrs,
   int numa_node = -1)
 {
   const struct sockaddr *found = find_ip_in_subnet_list(cct, ifa, ipv,
                                                        networks,
                                                        interfaces,
-                                                       exclude_lo_iface,
                                                        numa_node);
   if (!found) {
     std::string ip_type = "";
@@ -384,8 +409,6 @@ int pick_addresses(
       !networks.empty()) {
     int ipv4_r = !(ipv & CEPH_PICK_ADDRESS_IPV4) ? 0 : -1;
     int ipv6_r = !(ipv & CEPH_PICK_ADDRESS_IPV6) ? 0 : -1;
-    const bool exclude_lo_iface =
-      cct->_conf.get_val<bool>("ms_bind_exclude_lo_iface");
     // first try on preferred numa node (if >= 0), then anywhere.
     while (true) {
       // note: pass in ipv to filter the matching addresses
@@ -393,14 +416,12 @@ int pick_addresses(
          (flags & CEPH_PICK_ADDRESS_PREFER_IPV4)) {
        ipv4_r = fill_in_one_address(cct, ifa, CEPH_PICK_ADDRESS_IPV4,
                                      networks, interfaces,
-                                    exclude_lo_iface,
                                     addrs,
                                      preferred_numa_node);
       }
       if (ipv & CEPH_PICK_ADDRESS_IPV6) {
        ipv6_r = fill_in_one_address(cct, ifa, CEPH_PICK_ADDRESS_IPV6,
                                      networks, interfaces,
-                                    exclude_lo_iface,
                                     addrs,
                                      preferred_numa_node);
       }
@@ -408,7 +429,6 @@ int pick_addresses(
          !(flags & CEPH_PICK_ADDRESS_PREFER_IPV4)) {
        ipv4_r = fill_in_one_address(cct, ifa, CEPH_PICK_ADDRESS_IPV4,
                                      networks, interfaces,
-                                    exclude_lo_iface,
                                     addrs,
                                      preferred_numa_node);
       }

diff --git a/src/common/pick_address.h b/src/common/pick_address.h
index 2a41f5a8d03c96acb952daf511d74922c3f6d5ac..1faf60dbd75bcf65f183311df305d7f2cbcc377e 100644 (file)
--- a/src/common/pick_address.h
+++ b/src/common/pick_address.h
@@ -88,7 +88,6 @@ const struct sockaddr *find_ip_in_subnet_list(
   unsigned ipv,
   const std::string &networks,
   const std::string &interfaces,
-  bool exclude_lo_iface,
   int numa_node=-1);
 
 int get_iface_numa_node(

diff --git a/src/test/test_ipaddr.cc b/src/test/test_ipaddr.cc
index d2ab8c50eeed37a34bac7bde8183863bd68effe7..def7df0b8c6f15202394050954abf942f067ad34 100644 (file)
--- a/src/test/test_ipaddr.cc
+++ b/src/test/test_ipaddr.cc
@@ -13,6 +13,7 @@
 #endif
 #include <arpa/inet.h>
 #include <ifaddrs.h>
+#include <net/if.h>
 
 static void ipv4(struct sockaddr_in *addr, const char *s) {
   int err;
@@ -190,10 +191,12 @@ TEST(CommonIPAddr, TestV4_SkipLoopback)
   one.ifa_name = lo;
 
   two.ifa_next = &three;
+  two.ifa_flags = IFF_UP;
   two.ifa_addr = (struct sockaddr*)&a_two;
   two.ifa_name = lo0;
 
   three.ifa_next = NULL;
+  three.ifa_flags = IFF_UP;
   three.ifa_addr = (struct sockaddr*)&a_three;
   three.ifa_name = eth0;
 
@@ -202,21 +205,18 @@ TEST(CommonIPAddr, TestV4_SkipLoopback)
   ipv4(&a_three, "10.1.2.3");
 
   const struct sockaddr *result = nullptr;
+  // we prefer the non-loopback address despite the loopback addresses
   result =
     find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
                            CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "10.0.0.0/8", "", true);
+                           "", "");
   ASSERT_EQ((struct sockaddr*)&a_three, result);
+  // the subnet criteria leaves us no choice but the UP loopback address
   result =
     find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
                            CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "127.0.0.0/8", "", true);
-  ASSERT_EQ(nullptr, result);
-  result =
-    find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
-                           CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "127.0.0.0/8", "", false);
-  ASSERT_EQ((struct sockaddr*)&a_one, result);
+                           "127.0.0.0/8", "");
+  ASSERT_EQ((struct sockaddr*)&a_two, result);
 }
 
 TEST(CommonIPAddr, TestV6_Simple)
@@ -316,43 +316,37 @@ TEST(CommonIPAddr, TestV6_SkipLoopback)
   struct sockaddr_in6 a_one;
   struct sockaddr_in6 a_two;
   struct sockaddr_in6 a_three;
-  struct sockaddr_in6 net;
-
-  memset(&net, 0, sizeof(net));
 
   one.ifa_next = &two;
+  ipv6(&a_one, "::1");
   one.ifa_addr = (struct sockaddr*)&a_one;
   one.ifa_name = lo;
 
   two.ifa_next = &three;
+  two.ifa_flags = IFF_UP;
+  ipv6(&a_two, "::1");
   two.ifa_addr = (struct sockaddr*)&a_two;
   two.ifa_name = lo0;
 
   three.ifa_next = NULL;
+  three.ifa_flags = IFF_UP;
+  ipv6(&a_three, "2001:1234:5678:90ab::beef");
   three.ifa_addr = (struct sockaddr*)&a_three;
   three.ifa_name = eth0;
 
-  ipv6(&a_one, "::1");
-  ipv6(&a_two, "::1");
-  ipv6(&a_three, "2001:1234:5678:90ab::beef");
-  ipv6(&net, "ff00::1");
-
   const struct sockaddr *result = nullptr;
+  // we prefer the non-loopback address despite the loopback addresses
   result =
     find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
                            CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "2001:1234:5678:90ab::0/64", "", true);
+                           "", "");
   ASSERT_EQ((struct sockaddr*)&a_three, result);
+  // the subnet criteria leaves us no choice but the UP loopback address
   result =
     find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
                            CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "::1/128", "", true);
-  ASSERT_EQ(nullptr, result);
-  result =
-    find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
-                           CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "::1/128", "", false);
-  ASSERT_EQ((struct sockaddr*)&a_one, result);
+                           "::1/128", "");
+  ASSERT_EQ((struct sockaddr*)&a_two, result);
 }
 
 TEST(CommonIPAddr, ParseNetwork_Empty)
@@ -709,8 +703,7 @@ TEST(pick_address, find_ip_in_subnet_list)
     &one,
     CEPH_PICK_ADDRESS_IPV4,
     "10.1.0.0/16",
-    "eth0",
-    true);
+    "eth0");
   ASSERT_EQ((struct sockaddr*)&a_one, result);
 
   result = find_ip_in_subnet_list(
@@ -718,8 +711,7 @@ TEST(pick_address, find_ip_in_subnet_list)
     &one,
     CEPH_PICK_ADDRESS_IPV4,
     "10.2.0.0/16",
-    "eth1",
-    true);
+    "eth1");
   ASSERT_EQ((struct sockaddr*)&a_two, result);
 
   // match by eth name
@@ -728,8 +720,7 @@ TEST(pick_address, find_ip_in_subnet_list)
     &one,
     CEPH_PICK_ADDRESS_IPV4,
     "10.0.0.0/8",
-    "eth0",
-    true);
+    "eth0");
   ASSERT_EQ((struct sockaddr*)&a_one, result);
 
   result = find_ip_in_subnet_list(
@@ -737,8 +728,7 @@ TEST(pick_address, find_ip_in_subnet_list)
     &one,
     CEPH_PICK_ADDRESS_IPV4,
     "10.0.0.0/8",
-    "eth1",
-    true);
+    "eth1");
   ASSERT_EQ((struct sockaddr*)&a_two, result);
 
   result = find_ip_in_subnet_list(
@@ -746,8 +736,7 @@ TEST(pick_address, find_ip_in_subnet_list)
     &one,
     CEPH_PICK_ADDRESS_IPV6,
     "2001::/16",
-    "eth1",
-    true);
+    "eth1");
   ASSERT_EQ((struct sockaddr*)&a_three, result);
 }