Fix: Append child channel configurators instead of overwriting (#12921)

appends child channels instead of overwriting them while propagating to subsequent child channels

Author: AgraVator

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

@@ -762,8 +762,12 @@ protected ManagedChannelImplBuilder addMetricSink(MetricSink metricSink) {
   @Override
   public ManagedChannelImplBuilder childChannelConfigurator(
       ChannelConfigurator channelConfigurator) {
-    this.channelConfigurator = checkNotNull(channelConfigurator,
-        "childChannelConfigurator");
+    checkNotNull(channelConfigurator, "childChannelConfigurator");
+    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

@@ -46,6 +46,7 @@
 import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
+
 /**
  * A version of {@link ServerBuilder} to create xDS managed servers.
  */
@@ -118,7 +119,12 @@ public XdsServerBuilder drainGraceTime(long drainGraceTime, TimeUnit drainGraceT
    * @return this
    */
   public XdsServerBuilder childChannelConfigurator(ChannelConfigurator channelConfigurator) {
-    this.channelConfigurator = checkNotNull(channelConfigurator, "channelConfigurator");
+    checkNotNull(channelConfigurator, "channelConfigurator");
+    ChannelConfigurator oldConfigurator = this.channelConfigurator;
+    this.channelConfigurator = builder -> {
+      oldConfigurator.configureChannelBuilder(builder);
+      channelConfigurator.configureChannelBuilder(builder);
+    };
     return this;
   }
 

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

@@ -62,7 +62,6 @@
 import io.grpc.LoadBalancerRegistry;
 import io.grpc.LongCounterMetricInstrument;
 import io.grpc.ManagedChannel;
-import io.grpc.ManagedChannelBuilder;
 import io.grpc.Metadata;
 import io.grpc.MethodDescriptor;
 import io.grpc.NoopMetricSink;
@@ -375,17 +374,18 @@ public void pingPong_logicalDns_authorityOverride() {
 
   @Test
   public void childChannelConfigurator_passesMetricSinkToChannel_E2E() throws Exception {
-    CountingMetricSink sink = new CountingMetricSink();
-    ChannelConfigurator configurator = new ChannelConfigurator() {
-      @Override
-      public void configureChannelBuilder(ManagedChannelBuilder<?> builder) {
-        InternalManagedChannelBuilder.addMetricSink(builder, sink);
-      }
-    };
+    CountingMetricSink sink1 = new CountingMetricSink();
+    ChannelConfigurator configurator1 =
+        builder -> InternalManagedChannelBuilder.addMetricSink(builder, sink1);
+
+    CountingMetricSink sink2 = new CountingMetricSink();
+    ChannelConfigurator configurator2 =
+        builder -> InternalManagedChannelBuilder.addMetricSink(builder, sink2);
 
     ManagedChannel channel = Grpc.newChannelBuilder("test-xds:///test-server",
             InsecureChannelCredentials.create())
-        .childChannelConfigurator(configurator)
+        .childChannelConfigurator(configurator1)
+        .childChannelConfigurator(configurator2)
         .build();
 
     try {
@@ -394,34 +394,39 @@ public void configureChannelBuilder(ManagedChannelBuilder<?> builder) {
       blockingStub.unaryRpc(SimpleRequest.getDefaultInstance());
 
       // The xDS client inside the channel configurator will have created an ADS stream.
-      // The metric sink should have received attempt or connection metrics.
-      sink.awaitCall();
+      // Both metric sinks should have received attempt or connection metrics.
+      sink1.awaitCall();
+      sink2.awaitCall();
     } finally {
       channel.shutdownNow();
     }
   }
 
   @Test
   public void childChannelConfigurator_passesMetricSinkToServer_E2E() throws Exception {
-    CountingMetricSink sink = new CountingMetricSink();
-    ChannelConfigurator configurator = builder -> {
-      // Child channels (xDS client connections) created by this server get the sink.
-      InternalManagedChannelBuilder.addMetricSink(builder, sink);
-    };
+    CountingMetricSink sink1 = new CountingMetricSink();
+    ChannelConfigurator configurator1 =
+        builder -> InternalManagedChannelBuilder.addMetricSink(builder, sink1);
+
+    CountingMetricSink sink2 = new CountingMetricSink();
+    ChannelConfigurator configurator2 =
+        builder -> InternalManagedChannelBuilder.addMetricSink(builder, sink2);
 
     // We start an XdsServer manually.
     // XdsServer needs RDS, LDS, etc. from control plane.
     XdsServerBuilder serverBuilder = XdsServerBuilder.forPort(
             0, InsecureServerCredentials.create())
         .addService(new SimpleServiceGrpc.SimpleServiceImplBase() {})
         .overrideBootstrapForTest(controlPlane.defaultBootstrapOverride())
-        .childChannelConfigurator(configurator);
+        .childChannelConfigurator(configurator1)
+        .childChannelConfigurator(configurator2);
 
     Server childServer = serverBuilder.build().start();
 
     try {
       // The server xDS client will connect to control plane to get LDS.
-      sink.awaitCall();
+      sink1.awaitCall();
+      sink2.awaitCall();
     } finally {
       childServer.shutdownNow();
     }

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;
@@ -33,6 +33,7 @@
 import io.grpc.BindableService;
 import io.grpc.ChannelConfigurator;
 import io.grpc.InsecureServerCredentials;
+import io.grpc.ManagedChannelBuilder;
 import io.grpc.ServerServiceDefinition;
 import io.grpc.Status;
 import io.grpc.StatusException;
@@ -406,7 +407,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);
@@ -420,8 +424,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());
+    
+    ManagedChannelBuilder<?> testBuilder = mock(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