Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to contain te header in gRPC request #1965

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Aug 6, 2019

Motivation:
According to the spec,
gRPC Request-Headers has to contain te header in order to detect incompatible proxies.

Modification:

  • Add te header to gRPC request headers

Result:

Motivation:
According to the [spec](https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#requests),
gRPC `Request-Headers` has to conatin te header in order to detect incompatible proxies.

Modification:
- Add te header to gRPC request headers

Result:
- Fix line#1963
@minwoox minwoox added the defect label Aug 6, 2019
@minwoox minwoox added this to the 0.90.0 milestone Aug 6, 2019
@@ -79,7 +80,8 @@ public UnaryGrpcClient(HttpClient httpClient) {
public CompletableFuture<byte[]> execute(String uri, byte[] payload) {
final HttpRequest request = HttpRequest.of(
RequestHeaders.of(HttpMethod.POST, uri,
HttpHeaderNames.CONTENT_TYPE, "application/grpc+proto"),
HttpHeaderNames.CONTENT_TYPE, "application/grpc+proto",
HttpHeaderNames.TE, HttpHeaderValues.TRAILERS),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't add a test case for this because the upstream server does not include te header while converting request headers to MetaData here.

Copy link
Collaborator

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@minwoox
Copy link
Member Author

minwoox commented Aug 6, 2019

I also found out that the upstream metadata doesn't include the pseudo headers.
But we do have all pseudo headers and te. Do we need to follow the upstream's conversion strategy? @anuraaga

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@anuraaga
Copy link
Collaborator

anuraaga commented Aug 6, 2019

There's some context in grpc/grpc-java#5788

There isn't a clear definition of what headers should be in Metadata but you're right it's recommended to hide pseudoheaders and hop-by-hop ones. Since it's not defined and seems like an implementation detail, I just went with reduced complexity 😅

@codecov
Copy link

codecov bot commented Aug 6, 2019

Codecov Report

Merging #1965 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1965      +/-   ##
============================================
- Coverage     73.38%   73.34%   -0.05%     
+ Complexity     9103     9100       -3     
============================================
  Files           805      805              
  Lines         35474    35474              
  Branches       4355     4355              
============================================
- Hits          26032    26017      -15     
- Misses         7181     7209      +28     
+ Partials       2261     2248      -13
Impacted Files Coverage Δ Complexity Δ
.../armeria/common/grpc/protocol/UnaryGrpcClient.java 70% <ø> (ø) 5 <0> (ø) ⬇️
...m/linecorp/armeria/client/grpc/ArmeriaChannel.java 97.5% <100%> (ø) 8 <0> (ø) ⬇️
...rp/armeria/server/tomcat/ManagedTomcatService.java 50.84% <0%> (-22.04%) 6% <0%> (-1%)
...armeria/server/tomcat/Tomcat90ProtocolHandler.java 48.14% <0%> (-14.82%) 11% <0%> (-4%)
...om/linecorp/armeria/server/jetty/JettyService.java 63.05% <0%> (-2.96%) 23% <0%> (-1%)
.../linecorp/armeria/server/tomcat/TomcatService.java 63.29% <0%> (-2.66%) 22% <0%> (-1%)
.../com/linecorp/armeria/server/docs/ServiceInfo.java 70.73% <0%> (-2.44%) 16% <0%> (-1%)
...a/common/grpc/protocol/ArmeriaMessageDeframer.java 71.77% <0%> (+0.95%) 48% <0%> (+1%) ⬆️
.../main/java/com/linecorp/armeria/server/Server.java 82.43% <0%> (+1.35%) 17% <0%> (ø) ⬇️
...inecorp/armeria/server/grpc/ArmeriaServerCall.java 88.18% <0%> (+1.57%) 82% <0%> (+1%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15cdfd5...fedecea. Read the comment docs.

@trustin trustin merged commit 323c66e into line:master Aug 6, 2019
@trustin
Copy link
Member

trustin commented Aug 6, 2019

Thanks, @minwoox !

@minwoox minwoox deleted the fix_to_contain_te branch August 6, 2019 06:17
@minwoox
Copy link
Member Author

minwoox commented Aug 6, 2019

There's some context in grpc/grpc-java#5788

Ah, thanks! Yeah, it's not defined and I think it's fine as it is. :-)

@minwoox
Copy link
Member Author

minwoox commented Aug 6, 2019

Thanks for reviewing!

fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:
According to the [spec](https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#requests),
gRPC `Request-Headers` has to conatin te header in order to detect incompatible proxies.

Modification:
- Add te header to gRPC request headers

Result:
- Fix line#1963
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Armeria failed to call python gRPC server
4 participants