Merge pull request #22612 from markdroth/xds_eds_no_localities_fix

xds: Don't NACK EDS updates with no localities, but report TRANSIENT_FAILURE.

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 {

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));
   }

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) {