Skip to content

fix: DirectPath non-default SA requires creds #2281

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

Merged
merged 7 commits into from
Jan 8, 2024
Merged

fix: DirectPath non-default SA requires creds #2281

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b836def
fix: DirectPath non-default SA requires creds
mohanli-ml Dec 1, 2023
26cb584
fix: DirectPath non-default SA requires creds
mohanli-ml Dec 4, 2023
15db7e8
fix: DirectPath non-default SA requires creds
mohanli-ml Dec 4, 2023
1f389a9
Merge branch 'main' into dp-null-creds
mohanli-ml Dec 13, 2023
33d5b82
fix: DirectPath non-default SA requires creds
mohanli-ml Dec 13, 2023
5c0418b
fix: DirectPath non-default SA requires creds
mohanli-ml Dec 14, 2023
1b97839
fix: DirectPath non-default SA requires creds
mohanli-ml Jan 8, 2024
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
20 changes: 14 additions & 6 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 @@ -282,7 +282,7 @@ private void logDirectPathMisconfig() {
+ " attemptDirectPathXds option.");
} else {
// Case 2: credential is not correctly set
if (!isNonDefaultServiceAccountAllowed()) {
if (!isCredentialDirectPathCompatible()) {
LOG.log(
Level.WARNING,
"DirectPath is misconfigured. Please make sure the credential is an instance of "
Expand All @@ -299,8 +299,10 @@ private void logDirectPathMisconfig() {
}
}

private boolean isNonDefaultServiceAccountAllowed() {
@VisibleForTesting
boolean isCredentialDirectPathCompatible() {
if (allowNonDefaultServiceAccount != null && allowNonDefaultServiceAccount) {
// If non-default service account is allowed, a null call credential is OK.
return true;
}
return credentials instanceof ComputeEngineCredentials;
Expand Down Expand Up @@ -356,10 +358,16 @@ private ManagedChannel createSingleChannel() throws IOException {

// Check DirectPath traffic.
boolean useDirectPathXds = false;
if (isDirectPathEnabled() && isNonDefaultServiceAccountAllowed() && isOnComputeEngine()) {
CallCredentials callCreds = MoreCallCredentials.from(credentials);
ChannelCredentials channelCreds =
GoogleDefaultChannelCredentials.newBuilder().callCredentials(callCreds).build();
if (isDirectPathEnabled() && isCredentialDirectPathCompatible() && isOnComputeEngine()) {
ChannelCredentials channelCreds;
if (credentials == null) {
// GoogleDefaultChannelCredentials will use application default call credential.
channelCreds = GoogleDefaultChannelCredentials.newBuilder().build();
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 should skip the whole DirectPath feature if credentials is null like you originally proposed? This would still fail the emulator tests in Spanner if directPath is enabled.
For more context, credentials should never be null unless the CredentialProvider is explicitly overridden to a NoCreentialsProvider like this, which is probably what the Spanner tests did.

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 problem here is that if the use case mentioned by @olavloite is valid, i.e., if customers set .setCallCredentialsProvider(...).setCredentials(NoCredentials.getInstance()), should we support it? My understanding is yes, but I do not know who should be the approver.

About the spanner emulator test failure, I think the Spanner test should understand the environment and fix it in their repo.

Copy link

Choose a reason for hiding this comment

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

About the spanner emulator test failure, I think the Spanner test should understand the environment and fix it in their repo.

Yes, we will take care of that (it's actually not the emulator tests, but some tests using a MockSpannerService implementation).

I think we should skip the whole DirectPath feature if credentials is null like you originally proposed?

That sounds reasonable to me. I think that it would be reasonable to instruct users who want to use DirectPath in combination with CallCredentials per RPC, that they have to supply a default set of credentials as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I reverted the change back. PTAL. @blakeli0 @olavloite

} else {
CallCredentials callCreds = MoreCallCredentials.from(credentials);
channelCreds =
GoogleDefaultChannelCredentials.newBuilder().callCredentials(callCreds).build();
}
useDirectPathXds = isDirectPathXdsEnabled();
if (useDirectPathXds) {
// google-c2p: CloudToProd(C2P) Directpath. This scheme is defined in
Expand Down
8 changes: 8 additions & 0 deletions .../gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,14 @@ public void testDirectPathXdsDisableByDefault() throws IOException {
assertThat(provider.isDirectPathXdsEnabled()).isFalse();
}

@Test
public void testDirectPathDisallowNullCredentials() throws IOException {
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder().build();

assertThat(provider.isCredentialDirectPathCompatible()).isFalse();
}

@Test
public void testDirectPathXdsEnabled() throws IOException {
InstantiatingGrpcChannelProvider provider =
Expand Down