happy_eyeballs: Validate that additional_address are IP addresses ins…

…tead of crashing when sorting. Signed-off-by: Ryan Hamilton <[email protected]> Signed-off-by: Ryan Northey <[email protected]>
envoyproxy · Dec 18, 2024 · cb73f42 · cb73f42
1 parent 9f5cc05
commit cb73f42
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 6 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -11,6 +11,9 @@ bug_fixes:
 - area: http/1
   change: |
     Fixes sending overload crashes when HTTP/1 request is reset.
+- area: happy_eyeballs
+  change: |
+    Validate that ``additional_address`` are IP addresses instead of crashing when sorting.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

diff --git a/source/extensions/clusters/eds/eds.cc b/source/extensions/clusters/eds/eds.cc
@@ -160,9 +160,15 @@ void EdsClusterImpl::BatchUpdateHelper::updateLocalityEndpoints(
   if (!lb_endpoint.endpoint().additional_addresses().empty()) {
     address_list.push_back(address);
     for (const auto& additional_address : lb_endpoint.endpoint().additional_addresses()) {
-      address_list.emplace_back(
-          THROW_OR_RETURN_VALUE(parent_.resolveProtoAddress(additional_address.address()),
-                                const Network::Address::InstanceConstSharedPtr));
+      Network::Address::InstanceConstSharedPtr address =
+          returnOrThrow(parent_.resolveProtoAddress(additional_address.address()));
+      address_list.emplace_back(address);
+    }
+    for (const Network::Address::InstanceConstSharedPtr& address : address_list) {
+      // All addresses must by IP addresses.
+      if (!address->ip()) {
+        throwEnvoyExceptionOrPanic("additional_addresses must be IP addresses.");
+      }
     }
   }
 

diff --git a/source/extensions/clusters/static/static_cluster.cc b/source/extensions/clusters/static/static_cluster.cc
@@ -31,9 +31,15 @@ StaticClusterImpl::StaticClusterImpl(const envoy::config::cluster::v3::Cluster&
             THROW_OR_RETURN_VALUE(resolveProtoAddress(lb_endpoint.endpoint().address()),
                                   const Network::Address::InstanceConstSharedPtr));
         for (const auto& additional_address : lb_endpoint.endpoint().additional_addresses()) {
-          address_list.emplace_back(
-              THROW_OR_RETURN_VALUE(resolveProtoAddress(additional_address.address()),
-                                    const Network::Address::InstanceConstSharedPtr));
+          Network::Address::InstanceConstSharedPtr address =
+              returnOrThrow(resolveProtoAddress(additional_address.address()));
+          address_list.emplace_back(address);
+        }
+        for (const Network::Address::InstanceConstSharedPtr& address : address_list) {
+          // All addresses must by IP addresses.
+          if (!address->ip()) {
+            throwEnvoyExceptionOrPanic("additional_addresses must be IP addresses.");
+          }
         }
       }
       priority_state_manager_->registerHostForPriority(

diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc
@@ -6409,6 +6409,37 @@ TEST_F(ClusterManagerImplTest, CheckAddressesList) {
   ASSERT_EQ(hosts[0]->addressListOrNull()->size(), 2);
 }
 
+// Verify that non-IP additional addresses are rejected.
+TEST_F(ClusterManagerImplTest, RejectNonIpAdditionalAddresses) {
+  const std::string bootstrap = R"EOF(
+  static_resources:
+    clusters:
+    - name: cluster_0
+      connect_timeout: 0.250s
+      type: STATIC
+      lb_policy: ROUND_ROBIN
+      load_assignment:
+        cluster_name: cluster_0
+        endpoints:
+        - lb_endpoints:
+          - endpoint:
+              additionalAddresses:
+              - address:
+                  envoyInternalAddress:
+                   server_listener_name: internal_address
+              address:
+                socket_address:
+                  address: 127.0.0.1
+                  port_value: 11001
+  )EOF";
+  try {
+    create(parseBootstrapFromV3Yaml(bootstrap));
+    FAIL() << "Invalid address was not rejected";
+  } catch (const EnvoyException& e) {
+    EXPECT_STREQ("additional_addresses must be IP addresses.", e.what());
+  }
+}
+
 TEST_F(ClusterManagerImplTest, CheckActiveStaticCluster) {
   const std::string yaml = R"EOF(
   static_resources:

diff --git a/test/extensions/clusters/eds/eds_test.cc b/test/extensions/clusters/eds/eds_test.cc
@@ -436,6 +436,36 @@ TEST_F(EdsTest, DualStackEndpoint) {
   EXPECT_NE(connection, connection_data.connection_.get());
 }
 
+// Verify that non-IP additional addresses are rejected.
+TEST_F(EdsTest, RejectNonIpAdditionalAddresses) {
+  envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment;
+  cluster_load_assignment.set_cluster_name("fare");
+
+  // Add dual stack endpoint
+  auto* endpoints = cluster_load_assignment.add_endpoints();
+  auto* endpoint = endpoints->add_lb_endpoints();
+  endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_address("::1");
+  endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_port_value(80);
+  endpoint->mutable_endpoint()
+      ->mutable_additional_addresses()
+      ->Add()
+      ->mutable_address()
+      ->mutable_envoy_internal_address()
+      ->set_server_listener_name("internal_address");
+
+  endpoint->mutable_load_balancing_weight()->set_value(30);
+
+  initialize();
+  const auto decoded_resources =
+      TestUtility::decodeResources({cluster_load_assignment}, "cluster_name");
+  try {
+    (void)eds_callbacks_->onConfigUpdate(decoded_resources.refvec_, "");
+    FAIL() << "Invalid address was not rejected";
+  } catch (const EnvoyException& e) {
+    EXPECT_STREQ("additional_addresses must be IP addresses.", e.what());
+  }
+}
+
 // Validate that onConfigUpdate() updates the endpoint metadata.
 TEST_F(EdsTest, EndpointMetadata) {
   envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment;