Copy the request ContentLength and TransferEncoding field in proxy #16400

caesarxuchao · 2015-10-28T00:50:21Z

Thanks to @tiran for identifying the problem and proposing the fix.

@lavalamp @liggitt

fix #16093

k8s-github-robot · 2015-10-28T00:57:54Z

Labelling this PR as size/XS

k8s-bot · 2015-10-28T01:19:20Z

GCE e2e test build/test passed for commit 3289397.

liggitt · 2015-10-28T12:54:36Z

pkg/apiserver/proxy.go

@@ -151,6 +151,8 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	}
 	httpCode = http.StatusOK
 	newReq.Header = req.Header
+	newReq.ContentLength = req.ContentLength
+	newReq.TransferEncoding = req.TransferEncoding


whatever we do here we also need to do in generic/rest/proxy.go (a refactor to make apiserver/proxy.go delegate to the generic proxy is overdue, but should be a separate PR)

Looks like the reverse proxy strips the transfer encoding header.... isn't it a problem to send content length to the backend without transfer encoding?

in any case, I'd want tests through the proxy to a backend using no transport encoding, gzip, and chunked, with and without content-length, to make sure the backend gets a correct content length in all cases

hmm... this also gets set prior to the upgrade attempt... not sure if spdy or websockets would ever set a content length or transfer encoding, but we should at least check on that

Thanks @liggitt. I'll take a look at the issues you mentioned tomorrow or Friday.

After digging more, I'm pretty sure we should only be setting ContentLength. TransferEncoding is interpreted, and the request body wrapped in an interpreting reader before we get it (that's why a request sent as "chunked" comes in with no transfer encoding, and reading the body returns unchunked data).

But we need to keep the new request being "chunked" when it hits wire, right? If so, I think we need to copy the Request.TransferEncoding

The transfer writer will set the transfer-encoding header field to "chunked" if Request.TransferEncoding contains "chunked:
https://golang.org/src/net/http/transfer.go#L177
Same for the Body, the body will be chunked if Request.TransferEncoding contains "chunked":
https://golang.org/src/net/http/transfer.go#L213

hmm, I suppose so... I'm pretty sure the http package will always unwrap any transfer-encodings that are present, but in case they don't, it doesn't hurt to copy what remains to the new request

tl;dr: we can keep this line, though it may not be useful.

After more digging, I found actually the http package is powerful enough deduce the outbound request should be "chunked" based on the fact that the content-length is left unset && there is content:
https://golang.org/src/net/http/transfer.go#L88

Well, it doesn't hurt to keep the copy in the PR, and perhaps it will save a broken request where "transfer-encoding:chunked" and "content-length: non-zero-value" are set at the same time.

k8s-github-robot · 2015-11-03T07:08:39Z

Labelling this PR as size/M

k8s-bot · 2015-11-03T07:34:37Z

GCE e2e test build/test passed for commit 0940741481341fff406e39dae5865b332d1933e0.

k8s-github-robot · 2015-11-03T19:19:58Z

Labelling this PR as size/L

caesarxuchao · 2015-11-03T19:20:54Z

pkg/apiserver/proxy_test.go

+				}
+			}
+
+			// Help wanted: why is the transfer-encoding header field always empty?


@liggitt @lavalamp, the backend server always receives request with empty transfer-encoding header. Is this expected?

Perhaps look at the header instead of the variable?

Oh, you are... hm

For the record, in the received request, the struct field req.TranferEncoding = []string{"chunked"} for the last 2 test cases. I think go library deduced this somehow.

I prints the content in the buffer in the conn struct and persistConn struct to understand what's written/read by the go standard library to/from the wire. The golang library only supports chuncked as a transfer-encoding, more specifically,

When a request is sent either by a default golang http client or by a golang server, the only possible value in the transfer-encoding field is chunked,
https://golang.org/src/net/http/transfer.go#L178

When a golang server reads the next request, the standard library parses the transfer-encoding field, passes the result to Request.TransferEncoding, and deletes the transfer-encoding field in the header,
https://golang.org/src/net/http/transfer.go#L433,
The standard library actually considers it’s an error if the transfer-encoding field contains value other than identity or chunked,
https://golang.org/src/net/http/transfer.go#L447

