Merge pull request #22468 from markdroth/xds_eds_priority_validation

xds: NACK EDS update with sparse priorities

src/core/ext/filters/client_channel/xds/xds_api.cc

@@ -1347,7 +1347,7 @@ grpc_error* DropParseAndAppend(
   return GRPC_ERROR_NONE;
 }
 
-grpc_error* EdsResponsedParse(
+grpc_error* EdsResponseParse(
     XdsClient* client, TraceFlag* tracer,
     const envoy_api_v2_DiscoveryResponse* response,
     const std::set<StringView>& expected_eds_service_names,
@@ -1397,6 +1397,14 @@ grpc_error* EdsResponsedParse(
       if (locality.lb_weight == 0) continue;
       eds_update.priority_list_update.Add(locality);
     }
+    for (uint32_t priority = 0;
+         priority < eds_update.priority_list_update.size(); ++priority) {
+      auto* locality_map = eds_update.priority_list_update.Find(priority);
+      if (locality_map == nullptr || locality_map->size() == 0) {
+        return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+            "EDS update includes sparse priority list");
+      }
+    }
     // Get the drop config.
     eds_update.drop_config = MakeRefCounted<XdsApi::DropConfig>();
     const envoy_api_v2_ClusterLoadAssignment_Policy* policy =
@@ -1471,7 +1479,7 @@ grpc_error* XdsApi::ParseAdsResponse(
     return CdsResponseParse(client_, tracer_, response, expected_cluster_names,
                             cds_update_map, arena.ptr());
   } else if (*type_url == kEdsTypeUrl) {
-    return EdsResponsedParse(client_, tracer_, response,
+    return EdsResponseParse(client_, tracer_, response,
                             expected_eds_service_names, eds_update_map,
                             arena.ptr());
   } else {

test/cpp/end2end/xds_end2end_test.cc

@@ -2339,8 +2339,6 @@ TEST_P(CdsTest, Timeout) {
 
 using EdsTest = BasicTest;
 
-// TODO(roth): Add tests showing that RPCs fail when EDS data is invalid.
-
 TEST_P(EdsTest, Timeout) {
   ResetStub(0, 0, "", 500);
   balancers_[0]->ads_service()->SetResourceIgnore(kEdsTypeUrl);
@@ -2349,6 +2347,34 @@ TEST_P(EdsTest, Timeout) {
   CheckRpcSendFailure();
 }
 
+// Tests that EDS client should send a NACK if the EDS update contains
+// no localities but does not say to drop all calls.
+TEST_P(EdsTest, NacksNoLocalitiesWithoutDropAll) {
+  SetNextResolution({});
+  SetNextResolutionForLbChannelAllBalancers();
+  AdsServiceImpl::EdsResourceArgs args;
+  balancers_[0]->ads_service()->SetEdsResource(
+      AdsServiceImpl::BuildEdsResource(args), kDefaultResourceName);
+  CheckRpcSendFailure();
+  EXPECT_EQ(balancers_[0]->ads_service()->eds_response_state(),
+            AdsServiceImpl::NACKED);
+}
+
+// Tests that EDS client should send a NACK if the EDS update contains
+// sparse priorities.
+TEST_P(EdsTest, NacksSparsePriorityList) {
+  SetNextResolution({});
+  SetNextResolutionForLbChannelAllBalancers();
+  AdsServiceImpl::EdsResourceArgs args({
+      {"locality0", GetBackendPorts(), kDefaultLocalityWeight, 1},
+  });
+  balancers_[0]->ads_service()->SetEdsResource(
+      AdsServiceImpl::BuildEdsResource(args), kDefaultResourceName);
+  CheckRpcSendFailure();
+  EXPECT_EQ(balancers_[0]->ads_service()->eds_response_state(),
+            AdsServiceImpl::NACKED);
+}
+
 using LocalityMapTest = BasicTest;
 
 // Tests that the localities in a locality map are picked according to their