diff --git a/backend/azure/azure.go b/backend/azure/azure.go index cdcb7a45..b18708f7 100644 --- a/backend/azure/azure.go +++ b/backend/azure/azure.go @@ -1492,6 +1492,7 @@ func (az *Azure) UploadPart(ctx context.Context, input *s3.UploadPartInput) (*s3 // block id serves as etag here etag := blockIDInt32ToBase64(*input.PartNumber) + quotedETag := quoteETag(etag) // Azure StageBlock rejects Content-Length: 0 as an invalid header value. // Track zero-byte parts in the sgwtmp metadata instead of staging them. @@ -1507,7 +1508,7 @@ func (az *Azure) UploadPart(ctx context.Context, input *s3.UploadPartInput) (*s3 if err != nil { return nil, err } - return &s3.UploadPartOutput{ETag: &etag}, nil + return &s3.UploadPartOutput{ETag: "edETag}, nil } client, err := az.getBlockBlobClient(*input.Bucket, *input.Key) @@ -1521,7 +1522,7 @@ func (az *Azure) UploadPart(ctx context.Context, input *s3.UploadPartInput) (*s3 } return &s3.UploadPartOutput{ - ETag: &etag, + ETag: "edETag, }, nil } @@ -1544,7 +1545,13 @@ func (az *Azure) UploadPartCopy(ctx context.Context, input *s3.UploadPartCopyInp return s3response.CopyPartResult{}, parseMpError(err) } - return s3response.CopyPartResult{}, nil + // The staged block id serves as the part ETag, returned quoted to match + // the S3 contract and the form UploadPart/ListParts emit. + quotedETag := quoteETag(eTag) + return s3response.CopyPartResult{ + ETag: "edETag, + LastModified: time.Now(), + }, nil } // Lists all uncommitted parts from the blob @@ -1595,7 +1602,7 @@ func (az *Azure) ListParts(ctx context.Context, input *s3.ListPartsInput) (s3res } parts = append(parts, s3response.Part{ Size: *el.Size, - ETag: *el.Name, + ETag: quoteETag(*el.Name), PartNumber: partNumber, LastModified: time.Now(), }) @@ -1610,7 +1617,7 @@ func (az *Azure) ListParts(ctx context.Context, input *s3.ListPartsInput) (s3res } parts = append(parts, s3response.Part{ Size: 0, - ETag: blockIDInt32ToBase64(zbPartNum), + ETag: quoteETag(blockIDInt32ToBase64(zbPartNum)), PartNumber: int(zbPartNum), LastModified: time.Now(), }) @@ -1873,6 +1880,10 @@ func (az *Azure) CompleteMultipartUpload(ctx context.Context, input *s3.Complete if part.ETag == nil { return res, "", s3err.GetAPIError(s3err.ErrMalformedXML) } + // Clients may submit the part ETag in quoted form (as returned by + // UploadPart/ListParts) or raw; normalize before comparing to the + // raw Azure block id. + clientETag := getString(backend.TrimEtag(part.ETag)) if *part.PartNumber < 1 { return res, "", s3err.GetInvalidArgumentErr(s3err.InvalidArgCompleteMpPartNumber, fmt.Sprint(*part.PartNumber)) } @@ -1886,7 +1897,7 @@ func (az *Azure) CompleteMultipartUpload(ctx context.Context, input *s3.Complete // Check if this is a tracked zero-byte part. if zbPartsMap[*part.PartNumber] { expectedETag := blockIDInt32ToBase64(*part.PartNumber) - if getString(part.ETag) != expectedETag { + if clientETag != expectedETag { return res, "", s3err.GetInvalidPartErr(*input.UploadId, *part.PartNumber, expectedETag) } // Non-last zero-byte parts violate the minimum part size. @@ -1900,8 +1911,8 @@ func (az *Azure) CompleteMultipartUpload(ctx context.Context, input *s3.Complete return res, "", s3err.GetInvalidPartErr(*input.UploadId, *part.PartNumber, "") } - if *part.ETag != *block.Name { - return res, "", s3err.GetInvalidPartErr(*input.UploadId, *part.PartNumber, getString(part.ETag)) + if clientETag != *block.Name { + return res, "", s3err.GetInvalidPartErr(*input.UploadId, *part.PartNumber, clientETag) } // all parts except the last need to be greater, than // the minimum allowed size (5 Mib) @@ -2361,6 +2372,16 @@ func getReadSeekCloser(input io.Reader) (io.ReadSeekCloser, error) { return streaming.NopCloser(bytes.NewReader(buffer.Bytes())), nil } +// quoteETag wraps a raw Azure block id (used as a multipart part ETag) in +// double quotes. S3 ETags are quoted strings, and the posix backend follows +// the same convention (see backend.GenerateEtag). Keeping all Azure part-ETag +// surfaces (UploadPart, ListParts, UploadPartCopy) quoted ensures the values +// returned to clients are consistent, while the raw (unquoted) block id is +// still used for the Azure StageBlock/GetBlockList APIs. +func quoteETag(etag string) string { + return fmt.Sprintf("%q", etag) +} + // Creates a new Base64 encoded block id from a 32 bit integer func blockIDInt32ToBase64(blockID int32) string { binaryBlockID := &[4]byte{} // All block IDs are 4 bytes long diff --git a/tests/integration/UploadPart.go b/tests/integration/UploadPart.go index 8576f6e8..1314387e 100644 --- a/tests/integration/UploadPart.go +++ b/tests/integration/UploadPart.go @@ -501,3 +501,87 @@ func UploadPart_success(s *S3Conf) error { return nil }) } + +// isQuotedEtag reports whether an ETag is a non-empty double-quoted string, +// as required by the S3 contract (e.g. "\"abc\""). +func isQuotedEtag(etag string) bool { + return len(etag) >= 3 && + strings.HasPrefix(etag, "\"") && + strings.HasSuffix(etag, "\"") +} + +// UploadPart_etag_quoting_consistency verifies that multipart part ETags are +// returned as quoted strings and stay consistent across the UploadPart +// response, ListParts, and CompleteMultipartUpload. This is the S3 contract +// (ETags are quoted) and matches the posix backend's GenerateEtag convention; +// it is the regression guard for the Azure backend, which previously returned +// raw, unquoted block ids. +func UploadPart_etag_quoting_consistency(s *S3Conf) error { + testName := "UploadPart_etag_quoting_consistency" + 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 + } + + parts, _, err := uploadParts(s3client, 18*1024*1024, 3, bucket, obj, *out.UploadId) + if err != nil { + return err + } + + // Every ETag returned by UploadPart must be a quoted string. + for _, p := range parts { + etag := getString(p.ETag) + if !isQuotedEtag(etag) { + return fmt.Errorf("expected UploadPart etag to be quoted, instead got %q", etag) + } + } + + // ListParts must report the same quoted ETags as UploadPart. + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.ListParts(ctx, &s3.ListPartsInput{ + Bucket: &bucket, + Key: &obj, + UploadId: out.UploadId, + }) + cancel() + if err != nil { + return err + } + for _, p := range res.Parts { + etag := getString(p.ETag) + if !isQuotedEtag(etag) { + return fmt.Errorf("expected ListParts etag to be quoted, instead got %q", etag) + } + } + if ok := compareParts(parts, res.Parts); !ok { + return fmt.Errorf("expected ListParts parts %+v to match UploadPart parts %+v", + res.Parts, parts) + } + + // CompleteMultipartUpload must accept the quoted ETags returned above. + compParts := []types.CompletedPart{} + for _, p := range parts { + compParts = append(compParts, types.CompletedPart{ + ETag: p.ETag, + PartNumber: p.PartNumber, + }) + } + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ + Bucket: &bucket, + Key: &obj, + UploadId: out.UploadId, + MultipartUpload: &types.CompletedMultipartUpload{ + Parts: compParts, + }, + }) + cancel() + if err != nil { + return fmt.Errorf("complete multipart upload with quoted etags: %w", err) + } + + return nil + }) +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 89503a05..5e627c4d 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -460,6 +460,7 @@ func TestUploadPart(ts *TestState) { ts.Run(UploadPart_with_checksums_success) } ts.Run(UploadPart_success) + ts.Run(UploadPart_etag_quoting_consistency) } func TestUploadPartCopy(ts *TestState) { @@ -898,6 +899,11 @@ func TestFullFlow(ts *TestState) { TestDeleteObjectTagging(ts) TestCreateMultipartUpload(ts) TestUploadPart(ts) + // UploadPartCopy maps to Azure StageBlockFromURL, which Azurite does not + // implement (returns HTTP 500 InternalError), so the group is skipped in + // Azure full-flow. The backend now returns a populated, quoted CopyPartResult + // ETag (consistent with UploadPart/ListParts); that path is exercised against + // real Azure rather than Azurite. if !ts.conf.azureTests { TestUploadPartCopy(ts) } @@ -1685,6 +1691,7 @@ func GetIntTests() IntTests { "UploadPart_no_checksum_with_composite_checksum_type": UploadPart_no_checksum_with_composite_checksum_type, "UploadPart_with_checksums_success": UploadPart_with_checksums_success, "UploadPart_success": UploadPart_success, + "UploadPart_etag_quoting_consistency": UploadPart_etag_quoting_consistency, "UploadPartCopy_non_existing_bucket": UploadPartCopy_non_existing_bucket, "UploadPartCopy_incorrect_uploadId": UploadPartCopy_incorrect_uploadId, "UploadPartCopy_incorrect_object_key": UploadPartCopy_incorrect_object_key,