fix: improve warnings for Direct Path xDS set via env (#3019)

Fixes #2427

### Context
If `GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS` is set in the environment
aiming to enable Direct Path xDS in another client, we want to just warn
the user that _if this is intended_, it can be a misconfiguration and
that the gRPCLB Direct Path setting should be enabled as well.

### Approach
As said in the context, we will _warn_ the user that it _could_ be a
misconfiguration _if_ the env var setting was meant for the client in
question. Other warn cases remain the same.

### Coverage
Due to the fact that part of the method in question is tested via a
special env var test (not detected by SonarCloud), it will show as
uncovered. See this screenshot for coverage of all tests combined

![image](https://github.com/googleapis/sdk-platform-java/assets/22083784/af24e721-6495-4030-8804-c013b14679d7)

---------

Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>

.github/workflows/ci.yaml

@@ -30,6 +30,7 @@ jobs:
  # Set the Env Var for this step only
  env:
  GOOGLE_CLOUD_UNIVERSE_DOMAIN: random.com
+ GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS: true
  - run: bazelisk version
  - name: Install Maven modules
  run: |
@@ -80,6 +81,7 @@ jobs:
  # Set the Env Var for this step only
  env:
  GOOGLE_CLOUD_UNIVERSE_DOMAIN: random.com
+ GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS: true
  - run: bazelisk version
  - name: Install Maven modules
  run: |
@@ -130,6 +132,7 @@ jobs:
  # Set the Env Var for this step only
  env:
  GOOGLE_CLOUD_UNIVERSE_DOMAIN: random.com
+ GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS: true
 
  build-java8-gapic-generator-java:
  name: "build(8) for gapic-generator-java"

gax-java/gax-grpc/pom.xml

@@ -128,6 +128,30 @@
  </execution>
  </executions>
  </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <!-- These tests require an Env Var to be set. Use -PenvVarTest to ONLY run these tests -->
+ <test>!InstantiatingGrpcChannelProviderTest#testLogDirectPathMisconfig_AttemptDirectPathNotSetAndAttemptDirectPathXdsSetViaEnv_warns</test>
+ </configuration>
+ </plugin>
  </plugins>
  </build>
+ <profiles>
+ <profile>
+ <id>envVarTest</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <test>InstantiatingGrpcChannelProviderTest#testLogDirectPathMisconfig_AttemptDirectPathNotSetAndAttemptDirectPathXdsSetViaEnv_warns</test>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ </profiles>
 </project>

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

@@ -92,7 +92,10 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
  static final Logger LOG = Logger.getLogger(InstantiatingGrpcChannelProvider.class.getName());
 
  static final String DIRECT_PATH_ENV_DISABLE_DIRECT_PATH = "GOOGLE_CLOUD_DISABLE_DIRECT_PATH";
- private static final String DIRECT_PATH_ENV_ENABLE_XDS = "GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS";
+
+ @VisibleForTesting
+ static final String DIRECT_PATH_ENV_ENABLE_XDS = "GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS";
+
  static final long DIRECT_PATH_KEEP_ALIVE_TIME_SECONDS = 3600;
  static final long DIRECT_PATH_KEEP_ALIVE_TIMEOUT_SECONDS = 20;
  static final String GCE_PRODUCTION_NAME_PRIOR_2016 = "Google";
@@ -273,42 +276,63 @@ private boolean isDirectPathEnabled() {
  return false;
  }
 
+ private boolean isDirectPathXdsEnabledViaBuilderOption() {
+ return Boolean.TRUE.equals(attemptDirectPathXds);
+ }
+
+ private boolean isDirectPathXdsEnabledViaEnv() {
+ String directPathXdsEnv = envProvider.getenv(DIRECT_PATH_ENV_ENABLE_XDS);
+ return Boolean.parseBoolean(directPathXdsEnv);
+ }
+
+ /**
+ * This method tells if Direct Path xDS was enabled. There are two ways of enabling it: via
+ * environment variable (by setting GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS=true) or when building
+ * this channel provider (via the {@link Builder#setAttemptDirectPathXds()} method).
+ *
+ * @return true if Direct Path xDS was either enabled via env var or via builder option
+ */
  @InternalApi
  public boolean isDirectPathXdsEnabled() {
- // Method 1: Enable DirectPath xDS by option.
- if (Boolean.TRUE.equals(attemptDirectPathXds)) {
- return true;
- }
- // Method 2: Enable DirectPath xDS by env.
- String directPathXdsEnv = envProvider.getenv(DIRECT_PATH_ENV_ENABLE_XDS);
- boolean isDirectPathXdsEnv = Boolean.parseBoolean(directPathXdsEnv);
- if (isDirectPathXdsEnv) {
- return true;
- }
- return false;
+ return isDirectPathXdsEnabledViaEnv() || isDirectPathXdsEnabledViaBuilderOption();
  }
 
  // 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
  if (!isDirectPathEnabled()) {
- LOG.log(
- Level.WARNING,
- "DirectPath is misconfigured. Please set the attemptDirectPath option along with the"
- + " attemptDirectPathXds option.");
+ // This misconfiguration occurs when Direct Path xDS is enabled, but Direct Path is not
+ // Direct Path xDS can be enabled two ways: via environment variable or via builder.
+ // Case 1: Direct Path is only enabled via xDS env var. We will _warn_ the user that this is
+ // a misconfiguration if they intended to set the env var.
+ if (isDirectPathXdsEnabledViaEnv()) {
+ LOG.log(
+ Level.WARNING,
+ "Env var "
+ + DIRECT_PATH_ENV_ENABLE_XDS
+ + " was found and set to TRUE, but DirectPath was not enabled for this client. If this is intended for "
+ + "this client, please note that this is a misconfiguration and set the attemptDirectPath option as well.");
+ }
+ // Case 2: Direct Path xDS was enabled via Builder. Direct Path Traffic Director must be set
+ // (enabled with `setAttemptDirectPath(true)`) along with xDS.
+ // Here we warn the user about this.
+ else if (isDirectPathXdsEnabledViaBuilderOption()) {
+ LOG.log(
+ Level.WARNING,
+ "DirectPath is misconfigured. The DirectPath XDS option was set, but the attemptDirectPath option was not. Please set both the attemptDirectPath and attemptDirectPathXds options.");
+ }
  } else {
- // Case 2: credential is not correctly set
+ // Case 3: credential is not correctly set
  if (!isCredentialDirectPathCompatible()) {
  LOG.log(
  Level.WARNING,
  "DirectPath is misconfigured. Please make sure the credential is an instance of "
  + ComputeEngineCredentials.class.getName()
  + " .");
  }
- // Case 3: not running on GCE
+ // Case 4: not running on GCE
  if (!isOnComputeEngine()) {
  LOG.log(
  Level.WARNING,

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

@@ -594,28 +594,50 @@ protected Object getMtlsObjectFromTransportChannel(MtlsProvider provider)
  return channelProvider.createMtlsChannelCredentials();
  }
 
+ private InstantiatingGrpcChannelProvider.Builder
+ createChannelProviderBuilderForDirectPathLogTests() {
+ return InstantiatingGrpcChannelProvider.newBuilder()
+ .setHeaderProvider(Mockito.mock(HeaderProvider.class))
+ .setExecutor(Mockito.mock(Executor.class))
+ .setEndpoint("localhost:8080");
+ }
+
+ private void createAndCloseTransportChannel(InstantiatingGrpcChannelProvider provider)
+ throws Exception {
+ TransportChannel transportChannel = provider.getTransportChannel();
+ transportChannel.close();
+ transportChannel.awaitTermination(10, TimeUnit.SECONDS);
+ }
+
  @Test
- void testLogDirectPathMisconfigAttrempDirectPathNotSet() throws Exception {
+ void
+ testLogDirectPathMisconfig_AttemptDirectPathNotSetAndAttemptDirectPathXdsSetViaBuilder_warns()
+ throws Exception {
  FakeLogHandler logHandler = new FakeLogHandler();
  InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
  InstantiatingGrpcChannelProvider provider =
- InstantiatingGrpcChannelProvider.newBuilder()
- .setAttemptDirectPathXds()
- .setHeaderProvider(Mockito.mock(HeaderProvider.class))
- .setExecutor(Mockito.mock(Executor.class))
- .setEndpoint("localhost:8080")
- .build();
+ createChannelProviderBuilderForDirectPathLogTests().setAttemptDirectPathXds().build();
+ createAndCloseTransportChannel(provider);
+ assertThat(logHandler.getAllMessages())
+ .contains(
+ "DirectPath is misconfigured. The DirectPath XDS option was set, but the attemptDirectPath option was not. Please set both the attemptDirectPath and attemptDirectPathXds options.");
+ InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
+ }
 
- TransportChannel transportChannel = provider.getTransportChannel();
+ @Test
+ void testLogDirectPathMisconfig_AttemptDirectPathNotSetAndAttemptDirectPathXdsSetViaEnv_warns()
+ throws Exception {
+ FakeLogHandler logHandler = new FakeLogHandler();
+ InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
 
+ InstantiatingGrpcChannelProvider provider =
+ createChannelProviderBuilderForDirectPathLogTests().build();
+ createAndCloseTransportChannel(provider);
  assertThat(logHandler.getAllMessages())
  .contains(
- "DirectPath is misconfigured. Please set the attemptDirectPath option along with the"
- + " attemptDirectPathXds option.");
+ "Env var GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS was found and set to TRUE, but DirectPath was not enabled for this client. If this is intended for "
+ + "this client, please note that this is a misconfiguration and set the attemptDirectPath option as well.");
  InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
-
- transportChannel.close();
- transportChannel.awaitTermination(10, TimeUnit.SECONDS);
  }
 
  @Test