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.

Sign up for GitHub

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

Jump to bottom

Proxy sets "Transfer-Encoding: chunked" when proxying GET request #402

Closed
klingerf opened this issue Feb 21, 2018 · 6 comments · Fixed by #410
Closed

Proxy sets "Transfer-Encoding: chunked" when proxying GET request #402

klingerf opened this issue Feb 21, 2018 · 6 comments · Fixed by #410
Assignees
@seanmonstar
Labels
area/proxy priority/P0 Release Blocker
Milestone
0.3.1

Comments

@klingerf
Copy link
Member

klingerf commented Feb 21, 2018 •
edited
Loading

First reported by @franziskagoltz.

It looks like the conduit proxy is adding a "Transfer-Encoding: chunked" header when proxying my get request.

To test, I've been injecting the proxy into a linkerd config that forwards traffic to https://httpbin.org, available here:

proxy.txt (actually .yml but file extension not supported)

If I apply that config in minikube and curl the 'proxy-https' service, it successfully returns a 200:

$ kubectl apply -f proxy.txt
$ curl -sIH 'Host: httpbin.org' $(minikube service proxy-https --url)/get | head -n1
HTTP/1.1 200 OK

Whereas if I inject that config with conduit and try curling, it fails with a 501:

$ curl -sIH 'Host: httpbin.org' $(minikube service proxy-https --url)/get | head -n1
HTTP/1.1 501 Not Implemented

The 501 response body is:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN">
<title>501 Not Implemented</title>
<h1>Not Implemented</h1>
<p>Chunked requests are not supported for server </p>

Fwiw, I've tested this with conduit master before and after #397 merged, and it appears to behave the same.
@seanmonstar
Copy link
Contributor

I don't believe TLS is related, but rather that it's a GET request, and the way body is constructed and the given to hyper means it isn't able to detect that the body is empty.

There is a fix in hyper master to just assume no body on GET if no content length is set...

@briansmith
Copy link
Contributor

Is this a regression from 0.2, or was this the case for 0.2 too?

@seanmonstar
Copy link
Contributor

It's not a regression. Just likely no test case to see it.

@briansmith
Copy link
Contributor

There is a fix in hyper master to just assume no body on GET if no content length is set...

The RFC says: "The presence of a message body in a request is signaled by a Content-Length or Transfer-Encoding header field." So it isn't just GET and it isn't just Content-Length.

@seanmonstar
Copy link
Contributor

I realize that. It's a combination of how it's used in the proxy. By default, in hyper, a request doesn't have a body unless set. In the proxy, since it's translating from potentially an h2 stream, a body is added to the request. hyper sees the set body and assumes a transfer encoding if content length wasn't set.

It's a deficiency in the current API, where it's not possible to know if a set body is actually empty until after the headers are sent.

The body trait will gain a method to check for it being empty in the next breaking release. In the mean time, it ignores chunked bodies for GET, since that's probably not what you meant.

@klingerf klingerf changed the title Proxying https traffic sets "Transfer-Encoding: chunked"? Proxy sets "Transfer-Encoding: chunked" when proxying GET request Feb 21, 2018
@klingerf
Copy link
Member Author

Thanks for the explanation -- have updated the title accordingly.

@olix0r olix0r added this to the Conduit 0.3.1 milestone Feb 21, 2018
seanmonstar added a commit that referenced this issue Feb 21, 2018
@seanmonstar
proxy: don't send transfer-encoding for empty GET requests
36c831c 
This is fixed in hyper v0.11.19.

Closes #402
@seanmonstar seanmonstar mentioned this issue Feb 21, 2018
Merged
@seanmonstar seanmonstar self-assigned this Feb 21, 2018
@seanmonstar seanmonstar added the review/ready Issue has a reviewable PR label Feb 21, 2018
seanmonstar added a commit that referenced this issue Feb 21, 2018
@seanmonstar
proxy: don't send transfer-encoding for empty GET requests
a2941da 
This is fixed in hyper v0.11.19.

Closes #402

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
seanmonstar added a commit that referenced this issue Feb 23, 2018
@seanmonstar
proxy: don't send transfer-encoding for empty GET requests
8a3e2f4 
This is fixed in hyper v0.11.19.

Closes #402

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
@olix0r olix0r added the priority/P0 Release Blocker label Feb 23, 2018
seanmonstar added a commit that referenced this issue Feb 24, 2018
@seanmonstar
proxy: don't send transfer-encoding for empty GET requests (#410)
381fb38 
This is fixed in hyper v0.11.19.

Closes #402
@seanmonstar seanmonstar removed the review/ready Issue has a reviewable PR label Feb 24, 2018
@briansmith briansmith mentioned this issue Feb 24, 2018
Closed
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this issue Mar 5, 2019
@seanmonstar
proxy: don't send transfer-encoding for empty GET requests (linkerd#410)
427335a 
This is fixed in hyper v0.11.19.

Closes linkerd#402
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@seanmonstar seanmonstar

Labels
area/proxy priority/P0 Release Blocker
Projects
None yet
Milestone
0.3.1
Development

Successfully merging a pull request may close this issue.

proxy: don't send transfer-encoding for empty GET requests linkerd/linkerd2
4 participants
@klingerf @briansmith @seanmonstar @olix0r