Bug #30617645 MYSQLROUTER DOES NOT ROUTE WHEN MYSQLX PORT VALUE IS INVALID

Andrzej Religa · dahlerlend · commit 450c8bc8b740 · 2019-12-04T18:17:16.000+01:00
Fix for Bug #30354273 SUPPORT IPV6 ADDRESSES FOUND IN THE METADATA.
changed the way the host and port in the metadata are parsed.
As a result of that change now if the mysqlx port is invalid the node
is discarded as a candidate also for a classic protocol connections.
This is a change of behaviour and on top of the Shell bug#27677227
can cause the users stop being able to connect to cluster nodes via
the classic protocol if they upgrade the Router without fixing the
invalid metadata entries.

This patch restores the old (pre .19) behavior and silently ignores
the invalid mysqlx port in the metadata, letting the node still be used
for the classic protocol connections.

RB: 23492
diff --git a/router/src/metadata_cache/src/cluster_metadata.cc b/router/src/metadata_cache/src/cluster_metadata.cc
@@ -190,10 +190,17 @@ bool set_instance_ports(metadata_cache::ManagedInstance &instance,
       const auto addr_port = mysqlrouter::split_addr_port(x_port);
       instance.xport = addr_port.second != 0 ? addr_port.second : 33060;
     } catch (const std::runtime_error &e) {
-      log_warning(
-          "Error parsing host:xport in metadata for instance %s: '%s': %s",
-          instance.mysql_server_uuid.c_str(), row[x_port_column], e.what());
-      return false;
+      // There is a Shell bug (#27677227) that can cause the mysqlx port be
+      // invalid in the metadata (>65535). For the backward compatibility we
+      // need to tolerate this and still let the node be used for classic
+      // connections (as the older Router versions did).
+
+      // log_warning(
+      //   "Error parsing host:xport in metadata for instance %s:"
+      //   "'%s': %s",
+      //   instance.mysql_server_uuid.c_str(), row[x_port_column],
+      //   e.what());
+      instance.xport = 0;
     }
   } else {
     instance.xport = instance.port * 10;
diff --git a/router/tests/component/test_metadata_ttl.cc b/router/tests/component/test_metadata_ttl.cc
@@ -126,28 +126,41 @@ class MetadataChacheTTLTest : public RouterComponentTest {
     return get_int_field_value(json_string, "update_last_check_in_count");
   }
 
-  bool wait_for_refresh_thread_started(const ProcessWrapper &router,
-                                       std::chrono::milliseconds timeout) {
+  bool wait_log_contains(const ProcessWrapper &router,
+                         const std::string &needle,
+                         std::chrono::milliseconds timeout) {
     if (getenv("WITH_VALGRIND")) {
       timeout *= 10;
     }
 
-    const auto MSEC_STEP = 10ms;
-    bool thread_started = false;
+    const auto MSEC_STEP = 50ms;
+    bool found = false;
     const auto started = std::chrono::steady_clock::now();
     do {
       const std::string log_content = router.get_full_logfile();
-      const std::string needle = "Starting metadata cache refresh thread";
-      thread_started = (log_content.find(needle) != log_content.npos);
-      if (!thread_started) {
+      found = (log_content.find(needle) != log_content.npos);
+      if (!found) {
         auto step = std::min(timeout, MSEC_STEP);
         std::this_thread::sleep_for(std::chrono::milliseconds(step));
         timeout -= step;
       }
-    } while (!thread_started &&
-             timeout > std::chrono::steady_clock::now() - started);
+    } while (!found && timeout > std::chrono::steady_clock::now() - started);
 
-    return thread_started;
+    return found;
+  }
+
+  bool wait_for_refresh_thread_started(
+      const ProcessWrapper &router, const std::chrono::milliseconds timeout) {
+    const std::string needle = "Starting metadata cache refresh thread";
+
+    return wait_log_contains(router, needle, timeout);
+  }
+
+  bool wait_metadata_read(const ProcessWrapper &router,
+                          const std::chrono::milliseconds timeout) {
+    const std::string needle = "Potential changes detected in cluster";
+
+    return wait_log_contains(router, needle, timeout);
   }
 
   auto &launch_router(const std::string &temp_test_dir,
@@ -481,6 +494,73 @@ INSTANTIATE_TEST_CASE_P(
                               "unordered_ar_v2", ClusterType::RS_V2, "0.1")),
     get_test_description);
 
+class MetadataChacheTTLTestInvalidMysqlXPort
+    : public MetadataChacheTTLTest,
+      public ::testing::WithParamInterface<MetadataTTLTestParams> {};
+
+/**
+ * @test Check that invalid mysqlx port in the metadata does not cause the node
+ * to be discarded for the classic protocol connections (Bug#30617645)
+ */
+TEST_P(MetadataChacheTTLTestInvalidMysqlXPort, InvalidMysqlXPort) {
+  TempDirectory temp_test_dir;
+  TempDirectory conf_dir("conf");
+
+  const std::string json_metadata =
+      get_data_dir().join(GetParam().tracefile).str();
+
+  SCOPED_TRACE("// single node cluster is fine for this test");
+  const uint16_t node_classic_port{port_pool_.get_next_available()};
+  const uint16_t node_http_port{port_pool_.get_next_available()};
+  const uint32_t kInvalidPort{76000};
+
+  auto &cluster_node = launch_mysql_server_mock(
+      json_metadata, node_classic_port, EXIT_SUCCESS, false, node_http_port);
+  ASSERT_NO_FATAL_FAILURE(check_port_ready(cluster_node, node_classic_port));
+
+  ASSERT_TRUE(
+      MockServerRestClient(node_http_port).wait_for_rest_endpoint_ready())
+      << cluster_node.get_full_output();
+
+  SCOPED_TRACE(
+      "// let the metadata for our single node report invalid mysqlx port");
+  set_mock_metadata(node_http_port, "", {node_classic_port}, 0, 0, false,
+                    "127.0.0.1", {kInvalidPort});
+
+  SCOPED_TRACE("// launch the router with metadata-cache configuration");
+  const auto router_port = port_pool_.get_next_available();
+  const std::string metadata_cache_section = get_metadata_cache_section(
+      {node_classic_port}, GetParam().cluster_type, GetParam().ttl);
+  const std::string routing_section = get_metadata_cache_routing_section(
+      router_port, "PRIMARY", "first-available");
+  auto &router = launch_router(temp_test_dir.name(), conf_dir.name(),
+                               metadata_cache_section, routing_section,
+                               EXIT_SUCCESS, true);
+
+  ASSERT_NO_FATAL_FAILURE(check_port_ready(router, router_port));
+  ASSERT_TRUE(wait_metadata_read(router, 5000ms)) << router.get_full_output();
+
+  SCOPED_TRACE(
+      "// Even though the metadata contains invalid mysqlx port we still "
+      "should be able to connect on the classic port");
+  MySQLSession client;
+  try {
+    client.connect("127.0.0.1", router_port, "username", "password", "", "");
+  } catch (...) {
+    FAIL() << router.get_full_logfile();
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(
+    InvalidMysqlXPort, MetadataChacheTTLTestInvalidMysqlXPort,
+    ::testing::Values(MetadataTTLTestParams("metadata_dynamic_nodes_v2_gr.js",
+                                            "gr_v2", ClusterType::GR_V1, "5"),
+                      MetadataTTLTestParams("metadata_dynamic_nodes.js", "gr",
+                                            ClusterType::GR_V2, "5"),
+                      MetadataTTLTestParams("metadata_dynamic_nodes_v2_ar.js",
+                                            "ar_v2", ClusterType::RS_V2, "5")),
+    get_test_description);
+
 /**
  * @test Checks that the router operates smoothly when the metadata version has
  * changed between the metadata refreshes.
diff --git a/router/tests/helpers/mock_server_testutils.cc b/router/tests/helpers/mock_server_testutils.cc
@@ -51,21 +51,31 @@ std::string json_to_string(const JsonValue &json_doc) {
   return out_buffer.GetString();
 }
 
-JsonValue mock_GR_metadata_as_json(const std::string &gr_id,
-                                   const std::vector<uint16_t> &gr_node_ports,
-                                   unsigned primary_id, unsigned view_id,
-                                   bool error_on_md_query,
-                                   const std::string &gr_node_host) {
+JsonValue mock_GR_metadata_as_json(
+    const std::string &gr_id, const std::vector<uint16_t> &gr_node_ports,
+    unsigned primary_id, unsigned view_id, bool error_on_md_query,
+    const std::string &gr_node_host,
+    const std::vector<uint32_t> &gr_node_xports) {
   JsonValue json_doc(rapidjson::kObjectType);
   JsonAllocator allocator;
   json_doc.AddMember(
       "gr_id", JsonValue(gr_id.c_str(), gr_id.length(), allocator), allocator);
 
   JsonValue gr_nodes_json(rapidjson::kArrayType);
+  if (!gr_node_xports.empty() &&
+      gr_node_xports.size() != gr_node_ports.size()) {
+    throw std::runtime_error("gr_node_xports.size() != gr_node_ports.size(), " +
+                             std::to_string(gr_node_xports.size()) +
+                             " != " + std::to_string(gr_node_ports.size()));
+  }
+  size_t i = 0;
   for (auto &gr_node : gr_node_ports) {
     JsonValue node(rapidjson::kArrayType);
     node.PushBack(static_cast<int>(gr_node), allocator);
     node.PushBack(JsonValue("ONLINE", strlen("ONLINE"), allocator), allocator);
+    if (!gr_node_xports.empty())
+      node.PushBack(static_cast<int>(gr_node_xports[i]), allocator);
+    ++i;
     gr_nodes_json.PushBack(node, allocator);
   }
   json_doc.AddMember("gr_nodes", gr_nodes_json, allocator);
@@ -85,11 +95,11 @@ JsonValue mock_GR_metadata_as_json(const std::string &gr_id,
 void set_mock_metadata(uint16_t http_port, const std::string &gr_id,
                        const std::vector<uint16_t> &gr_node_ports,
                        unsigned primary_id, unsigned view_id,
-                       bool error_on_md_query,
-                       const std::string &gr_node_host) {
+                       bool error_on_md_query, const std::string &gr_node_host,
+                       const std::vector<uint32_t> &gr_node_xports) {
   const auto json_doc =
       mock_GR_metadata_as_json(gr_id, gr_node_ports, primary_id, view_id,
-                               error_on_md_query, gr_node_host);
+                               error_on_md_query, gr_node_host, gr_node_xports);
 
   const auto json_str = json_to_string(json_doc);
 
diff --git a/router/tests/helpers/mock_server_testutils.h b/router/tests/helpers/mock_server_testutils.h
@@ -53,38 +53,46 @@ using JsonStringBuffer =
  * Converts the GR mock data to the JSON object.
  *
  * @param gr_id replication group id to set
- * @param gr_node_ports vector with the ports of the nodes that mock server
+ * @param gr_node_ports vector with the classic protocol ports of the cluster
+ * nodes
  * @param primary_id which node is the primary
  * @param view_id metadata view id (for AR cluster)
  * @param error_on_md_query if true the mock should return an error when
  * handling the metadata query
  * @param gr_node_host address of the host with the nodes
+ * @param gr_node_xports vector with the X protocol ports of the cluster nodes
+ * reported by the metadata
  *
  * @return JSON object with the GR mock data.
  */
 JsonValue mock_GR_metadata_as_json(
     const std::string &gr_id, const std::vector<uint16_t> &gr_node_ports,
     unsigned primary_id = 0, unsigned view_id = 0,
     bool error_on_md_query = false,
-    const std::string &gr_node_host = "127.0.0.1");
+    const std::string &gr_node_host = "127.0.0.1",
+    const std::vector<uint32_t> &gr_node_xports = {});
 
 /**
  * Sets the metadata returned by the mock server.
  *
  * @param http_port mock server's http port where it services the http requests
  * @param gr_id replication group id to set
- * @param gr_node_ports vector with the ports of the nodes that mock server
+ * @param gr_node_ports vector with the classic protocol ports of the cluster
+ * nodes
  * @param primary_id which node is the primary
  * @param view_id metadata view id (for AR cluster)
  * @param error_on_md_query if true the mock should return an error when
- * @param gr_node_host address of the host with the nodes
- * handling the metadata query
+ * @param gr_node_host address of the host with the nodes handling the metadata
+ * query
+ * @param gr_node_xports vector with the X protocol ports of the cluster nodes
+ * reported by the metadata
  */
 void set_mock_metadata(uint16_t http_port, const std::string &gr_id,
                        const std::vector<uint16_t> &gr_node_ports,
                        unsigned primary_id = 0, unsigned view_id = 0,
                        bool error_on_md_query = false,
-                       const std::string &gr_node_host = "127.0.0.1");
+                       const std::string &gr_node_host = "127.0.0.1",
+                       const std::vector<uint32_t> &gr_node_xports = {});
 
 void set_mock_bootstrap_data(
     uint16_t http_port, const std::string &cluster_name,
