config: IPv6 v2 ResolvedAddresses no longer need square brackets. (#1455

)

Use the "try IPv6, then try IPv4 if it fails" parse algorithm.

source/common/config/address_json.cc

@@ -30,7 +30,7 @@ void AddressJson::translateResolvedAddress(const std::string& json_address, bool
       socket_address->set_ip_address(address->ip()->addressAsString());
       break;
     case Network::Address::IpVersion::v6:
-      socket_address->set_ip_address("[" + address->ip()->addressAsString() + "]");
+      socket_address->set_ip_address(address->ip()->addressAsString());
       break;
     }
     socket_address->mutable_port()->set_value(address->ip()->port());

source/common/network/utility.cc

@@ -76,17 +76,18 @@ uint32_t Utility::portFromTcpUrl(const std::string& url) {
   }
 }
 
-Address::InstanceConstSharedPtr Utility::parseInternetAddress(const std::string& ip_address) {
+Address::InstanceConstSharedPtr Utility::parseInternetAddress(const std::string& ip_address,
+                                                              uint16_t port) {
   sockaddr_in sa4;
   if (inet_pton(AF_INET, ip_address.c_str(), &sa4.sin_addr) == 1) {
     sa4.sin_family = AF_INET;
-    sa4.sin_port = 0;
+    sa4.sin_port = htons(port);
     return std::make_shared<Address::Ipv4Instance>(&sa4);
   }
   sockaddr_in6 sa6;
   if (inet_pton(AF_INET6, ip_address.c_str(), &sa6.sin6_addr) == 1) {
     sa6.sin6_family = AF_INET6;
-    sa6.sin6_port = 0;
+    sa6.sin6_port = htons(port);
     return std::make_shared<Address::Ipv6Instance>(sa6);
   }
   throwWithMalformedIp(ip_address);
@@ -149,11 +150,9 @@ Address::InstanceConstSharedPtr
 Utility::fromProtoResolvedAddress(const envoy::api::v2::ResolvedAddress& resolved_address) {
   switch (resolved_address.address_case()) {
   case envoy::api::v2::ResolvedAddress::kSocketAddress:
-    // TODO(htuch): Can do this more efficiently if it matters with direct sockaddr manipulation,
-    // keeping it simple for now.
-    return parseInternetAddressAndPort(fmt::format(
-        "{}:{}", ProtobufTypes::FromString(resolved_address.socket_address().ip_address()),
-        resolved_address.socket_address().port().value()));
+    return parseInternetAddress(
+        ProtobufTypes::FromString(resolved_address.socket_address().ip_address()),
+        resolved_address.socket_address().port().value());
   case envoy::api::v2::ResolvedAddress::kPipe:
     return Address::InstanceConstSharedPtr{
         new Address::PipeInstance(resolved_address.pipe().path())};

source/common/network/utility.h

@@ -61,9 +61,11 @@ class Utility {
    * Parse an internet host address (IPv4 or IPv6) and create an Instance from it. The address must
    * not include a port number. Throws EnvoyException if unable to parse the address.
    * @param ip_address string to be parsed as an internet address.
+   * @param port optional port to include in Instance created from ip_address, 0 by default.
    * @return pointer to the Instance, or nullptr if unable to parse the address.
    */
-  static Address::InstanceConstSharedPtr parseInternetAddress(const std::string& ip_address);
+  static Address::InstanceConstSharedPtr parseInternetAddress(const std::string& ip_address,
+                                                              uint16_t port = 0);
 
   /**
    * Parse an internet host address (IPv4 or IPv6) AND port, and create an Instance from it. Throws

test/common/network/utility_test.cc

@@ -119,6 +119,22 @@ TEST(NetworkUtility, ParseInternetAddressAndPort) {
   EXPECT_EQ("[::1]:0", Utility::parseInternetAddressAndPort("[::1]:0")->asString());
 }
 
+TEST(NetworkUtility, FromProtoResolvedAddress) {
+  envoy::api::v2::ResolvedAddress ipv4_address;
+  ipv4_address.mutable_socket_address()->set_ip_address("1.2.3.4");
+  ipv4_address.mutable_socket_address()->mutable_port()->set_value(5);
+  EXPECT_EQ("1.2.3.4:5", Utility::fromProtoResolvedAddress(ipv4_address)->asString());
+
+  envoy::api::v2::ResolvedAddress ipv6_address;
+  ipv4_address.mutable_socket_address()->set_ip_address("1::1");
+  ipv4_address.mutable_socket_address()->mutable_port()->set_value(2);
+  EXPECT_EQ("[1::1]:2", Utility::fromProtoResolvedAddress(ipv4_address)->asString());
+
+  envoy::api::v2::ResolvedAddress pipe_address;
+  pipe_address.mutable_pipe()->set_path("/foo/bar");
+  EXPECT_EQ("/foo/bar", Utility::fromProtoResolvedAddress(pipe_address)->asString());
+}
+
 class NetworkUtilityGetLocalAddress : public testing::TestWithParam<Address::IpVersion> {};
 
 INSTANTIATE_TEST_CASE_P(IpVersions, NetworkUtilityGetLocalAddress,

test/config/integration/server_xds.eds.json

@@ -8,7 +8,7 @@
         "endpoint": {
           "address": {
             "socketAddress": {
-              "ipAddress": "{{ ip_loopback_address }}",
+              "ipAddress": "{{ ntop_ip_loopback_address }}",
               "port": {"value": {{ upstream_0 }} }
             }
           }

test/test_common/environment.cc

@@ -120,6 +120,9 @@ std::string TestEnvironment::substitute(const std::string& str,
   const std::regex loopback_address_regex("\\{\\{ ip_loopback_address \\}\\}");
   out_json_string = std::regex_replace(out_json_string, loopback_address_regex,
                                        Network::Test::getLoopbackAddressUrlString(version));
+  const std::regex ntop_loopback_address_regex("\\{\\{ ntop_ip_loopback_address \\}\\}");
+  out_json_string = std::regex_replace(out_json_string, ntop_loopback_address_regex,
+                                       Network::Test::getLoopbackAddressString(version));
 
   // Substitute dns lookup family.
   const std::regex lookup_family_regex("\\{\\{ dns_lookup_family \\}\\}");

test/test_common/network_utility.cc

@@ -88,6 +88,13 @@ const std::string getLoopbackAddressUrlString(const Address::IpVersion version)
   return std::string("127.0.0.1");
 }
 
+const std::string getLoopbackAddressString(const Address::IpVersion version) {
+  if (version == Address::IpVersion::v6) {
+    return std::string("::1");
+  }
+  return std::string("127.0.0.1");
+}
+
 const std::string getAnyAddressUrlString(const Address::IpVersion version) {
   if (version == Address::IpVersion::v6) {
     return std::string("[::]");

test/test_common/network_utility.h

@@ -40,6 +40,14 @@ Address::InstanceConstSharedPtr findOrCheckFreePort(const std::string& addr_port
  */
 const std::string getLoopbackAddressUrlString(const Address::IpVersion version);
 
+/**
+ * Get a IP loopback address as a string. There are no square brackets around IPv6 addresses, this
+ * is what inet_ntop() gives.
+ * @param version IP address version of loopback address.
+ * @return std::string loopback address as a string.
+ */
+const std::string getLoopbackAddressString(const Address::IpVersion version);
+
 /**
  * Get a URL ready IP any address as a string.
  * @param version IP address version of any address.