Merge pull request #2488 from LuisFigueroaG/fix/invalid-part-error-details

fix: report the upload, part and ETag in InvalidPart for a malformed part ETag

backend/common.go

@@ -425,8 +425,9 @@ func isValidTagComponent(str string) bool {
 }
 
 // ComputeMultipartETagFromPartETags computes the S3 multipart ETag
-// ("<md5>-<partCount>") from the completed-part ETags.
-func ComputeMultipartETagFromPartETags(parts []types.CompletedPart) (string, error) {
+// ("<md5>-<partCount>") from the completed-part ETags. uploadID is only
+// used to report the upload in an InvalidPart error.
+func ComputeMultipartETagFromPartETags(uploadID string, parts []types.CompletedPart) (string, error) {
 	var partsEtagBytes []byte
 	for _, part := range parts {
 		if part.ETag == nil {
@@ -437,7 +438,7 @@ func ComputeMultipartETagFromPartETags(parts []types.CompletedPart) (string, err
 		}
 		bts, err := getEtagBytes(*part.ETag)
 		if err != nil {
-			return "", s3err.GetAPIError(s3err.ErrInvalidPart)
+			return "", s3err.GetInvalidPartErr(uploadID, *part.PartNumber, *part.ETag)
 		}
 		partsEtagBytes = append(partsEtagBytes, bts...)
 	}

backend/common_test.go

@@ -16,11 +16,15 @@ package backend
 
 import (
 	"bytes"
+	"crypto/md5"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"reflect"
 	"testing"
 
+	"github.com/aws/aws-sdk-go-v2/service/s3/types"
 	"github.com/versity/versitygw/s3err"
 )
 
@@ -330,3 +334,50 @@ func TestParseCopySource(t *testing.T) {
 		})
 	}
 }
+
+func TestComputeMultipartETagFromPartETags(t *testing.T) {
+	etag1 := "5d41402abc4b2a76b9719d911017c592"
+	etag2 := "7d793037a0760186574b0282f2f435e7"
+	raw1, _ := hex.DecodeString(etag1)
+	raw2, _ := hex.DecodeString(etag2)
+	sum := md5.Sum(append(raw1, raw2...))
+	wantETag := fmt.Sprintf("\"%s-2\"", hex.EncodeToString(sum[:]))
+
+	part := func(n int32, etag string) types.CompletedPart {
+		return types.CompletedPart{PartNumber: &n, ETag: &etag}
+	}
+
+	tests := []struct {
+		name     string
+		parts    []types.CompletedPart
+		wantETag string
+		wantErr  error
+	}{
+		{
+			name:     "quoted and unquoted part etags",
+			parts:    []types.CompletedPart{part(1, "\""+etag1+"\""), part(2, etag2)},
+			wantETag: wantETag,
+		},
+		{
+			name:    "missing part etag",
+			parts:   []types.CompletedPart{{PartNumber: new(int32)}},
+			wantErr: s3err.GetAPIError(s3err.ErrMalformedXML),
+		},
+		{
+			name:    "invalid part etag reports the upload, part and etag",
+			parts:   []types.CompletedPart{part(1, etag1), part(2, "\"invalidETag\"")},
+			wantErr: s3err.GetInvalidPartErr("upload-id", 2, "\"invalidETag\""),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			etag, err := ComputeMultipartETagFromPartETags("upload-id", tt.parts)
+			if !reflect.DeepEqual(err, tt.wantErr) {
+				t.Fatalf("expected error %#v, got %#v", tt.wantErr, err)
+			}
+			if etag != tt.wantETag {
+				t.Fatalf("expected etag %q, got %q", tt.wantETag, etag)
+			}
+		})
+	}
+}

backend/posix/posix.go

