From a8d887ffd329f3927fa5b4164db481ab10cd8c86 Mon Sep 17 00:00:00 2001
From: Eric Anderson <ejona@google.com>
Date: Tue, 22 Mar 2022 16:41:59 -0700
Subject: [PATCH] xds: Fix LBs blindly propagating XdsClient errors

This is similar to 2a45524 (for #8950) but for additional similar cases.
---
 .../java/io/grpc/xds/CdsLoadBalancer2.java     |  9 +++++++--
 .../grpc/xds/ClusterResolverLoadBalancer.java  |  6 +++++-
 .../java/io/grpc/xds/CdsLoadBalancer2Test.java |  8 ++++++--
 .../xds/ClusterResolverLoadBalancerTest.java   | 18 ++++++++++++++++++
 4 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java b/xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java
index 7a346e01871..2dcc51ad8ab 100644
--- a/xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java
+++ b/xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java
@@ -246,7 +246,12 @@ void shutdown() {
       }
 
       @Override
-      public void onError(final Status error) {
+      public void onError(Status error) {
+        Status status = Status.UNAVAILABLE
+            .withDescription(
+                String.format("Unable to load CDS %s. xDS server returned: %s: %s",
+                  name, error.getCode(), error.getDescription()))
+            .withCause(error.getCause());
         syncContext.execute(new Runnable() {
           @Override
           public void run() {
@@ -255,7 +260,7 @@ public void run() {
             }
             // All watchers should receive the same error, so we only propagate it once.
             if (ClusterState.this == root) {
-              handleClusterDiscoveryError(error);
+              handleClusterDiscoveryError(status);
             }
           }
         });
diff --git a/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java b/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java
index 309daf55a18..1642aba93d4 100644
--- a/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java
+++ b/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java
@@ -461,7 +461,11 @@ public void run() {
             if (shutdown) {
               return;
             }
-            status = error;
+            String resourceName = edsServiceName != null ? edsServiceName : name;
+            status = Status.UNAVAILABLE
+                .withDescription(String.format("Unable to load EDS %s. xDS server returned: %s: %s",
+                      resourceName, error.getCode(), error.getDescription()))
+                .withCause(error.getCause());
             logger.log(XdsLogLevel.WARNING, "Received EDS error: {0}", error);
             handleEndpointResolutionError();
           }
diff --git a/xds/src/test/java/io/grpc/xds/CdsLoadBalancer2Test.java b/xds/src/test/java/io/grpc/xds/CdsLoadBalancer2Test.java
index 78e6d6473ca..cd1d33077ad 100644
--- a/xds/src/test/java/io/grpc/xds/CdsLoadBalancer2Test.java
+++ b/xds/src/test/java/io/grpc/xds/CdsLoadBalancer2Test.java
@@ -459,7 +459,10 @@ public void aggregateCluster_discoveryErrorBeforeChildLbCreated_returnErrorPicke
     xdsClient.deliverError(error);
     verify(helper).updateBalancingState(
         eq(ConnectivityState.TRANSIENT_FAILURE), pickerCaptor.capture());
-    assertPicker(pickerCaptor.getValue(), error, null);
+    Status expectedError = Status.UNAVAILABLE.withDescription(
+        "Unable to load CDS cluster-foo.googleapis.com. xDS server returned: "
+        + "RESOURCE_EXHAUSTED: OOM");
+    assertPicker(pickerCaptor.getValue(), expectedError, null);
     assertThat(childBalancers).isEmpty();
   }
 
@@ -481,7 +484,8 @@ public void aggregateCluster_discoveryErrorAfterChildLbCreated_propagateToChildL
 
     Status error = Status.RESOURCE_EXHAUSTED.withDescription("OOM");
     xdsClient.deliverError(error);
-    assertThat(childLb.upstreamError).isEqualTo(error);
+    assertThat(childLb.upstreamError.getCode()).isEqualTo(Status.Code.UNAVAILABLE);
+    assertThat(childLb.upstreamError.getDescription()).contains("RESOURCE_EXHAUSTED: OOM");
     assertThat(childLb.shutdown).isFalse();  // child LB may choose to keep working
   }
 
diff --git a/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java
index 51a7ce5066b..9b9df80f1ba 100644
--- a/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java
+++ b/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java
@@ -850,6 +850,24 @@ public void resolutionErrorBeforeChildLbCreated_returnErrorPickerIfAllClustersEn
         null);
   }
 
+  @Test
+  public void resolutionErrorBeforeChildLbCreated_edsOnly_returnErrorPicker() {
+    ClusterResolverConfig config = new ClusterResolverConfig(
+        Arrays.asList(edsDiscoveryMechanism1), roundRobin);
+    deliverLbConfig(config);
+    assertThat(xdsClient.watchers.keySet()).containsExactly(EDS_SERVICE_NAME1);
+    assertThat(childBalancers).isEmpty();
+    reset(helper);
+    xdsClient.deliverError(Status.RESOURCE_EXHAUSTED.withDescription("OOM"));
+    assertThat(childBalancers).isEmpty();
+    verify(helper).updateBalancingState(
+        eq(ConnectivityState.TRANSIENT_FAILURE), pickerCaptor.capture());
+    PickResult result = pickerCaptor.getValue().pickSubchannel(mock(PickSubchannelArgs.class));
+    Status actualStatus = result.getStatus();
+    assertThat(actualStatus.getCode()).isEqualTo(Status.Code.UNAVAILABLE);
+    assertThat(actualStatus.getDescription()).contains("RESOURCE_EXHAUSTED: OOM");
+  }
+
   @Test
   public void handleNameResolutionErrorFromUpstream_beforeChildLbCreated_returnErrorPicker() {
     ClusterResolverConfig config = new ClusterResolverConfig(