Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: log DirectPath misconfiguration #2105

Merged
merged 9 commits into from
Oct 18, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@
import java.util.concurrent.Executor;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nullable;
import javax.net.ssl.KeyManagerFactory;
import org.threeten.bp.Duration;
Expand All @@ -82,6 +84,9 @@
*/
@InternalExtensionOnly
public final class InstantiatingGrpcChannelProvider implements TransportChannelProvider {
@VisibleForTesting
static final Logger LOG = Logger.getLogger(InstantiatingGrpcChannelProvider.class.getName());

private 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";
Expand Down Expand Up @@ -140,6 +145,7 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
builder.directPathServiceConfig == null
? getDefaultDirectPathServiceConfig()
: builder.directPathServiceConfig;
logDirectPathMisconfig();
}

/**
Expand Down Expand Up @@ -266,6 +272,33 @@ boolean isDirectPathXdsEnabled() {
return false;
}

private void logDirectPathMisconfig() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this logic is added specifically for log statements. How do we make sure it is in parity with directPath decision making?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to the bit that actually handles connecting to DirectPath in gRPC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DirectPath decision making is L363:

if (isDirectPathEnabled() && isNonDefaultServiceAccountAllowed() && isOnComputeEngine()) {

And I came up with the 4 cases based on it. WDYT?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant was that this logic will be invoked only when directPath is misconfigured. But this is not the logic which decides if directPath is misconfigured. This code snippet could become stale if there is change in logic which is actually deciding when directpath is misconfigured.

Just as an e.g. this is the code snippet which decides directPath misconfiguration

  if (isDirectPathEnabled() && isNonDefaultServiceAccountAllowed() && isOnComputeEngine()) {

Here we check for isNonDefaultServiceAccountAllowed. Hypothetically in future if isNonDefaultServiceAccountAllowed check is removed there is no easy way to know that fun:logDirectPathMisconfig needs to be updated as well.

I guess there is no easy way to enforce it and this is fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your inputs! isNonDefaultServiceAccountAllowed() and isOnComputeEngine() should be kept even after DirectPath rolls out, and then we will need to cleanup env/options, and this warning logs should also be cleaned.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same concern as @singhravidutt, the logic in logDirectPathMisconfig() could evolve to be different than (isDirectPathEnabled() && isNonDefaultServiceAccountAllowed() && isOnComputeEngine()). Ideally, we should check if we should use DirectPath or not during the initialization of this class, set the value to a field useDirectPath, and use it to check if we should create a DirectPath channel afterwards.
See the following code that demonstrates my idea:

public InstantiatingGrpcChannelProvider build() {
      if (shouldUseDirectPath()) {
          setUseDirectPath(true);
      }
      return new InstantiatingGrpcChannelProvider(this);
}

  private boolean shouldUseDirectPath() {
    if (isDirectPathEnabled()) {
      return isOnComputeWithValidCredential();
    } else {
      if (isDirectPathXdsEnabled()) {
        //log
        isOnComputeWithValidCredential();
      }
    }
    return false;
  }

  private boolean isOnComputeWithValidCredential() {
    if (!isNonDefaultServiceAccountAllowed()) {
      //log
      return false;
    }
    if (!isOnComputeEngine()) {
      //log
      return false;
    }
    return true;
  }

This also makes the new code much more testable as SonarCheck is failing due to lacking test coverage of the new code.

@mohanli-ml Let me know if it makes sense, if you think it might be too much at this moment, our team can take it as a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! Moving the DirectPath enabling logic to the builder instead of checking it every time when construct a channel sounds good to me. However, pay attention to the logic here:

private boolean shouldUseDirectPath() {
    if (isDirectPathEnabled()) {       <- This should be `if (isDirectPathEnabled() && !isDirectPathXdsEnabled())`
      return isOnComputeWithValidCredential();
    } else {
      if (isDirectPathXdsEnabled()) {
        //log
        isOnComputeWithValidCredential();
      }
    }
    return false;
  }

In short I feel the logic is already complex and I would suggest to do this with the cleanup of removing DirectPath gRPCLB, which is blocked by CBT and is expected to be done in Q4.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In short I feel the logic is already complex and I would suggest to do this with the cleanup of removing DirectPath gRPCLB, which is blocked by CBT and is expected to be done in Q4.

I think that's reasonable. For now, can you please add some unit tests for these logging statement? So that the test coverage for new code meets our least standard and gives us more confidence when we refactor it. For example, one way to test it is to create a FakeLogger and assert the log message captured in the FakeLogger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!

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.");
} else {
// Case 2: credential is not correctly set
if (!isNonDefaultServiceAccountAllowed()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have a ComputeEngineCredentials but not on ComputeEngine? I though auth would only return ComputeEngineCredentials if on ComputeEngine, so isOnComputeEngine() may not be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is possible, this is the original fix for this problem, googleapis/gax-java#1250.

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
if (!isOnComputeEngine()) {
LOG.log(
Level.WARNING,
"DirectPath is misconfigured. DirectPath is only available in a GCE environment.");
}
}
}
}

private boolean isNonDefaultServiceAccountAllowed() {
if (allowNonDefaultServiceAccount != null && allowNonDefaultServiceAccount) {
return true;
Expand All @@ -275,6 +308,7 @@ private boolean isNonDefaultServiceAccountAllowed() {

// DirectPath should only be used on Compute Engine.
// Notice Windows is supported for now.
@VisibleForTesting
static boolean isOnComputeEngine() {
String osName = System.getProperty("os.name");
if ("Linux".equals(osName)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@
import java.util.concurrent.Executor;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.logging.Handler;
import java.util.logging.LogRecord;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -503,4 +506,70 @@ protected Object getMtlsObjectFromTransportChannel(MtlsProvider provider)
.build();
return channelProvider.createMtlsChannelCredentials();
}

@Test
public void testLogDirectPathMisconfigAttrempDirectPathNotSet() {
FakeLogHandler logHandler = new FakeLogHandler();
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder().setAttemptDirectPathXds().build();
assertThat(logHandler.getAllMessages())
.contains(
"DirectPath is misconfigured. Please set the attemptDirectPath option along with the"
+ " attemptDirectPathXds option.");
InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
}

@Test
public void testLogDirectPathMisconfigWrongCredential() {
FakeLogHandler logHandler = new FakeLogHandler();
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPathXds()
.setAttemptDirectPath(true)
.build();
assertThat(logHandler.getAllMessages())
.contains(
"DirectPath is misconfigured. Please make sure the credential is an instance of"
+ " com.google.auth.oauth2.ComputeEngineCredentials .");
InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
}

@Test
public void testLogDirectPathMisconfigNotOnGCE() {
FakeLogHandler logHandler = new FakeLogHandler();
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPathXds()
.setAttemptDirectPath(true)
.setAllowNonDefaultServiceAccount(true)
.build();
if (!InstantiatingGrpcChannelProvider.isOnComputeEngine()) {
assertThat(logHandler.getAllMessages())
.contains(
"DirectPath is misconfigured. DirectPath is only available in a GCE environment.");
}
InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
}

private static class FakeLogHandler extends Handler {
List<LogRecord> records = new ArrayList<>();

@Override
public void publish(LogRecord record) {
records.add(record);
}

@Override
public void flush() {}

@Override
public void close() throws SecurityException {}

List<String> getAllMessages() {
return records.stream().map(LogRecord::getMessage).collect(Collectors.toList());
}
}
}
Loading