From 38f02d8e2b5dbcd70f72432780adb05aa8d4403d Mon Sep 17 00:00:00 2001
From: "Mark D. Roth" <roth@google.com>
Date: Tue, 24 Mar 2020 12:13:47 -0700
Subject: [PATCH] xds: NACK EDS update with sparse priorities

---
 .../ext/filters/client_channel/xds/xds_api.cc | 16 +++++++---
 test/cpp/end2end/xds_end2end_test.cc          | 30 +++++++++++++++++--
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/src/core/ext/filters/client_channel/xds/xds_api.cc b/src/core/ext/filters/client_channel/xds/xds_api.cc
index 9f0b17f5865..5a41d0af32a 100644
--- a/src/core/ext/filters/client_channel/xds/xds_api.cc
+++ b/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,9 +1479,9 @@ 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,
-                             expected_eds_service_names, eds_update_map,
-                             arena.ptr());
+    return EdsResponseParse(client_, tracer_, response,
+                            expected_eds_service_names, eds_update_map,
+                            arena.ptr());
   } else {
     return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
         "Unsupported ADS resource type.");
diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc
index 622022a0bd0..3050af3caba 100644
--- a/test/cpp/end2end/xds_end2end_test.cc
+++ b/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