fix: DirectPath non-default SA requires creds

[mohanli-ml]

Spanner tries to set the allowNonDefaultServiceAccount option in its client library, which makes some tests fail. In these tests, client and server are running on the same machine, and no credentials are provided. DirectPath is not supposed to be tested by these tests, so we add a requirement that if the client wants to use non-default service account for DirectPath, the credential associated with the service account must be provided.

[blakeli0]

@mohanli-ml Can you please create an issue for tracking purpose?

[blakeli0]

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?

[mohanli-ml]

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

[olavloite]

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).

[blakeli0]

@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?

[mohanli-ml]

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?

[olavloite]

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.

[blakeli0]

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.

[mohanli-ml]

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?

[mohanli-ml]

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

[blakeli0]

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.

[blakeli0]

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.

[mohanli-ml]

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.

[olavloite]

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.

[mohanli-ml]

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

[blakeli0]

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.

[sonarqubecloud]

Quality Gate passed for 'gapic-generator-java-root'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
80.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[sonarqubecloud]

Quality Gate passed for 'java_showcase_integration_tests'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud