Fix: Append child channel configurators instead of overwriting

Author: AgraVator

core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java

@@ -762,8 +762,16 @@ protected ManagedChannelImplBuilder addMetricSink(MetricSink metricSink) {
   @Override
   public ManagedChannelImplBuilder childChannelConfigurator(
       ChannelConfigurator channelConfigurator) {
-    this.channelConfigurator = checkNotNull(channelConfigurator,
-        "childChannelConfigurator");
+    checkNotNull(channelConfigurator, "childChannelConfigurator");
+    if (this.channelConfigurator == null) {
+      this.channelConfigurator = channelConfigurator;
+    } else {
+      ChannelConfigurator oldConfigurator = this.channelConfigurator;
+      this.channelConfigurator = builder -> {
+        oldConfigurator.configureChannelBuilder(builder);
+        channelConfigurator.configureChannelBuilder(builder);
+      };
+    }
     return this;
   }
 

core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java

@@ -808,13 +808,6 @@ public void setNameResolverExtArgs() {
     assertThat(builder.nameResolverCustomArgs.get(testKey)).isEqualTo(42);
   }
 
-  @Test
-  public void childChannelConfigurator_setsField() {
-    ChannelConfigurator configurator = builder -> { };
-    assertSame(builder, builder.childChannelConfigurator(configurator));
-    assertSame(configurator, builder.channelConfigurator);
-  }
-
   @Test
   public void childChannelConfigurator_propagatesMetricsAndInterceptors_xdsTarget() {
     // Setup Mocks
@@ -902,16 +895,13 @@ public String getDefaultScheme() {
     assertNotNull("Child channel configurator should be present in NameResolver.Args",
         channelConfiguratorInArgs);
 
-    // Verify the configurator is the one we passed
-    assertThat(channelConfiguratorInArgs).isSameInstanceAs(configurator);
-
     // Verify the configurator logically applies (by running it on a real builder)
     ManagedChannelImplBuilder childBuilder = new ManagedChannelImplBuilder(
         "xds:///child-service-target",
         mockClientTransportFactoryBuilder,
         new FixedPortProvider(DUMMY_PORT));
 
-    configurator.configureChannelBuilder(childBuilder);
+    channelConfiguratorInArgs.configureChannelBuilder(childBuilder);
     assertThat(childBuilder.metricSinks).contains(mockMetricSink);
   }
 

core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java

@@ -499,7 +499,10 @@ public void immediateDeadlineExceeded() {
 
   @Test
   public void childChannelConfigurator_passedToNameResolverArgs() {
-    ChannelConfigurator configurator = builder -> { };
+    final boolean[] configuratorInvoked = new boolean[1];
+    ChannelConfigurator configurator = builder -> {
+      configuratorInvoked[0] = true;
+    };
     channelBuilder.childChannelConfigurator(configurator);
     AtomicReference<NameResolver.Args> actualArgs = new AtomicReference<>();
     channelBuilder.nameResolverRegistry.register(new NameResolverProvider() {
@@ -528,12 +531,18 @@ protected int priority() {
     });
     createChannel();
     assertNotNull(actualArgs.get());
-    assertSame(configurator, actualArgs.get().getChildChannelConfigurator());
+    ChannelConfigurator childConfigurator = actualArgs.get().getChildChannelConfigurator();
+    assertNotNull(childConfigurator);
+    childConfigurator.configureChannelBuilder(channelBuilder);
+    assertTrue(configuratorInvoked[0]);
   }
 
   @Test
   public void childChannelConfigurator_passedToResolvingOobChannelNameResolverArgs() {
-    ChannelConfigurator configurator = builder -> { };
+    final boolean[] configuratorInvoked = new boolean[1];
+    ChannelConfigurator configurator = builder -> {
+      configuratorInvoked[0] = true;
+    };
     channelBuilder.childChannelConfigurator(configurator);
     AtomicReference<NameResolver.Args> oobArgs = new AtomicReference<>();
     channelBuilder.nameResolverRegistry.register(new NameResolverProvider() {
@@ -567,7 +576,10 @@ protected int priority() {
     ManagedChannel oob = helper.createResolvingOobChannelBuilder("oobauthority").build();
     oob.getState(true);
     assertNotNull(oobArgs.get());
-    assertSame(configurator, oobArgs.get().getChildChannelConfigurator());
+    ChannelConfigurator childConfigurator = oobArgs.get().getChildChannelConfigurator();
+    assertNotNull(childConfigurator);
+    childConfigurator.configureChannelBuilder(channelBuilder);
+    assertTrue(configuratorInvoked[0]);
     oob.shutdownNow();
   }
 

xds/src/main/java/io/grpc/xds/XdsServerBuilder.java

@@ -113,7 +113,16 @@ public XdsServerBuilder drainGraceTime(long drainGraceTime, TimeUnit drainGraceT
    * @return this
    */
   public XdsServerBuilder childChannelConfigurator(ChannelConfigurator channelConfigurator) {
-    this.channelConfigurator = checkNotNull(channelConfigurator, "channelConfigurator");
+    checkNotNull(channelConfigurator, "channelConfigurator");
+    if (this.channelConfigurator == null) {
+      this.channelConfigurator = channelConfigurator;
+    } else {
+      ChannelConfigurator oldConfigurator = this.channelConfigurator;
+      this.channelConfigurator = builder -> {
+        oldConfigurator.configureChannelBuilder(builder);
+        channelConfigurator.configureChannelBuilder(builder);
+      };
+    }
     return this;
   }
 

xds/src/test/java/io/grpc/xds/GrpcXdsTransportFactoryTest.java

@@ -18,7 +18,6 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertSame;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -260,13 +259,20 @@ protected int priority() {
     };
     NameResolverRegistry.getDefaultRegistry().register(testProvider);
     try {
-      ChannelConfigurator configurer = builder -> { };
+      final boolean[] configuratorInvoked = new boolean[1];
+      ChannelConfigurator configurer = builder -> {
+        configuratorInvoked[0] = true;
+      };
       GrpcXdsTransportFactory factory = new GrpcXdsTransportFactory(null, configurer);
       XdsTransportFactory.XdsTransport transport = factory.create(
           Bootstrapper.ServerInfo.create(
               "test-xds-transport://localhost:8080", InsecureChannelCredentials.create()));
       assertNotNull(capturedArgs.get());
-      assertSame(configurer, capturedArgs.get().getChildChannelConfigurator());
+      ChannelConfigurator childConfigurator = capturedArgs.get().getChildChannelConfigurator();
+      assertNotNull(childConfigurator);
+      ManagedChannelBuilder<?> testBuilder = mock(ManagedChannelBuilder.class);
+      childConfigurator.configureChannelBuilder(testBuilder);
+      assertThat(configuratorInvoked[0]).isTrue();
       transport.shutdown();
     } finally {
       NameResolverRegistry.getDefaultRegistry().deregister(testProvider);

xds/src/test/java/io/grpc/xds/XdsServerBuilderTest.java

@@ -18,9 +18,9 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static io.grpc.xds.XdsServerTestHelper.buildTestListener;
+import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.any;
-import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
@@ -332,7 +332,10 @@ public void testOverrideBootstrap() throws Exception {
 
   @Test
   public void start_passesChannelConfiguratorToClientPoolFactory() throws Exception {
-    ChannelConfigurator configurer = builder -> { };
+    final boolean[] configuratorInvoked = new boolean[1];
+    ChannelConfigurator configurer = builder -> {
+      configuratorInvoked[0] = true;
+    };
     XdsClientPoolFactory mockPoolFactory = mock(XdsClientPoolFactory.class);
     @SuppressWarnings("unchecked")
     ObjectPool<XdsClient> mockPool = mock(ObjectPool.class);
@@ -346,8 +349,43 @@ public void start_passesChannelConfiguratorToClientPoolFactory() throws Exceptio
 
     Future<?> unused = startServerAsync();
 
+    ArgumentCaptor<ChannelConfigurator> configuratorCaptor =
+        ArgumentCaptor.forClass(ChannelConfigurator.class);
     verify(mockPoolFactory).getOrCreate(
-        any(), any(), any(), eq(configurer));
+        any(), any(), any(), configuratorCaptor.capture());
+    
+    io.grpc.ManagedChannelBuilder<?> testBuilder = mock(io.grpc.ManagedChannelBuilder.class);
+    configuratorCaptor.getValue().configureChannelBuilder(testBuilder);
+    assertThat(configuratorInvoked[0]).isTrue();
+  }
+
+  @Test
+  public void childChannelConfigurator_appendsConfigurators() throws Exception {
+    ChannelConfigurator configurer1 = builder -> { };
+    ChannelConfigurator configurer2 = builder -> { };
+    
+    XdsClientPoolFactory mockPoolFactory = mock(XdsClientPoolFactory.class);
+    @SuppressWarnings("unchecked")
+    ObjectPool<XdsClient> mockPool = mock(ObjectPool.class);
+    when(mockPool.getObject()).thenReturn(xdsClient);
+    when(mockPoolFactory.getOrCreate(any(), any(), any(), any())).thenReturn(mockPool);
+
+    buildBuilder(null);
+    builder.childChannelConfigurator(configurer1);
+    builder.childChannelConfigurator(configurer2);
+    builder.xdsClientPoolFactory(mockPoolFactory);
+    xdsServer = cleanupRule.register((XdsServerWrapper) builder.build());
+
+    Future<?> unused = startServerAsync();
+
+    // The captured configurator should be a composite of configurer1 and configurer2
+    ArgumentCaptor<ChannelConfigurator> captor = ArgumentCaptor.forClass(ChannelConfigurator.class);
+    verify(mockPoolFactory).getOrCreate(
+        any(), any(), any(), captor.capture());
+    ChannelConfigurator captured = captor.getValue();
+    
+    assertNotSame(configurer1, captured);
+    assertNotSame(configurer2, captured);
   }
 
   @Test