From c3fc65c9e00bb630cadfc4e420501d66633f9257 Mon Sep 17 00:00:00 2001
From: "Mark D. Roth" <roth@google.com>
Date: Wed, 8 Apr 2020 07:29:45 -0700
Subject: [PATCH] Don't NACK EDS updates with no localities, but report
 TRANSIENT_FAILURE.

---
 .../client_channel/lb_policy/xds/eds.cc       | 19 ++++++++------
 .../ext/filters/client_channel/xds/xds_api.cc |  7 ------
 test/cpp/end2end/xds_end2end_test.cc          | 25 +++++++++----------
 3 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc
index 2f1acdb9f80..099fb286223 100644
--- a/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc
@@ -405,19 +405,22 @@ class EdsLb::EndpointWatcher : public XdsClient::EndpointWatcherInterface {
       }
       eds_policy_->drop_config_ = std::move(update.drop_config);
       eds_policy_->MaybeUpdateDropPickerLocked();
+    } else if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_eds_trace)) {
+      gpr_log(GPR_INFO, "[edslb %p] Drop config unchanged, ignoring",
+              eds_policy_.get());
     }
     // Update priority and locality info.
-    if (eds_policy_->priority_list_update_ == update.priority_list_update) {
+    if (eds_policy_->child_policy_ == nullptr ||
+        eds_policy_->priority_list_update_ != update.priority_list_update) {
       if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_eds_trace)) {
-        gpr_log(GPR_INFO,
-                "[edslb %p] Incoming locality update identical to current, "
-                "ignoring. (drop_config_changed=%d)",
-                eds_policy_.get(), drop_config_changed);
+        gpr_log(GPR_INFO, "[edslb %p] Updating priority list",
+                eds_policy_.get());
       }
-      return;
+      eds_policy_->UpdatePriorityList(std::move(update.priority_list_update));
+    } else if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_eds_trace)) {
+      gpr_log(GPR_INFO, "[edslb %p] Priority list unchanged, ignoring",
+              eds_policy_.get());
     }
-    // Update the child policy with the new priority and endpoint data.
-    eds_policy_->UpdatePriorityList(std::move(update.priority_list_update));
   }
 
   void OnError(grpc_error* error) override {
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 2ac824ba08e..4cf5d8c26a2 100644
--- a/src/core/ext/filters/client_channel/xds/xds_api.cc
+++ b/src/core/ext/filters/client_channel/xds/xds_api.cc
@@ -1420,13 +1420,6 @@ grpc_error* EdsResponseParse(
         if (error != GRPC_ERROR_NONE) return error;
       }
     }
-    // Validate the update content.
-    if (eds_update.priority_list_update.empty() &&
-        !eds_update.drop_config->drop_all()) {
-      return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-          "EDS response doesn't contain any valid "
-          "locality but doesn't require to drop all calls.");
-    }
     eds_update_map->emplace(std::string(cluster_name.data, cluster_name.size),
                             std::move(eds_update));
   }
diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc
index 2efbc38a364..2e643a43670 100644
--- a/test/cpp/end2end/xds_end2end_test.cc
+++ b/test/cpp/end2end/xds_end2end_test.cc
@@ -2355,19 +2355,6 @@ 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) {
@@ -2454,6 +2441,18 @@ TEST_P(LocalityMapTest, LocalityContainingNoEndpoints) {
             kNumRpcs / backends_.size());
 }
 
+// EDS update with no localities.
+TEST_P(LocalityMapTest, NoLocalities) {
+  SetNextResolution({});
+  SetNextResolutionForLbChannelAllBalancers();
+  // EDS response contains 2 localities, one with no endpoints.
+  balancers_[0]->ads_service()->SetEdsResource(
+      AdsServiceImpl::BuildEdsResource({}), kDefaultResourceName);
+  Status status = SendRpc();
+  EXPECT_FALSE(status.ok());
+  EXPECT_EQ(status.error_code(), GRPC_STATUS_UNAVAILABLE);
+}
+
 // Tests that the locality map can work properly even when it contains a large
 // number of localities.
 TEST_P(LocalityMapTest, StressTest) {