Copilot commented on code in PR #17989:
URL: https://github.com/apache/pinot/pull/17989#discussion_r2998625062
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java:
##########
@@ -67,23 +67,23 @@ public JsonAsyncHttpPinotClientTransport() {
_scheme = CommonConstants.HTTP_PROTOCOL;
_extraOptionStr = DEFAULT_EXTRA_QUERY_OPTION_STRING;
Builder builder = Dsl.config();
- SSL_CONTEXT_PROVIDER.configure(builder, null,
TlsProtocols.defaultProtocols(false));
+ SSL_CONTEXT_PROVIDER.configure(builder, null,
TlsProtocols.defaultProtocols(false), "");
_httpClient =
Dsl.asyncHttpClient(builder.setRequestTimeout(Duration.ofMillis(_brokerReadTimeout)).build());
_useMultistageEngine = false;
}
public JsonAsyncHttpPinotClientTransport(Map<String, String> headers, String
scheme, String extraOptionString,
boolean useMultistageEngine, @Nullable SSLContext sslContext,
ConnectionTimeouts connectionTimeouts,
- TlsProtocols tlsProtocols, @Nullable String appId) {
+ TlsProtocols tlsProtocols, @Nullable String appId, String
endpointIdentificationAlgorithm) {
_brokerReadTimeout = connectionTimeouts.getReadTimeoutMs();
_headers = headers;
_scheme = scheme;
_extraOptionStr = StringUtils.isEmpty(extraOptionString) ?
DEFAULT_EXTRA_QUERY_OPTION_STRING : extraOptionString;
_useMultistageEngine = useMultistageEngine;
Builder builder = Dsl.config();
- SSL_CONTEXT_PROVIDER.configure(builder, sslContext, tlsProtocols);
+ SSL_CONTEXT_PROVIDER.configure(builder, sslContext, tlsProtocols,
endpointIdentificationAlgorithm);
Review Comment:
The public constructor `JsonAsyncHttpPinotClientTransport(Map, String, ...)`
now requires an extra `endpointIdentificationAlgorithm` parameter, which is a
breaking API change for callers who instantiate this transport directly.
Consider adding an overload matching the prior signature that delegates to this
constructor with a default value so existing client code continues to
compile/run unchanged.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerChannels.java:
##########
@@ -205,8 +213,28 @@ protected void initChannel(SocketChannel ch) {
}
void closeChannel() {
- if (_channel != null) {
- _channel.close();
+ closeChannel(false);
+ }
+
+ void closeChannel(boolean silentShutdown) {
+ Channel channel = _channel;
+ if (channel == null) {
+ return;
+ }
+ if (silentShutdown) {
+ setSilentShutdown();
+ }
+ io.netty.channel.ChannelFuture closeFuture = channel.close();
+ if (closeFuture != null) {
Review Comment:
Avoid using the fully-qualified `io.netty.channel.ChannelFuture` type inline
here; the rest of the file uses imports for Netty types. Import `ChannelFuture`
at the top for consistency/readability.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ChannelHandlerFactory.java:
##########
@@ -72,7 +74,24 @@ public static ChannelHandler getLengthFieldPrepender() {
public static ChannelHandler getClientTlsHandler(TlsConfig tlsConfig,
SocketChannel ch) {
SslContext sslContext = CLIENT_SSL_CONTEXTS_CACHE
.computeIfAbsent(tlsConfig.hashCode(), tlsConfigHashCode ->
TlsUtils.buildClientContext(tlsConfig));
- return sslContext.newHandler(ch.alloc());
+ return configureClientTlsHandler(tlsConfig,
sslContext.newHandler(ch.alloc()));
+ }
+
+ public static ChannelHandler getClientTlsHandler(TlsConfig tlsConfig,
SocketChannel ch, String peerHost,
+ int peerPort) {
+ SslContext sslContext = CLIENT_SSL_CONTEXTS_CACHE
+ .computeIfAbsent(tlsConfig.hashCode(), tlsConfigHashCode ->
TlsUtils.buildClientContext(tlsConfig));
+ return configureClientTlsHandler(tlsConfig,
sslContext.newHandler(ch.alloc(), peerHost, peerPort));
+ }
+
+ private static SslHandler configureClientTlsHandler(TlsConfig tlsConfig,
SslHandler sslHandler) {
+ String endpointIdentificationAlgorithm =
tlsConfig.getEndpointIdentificationAlgorithm();
+ if (endpointIdentificationAlgorithm != null &&
!endpointIdentificationAlgorithm.isBlank()) {
+ SSLParameters sslParameters = sslHandler.engine().getSSLParameters();
+
sslParameters.setEndpointIdentificationAlgorithm(endpointIdentificationAlgorithm);
+ sslHandler.engine().setSSLParameters(sslParameters);
+ }
Review Comment:
`configureClientTlsHandler` only sets the endpoint identification algorithm
when the config value is non-blank; it never explicitly clears/disables it. If
the underlying `SSLEngine` already has an endpoint identification algorithm set
(e.g., via TLS stack defaults), then configuring
`TlsConfig.endpointIdentificationAlgorithm` as blank/null will not disable
hostname verification as intended. Consider always applying `SSLParameters`
here and setting `setEndpointIdentificationAlgorithm(null)` when the config is
blank/null so the behavior is deterministic.
```suggestion
SSLParameters sslParameters = sslHandler.engine().getSSLParameters();
if (endpointIdentificationAlgorithm == null ||
endpointIdentificationAlgorithm.isBlank()) {
// Clear any existing endpoint identification algorithm so behavior
matches the Pinot config.
sslParameters.setEndpointIdentificationAlgorithm(null);
} else {
sslParameters.setEndpointIdentificationAlgorithm(endpointIdentificationAlgorithm);
}
sslHandler.engine().setSSLParameters(sslParameters);
```
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -47,20 +47,20 @@
public class PinotControllerTransport {
private static final Logger LOGGER =
LoggerFactory.getLogger(PinotControllerTransport.class);
+ private static final SslContextProvider SSL_CONTEXT_PROVIDER =
SslContextProviderFactory.create();
Map<String, String> _headers;
private final String _scheme;
private final AsyncHttpClient _httpClient;
public PinotControllerTransport(Map<String, String> headers, String scheme,
@Nullable SSLContext sslContext,
- ConnectionTimeouts connectionTimeouts, TlsProtocols tlsProtocols,
@Nullable String appId) {
+ ConnectionTimeouts connectionTimeouts, TlsProtocols tlsProtocols,
@Nullable String appId,
+ String endpointIdentificationAlgorithm) {
_headers = headers;
_scheme = scheme;
DefaultAsyncHttpClientConfig.Builder builder = Dsl.config();
- if (sslContext != null) {
- builder.setSslContext(new JdkSslContext(sslContext, true,
ClientAuth.OPTIONAL));
- }
+ SSL_CONTEXT_PROVIDER.configure(builder, sslContext, tlsProtocols,
endpointIdentificationAlgorithm);
Review Comment:
The public `PinotControllerTransport` constructor signature changed by
adding `endpointIdentificationAlgorithm`, which is a source/binary breaking
change for any downstream code instantiating this transport directly. Consider
adding an overloaded constructor with the previous signature that delegates to
this one with a default (e.g., empty string / null) to preserve compatibility.
--
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]