Asked the question in golang-nuts group:
https://groups.google.com/forum/#!topic/golang-nuts/g7xJk09xBxU

k8s-bot · 2015-11-03T19:24:24Z

GCE e2e build/test failed for commit 34a863e65765a37768b1233f58accef2ecf84657.

k8s-bot · 2015-11-03T19:54:18Z

GCE e2e test build/test passed for commit 135b0b39c486081af26e9a00dbe049450fb10edb.

lavalamp · 2015-11-11T19:15:36Z

pkg/apiserver/proxy_test.go

+	}
+
+	for _, item := range table {
+		proxyServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {


Suggest naming this "downstreamServer"-- it's not the proxy, it's the thing the proxy dials, right?

lavalamp · 2015-11-11T19:22:06Z

I think I found a couple bugs in the test, but I'm not sure if they'll actually fix your problem or not. Let me know.

caesarxuchao · 2015-11-17T22:04:21Z

pkg/apiserver/proxy_test.go

+				}
+			}
+
+			// The http library will strip the "Transfer-Encoding" field from request.Header.


As the comment reads, Go library strips the "Transfer-Encoding" field. It will stores the value in request.TransferEncodings after some processing, the only possible value will be either "" or "chunked".

People say usually "gzip" will be a "Content-Encoding", so hopefully we won't get bit for this.

The discussion in go-nuts group: https://groups.google.com/forum/#!topic/golang-nuts/g7xJk09xBxU

Thanks for the investigation!

yes, thanks... we should test with content-encoding: gzip, transfer-encoding:'' as well

k8s-bot · 2015-11-17T22:43:09Z

GCE e2e test build/test passed for commit 9690029.

k8s-bot · 2015-11-17T23:16:08Z

GCE e2e build/test failed for commit 1fe49b6.

liggitt · 2015-11-18T05:51:31Z

After digging into the http DefaultClient/DefaultTransport more, it looks like there is no way to make it faithfully echo transfer-encoding/content-length headers to the server... it really wants to own the transfer. For low-level tests like this, I think we need to write headers and request bodies ourselves. Opened caesarxuchao#2 to do so

caesarxuchao · 2015-11-18T06:09:55Z

it looks like there is no way to make it faithfully echo transfer-encoding/content-length headers to the server

That's true. I'll take a look at the PR. Thanks.

googlebot · 2015-11-20T17:57:55Z

CLAs look good, thanks!

caesarxuchao · 2015-11-20T17:59:42Z

Squashed. I kept liggitt in the commit message as an acknowledgement to his contribution.

caesarxuchao · 2015-11-20T18:00:06Z

@lavalamp PTAL. Thanks.

k8s-bot · 2015-11-20T18:22:49Z

GCE e2e test build/test passed for commit e1e393ec1287fe5ca5f72e9a48c7b6c135f82bc1.

k8s-bot · 2015-11-20T18:29:15Z

GCE e2e build/test failed for commit 597492b845fb4dde54f5c82350391fc1a5debfc9.

k8s-bot · 2015-11-20T18:29:38Z

GCE e2e build/test failed for commit 32f2ec7.

caesarxuchao · 2015-11-20T21:23:07Z

@k8s-bot test this please

k8s-bot · 2015-11-20T21:54:18Z

GCE e2e test build/test passed for commit 32f2ec7.

lavalamp · 2015-11-30T23:44:11Z

LGTM, but the code duplication is unfortunate. I guess we already had that problem. :)

k8s-github-robot · 2015-11-30T23:46:30Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2015-12-01T00:51:32Z

GCE e2e test build/test passed for commit 32f2ec7.

wojtek-t · 2015-12-03T12:48:21Z

@k8s-bot unit test this please

caesarxuchao · 2015-12-03T18:02:12Z

@k8s-bot unit test this please

k8s-github-robot · 2015-12-03T18:35:07Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2015-12-03T19:04:01Z

GCE e2e test build/test passed for commit 32f2ec7.

k8s-github-robot · 2015-12-03T19:32:10Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-12-03T19:48:59Z

GCE e2e build/test failed for commit 32f2ec7.

caesarxuchao · 2015-12-03T20:58:26Z

Unrelated test failure:

