Copilot commented on code in PR #17358:
URL: https://github.com/apache/pinot/pull/17358#discussion_r2689878097
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/ChannelManager.java:
##########
@@ -51,11 +53,15 @@ public class ChannelManager {
*/
private final Duration _idleTimeout;
private final int _maxInboundMessageSize;
+ @Nullable
+ private final SslContext _clientSslContext;
Review Comment:
The new `clientSslContext` parameter lacks documentation. Add a Javadoc
comment explaining that this optional parameter provides a cached SSL context
to reuse across channels.
```suggestion
/**
* Constructs a {@code ChannelManager}.
*
* @param tlsConfig TLS configuration used to build a client {@link
SslContext} when {@code clientSslContext} is null
* @param clientSslContext optional cached client {@link SslContext} to
reuse across channels instead of building a
* new one for each connection; when null, an SSL
context will be created from
* {@code tlsConfig}
* @param maxInboundMessageSize maximum inbound message size for gRPC
channels
* @param idleTimeout idle timeout for gRPC channels; channels will be
closed after this period of inactivity
*/
```
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/GrpcMailboxServer.java:
##########
@@ -58,7 +59,7 @@ public class GrpcMailboxServer extends
PinotMailboxGrpc.PinotMailboxImplBase {
private final Server _server;
Review Comment:
The new `sslContext` parameter lacks documentation. Add a Javadoc comment
explaining that this optional parameter allows passing a pre-built SSL context
to avoid redundant context creation.
```suggestion
/**
* Constructs a gRPC-based mailbox server.
*
* @param mailboxService mailbox service providing configuration such as
port and instance type
* @param config Pinot configuration used to initialize access control and
server options
* @param tlsConfig optional TLS configuration; when {@code null}, the
server is started without TLS
* @param sslContext optional pre-built SSL context; when non-null, this
context is used instead of
* creating a new one from {@code tlsConfig}, avoiding
redundant SSL context creation
* @param accessControlFactory optional factory for building query access
control; when {@code null},
* a factory is created from {@code config}
*/
```
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/DefaultSslContextProvider.java:
##########
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.client;
+
+import java.util.Collections;
+import java.util.List;
+import javax.annotation.Nullable;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import org.asynchttpclient.DefaultAsyncHttpClientConfig;
+
+
+/**
+ * Default SSL context provider that uses the JDK/BCJSSE stack and disables
OpenSSL.
+ */
+public class DefaultSslContextProvider implements SslContextProvider {
+
+ @Override
+ public DefaultAsyncHttpClientConfig.Builder
configure(DefaultAsyncHttpClientConfig.Builder builder,
+ @Nullable SSLContext sslContext, TlsProtocols tlsProtocols) {
+ builder.setUseOpenSsl(false);
+
+ List<String> enabledProtocolList =
+ tlsProtocols != null ? tlsProtocols.getEnabledProtocols() :
Collections.emptyList();
+ if (enabledProtocolList == null) {
+ enabledProtocolList = Collections.emptyList();
+ }
Review Comment:
The null check on line 41 is redundant. If `tlsProtocols` is null, line 40
already assigns `Collections.emptyList()`, which cannot be null. If
`tlsProtocols.getEnabledProtocols()` can return null, handle that case directly
in the ternary expression.
```suggestion
(tlsProtocols != null && tlsProtocols.getEnabledProtocols() != null)
? tlsProtocols.getEnabledProtocols()
: Collections.emptyList();
```
##########
pinot-clients/pinot-java-client/src/test/java/org/apache/pinot/client/SslContextProviderTest.java:
##########
@@ -0,0 +1,230 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.client;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.reflect.Constructor;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.List;
+import java.util.stream.Stream;
+import javax.annotation.Nullable;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import org.asynchttpclient.DefaultAsyncHttpClientConfig;
+import org.asynchttpclient.Dsl;
+import org.asynchttpclient.SslEngineFactory;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertTrue;
+
+
+public class SslContextProviderTest {
+ private static final String PROVIDER_PROPERTY =
"pinot.client.sslContextProvider";
+
+ @Test
+ public void testDefaultProviderConfiguresProtocolsAndDisablesOpenSsl()
+ throws Exception {
+ DefaultAsyncHttpClientConfig.Builder builder = Dsl.config();
+ SSLContext probeContext = SSLContext.getInstance("TLS");
+ probeContext.init(null, null, null);
+ String protocol = selectSupportedProtocol(probeContext);
+ TlsProtocols tlsProtocols = tlsProtocolsWith(protocol);
+
+ SslContextProvider provider = new DefaultSslContextProvider();
+ provider.configure(builder, null, tlsProtocols);
+
+ DefaultAsyncHttpClientConfig config = builder.build();
+ assertFalse(config.isUseOpenSsl());
+ assertEquals(config.getEnabledProtocols(), new String[] {protocol});
+ }
+
+ @Test
+ public void testDefaultProviderCreatesClientSslEngine()
+ throws Exception {
+ DefaultAsyncHttpClientConfig.Builder builder = Dsl.config();
+
+ SSLContext sslContext = SSLContext.getInstance("TLS");
+ sslContext.init(null, null, null);
+ String protocol = selectSupportedProtocol(sslContext);
+
+ SslContextProvider provider = new DefaultSslContextProvider();
+ provider.configure(builder, sslContext, tlsProtocolsWith(protocol));
+
+ DefaultAsyncHttpClientConfig config = builder.build();
+ assertFalse(config.isUseOpenSsl());
+ assertEquals(config.getEnabledProtocols(), new String[] {protocol});
+
+ SslEngineFactory engineFactory = config.getSslEngineFactory();
+ assertNotNull(engineFactory);
+ SSLEngine engine = engineFactory.newSslEngine(config, "localhost", 443);
+ assertTrue(engine.getUseClientMode());
+ assertEquals(engine.getEnabledProtocols(), new String[] {protocol});
+ }
+
+ @Test
+ public void testFactoryUsesSystemPropertyProvider() {
+ String originalProperty = System.getProperty(PROVIDER_PROPERTY);
+ System.setProperty(PROVIDER_PROPERTY, PropertyProvider.class.getName());
+ try {
+ SslContextProvider provider = SslContextProviderFactory.create();
+ assertTrue(provider instanceof PropertyProvider);
+ } finally {
+ restoreProperty(originalProperty);
+ }
+ }
+
+ @Test
+ public void testFactoryUsesServiceLoaderProviderWhenPropertyMissing()
+ throws Exception {
+ String originalProperty = System.getProperty(PROVIDER_PROPERTY);
+ restoreProperty(null);
+ try {
+ SslContextProvider provider =
createFromServiceLoader(ServiceLoaderProvider.class);
+ assertTrue(provider instanceof ServiceLoaderProvider);
+ } finally {
+ restoreProperty(originalProperty);
+ }
+ }
+
+ @Test
+ public void testFactoryAcceptsDefaultProviderSubclassFromServiceLoader()
+ throws Exception {
+ String originalProperty = System.getProperty(PROVIDER_PROPERTY);
+ restoreProperty(null);
+ try {
+ SslContextProvider provider =
createFromServiceLoader(DefaultProviderSubclass.class);
+ assertTrue(provider instanceof DefaultProviderSubclass);
+ } finally {
+ restoreProperty(originalProperty);
+ }
+ }
+
+ @Test
+ public void
testFactoryFallsBackToDefaultWhenPropertyInvalidAndNoServiceLoader() {
+ String originalProperty = System.getProperty(PROVIDER_PROPERTY);
+ System.setProperty(PROVIDER_PROPERTY, String.class.getName());
+ try {
+ SslContextProvider provider = SslContextProviderFactory.create();
+ assertTrue(provider instanceof DefaultSslContextProvider);
+ } finally {
+ restoreProperty(originalProperty);
+ }
+ }
+
+ private static void restoreProperty(@Nullable String value) {
+ if (value == null) {
+ System.clearProperty(PROVIDER_PROPERTY);
+ } else {
+ System.setProperty(PROVIDER_PROPERTY, value);
+ }
+ }
+
+ private static SslContextProvider createFromServiceLoader(Class<? extends
SslContextProvider> providerClass)
+ throws Exception {
+ ClassLoader originalClassLoader =
Thread.currentThread().getContextClassLoader();
+
+ Path tempDir = Files.createTempDirectory("pinot-ssl-provider");
+ Path servicesDir = tempDir.resolve("META-INF").resolve("services");
+ Files.createDirectories(servicesDir);
+ Path serviceFile = servicesDir.resolve(SslContextProvider.class.getName());
+ Files.writeString(serviceFile, providerClass.getName(),
StandardCharsets.UTF_8);
+
+ Throwable failure = null;
+ try (URLClassLoader loader =
+ new URLClassLoader(new URL[] {tempDir.toUri().toURL()},
SslContextProviderTest.class.getClassLoader())) {
+ Thread.currentThread().setContextClassLoader(loader);
+ return SslContextProviderFactory.create();
Review Comment:
The context class loader is set inside the try-with-resources block but the
factory call on line 162 returns early. If `SslContextProviderFactory.create()`
throws an exception before the finally block executes, the original class
loader may not be restored. Consider restructuring to ensure the class loader
is always restored.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/grpc/BrokerGrpcServer.java:
##########
@@ -362,15 +362,19 @@ static SslContext buildGRpcSslContext(TlsConfig tlsConfig)
SSLFactory sslFactory =
RenewableTlsUtils.createSSLFactoryAndEnableAutoRenewalWhenUsingFileStores(tlsConfig);
// since tlsConfig.getKeyStorePath() is not null,
sslFactory.getKeyManagerFactory().get() should not be null
- SslContextBuilder sslContextBuilder =
SslContextBuilder.forServer(sslFactory.getKeyManagerFactory().get())
- .sslProvider(SslProvider.valueOf(tlsConfig.getSslProvider()));
+ SslProvider sslProvider =
SslProvider.valueOf(tlsConfig.getSslProvider());
+ if (sslProvider != SslProvider.JDK) {
+ LOGGER.warn("Configured SSL provider is {}. For FIPS/BCJSSE
environments use JDK provider.", sslProvider);
+ }
+ SslContextBuilder sslContextBuilder =
+
SslContextBuilder.forServer(sslFactory.getKeyManagerFactory().get()).sslProvider(sslProvider);
sslFactory.getTrustManagerFactory().ifPresent(sslContextBuilder::trustManager);
if (tlsConfig.isClientAuthEnabled()) {
sslContextBuilder.clientAuth(ClientAuth.REQUIRE);
}
- return GrpcSslContexts.configure(sslContextBuilder).build();
+ return GrpcSslContexts.configure(sslContextBuilder, sslProvider).build();
Review Comment:
The method `GrpcSslContexts.configure()` is being called with two arguments,
but the standard gRPC API only accepts a single `SslContextBuilder` argument.
Verify that this overload exists or if this should be
`GrpcSslContexts.configure(sslContextBuilder).build()`.
```suggestion
return GrpcSslContexts.configure(sslContextBuilder).build();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
