Skip to content

fix S3 GetObjectAttributes and GetObject to return Checksum if requested#7704

Merged
bentsku merged 2 commits intomasterfrom
fix-s3-attr-checksum
Feb 21, 2023
Merged

fix S3 GetObjectAttributes and GetObject to return Checksum if requested#7704
bentsku merged 2 commits intomasterfrom
fix-s3-attr-checksum

Conversation

@bentsku
Copy link
Contributor

@bentsku bentsku commented Feb 16, 2023

This PR implements missing behaviour in S3 regarding checksums. It's not implemented yet in moto and a TODO is written about it:

TODO: AWS computes the provided value and verifies it's the same. Afterwards, it should be returned in every subsequent call

I've implemented the behaviour inside the ASF provider, with multiple AWS validated and snapshotted tests.
As moto does not store the computed or given hash in the key, for minimal changes we can compute the hash when it's requested (aka not often). This PR does not implement additional checksum validation (we have some on PutObject), but simply return the object checksum where it was missing.

For multipart uploads, I added some AWS validated tests, but we need additional behaviour for this, and it's out of scope of this PR.

Fixes:

  • Add Checksum field to GetObjectAttributes if the object was created with a checksum
  • Add Checksum field to GetObject if the object was created with a checksum and ChecksumMode is "ENABLED"
  • Investigate Content-Encoding: '' from AWS response and copy behaviour
  • Set a hardcoded timestamp when creating gzip files so that tests are reproducible
  • Create tests for checksum for multipart uploads (out of scope of this PR to add functionality, would be best in moto)

See:

@bentsku bentsku temporarily deployed to localstack-ext-tests February 16, 2023 18:33 — with GitHub Actions Inactive
@coveralls
Copy link

coveralls commented Feb 16, 2023

Coverage Status

Coverage: 84.974% (-0.02%) from 84.995% when pulling 3b1bfa1 on fix-s3-attr-checksum into 27b4e32 on master.

@github-actions
Copy link

github-actions bot commented Feb 16, 2023

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 40m 14s ⏱️ + 11m 18s
1 736 tests +7  1 382 ✔️ +12  354 💤  - 5  0 ±0 
2 454 runs  +7  1 758 ✔️ +12  696 💤  - 5  0 ±0 

Results for commit 3b1bfa1. ± Comparison against base commit 27b4e32.

♻️ This comment has been updated with latest results.

@bentsku bentsku temporarily deployed to localstack-ext-tests February 17, 2023 21:02 — with GitHub Actions Inactive
@bentsku bentsku marked this pull request as ready for review February 20, 2023 10:26
@bentsku bentsku requested a review from macnev2013 as a code owner February 20, 2023 10:26
Copy link
Contributor

@macnev2013 macnev2013 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@bentsku bentsku merged commit 10914b5 into master Feb 21, 2023
@bentsku bentsku deleted the fix-s3-attr-checksum branch February 21, 2023 12:40
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