+++ [1203 19:55:59] Testing kubectl(:multiple resources)
Testing with file hack/testdata/multi-resource-yaml.yaml and replace with file hack/testdata/multi-resource-yaml-modify.yaml
Successful get services {{range.items}}{{.metadata.name}}:{{end}}: kubernetes:
�
FAIL!
Get rc {{range.items}}{{.metadata.name}}:{{end}}
  Expected: 
  Got:      nginx-deployment-p889s:
�
1058 ./hack/test-cmd.sh
�
!!! Error in ./hack/test-cmd.sh:50

caesarxuchao · 2015-12-03T20:58:35Z

@k8s-bot test this please

k8s-bot · 2015-12-03T21:31:28Z

GCE e2e build/test failed for commit 32f2ec7.

caesarxuchao · 2015-12-03T21:33:47Z

unrelated e2e test failure:

Kubernetes e2e suite.Kubectl client Kubectl describe should check if kubectl describe prints relevant information for rc and pods

caesarxuchao · 2015-12-03T21:33:57Z

@k8s-bot e2e test this please

k8s-bot · 2015-12-03T22:08:34Z

GCE e2e test build/test passed for commit 32f2ec7.

k8s-github-robot · 2015-12-03T23:16:51Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-12-03T23:55:37Z

GCE e2e test build/test passed for commit 32f2ec7.

k8s-github-robot · 2015-12-03T23:56:01Z

Automatic merge from submit-queue

Auto commit by PR queue bot

caesarxuchao assigned lavalamp Oct 28, 2015

googlebot added the cla: yes label Oct 28, 2015

k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 28, 2015

liggitt reviewed Oct 28, 2015
View reviewed changes

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 3, 2015

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 3, 2015

caesarxuchao reviewed Nov 3, 2015
View reviewed changes

lavalamp reviewed Nov 11, 2015
View reviewed changes

caesarxuchao force-pushed the fix-proxy-content-length branch from d3e3562 to 9690029 Compare November 17, 2015 21:58

caesarxuchao reviewed Nov 17, 2015
View reviewed changes

caesarxuchao force-pushed the fix-proxy-content-length branch from 597492b to 32f2ec7 Compare November 20, 2015 17:57

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2015

k8s-github-robot pushed a commit that referenced this pull request Dec 3, 2015

Merge pull request #16400 from caesarxuchao/fix-proxy-content-length

c849825

Auto commit by PR queue bot

k8s-github-robot merged commit c849825 into kubernetes:master Dec 3, 2015

Copy the request ContentLength and TransferEncoding field in proxy #16400

Copy the request ContentLength and TransferEncoding field in proxy #16400

Conversation

caesarxuchao commented Oct 28, 2015

k8s-github-robot commented Oct 28, 2015

k8s-bot commented Oct 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Nov 3, 2015

k8s-bot commented Nov 3, 2015

k8s-github-robot commented Nov 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Nov 3, 2015

k8s-bot commented Nov 3, 2015

Choose a reason for hiding this comment

lavalamp commented Nov 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Nov 17, 2015

k8s-bot commented Nov 17, 2015

liggitt commented Nov 18, 2015

caesarxuchao commented Nov 18, 2015

googlebot commented Nov 20, 2015

caesarxuchao commented Nov 20, 2015

caesarxuchao commented Nov 20, 2015

k8s-bot commented Nov 20, 2015

k8s-bot commented Nov 20, 2015

k8s-bot commented Nov 20, 2015

caesarxuchao commented Nov 20, 2015

k8s-bot commented Nov 20, 2015

lavalamp commented Nov 30, 2015

k8s-github-robot commented Nov 30, 2015

k8s-bot commented Dec 1, 2015

wojtek-t commented Dec 3, 2015

caesarxuchao commented Dec 3, 2015

k8s-github-robot commented Dec 3, 2015

k8s-bot commented Dec 3, 2015

k8s-github-robot commented Dec 3, 2015

k8s-bot commented Dec 3, 2015

caesarxuchao commented Dec 3, 2015

caesarxuchao commented Dec 3, 2015

k8s-bot commented Dec 3, 2015

caesarxuchao commented Dec 3, 2015

caesarxuchao commented Dec 3, 2015

k8s-bot commented Dec 3, 2015

k8s-github-robot commented Dec 3, 2015

k8s-bot commented Dec 3, 2015

k8s-github-robot commented Dec 3, 2015