feat(api): (poc) add SslContext setting#12848
Conversation
…/poc-prefer-jdk
…zp/grpc-java into chore/poc-prefer-jdk
|
/gcbrun |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the preferJdkSslProvider(javax.net.ssl.SSLContext) method to ManagedChannelBuilder and implements it across ForwardingChannelBuilder2, NettyChannelBuilder, and OkHttpChannelBuilder to allow configuring a custom JDK SSLContext for secure connections. The review feedback correctly identifies that passing null to preferJdkSslProvider should reset the builder to its default SSL configuration (by delegating to sslContext(null) or sslSocketFactory(null)) rather than acting as a no-op, and suggests updating the corresponding unit tests to verify this reset behavior.
| if (sslContext == null) { | ||
| return this; | ||
| } |
There was a problem hiding this comment.
According to the Javadoc in ManagedChannelBuilder, passing null to preferJdkSslProvider should reset the builder to use defaults. Currently, passing null is a no-op, which prevents resetting the SSL context if a custom one was previously configured. We should delegate to sslContext(null) when sslContext is null.
| if (sslContext == null) { | |
| return this; | |
| } | |
| if (sslContext == null) { | |
| return sslContext(null); | |
| } |
| if (sslContext != null) { | ||
| sslSocketFactory(sslContext.getSocketFactory()); | ||
| } |
There was a problem hiding this comment.
According to the Javadoc in ManagedChannelBuilder, passing null to preferJdkSslProvider should reset the builder to use defaults. Currently, passing null is a no-op, which prevents resetting the SSL socket factory if a custom one was previously configured. We should delegate to sslSocketFactory(null) when sslContext is null.
if (sslContext != null) {
sslSocketFactory(sslContext.getSocketFactory());
} else {
sslSocketFactory(null);
}| @Test | ||
| public void preferJdkSslProviderJavax_null_isNoop() throws Exception { | ||
| NettyChannelBuilder builder = NettyChannelBuilder.forTarget("fakeTarget"); | ||
| SslContext customContext = mock(SslContext.class); | ||
| org.mockito.Mockito.when(customContext.isClient()).thenReturn(true); | ||
| io.netty.handler.ssl.ApplicationProtocolNegotiator apn = mock(io.netty.handler.ssl.ApplicationProtocolNegotiator.class); | ||
| org.mockito.Mockito.when(apn.protocols()).thenReturn(java.util.Arrays.asList("h2")); | ||
| org.mockito.Mockito.when(customContext.applicationProtocolNegotiator()).thenReturn(apn); | ||
| builder.sslContext(customContext); | ||
|
|
||
| builder.preferJdkSslProvider(null); | ||
|
|
||
| java.lang.reflect.Field factoryField = NettyChannelBuilder.class.getDeclaredField("protocolNegotiatorFactory"); | ||
| factoryField.setAccessible(true); | ||
| Object factory = factoryField.get(builder); | ||
|
|
||
| java.lang.reflect.Field sslContextField = factory.getClass().getDeclaredField("sslContext"); | ||
| sslContextField.setAccessible(true); | ||
| assertThat(sslContextField.get(factory)).isSameInstanceAs(customContext); | ||
| } |
There was a problem hiding this comment.
Update the test to assert that passing null to preferJdkSslProvider resets the SSL context to defaults (i.e., null), rather than acting as a no-op.
| @Test | |
| public void preferJdkSslProviderJavax_null_isNoop() throws Exception { | |
| NettyChannelBuilder builder = NettyChannelBuilder.forTarget("fakeTarget"); | |
| SslContext customContext = mock(SslContext.class); | |
| org.mockito.Mockito.when(customContext.isClient()).thenReturn(true); | |
| io.netty.handler.ssl.ApplicationProtocolNegotiator apn = mock(io.netty.handler.ssl.ApplicationProtocolNegotiator.class); | |
| org.mockito.Mockito.when(apn.protocols()).thenReturn(java.util.Arrays.asList("h2")); | |
| org.mockito.Mockito.when(customContext.applicationProtocolNegotiator()).thenReturn(apn); | |
| builder.sslContext(customContext); | |
| builder.preferJdkSslProvider(null); | |
| java.lang.reflect.Field factoryField = NettyChannelBuilder.class.getDeclaredField("protocolNegotiatorFactory"); | |
| factoryField.setAccessible(true); | |
| Object factory = factoryField.get(builder); | |
| java.lang.reflect.Field sslContextField = factory.getClass().getDeclaredField("sslContext"); | |
| sslContextField.setAccessible(true); | |
| assertThat(sslContextField.get(factory)).isSameInstanceAs(customContext); | |
| } | |
| @Test | |
| public void preferJdkSslProviderJavax_null_resetsToDefault() throws Exception { | |
| NettyChannelBuilder builder = NettyChannelBuilder.forTarget("fakeTarget"); | |
| SslContext customContext = mock(SslContext.class); | |
| org.mockito.Mockito.when(customContext.isClient()).thenReturn(true); | |
| io.netty.handler.ssl.ApplicationProtocolNegotiator apn = mock(io.netty.handler.ssl.ApplicationProtocolNegotiator.class); | |
| org.mockito.Mockito.when(apn.protocols()).thenReturn(java.util.Arrays.asList("h2")); | |
| org.mockito.Mockito.when(customContext.applicationProtocolNegotiator()).thenReturn(apn); | |
| builder.sslContext(customContext); | |
| builder.preferJdkSslProvider(null); | |
| java.lang.reflect.Field factoryField = NettyChannelBuilder.class.getDeclaredField("protocolNegotiatorFactory"); | |
| factoryField.setAccessible(true); | |
| Object factory = factoryField.get(builder); | |
| java.lang.reflect.Field sslContextField = factory.getClass().getDeclaredField("sslContext"); | |
| sslContextField.setAccessible(true); | |
| assertThat(sslContextField.get(factory)).isNull(); | |
| } |
This PR works as a POC for exposing a setting in
ManagedChannelBuilderthat allows for preferring the JDK implementation in transports that
support it (e.g. netty), or throw an unsupported op exception for those
who don't.
This would allow gax-grpc to easily insert a custom ssl context that
contains the providers we need as follows:
This was tested locally in googleapis/google-cloud-java#13388