TUN-5195: Do not set empty body if not applicable

Go's client defaults to chunked encoding after a 200ms delay if the following cases are true:
  * the request body blocks
  * the content length is not set (or set to -1)
  * the method doesn't usually have a body (GET, HEAD, DELETE, ...)
  * there is no transfer-encoding=chunked already set.
So for non websocket requests, if transfer-encoding isn't chunked and content length is 0, we dont set a request body.

Author: sudarshan-reddy

connection/quic.go

@@ -158,11 +158,12 @@ func (hrw httpResponseAdapter) WriteErrorResponse(err error) {
 	quicpogs.WriteConnectResponseData(hrw, err, quicpogs.Metadata{Key: "HttpStatus", Val: strconv.Itoa(http.StatusBadGateway)})
 }
 
-func buildHTTPRequest(connectRequest *quicpogs.ConnectRequest, body io.Reader) (*http.Request, error) {
+func buildHTTPRequest(connectRequest *quicpogs.ConnectRequest, body io.ReadCloser) (*http.Request, error) {
 	metadata := connectRequest.MetadataMap()
 	dest := connectRequest.Dest
 	method := metadata[HTTPMethodKey]
 	host := metadata[HTTPHostKey]
+	isWebsocket := connectRequest.Type == quicpogs.ConnectionTypeWebsocket
 
 	req, err := http.NewRequest(method, dest, body)
 	if err != nil {
@@ -186,6 +187,16 @@ func buildHTTPRequest(connectRequest *quicpogs.ConnectRequest, body io.Reader) (
 	if err := setContentLength(req); err != nil {
 		return nil, fmt.Errorf("Error setting content-length: %w", err)
 	}
+
+	// Go's client defaults to chunked encoding after a 200ms delay if the following cases are true:
+	//   * the request body blocks
+	//   * the content length is not set (or set to -1)
+	//   * the method doesn't usually have a body (GET, HEAD, DELETE, ...)
+	//   * there is no transfer-encoding=chunked already set.
+	// So, if transfer cannot be chunked and content length is 0, we dont set a request body.
+	if !isWebsocket && !isTransferEncodingChunked(req) && req.ContentLength == 0 {
+		req.Body = nil
+	}
 	stripWebsocketUpgradeHeader(req)
 	return req, err
 }
@@ -197,3 +208,10 @@ func setContentLength(req *http.Request) error {
 	}
 	return err
 }
+
+func isTransferEncodingChunked(req *http.Request) bool {
+	transferEncodingVal := req.Header.Get("Transfer-Encoding")
+	// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding suggests that this can be a comma
+	// separated value as well.
+	return strings.Contains(strings.ToLower(transferEncodingVal), "chunked")
+}

connection/quic_test.go

@@ -108,6 +108,10 @@ func TestQUICServer(t *testing.T) {
 					Key: "HttpMethod",
 					Val: "POST",
 				},
+				quicpogs.Metadata{
+					Key: "HttpHeader:Content-Length",
+					Val: "24",
+				},
 			},
 			message:          []byte("This is the message body"),
 			expectedResponse: []byte("This is the message body"),
@@ -297,6 +301,7 @@ func TestBuildHTTPRequest(t *testing.T) {
 	var tests = []struct {
 		name           string
 		connectRequest *quicpogs.ConnectRequest
+		body           io.ReadCloser
 		req            *http.Request
 	}{
 		{
@@ -341,7 +346,9 @@ func TestBuildHTTPRequest(t *testing.T) {
 				},
 				ContentLength: 514,
 				Host:          "cf.host",
+				Body:          io.NopCloser(&bytes.Buffer{}),
 			},
+			body: io.NopCloser(&bytes.Buffer{}),
 		},
 		{
 			name: "if content length isn't part of request headers, then it's not set",
@@ -380,13 +387,137 @@ func TestBuildHTTPRequest(t *testing.T) {
 				},
 				ContentLength: 0,
 				Host:          "cf.host",
+				Body:          nil,
+			},
+			body: io.NopCloser(&bytes.Buffer{}),
+		},
+		{
+			name: "if content length is 0, but transfer-encoding is chunked, body is not nil",
+			connectRequest: &quicpogs.ConnectRequest{
+				Dest: "http://test.com",
+				Metadata: []quicpogs.Metadata{
+					quicpogs.Metadata{
+						Key: "HttpHeader:Another-Header",
+						Val: "Misc",
+					},
+					quicpogs.Metadata{
+						Key: "HttpHeader:Transfer-Encoding",
+						Val: "chunked",
+					},
+					quicpogs.Metadata{
+						Key: "HttpHost",
+						Val: "cf.host",
+					},
+					quicpogs.Metadata{
+						Key: "HttpMethod",
+						Val: "get",
+					},
+				},
+			},
+			req: &http.Request{
+				Method: "get",
+				URL: &url.URL{
+					Scheme: "http",
+					Host:   "test.com",
+				},
+				Proto:      "HTTP/1.1",
+				ProtoMajor: 1,
+				ProtoMinor: 1,
+				Header: http.Header{
+					"Another-Header":    []string{"Misc"},
+					"Transfer-Encoding": []string{"chunked"},
+				},
+				ContentLength: 0,
+				Host:          "cf.host",
+				Body:          io.NopCloser(&bytes.Buffer{}),
+			},
+			body: io.NopCloser(&bytes.Buffer{}),
+		},
+		{
+			name: "if content length is 0, but transfer-encoding is gzip,chunked, body is not nil",
+			connectRequest: &quicpogs.ConnectRequest{
+				Dest: "http://test.com",
+				Metadata: []quicpogs.Metadata{
+					quicpogs.Metadata{
+						Key: "HttpHeader:Another-Header",
+						Val: "Misc",
+					},
+					quicpogs.Metadata{
+						Key: "HttpHeader:Transfer-Encoding",
+						Val: "gzip,chunked",
+					},
+					quicpogs.Metadata{
+						Key: "HttpHost",
+						Val: "cf.host",
+					},
+					quicpogs.Metadata{
+						Key: "HttpMethod",
+						Val: "get",
+					},
+				},
+			},
+			req: &http.Request{
+				Method: "get",
+				URL: &url.URL{
+					Scheme: "http",
+					Host:   "test.com",
+				},
+				Proto:      "HTTP/1.1",
+				ProtoMajor: 1,
+				ProtoMinor: 1,
+				Header: http.Header{
+					"Another-Header":    []string{"Misc"},
+					"Transfer-Encoding": []string{"gzip,chunked"},
+				},
+				ContentLength: 0,
+				Host:          "cf.host",
+				Body:          io.NopCloser(&bytes.Buffer{}),
+			},
+			body: io.NopCloser(&bytes.Buffer{}),
+		},
+		{
+			name: "if content length is 0, and connect request is a websocket, body is not nil",
+			connectRequest: &quicpogs.ConnectRequest{
+				Type: quicpogs.ConnectionTypeWebsocket,
+				Dest: "http://test.com",
+				Metadata: []quicpogs.Metadata{
+					quicpogs.Metadata{
+						Key: "HttpHeader:Another-Header",
+						Val: "Misc",
+					},
+					quicpogs.Metadata{
+						Key: "HttpHost",
+						Val: "cf.host",
+					},
+					quicpogs.Metadata{
+						Key: "HttpMethod",
+						Val: "get",
+					},
+				},
+			},
+			req: &http.Request{
+				Method: "get",
+				URL: &url.URL{
+					Scheme: "http",
+					Host:   "test.com",
+				},
+				Proto:      "HTTP/1.1",
+				ProtoMajor: 1,
+				ProtoMinor: 1,
+				Header: http.Header{
+					"Another-Header": []string{"Misc"},
+				},
+				ContentLength: 0,
+				Host:          "cf.host",
+				Body:          io.NopCloser(&bytes.Buffer{}),
 			},
+			body: io.NopCloser(&bytes.Buffer{}),
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			req, err := buildHTTPRequest(test.connectRequest, nil)
+			req, err := buildHTTPRequest(test.connectRequest, test.body)
 			assert.NoError(t, err)
 			test.req = test.req.WithContext(req.Context())
 			assert.Equal(t, test.req, req)