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.

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

Add PutObject checksums #1690

Merged
merged 4 commits into from
Sep 16, 2022
Merged

Conversation

klauspost
Copy link
Contributor

@klauspost klauspost commented Aug 26, 2022

Single part, checksums must be done manually for now until we have trailing headers.

For multipart we add CRC32C if we don't already send a checksum and we read the content before sending.

Also serves as test for minio/minio#15433 - but should be backwards compatible.

AWS S3 tests

Single Part

{"args":{"bucketName":"minio-go-test-gyprb0a1v4l9x05q","objectName":"i3d5m60tguye3hkgxe1o3gcijmynm0","opts":"minio.PutObjectOptions{UserMetadata: metadata, Progress: progress}"},"duration":246249,"function":"PutObject(bucketName, objectName, reader,size, opts)","name":"minio-go: testPutObjectWithChecksums","status":"PASS"}

Multipart

These send checksum:
	testPutObjectWithMetadata()
	testPutObjectReadAt()
	testPutObjectsUnknownV2()
	testPutObjectNoLengthV2()

go build functional_tests.go   && ./functional_tests
CHECKSUM: 1yLc4A==
{"args":{"bucketName":"minio-go-test-duyyipdmn2jt2axy","metadata":{"Content-Type":["custom/contenttype"],"X-Amz-Meta-CustomKey":["extra  spaces  in   value"]},"objectName":"zn2pdxznyk6rg46yx2jfifgtemt2u9","opts":"minio.PutObjectOptions{UserMetadata: metadata, Progress: progress}"},"duration":71907,"function":"PutObject(bucketName, objectName, reader,size, opts)","name":"minio-go: testPutObjectWithMetadata","status":"PASS"}
CHECKSUM: 1yLc4A==
{"args":{"bucketName":"minio-go-test-d9smpwyxtqnxcxug","objectContentType":"binary/octet-stream","objectName":"fjuqzickiy94a5i2e1od5exi41bvyh","opts":"objectContentType"},"duration":87869,"function":"PutObject(bucketName, objectName, reader, opts)","name":"minio-go: testPutObjectReadAt","status":"PASS"}
CHECKSUM: 73DecA==
CHECKSUM: 73DecA==
CHECKSUM: 73DecA==
CHECKSUM: 73DecA==
{"args":{"bucketName":"minio-go-test-zlq0e6ayw0hgzmhh","objectName":"minio-go-test-zlq0e6ayw0hgzmhhunique4","opts":"","size":""},"duration":3278,"function":"PutObject(bucketName, objectName, reader,size,opts)","name":"minio-go: testPutObjectsUnknownV2","status":"PASS"}
CHECKSUM: LEgalg==
{"args":{"bucketName":"minio-go-test-f5fssd4wb3opahi0","objectName":"minio-go-test-f5fssd4wb3opahi0unique","opts":"","size":135266304},"duration":46122,"function":"PutObject(bucketName, objectName, reader, size, opts)","name":"minio-go: testPutObjectNoLengthV2","status":"PASS"}	

Seems we are compatible with S3.

Single part, checksums must be done manually for now until we have trailing headers.

For multipart we add CRC32C if we don't already send a checksum and we read the content before sending.

Also serves as test for minio/minio#15433 - but should be backwards compatible.

Needs to be tested against S3, only implemented from docs.
@klauspost klauspost mentioned this pull request Aug 30, 2022
1 task
Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments

api-put-object.go Outdated Show resolved Hide resolved
api-put-object.go Outdated Show resolved Hide resolved
api-put-object.go Show resolved Hide resolved
@klauspost klauspost requested a review from vadmeste September 7, 2022 15:42
@harshavardhana
Copy link
Member

Looks like a nil map crash

{"args":{"bucketName":"minio-go-test-5gemq5a4vzcj1kan","objectName":"d1vjw40g3svlve49qpvwwmpqenud6o"},"duration":15,"function":"GetObject(bucketName, objectName)","name":"minio-go: testGetObjectReadAtFunctionalV2","status":"PASS"}
panic: assignment to entry in nil map

goroutine 1 [running]:
github.com/minio/minio-go/v7.(*Client).putObjectMultipartStreamOptionalChecksum(_, {_, _}, {_, _}, {_, _}, {_, _}, 0x8100000, ...)
	/home/runner/work/minio-go/minio-go/api-put-object-streaming.go:390 +0x163c
github.com/minio/minio-go/v7.(*Client).putObjectMultipartStream(_, {_, _}, {_, _}, {_, _}, {_, _}, 0x8100000, ...)
	/home/runner/work/minio-go/minio-go/api-put-object-streaming.go:51 +0x2fe
github.com/minio/minio-go/v7.(*Client).putObjectCommon(_, {_, _}, {_, _}, {_, _}, {_, _}, 0x8100000, ...)
	/home/runner/work/minio-go/minio-go/api-put-object.go:279 +0x718
github.com/minio/minio-go/v7.(*Client).PutObject(_, {_, _}, {_, _}, {_, _}, {_, _}, 0x8100000, ...)
	/home/runner/work/minio-go/minio-go/api-put-object.go:245 +0x265
main.testGetObjectRanges()
	/home/runner/work/minio-go/minio-go/functional_tests.go:11526 +0x8d5
main.main()
	/home/runner/work/minio-go/minio-go/functional_tests.go:12290 +0x2af

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@klauspost
Copy link
Contributor Author

Ping @harshavardhana

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants