xds: fix parsing retryOn values (grpc#8477)

- Envoy ignores white spaces in `retryOn` field https://github.com/envoyproxy/envoy/blob/v1.19.1/source/common/router/retry_state_impl.cc#L166 We should do the same. - Envoy ignores unsupported values https://github.com/envoyproxy/envoy/blob/v1.19.1/source/common/router/config_impl.cc#L89-L90 and we should do the same.
dapengzhang0 · Sep 7, 2021 · 6448cfa · 6448cfa
1 parent fcffe92
commit 6448cfa
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 7 deletions.
diff --git a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java
@@ -1254,21 +1254,20 @@ private static StructOrError<RetryPolicy> parseRetryPolicy(
         maxBackoff = Durations.fromNanos(Durations.toNanos(initialBackoff) * 10);
       }
     }
-    Iterable<String> retryOns = Splitter.on(',').split(retryPolicyProto.getRetryOn());
+    Iterable<String> retryOns =
+        Splitter.on(',').omitEmptyStrings().trimResults().split(retryPolicyProto.getRetryOn());
     ImmutableList.Builder<Code> retryableStatusCodesBuilder = ImmutableList.builder();
     for (String retryOn : retryOns) {
       Code code;
       try {
         code = Code.valueOf(retryOn.toUpperCase(Locale.US).replace('-', '_'));
       } catch (IllegalArgumentException e) {
-        // TODO(zdapeng): TBD
         // unsupported value, such as "5xx"
-        return null;
+        continue;
       }
       if (!SUPPORTED_RETRYABLE_CODES.contains(code)) {
-        // TODO(zdapeng): TBD
         // unsupported value
-        return null;
+        continue;
       }
       retryableStatusCodesBuilder.add(code);
     }

diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java
@@ -646,7 +646,8 @@ public void parseRouteAction_withRetryPolicy() {
         .setRetryPolicy(builder)
         .build();
     struct = ClientXdsClient.parseRouteAction(proto, filterRegistry, false);
-    assertThat(struct.getStruct().retryPolicy()).isNull();
+    assertThat(struct.getStruct().retryPolicy().retryableStatusCodes())
+        .containsExactly(Code.CANCELLED);
 
     // unsupported retry_on code
     builder = RetryPolicy.newBuilder()
@@ -662,7 +663,25 @@ public void parseRouteAction_withRetryPolicy() {
         .setRetryPolicy(builder)
         .build();
     struct = ClientXdsClient.parseRouteAction(proto, filterRegistry, false);
-    assertThat(struct.getStruct().retryPolicy()).isNull();
+    assertThat(struct.getStruct().retryPolicy().retryableStatusCodes())
+        .containsExactly(Code.CANCELLED);
+
+    // whitespace in retry_on
+    builder = RetryPolicy.newBuilder()
+        .setNumRetries(UInt32Value.of(3))
+        .setRetryBackOff(
+            RetryBackOff.newBuilder()
+                .setBaseInterval(Durations.fromMillis(500))
+                .setMaxInterval(Durations.fromMillis(600)))
+        .setPerTryTimeout(Durations.fromMillis(300))
+        .setRetryOn("abort, , cancelled , ");
+    proto = io.envoyproxy.envoy.config.route.v3.RouteAction.newBuilder()
+        .setCluster("cluster-foo")
+        .setRetryPolicy(builder)
+        .build();
+    struct = ClientXdsClient.parseRouteAction(proto, filterRegistry, false);
+    assertThat(struct.getStruct().retryPolicy().retryableStatusCodes())
+        .containsExactly(Code.CANCELLED);
   }
 
   @Test