Allow users to set ClientTlsSpec to RequestOptions - #6551
Conversation
WalkthroughAdds per-request TLS configuration (ClientTlsSpec): new APIs on request options, builders, and preparation classes; stored and propagated in ClientRequestContext and DefaultClientRequestContext; HttpClientDelegate now consults the context for a per-request TLS override; comprehensive integration tests added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Prep as RequestPreparation
participant ReqOpts as RequestOptions
participant ReqCtx as ClientRequestContext
participant HttpDel as HttpClientDelegate
participant TLSProv as TLSProvider
Client->>Prep: call clientTlsSpec(spec)
Prep->>ReqOpts: requestOptionsBuilder.clientTlsSpec(spec)
ReqOpts->>ReqOpts: store clientTlsSpec
Client->>Prep: execute request
Prep->>ReqCtx: create/init context from RequestOptions
ReqCtx->>ReqCtx: setClientTlsSpec(from options)
HttpDel->>ReqCtx: clientTlsSpec()
alt per-request spec present
ReqCtx-->>HttpDel: return spec
HttpDel->>HttpDel: set ALPN to session protocol
HttpDel->>TLSProv: use provided ClientTlsSpec to establish connection
else no per-request spec
ReqCtx-->>HttpDel: null
HttpDel->>TLSProv: build default spec (hostname/SNI, ALPN)
end
TLSProv-->>HttpDel: resolved TLS configuration
HttpDel->>TLSProv: establish TLS connection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.java⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)core/src/test/java/com/linecorp/armeria/client/RequestOptionsTest.java (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). (14)
🔇 Additional comments (6)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6551 +/- ##
============================================
- Coverage 74.46% 74.25% -0.21%
- Complexity 22234 23548 +1314
============================================
Files 1963 2116 +153
Lines 82437 88274 +5837
Branches 10764 11571 +807
============================================
+ Hits 61385 65550 +4165
- Misses 15918 17206 +1288
- Partials 5134 5518 +384 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/linecorp/armeria/client/RequestOptionsBuilder.java (1)
55-68: MissingclientTlsSpecinitialization from inputRequestOptions.The constructor copies all fields from the input
RequestOptions(e.g.,responseTimeoutMillis,exchangeType,responseTimeoutMode), butclientTlsSpecis not copied. This will cause the TLS spec to be lost when creating a builder from existing options.🔎 Apply this diff to copy clientTlsSpec:
RequestOptionsBuilder(@Nullable RequestOptions options) { if (options != null) { responseTimeoutMillis = options.responseTimeoutMillis(); writeTimeoutMillis = options.writeTimeoutMillis(); maxResponseLength = options.maxResponseLength(); requestAutoAbortDelayMillis = options.requestAutoAbortDelayMillis(); final Map<AttributeKey<?>, Object> attrs = options.attrs(); if (!attrs.isEmpty()) { attributes = new HashMap<>(attrs); } exchangeType = options.exchangeType(); responseTimeoutMode = options.responseTimeoutMode(); + clientTlsSpec = options.clientTlsSpec(); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/src/main/java/com/linecorp/armeria/client/BlockingWebClientRequestPreparation.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/ClientRequestContextWrapper.java(2 hunks)core/src/main/java/com/linecorp/armeria/client/DefaultRequestOptions.java(5 hunks)core/src/main/java/com/linecorp/armeria/client/FutureTransformingRequestPreparation.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java(2 hunks)core/src/main/java/com/linecorp/armeria/client/RequestOptions.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/RequestOptionsBuilder.java(3 hunks)core/src/main/java/com/linecorp/armeria/client/RequestOptionsSetters.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/RestClientPreparation.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/TransformingRequestPreparation.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/WebClientRequestPreparation.java(1 hunks)core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java(4 hunks)core/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.java(1 hunks)scala/scala_2.13/src/main/scala/com/linecorp/armeria/client/scala/ScalaRestClientPreparation.scala(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/client/RequestOptions.javacore/src/main/java/com/linecorp/armeria/client/ClientRequestContext.javacore/src/main/java/com/linecorp/armeria/client/ClientRequestContextWrapper.javacore/src/main/java/com/linecorp/armeria/client/RequestOptionsSetters.javacore/src/main/java/com/linecorp/armeria/client/RequestOptionsBuilder.javacore/src/main/java/com/linecorp/armeria/client/TransformingRequestPreparation.javacore/src/main/java/com/linecorp/armeria/client/RestClientPreparation.javacore/src/main/java/com/linecorp/armeria/client/WebClientRequestPreparation.javacore/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.javacore/src/main/java/com/linecorp/armeria/client/FutureTransformingRequestPreparation.javacore/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.javacore/src/main/java/com/linecorp/armeria/client/DefaultRequestOptions.javacore/src/main/java/com/linecorp/armeria/client/BlockingWebClientRequestPreparation.javacore/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java
🧬 Code graph analysis (5)
core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java (6)
core/src/main/java/com/linecorp/armeria/client/BlockingWebClientRequestPreparation.java (1)
UnstableApi(55-596)core/src/main/java/com/linecorp/armeria/client/FutureTransformingRequestPreparation.java (1)
UnstableApi(51-433)core/src/main/java/com/linecorp/armeria/client/RestClientPreparation.java (1)
UnstableApi(50-329)core/src/main/java/com/linecorp/armeria/client/TransformingRequestPreparation.java (1)
UnstableApi(46-357)core/src/main/java/com/linecorp/armeria/client/ClientPreprocessorsBuilder.java (1)
UnstableApi(29-73)scala/scala_2.13/src/main/scala/com/linecorp/armeria/client/scala/ScalaRestClientPreparation.scala (1)
clientTlsSpec(267-270)
core/src/main/java/com/linecorp/armeria/client/ClientRequestContextWrapper.java (4)
core/src/main/java/com/linecorp/armeria/client/BlockingWebClientRequestPreparation.java (1)
UnstableApi(55-596)core/src/main/java/com/linecorp/armeria/client/FutureTransformingRequestPreparation.java (1)
UnstableApi(51-433)core/src/main/java/com/linecorp/armeria/client/RestClientPreparation.java (1)
UnstableApi(50-329)core/src/main/java/com/linecorp/armeria/client/TransformingRequestPreparation.java (1)
UnstableApi(46-357)
core/src/main/java/com/linecorp/armeria/client/FutureTransformingRequestPreparation.java (2)
core/src/main/java/com/linecorp/armeria/client/RestClientPreparation.java (1)
UnstableApi(50-329)core/src/main/java/com/linecorp/armeria/client/TransformingRequestPreparation.java (1)
UnstableApi(46-357)
scala/scala_2.13/src/main/scala/com/linecorp/armeria/client/scala/ScalaRestClientPreparation.scala (1)
core/src/main/java/com/linecorp/armeria/client/RestClientPreparation.java (1)
UnstableApi(50-329)
core/src/main/java/com/linecorp/armeria/client/DefaultRequestOptions.java (3)
core/src/main/java/com/linecorp/armeria/client/FutureTransformingRequestPreparation.java (1)
UnstableApi(51-433)core/src/main/java/com/linecorp/armeria/client/RestClientPreparation.java (1)
UnstableApi(50-329)core/src/main/java/com/linecorp/armeria/client/TransformingRequestPreparation.java (1)
UnstableApi(46-357)
⏰ 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)
- GitHub Check: Summary
🔇 Additional comments (12)
core/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.java (1)
1-376: LGTM!Comprehensive test coverage for the new per-request TLS configuration feature. The tests cover:
- Successful mTLS configuration
- Certificate trust failures
- Custom verifiers (noVerify, always-throwing, delegating)
- Verifier chaining and invocation order
- ALPN protocol override behavior
- Context-based TLS spec mutation
- Connection pool keying with different TLS specs
The test structure is clean and the helper classes are appropriately scoped as private.
core/src/main/java/com/linecorp/armeria/client/RequestOptions.java (1)
125-131: LGTM!The new
clientTlsSpec()method is correctly added to theRequestOptionsinterface with proper annotations (@UnstableApi,@Nullable) and documentation. This aligns with the existing patterns in the interface (e.g.,responseTimeoutMode()).core/src/main/java/com/linecorp/armeria/client/RequestOptionsSetters.java (1)
168-175: LGTM!The new
clientTlsSpecmethod follows the established pattern of other setters in this interface. The@UnstableApiannotation is consistent with similar methods likeexchangeType, and the Javadoc clearly explains the fallback behavior.core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java (1)
268-273: LGTM!The per-request TLS spec override logic is correctly implemented. The code properly prioritizes the request-level
clientTlsSpecover factory defaults, and correctly sets ALPN protocols based on the session protocol. This aligns with the PR objectives that statealpnProtocolsshould be "set based on theSessionProtocol".scala/scala_2.13/src/main/scala/com/linecorp/armeria/client/scala/ScalaRestClientPreparation.scala (1)
266-270: LGTM!The new
clientTlsSpecoverride follows the established delegation pattern used by all other methods in this class. Since the class itself is annotated with@UnstableApi, no additional annotation is required on this method. As per coding guidelines,@UnstableApiis not required when the class is already annotated.core/src/main/java/com/linecorp/armeria/client/RestClientPreparation.java (1)
322-328: LGTM!The new
clientTlsSpecmethod correctly delegates to the underlyingWebClientRequestPreparationand returnsthisfor method chaining. The pattern is consistent with all other setter methods in this class.core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java (1)
185-186: Field initialization looks correct.The
clientTlsSpecfield is properly declared as@Nullableand initialized fromrequestOptions.clientTlsSpec()during construction, which aligns with how other request options are handled.Also applies to: 287-287
core/src/main/java/com/linecorp/armeria/client/WebClientRequestPreparation.java (1)
406-411: LGTM!The new
clientTlsSpecmethod correctly delegates to theRequestOptionsBuilderand follows the same pattern as other request option setters in this class. The@UnstableApiannotation is appropriately present sinceWebClientRequestPreparationis not itself annotated with@UnstableApi. As per coding guidelines, new public methods in non-annotated classes require this annotation.core/src/main/java/com/linecorp/armeria/client/RequestOptionsBuilder.java (1)
151-156: LGTM!The
clientTlsSpecmethod correctly validates input withrequireNonNull, is properly annotated with@UnstableApi, and follows the builder pattern consistently with other setters in this class.core/src/main/java/com/linecorp/armeria/client/BlockingWebClientRequestPreparation.java (1)
341-346: LGTM!The
clientTlsSpecmethod correctly follows the established delegation pattern used throughout this class, with proper@UnstableApiannotation for API stability signaling.core/src/main/java/com/linecorp/armeria/client/DefaultRequestOptions.java (2)
103-107: LGTM!The
clientTlsSpecaccessor is properly annotated with@UnstableApiand follows the same pattern as other nullable getters in this class.
126-127: LGTM!The
equals()method correctly usesObjects.equals()for the nullableclientTlsSpecfield, which properly handles null comparisons.
3 participants
Motivation:
This PR allows users to specify
ClientTlsSpecfor each request usingRequestOptions#clientTlsSpecorClientRequestContext#setClientTlsSpec.tlsCustomizer,alpnProtocolsare not allowed to be specified by users, and are set based on theSessionProtocolandClientFactorywhen reaching theHttpClientDelegateI'm unsure whether client-level APIs (
AbstractClientOptionsBuilder) will end up using theTlsProvider-style API, orClientTlsSpec-style API. For this iteration, only request-level constructs can specifyClientTlsSpec.Modifications:
RequestOptions#clientTlsSpec,ClientRequestContext#setClientTlsSpecto allow users to specifyClientTlsSpecHttpClientDelegatesetstlsCustomizer,alpnProtocolsbefore finalizing theClientTlsSpecResult:
ClientTlsSpecfor each request usingRequestOptions#clientTlsSpecorClientRequestContext#setClientTlsSpec