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 all 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
11 changes: 8 additions & 3 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 @@ -303,7 +303,12 @@ private void logDirectPathMisconfig() {
}
}

private boolean isNonDefaultServiceAccountAllowed() {
@VisibleForTesting
boolean isCredentialDirectPathCompatible() {
// DirectPath requires a call credential during gRPC channel construction.
if (needsCredentials()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the whole DirectPath feature is not supposed to be enabled in a local testing environment, can we move this check to an earlier place? For example, in isDirectPathXdsEnabled()? Also can we add a unit test for this scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the method isNonDefaultServiceAccountAllowed() to isCredentialDirectPathCompatible(), because DirectPath requires a credential no matter if non-default service account is allowed or not. PTAL.

Copy link

Choose a reason for hiding this comment

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

While calling this method will in most cases give you the correct result, it is not really intended to detect whether you are in a test situation or not. My understanding is that needsCredentials() is intended to indicate whether CallCredentials need to be supplied in the call context, instead of having been set to a static value at channel creation.

Would it instead be possible to do the detection for 'are we in a test or not?' based on whether the channel uses TLS or not? Local emulators and mock servers (normally) do not use this.

Further context:

There is a corner case possible in the Cloud Spanner client that would have needsCredentials return true, even though we are in a production situation using actual service account credentials.

import com.google.auth.oauth2.GoogleCredentials;
import com.google.cloud.NoCredentials;
import com.google.cloud.spanner.DatabaseClient;
import com.google.cloud.spanner.DatabaseId;
import com.google.cloud.spanner.ResultSet;
import com.google.cloud.spanner.Spanner;
import com.google.cloud.spanner.SpannerOptions;
import com.google.cloud.spanner.Statement;
import io.grpc.auth.MoreCallCredentials;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;

public class CredentialsProviderExample {
  
  public static void main(String[] args) {
    Spanner spanner = SpannerOptions.newBuilder()
        .setCallCredentialsProvider(() -> {
          try {
            return MoreCallCredentials.from(GoogleCredentials.fromStream(Files.newInputStream(
                new File("/home/loite/appdev-soda-spanner-staging.json").toPath())));
          } catch (IOException e) {
            throw new RuntimeException(e);
          }
        })
        // Set NoCredentials to prevent the libraries from picking up the application default credentials.
        .setCredentials(NoCredentials.getInstance())
        .setProjectId("appdev-soda-spanner-staging")
        .build().getService();
    DatabaseClient client = spanner.getDatabaseClient(
        DatabaseId.of("appdev-soda-spanner-staging", "knut-test-ycsb", "knut-test-db"));
    try (ResultSet resultSet = client.singleUse().executeQuery(Statement.of("select 1"))) {
      while (resultSet.next()) {
        System.out.println(resultSet.getLong(0));
      }
    }
  }

}

The above example shows how you (in theory) could set a CallCredentialsProvider for the Spanner client. This will request dynamic credentials for each RPC invocation instead of using a fixed set of credentials that are supplied at channel creation. (The above example however returns the same credentials for each RPC invocation, but this feature could be used to use different credentials for different requests).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@olavloite needsCredentials() in Gax was originally used for ClientContext to decide do we need to supply a Credentials object to the TransportChannelProvider, see relevant code here. Theoretically all gRPC calls should get Credentials from ClientContext, but looks like Spanner did some customization to override it to CallCredentials in GrpcCallContext before every new call, as I see here. This would definitely make @mohanli-ml's solution not covering all cases as you suggested above.

Regarding the possible solutions, I'm not sure we want to add a way to detect emulator use case in Gax, especially at the cost of breaking certain features. To me this seems the responsibility of each service, as we don't have many services supporting emulators and each of them also implemented differently.
Since this PR was originally attempted to fix a test, can we change the test set up against the emulator? To not enable direct path? Or we want to have the exact same client settings between emulator testing and a real integration test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me there are two issues:

  1. DirectPath requires a call credential, and if it is null crash will happen, which is what we saw in Spanner tests. This is a bug, though it should rarely happen unless customers explicitly set the call credentials to null as the default credentials are application default credentials. We should add the check to make sure the call credential is not null, which should be covered by this PR.
  2. Differentiate Spanner tests should use DirectPath or not. Setting a call credential in a Spanner test does not mean the test wants to use DirectPath, and we need to further detect it. For this I think Blake's idea seems make sense to me that gax should not be responsible for detecting if the code is running a test env or not. Can Spanner change its test setting?

Copy link

Choose a reason for hiding this comment

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

  1. We have chore: allow non-default service account for DP java-spanner#2635 ready that we can submit to fix the test failures in the Spanner client. That PR disables DirectPath explicitly for the tests that are using an in-memory mock Spanner gRPC server. That will fix the failures independently of whether this fix is submitted or not. The fix in this PR would also fix almost all of the current test failures in the Spanner client, except for a test that verifies that using a CallCredentialsProvider works.
  2. The customization in the Spanner client for potentially using CallCredentials is something that any client could do. Any client could create a GrpcCallContext and call GrpcCallContext#withCallCredentials to use different credentials per RPC, instead of setting up the credentials at/before gRPC channel creation. The customization in the Spanner client boils down to that we have surfaced this option to the public API of the Spanner client so customers can use it. (I don't believe it is used much)

My suggestion for the way forward would be:

  1. We'll submit the fix in the Spanner client so we can proceed with the rollout of DirectPath for Spanner.
  2. We should also submit this fix. As mentioned above, it will fix most of the failing tests in the Spanner client, and also fixes a potential error/bug in Gax. Once the update has bubbled into the Spanner client, we could potentially remove some of the custom 'turn off DirectPath test configuration' in the Spanner client (but it doesn't hurt if it stays either).

I don't really know if there is a possible solution for the problem described above for anyone not setting any default credentials, but instead is supplying credentials on a per-RPC basis in the GrpcCallContext. For now, it would mean that DirectPath would be skipped.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really know if there is a possible solution for the problem described above for anyone not setting any default credentials, but instead is supplying credentials on a per-RPC basis in the GrpcCallContext.

The main problem I see is that all the DirectPath logic happens in Gax during channel creation, but Gax is not aware of the customization in Spanner, where we could get a dynamic Credentials on every call. So I agree that it's not possible to fix it in Gax right now, maybe we can fix it in Spanner by setting Credentials along side setting call credentials. Based on your example above, it could be

        Credentials credentials = GoogleCredentials.fromStream(Files.newInputStream(
            new File("/home/loite/appdev-soda-spanner-staging.json").toPath()));
        Spanner spanner = SpannerOptions.newBuilder()
            .setCallCredentialsProvider(() -> MoreCallCredentials.from(credentials))
            .setCredentials(credentials)
            .setProjectId("appdev-soda-spanner-staging")
            .build().getService();

but there is no guarantee that customers would do it and it would not work if CallCredentials can be created from something other than GoogleCredentials.

Separately, I'm not sure if this use case is DirectPath compatible, as all the DirectPath logic is during channel creation, if the call credential is different than what was used for channel creation, would DirectPath continue to work? @mohanli-ml

To move forward, if both of you agree that DirectPath can be skipped for this use case, then this PR could be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, if we could not extract a call credential from credentials, GoogleDefaultChannelCredentials will use the application default for the call credential. So there will be a call credential during channel creation. Are you saying the dynamic call credential will override the static call credential?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No matter override or not (though I am still curious), given GoogleDefaultChannelCredentials will use the application default for the call credential, I think the problem is not whether allow a null call credential when constructing a DirectPath channel, instead the problem is just to avoid the crash if the call credential is null. Please take another look. @blakeli0 @olavloite

Copy link
Collaborator

Choose a reason for hiding this comment

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

instead the problem is just to avoid the crash if the call credential is null

I agree, but I think we should avoid it by skipping DirectPath like you originally had, not recreating the credentials.

return false;
}
if (allowNonDefaultServiceAccount != null && allowNonDefaultServiceAccount) {
return true;
}
Expand Down Expand Up @@ -365,7 +370,7 @@ private ManagedChannel createSingleChannel() throws IOException {
// Check DirectPath traffic.
boolean useDirectPathXds = false;
if (isDirectPathEnabled()
&& isNonDefaultServiceAccountAllowed()
&& isCredentialDirectPathCompatible()
&& isOnComputeEngine()
&& canUseDirectPathWithUniverseDomain()) {
CallCredentials callCreds = MoreCallCredentials.from(credentials);
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