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.

Sign up for GitHub

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

Jump to bottom

feat: log DirectPath misconfiguration #2105

Merged
merged 9 commits into from
Oct 18, 2023
Merged

feat: log DirectPath misconfiguration #2105

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f93bfb7
feat: log DirectPath misconfiguration
mohanli-ml Oct 11, 2023
6454a46
feat: log DirectPath misconfiguration
mohanli-ml Oct 12, 2023
ec9116e
feat: log DirectPath misconfiguration
mohanli-ml Oct 12, 2023
908a54d
Merge branch 'main' into dp-warn-logs
mohanli-ml Oct 16, 2023
752b4a3
feat: log DirectPath misconfiguration
mohanli-ml Oct 16, 2023
f764786
feat: log DirectPath misconfiguration
mohanli-ml Oct 17, 2023
1c5f4bb
feat: log DirectPath misconfiguration
mohanli-ml Oct 17, 2023
b3ce533
feat: log DirectPath misconfiguration
mohanli-ml Oct 17, 2023
cb685dc
feat: log DirectPath misconfiguration
mohanli-ml Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions ...java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
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,8 @@
*/
@InternalExtensionOnly
public final class InstantiatingGrpcChannelProvider implements TransportChannelProvider {
private 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 @@ -233,6 +237,7 @@ public TransportChannel getTransportChannel() throws IOException {
}

private TransportChannel createChannel() throws IOException {
logDirectPathMisconfig();
Copy link
Collaborator

Choose a reason for hiding this comment

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

createChannel() could still be called multiple times, I would suggest to log the info in the builder

Copy link
Contributor Author

@mohanli-ml mohanli-ml Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We can not do this because logDirectPathMisconfig() is a non-static function so it can not be used before the construction. Can you suggest more?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we move logDirectPathMisconfig() into the Builder class then it should be fine, ideally we should do it but then we would need to move all related logics into it as well. The quickest solution would be add it to the constructor.

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.

return GrpcTransportChannel.create(
ChannelPool.create(
channelPoolSettings, InstantiatingGrpcChannelProvider.this::createSingleChannel));
Expand Down Expand Up @@ -266,6 +271,33 @@ boolean isDirectPathXdsEnabled() {
return false;
}

private void logDirectPathMisconfig() {
Copy link

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?

Copy link

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!

boolean isDirectPathOptionSet = Boolean.TRUE.equals(attemptDirectPath);
boolean isDirectPathXdsEnvSet =
Boolean.parseBoolean(envProvider.getenv(DIRECT_PATH_ENV_ENABLE_XDS));
boolean isDirectPathXdsOptionSet = Boolean.TRUE.equals(attemptDirectPathXds);
if (isDirectPathXdsEnvSet || isDirectPathXdsOptionSet) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we already have isDirectPathXdsEnabled() for the same purpose?

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.

// Case 1: does not enable DirectPath
if (!isDirectPathOptionSet) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use isDirectPathEnabled()?

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.

LOG.log(
Level.WARNING, "DirectPath is misconfigured. Please set the attemptDirectPath option.");
Copy link
Member

Choose a reason for hiding this comment

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

@singhravidutt this PR is w.r.t R.1 for observability.

Would hadoop connector customers be able to see these warnings emitted when running the connector or would this require a change?

Copy link

Choose a reason for hiding this comment

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

Yes, we should be able to surface these logs.

Copy link

Choose a reason for hiding this comment

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

should we add values of isDirectPathOptionSet, isDirectPathXdsEnvSet and isDirectPathXdsOptionSet as part of log statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case user enables DirectPathXds either by env or option, but DirectPath itself is not enabled. I think the log is already clear that the user needs to set attemptDirectPath and there is no need to log the values for judging the case. WDYT?

Copy link
Member

@frankyn frankyn Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Recommend:

DirectPath is misconfigured. Please enable the attemptDirectPath option along with the attemptDirectPathXds option

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.

} 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.");
Copy link
Member

Choose a reason for hiding this comment

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

use fully qualified class name ComputeEngineCredentials.class.getName()

TIL you need GCE credentials for DirectPath; what is a customer is using Workload Identity would that work?

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.

GKE workload identity is compatible with DirectPath.

}
// Case 3: not running on GCE
if (!isOnComputeEngine()) {
LOG.log(
Level.WARNING, "DirectPath is misconfigured. Please run in the GCE environment. ");
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:

DirectPath is misconfigured. DirectPath is only available in a GCE environment.

You also have whitespace at the end.

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.

}
}
}
}

private boolean isNonDefaultServiceAccountAllowed() {
if (allowNonDefaultServiceAccount != null && allowNonDefaultServiceAccount) {
return true;
Expand Down
Loading