Add support for HttpProtocolOptions - #6843
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds XDS proto support annotations, parses ChangesHttpProtocolOptions session protocol wiring
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterStream
participant ExtensionRegistry
participant ClusterFilterFactory
participant ClusterSnapshot
participant Server as Armeria Server
ClusterStream->>ClusterStream: read typed_extension_protocol_options
ClusterStream->>ExtensionRegistry: unpack HttpProtocolOptions from Any
ClusterStream->>ClusterFilterFactory: new ClusterFilterFactory(loadBalancer, httpProtocolOptions, transportSocket)
ClusterStream->>ClusterSnapshot: new ClusterSnapshot(..., clusterFilterFactory)
ClusterSnapshot->>ClusterFilterFactory: httpPreprocessor()
ClusterFilterFactory->>ClusterFilterFactory: sessionProtocol(tls, httpProtocolOptions)
ClusterSnapshot->>Server: preprocessor output for request
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/HttpProtocolOptionsTest.java (1)
63-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared test scaffolding to reduce duplication.
BOOTSTRAP_TEMPLATE/sessionProtocolSelectionandTLS_BOOTSTRAP_TEMPLATE/tlsSessionProtocolSelectionduplicate nearly the entire YAML template and test body, differing only in thetransport_socketblock and port. Consider parameterizing a single template/method with an optional TLS block to avoid the duplication.Also applies to: 136-211
🤖 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/HttpProtocolOptionsTest.java` around lines 63 - 133, The `sessionProtocolSelection` and `tlsSessionProtocolSelection` tests duplicate the same bootstrap YAML and request/assertion flow, differing mainly in the `transport_socket` block and port. Refactor the shared setup into one parameterized test (or shared helper) in `HttpProtocolOptionsTest`, and make `BOOTSTRAP_TEMPLATE`/`TLS_BOOTSTRAP_TEMPLATE` a single template that accepts an optional TLS block so both cases reuse the same `XdsResourceReader`, `XdsBootstrap`, `XdsHttpPreprocessor`, and client execution path.
🤖 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.
Inline comments:
In
`@xds-api/src/main/proto/envoy/extensions/upstreams/http/v3/http_protocol_options.proto`:
- Around line 156-165: Add support for the oneof arm UseDownstreamHttpConfig in
the HTTP protocol options handling, since it is currently only warned/rejected
and not respected by ClusterFilterFactory.sessionProtocol(). Update the logic
that parses the HttpProtocolOptions oneof in the relevant validator/index and
cluster filter code so this arm is accepted and mapped to the downstream
protocol behavior instead of falling back to HTTP/HTTPS; reference the oneof in
HttpProtocolOptions, DefaultXdsValidatorIndex, StrictXdsValidatorIndex, and
ClusterFilterFactory.sessionProtocol() when implementing the change.
In `@xds/src/main/java/com/linecorp/armeria/xds/ClusterStream.java`:
- Around line 112-129: In ClusterStream.parseHttpProtocolOptions, invalid or
mismatched Any values can still throw IllegalArgumentException from
context.extensionRegistry().unpack() and break snapshot creation. Update the
parsing flow to catch that failure around the unpack of
HTTP_PROTOCOL_OPTIONS_KEY and either return null as if the option is missing or
convert it into a targeted validation error, so the exception does not escape
the lbStream mapping in ClusterStream.
---
Nitpick comments:
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/HttpProtocolOptionsTest.java`:
- Around line 63-133: The `sessionProtocolSelection` and
`tlsSessionProtocolSelection` tests duplicate the same bootstrap YAML and
request/assertion flow, differing mainly in the `transport_socket` block and
port. Refactor the shared setup into one parameterized test (or shared helper)
in `HttpProtocolOptionsTest`, and make
`BOOTSTRAP_TEMPLATE`/`TLS_BOOTSTRAP_TEMPLATE` a single template that accepts an
optional TLS block so both cases reuse the same `XdsResourceReader`,
`XdsBootstrap`, `XdsHttpPreprocessor`, and client execution path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1410acd1-dcd6-4c52-adfd-2ea36b31aa2f
📒 Files selected for processing (6)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/HttpProtocolOptionsTest.javaxds-api/src/main/proto/envoy/config/cluster/v3/cluster.protoxds-api/src/main/proto/envoy/extensions/upstreams/http/v3/http_protocol_options.protoxds/src/main/java/com/linecorp/armeria/xds/ClusterFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterStream.java
Contributor
jrhee17
marked this pull request as ready for review
July 8, 2026 01:25
|
Tick the box to add this pull request to the merge queue (same as
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6843 +/- ##
============================================
- Coverage 74.46% 0 -74.47%
============================================
Files 1963 0 -1963
Lines 82437 0 -82437
Branches 10764 0 -10764
============================================
- Hits 61385 0 -61385
+ Misses 15918 0 -15918
+ Partials 5134 0 -5134 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
minwoox
approved these changes
Jul 9, 2026
| final ClusterFilterFactory factory = new ClusterFilterFactory(loadBalancer, transportSocket); | ||
| httpPreprocessor = factory.httpPreprocessor(); | ||
| rpcPreprocessor = factory.rpcPreprocessor(); | ||
| this.clusterFilterFactory = clusterFilterFactory; |
Contributor
There was a problem hiding this comment.
Do we need to add this to toString, hashCode, equals?
Contributor
Author
There was a problem hiding this comment.
Fields in clusterFilterFactory are XdsLoadBalancer, HttpProtocolOptions, TransportSocketSnapshot.
All of these are derived from the raw resources, or are already part of the equality/hashcode contract.
I'm not sure if adding toString would add much value
| } | ||
| return lbStream.map(lb -> new ClusterSnapshot( | ||
| resource, lb, input.transportSocket, input.transportSocketMatches)); | ||
| final HttpProtocolOptions httpProtocolOptions = parseHttpProtocolOptions(resource.resource()); |
Contributor
There was a problem hiding this comment.
nit: can call this at line 92 not to parse the resource multiple time.
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
3 participants
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:
When a cluster configures
typed_extension_protocol_optionswithenvoy.extensions.upstreams.http.v3.HttpProtocolOptions, Envoy uses theExplicitHttpConfigorAutoHttpConfigto determine which HTTP version to use for upstream connections. Previously, Armeria's xDS client ignored this configuration and always defaulted toHTTP/HTTPSbased solely on whether TLS was enabled, which meant clusters that explicitly declared HTTP/2 or HTTP/1 were not honored.Modifications:
HttpProtocolOptionsfrom the cluster'styped_extension_protocol_optionsmap inClusterStreamand pass it intoClusterFilterFactory.SessionProtocolfrom the parsed options inClusterFilterFactory:explicit_http_configwithhttp2_protocol_options→H2/H2Cexplicit_http_configwithhttp_protocol_options→H1/H1Cauto_configor absent →HTTPS/HTTP(existing default behavior)Result:
HttpProtocolOptionsfrom cluster configuration and selects the correctSessionProtocol(H1,H1C,H2,H2C,HTTP,HTTPS) accordingly.