-
Notifications
You must be signed in to change notification settings - Fork 114
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.
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
chore: allow non-default service account for DP #2635
Conversation
…apis/java-spanner into allow-non-default-service-account
@@ -1220,6 +1225,12 @@ public Builder disableLeaderAwareRouting() { | |||
return this; | |||
} | |||
|
|||
@BetaApi | |||
public Builder disableDirectPath() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
&& !Objects.equals( | ||
options.getScopedCredentials(), NoCredentials.getInstance())); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
thanks a lot for the fix! |
By default, direct path will only be attempted if the application uses the default service account of the Google Compute Engine that it is running on. This pull request sets the option to allow the use of a non-default service account in combination with direct path. This is necessary both for our tests, that use a separate service account than the default service account of the GCE instance that the test is running on, and for end users who might want to use a separate service account for their application.
The pull request additionally ensures that direct path is only attempted in the following cases:
NoCredentials
. The latter is used for plain text connections to the emulator and to the in-memory mock Spanner server that is used for testing.disableDirectPath()
option has not been set. This option is used for some specific tests that run against the in-memory Spanner mock server, and that use (test) customCredentialProvider
implementations. The option is also an escape hatch for applications that for whatever reason would want to disable direct path. The option is marked as a@BetaApi
, so we have the option of removing it in the future. The method cannot be package-private, as the tests that rely on it live in a different package.Replaces #2603