transport-netty4 module set TLS SNI when server_name is provided #20321
Conversation
Signed-off-by: Orlando AGESOKO <[email protected]>
📝 WalkthroughWalkthroughAdds SNI/server_name propagation to the Netty4 client TLS flow: a serverName field and constructor are added to ClientSSLHandler, node Changes
Sequence Diagram(s)sequenceDiagram
participant Node as Discovered Node
participant Netty as Netty4 Transport
participant Init as SSLClientChannelInitializer
participant Handler as ClientSSLHandler
participant Provider as SecureTransportSettingsProvider
participant SSLEngine as SSLEngine
Node->>Netty: initiate connection (hostname, port, attributes incl. server_name)
Netty->>Init: initialize channel with node info
Init->>Handler: construct ClientSSLHandler(..., serverName)
Handler->>Provider: buildSecureClientTransportEngine(settings, serverName?, hostname, port)
Provider-->>Handler: Optional<SSLEngine>
Handler->>SSLEngine: configure SNI via SSLParameters if serverName present
Handler->>Netty: attach configured SSLEngine to pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java (2)
191-216: Consider constructor chaining to reduce duplication.Both constructors assign the same four fields independently. The original constructor could delegate to the new one to reduce duplication.
🔎 Proposed refactor using constructor chaining
private ClientSSLHandler( final Settings settings, final SecureTransportSettingsProvider secureTransportSettingsProvider, final boolean hostnameVerificationEnabled, final boolean hostnameVerificationResovleHostName ) { - this.settings = settings; - this.secureTransportSettingsProvider = secureTransportSettingsProvider; - this.hostnameVerificationEnabled = hostnameVerificationEnabled; - this.hostnameVerificationResovleHostName = hostnameVerificationResovleHostName; - this.serverName = null; + this(settings, secureTransportSettingsProvider, hostnameVerificationEnabled, hostnameVerificationResovleHostName, null); }
267-273: SNI configuration logic is correct; consider adding debug logging.The implementation correctly applies SNI to the SSLEngine when a server name is provided. The logic flow is sound: retrieve SSLParameters, add SNIHostName, and apply back to the engine.
Adding debug logging when SNI is configured would aid troubleshooting TLS connection issues.
🔎 Suggested logging addition
if (serverName != null) { + if (log.isDebugEnabled()) { + log.debug("Setting TLS SNI server name to: {}", serverName); + } SSLParameters params = sslEngine.getSSLParameters(); List<SNIServerName> serverNames = new ArrayList<>(1); serverNames.add(new SNIHostName(serverName)); params.setServerNames(serverNames); sslEngine.setSSLParameters(params); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4Http3ServerTransport.java:101-123
Timestamp: 2025-12-13T20:16:15.318Z
Learning: In OpenSearch, only one HTTP transport implementation can be active and loaded at a time, so duplicate setting definitions (such as h3.max_stream_local_length, h3.max_stream_remote_length, and h3.max_streams) across different transport implementations like Netty4Http3ServerTransport and ReactorNetty4HttpServerTransport will not cause setting registration conflicts.
📚 Learning: 2025-12-19T21:26:37.090Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4QuicServerTransport.java:0-0
Timestamp: 2025-12-19T21:26:37.090Z
Learning: In Netty4QuicServerTransport (modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4QuicServerTransport.java), the secureHttpTransportSettingsProvider parameter is guaranteed to be non-null because the plugin verifies its presence during instantiation, so no explicit null-check is needed in the constructor.
Applied to files:
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java
📚 Learning: 2025-12-12T18:40:08.452Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java
📚 Learning: 2025-12-12T13:31:51.234Z
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:51.234Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
Applied to files:
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java
📚 Learning: 2025-12-13T20:16:15.318Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4Http3ServerTransport.java:101-123
Timestamp: 2025-12-13T20:16:15.318Z
Learning: In OpenSearch, only one HTTP transport implementation can be active and loaded at a time, so duplicate setting definitions (such as h3.max_stream_local_length, h3.max_stream_remote_length, and h3.max_streams) across different transport implementations like Netty4Http3ServerTransport and ReactorNetty4HttpServerTransport will not cause setting registration conflicts.
Applied to files:
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java
📚 Learning: 2025-12-19T21:16:42.541Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.java:138-154
Timestamp: 2025-12-19T21:16:42.541Z
Learning: In Netty4HttpChannel.java (modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.java), the recursive traversal of parent channels in getAddressFromParent() is safe because Netty enforces a hard limit on the depth of channel parent chains, preventing unbounded recursion and potential StackOverflowError. When reviewing similar code, rely on the library's depth limits rather than adding custom recursion guards. If future changes remove Netty's bound, consider adding an explicit depth limit or converting to an iterative traversal with a guard.
Applied to files:
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: gradle-check
🔇 Additional comments (2)
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java (2)
53-54: LGTM! Necessary imports for SNI support.The added imports (SNIHostName, SNIServerName, SSLParameters, ArrayList, List) are all required for implementing the TLS SNI functionality.
Also applies to: 57-57, 63-64
330-331: Theserver_nameattribute is correctly populated from thecluster.remote.<cluster_alias>.server_namesetting. InProxyConnectionStrategy.java, the configured value is read and added to the DiscoveryNode attributes map (line 294), allowingSecureNetty4Transportto retrieve it at line 331. The mechanism is validated by tests and includes proper null-handling in the SSL handler.
...ransport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java
Show resolved
Hide resolved
|
❌ Gradle check result for 6775bdb: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Orlando AGESOKO <[email protected]>
Signed-off-by: Orlando AGESOKO <[email protected]>
Signed-off-by: Orlando AGESOKO <[email protected]>
|
❌ Gradle check result for 848871a: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
entry pointed to Issue instead of PR Signed-off-by: Orlando AGESOKO <[email protected]>
|
❌ Gradle check result for 13d3a22: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Thanks @orages , the change is looking good, could you please resolve the conflicts and take all checks to passing state? thank you |
…-using-server-name-in-sni
|
❌ Gradle check result for 755a500: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Orlando AGESOKO <[email protected]>
|
❌ Gradle check result for edb90c1: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for edb90c1: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for edb90c1: TIMEOUT Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for edb90c1: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for edb90c1: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20321 +/- ##
============================================
- Coverage 73.30% 73.20% -0.10%
+ Complexity 71777 71676 -101
============================================
Files 5784 5784
Lines 328141 328151 +10
Branches 47269 47270 +1
============================================
- Hits 240531 240223 -308
- Misses 68329 68636 +307
- Partials 19281 19292 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Description
Populate the "ServerNames" SSL parameter when
cluster.remote.<cluster_alias>.server_nameis set (according to the documentation https://docs.opensearch.org/latest/install-and-configure/configuring-opensearch/cluster-settings/).Related Issues
Resolves #17316
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.