Copilot commented on code in PR #17989:
URL: https://github.com/apache/pinot/pull/17989#discussion_r2999276047
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/admin/PinotAdminTransport.java:
##########
@@ -86,9 +90,13 @@ public PinotAdminTransport(Properties properties,
Map<String, String> authHeader
// Configure SSL if needed
if (CommonConstants.HTTPS_PROTOCOL.equalsIgnoreCase(scheme)) {
try {
- SSLContext sslContext = SSLContext.getDefault();
- builder.setSslContext(new
io.netty.handler.ssl.JdkSslContext(sslContext, true,
- io.netty.handler.ssl.ClientAuth.OPTIONAL));
+ TlsConfig tlsConfig =
ConnectionUtils.getTlsConfigFromProperties(properties);
+ // NOTE: ConnectionUtils.getSSLContextFromProperties(...) installs a
JVM-global default SSLSocketFactory
+ // and updates the shared TlsUtils SSLContext. PinotAdminTransport has
historically inherited that process-
+ // wide side effect from the shared helper, so callers enabling HTTPS
here should treat the TLS config as
+ // affecting other HttpsURLConnection-based clients in the same JVM.
+ SSLContext sslContext =
ConnectionUtils.getSSLContextFromProperties(properties);
Review Comment:
PinotAdminTransport now calls
ConnectionUtils.getSSLContextFromProperties(properties), which installs a
JVM-global default SSLSocketFactory and updates the shared TlsUtils SSLContext.
This is a behavioral change from the previous approach
(SSLContext.getDefault()) and can unintentionally affect other HTTPS clients in
the same JVM. Consider constructing an SSLContext for this AsyncHttpClient
instance without mutating global defaults (e.g., build from the extracted
TlsConfig/SSLFactory directly), or keep the prior per-client SSLContext
behavior; if the global side effect is intentional, the comment should be
updated to reflect that it is newly introduced here and document the impact
explicitly.
```suggestion
// Use the JVM default SSLContext to avoid mutating global TLS
configuration here; TlsConfig is still
// consulted for endpoint identification behavior via the
SslContextProvider.
SSLContext sslContext = SSLContext.getDefault();
```
--
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]
