From b751a0dba58ce67149a603443b18e019df607a84 Mon Sep 17 00:00:00 2001
From: "Penn (Dapeng) Zhang" <zdapeng@google.com>
Date: Fri, 10 Sep 2021 10:45:08 -0700
Subject: [PATCH] xds: populate envoy RetryPolicy with no retryOn to resolver

---
 xds/src/main/java/io/grpc/xds/ClientXdsClient.java | 11 ++++-------
 xds/src/main/java/io/grpc/xds/XdsNameResolver.java |  5 +++--
 .../java/io/grpc/xds/ClientXdsClientDataTest.java  |  3 ++-
 .../test/java/io/grpc/xds/XdsNameResolverTest.java | 14 +++++++++++++-
 4 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java
index 693848897c6..f79b0c4e9f6 100644
--- a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java
+++ b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java
@@ -1281,13 +1281,10 @@ private static StructOrError<RetryPolicy> parseRetryPolicy(
       retryableStatusCodesBuilder.add(code);
     }
     List<Code> retryableStatusCodes = retryableStatusCodesBuilder.build();
-    if (!retryableStatusCodes.isEmpty()) {
-      return StructOrError.fromStruct(
-          RetryPolicy.create(
-              maxAttempts, retryableStatusCodes, initialBackoff, maxBackoff,
-              /* perAttemptRecvTimeout= */ null));
-    }
-    return null;
+    return StructOrError.fromStruct(
+        RetryPolicy.create(
+            maxAttempts, retryableStatusCodes, initialBackoff, maxBackoff,
+            /* perAttemptRecvTimeout= */ null));
   }
 
   @VisibleForTesting
diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java
index e2905dee26e..4cd52c8b3f9 100644
--- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java
+++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java
@@ -182,13 +182,14 @@ public void shutdown() {
   @VisibleForTesting
   static Map<String, ?> generateServiceConfigWithMethodConfig(
       @Nullable Long timeoutNano, @Nullable RetryPolicy retryPolicy) {
-    if (timeoutNano == null && retryPolicy == null) {
+    if (timeoutNano == null
+        && (retryPolicy == null || retryPolicy.retryableStatusCodes().isEmpty())) {
       return Collections.emptyMap();
     }
     ImmutableMap.Builder<String, Object> methodConfig = ImmutableMap.builder();
     methodConfig.put(
         "name", Collections.singletonList(Collections.emptyMap()));
-    if (retryPolicy != null) {
+    if (retryPolicy != null && !retryPolicy.retryableStatusCodes().isEmpty()) {
       ImmutableMap.Builder<String, Object> rawRetryPolicy = ImmutableMap.builder();
       rawRetryPolicy.put("maxAttempts", (double) retryPolicy.maxAttempts());
       rawRetryPolicy.put("initialBackoff", Durations.toString(retryPolicy.initialBackoff()));
diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java
index 807f512b0f4..876615d0b39 100644
--- a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java
+++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java
@@ -552,7 +552,8 @@ public void parseRouteAction_withRetryPolicy() {
         .setRetryPolicy(builder.build())
         .build();
     struct = ClientXdsClient.parseRouteAction(proto, filterRegistry, false);
-    assertThat(struct.getStruct().retryPolicy()).isNull();
+    assertThat(struct.getStruct().retryPolicy()).isNotNull();
+    assertThat(struct.getStruct().retryPolicy().retryableStatusCodes()).isEmpty();
 
     // base_interval unset
     builder
diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java
index 7a8fec5f74a..babaa2b3034 100644
--- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java
+++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java
@@ -988,6 +988,8 @@ public void generateServiceConfig_forPerMethodConfig() throws IOException {
     RetryPolicy retryPolicy = RetryPolicy.create(
         4, ImmutableList.of(Code.UNAVAILABLE, Code.CANCELLED), Durations.fromMillis(100),
         Durations.fromMillis(200), null);
+    RetryPolicy retryPolicyWithEmptyStatusCodes = RetryPolicy.create(
+        4, ImmutableList.<Code>of(), Durations.fromMillis(100), Durations.fromMillis(200), null);
 
     // timeout only
     String expectedServiceConfigJson = "{\n"
@@ -1001,6 +1003,11 @@ public void generateServiceConfig_forPerMethodConfig() throws IOException {
     assertThat(XdsNameResolver.generateServiceConfigWithMethodConfig(timeoutNano, null))
         .isEqualTo(expectedServiceConfig);
 
+    // timeout and retry with empty retriable status codes
+    assertThat(XdsNameResolver.generateServiceConfigWithMethodConfig(
+            timeoutNano, retryPolicyWithEmptyStatusCodes))
+        .isEqualTo(expectedServiceConfig);
+
     // retry only
     expectedServiceConfigJson = "{\n"
         + "  \"methodConfig\": [{\n"
@@ -1021,6 +1028,7 @@ public void generateServiceConfig_forPerMethodConfig() throws IOException {
     assertThat(XdsNameResolver.generateServiceConfigWithMethodConfig(null, retryPolicy))
         .isEqualTo(expectedServiceConfig);
 
+
     // timeout and retry
     expectedServiceConfigJson = "{\n"
         + "  \"methodConfig\": [{\n"
@@ -1043,12 +1051,16 @@ public void generateServiceConfig_forPerMethodConfig() throws IOException {
         .isEqualTo(expectedServiceConfig);
 
     // no timeout and no retry
-    // timeout and retry
     expectedServiceConfigJson = "{}";
     expectedServiceConfig =
         (Map<String, ?>) JsonParser.parse(expectedServiceConfigJson);
     assertThat(XdsNameResolver.generateServiceConfigWithMethodConfig(null, null))
         .isEqualTo(expectedServiceConfig);
+
+    // retry with emtry retriable status codes only
+    assertThat(XdsNameResolver.generateServiceConfigWithMethodConfig(
+            null, retryPolicyWithEmptyStatusCodes))
+        .isEqualTo(expectedServiceConfig);
   }
 
   @Test