localhost_only prevails auto discovery options if enabled. (ros2#1069)

fujitatomoya · web-flow · commit 101e406577a7 · 2023-05-16T20:12:20.000-04:00
* localhost_only prevails auto discovery options if enabled.

Signed-off-by: Tomoya Fujita &lt;Tomoya.Fujita@sony.com&gt;
diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c
@@ -159,8 +159,8 @@ rcl_init(
   if (RMW_LOCALHOST_ONLY_DEFAULT != *localhost_only) {
     RCUTILS_LOG_WARN_NAMED(
       ROS_PACKAGE_NAME,
-      "'localhost_only' init option is deprecated. Use 'automatic_discovery_range' and "
-      "'static_peers' instead.");
+      "'localhost_only' init option is deprecated but still honored if it is enabled. "
+      "Use 'automatic_discovery_range' and 'static_peers' instead.");
   } else {
     // Get actual localhost_only value based on environment variable, if needed.
     ret = rcl_get_localhost_only(localhost_only);
@@ -171,8 +171,8 @@ rcl_init(
     if (RMW_LOCALHOST_ONLY_DEFAULT != *localhost_only) {
       RCUTILS_LOG_WARN_NAMED(
         ROS_PACKAGE_NAME,
-        "ROS_LOCALHOST_ONLY is deprecated. Use ROS_AUTOMATIC_DISCOVERY_RANGE and "
-        "ROS_STATIC_PEERS instead.");
+        "ROS_LOCALHOST_ONLY is deprecated but still honored if it is enabled. "
+        "Use ROS_AUTOMATIC_DISCOVERY_RANGE and ROS_STATIC_PEERS instead.");
     }
   }
 
@@ -181,38 +181,58 @@ rcl_init(
   rmw_discovery_options_t * discovery_options =
     &context->impl->init_options.impl->rmw_init_options.discovery_options;
 
-  // Get actual discovery range option based on environment variable, if not given
-  // to original options passed to function
-  if (  // NOLINT
-    RMW_AUTOMATIC_DISCOVERY_RANGE_NOT_SET == original_discovery_options.automatic_discovery_range)
-  {
-    ret = rcl_get_automatic_discovery_range(discovery_options);
-    if (RCL_RET_OK != ret) {
-      fail_ret = ret;
-      goto fail;
+  // this happens either rmw implementation forces or via environmental variable.
+  // localhost_only is deprecated but still honored to prevail discovery_options.
+  // see https://github.com/ros2/ros2_documentation/pull/3519#discussion_r1186541935
+  // TODO(fujitatomoya): remove localhost_only completely after deprecation period.
+  if (*localhost_only == RMW_LOCALHOST_ONLY_ENABLED) {
+    RCUTILS_LOG_WARN_NAMED(
+      ROS_PACKAGE_NAME,
+      "'localhost_only' is enabled, "
+      "'automatic_discovery_range' and 'static_peers' will be ignored.");
+    discovery_options->automatic_discovery_range = RMW_AUTOMATIC_DISCOVERY_RANGE_LOCALHOST;
+    discovery_options->static_peers_count = 0;
+  } else {
+    if (*localhost_only == RMW_LOCALHOST_ONLY_DISABLED) {
+      RCUTILS_LOG_WARN_NAMED(
+        ROS_PACKAGE_NAME,
+        "'localhost_only' is disabled, "
+        "'automatic_discovery_range' and 'static_peers' will be used.");
     }
-  }
 
-  if (0 == discovery_options->static_peers_count &&
-    discovery_options->automatic_discovery_range != RMW_AUTOMATIC_DISCOVERY_RANGE_OFF)
-  {
-    // Get static peers.
-    // If off is set, it makes sense to not get any static peers.
-    ret = rcl_get_discovery_static_peers(discovery_options, &allocator);
-    if (RCL_RET_OK != ret) {
-      fail_ret = ret;
-      goto fail;
+    // Get actual discovery range option based on environment variable, if not given
+    // to original options passed to function
+    if (  // NOLINT
+      RMW_AUTOMATIC_DISCOVERY_RANGE_NOT_SET == original_discovery_options.automatic_discovery_range)
+    {
+      ret = rcl_get_automatic_discovery_range(discovery_options);
+      if (RCL_RET_OK != ret) {
+        fail_ret = ret;
+        goto fail;
+      }
     }
-  }
 
-  if (discovery_options->static_peers_count > 0 &&
-    discovery_options->automatic_discovery_range == RMW_AUTOMATIC_DISCOVERY_RANGE_OFF)
-  {
-    RCUTILS_LOG_WARN_NAMED(
-      ROS_PACKAGE_NAME,
-      "Note: ROS_AUTOMATIC_DISCOVERY_RANGE is set to OFF, but "
-      "found static peers in ROS_STATIC_PEERS. "
-      "ROS_STATIC_PEERS will be ignored.");
+    if (0 == discovery_options->static_peers_count &&
+      discovery_options->automatic_discovery_range != RMW_AUTOMATIC_DISCOVERY_RANGE_OFF)
+    {
+      // Get static peers.
+      // If off is set, it makes sense to not get any static peers.
+      ret = rcl_get_discovery_static_peers(discovery_options, &allocator);
+      if (RCL_RET_OK != ret) {
+        fail_ret = ret;
+        goto fail;
+      }
+    }
+
+    if (discovery_options->static_peers_count > 0 &&
+      discovery_options->automatic_discovery_range == RMW_AUTOMATIC_DISCOVERY_RANGE_OFF)
+    {
+      RCUTILS_LOG_WARN_NAMED(
+        ROS_PACKAGE_NAME,
+        "Note: ROS_AUTOMATIC_DISCOVERY_RANGE is set to OFF, but "
+        "found static peers in ROS_STATIC_PEERS. "
+        "ROS_STATIC_PEERS will be ignored.");
+    }
   }
 
   const char * discovery_range_string =
diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt
@@ -402,6 +402,7 @@ rcl_add_custom_gtest(test_discovery_options
   SRCS rcl/test_discovery_options.cpp
   APPEND_LIBRARY_DIRS ${extra_lib_dirs}
   LIBRARIES ${PROJECT_NAME}
+  AMENT_DEPENDENCIES "osrf_testing_tools_cpp"
 )
 
 rcl_add_custom_gtest(test_domain_id
diff --git a/rcl/test/rcl/test_discovery_options.cpp b/rcl/test/rcl/test_discovery_options.cpp
@@ -14,14 +14,45 @@
 
 #include <gtest/gtest.h>
 
-#include "rcl/rcl.h"
+#include "osrf_testing_tools_cpp/scope_exit.hpp"
+
 #include "rcl/discovery_options.h"
+#include "rcl/rcl.h"
 
 #include "rcutils/allocator.h"
 #include "rcutils/env.h"
 
 #include "rmw/discovery_options.h"
 
+#include "../src/rcl/context_impl.h"
+#include "../src/rcl/init_options_impl.h"
+
+void check_discovery(
+  rmw_automatic_discovery_range_t discovery_range,
+  size_t static_peer_count)
+{
+  rcl_ret_t ret;
+  rcl_init_options_t init_options = rcl_get_zero_initialized_init_options();
+  ret = rcl_init_options_init(&init_options, rcl_get_default_allocator());
+  ASSERT_EQ(RCL_RET_OK, ret);
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
+  {
+    EXPECT_EQ(RCL_RET_OK, rcl_init_options_fini(&init_options));
+  });
+  rcl_context_t context = rcl_get_zero_initialized_context();
+  ret = rcl_init(0, nullptr, &init_options, &context);
+  ASSERT_EQ(RCL_RET_OK, ret);
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
+  {
+    ASSERT_EQ(RCL_RET_OK, rcl_shutdown(&context));
+    ASSERT_EQ(RCL_RET_OK, rcl_context_fini(&context));
+  });
+  rmw_discovery_options_t * discovery_options =
+    &context.impl->init_options.impl->rmw_init_options.discovery_options;
+  EXPECT_EQ(discovery_range, discovery_options->automatic_discovery_range);
+  EXPECT_EQ(static_peer_count, discovery_options->static_peers_count);
+}
+
 TEST(TestDiscoveryInfo, test_get_peers) {
   rcutils_allocator_t allocator = rcutils_get_default_allocator();
   rmw_discovery_options_t discovery_options_var = rmw_get_zero_initialized_discovery_options();
@@ -241,3 +272,49 @@ TEST(TestDiscoveryInfo, test_get_both) {
   EXPECT_EQ(0u, discovery_options_var.static_peers_count);
   EXPECT_EQ(RCL_RET_OK, rmw_discovery_options_fini(&discovery_options_var));
 }
+
+// localhost_only is deprecated but still honored to prevail discovery_options.
+// see https://github.com/ros2/ros2_documentation/pull/3519#discussion_r1186541935
+// TODO(fujitatomoya): remove localhost_only completely after deprecation period.
+TEST(TestDiscoveryInfo, test_with_localhost_only) {
+  {
+    // No environment variable set (default subnet, no specific peers)
+    check_discovery(RMW_AUTOMATIC_DISCOVERY_RANGE_SUBNET, 0);
+  }
+
+  {
+    // only ROS_AUTOMATIC_DISCOVERY_RANGE and ROS_STATIC_PEERS set
+    ASSERT_TRUE(rcutils_set_env("ROS_AUTOMATIC_DISCOVERY_RANGE", "LOCALHOST"));
+    ASSERT_TRUE(rcutils_set_env("ROS_STATIC_PEERS", "127.0.0.1;localhost.com"));
+    check_discovery(RMW_AUTOMATIC_DISCOVERY_RANGE_LOCALHOST, 2);
+  }
+
+  {
+    // Only ROS_LOCALHOST_ONLY is enabled
+    ASSERT_TRUE(rcutils_set_env("ROS_LOCALHOST_ONLY", "1"));
+    check_discovery(RMW_AUTOMATIC_DISCOVERY_RANGE_LOCALHOST, 0);
+  }
+
+  {
+    // ROS_LOCALHOST_ONLY is enabled and prevails over SUBNET.
+    ASSERT_TRUE(rcutils_set_env("ROS_LOCALHOST_ONLY", "1"));
+    ASSERT_TRUE(rcutils_set_env("ROS_AUTOMATIC_DISCOVERY_RANGE", "SUBNET"));
+    ASSERT_TRUE(rcutils_set_env("ROS_STATIC_PEERS", "192.168.0.1;remote.com"));
+    check_discovery(RMW_AUTOMATIC_DISCOVERY_RANGE_LOCALHOST, 0);
+  }
+
+  {
+    // ROS_LOCALHOST_ONLY is enabled and prevails over OFF.
+    ASSERT_TRUE(rcutils_set_env("ROS_LOCALHOST_ONLY", "1"));
+    ASSERT_TRUE(rcutils_set_env("ROS_AUTOMATIC_DISCOVERY_RANGE", "OFF"));
+    check_discovery(RMW_AUTOMATIC_DISCOVERY_RANGE_LOCALHOST, 0);
+  }
+
+  {
+    // ROS_LOCALHOST_ONLY is disabled, falls down to use discovery option.
+    ASSERT_TRUE(rcutils_set_env("ROS_LOCALHOST_ONLY", "0"));
+    ASSERT_TRUE(rcutils_set_env("ROS_AUTOMATIC_DISCOVERY_RANGE", "SUBNET"));
+    ASSERT_TRUE(rcutils_set_env("ROS_STATIC_PEERS", "192.168.0.1;remote.com"));
+    check_discovery(RMW_AUTOMATIC_DISCOVERY_RANGE_SUBNET, 2);
+  }
+}

Original file line number	Diff line number	Diff line change
`@@ -402,6 +402,7 @@ rcl_add_custom_gtest(test_discovery_options`
`402`	`402`	`SRCS rcl/test_discovery_options.cpp`
`403`	`403`	`APPEND_LIBRARY_DIRS ${extra_lib_dirs}`
`404`	`404`	`LIBRARIES ${PROJECT_NAME}`
	`405`	`+ AMENT_DEPENDENCIES "osrf_testing_tools_cpp"`
`405`	`406`	`)`
`406`	`407`
`407`	`408`	`rcl_add_custom_gtest(test_domain_id`