Stricter NodeInfoNetwork handling (#7576)

Author: achamayou

include/ccf/service/node_info_network.h

@@ -206,27 +206,12 @@ namespace ccf
     return fmt::format("{}:{}", host, port);
   }
 
-  // The JSON representation of a NodeInfoNetwork is the union of a
-  // NodeInfoNetwork_v1 and a NodeInfoNetwork_v2. It contains the fields of
-  // both, so can be read as (or from!) either
+  // All NodeInfoNetwork read that may lead to re-serialization for
+  // write purposes must be v2 by now, so we only serialize as v2.
+  // If any v1 NodeInfoNetwork is read at all, it must be for historical
+  // query purposes, and does not need to be re-serialized to v2.
   inline void to_json(nlohmann::json& j, const NodeInfoNetwork& nin)
   {
-    {
-      NodeInfoNetwork_v1 v1;
-      std::tie(v1.nodehost, v1.nodeport) =
-        split_net_address(nin.node_to_node_interface.bind_address);
-
-      if (!nin.rpc_interfaces.empty())
-      {
-        const auto& primary_interface = nin.rpc_interfaces.begin()->second;
-        std::tie(v1.rpchost, v1.rpcport) =
-          split_net_address(primary_interface.bind_address);
-        std::tie(v1.pubhost, v1.pubport) =
-          split_net_address(primary_interface.published_address);
-      }
-      to_json(j, v1);
-    }
-
     to_json(j, (const NodeInfoNetwork_v2&)nin);
   }
 
@@ -243,6 +228,14 @@ namespace ccf
       NodeInfoNetwork_v1 v1;
       try
       {
+        if (
+          j.contains("rpc_interfaces") || j.contains("node_to_node_interface"))
+        {
+          // If these v2 fields are present, rethrow the error - the JSON is
+          // malformed for v2. Only proceed to parse as v1 if these fields are
+          // absent and the NodeInfoNetwork is a pure v1.
+          throw jpe;
+        }
         from_json(j, v1);
       }
       catch (const ccf::JsonParseError& _)
@@ -255,6 +248,9 @@ namespace ccf
 
       nin.node_to_node_interface.bind_address =
         make_net_address(v1.nodehost, v1.nodeport);
+      // If published address is not explicitly set, default to bind address
+      nin.node_to_node_interface.published_address =
+        make_net_address(v1.nodehost, v1.nodeport);
 
       NodeInfoNetwork::NetInterface primary_interface;
       primary_interface.bind_address = make_net_address(v1.rpchost, v1.rpcport);

src/node/test/node_info_json.cpp

@@ -31,7 +31,7 @@ TEST_CASE("Multiple versions of NodeInfoNetwork")
 
   ccf::NodeInfoNetwork_v1 v1;
   std::tie(v1.nodehost, v1.nodeport) = ccf::split_net_address(node);
-  std::tie(v1.rpchost, v1.nodeport) = ccf::split_net_address(rpc_a);
+  std::tie(v1.rpchost, v1.rpcport) = ccf::split_net_address(rpc_a);
   std::tie(v1.pubhost, v1.pubport) = ccf::split_net_address(rpc_b);
 
   {
@@ -41,56 +41,58 @@ TEST_CASE("Multiple versions of NodeInfoNetwork")
     REQUIRE(current == converted);
   }
 
