feat(spanner): mTLS setup for spanner external host clients #3574
+101
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
product-auto-label
bot
added
size: m
|
Warning: This pull request is touching the following templated files:
|
sagnghos
force-pushed
the
sagnghos/tlsConfig
branch
from
b85de2b to
1f1ef13
Compare
olavloite
reviewed
Jan 6, 2025
| @@ -393,6 +393,10 @@ Spanner createSpanner(SpannerPoolKey key, ConnectionOptions options) { | |||
| // Set a custom channel configurator to allow http instead of https. | |||
| builder.setChannelConfigurator(ManagedChannelBuilder::usePlaintext); | |||
| } | |||
| if (options.getClientCertificate() != null && options.getClientCertificateKey() != null) { | |||
There was a problem hiding this comment.
You should also add these properties to SpannerPoolKey. That will ensure that if you use two different client certificates, that you also get two different Spanner instances.
| @@ -1485,6 +1496,34 @@ public Builder setEmulatorHost(String emulatorHost) { | |||
| return this; | |||
| } | |||
|
|
|||
| public Builder useClientCert(String host, String clientCertificate, String clientKey) { | |||
There was a problem hiding this comment.
I don't think we should add this method here. Instead, this logic should probably be moved to ConnectionOptions, and then that class can just call SpannerOptions$Builder#setChannelProvider(..)
There was a problem hiding this comment.
If you do decide to keep it here, then:
- Add Javadoc. It is a public method.
- Consider adding
@ExperimentalApito indicate that it might change in the future.
| try { | ||
| URI uri = new URI(host); | ||
| managedChannel = | ||
| NettyChannelBuilder.forAddress(uri.getHost(), uri.getPort()) |
There was a problem hiding this comment.
I think that this will create a single gRPC channel, and not a channel pool. That means that the potential concurrency of any client that uses this option will be lower than the default for the Java client (the default is to use a channel pool with 4 channels).
There was a problem hiding this comment.
Also, I'm not really a fan of a set method on a builder like this doing this much logic. Normally, I would expect this logic to be executed when the object is actually built, and not in the set method. (See also my first comment on this method)
| .build()) | ||
| .build(); | ||
|
|
||
| setChannelProvider( |
There was a problem hiding this comment.
This will override all the additional settings etc. that are set on the channel provider that is used by default by the Java client. That means that you will potentially be missing out on a lot of default settings and features, such as compression, keep-alive settings, interceptors, end-to-end tracing, direct-path etc. Are you sure you want that?
| throw new IllegalArgumentException( | ||
| "Invalid host format. Expected format: 'protocol://host[:port]'.", e); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Unexpected error during mTLS setup.", e); |
There was a problem hiding this comment.
Throw a SpannerException in a case like this. They can be created with SpannerExceptionFactory.
| return this; | ||
| } | ||
|
|
||
| public Builder usePlainText() { |
There was a problem hiding this comment.
I'm not sure we need a separate method for this. This is not something a normal user would normally need to call separately.
| @@ -1593,6 +1632,24 @@ public SpannerOptions build() { | |||
| this.setChannelConfigurator(ManagedChannelBuilder::usePlaintext); | |||
| // As we are using plain text, we should never send any credentials. | |||
| this.setCredentials(NoCredentials.getInstance()); | |||
| } else if (managedChannel != null) { | |||
| // Add shutdown hook for the ManagedChannel if created to prevent resource leak | |||
There was a problem hiding this comment.
We should not add a shutdown hook for this. GapicSpannerRpc instances can also be closed without shutting down the application, and this would then not be invoked. Instead, I think it would be better if GapicSpannerRpc kept track of the fact that it uses a custom channel provider that has a shutdown method, and call that shutdown method when a GapicSpannerRpc instance is closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.