@@ -2371,7 +2371,7 @@ func (p *Posix) CompleteMultipartUploadWithCopy(ctx context.Context, input *s3.C
 	// Compute the default multipart ETag token used for claim naming.
 	// In standard mode this is the S3-compatible multipart MD5 ETag; in
 	// dataIntegrityEtag mode it may fall back to a deterministic claim token.
-	multipartClaimToken, err := backend.ComputeMultipartETagFromPartETags(parts)
+	multipartClaimToken, err := backend.ComputeMultipartETagFromPartETags(uploadID, parts)
 	if err != nil {
 		if !p.dataIntegrityEtag {
 			return res, "", err

tests/integration/CompleteMultipartUpload.go

@@ -190,6 +190,47 @@ func CompleteMultipartUpload_invalid_ETag(s *S3Conf) error {
 		return nil
 	})
 }
+
+func CompleteMultipartUpload_invalid_ETag_error_fields(s *S3Conf) error {
+	testName := "CompleteMultipartUpload_invalid_ETag_error_fields"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		obj := "my-obj"
+		out, err := createMp(s3client, bucket, obj)
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.UploadPart(ctx, &s3.UploadPartInput{
+			Bucket:     &bucket,
+			Key:        &obj,
+			UploadId:   out.UploadId,
+			PartNumber: getPtr(int32(1)),
+			Body:       bytes.NewReader([]byte("dummy")),
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		// the sdk doesn't expose the InvalidPart error fields, so the
+		// request is signed and sent by hand
+		body := []byte(`<CompleteMultipartUpload><Part><ETag>invalidETag</ETag><PartNumber>1</PartNumber></Part></CompleteMultipartUpload>`)
+		req, err := createSignedReq(http.MethodPost, s.endpoint,
+			fmt.Sprintf("%v/%v?uploadId=%v", bucket, obj, *out.UploadId),
+			s.awsID, s.awsSecret, "s3", s.awsRegion, "", body, time.Now(), nil)
+		if err != nil {
+			return err
+		}
+
+		resp, err := s.httpClient.Do(req)
+		if err != nil {
+			return err
+		}
+
+		return checkHTTPResponseApiErr(resp, s3err.GetInvalidPartErr(*out.UploadId, 1, "invalidETag"))
+	})
+}
 func CompleteMultipartUpload_invalid_checksum_type(s *S3Conf) error {
 	testName := "CompleteMultipartUpload_invalid_checksum_type"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {

tests/integration/group-tests.go

@@ -588,6 +588,7 @@ func TestCompleteMultipartUpload(ts *TestState) {
 	ts.Run(CompleteMultipartUpload_default_content_type)
 	ts.Run(CompleteMultipartUpload_overwrite_resets_attributes)
 	ts.Run(CompleteMultipartUpload_invalid_ETag)
+	ts.Run(CompleteMultipartUpload_invalid_ETag_error_fields)
 	ts.Run(CompleteMultipartUpload_small_upload_size)
 	ts.Run(CompleteMultipartUpload_empty_parts)
 	ts.Run(CompleteMultipartUpload_missing_part_fields)
@@ -3356,6 +3357,7 @@ func GetIntTests() IntTests {
 		"CompleteMultipartUpload_default_content_type":                                     CompleteMultipartUpload_default_content_type,
 		"CompleteMultipartUpload_overwrite_resets_attributes":                              CompleteMultipartUpload_overwrite_resets_attributes,
 		"CompleteMultipartUpload_invalid_ETag":                                             CompleteMultipartUpload_invalid_ETag,
+		"CompleteMultipartUpload_invalid_ETag_error_fields":                                CompleteMultipartUpload_invalid_ETag_error_fields,
 		"CompleteMultipartUpload_small_upload_size":                                        CompleteMultipartUpload_small_upload_size,
 		"CompleteMultipartUpload_empty_parts":                                              CompleteMultipartUpload_empty_parts,
 		"CompleteMultipartUpload_missing_part_fields":                                      CompleteMultipartUpload_missing_part_fields,

tests/test_rest_multipart.sh

@@ -65,9 +65,6 @@ source ./tests/drivers/upload_part/upload_part_rest.sh
 
 # tags: curl,multipart,CompleteMultipartUpload,invalid-header,ETag
 @test "REST - complete upload - invalid part" {
-  if [ "$DIRECT" != "true" ]; then
-    skip "https://github.com/versity/versitygw/issues/1008"
-  fi
   run get_bucket_name "$BUCKET_ONE_NAME"
   assert_success
   bucket_name="$output"