feat: default authentication support for external hosts #3655
Conversation
product-auto-label
bot
added
size: s
| private boolean enableBuiltInMetrics = SpannerOptions.environment.isEnableBuiltInMetrics(); | ||
| private String monitoringHost = SpannerOptions.environment.getMonitoringHost(); | ||
| private SslContext mTLSContext = null; | ||
| private boolean isExternalHost = false; |
There was a problem hiding this comment.
Can we make this a method instead of a field. This now gives the impression of being in sync with the current settings, but it isn't if you do weird stuff (like first setting an emulator host and then overriding that by setting a host). By implementing it as a method, you can just calculate it based on the values at that time, instead of trying to set the correct value when setHost / setEmulatorHost is being set.
There was a problem hiding this comment.
removed the need to check emulatorHost. But had to keep isExternalHost calculation in setHost. Can't convert it to a method to do a on demand calculation since host is a private member in the parent builder class
#3656
| enableEndToEndTracing = builder.enableEndToEndTracing; | ||
| monitoringHost = builder.monitoringHost; | ||
| String externalHostTokenPath = System.getenv("SPANNER_EXTERNAL_HOST_AUTH_TOKEN"); | ||
| if (builder.isExternalHost && builder.emulatorHost == null && externalHostTokenPath != null) { |
There was a problem hiding this comment.
- Prefer
!Strings.isNullOrEmpty(externalHostTokenPath)over a null check. - This check should also include
&& builder.credentials == nullto prevent the default credentials to override specific credentials that have been set for this builder.
| if (options.getConfigurator() != null) { | ||
| options.getConfigurator().configure(builder); | ||
| } | ||
| if (options.usesEmulator()) { |
There was a problem hiding this comment.
Why do we need this now?
| } catch (IOException e) { | ||
| throw SpannerExceptionFactory.newSpannerException(e); | ||
| } | ||
| credentials = new GoogleCredentials(new AccessToken(token, null)); |
There was a problem hiding this comment.
This constructor is deprecated. Use the static create method instead.
|
Closing this PR as its not able to upload the latest commit and stuck in this state for 10hrs 
|
Default authentication support for external hosts by using auth token path mentioned in env variable