Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Accept multipart checksums with part count #19680

Merged
merged 2 commits into from
May 8, 2024
Merged

Accept multipart checksums with part count #19680

merged 2 commits into from
May 8, 2024

Conversation

klauspost
Copy link
Contributor

Description

Accept multipart uploads where the combined checksum provides expected part count.

Seems this was added by AWS to make the API more consistent even if the data is completely superfluous on multiple levels.

Fixes #19670

Motivation and Context

Improve AWS S3 compatibility.

How to test this PR?

See #19670

Types of changes

  • New feature (non-breaking change which adds functionality)

@klauspost
Accept multipart checksums with part count
0b245da 
Accept multipart uploads where the combined checksum provides expected part count.

Seems this was added by AWS to make the API more consistent even if the data is completely superfluous on multiple levels.

Improves AWS S3 compatibility.
@klauspost klauspost mentioned this pull request May 6, 2024
Closed
@klauspost klauspost marked this pull request as draft May 6, 2024 17:56
@klauspost
Copy link
Contributor Author

klauspost commented May 6, 2024 •
edited

(draft while checking out some unexpected test results)

Functionality verified by running mint with

this diff (click) 
 api-put-object-multipart.go | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/api-put-object-multipart.go b/api-put-object-multipart.go
index 5f117af..b913f7b 100644
--- a/api-put-object-multipart.go
+++ b/api-put-object-multipart.go
@@ -207,7 +207,8 @@ func (c *Client) putObjectMultipartNoStream(ctx context.Context, bucketName, obj
 		// Add hash of hashes.
 		crc.Reset()
 		crc.Write(crcBytes)
-		opts.UserMetadata = map[string]string{"X-Amz-Checksum-Crc32c": base64.StdEncoding.EncodeToString(crc.Sum(nil))}
+		hash := fmt.Sprintf("%s-%d", base64.StdEncoding.EncodeToString(crc.Sum(nil)), len(complMultipartUpload.Parts))
+		opts.UserMetadata = map[string]string{"X-Amz-Checksum-Crc32c": hash}
 	}
 	uploadInfo, err := c.completeMultipartUpload(ctx, bucketName, objectName, uploadID, complMultipartUpload, opts)
 	if err != nil {
 api-put-object-streaming.go | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/api-put-object-streaming.go b/api-put-object-streaming.go
index 9182d4e..8f87df1 100644
--- a/api-put-object-streaming.go
+++ b/api-put-object-streaming.go
@@ -281,7 +281,8 @@ func (c *Client) putObjectMultipartStreamFromReadAt(ctx context.Context, bucketN
 				crc.Write(cs)
 			}
 		}
-		opts.UserMetadata = map[string]string{"X-Amz-Checksum-Crc32c": base64.StdEncoding.EncodeToString(crc.Sum(nil))}
+		hash := fmt.Sprintf("%s-%d", base64.StdEncoding.EncodeToString(crc.Sum(nil)), len(complMultipartUpload.Parts))
+		opts.UserMetadata = map[string]string{"X-Amz-Checksum-Crc32c": hash}
 	}
 
 	uploadInfo, err := c.completeMultipartUpload(ctx, bucketName, objectName, uploadID, complMultipartUpload, opts)
@@ -438,7 +439,8 @@ func (c *Client) putObjectMultipartStreamOptionalChecksum(ctx context.Context, b
 		// Add hash of hashes.
 		crc.Reset()
 		crc.Write(crcBytes)
-		opts.UserMetadata = map[string]string{"X-Amz-Checksum-Crc32c": base64.StdEncoding.EncodeToString(crc.Sum(nil))}
+		hash := fmt.Sprintf("%s-%d", base64.StdEncoding.EncodeToString(crc.Sum(nil)), len(complMultipartUpload.Parts))
+		opts.UserMetadata = map[string]string{"X-Amz-Checksum-Crc32c": hash}
 	}
 	uploadInfo, err := c.completeMultipartUpload(ctx, bucketName, objectName, uploadID, complMultipartUpload, opts)
 	if err != nil {
@@ -642,7 +644,8 @@ func (c *Client) putObjectMultipartStreamParallel(ctx context.Context, bucketNam
 		// Add hash of hashes.
 		crc.Reset()
 		crc.Write(crcBytes)
-		opts.UserMetadata = map[string]string{"X-Amz-Checksum-Crc32c": base64.StdEncoding.EncodeToString(crc.Sum(nil))}
+		hash := fmt.Sprintf("%s-%d", base64.StdEncoding.EncodeToString(crc.Sum(nil)), len(complMultipartUpload.Parts))
+		opts.UserMetadata = map[string]string{"X-Amz-Checksum-Crc32c": hash}
 	}
 	uploadInfo, err := c.completeMultipartUpload(ctx, bucketName, objectName, uploadID, complMultipartUpload, opts)
 	if err != nil {
 api-put-object.go | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/api-put-object.go b/api-put-object.go
index 4dec604..ecc4201 100644
--- a/api-put-object.go
+++ b/api-put-object.go
@@ -461,7 +461,8 @@ func (c *Client) putObjectMultipartStreamNoLength(ctx context.Context, bucketNam
 		// Add hash of hashes.
 		crc.Reset()
 		crc.Write(crcBytes)
-		opts.UserMetadata = map[string]string{"X-Amz-Checksum-Crc32c": base64.StdEncoding.EncodeToString(crc.Sum(nil))}
+		hash := fmt.Sprintf("%s-%d", base64.StdEncoding.EncodeToString(crc.Sum(nil)), len(complMultipartUpload.Parts))
+		opts.UserMetadata = map[string]string{"X-Amz-Checksum-Crc32c": hash}
 	}
 	uploadInfo, err := c.completeMultipartUpload(ctx, bucketName, objectName, uploadID, complMultipartUpload, opts)
 	if err != nil {
 functional_tests.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I will not change minio-go, since that would just break backcompat.

@klauspost klauspost marked this pull request as ready for review May 6, 2024 18:03
@harshavardhana
Copy link
Member

I will not change minio-go, since that would just break backcompat.

like? @klauspost

@klauspost
Copy link
Contributor Author

@harshavardhana It would break checksums on older minio. AWS supports both.

@harshavardhana
Copy link
Member

@harshavardhana It would break checksums on older minio. AWS supports both.

wait are you saying that this PR will break clients?

@klauspost
Copy link
Contributor Author

@harshavardhana No. This is not minio-go. This is the server. IF we changed minio-go, then it would break on older minio server versions.

@harshavardhana
Copy link
Member

@harshavardhana No. This is not minio-go. This is the server. IF we changed minio-go, then it would break on older minio server versions.

Oh I see so it is an option right?

@klauspost
Copy link
Contributor Author

klauspost commented May 7, 2024 •
edited

@harshavardhana Correct. And the server supports both variants xxxxxx (original S3 implementation) and xxxxx-2 (modified) - as it seems AWS does.

@harshavardhana harshavardhana merged commit ec49fff into minio:master May 8, 2024
20 checks passed
@klauspost klauspost deleted the multipart-checksum-part-count branch May 8, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshavardhana harshavardhana harshavardhana approved these changes

@zveinn zveinn Awaiting requested review from zveinn

@vadmeste vadmeste Awaiting requested review from vadmeste

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Completing Multipart Uploads with Checksum That Contains Number of Parts Fails
3 participants
@klauspost @harshavardhana @vadmeste