Skip to content

Commit 9916540

Browse files
authored
fix: Move direct path misconfiguration log to before creating the first channel (#2430)
Fixes #2423 The root cause of the issue is that `logDirectPathMisconfig()` is called in the builder of `InstantiatingGrpcChannelProvider`, which could be called multiple times before it is fully instantiated. We should only call `logDirectPathMisconfig()` right before `createChannel()` which creates the first channel. We can not move it to before `createSingleChannel()` because it is used for resizing channel regularly after a client is initialized, and we only want to log direct path misconfiguration once.
1 parent 5c291e8 commit 9916540

File tree

2 files changed

+39
-5
lines changed

2 files changed

+39
-5
lines changed

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

+4-1
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,6 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
145145
builder.directPathServiceConfig == null
146146
? getDefaultDirectPathServiceConfig()
147147
: builder.directPathServiceConfig;
148-
logDirectPathMisconfig();
149148
}
150149

151150
/**
@@ -234,6 +233,7 @@ public TransportChannel getTransportChannel() throws IOException {
234233
} else if (needsEndpoint()) {
235234
throw new IllegalStateException("getTransportChannel() called when needsEndpoint() is true");
236235
} else {
236+
logDirectPathMisconfig();
237237
return createChannel();
238238
}
239239
}
@@ -272,6 +272,9 @@ boolean isDirectPathXdsEnabled() {
272272
return false;
273273
}
274274

275+
// This method should be called once per client initialization, hence can not be called in the
276+
// builder or createSingleChannel, only in getTransportChannel which creates the first channel
277+
// for a client.
275278
private void logDirectPathMisconfig() {
276279
if (isDirectPathXdsEnabled()) {
277280
// Case 1: does not enable DirectPath

gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java

+35-4
Original file line numberDiff line numberDiff line change
@@ -539,11 +539,19 @@ protected Object getMtlsObjectFromTransportChannel(MtlsProvider provider)
539539
}
540540

541541
@Test
542-
public void testLogDirectPathMisconfigAttrempDirectPathNotSet() {
542+
public void testLogDirectPathMisconfigAttrempDirectPathNotSet() throws Exception {
543543
FakeLogHandler logHandler = new FakeLogHandler();
544544
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
545545
InstantiatingGrpcChannelProvider provider =
546-
InstantiatingGrpcChannelProvider.newBuilder().setAttemptDirectPathXds().build();
546+
InstantiatingGrpcChannelProvider.newBuilder()
547+
.setAttemptDirectPathXds()
548+
.setHeaderProvider(Mockito.mock(HeaderProvider.class))
549+
.setExecutor(Mockito.mock(Executor.class))
550+
.setEndpoint("localhost:8080")
551+
.build();
552+
553+
provider.getTransportChannel();
554+
547555
assertThat(logHandler.getAllMessages())
548556
.contains(
549557
"DirectPath is misconfigured. Please set the attemptDirectPath option along with the"
@@ -552,15 +560,33 @@ public void testLogDirectPathMisconfigAttrempDirectPathNotSet() {
552560
}
553561

554562
@Test
555-
public void testLogDirectPathMisconfigWrongCredential() {
563+
public void testLogDirectPathMisconfig_shouldNotLogInTheBuilder() {
564+
FakeLogHandler logHandler = new FakeLogHandler();
565+
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
566+
InstantiatingGrpcChannelProvider.newBuilder()
567+
.setAttemptDirectPathXds()
568+
.setAttemptDirectPath(true)
569+
.build();
570+
571+
assertThat(logHandler.getAllMessages()).isEmpty();
572+
InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
573+
}
574+
575+
@Test
576+
public void testLogDirectPathMisconfigWrongCredential() throws Exception {
556577
FakeLogHandler logHandler = new FakeLogHandler();
557578
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
558579
InstantiatingGrpcChannelProvider provider =
559580
InstantiatingGrpcChannelProvider.newBuilder()
560581
.setAttemptDirectPathXds()
561582
.setAttemptDirectPath(true)
583+
.setHeaderProvider(Mockito.mock(HeaderProvider.class))
584+
.setExecutor(Mockito.mock(Executor.class))
562585
.setEndpoint("test.googleapis.com:443")
563586
.build();
587+
588+
provider.getTransportChannel();
589+
564590
assertThat(logHandler.getAllMessages())
565591
.contains(
566592
"DirectPath is misconfigured. Please make sure the credential is an instance of"
@@ -569,16 +595,21 @@ public void testLogDirectPathMisconfigWrongCredential() {
569595
}
570596

571597
@Test
572-
public void testLogDirectPathMisconfigNotOnGCE() {
598+
public void testLogDirectPathMisconfigNotOnGCE() throws Exception {
573599
FakeLogHandler logHandler = new FakeLogHandler();
574600
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
575601
InstantiatingGrpcChannelProvider provider =
576602
InstantiatingGrpcChannelProvider.newBuilder()
577603
.setAttemptDirectPathXds()
578604
.setAttemptDirectPath(true)
579605
.setAllowNonDefaultServiceAccount(true)
606+
.setHeaderProvider(Mockito.mock(HeaderProvider.class))
607+
.setExecutor(Mockito.mock(Executor.class))
580608
.setEndpoint("test.googleapis.com:443")
581609
.build();
610+
611+
provider.getTransportChannel();
612+
582613
if (!InstantiatingGrpcChannelProvider.isOnComputeEngine()) {
583614
assertThat(logHandler.getAllMessages())
584615
.contains(

0 commit comments

Comments
 (0)