Skip to content

fix ObjectTagging with DeleteMarker #11185

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 1 commit into from
Jul 12, 2024
Merged

fix ObjectTagging with DeleteMarker #11185

merged 1 commit into from
Jul 12, 2024

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jul 11, 2024 •
edited
Loading

Motivation

As reported in #11180, we've had the wrong behavior for DeleteMarker with ObjectTagging operations.

However, my previous assumptions were wrong due to a mistake in a test, which lead to a design decision in the S3 models (the S3Bucket get_object method signature).

We will need to update it to support properly this behavior, however this might break persistence as the previous loaded objects might have the previous signature, and fails when used in the provider. We will need to wait for the next major release to remove the previous parameter raise_for_delete_marker and add a new one permitting use to separate behavior between calls from GetObject vs GetObjectTagging (one raise NoSuchKey and the other MethodNotAllowed).

Changes

  • add a hacky way to support the behavior reported, and add a TODO for the future to fix the behavior without that hack

fixes #11180

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Jul 11, 2024
@bentsku bentsku requested a review from cloutierMat July 11, 2024 14:30
@bentsku bentsku self-assigned this Jul 11, 2024
@github-actions GitHub Actions
Copy link

S3 Image Test Results (AMD64 / ARM64)

  2 files  ±0    2 suites  ±0   3m 32s ⏱️ +12s
404 tests ±0  352 ✅ ±0   52 💤 ±0  0 ❌ ±0 
808 runs  ±0  704 ✅ ±0  104 💤 ±0  0 ❌ ±0 

Results for commit 9bb688e. ± Comparison against base commit b58b0f8.

@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   48m 21s ⏱️ - 45m 50s
1 395 tests  - 1 784  1 243 ✅  - 1 541  152 💤  - 243  0 ❌ ±0 
1 397 runs   - 1 784  1 243 ✅  - 1 541  154 💤  - 243  0 ❌ ±0 

Results for commit 9bb688e. ± Comparison against base commit b58b0f8.

This pull request removes 1784 tests. 
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]
…

cloutierMat
cloutierMat approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@cloutierMat cloutierMat left a comment

Choose a reason for hiding this comment

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

Good thinking for the store!

Nice set of extra test to validate the behaviour!

@bentsku bentsku merged commit 30249e8 into master Jul 12, 2024
46 checks passed
@bentsku bentsku deleted the fix-s3-tag-delete-marker branch July 12, 2024 10:40
chubberlisk added a commit to ministryofjustice/bichard7-next-core that referenced this pull request Jul 15, 2024
@chubberlisk
Skip E2E test for checking locking of S3 file
87dd2fc 
LocalStack doesn't replicate the behaviour of AWS correctly for
DeleteMarker with ObjectTagging operations.

See localstack/localstack#11180.

This issue has been fixed by LocalStack, but has yet to be released
in a numbered version. We want to wait until that happens before we
enable this test and update our Docker Compose file with a new image
version for our LocalStack container.

See localstack/localstack#11185.
@chubberlisk chubberlisk mentioned this pull request Jul 15, 2024
Merged
chubberlisk added a commit to ministryofjustice/bichard7-next-core that referenced this pull request Jul 15, 2024
@chubberlisk
Skip E2E test for checking locking of S3 file
d46764d 
LocalStack doesn't replicate the behaviour of AWS correctly for
DeleteMarker with ObjectTagging operations.

See localstack/localstack#11180.

This issue has been fixed by LocalStack, but has yet to be released
in a numbered version. We want to wait until that happens before we
enable this test and update our Docker Compose file with a new image
version for our LocalStack container.

See localstack/localstack#11185.
chubberlisk added a commit to ministryofjustice/bichard7-next-core that referenced this pull request Jul 15, 2024
@chubberlisk
Skip E2E test for checking locking of S3 file
3e646a4 
LocalStack doesn't replicate the behaviour of AWS correctly for
DeleteMarker with ObjectTagging operations.

See localstack/localstack#11180.

This issue has been fixed by LocalStack, but has yet to be released
in a numbered version. We want to wait until that happens before we
enable this test and update our Docker Compose file with a new image
version for our LocalStack container.

See localstack/localstack#11185.
chubberlisk added a commit to ministryofjustice/bichard7-next-core that referenced this pull request Jul 15, 2024
@github-actions @web-flow @chubberlisk
Update npm dependencies (#859)
842a2b9 
* Update npm dependencies

* Ensure Conductor tests run when there are changes in Core

* Improve readability of lockS3File Conductor task

* Skip E2E test for checking locking of S3 file

LocalStack doesn't replicate the behaviour of AWS correctly for
DeleteMarker with ObjectTagging operations.

See localstack/localstack#11180.

This issue has been fixed by LocalStack, but has yet to be released
in a numbered version. We want to wait until that happens before we
enable this test and update our Docker Compose file with a new image
version for our LocalStack container.

See localstack/localstack#11185.

---------

Co-authored-by: GitHub <noreply@github.com>
Co-authored-by: Wen Ting Wang <42817036+chubberlisk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloutierMat cloutierMat cloutierMat approved these changes

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
No milestone
Development

Successfully merging this pull request may close these issues.

bug: Different behaviour from AWS for getObjectTagging on a delete marker
2 participants
@bentsku @cloutierMat