Merge branch 'backport-sendgrid-fixes-8-30' into '8-30-stable'

Backport sendgrid fixes to 8-30-stable branch

See merge request gitlab-org/gitlab-workhorse!519

changelogs/unreleased/fix-content-length-missing.yml

+---
+title: Fix Content-Length set prior to SendUrl injection
+merge_request: 496
+author: Georges-Etienne Legendre
+type: fixed

changelogs/unreleased/fix-issue-264.yml

+---
+title: Disable compression for open archive
+merge_request: 508
+author: Georges-Etienne Legendre
+type: fixed

internal/sendurl/sendurl.go

@@ -148,6 +148,9 @@ func (e *entry) Inject(w http.ResponseWriter, r *http.Request, sendData string)
 		return
 	}
 
+	// Prevent Go from adding a Content-Length header automatically
+	w.Header().Del("Content-Length")
+
 	// copy response headers and body, except the headers from preserveHeaderKeys
 	for key, value := range resp.Header {
 		if !preserveHeaderKeys[key] {
@@ -162,7 +165,7 @@ func (e *entry) Inject(w http.ResponseWriter, r *http.Request, sendData string)
 
 	if err != nil {
 		sendURLRequestsRequestFailed.Inc()
-		helper.Fail500(w, r, fmt.Errorf("SendURL: Copy response: %v", err))
+		helper.LogError(r, fmt.Errorf("SendURL: Copy response: %v", err))
 		return
 	}
 

internal/zipartifacts/.gitignore

+/testdata

internal/zipartifacts/open_archive.go

@@ -29,6 +29,7 @@ var httpClient = &http.Client{
 		TLSHandshakeTimeout:   10 * time.Second,
 		ExpectContinueTimeout: 10 * time.Second,
 		ResponseHeaderTimeout: 30 * time.Second,
+		DisableCompression:    true,
 	})),
 }
 

internal/zipartifacts/open_archive_test.go

+package zipartifacts
+
+import (
+	"archive/zip"
+	"context"
+	"fmt"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestOpenHTTPArchive(t *testing.T) {
+	const (
+		zipFile   = "test.zip"
+		entryName = "hello.txt"
+		contents  = "world"
+		testRoot  = "testdata/public"
+	)
+
+	require.NoError(t, os.MkdirAll(testRoot, 0755))
+	f, err := os.Create(filepath.Join(testRoot, zipFile))
+	require.NoError(t, err, "create file")
+	defer f.Close()
+
+	zw := zip.NewWriter(f)
+	w, err := zw.Create(entryName)
+	require.NoError(t, err, "create zip entry")
+	_, err = fmt.Fprint(w, contents)
+	require.NoError(t, err, "write zip entry contents")
+	require.NoError(t, zw.Close(), "close zip writer")
+	require.NoError(t, f.Close(), "close file")
+
+	srv := httptest.NewServer(http.FileServer(http.Dir(testRoot)))
+	defer srv.Close()
+
+	zr, err := OpenArchive(context.Background(), srv.URL+"/"+zipFile)
+	require.NoError(t, err, "call OpenArchive")
+	require.Len(t, zr.File, 1)
+
+	zf := zr.File[0]
+	require.Equal(t, entryName, zf.Name, "zip entry name")
+
+	entry, err := zf.Open()
+	require.NoError(t, err, "get zip entry reader")
+	defer entry.Close()
+
+	actualContents, err := ioutil.ReadAll(entry)
+	require.NoError(t, err, "read zip entry contents")
+	require.Equal(t, contents, string(actualContents), "compare zip entry contents")
+}
+
+func TestOpenHTTPArchiveNotSendingAcceptEncodingHeader(t *testing.T) {
+	requestHandler := func(w http.ResponseWriter, r *http.Request) {
+		require.Equal(t, "GET", r.Method)
+		require.Nil(t, r.Header["Accept-Encoding"])
+		w.WriteHeader(http.StatusOK)
+	}
+
+	srv := httptest.NewServer(http.HandlerFunc(requestHandler))
+	defer srv.Close()
+
+	OpenArchive(context.Background(), srv.URL)
+}

main_test.go

@@ -14,6 +14,8 @@ import (
 	"os/exec"
 	"path"
 	"regexp"
+	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -366,21 +368,58 @@ func TestArtifactsGetSingleFile(t *testing.T) {
 }
 
 func TestSendURLForArtifacts(t *testing.T) {
-	fileContents := "12345678901234567890\n"
+	expectedBody := strings.Repeat("CONTENT!", 1024)
 
-	server := httptest.NewServer(http.FileServer(http.Dir("testdata")))
-	defer server.Close()
+	regularHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Length", strconv.Itoa(len(expectedBody)))
+		w.Write([]byte(expectedBody))
+	})
 
-	// We manually created this txt file in the gitlab-workhorse Git repository
-	url := server.URL + "/test-file.txt"
-	jsonParams := fmt.Sprintf(`{"URL":%q}`, url)
+	chunkedHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Transfer-Encoding", "chunked")
+		w.Write([]byte(expectedBody))
+	})
 
-	resourcePath := `/namespace/project/builds/123/artifacts/file/download`
-	resp, body, err := doSendDataRequest(resourcePath, "send-url", jsonParams)
-	require.NoError(t, err)
+	rawHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		hj, ok := w.(http.Hijacker)
+		require.Equal(t, true, ok)
 
-	assert.Equal(t, 200, resp.StatusCode, "GET %q: status code", resourcePath)
-	assert.Equal(t, fileContents, string(body), "GET %q: response body", resourcePath)
+		conn, buf, err := hj.Hijack()
+		require.NoError(t, err)
+		defer conn.Close()
+		defer buf.Flush()
+
+		fmt.Fprint(buf, "HTTP/1.1 200 OK\r\nContent-Type: application/zip\r\n\r\n")
+		fmt.Fprint(buf, expectedBody)
+	})
+
+	for _, tc := range []struct {
+		name             string
+		handler          http.Handler
+		transferEncoding []string
+		contentLength    int
+	}{
+		{"No content-length, chunked TE", chunkedHandler, []string{"chunked"}, -1},    // Case 3 in https://tools.ietf.org/html/rfc7230#section-3.3.2
+		{"Known content-length, identity TE", regularHandler, nil, len(expectedBody)}, // Case 5 in https://tools.ietf.org/html/rfc7230#section-3.3.2
+		{"No content-length, identity TE", rawHandler, []string{"chunked"}, -1},       // Case 7 in https://tools.ietf.org/html/rfc7230#section-3.3.2
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			server := httptest.NewServer(tc.handler)
+			defer server.Close()
+
+			jsonParams := fmt.Sprintf(`{"URL":%q}`, server.URL)
+
+			resourcePath := `/namespace/project/builds/123/artifacts/file/download`
+			resp, body, err := doSendDataRequest(resourcePath, "send-url", jsonParams)
+			require.NoError(t, err)
+
+			require.Equal(t, http.StatusOK, resp.StatusCode, "GET %q: status code", resourcePath)
+			require.Equal(t, int64(tc.contentLength), resp.ContentLength, "GET %q: Content-Length", resourcePath)
+			require.Equal(t, tc.transferEncoding, resp.TransferEncoding, "GET %q: Transfer-Encoding", resourcePath)
+			require.Equal(t, expectedBody, string(body), "GET %q: response body", resourcePath)
+			assertNginxResponseBuffering(t, "no", resp, "GET %q: nginx response buffering", resourcePath)
+		})
+	}
 }
 
 func TestApiContentTypeBlock(t *testing.T) {

testdata/test-file.txt

-12345678901234567890