diff --git a/src/core/ext/xds/xds_endpoint.cc b/src/core/ext/xds/xds_endpoint.cc index da285524ec0..89f433f6ede 100644 --- a/src/core/ext/xds/xds_endpoint.cc +++ b/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( @@ -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()) { diff --git a/test/cpp/end2end/xds/xds_end2end_test.cc b/test/cpp/end2end/xds/xds_end2end_test.cc index bad4896e10d..e0df895598e 100644 --- a/test/cpp/end2end/xds/xds_end2end_test.cc +++ b/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