Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #3764:
URL: https://github.com/apache/celeborn/pull/3764#discussion_r3636486565


##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClientFactory.java:
##########
@@ -306,7 +306,9 @@ private TransportClient internalCreateClient(
         new ChannelInitializer<SocketChannel>() {
           @Override
           public void initChannel(SocketChannel ch) {
-            TransportChannelHandler clientHandler = context.initializePipeline(ch, decoder, true);
+            TransportChannelHandler clientHandler =
+                context.initializePipeline(
+                    ch, decoder, true, address.getHostString(), address.getPort());
             clientRef.set(clientHandler.getClient());

Review Comment:
   The new SNI behavior (threading peer host/port into SSLEngine creation) is not covered by tests, so regressions (e.g., accidentally dropping peerHost/peerPort in the call chain) would be hard to catch. Consider adding a focused test in the existing SSL test suites (e.g., common/src/test/java/.../ssl/SslConnectivitySuiteJ) that verifies client-side engines are created with the expected peer host/port (engine.getPeerHost()/getPeerPort()), and/or that SNI is present during handshake if you have a way to observe it on the server side.



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org