xds: NACK EDS resources with duplicate localities in the same priority (#29231)

src/core/ext/xds/xds_endpoint.cc

@@ -202,7 +202,7 @@ grpc_error_handle LocalityParse(
   std::string region =
       UpbStringToStdString(envoy_config_core_v3_Locality_region(locality));
   std::string zone =
-      UpbStringToStdString(envoy_config_core_v3_Locality_region(locality));
+      UpbStringToStdString(envoy_config_core_v3_Locality_zone(locality));
   std::string sub_zone =
       UpbStringToStdString(envoy_config_core_v3_Locality_sub_zone(locality));
   output_locality->name = MakeRefCounted<XdsLocalityName>(
@@ -285,11 +285,18 @@ grpc_error_handle EdsResourceParse(
     if (locality.lb_weight == 0) continue;
     // Make sure prorities is big enough. Note that they might not
     // arrive in priority order.
-    while (eds_update->priorities.size() < priority + 1) {
-      eds_update->priorities.emplace_back();
+    if (eds_update->priorities.size() < priority + 1) {
+      eds_update->priorities.resize(priority + 1);
+    }
+    auto& locality_map = eds_update->priorities[priority].localities;
+    auto it = locality_map.find(locality.name.get());
+    if (it != locality_map.end()) {
+      errors.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING(absl::StrCat(
+          "duplicate locality ", locality.name->AsHumanReadableString(),
+          " found in priority ", priority)));
+    } else {
+      locality_map.emplace(locality.name.get(), std::move(locality));
     }
-    eds_update->priorities[priority].localities.emplace(locality.name.get(),
-                                                        std::move(locality));
   }
   for (const auto& priority : eds_update->priorities) {
     if (priority.localities.empty()) {

test/cpp/end2end/xds/xds_end2end_test.cc

@@ -11618,6 +11618,25 @@ TEST_P(EdsTest, NacksSparsePriorityList) {
               ::testing::HasSubstr("sparse priority list"));
 }
 
+// Tests that EDS client should send a NACK if the EDS update contains
+// multiple instances of the same locality in the same priority.
+TEST_P(EdsTest, NacksDuplicateLocalityInSamePriority) {
+  EdsResourceArgs args({
+      {"locality0", CreateEndpointsForBackends(0, 1), kDefaultLocalityWeight,
+       0},
+      {"locality0", CreateEndpointsForBackends(1, 2), kDefaultLocalityWeight,
+       0},
+  });
+  balancer_->ads_service()->SetEdsResource(BuildEdsResource(args));
+  const auto response_state = WaitForEdsNack();
+  ASSERT_TRUE(response_state.has_value()) << "timed out waiting for NACK";
+  EXPECT_THAT(response_state->error_message,
+              ::testing::HasSubstr(
+                  "duplicate locality {region=\"xds_default_locality_region\", "
+                  "zone=\"xds_default_locality_zone\", sub_zone=\"locality0\"} "
+                  "found in priority 0"));
+}
+
 // In most of our tests, we use different names for different resource
 // types, to make sure that there are no cut-and-paste errors in the code
 // that cause us to look at data for the wrong resource type.  So we add