Skip to content

S3: fix exception from ObjectLock retention #12165

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
Jan 23, 2025
Merged

S3: fix exception from ObjectLock retention #12165

merged 2 commits into from
Jan 23, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jan 22, 2025

Motivation

Follow up from regenerating snapshots in #12145, one date that was set in the future actually was now in the past 😅 and triggered a new kind of exception. It had been skipped. This PR now implements this exception.

Also removed a few now useless snapshot markers that have been fixed in the meantime but not removed.

Note: weird thing but S3 returns the error in the PST timezone, interesting 🤔

Changes

  • fix the exception raised in the date was invalid
  • remove a few now useless snapshot markers

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Jan 22, 2025
@bentsku bentsku added this to the 4.1 milestone Jan 22, 2025
@bentsku bentsku self-assigned this Jan 22, 2025
bentsku
bentsku commented Jan 22, 2025
View reviewed changes
localstack-core/localstack/services/s3/provider.py
@@ -2011,7 +2012,7 @@ def restore_object(
return RestoreObjectOutput()

restore_expiration_date = add_expiration_days_to_datetime(
datetime.datetime.utcnow(), restore_days
datetime.datetime.now(datetime.UTC), restore_days
Copy link
Contributor Author

Choose a reason for hiding this comment

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

datetime.datetime.utcnow() is now deprecated and should be replaced by datetime.datetime.now(datetime.UTC)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 22, 2025 •
edited
Loading

S3 Image Test Results (AMD64 / ARM64)

  2 files  ±0    2 suites  ±0   4m 41s ⏱️ -1s
442 tests ±0  389 ✅ ±0   53 💤 ±0  0 ❌ ±0 
884 runs  ±0  778 ✅ ±0  106 💤 ±0  0 ❌ ±0 

Results for commit 345737d. ± Comparison against base commit ed8c76e.

♻️ This comment has been updated with latest results.

@bentsku bentsku requested a review from k-a-il January 22, 2025 18:48
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 22, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   59m 47s ⏱️ - 53m 33s
1 673 tests  - 2 331  1 509 ✅  - 2 178  164 💤  - 153  0 ❌ ±0 
1 675 runs   - 2 331  1 509 ✅  - 2 178  166 💤  - 153  0 ❌ ±0 

Results for commit 345737d. ± Comparison against base commit ed8c76e.

This pull request removes 2333 and adds 2 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.apigateway.test_apigateway_import.TestApiGatewayImportRestApi ‑ test_import_with_integer_http_status_code
tests.aws.services.cloudformation.test_template_engine.TestIntrinsicFunctions ‑ test_join_no_value_construct

♻️ This comment has been updated with latest results.

k-a-il
k-a-il approved these changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@k-a-il k-a-il left a comment

Choose a reason for hiding this comment

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

Nice 👍

tests/aws/services/s3/test_s3.py Outdated
@@ -9409,10 +9377,20 @@ def test_s3_object_retention_exc(self, aws_client, s3_create_bucket, snapshot):
Bucket=s3_bucket_locked,
Key=object_key,
VersionId=version_id,
Retention={"Mode": "GOVERNANCE", "RetainUntilDate": datetime.datetime(2025, 1, 1)},
Retention={"Mode": "GOVERNANCE", "RetainUntilDate": datetime.datetime(2029, 1, 1)},
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: did you consider using a variable datetime based on the current date, like now() + 1 year?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually did not, and this is a much better long term solution 🙃 thanks, will update!

@k-a-il
Copy link
Contributor

k-a-il commented Jan 23, 2025

Note: weird thing but S3 returns the error in the PST timezone, interesting 🤔

yes, indeed, interesting decision from their side

@bentsku bentsku merged commit 44992c0 into master Jan 23, 2025
38 checks passed
@bentsku bentsku deleted the fix-s3-object-lock branch January 23, 2025 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k-a-il k-a-il k-a-il 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
4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@bentsku @k-a-il