Support for Envoy gRPC HTTP/1.1 bridge#6689
Merged
Conversation
kwondh5217
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
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 (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds an Envoy gRPC HTTP/1.1 bridge toggle to the gRPC builder, propagates it through FramedGrpcService into server-call constructors, and updates UnaryServerCall to optionally merge gRPC trailers into HTTP/1 response headers; includes tests covering bridge behavior across protocols, call types, and transcoding. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
grpc/src/test/java/com/linecorp/armeria/server/grpc/EnvoyHttp1BridgeTest.java (1)
105-107: Add one explicit HTTP/2 regression assertion for bridge-enabled unary calls.Current tests validate HTTP/1.1 behavior well, but a dedicated HTTP/2 case would lock in the “HTTP/2 unaffected” objective and prevent accidental regressions.
Suggested test addition
+ private static BlockingWebClient h2cClient(ServerExtension server) { + return WebClient.of(server.uri(SessionProtocol.H2C)).blocking(); + } + + `@Test` + void bridgeNotAppliedOverHttp2() { + final BlockingWebClient client = h2cClient(bridgeServer); + final AggregatedHttpResponse response = + client.prepare() + .post(TestServiceGrpc.getEmptyCallMethod().getFullMethodName()) + .content(GrpcSerializationFormats.PROTO.mediaType(), EMPTY_GRPC_FRAME) + .execute(); + + assertThat(response.status()).isEqualTo(HttpStatus.OK); + assertThat(response.headers().get(GrpcHeaderNames.GRPC_STATUS)).isNull(); + assertThat(response.trailers().get(GrpcHeaderNames.GRPC_STATUS)).isEqualTo("0"); + }Also applies to: 109-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc/src/test/java/com/linecorp/armeria/server/grpc/EnvoyHttp1BridgeTest.java` around lines 105 - 107, Add a focused HTTP/2 regression test in EnvoyHttp1BridgeTest to assert that bridge-enabled unary calls remain HTTP/2: create a helper analogous to h1cClient (e.g., h2cClient using WebClient.of(server.uri(SessionProtocol.H2C)).blocking()) and add a test method that uses that client to invoke the same unary endpoint exercised by the HTTP/1.1 tests and assert the response uses HTTP/2 and the payload/headers match expectations; place the test near the existing unary tests (the block around h1cClient and tests at lines 109-169) so it runs alongside the current cases and catches regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java`:
- Around line 654-657: The new public builder method enableEnvoyHttp1Bridge in
class GrpcServiceBuilder must be annotated with `@UnstableApi`; add the annotation
above the method declaration and ensure the corresponding import
(com.linecorp.armeria.common.annotation.UnstableApi) is added to the file so the
method follows the repository rule for newly added public APIs.
---
Nitpick comments:
In
`@grpc/src/test/java/com/linecorp/armeria/server/grpc/EnvoyHttp1BridgeTest.java`:
- Around line 105-107: Add a focused HTTP/2 regression test in
EnvoyHttp1BridgeTest to assert that bridge-enabled unary calls remain HTTP/2:
create a helper analogous to h1cClient (e.g., h2cClient using
WebClient.of(server.uri(SessionProtocol.H2C)).blocking()) and add a test method
that uses that client to invoke the same unary endpoint exercised by the
HTTP/1.1 tests and assert the response uses HTTP/2 and the payload/headers match
expectations; place the test near the existing unary tests (the block around
h1cClient and tests at lines 109-169) so it runs alongside the current cases and
catches regressions.
🪄 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: 9b10404b-50c1-4341-a506-8faf5c8e16d8
📒 Files selected for processing (6)
grpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/UnaryServerCall.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/DeferredListenerTest.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/EnvoyHttp1BridgeTest.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/UnaryServerCallTest.java
grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java
Show resolved
Hide resolved
Contributor
jrhee17
reviewed
Mar 31, 2026
Contributor
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Looks good overall, left a minor comment
grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java
Show resolved
Hide resolved
grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java
Show resolved
Hide resolved
jrhee17
reviewed
Apr 1, 2026
grpc/src/test/java/com/linecorp/armeria/server/grpc/EnvoyHttp1BridgeTest.java
Show resolved
Hide resolved
ikhoon
reviewed
Apr 1, 2026
grpc/src/test/java/com/linecorp/armeria/server/grpc/EnvoyHttp1BridgeTest.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
grpc/src/test/java/com/linecorp/armeria/server/grpc/EnvoyHttp1BridgeTest.java (1)
151-240: Consider extracting a small request helper to reduce repetition.The same
client.prepare().post(...).content(...).execute()flow appears multiple times. A local helper would make future additions/edits less error-prone.Proposed refactor
+ private static AggregatedHttpResponse postGrpcEmptyFrame(BlockingWebClient client, String methodPath) { + return client.prepare() + .post(methodPath) + .content(GrpcSerializationFormats.PROTO.mediaType(), EMPTY_GRPC_FRAME) + .execute(); + } ... - final AggregatedHttpResponse response = - client.prepare() - .post(TestServiceGrpc.getEmptyCallMethod().getFullMethodName()) - .content(GrpcSerializationFormats.PROTO.mediaType(), EMPTY_GRPC_FRAME) - .execute(); + final AggregatedHttpResponse response = + postGrpcEmptyFrame(client, TestServiceGrpc.getEmptyCallMethod().getFullMethodName());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc/src/test/java/com/linecorp/armeria/server/grpc/EnvoyHttp1BridgeTest.java` around lines 151 - 240, Multiple tests repeat the client.prepare().post(...).content(...).execute() flow; extract a small helper (e.g., sendPost(BlockingWebClient client, String pathOrMethod, MediaType contentType, byte[] body) or an overloaded sendPost for String bodies) and replace the repeated chains in tests like trailersAreMergedIntoHeadersForUnaryOverHttp1, grpcStatusInHeadersOnErrorOverHttp1, bridgeNotAppliedForStreamingMethod, bridgeNotAppliedWhenDisabled, bridgeNotAppliedOverHttp2 and jsonTranscodingNotAffectedByBridge to call this helper to reduce duplication and make future edits safer. Ensure the helper returns AggregatedHttpResponse and supports both GRPC method full names and regular URI paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@grpc/src/test/java/com/linecorp/armeria/server/grpc/EnvoyHttp1BridgeTest.java`:
- Around line 151-240: Multiple tests repeat the
client.prepare().post(...).content(...).execute() flow; extract a small helper
(e.g., sendPost(BlockingWebClient client, String pathOrMethod, MediaType
contentType, byte[] body) or an overloaded sendPost for String bodies) and
replace the repeated chains in tests like
trailersAreMergedIntoHeadersForUnaryOverHttp1,
grpcStatusInHeadersOnErrorOverHttp1, bridgeNotAppliedForStreamingMethod,
bridgeNotAppliedWhenDisabled, bridgeNotAppliedOverHttp2 and
jsonTranscodingNotAffectedByBridge to call this helper to reduce duplication and
make future edits safer. Ensure the helper returns AggregatedHttpResponse and
supports both GRPC method full names and regular URI paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b45505fc-b2eb-4b01-b368-0bdc1ac50494
📒 Files selected for processing (2)
grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/EnvoyHttp1BridgeTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java
ikhoon
approved these changes
Apr 2, 2026
Signed-off-by: Daeho Kwon <trewq231@naver.com>
Signed-off-by: Daeho Kwon <trewq231@naver.com>
kwondh5217
force-pushed
the
envoy-http1-bridge
branch
from
d2e7941 to
370c2be
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6689 +/- ##
============================================
- Coverage 74.46% 73.97% -0.49%
- Complexity 22234 23997 +1763
============================================
Files 1963 2170 +207
Lines 82437 90034 +7597
Branches 10764 11788 +1024
============================================
+ Hits 61385 66601 +5216
- Misses 15918 17860 +1942
- Partials 5134 5573 +439 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
4 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 serving framed gRPC over HTTP/1.1, Armeria delivers gRPC trailers via chunked transfer-encoding.
Some legacy L7 proxies cannot handle chunked trailers correctly, which breaks gRPC traffic.
The Envoy gRPC HTTP/1.1 bridge specification addresses this by merging gRPC trailers into response headers, producing a plain HTTP/1.1 response that legacy proxies can handle.
Modifications:
enableEnvoyHttp1Bridge(boolean)option toGrpcServiceBuilderFramedGrpcServicedown toUnaryServerCallUnaryServerCall.doClose(), merge gRPC trailers into responseheaders when the bridge is enabled and the connection is HTTP/1.1
to the existing chunked transfer-encoding behavior
EnvoyHttp1BridgeTestto verify the behaviorResult:
enableEnvoyHttp1Bridge(true)is set, unary gRPC responses overHTTP/1.1 include
grpc-statusand other trailers in the responseheaders instead of chunked trailers, improving compatibility with
legacy proxy servers. HTTP/2 connections are unaffected.