From 99165403902ff91ecb0b14b858333855e7a10c60 Mon Sep 17 00:00:00 2001
From: Blake Li <blakeli@google.com>
Date: Wed, 31 Jan 2024 00:10:37 -0500
Subject: [PATCH] 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.
---
 .../InstantiatingGrpcChannelProvider.java     |  5 ++-
 .../InstantiatingGrpcChannelProviderTest.java | 39 +++++++++++++++++--
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java b/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
index 280f9cf78e..bf8ffc81da 100644
--- a/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
+++ b/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
@@ -145,7 +145,6 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
         builder.directPathServiceConfig == null
             ? getDefaultDirectPathServiceConfig()
             : builder.directPathServiceConfig;
-    logDirectPathMisconfig();
   }
 
   /**
@@ -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();
     }
   }
@@ -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
diff --git a/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java b/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
index f6fb9d00ac..a58d10ffc6 100644
--- a/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
+++ b/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
@@ -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"
@@ -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"
@@ -569,7 +595,7 @@ public void testLogDirectPathMisconfigWrongCredential() {
   }
 
   @Test
-  public void testLogDirectPathMisconfigNotOnGCE() {
+  public void testLogDirectPathMisconfigNotOnGCE() throws Exception {
     FakeLogHandler logHandler = new FakeLogHandler();
     InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
     InstantiatingGrpcChannelProvider provider =
@@ -577,8 +603,13 @@ public void testLogDirectPathMisconfigNotOnGCE() {
             .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(