+  // to_json(NodeInfoNetwork) NEVER writes v1 fields now
+  // No current node should be using v1 anymore
   {
-    INFO("Old format survives round-trip through current");
+    INFO("Old format loses old fields when converted to new format");
     nlohmann::json j = v1;
-    const auto intermediate = j.get<ccf::NodeInfoNetwork>();
-    nlohmann::json j2 = intermediate;
-    const auto converted = j2.get<ccf::NodeInfoNetwork_v1>();
-
-    // Manual equality check - not implementing it now for a deprecated format
-    REQUIRE(v1.nodehost == converted.nodehost);
-    REQUIRE(v1.nodeport == converted.nodeport);
-    REQUIRE(v1.rpchost == converted.rpchost);
-    REQUIRE(v1.rpcport == converted.rpcport);
-    REQUIRE(v1.pubhost == converted.pubhost);
-    REQUIRE(v1.pubport == converted.pubport);
+    nlohmann::json converted;
+    to_json(converted, j.get<ccf::NodeInfoNetwork>());
+    const auto dumped_converted = converted.dump();
+    const auto deserialized_converted = nlohmann::json::parse(dumped_converted);
+
+    // v1 fields are not present anymore
+    REQUIRE(!deserialized_converted.contains("nodehost"));
+    REQUIRE(!deserialized_converted.contains("nodeport"));
+    REQUIRE(!deserialized_converted.contains("rpchost"));
+    REQUIRE(!deserialized_converted.contains("rpcport"));
+    REQUIRE(!deserialized_converted.contains("pubhost"));
+    REQUIRE(!deserialized_converted.contains("pubport"));
+
+    const auto new_converted =
+      deserialized_converted.get<ccf::NodeInfoNetwork>();
+
+    // v2 fields have been constructed correctly
+    REQUIRE(
+      new_converted.node_to_node_interface.bind_address ==
+      ccf::NodeInfoNetwork::NetAddress(v1.nodehost + ":" + v1.nodeport));
+    REQUIRE(
+      new_converted.node_to_node_interface.published_address ==
+      ccf::NodeInfoNetwork::NetAddress(v1.nodehost + ":" + v1.nodeport));
+
+    REQUIRE(new_converted.rpc_interfaces.size() == 1);
+    const auto& primary_rpc_it =
+      new_converted.rpc_interfaces.find(ccf::PRIMARY_RPC_INTERFACE);
+    const auto& primary_rpc = primary_rpc_it->second;
+    REQUIRE(
+      primary_rpc.bind_address ==
+      ccf::NodeInfoNetwork::NetAddress(v1.rpchost + ":" + v1.rpcport));
+    REQUIRE(
+      primary_rpc.published_address ==
+      ccf::NodeInfoNetwork::NetAddress(v1.pubhost + ":" + v1.pubport));
   }
 
+  // Test that slightly malformed v2 JSON does not get misparsed as v1
+  // and triggers an exception instead, for example when an unknown
+  // operator feature is present
   {
-    INFO(
-      "Current format loses some information when round-tripping through old");
+    INFO("Malformed new format does not get misparsed as old format");
     nlohmann::json j = current;
-    const auto intermediate = j.get<ccf::NodeInfoNetwork_v1>();
-    nlohmann::json j2 = intermediate;
-    const auto converted = j2.get<ccf::NodeInfoNetwork>();
-    REQUIRE(!(current == converted));
-
-    // The node information has been kept
-    REQUIRE(current.node_to_node_interface == converted.node_to_node_interface);
-
-    // Only the _first_ RPC interface has kept its addresses, though lost its
-    // sessions caps
-    REQUIRE(converted.rpc_interfaces.size() > 0);
-
-    const auto& current_interface = current.rpc_interfaces.begin()->second;
-    const auto& converted_interface =
-      converted.rpc_interfaces.at(ccf::PRIMARY_RPC_INTERFACE);
-
-    REQUIRE(current_interface.bind_address == converted_interface.bind_address);
-    REQUIRE(
-      current_interface.published_address ==
-      converted_interface.published_address);
-    REQUIRE(
-      current_interface.max_open_sessions_hard !=
-      converted_interface.max_open_sessions_hard);
-    REQUIRE(
-      current_interface.max_open_sessions_soft !=
-      converted_interface.max_open_sessions_soft);
+    // Inject an unknown operator feature to make the JSON invalid for v2
+    j["node_to_node_interface"]["enabled_operator_features"].push_back(
+      "UnknownFeature");
 
-    // The second RPC interface has been lost
-    REQUIRE(converted.rpc_interfaces.size() == 1);
-    REQUIRE(converted.rpc_interfaces.size() < current.rpc_interfaces.size());
+    REQUIRE_THROWS_AS(j.get<ccf::NodeInfoNetwork>(), ccf::JsonParseError);
   }
 
   {