Add SessionProtocol.withTls()/withoutTls() and per-endpoint TLS/cleartext switching#6850
Draft
jrhee17 wants to merge 2 commits into
Draft
Add SessionProtocol.withTls()/withoutTls() and per-endpoint TLS/cleartext switching#6850jrhee17 wants to merge 2 commits into
SessionProtocol.withTls()/withoutTls() and per-endpoint TLS/cleartext switching#6850jrhee17 wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds ChangesClient TLS spec clear API and session protocol switching
xDS cluster session protocol simplification and integration tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClientRequestContext
participant SessionProtocol
participant ClientRequestContextWrapper
ClientRequestContext->>SessionProtocol: withoutTls()
ClientRequestContextWrapper->>ClientRequestContext: clearClientTlsSpec()
sequenceDiagram
participant ClusterStream
participant ClusterFilterFactory
participant RequestContext
ClusterStream->>ClusterFilterFactory: new ClusterFilterFactory(lb, httpProtocolOptions)
ClusterFilterFactory->>ClusterFilterFactory: sessionProtocol(httpProtocolOptions)
ClusterFilterFactory->>RequestContext: applyClusterSettings(endpoint)
ClusterFilterFactory->>RequestContext: clearClientTlsSpec() when absent
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/MixedTransportSocketRetryTest.java (1)
195-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting
base64Certto a shared utility.The
base64Certmethod is duplicated verbatim inMixedTransportSocketSessionProtocolTest. Consider extracting it (andXdsCertificateExtensionif co-located) to a shared test helper in the same package to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/MixedTransportSocketRetryTest.java` around lines 195 - 197, The `base64Cert` helper is duplicated in `MixedTransportSocketRetryTest` and `MixedTransportSocketSessionProtocolTest`; move it into a shared test utility in the same package and update both tests to call that shared helper. If `XdsCertificateExtension` belongs with it, co-locate that logic in the same helper so the duplicated certificate-loading code is centralized and easier to maintain.core/src/test/java/com/linecorp/armeria/internal/client/DefaultClientRequestContextTest.java (1)
447-552: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM on the new
setSessionProtocoltests — they're logically consistent with the implementation. One gap: this file doesn't directly testsetClientTlsSpec()/clearClientTlsSpec()driving the sessionProtocol TLS/cleartext switch (lines 1112-1126 inDefaultClientRequestContext.java), onlysetSessionProtocol()itself. Consider adding a test mirroring the existing pattern to cover that path directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/test/java/com/linecorp/armeria/internal/client/DefaultClientRequestContextTest.java` around lines 447 - 552, The new tests cover setSessionProtocol() directly, but they do not exercise the TLS/cleartext transition triggered by setClientTlsSpec() and clearClientTlsSpec(). Add a test in DefaultClientRequestContextTest that follows the existing init/update assertions and verifies that calling setClientTlsSpec() switches sessionProtocol() and URI/header scheme to HTTPS, and clearClientTlsSpec() switches them back to HTTP. Use the existing DefaultClientRequestContext, sessionProtocol(), uri(), and internalRequestHeaders() patterns to keep the coverage aligned with the current tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@core/src/test/java/com/linecorp/armeria/internal/client/DefaultClientRequestContextTest.java`:
- Around line 447-552: The new tests cover setSessionProtocol() directly, but
they do not exercise the TLS/cleartext transition triggered by
setClientTlsSpec() and clearClientTlsSpec(). Add a test in
DefaultClientRequestContextTest that follows the existing init/update assertions
and verifies that calling setClientTlsSpec() switches sessionProtocol() and
URI/header scheme to HTTPS, and clearClientTlsSpec() switches them back to HTTP.
Use the existing DefaultClientRequestContext, sessionProtocol(), uri(), and
internalRequestHeaders() patterns to keep the coverage aligned with the current
tests.
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/MixedTransportSocketRetryTest.java`:
- Around line 195-197: The `base64Cert` helper is duplicated in
`MixedTransportSocketRetryTest` and `MixedTransportSocketSessionProtocolTest`;
move it into a shared test utility in the same package and update both tests to
call that shared helper. If `XdsCertificateExtension` belongs with it, co-locate
that logic in the same helper so the duplicated certificate-loading code is
centralized and easier to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8870940b-b434-4f1e-96ba-66b192801121
📒 Files selected for processing (9)
core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.javacore/src/main/java/com/linecorp/armeria/client/ClientRequestContextWrapper.javacore/src/main/java/com/linecorp/armeria/common/SessionProtocol.javacore/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.javacore/src/test/java/com/linecorp/armeria/internal/client/DefaultClientRequestContextTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/MixedTransportSocketRetryTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/MixedTransportSocketSessionProtocolTest.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterStream.java
Contributor
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6850 +/- ##
============================================
+ Coverage 74.46% 75.05% +0.59%
- Complexity 22234 25326 +3092
============================================
Files 1963 2252 +289
Lines 82437 94112 +11675
Branches 10764 12307 +1543
============================================
+ Hits 61385 70636 +9251
- Misses 15918 17625 +1707
- Partials 5134 5851 +717 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
I found this bug while debugging the xDS integration with Istio.
When a cluster has mixed TLS and plaintext endpoints (via
transport_socket_matches), the session protocol must switch dynamically per endpoint. Previously,ClusterFilterFactorypicked a fixed session protocol at cluster level based on whether a defaulttransport_socketwas present. This meant all endpoints in a cluster used the same TLS/cleartext setting, which broke mixed clusters where some endpoints useUpstreamTlsContextand others useraw_buffer.Modifications:
SessionProtocol.withTls()andwithoutTls()methods that switch between TLS and non-TLS variants while preserving the HTTP version (e.g.,H2C↔H2,H1C↔H1,HTTP↔HTTPS).ClientRequestContext.setClientTlsSpec()andclearClientTlsSpec()methods.setClientTlsSpec()automatically switches the session protocol to its TLS variant;clearClientTlsSpec()switches to the non-TLS variant.ClusterFilterFactoryto always default to a TLS session protocol (based onHttpProtocolOptions), and defer per-endpoint TLS decisions to theapplyClusterSettingsdecorator which inspectsTransportSocketSnapshot: if the endpoint's transport socket has aClientTlsSpec, it is set; if it is araw_buffer(null spec), the TLS spec is cleared and the protocol switches to cleartext.transportSocketparameter from theClusterFilterFactoryconstructor since TLS is no longer decided at cluster level.MixedTransportSocketSessionProtocolTestverifying that a round-robin cluster with one TLS endpoint and one plaintext endpoint uses the correct session protocol for each.MixedTransportSocketRetryTestverifying that retry-created derived contexts also get the correct per-endpoint session protocol when retrying across mixed endpoints through real server connections.Result:
transport_socket_matchescontaining both TLS andraw_bufferentries now correctly switch between TLS and cleartext per endpoint.