Skip to content

Commit

Permalink
fix: Move direct path misconfiguration log to before creating the fir…
Browse files Browse the repository at this point in the history
…st 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.
  • Loading branch information
blakeli0 authored Jan 31, 2024
1 parent 5c291e8 commit 9916540
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
builder.directPathServiceConfig == null
? getDefaultDirectPathServiceConfig()
: builder.directPathServiceConfig;
logDirectPathMisconfig();
}

/**
Expand Down Expand Up @@ -234,6 +233,7 @@ public TransportChannel getTransportChannel() throws IOException {
} else if (needsEndpoint()) {
throw new IllegalStateException("getTransportChannel() called when needsEndpoint() is true");
} else {
logDirectPathMisconfig();
return createChannel();
}
}
Expand Down Expand Up @@ -272,6 +272,9 @@ boolean isDirectPathXdsEnabled() {
return false;
}

// This method should be called once per client initialization, hence can not be called in the
// builder or createSingleChannel, only in getTransportChannel which creates the first channel
// for a client.
private void logDirectPathMisconfig() {
if (isDirectPathXdsEnabled()) {
// Case 1: does not enable DirectPath
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -539,11 +539,19 @@ protected Object getMtlsObjectFromTransportChannel(MtlsProvider provider)
}

@Test
public void testLogDirectPathMisconfigAttrempDirectPathNotSet() {
public void testLogDirectPathMisconfigAttrempDirectPathNotSet() throws Exception {
FakeLogHandler logHandler = new FakeLogHandler();
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder().setAttemptDirectPathXds().build();
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPathXds()
.setHeaderProvider(Mockito.mock(HeaderProvider.class))
.setExecutor(Mockito.mock(Executor.class))
.setEndpoint("localhost:8080")
.build();

provider.getTransportChannel();

assertThat(logHandler.getAllMessages())
.contains(
"DirectPath is misconfigured. Please set the attemptDirectPath option along with the"
Expand All @@ -552,15 +560,33 @@ public void testLogDirectPathMisconfigAttrempDirectPathNotSet() {
}

@Test
public void testLogDirectPathMisconfigWrongCredential() {
public void testLogDirectPathMisconfig_shouldNotLogInTheBuilder() {
FakeLogHandler logHandler = new FakeLogHandler();
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPathXds()
.setAttemptDirectPath(true)
.build();

assertThat(logHandler.getAllMessages()).isEmpty();
InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
}

@Test
public void testLogDirectPathMisconfigWrongCredential() throws Exception {
FakeLogHandler logHandler = new FakeLogHandler();
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPathXds()
.setAttemptDirectPath(true)
.setHeaderProvider(Mockito.mock(HeaderProvider.class))
.setExecutor(Mockito.mock(Executor.class))
.setEndpoint("test.googleapis.com:443")
.build();

provider.getTransportChannel();

assertThat(logHandler.getAllMessages())
.contains(
"DirectPath is misconfigured. Please make sure the credential is an instance of"
Expand All @@ -569,16 +595,21 @@ public void testLogDirectPathMisconfigWrongCredential() {
}

@Test
public void testLogDirectPathMisconfigNotOnGCE() {
public void testLogDirectPathMisconfigNotOnGCE() throws Exception {
FakeLogHandler logHandler = new FakeLogHandler();
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPathXds()
.setAttemptDirectPath(true)
.setAllowNonDefaultServiceAccount(true)
.setHeaderProvider(Mockito.mock(HeaderProvider.class))
.setExecutor(Mockito.mock(Executor.class))
.setEndpoint("test.googleapis.com:443")
.build();

provider.getTransportChannel();

if (!InstantiatingGrpcChannelProvider.isOnComputeEngine()) {
assertThat(logHandler.getAllMessages())
.contains(
Expand Down

0 comments on commit 9916540

Please sign in to comment.