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

chore: allow non-default service account for DP #2635

Merged
merged 8 commits into from
Dec 20, 2023
Merged

chore: allow non-default service account for DP #2635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
96f3f9b
chore: allow non-default service account for DP
olavloite Sep 19, 2023
eafbd4c
Merge branch 'main' into allow-non-default-service-account
olavloite Oct 24, 2023
6b7c9aa
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Oct 24, 2023
47c9ea1
fix: only try DP with credentials
olavloite Oct 24, 2023
608cf1b
Merge branch 'allow-non-default-service-account' of github.com:google…
olavloite Oct 24, 2023
11b2ac9
fix: add option for disabling direct path
olavloite Oct 24, 2023
4a196b0
fix: disable direct path for test
olavloite Oct 24, 2023
d36fb3a
Merge branch 'main' into allow-non-default-service-account
olavloite Dec 20, 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
16 changes: 16 additions & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.cloud.spanner;

import com.google.api.core.ApiFunction;
import com.google.api.core.BetaApi;
import com.google.api.core.InternalApi;
import com.google.api.gax.core.ExecutorProvider;
import com.google.api.gax.grpc.GrpcCallContext;
Expand Down Expand Up @@ -135,6 +136,7 @@ public class SpannerOptions extends ServiceOptions<Spanner, SpannerOptions> {
private final CloseableExecutorProvider asyncExecutorProvider;
private final String compressorName;
private final boolean leaderAwareRoutingEnabled;
private final boolean attemptDirectPath;

/** Interface that can be used to provide {@link CallCredentials} to {@link SpannerOptions}. */
public interface CallCredentialsProvider {
Expand Down Expand Up @@ -624,6 +626,7 @@ private SpannerOptions(Builder builder) {
asyncExecutorProvider = builder.asyncExecutorProvider;
compressorName = builder.compressorName;
leaderAwareRoutingEnabled = builder.leaderAwareRoutingEnabled;
attemptDirectPath = builder.attemptDirectPath;
}

/**
Expand Down Expand Up @@ -725,6 +728,7 @@ public static class Builder
private String compressorName;
private String emulatorHost = System.getenv("SPANNER_EMULATOR_HOST");
private boolean leaderAwareRoutingEnabled = true;
private boolean attemptDirectPath = true;

private static String createCustomClientLibToken(String token) {
return token + " " + ServiceOptions.getGoogApiClientLibName();
Expand Down Expand Up @@ -784,6 +788,7 @@ private Builder() {
this.channelProvider = options.channelProvider;
this.channelConfigurator = options.channelConfigurator;
this.interceptorProvider = options.interceptorProvider;
this.attemptDirectPath = options.attemptDirectPath;
}

@Override
Expand Down Expand Up @@ -1220,6 +1225,12 @@ public Builder disableLeaderAwareRouting() {
return this;
}

@BetaApi
public Builder disableDirectPath() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Instead of a method, wouldn't it be better to use an internal system property? AFAIU, we only require this for our test infrastructure right now.
  • Also, my understanding is customers would never want to disable directpath, given they don't need to understand about internal proxy connections between GCP and Borg. If yes, that also pushes us to not expose a public method and instead use a system property?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure. It is certainly something that they should not do for normal production connections. As we've seen in our own tests however, it is something that you need to do when connecting to something local that does not use credentials. That could be the emulator (and we turn it off by default if they connect in a known way to the emulator). It is also required for connecting to an in-mem Spanner test server like the ones that we use for our own testing. I don't think many customers use it, but it is not unthinkable.

this.attemptDirectPath = false;
return this;
}

@SuppressWarnings("rawtypes")
@Override
public SpannerOptions build() {
Expand Down Expand Up @@ -1360,6 +1371,11 @@ public boolean isLeaderAwareRoutingEnabled() {
return leaderAwareRoutingEnabled;
}

@BetaApi
public boolean isAttemptDirectPath() {
return attemptDirectPath;
}

/** Returns the default query options to use for the specific database. */
public QueryOptions getDefaultQueryOptions(DatabaseId databaseId) {
// Use the specific query options for the database if any have been specified. These have
Expand Down
10 changes: 9 additions & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import com.google.api.gax.rpc.UnavailableException;
import com.google.api.gax.rpc.WatchdogProvider;
import com.google.api.pathtemplate.PathTemplate;
import com.google.cloud.NoCredentials;
import com.google.cloud.RetryHelper;
import com.google.cloud.RetryHelper.RetryHelperException;
import com.google.cloud.grpc.GcpManagedChannelBuilder;
Expand Down Expand Up @@ -339,9 +340,16 @@ public GapicSpannerRpc(final SpannerOptions options) {
// This sets the response compressor (Server -> Client).
.withEncoding(compressorName))
.setHeaderProvider(headerProviderWithUserAgent)
.setAllowNonDefaultServiceAccount(true)
// Attempts direct access to spanner service over gRPC to improve throughput,
// whether the attempt is allowed is totally controlled by service owner.
.setAttemptDirectPath(true);
// We'll only attempt DirectPath if we are using real credentials.
// NoCredentials is used for plain text connections, for example when connecting to
// the emulator.
.setAttemptDirectPath(
options.isAttemptDirectPath()
&& !Objects.equals(
options.getScopedCredentials(), NoCredentials.getInstance()));
Comment on lines +351 to +352
Copy link
Collaborator

Choose a reason for hiding this comment

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

!Objects.equals( options.getScopedCredentials(), NoCredentials.getInstance())

This sounds more like a identification strategy for emulator. Can't we use the env variable used to identify emulator instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need this for more than just the emulator. This is also used for the in-memory Spanner mock server tests that also use NoCredentials. Also; It is possible to connect to the emulator without setting the environment variable. You can also just directly set the host name.


// If it is enabled in options uses the channel pool provided by the gRPC-GCP extension.
maybeEnableGrpcGcpExtension(defaultChannelProviderBuilder, options);
Expand Down
14 changes: 8 additions & 6 deletions ...ud-spanner/src/test/java/com/google/cloud/spanner/connection/CredentialsProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,10 @@ public void testCredentialsProvider() throws Throwable {
"cloudspanner://localhost:%d/projects/proj/instances/inst/databases/db?credentialsProvider=%s",
getPort(), TestCredentialsProvider.class.getName()))
.setConfigurator(
spannerOptions ->
spannerOptions.setChannelConfigurator(
ManagedChannelBuilder::usePlaintext))
spannerOptions -> {
spannerOptions.setChannelConfigurator(ManagedChannelBuilder::usePlaintext);
spannerOptions.disableDirectPath();
})
.build();

try (Connection connection = options.getConnection()) {
Expand Down Expand Up @@ -122,9 +123,10 @@ public void testCredentialsProvider() throws Throwable {
"cloudspanner://localhost:%d/projects/proj/instances/inst/databases/db?credentialsProvider=%s",
getPort(), TestCredentialsProvider.class.getName()))
.setConfigurator(
spannerOptions ->
spannerOptions.setChannelConfigurator(
ManagedChannelBuilder::usePlaintext))
spannerOptions -> {
spannerOptions.setChannelConfigurator(ManagedChannelBuilder::usePlaintext);
spannerOptions.disableDirectPath();
})
.build();
try (Connection connection = options.getConnection()) {
assertEquals(
Expand Down
8 changes: 3 additions & 5 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import com.google.spanner.v1.TypeCode;
import io.grpc.Context;
import io.grpc.Contexts;
import io.grpc.ManagedChannelBuilder;
import io.grpc.Metadata;
import io.grpc.Metadata.Key;
import io.grpc.MethodDescriptor;
Expand Down Expand Up @@ -641,11 +642,8 @@ private SpannerOptions createSpannerOptions() {
return SpannerOptions.newBuilder()
.setProjectId("[PROJECT]")
// Set a custom channel configurator to allow http instead of https.
.setChannelConfigurator(
input -> {
input.usePlaintext();
return input;
})
.setChannelConfigurator(ManagedChannelBuilder::usePlaintext)
.disableDirectPath()
.setHost("http://" + endpoint)
// Set static credentials that will return the static OAuth test token.
.setCredentials(STATIC_CREDENTIALS)
Expand Down
8 changes: 3 additions & 5 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GfeLatencyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.spanner.v1.StructType;
import com.google.spanner.v1.TypeCode;
import io.grpc.ForwardingServerCall;
import io.grpc.ManagedChannelBuilder;
import io.grpc.Metadata;
import io.grpc.Server;
import io.grpc.ServerCall;
Expand Down Expand Up @@ -311,11 +312,8 @@ private static SpannerOptions createSpannerOptions(InetSocketAddress address, Se
return SpannerOptions.newBuilder()
.setProjectId("[PROJECT]")
// Set a custom channel configurator to allow http instead of https.
.setChannelConfigurator(
input -> {
input.usePlaintext();
return input;
})
.setChannelConfigurator(ManagedChannelBuilder::usePlaintext)
.disableDirectPath()
.setHost("http://" + endpoint)
// Set static credentials that will return the static OAuth test token.
.setCredentials(STATIC_CREDENTIALS)
Expand Down
Loading