From 17a5decd339d26132f5c8ee2bebeefbee771fa20 Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Thu, 4 Jul 2024 14:58:11 -0400 Subject: [PATCH 01/15] fix: improve warnings for Direct Path xDS set via env Fixes #2427 --- .github/workflows/ci.yaml | 1 + gax-java/gax-grpc/pom.xml | 24 ++++++++ .../InstantiatingGrpcChannelProvider.java | 46 +++++++++----- .../InstantiatingGrpcChannelProviderTest.java | 61 +++++++++++++++---- 4 files changed, 105 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index f6a43b3557..3bbf5560fd 100644 --- a/.github/workflows/ci.yaml +++ b/.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: | diff --git a/gax-java/gax-grpc/pom.xml b/gax-java/gax-grpc/pom.xml index c415936dba..60710ec47e 100644 --- a/gax-java/gax-grpc/pom.xml +++ b/gax-java/gax-grpc/pom.xml @@ -128,6 +128,30 @@ + + org.apache.maven.plugins + maven-surefire-plugin + + + !InstantiatingGrpcChannelProviderTest#testLogDirectPathMisconfig_AttemptDirectPathNotSetAndAttemptDirectPathXdsSetViaEnv_warns + + + + + envVarTest + + + + org.apache.maven.plugins + maven-surefire-plugin + + InstantiatingGrpcChannelProviderTest#testLogDirectPathMisconfig_AttemptDirectPathNotSetAndAttemptDirectPathXdsSetViaEnv_warns + + + + + + \ No newline at end of file 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 91c38d916e..cb5c2fc63b 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 @@ -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,32 +276,36 @@ 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); + } + @InternalApi public boolean isDirectPathXdsEnabled() { // Method 1: Enable DirectPath xDS by option. - if (Boolean.TRUE.equals(attemptDirectPathXds)) { + if (isDirectPathXdsEnabledViaBuilderOption()) { 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(); } // 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."); + final String onlyOneDirectPathEnabledMessage = + "DirectPath is misconfigured. Please set both the " + + "attemptDirectPath and the attemptDirectPathXds options."; + if (isDirectPathEnabled()) { + // Case 1: does not enable DirectPath xDS + if (!isDirectPathXdsEnabled()) { + LOG.log(Level.WARNING, onlyOneDirectPathEnabledMessage); } else { // Case 2: credential is not correctly set if (!isCredentialDirectPathCompatible()) { @@ -315,6 +322,15 @@ private void logDirectPathMisconfig() { "DirectPath is misconfigured. DirectPath is only available in a GCE environment."); } } + } else if (isDirectPathXdsEnabledViaBuilderOption()) { + LOG.log(Level.WARNING, onlyOneDirectPathEnabledMessage); + } else if (isDirectPathXdsEnabledViaEnv()) { + LOG.log( + Level.WARNING, + "Env var " + + DIRECT_PATH_ENV_ENABLE_XDS + + " was found. If this is intended for " + + "this client, please note that this is a misconfiguration and set the attemptDirectPath option as well."); } } 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 b789224d95..edc613fe67 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 @@ -594,28 +594,65 @@ 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. Please set both the attemptDirectPath and the 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. 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 + void testLogDirectPathMisconfig_AttemptDirectPathSetAndAttemptDirectPathXdsNotSet_warns() + throws Exception { + FakeLogHandler logHandler = new FakeLogHandler(); + InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler); + InstantiatingGrpcChannelProvider provider = + createChannelProviderBuilderForDirectPathLogTests().setAttemptDirectPath(true).build(); + createAndCloseTransportChannel(provider); + assertThat(logHandler.getAllMessages()) + .contains( + "DirectPath is misconfigured. Please set both the attemptDirectPath and the " + + "attemptDirectPathXds options."); + InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler); } @Test From 00939a8ffe3cca9c590341bcc2831674bc5d30ad Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Mon, 8 Jul 2024 17:06:28 -0400 Subject: [PATCH 02/15] add env var to other test types --- .github/workflows/ci.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 3bbf5560fd..c4ab01f5bd 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -81,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: | @@ -131,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" From 2c509d4569947116a809580bcf63b661245aa742 Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Wed, 10 Jul 2024 09:42:12 -0400 Subject: [PATCH 03/15] adjust warn logic to only when Direct Path xDS is set --- .../InstantiatingGrpcChannelProvider.java | 38 ++++++++++--------- .../InstantiatingGrpcChannelProviderTest.java | 15 -------- 2 files changed, 20 insertions(+), 33 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 cb5c2fc63b..9eb5481c65 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 @@ -299,15 +299,26 @@ public boolean isDirectPathXdsEnabled() { // builder or createSingleChannel, only in getTransportChannel which creates the first channel // for a client. private void logDirectPathMisconfig() { - final String onlyOneDirectPathEnabledMessage = - "DirectPath is misconfigured. Please set both the " - + "attemptDirectPath and the attemptDirectPathXds options."; - if (isDirectPathEnabled()) { - // Case 1: does not enable DirectPath xDS - if (!isDirectPathXdsEnabled()) { - LOG.log(Level.WARNING, onlyOneDirectPathEnabledMessage); + if (isDirectPathXdsEnabled()) { + // Case 1: Direct Path is only enabled via XDS env var. We will _warn_ the user that this is a + // misconfig if they + // intended to set the env var. + if (!isDirectPathEnabled() && isDirectPathXdsEnabledViaEnv()) { + LOG.log( + Level.WARNING, + "Env var " + + DIRECT_PATH_ENV_ENABLE_XDS + + " was found. 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 TD must be set along with xDS. Here we warn the user about this. + else if (!isDirectPathEnabled()) { + LOG.log( + Level.WARNING, + "DirectPath is misconfigured. Please set both the " + + "attemptDirectPath and the attemptDirectPathXds options."); } else { - // Case 2: credential is not correctly set + // Case 3: credential is not correctly set if (!isCredentialDirectPathCompatible()) { LOG.log( Level.WARNING, @@ -315,22 +326,13 @@ private void logDirectPathMisconfig() { + ComputeEngineCredentials.class.getName() + " ."); } - // Case 3: not running on GCE + // Case 4: not running on GCE if (!isOnComputeEngine()) { LOG.log( Level.WARNING, "DirectPath is misconfigured. DirectPath is only available in a GCE environment."); } } - } else if (isDirectPathXdsEnabledViaBuilderOption()) { - LOG.log(Level.WARNING, onlyOneDirectPathEnabledMessage); - } else if (isDirectPathXdsEnabledViaEnv()) { - LOG.log( - Level.WARNING, - "Env var " - + DIRECT_PATH_ENV_ENABLE_XDS - + " was found. If this is intended for " - + "this client, please note that this is a misconfiguration and set the attemptDirectPath option as well."); } } 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 edc613fe67..3b3c41a42e 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 @@ -640,21 +640,6 @@ void testLogDirectPathMisconfig_AttemptDirectPathNotSetAndAttemptDirectPathXdsSe InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler); } - @Test - void testLogDirectPathMisconfig_AttemptDirectPathSetAndAttemptDirectPathXdsNotSet_warns() - throws Exception { - FakeLogHandler logHandler = new FakeLogHandler(); - InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler); - InstantiatingGrpcChannelProvider provider = - createChannelProviderBuilderForDirectPathLogTests().setAttemptDirectPath(true).build(); - createAndCloseTransportChannel(provider); - assertThat(logHandler.getAllMessages()) - .contains( - "DirectPath is misconfigured. Please set both the attemptDirectPath and the " - + "attemptDirectPathXds options."); - InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler); - } - @Test void testLogDirectPathMisconfig_shouldNotLogInTheBuilder() { FakeLogHandler logHandler = new FakeLogHandler(); From 23ebc1c2a7f19d61132dee17c2e14cf56e107f06 Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Wed, 10 Jul 2024 09:45:20 -0400 Subject: [PATCH 04/15] restore contents of original warning --- .../google/api/gax/grpc/InstantiatingGrpcChannelProvider.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 9eb5481c65..58dd60b5e8 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 @@ -315,8 +315,8 @@ private void logDirectPathMisconfig() { else if (!isDirectPathEnabled()) { LOG.log( Level.WARNING, - "DirectPath is misconfigured. Please set both the " - + "attemptDirectPath and the attemptDirectPathXds options."); + "DirectPath is misconfigured. Please set the attemptDirectPath option along with the" + + " attemptDirectPathXds option."); } else { // Case 3: credential is not correctly set if (!isCredentialDirectPathCompatible()) { From 14453c5fb0f4ff6f11b3c47b4a3a5055eb79230e Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Wed, 10 Jul 2024 10:23:47 -0400 Subject: [PATCH 05/15] adjust test --- .../api/gax/grpc/InstantiatingGrpcChannelProviderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3b3c41a42e..2db86576ac 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 @@ -620,7 +620,7 @@ private void createAndCloseTransportChannel(InstantiatingGrpcChannelProvider pro createAndCloseTransportChannel(provider); assertThat(logHandler.getAllMessages()) .contains( - "DirectPath is misconfigured. Please set both the attemptDirectPath and the attemptDirectPathXds options."); + "DirectPath is misconfigured. Please set the attemptDirectPath option along with the attemptDirectPathXds option."); InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler); } From 0224baa5d94a8690efb79ce90141fd1f31d663ab Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Fri, 12 Jul 2024 16:04:11 -0400 Subject: [PATCH 06/15] improve readability --- .../InstantiatingGrpcChannelProvider.java | 44 +++++++++---------- 1 file changed, 21 insertions(+), 23 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 58dd60b5e8..4720c7dba7 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 @@ -287,12 +287,7 @@ private boolean isDirectPathXdsEnabledViaEnv() { @InternalApi public boolean isDirectPathXdsEnabled() { - // Method 1: Enable DirectPath xDS by option. - if (isDirectPathXdsEnabledViaBuilderOption()) { - return true; - } - // Method 2: Enable DirectPath xDS by env. - return isDirectPathXdsEnabledViaEnv(); + return isDirectPathXdsEnabledViaEnv() || isDirectPathXdsEnabledViaBuilderOption(); } // This method should be called once per client initialization, hence can not be called in the @@ -300,23 +295,26 @@ public boolean isDirectPathXdsEnabled() { // for a client. private void logDirectPathMisconfig() { if (isDirectPathXdsEnabled()) { - // Case 1: Direct Path is only enabled via XDS env var. We will _warn_ the user that this is a - // misconfig if they - // intended to set the env var. - if (!isDirectPathEnabled() && isDirectPathXdsEnabledViaEnv()) { - LOG.log( - Level.WARNING, - "Env var " - + DIRECT_PATH_ENV_ENABLE_XDS - + " was found. 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 TD must be set along with xDS. Here we warn the user about this. - else if (!isDirectPathEnabled()) { - LOG.log( - Level.WARNING, - "DirectPath is misconfigured. Please set the attemptDirectPath option along with the" - + " attemptDirectPathXds option."); + if (!isDirectPathEnabled()) { + // 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. 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 TD must be set along with xDS. Here we warn the user about this. + else if (isDirectPathXdsEnabledViaBuilderOption()) { + LOG.log( + Level.WARNING, + "DirectPath is misconfigured. Please set the attemptDirectPath option along with the" + + " attemptDirectPathXds option."); + } else { + } // impossible } else { // Case 3: credential is not correctly set if (!isCredentialDirectPathCompatible()) { From 4ec21a8a88161245a1aac733ff44d78173b79144 Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Wed, 17 Jul 2024 13:09:47 -0400 Subject: [PATCH 07/15] reformat comments --- .../api/gax/grpc/InstantiatingGrpcChannelProvider.java | 5 ++--- 1 file changed, 2 insertions(+), 3 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 4720c7dba7..9f9b181239 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 @@ -297,8 +297,7 @@ private void logDirectPathMisconfig() { if (isDirectPathXdsEnabled()) { if (!isDirectPathEnabled()) { // 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. + // a misconfiguration if they intended to set the env var. if (isDirectPathXdsEnabledViaEnv()) { LOG.log( Level.WARNING, @@ -314,7 +313,7 @@ else if (isDirectPathXdsEnabledViaBuilderOption()) { "DirectPath is misconfigured. Please set the attemptDirectPath option along with the" + " attemptDirectPathXds option."); } else { - } // impossible + } // impossible - added this `else` for readability. } else { // Case 3: credential is not correctly set if (!isCredentialDirectPathCompatible()) { From c71d229569c43ef5415eb500ae34d730a4c7c591 Mon Sep 17 00:00:00 2001 From: Diego Marquez Date: Thu, 25 Jul 2024 10:13:11 -0400 Subject: [PATCH 08/15] Update gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Co-authored-by: Lawrence Qiu --- .../google/api/gax/grpc/InstantiatingGrpcChannelProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 9f9b181239..543428a717 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 @@ -303,7 +303,7 @@ private void logDirectPathMisconfig() { Level.WARNING, "Env var " + DIRECT_PATH_ENV_ENABLE_XDS - + " was found. If this is intended for " + + " 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 TD must be set along with xDS. Here we warn the user about this. From 4fede21efc25fecbfe304b5ed3d32bf79ca700c9 Mon Sep 17 00:00:00 2001 From: Diego Marquez Date: Thu, 25 Jul 2024 10:13:41 -0400 Subject: [PATCH 09/15] Update gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Co-authored-by: Lawrence Qiu --- .../google/api/gax/grpc/InstantiatingGrpcChannelProvider.java | 3 +-- 1 file changed, 1 insertion(+), 2 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 543428a717..7d829681c1 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 @@ -310,8 +310,7 @@ private void logDirectPathMisconfig() { else if (isDirectPathXdsEnabledViaBuilderOption()) { LOG.log( Level.WARNING, - "DirectPath is misconfigured. Please set the attemptDirectPath option along with the" - + " attemptDirectPathXds option."); + "DirectPath is misconfigured. The DirectPath XDS option was set, but the attemptDirectPath option was not. Please set both the attemptDirectPath and attemptDirectPathXds options."); } else { } // impossible - added this `else` for readability. } else { From ca723e82af568091041920298b39c74d3abdd68b Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Thu, 25 Jul 2024 11:05:31 -0400 Subject: [PATCH 10/15] improve comments and warning content --- .../api/gax/grpc/InstantiatingGrpcChannelProvider.java | 8 +++++--- .../gax/grpc/InstantiatingGrpcChannelProviderTest.java | 4 ++-- 2 files changed, 7 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 7d829681c1..be2cfe249c 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 @@ -296,6 +296,7 @@ public boolean isDirectPathXdsEnabled() { private void logDirectPathMisconfig() { if (isDirectPathXdsEnabled()) { if (!isDirectPathEnabled()) { + // Only two cases are possible: Direct path xDS enabled via env var 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()) { @@ -306,13 +307,14 @@ private void logDirectPathMisconfig() { + " 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 TD must be set along with xDS. Here we warn the user about this. + // Case 2: Direct Path xDS was enabled via Builder. Direct Path TD (via gRPCLB) must be set + // 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 { - } // impossible - added this `else` for readability. + } } else { // Case 3: credential is not correctly set if (!isCredentialDirectPathCompatible()) { 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 2db86576ac..fa06c0d057 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 @@ -635,8 +635,8 @@ void testLogDirectPathMisconfig_AttemptDirectPathNotSetAndAttemptDirectPathXdsSe createAndCloseTransportChannel(provider); assertThat(logHandler.getAllMessages()) .contains( - "Env var GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS was found. If this is intended for this client, please note " - + "that this is a misconfiguration and set the attemptDirectPath option as well."); + "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); } From cb5c240329feb2f2e6260790b14c62d2c8a100f3 Mon Sep 17 00:00:00 2001 From: Diego Marquez Date: Fri, 26 Jul 2024 08:29:21 -0400 Subject: [PATCH 11/15] Update gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Co-authored-by: Lawrence Qiu --- .../api/gax/grpc/InstantiatingGrpcChannelProvider.java | 5 +++-- 1 file changed, 3 insertions(+), 2 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 be2cfe249c..dfd5bd0766 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 @@ -296,8 +296,9 @@ public boolean isDirectPathXdsEnabled() { private void logDirectPathMisconfig() { if (isDirectPathXdsEnabled()) { if (!isDirectPathEnabled()) { - // Only two cases are possible: Direct path xDS enabled via env var or via builder. - // Case 1: Direct Path is only enabled via XDS env var. We will _warn_ the user that this is + // 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( From c7840f82ede18a15e8dc7a6d8e15b5b64f3a0482 Mon Sep 17 00:00:00 2001 From: Diego Marquez Date: Fri, 26 Jul 2024 08:33:01 -0400 Subject: [PATCH 12/15] Update gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Co-authored-by: Lawrence Qiu --- .../google/api/gax/grpc/InstantiatingGrpcChannelProvider.java | 3 +-- 1 file changed, 1 insertion(+), 2 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 dfd5bd0766..cec1d37227 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 @@ -308,8 +308,7 @@ private void logDirectPathMisconfig() { + " 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 TD (via gRPCLB) must be set - // along with xDS. + // 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( From b33009ffdae30b327a8b1123271dab17894e99c4 Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Fri, 26 Jul 2024 08:34:42 -0400 Subject: [PATCH 13/15] reformat --- .../google/api/gax/grpc/InstantiatingGrpcChannelProvider.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 cec1d37227..8a253792ea 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 @@ -308,7 +308,8 @@ private void logDirectPathMisconfig() { + " 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. + // 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( From d14e50c525bc132a1d62f66b0a9baa7d1600d111 Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Fri, 26 Jul 2024 08:40:10 -0400 Subject: [PATCH 14/15] add comment to isDirectPathXdsEnabled() --- .../api/gax/grpc/InstantiatingGrpcChannelProvider.java | 7 +++++++ 1 file changed, 7 insertions(+) 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 8a253792ea..c49d5265f7 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 @@ -285,6 +285,13 @@ private boolean isDirectPathXdsEnabledViaEnv() { 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() { return isDirectPathXdsEnabledViaEnv() || isDirectPathXdsEnabledViaBuilderOption(); From 26b81e1e1857a377c995a97fc19b9eca797257f7 Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Fri, 26 Jul 2024 08:48:27 -0400 Subject: [PATCH 15/15] fix test --- .../api/gax/grpc/InstantiatingGrpcChannelProviderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 fa06c0d057..4de1b8ee11 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 @@ -620,7 +620,7 @@ private void createAndCloseTransportChannel(InstantiatingGrpcChannelProvider pro createAndCloseTransportChannel(provider); assertThat(logHandler.getAllMessages()) .contains( - "DirectPath is misconfigured. Please set the attemptDirectPath option along with the attemptDirectPathXds option."); + "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); }