You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "shellfish007 (via GitHub)" <gi...@apache.org> on 2026/07/23 02:11:45 UTC
[PR] Pass peer hostname to SSLEngine for SNI support [celeborn]
shellfish007 opened a new pull request, #3764:
URL: https://github.com/apache/celeborn/pull/3764
### What changes were proposed in this pull request?
Thread peer hostname/port from `TransportClientFactory.internalCreateClient()` through `TransportContext.initializePipeline()` to `SSLFactory.createSSLEngine()`. `SSLFactory.createEngine()` now calls `jdkSslContext.createSSLEngine(peerHost, peerPort)` for client connections, which makes the JDK include the SNI extension in the TLS ClientHello. Existing server-side and no-arg callers use backward-compatible overloads.
### Why are the changes needed?
`SSLFactory.createEngine()` called `jdkSslContext.createSSLEngine()` with no arguments. The JDK only includes the SNI extension in the TLS ClientHello when a peer hostname is provided via `createSSLEngine(host, port)`. Without SNI, TLS termination/routing that depends on server name (e.g. an SNI-based ingress/gateway such as Istio TLS PASSTHROUGH or SIMPLE mode) cannot route or differentiate connections by hostname. This fix is a prerequisite for shared-port multi-tenant routing through an SNI-based ingress gateway.
### Does this PR introduce _any_ user-facing change?
No. Client `SSLEngine` creation now includes SNI; server-side and existing no-arg callers are unaffected.
### How was this patch tested?
Follows the existing `SSLFactory`/`TransportContext` call patterns with additive, backward-compatible overloads — no new config surface. Relies on CI for build/test verification.
--
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
Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]
Posted by "zaynt4606 (via GitHub) via issues" <is...@celeborn.apache.org>.
zaynt4606 closed pull request #3764: [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support
URL: https://github.com/apache/celeborn/pull/3764
--
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
Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]
Posted by "shellfish007 (via GitHub) via issues" <is...@celeborn.apache.org>.
shellfish007 commented on code in PR #3764:
URL: https://github.com/apache/celeborn/pull/3764#discussion_r4427780608
##########
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:
Thanks for the suggestion. Added `SslConnectivitySuiteJ.testSniHostNameReachesServer`. It starts a TLS server, connects to it by host name using `TransportClientFactory` (both pooled and unmanaged clients, over TLS 1.2 and TLS 1.3), and asserts that the server receives that host name as SNI via `ExtendedSSLSession.getRequestedServerNames()`.
--
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
Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]
Posted by "zaynt4606 (via GitHub)" <gi...@apache.org>.
zaynt4606 commented on code in PR #3764:
URL: https://github.com/apache/celeborn/pull/3764#discussion_r4256036031
##########
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:
Non-blocking suggestion: the new `SSLFactorySuiteJ` tests cover the engine configuration, but they call `SSLFactory` directly. Could we also add a test in `SslConnectivitySuiteJ` that connects through `TransportClientFactory` using a DNS hostname and asserts the SNI observed by the server, for example via `ExtendedSSLSession.getRequestedServerNames()`? This would catch regressions where `peerHost` or `peerPort` is dropped along the `TransportClientFactory` -> `TransportContext` -> `SSLFactory` call chain.
I verified the current implementation locally with both TLS 1.2 and TLS 1.3, using pooled and unmanaged connections; the expected hostname reaches the server. This suggestion is for regression coverage and is non-blocking.
--
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
Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]
Posted by "zaynt4606 (via GitHub) via issues" <is...@celeborn.apache.org>.
zaynt4606 commented on PR #3764:
URL: https://github.com/apache/celeborn/pull/3764#issuecomment-6542255143
Thanks @shellfish007 for the contribution!
Merged to main(v1.0.0).
--
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
Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3764:
URL: https://github.com/apache/celeborn/pull/3764#issuecomment-5422850055
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.
--
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
Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]
Posted by "zaynt4606 (via GitHub) via issues" <is...@celeborn.apache.org>.
zaynt4606 commented on PR #3764:
URL: https://github.com/apache/celeborn/pull/3764#issuecomment-6542244205
**Merge Summary:**
- merged into main https://github.com/apache/celeborn/commit/9ae2834e
*Posted by `merge_pr.py`*
--
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
Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]
Posted by "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
Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3764:
URL: https://github.com/apache/celeborn/pull/3764#issuecomment-5711480560
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.
--
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
Re: [PR] [CELEBORN-2386] Pass peer hostname to SSLEngine for SNI support [celeborn]
Posted by "codecov[bot] (via GitHub) via issues" <is...@celeborn.apache.org>.
codecov[bot] commented on PR #3764:
URL: https://github.com/apache/celeborn/pull/3764#issuecomment-6540670803
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/3764?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 61.63%. Comparing base ([`04ec937`](https://app.codecov.io/gh/apache/celeborn/commit/04ec937ff0b86b952d17b90a14566a4f07caf6b9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e6444da`](https://app.codecov.io/gh/apache/celeborn/commit/e6444da5844e34365690edcd309654c33f1ddefc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 23 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #3764 +/- ##
============================================
+ Coverage 61.09% 61.63% +0.54%
- Complexity 346 425 +79
============================================
Files 404 404
Lines 28670 28790 +120
Branches 2828 2841 +13
============================================
+ Hits 17513 17741 +228
+ Misses 9823 9672 -151
- Partials 1334 1377 +43
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/celeborn/pull/3764?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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