Merge branch 'ac-case-insensitive-md5' into 'master'

Case insensitive ETag comparison

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

internal/objectstore/multipart.go

@@ -155,11 +155,8 @@ func (m *Multipart) complete(cmu *CompleteMultipartUpload) error {
 	}
 
 	m.extractETag(result.ETag)
-	if err := m.verifyETag(cmu); err != nil {
-		return fmt.Errorf("ETag verification failure: %v", err)
-	}
 
-	return nil
+	return m.verifyETag(cmu)
 }
 
 func (m *Multipart) verifyETag(cmu *CompleteMultipartUpload) error {
@@ -167,11 +164,8 @@ func (m *Multipart) verifyETag(cmu *CompleteMultipartUpload) error {
 	if err != nil {
 		return err
 	}
-	if expectedChecksum != m.etag {
-		return fmt.Errorf("got %q expected %q", m.etag, expectedChecksum)
-	}
 
-	return nil
+	return compareMD5(expectedChecksum, m.etag)
 }
 
 func (m *Multipart) readAndUploadOnePart(partURL string, putHeaders map[string]string, src io.Reader, partNumber int) (*completeMultipartUploadPart, error) {

internal/objectstore/multipart_test.go

+package objectstore_test
+
+import (
+	"context"
+	"fmt"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/objectstore"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/objectstore/test"
+)
+
+func TestMultipartUploadWithUpcaseETags(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	var putCnt, postCnt int
+
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, err := ioutil.ReadAll(r.Body)
+		require.NoError(t, err)
+		defer r.Body.Close()
+
+		// Part upload request
+		if r.Method == "PUT" {
+			putCnt++
+
+			w.Header().Set("ETag", strings.ToUpper(test.ObjectMD5))
+		}
+
+		// POST with CompleteMultipartUpload request
+		if r.Method == "POST" {
+			expectedETag := "6e6b164c392b04bfbb82368179d9ade2-1"
+			completeBody := fmt.Sprintf(`<CompleteMultipartUploadResult>
+                                                       <Bucket>test-bucket</Bucket>
+			                               <ETag>%s</ETag>
+  	                                             </CompleteMultipartUploadResult>`,
+				strings.ToUpper(expectedETag))
+			postCnt++
+
+			w.Write([]byte(completeBody))
+		}
+	}))
+	defer ts.Close()
+
+	deadline := time.Now().Add(testTimeout)
+
+	m, err := objectstore.NewMultipart(ctx,
+		[]string{ts.URL},    // a single presigned part URL
+		ts.URL,              // the complete multipart upload URL
+		"",                  // no abort
+		"",                  // no delete
+		map[string]string{}, // no custom headers
+		deadline,
+		test.ObjectSize) // parts size equal to the whole content. Only 1 part
+	require.NoError(t, err)
+
+	_, err = m.Write([]byte(test.ObjectContent))
+	require.NoError(t, err)
+	require.NoError(t, m.Close())
+	require.Equal(t, 1, putCnt, "1 part expected")
+	require.Equal(t, 1, postCnt, "1 complete multipart upload expected")
+}

internal/objectstore/object.go

@@ -7,6 +7,7 @@ import (
 	"io/ioutil"
 	"net"
 	"net/http"
+	"strings"
 	"time"
 
 	"gitlab.com/gitlab-org/labkit/correlation"
@@ -124,10 +125,7 @@ func newObject(ctx context.Context, putURL, deleteURL string, putHeaders map[str
 		}
 
 		o.extractETag(resp.Header.Get("ETag"))
-		if o.etag != o.md5Sum() {
-			o.uploadError = fmt.Errorf("ETag mismatch. expected %q got %q", o.md5Sum(), o.etag)
-			return
-		}
+		o.uploadError = compareMD5(o.md5Sum(), o.etag)
 	}()
 
 	return o, nil
@@ -136,3 +134,11 @@ func newObject(ctx context.Context, putURL, deleteURL string, putHeaders map[str
 func (o *Object) delete() {
 	o.syncAndDelete(o.DeleteURL)
 }
+
+func compareMD5(local, remote string) error {
+	if !strings.EqualFold(local, remote) {
+		return fmt.Errorf("ETag mismatch. expected %q got %q", local, remote)
+	}
+
+	return nil
+}

internal/objectstore/object_test.go

@@ -18,10 +18,12 @@ import (
 
 const testTimeout = 10 * time.Second
 
-func testObjectUploadNoErrors(t *testing.T, useDeleteURL bool, contentType string) {
+type osFactory func() (*test.ObjectstoreStub, *httptest.Server)
+
+func testObjectUploadNoErrors(t *testing.T, startObjectStore osFactory, useDeleteURL bool, contentType string) {
 	assert := assert.New(t)
 
-	osStub, ts := test.StartObjectStore()
+	osStub, ts := startObjectStore()
 	defer ts.Close()
 
 	objectURL := ts.URL + test.ObjectPath
@@ -77,9 +79,26 @@ func testObjectUploadNoErrors(t *testing.T, useDeleteURL bool, contentType strin
 }
 
 func TestObjectUpload(t *testing.T) {
-	t.Run("with delete URL", func(t *testing.T) { testObjectUploadNoErrors(t, true, "application/octet-stream") })
-	t.Run("without delete URL", func(t *testing.T) { testObjectUploadNoErrors(t, false, "application/octet-stream") })
-	t.Run("with custom content type", func(t *testing.T) { testObjectUploadNoErrors(t, false, "image/jpeg") })
+	t.Run("with delete URL", func(t *testing.T) {
+		testObjectUploadNoErrors(t, test.StartObjectStore, true, "application/octet-stream")
+	})
+	t.Run("without delete URL", func(t *testing.T) {
+		testObjectUploadNoErrors(t, test.StartObjectStore, false, "application/octet-stream")
+	})
+	t.Run("with custom content type", func(t *testing.T) {
+		testObjectUploadNoErrors(t, test.StartObjectStore, false, "image/jpeg")
+	})
+	t.Run("with upcase ETAG", func(t *testing.T) {
+		factory := func() (*test.ObjectstoreStub, *httptest.Server) {
+			md5s := map[string]string{
+				test.ObjectPath: strings.ToUpper(test.ObjectMD5),
+			}
+
+			return test.StartObjectStoreWithCustomMD5(md5s)
+		}
+
+		testObjectUploadNoErrors(t, factory, false, "application/octet-stream")
+	})
 }
 
 func TestObjectUpload404(t *testing.T) {