Enforce locality order in upstream HostsPerLocality (envoyproxy#28527)

* enforce locality order Signed-off-by: Adam Anderson <[email protected]> Signed-off-by: Adam Anderson <[email protected]> * add invariant to description of HostsPerLocality::get() Signed-off-by: Adam Anderson <[email protected]> Signed-off-by: Adam Anderson <[email protected]> * remove invariant assertion, add PriorityStateManager test instead Signed-off-by: Adam Anderson <[email protected]> * fix tests Signed-off-by: Adam Anderson <[email protected]> --------- Signed-off-by: Adam Anderson <[email protected]>
wbpcode · Aug 10, 2023 · 8468cf2 · 8468cf2
1 parent e6b380b
commit 8468cf2
Show file tree

Hide file tree

Showing 12 changed files with 652 additions and 222 deletions.
diff --git a/envoy/upstream/upstream.h b/envoy/upstream/upstream.h
@@ -321,7 +321,10 @@ class HostsPerLocality {
   /**
    * @return const std::vector<HostVector>& list of hosts organized per
    *         locality. The local locality is the first entry if
-   *         hasLocalLocality() is true.
+   *         hasLocalLocality() is true. All hosts within the same entry have the same locality
+   *         and all hosts with a given locality are in the same entry. With the exception of
+   *         the local locality entry (if present), all entries are sorted by locality with
+   *         those considered less by the LocalityLess comparator ordered earlier in the list.
    */
   virtual const std::vector<HostVector>& get() const PURE;
 

diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h
@@ -495,9 +495,23 @@ class HostsPerLocalityImpl : public HostsPerLocality {
   HostsPerLocalityImpl() : HostsPerLocalityImpl(std::vector<HostVector>(), false) {}
 
   // Single locality constructor
+  //
+  // Parameter requirements:
+  // 1. All entries in hosts must have the same locality.
+  // 2. If has_local_locality is true, then the locality of all entries in hosts
+  //    must be equal to the current envoy's locality.
   HostsPerLocalityImpl(const HostVector& hosts, bool has_local_locality = false)
       : HostsPerLocalityImpl(std::vector<HostVector>({hosts}), has_local_locality) {}
 
+  // Multiple localities constructor
+  //
+  // locality_hosts must adhere to the following ordering constraints:
+  // 1. All hosts within a single HostVector bucket must have the same locality
+  // 2. No hosts in different HostVector buckets can have the same locality
+  // 3. If has_local_locality is true, then the locality of all hosts in the first HostVector bucket
+  //    must be equal to the current envoy's locality.
+  // 4. All non-local HostVector buckets must be sorted in ascending order by the LocalityLess
+  // comparator
   HostsPerLocalityImpl(std::vector<HostVector>&& locality_hosts, bool has_local_locality)
       : local_(has_local_locality), hosts_per_locality_(std::move(locality_hosts)) {
     ASSERT(!has_local_locality || !hosts_per_locality_.empty());

diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD
@@ -276,6 +276,7 @@ envoy_cc_test(
         "//test/test_common:simulated_time_system_lib",
         "//test/test_common:test_runtime_lib",
         "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
+        "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
     ],
 )
 

diff --git a/test/common/upstream/hds_test.cc b/test/common/upstream/hds_test.cc
@@ -290,7 +290,9 @@ TEST_F(HdsTest, TestProcessMessageEndpoints) {
     auto* health_check = message->add_cluster_health_checks();
     health_check->set_cluster_name("anna" + std::to_string(i));
     for (int j = 0; j < 3; j++) {
-      auto* address = health_check->add_locality_endpoints()->add_endpoints()->mutable_address();
+      auto* locality_endpoints = health_check->add_locality_endpoints();
+      locality_endpoints->mutable_locality()->set_zone(std::to_string(j));
+      auto* address = locality_endpoints->add_endpoints()->mutable_address();
       address->mutable_socket_address()->set_address("127.0.0." + std::to_string(i));
       address->mutable_socket_address()->set_port_value(1234 + j);
     }
@@ -1208,7 +1210,9 @@ TEST_F(HdsTest, TestCustomHealthCheckPortWhenCreate) {
   auto* health_check = message->add_cluster_health_checks();
   health_check->set_cluster_name("anna");
   for (int i = 0; i < 3; i++) {
-    auto* endpoint = health_check->add_locality_endpoints()->add_endpoints();
+    auto* locality_endpoints = health_check->add_locality_endpoints();
+    locality_endpoints->mutable_locality()->set_zone(std::to_string(i));
+    auto* endpoint = locality_endpoints->add_endpoints();
     endpoint->mutable_health_check_config()->set_port_value(4321 + i);
     auto* address = endpoint->mutable_address();
     address->mutable_socket_address()->set_address("127.0.0.1");

diff --git a/test/common/upstream/load_balancer_impl_test.cc b/test/common/upstream/load_balancer_impl_test.cc
diff --git a/test/common/upstream/subset_lb_test.cc b/test/common/upstream/subset_lb_test.cc
@@ -221,23 +221,28 @@ class SubsetLoadBalancerTest : public Event::TestUsingSimulatedTime,
   void configureWeightedHostSet(const HostURLMetadataMap& first_locality_host_metadata,
                                 const HostURLMetadataMap& second_locality_host_metadata,
                                 MockHostSet& host_set, LocalityWeights locality_weights) {
-    HostVector first_locality;
     HostVector all_hosts;
+    HostVector first_locality_hosts;
+    envoy::config::core::v3::Locality first_locality;
+    first_locality.set_zone("0");
     for (const auto& it : first_locality_host_metadata) {
-      auto host = makeHost(it.first, it.second);
-      first_locality.emplace_back(host);
+      auto host = makeHost(it.first, it.second, first_locality);
+      first_locality_hosts.emplace_back(host);
       all_hosts.emplace_back(host);
     }
 
-    HostVector second_locality;
+    envoy::config::core::v3::Locality second_locality;
+    second_locality.set_zone("1");
+    HostVector second_locality_hosts;
     for (const auto& it : second_locality_host_metadata) {
-      auto host = makeHost(it.first, it.second);
-      second_locality.emplace_back(host);
+      auto host = makeHost(it.first, it.second, second_locality);
+      second_locality_hosts.emplace_back(host);
       all_hosts.emplace_back(host);
     }
 
     host_set.hosts_ = all_hosts;
-    host_set.hosts_per_locality_ = makeHostsPerLocality({first_locality, second_locality});
+    host_set.hosts_per_locality_ =
+        makeHostsPerLocality({first_locality_hosts, second_locality_hosts});
     host_set.healthy_hosts_ = host_set.hosts_;
     host_set.healthy_hosts_per_locality_ = host_set.hosts_per_locality_;
     host_set.locality_weights_ = std::make_shared<const LocalityWeights>(locality_weights);
@@ -286,12 +291,22 @@ class SubsetLoadBalancerTest : public Event::TestUsingSimulatedTime,
                      const std::vector<HostURLMetadataMap>& local_host_metadata_per_locality) {
     EXPECT_CALL(subset_info_, isEnabled()).WillRepeatedly(Return(true));
 
+    std::vector<std::shared_ptr<const envoy::config::core::v3::Locality>> localities;
+    for (uint32_t i = 0; i < 10; ++i) {
+      envoy::config::core::v3::Locality locality;
+      locality.set_zone(std::to_string(i));
+      localities.emplace_back(std::make_shared<const envoy::config::core::v3::Locality>(locality));
+    }
+    ASSERT(host_metadata_per_locality.size() <= localities.size());
+    ASSERT(local_host_metadata_per_locality.size() <= localities.size());
+
     HostVector hosts;
     std::vector<HostVector> hosts_per_locality;
-    for (const auto& host_metadata : host_metadata_per_locality) {
+    for (uint32_t i = 0; i < host_metadata_per_locality.size(); ++i) {
+      const auto& host_metadata = host_metadata_per_locality[i];
       HostVector locality_hosts;
       for (const auto& host_entry : host_metadata) {
-        HostSharedPtr host = makeHost(host_entry.first, host_entry.second);
+        HostSharedPtr host = makeHost(host_entry.first, host_entry.second, *localities[i]);
         hosts.emplace_back(host);
         locality_hosts.emplace_back(host);
       }
@@ -306,10 +321,11 @@ class SubsetLoadBalancerTest : public Event::TestUsingSimulatedTime,
 
     local_hosts_ = std::make_shared<HostVector>();
     std::vector<HostVector> local_hosts_per_locality_vector;
-    for (const auto& local_host_metadata : local_host_metadata_per_locality) {
+    for (uint32_t i = 0; i < local_host_metadata_per_locality.size(); ++i) {
+      const auto& local_host_metadata = local_host_metadata_per_locality[i];
       HostVector local_locality_hosts;
       for (const auto& host_entry : local_host_metadata) {
-        HostSharedPtr host = makeHost(host_entry.first, host_entry.second);
+        HostSharedPtr host = makeHost(host_entry.first, host_entry.second, *localities[i]);
         local_hosts_->emplace_back(host);
         local_locality_hosts.emplace_back(host);
       }
@@ -344,6 +360,18 @@ class SubsetLoadBalancerTest : public Event::TestUsingSimulatedTime,
 
     return makeTestHost(info_, url, m, simTime());
   }
+
+  HostSharedPtr makeHost(const std::string& url, const HostMetadata& metadata,
+                         const envoy::config::core::v3::Locality& locality) {
+    envoy::config::core::v3::Metadata m;
+    for (const auto& m_it : metadata) {
+      Config::Metadata::mutableMetadataValue(m, Config::MetadataFilters::get().ENVOY_LB, m_it.first)
+          .set_string_value(m_it.second);
+    }
+
+    return makeTestHost(info_, url, m, locality, simTime());
+  }
+
   HostSharedPtr makeHost(const std::string& url, const HostListMetadata& metadata) {
     envoy::config::core::v3::Metadata m;
     for (const auto& m_it : metadata) {
@@ -1666,11 +1694,14 @@ TEST_P(SubsetLoadBalancerTest, ZoneAwareFallbackAfterUpdate) {
   EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(9999)).WillOnce(Return(2));
   EXPECT_EQ(host_set_.healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr));
 
-  modifyHosts({makeHost("tcp://127.0.0.1:8000", {{"version", "1.0"}})}, {host_set_.hosts_[0]},
-              absl::optional<uint32_t>(0));
+  envoy::config::core::v3::Locality local_locality;
+  local_locality.set_zone("0");
+
+  modifyHosts({makeHost("tcp://127.0.0.1:8000", {{"version", "1.0"}}, local_locality)},
+              {host_set_.hosts_[0]}, absl::optional<uint32_t>(0));
 
-  modifyLocalHosts({makeHost("tcp://127.0.0.1:9000", {{"version", "1.0"}})}, {local_hosts_->at(0)},
-                   0);
+  modifyLocalHosts({makeHost("tcp://127.0.0.1:9000", {{"version", "1.0"}}, local_locality)},
+                   {local_hosts_->at(0)}, 0);
 
   EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(100));
   EXPECT_EQ(host_set_.healthy_hosts_per_locality_->get()[0][0], lb_->chooseHost(nullptr));
@@ -1793,11 +1824,14 @@ TEST_P(SubsetLoadBalancerTest, ZoneAwareFallbackDefaultSubsetAfterUpdate) {
   EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(9999)).WillOnce(Return(2));
   EXPECT_EQ(host_set_.healthy_hosts_per_locality_->get()[1][1], lb_->chooseHost(nullptr));
 
-  modifyHosts({makeHost("tcp://127.0.0.1:8001", {{"version", "default"}})}, {host_set_.hosts_[1]},
-              absl::optional<uint32_t>(0));
+  envoy::config::core::v3::Locality local_locality;
+  local_locality.set_zone("0");
+
+  modifyHosts({makeHost("tcp://127.0.0.1:8001", {{"version", "default"}}, local_locality)},
+              {host_set_.hosts_[1]}, absl::optional<uint32_t>(0));
 
   modifyLocalHosts({local_hosts_->at(1)},
-                   {makeHost("tcp://127.0.0.1:9001", {{"version", "default"}})}, 0);
+                   {makeHost("tcp://127.0.0.1:9001", {{"version", "default"}}, local_locality)}, 0);
 
   EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(100));
   EXPECT_EQ(host_set_.healthy_hosts_per_locality_->get()[0][1], lb_->chooseHost(nullptr));
@@ -1916,11 +1950,14 @@ TEST_P(SubsetLoadBalancerTest, ZoneAwareBalancesSubsetsAfterUpdate) {
   EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(9999)).WillOnce(Return(2));
   EXPECT_EQ(host_set_.healthy_hosts_per_locality_->get()[1][1], lb_->chooseHost(&context));
 
-  modifyHosts({makeHost("tcp://127.0.0.1:8001", {{"version", "1.1"}})}, {host_set_.hosts_[1]},
-              absl::optional<uint32_t>(0));
+  envoy::config::core::v3::Locality local_locality;
+  local_locality.set_zone("0");
+
+  modifyHosts({makeHost("tcp://127.0.0.1:8001", {{"version", "1.1"}}, local_locality)},
+              {host_set_.hosts_[1]}, absl::optional<uint32_t>(0));
 
-  modifyLocalHosts({local_hosts_->at(1)}, {makeHost("tcp://127.0.0.1:9001", {{"version", "1.1"}})},
-                   0);
+  modifyLocalHosts({local_hosts_->at(1)},
+                   {makeHost("tcp://127.0.0.1:9001", {{"version", "1.1"}}, local_locality)}, 0);
 
   EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(100));
   EXPECT_EQ(host_set_.healthy_hosts_per_locality_->get()[0][1], lb_->chooseHost(&context));