diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 054075a1909..fd1631b1a82 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/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; } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index 2711c9af4b2..843f443ffdb 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/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); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 42fb374a68b..e958fcdae00 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/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 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 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(); } diff --git a/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java b/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java index 1c0eb3cd024..1e386a8e593 100644 --- a/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java +++ b/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java @@ -42,6 +42,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.logging.Logger; + /** * A version of {@link ServerBuilder} to create xDS managed servers. */ @@ -113,7 +114,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; } diff --git a/xds/src/test/java/io/grpc/xds/FakeControlPlaneXdsIntegrationTest.java b/xds/src/test/java/io/grpc/xds/FakeControlPlaneXdsIntegrationTest.java index d6361915940..e7d5d783546 100644 --- a/xds/src/test/java/io/grpc/xds/FakeControlPlaneXdsIntegrationTest.java +++ b/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,8 +394,9 @@ 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(); } @@ -403,11 +404,13 @@ public void configureChannelBuilder(ManagedChannelBuilder builder) { @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. @@ -415,13 +418,15 @@ public void childChannelConfigurator_passesMetricSinkToServer_E2E() throws Excep 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(); } diff --git a/xds/src/test/java/io/grpc/xds/GrpcXdsTransportFactoryTest.java b/xds/src/test/java/io/grpc/xds/GrpcXdsTransportFactoryTest.java index e0b41f64943..d6c3c6ea69e 100644 --- a/xds/src/test/java/io/grpc/xds/GrpcXdsTransportFactoryTest.java +++ b/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); diff --git a/xds/src/test/java/io/grpc/xds/XdsServerBuilderTest.java b/xds/src/test/java/io/grpc/xds/XdsServerBuilderTest.java index 503f1b670c0..dce077169c7 100644 --- a/xds/src/test/java/io/grpc/xds/XdsServerBuilderTest.java +++ b/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; @@ -332,7 +333,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 mockPool = mock(ObjectPool.class); @@ -346,8 +350,43 @@ public void start_passesChannelConfiguratorToClientPoolFactory() throws Exceptio Future unused = startServerAsync(); + ArgumentCaptor 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 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 captor = ArgumentCaptor.forClass(ChannelConfigurator.class); + verify(mockPoolFactory).getOrCreate( + any(), any(), any(), captor.capture()); + ChannelConfigurator captured = captor.getValue(); + + assertNotSame(configurer1, captured); + assertNotSame(configurer2, captured); } @Test