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

Remove content-length header #385

Merged
merged 1 commit into from
Apr 11, 2019
Merged

Conversation

LeonSha
Copy link
Contributor

@LeonSha LeonSha commented Apr 11, 2019

Changes

Move the patch from envoy proxy.
https://sourcegraph.com/github.com/envoyproxy/envoy@cfc514546bc0284536893cca5fa43d7128edcd35/-/blob/source/extensions/filters/http/grpc_web/grpc_web_filter.cc#L51

Verification

I tried the patch locally and check the content-length has been removed.

@johanbrandhorst
Copy link
Contributor

Thanks for this contribution! Is there a specific issue this fixes?

This seems to be the rationale:

// Remove content-length header since it represents http1.1 payload size, not the sum of the h2
// DATA frame payload lengths. https://http2.github.io/http2-spec/#malformed This effectively
// switches to chunked encoding which is the default for h2.

Is there an envoy issue related to this?

@johanbrandhorst johanbrandhorst self-requested a review April 11, 2019 10:05
@LeonSha
Copy link
Contributor Author

LeonSha commented Apr 11, 2019

Thanks for this contribution! Is there a specific issue this fixes?

This seems to be the rationale:

// Remove content-length header since it represents http1.1 payload size, not the sum of the h2
// DATA frame payload lengths. https://http2.github.io/http2-spec/#malformed This effectively
// switches to chunked encoding which is the default for h2.

Is there an envoy issue related to this?
envoyproxy/envoy#2946

@johanbrandhorst
Copy link
Contributor

OK, so the issue is if we have a daisy chain of proxies where this proxy will transform a HTTP/1.1 request to HTTP/2 but not remove the content-length header, then downstream proxies that receive a HTTP/2 request with a content-length header might be confused.

Thanks!

@johanbrandhorst
Copy link
Contributor

I'll merge as soon as travis has succeeded.

@johanbrandhorst johanbrandhorst merged commit e511d30 into improbable-eng:master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants