Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

feat: add allowNonDefaultServiceAccount option for DirectPath #1433

Merged
merged 3 commits into from Aug 5, 2021
Merged

feat: add allowNonDefaultServiceAccount option for DirectPath #1433

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db321f2
feat: add allowNonDefaultServiceAccount option for DirectPath
mohanli-ml Jul 23, 2021
d965d51
Merge branch 'master' into file-credential
mohanli-ml Aug 5, 2021
41c56d8
feat: add allowNonDefaultServiceAccount option for DirectPath
mohanli-ml Aug 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 19 additions & 1 deletion gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Expand Up @@ -105,6 +105,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
@Nullable private final Credentials credentials;
@Nullable private final ChannelPrimer channelPrimer;
@Nullable private final Boolean attemptDirectPath;
@Nullable private final Boolean allowNonDefaultServiceAccount;
@VisibleForTesting final ImmutableMap<String, ?> directPathServiceConfig;
@Nullable private final MtlsProvider mtlsProvider;

Expand All @@ -129,6 +130,7 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
this.credentials = builder.credentials;
this.channelPrimer = builder.channelPrimer;
this.attemptDirectPath = builder.attemptDirectPath;
this.allowNonDefaultServiceAccount = builder.allowNonDefaultServiceAccount;
this.directPathServiceConfig =
builder.directPathServiceConfig == null
? getDefaultDirectPathServiceConfig()
Expand Down Expand Up @@ -271,6 +273,13 @@ private boolean isDirectPathEnabled(String serviceAddress) {
return false;
}

private boolean isNonDefaultServiceAccountAllowed() {
if (allowNonDefaultServiceAccount != null) {
return allowNonDefaultServiceAccount;
}
return credentials instanceof ComputeEngineCredentials;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? I see there is already the isOnComputeEngine() method, so ideally we want to keep compute-engine specific check in one place. Also, this method (isNonDefaultServiceAccountAllowed()) is used only in conjunction with isOnComputeEngine() on line 324 of this file).

Copy link
Contributor Author

@mohanli-ml mohanli-ml Aug 3, 2021 •
edited

Choose a reason for hiding this comment

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

Sorry the name might be misleading:

  1. isOnComputeEngine() checks the environment of the VM as DirectPath is allowed on Compute Engine/GKE but not App Engine.

  2. isNonDefaultServiceAccountAllowed() checks which service account the VM is using to access DirectPath. credential is instance of ComputeEngineCredentials guarantees that the VM is using the default service account (because the credential is retrieved from the metadata server), but it can not guarantee the environment (i.e., ComputeEngineCredentials can also be used on App Engine).

Based on the above understanding I think the two checks are different and should be kept.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

}

// DirectPath should only be used on Compute Engine.
// Notice Windows is supported for now.
static boolean isOnComputeEngine() {
Expand Down Expand Up @@ -320,7 +329,7 @@ private ManagedChannel createSingleChannel() throws IOException, GeneralSecurity

// TODO(weiranf): Add API in ComputeEngineCredentials to check default service account.
if (isDirectPathEnabled(serviceAddress)
&& credentials instanceof ComputeEngineCredentials
&& isNonDefaultServiceAccountAllowed()
&& isOnComputeEngine()) {
builder = ComputeEngineChannelBuilder.forAddress(serviceAddress, port);
// Set default keepAliveTime and keepAliveTimeout when directpath environment is enabled.
Expand Down Expand Up @@ -432,6 +441,7 @@ public static final class Builder {
@Nullable private Credentials credentials;
@Nullable private ChannelPrimer channelPrimer;
@Nullable private Boolean attemptDirectPath;
@Nullable private Boolean allowNonDefaultServiceAccount;
@Nullable private ImmutableMap<String, ?> directPathServiceConfig;

private Builder() {
Expand All @@ -456,6 +466,7 @@ private Builder(InstantiatingGrpcChannelProvider provider) {
this.credentials = provider.credentials;
this.channelPrimer = provider.channelPrimer;
this.attemptDirectPath = provider.attemptDirectPath;
this.allowNonDefaultServiceAccount = provider.allowNonDefaultServiceAccount;
this.directPathServiceConfig = provider.directPathServiceConfig;
this.mtlsProvider = provider.mtlsProvider;
}
Expand Down Expand Up @@ -651,6 +662,13 @@ public Builder setAttemptDirectPath(boolean attemptDirectPath) {
return this;
}

/** Whether allow non-default service account for DirectPath. */
@InternalApi("For internal use by google-cloud-java clients only")
public Builder setAllowNonDefaultServiceAccount(boolean allowNonDefaultServiceAccount) {
this.allowNonDefaultServiceAccount = allowNonDefaultServiceAccount;
return this;
}

/**
* Sets a service config for direct path. If direct path is not enabled, the provided service
* config will be ignored.
Expand Down