Skip to content

implement S3 conditional write IfMatch #11941

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
Merged
merged 2 commits into from
Nov 27, 2024
Merged

implement S3 conditional write IfMatch #11941

merged 2 commits into from
Nov 27, 2024

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Nov 27, 2024

Motivation

Depends on #11940

AWS just released a new Conditional Write feature with IfMatch (also called Compare-and-Swap) with https://aws.amazon.com/about-aws/whats-new/2024/11/amazon-s3-functionality-conditional-writes/

Documented at: https://docs.aws.amazon.com/AmazonS3/latest/userguide/conditional-writes.html

This is a simple feature to implement now that we have everything is place already. Only issue is that we needed the latest ASF specs from the last botocore update.

This is needed for apache/arrow-rs-object-store#33 and would unblock the following PR: apache/arrow-rs#6802

Behavior is as expected by the documentation, except for CompleteMultipartUpload where executing an operation between a CreateMultipartUpload and CompleteMultipartUpload will trigger a 409 Conflict exception, and only a restart can unblock the situation (this was documented for IfNoneMatch, but not for IfMatch).

Changes

  • implement the IfMatch parameter and behavior for PutObject and CompleteMultipartUpload

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Nov 27, 2024
@bentsku bentsku self-assigned this Nov 27, 2024
@bentsku bentsku added this to the 4.0.3 milestone Nov 27, 2024
@bentsku bentsku mentioned this pull request Nov 27, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 27, 2024 •
edited
Loading

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   4m 37s ⏱️
441 tests 389 ✅  52 💤 0 ❌
882 runs  778 ✅ 104 💤 0 ❌

Results for commit 0ed6453.

♻️ This comment has been updated with latest results.

@benesch
Copy link
Contributor

benesch commented Nov 27, 2024

Thanks again for turning this around so fast, @bentsku! Very excited for this.

Base automatically changed from asf-auto-updates to master November 27, 2024 08:19
@viren-nadkarni viren-nadkarni removed their request for review November 27, 2024 08:56
@bentsku bentsku force-pushed the s3-conditional-write-if-match branch from bbd50e1 to a879437 Compare November 27, 2024 10:09
@bentsku bentsku removed the request for review from macnev2013 November 27, 2024 10:09
dominikschubert
dominikschubert approved these changes Nov 27, 2024
View reviewed changes
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

Kudos on immediately jumping on this new feature.
Gave the logic a quick sanity check and didn't find any obvious issues with it 👍

Nice attention to edge cases as well 🚀

tests/aws/services/s3/test_s3_api.py
Comment on lines +2003 to +2005
# it seems that even if we overwrite the object with the same content, S3 will still reject the request if a
# write operation was done between creation and completion of the multipart upload, like the `Delete`
# counterpart of `IfNoneMatch`
Copy link
Member

Choose a reason for hiding this comment

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

question: because every write changes the ETag I assume?

Copy link
Contributor Author

@bentsku bentsku Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

almost, if the content of the object is the same, the ETag does not change, it's most of the time a MD5 hash of the object content. Which is why it's a bit surprising! I also gave a quick write up here: https://github.com/apache/arrow-rs/issues/6799#issuecomment-2502344533
(I say most of the time because with encryption, sometimes it's the hash of the encrypted object)

This behavior was documented for IfNoneMatch, but not for IfMatch it seems, but I suspect it's because they use the same logic/primitive for both.

@bentsku
Copy link
Contributor Author

bentsku commented Nov 27, 2024 •
edited
Loading

Thanks a lot for the review @dominikschubert 🙏 I've just pushed a new test because I thought of some edge case I wanted to test, as we never know... 😅 (overwrite a multipart which overwrite a multipart which overwrite a put object 🌀) but things are all good, no code change. I'll merge once it's green again! Thanks again for the very quick review!

@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   55m 29s ⏱️ - 54m 49s
1 608 tests  - 2 159  1 437 ✅  - 1 984  171 💤  - 175  0 ❌ ±0 
1 610 runs   - 2 159  1 437 ✅  - 1 984  173 💤  - 175  0 ❌ ±0 

Results for commit 0ed6453. ± Comparison against base commit cf4b124.

This pull request removes 2167 and adds 8 tests. Note that renamed tests count towards both. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.s3.test_s3_api.TestS3ObjectWritePrecondition ‑ test_multipart_if_match_etag
tests.aws.services.s3.test_s3_api.TestS3ObjectWritePrecondition ‑ test_multipart_if_match_with_delete
tests.aws.services.s3.test_s3_api.TestS3ObjectWritePrecondition ‑ test_multipart_if_match_with_put
tests.aws.services.s3.test_s3_api.TestS3ObjectWritePrecondition ‑ test_multipart_if_match_with_put_identical
tests.aws.services.s3.test_s3_api.TestS3ObjectWritePrecondition ‑ test_put_object_if_match
tests.aws.services.s3.test_s3_api.TestS3ObjectWritePrecondition ‑ test_put_object_if_match_and_if_none_match_validation
tests.aws.services.s3.test_s3_api.TestS3ObjectWritePrecondition ‑ test_put_object_if_match_validation
tests.aws.services.s3.test_s3_api.TestS3ObjectWritePrecondition ‑ test_put_object_if_match_versioned_bucket

@bentsku bentsku merged commit b28bad3 into master Nov 27, 2024
39 checks passed
@bentsku bentsku deleted the s3-conditional-write-if-match branch November 27, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominikschubert dominikschubert dominikschubert approved these changes

@cloutierMat cloutierMat Awaiting requested review from cloutierMat

Assignees

@bentsku bentsku

Labels
aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.0.3
Development

Successfully merging this pull request may close these issues.
3 participants
@bentsku @benesch @dominikschubert