rls: fix rls oobChannel grpclb config service name

The serviceName field in oobChannel grpclb config should not be null, otherwise it will default to the lbHelper.getAuthority(), which perviously defaulted to the lookup service before #7852, but has been overridden to the backend service for authentication in #7852.
grpc · Feb 17, 2021 · da86eea · da86eea
1 parent f60a072
commit da86eea
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 4 deletions.
diff --git a/rls/build.gradle b/rls/build.gradle
@@ -17,6 +17,7 @@ dependencies {
     guavaDependency 'implementation'
     compileOnly libraries.javax_annotation
     testImplementation libraries.truth,
+            project(':grpc-grpclb'),
             project(':grpc-testing'),
             project(':grpc-testing-proto'),
             project(':grpc-core').sourceSets.test.output  // for FakeClock

diff --git a/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java b/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java
@@ -162,11 +162,13 @@ private CachingRlsLbClient(Builder builder) {
         rlsConfig.getLookupService(), helper.getUnsafeChannelCredentials());
     rlsChannelBuilder.overrideAuthority(helper.getAuthority());
     if (enableOobChannelDirectPath) {
+      Map<String, ?> directPathServiceConfig =
+          getDirectPathServiceConfig(rlsConfig.getLookupService());
       logger.log(
           ChannelLogLevel.DEBUG,
           "RLS channel direct path enabled. RLS channel service config: {0}",
-          getDirectpathServiceConfig());
-      rlsChannelBuilder.defaultServiceConfig(getDirectpathServiceConfig());
+          directPathServiceConfig);
+      rlsChannelBuilder.defaultServiceConfig(directPathServiceConfig);
       rlsChannelBuilder.disableServiceConfigLookUp();
     }
     rlsChannel = rlsChannelBuilder.build();
@@ -183,12 +185,14 @@ private CachingRlsLbClient(Builder builder) {
     logger.log(ChannelLogLevel.DEBUG, "CachingRlsLbClient created");
   }
 
-  private static ImmutableMap<String, Object> getDirectpathServiceConfig() {
+  private static ImmutableMap<String, Object> getDirectPathServiceConfig(String serviceName) {
     ImmutableMap<String, Object> pickFirstStrategy =
         ImmutableMap.<String, Object>of("pick_first", ImmutableMap.of());
 
     ImmutableMap<String, Object> childPolicy =
-        ImmutableMap.<String, Object>of("childPolicy", ImmutableList.of(pickFirstStrategy));
+        ImmutableMap.<String, Object>of(
+            "childPolicy", ImmutableList.of(pickFirstStrategy),
+            "serviceName", serviceName);
 
     ImmutableMap<String, Object> grpcLbPolicy =
         ImmutableMap.<String, Object>of("grpclb", childPolicy);

diff --git a/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java b/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java
@@ -145,12 +145,16 @@ public void uncaughtException(Thread t, Throwable e) {
 
   private CachingRlsLbClient rlsLbClient;
   private boolean existingEnableOobChannelDirectPath;
+  private Map<String, ?> rlsChannelServiceConfig;
+  private String rlsChannelOverriddenAuthority;
 
   @Before
   public void setUp() throws Exception {
     existingEnableOobChannelDirectPath = CachingRlsLbClient.enableOobChannelDirectPath;
     CachingRlsLbClient.enableOobChannelDirectPath = false;
+  }
 
+  private void setUpRlsLbClient() {
     rlsLbClient =
         CachingRlsLbClient.newBuilder()
             .setBackoffProvider(fakeBackoffProvider)
@@ -185,6 +189,7 @@ public void run() {
 
   @Test
   public void get_noError_lifeCycle() throws Exception {
+    setUpRlsLbClient();
     InOrder inOrder = inOrder(evictionListener);
     RouteLookupRequest routeLookupRequest =
         new RouteLookupRequest(
@@ -233,8 +238,44 @@ public void get_noError_lifeCycle() throws Exception {
     inOrder.verifyNoMoreInteractions();
   }
 
+  @Test
+  public void rls_overDirectPath() throws Exception {
+    CachingRlsLbClient.enableOobChannelDirectPath = true;
+    setUpRlsLbClient();
+    RouteLookupRequest routeLookupRequest =
+        new RouteLookupRequest(
+            "bigtable.googleapis.com", "/foo/bar", "grpc", ImmutableMap.<String, String>of());
+    rlsServerImpl.setLookupTable(
+        ImmutableMap.of(
+            routeLookupRequest,
+            new RouteLookupResponse(ImmutableList.of("target"), "header")));
+
+    // initial request
+    CachedRouteLookupResponse resp = getInSyncContext(routeLookupRequest);
+    assertThat(resp.isPending()).isTrue();
+
+    // server response
+    fakeTimeProvider.forwardTime(SERVER_LATENCY_MILLIS, TimeUnit.MILLISECONDS);
+
+    resp = getInSyncContext(routeLookupRequest);
+    assertThat(resp.hasData()).isTrue();
+
+    assertThat(rlsChannelOverriddenAuthority).isEqualTo("bigtable.googleapis.com:443");
+    assertThat(rlsChannelServiceConfig).isEqualTo(
+        ImmutableMap.of(
+            "loadBalancingConfig",
+            ImmutableList.of(ImmutableMap.of(
+                "grpclb",
+                ImmutableMap.of(
+                    "childPolicy",
+                    ImmutableList.of(ImmutableMap.of("pick_first", ImmutableMap.of())),
+                    "serviceName",
+                    "service1")))));
+  }
+
   @Test
   public void get_throttledAndRecover() throws Exception {
+    setUpRlsLbClient();
     RouteLookupRequest routeLookupRequest =
         new RouteLookupRequest("server", "/foo/bar", "grpc", ImmutableMap.<String, String>of());
     rlsServerImpl.setLookupTable(
@@ -276,6 +317,7 @@ public void get_throttledAndRecover() throws Exception {
 
   @Test
   public void get_updatesLbState() throws Exception {
+    setUpRlsLbClient();
     InOrder inOrder = inOrder(helper);
     RouteLookupRequest routeLookupRequest =
         new RouteLookupRequest(
@@ -344,6 +386,7 @@ public void get_updatesLbState() throws Exception {
 
   @Test
   public void get_childPolicyWrapper_reusedForSameTarget() throws Exception {
+    setUpRlsLbClient();
     RouteLookupRequest routeLookupRequest =
         new RouteLookupRequest("server", "/foo/bar", "grpc", ImmutableMap.<String, String>of());
     RouteLookupRequest routeLookupRequest2 =
@@ -565,6 +608,20 @@ protected ManagedChannelBuilder<?> delegate() {
         public ManagedChannel build() {
           return grpcCleanupRule.register(super.build());
         }
+
+        @Override
+        public CleaningChannelBuilder defaultServiceConfig(Map<String, ?> serviceConfig) {
+          rlsChannelServiceConfig = serviceConfig;
+          delegate().defaultServiceConfig(serviceConfig);
+          return this;
+        }
+
+        @Override
+        public CleaningChannelBuilder overrideAuthority(String authority) {
+          rlsChannelOverriddenAuthority = authority;
+          delegate().overrideAuthority(authority);
+          return this;
+        }
       }
 
       return new CleaningChannelBuilder();