Skip to content

S3: Support EU Bucket Location Constraint for CreateBucket #13450

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
aidehn merged 22 commits into from
Dec 4, 2025
Merged

S3: Support EU Bucket Location Constraint for CreateBucket #13450

aidehn merged 22 commits into from
Dec 4, 2025

Conversation

@aidehn
Copy link
Contributor

@aidehn aidehn commented Dec 1, 2025 •
edited
Loading

Motivation

  • AWS allows for the LocationConstraint field to be provided with the value of EU in the bucket configuration when creating a bucket which we don't currently support.

Changes

  • We now support creating buckets with the Location Constraint of EU which under the hood is just using the eu-west-1 region. I imagine since it was the first EU region to be supported they just called it EU as a generic term.
  • Main issue with implementing this was that when calling get_bucket_location it will actually return the value EU instead of eu-west-1 but in a list_buckets request the region would be eu-west-1, not EU.
    • This means that we need to store the original location constraint somewhere (which I've opted for the S3Bucket dataclass) whilst still using the correct underlying region which is eu-west-1.
  • Updated the create_bucket and get_bucket_location operation to support these changes.
  • Updated the location constraint logic slightly to work with the location_constraint variable rather than the bucket_region
  • The returned get_bucket_location XML format was updated for when there is no location constraint.
  • Added validation for us-east-1 to raise an error for all invalid location constraints, not just us-east-1 and aws-global

TODO

Misc.

Resolves FLC-209

@aidehn aidehn self-assigned this Dec 1, 2025
@aidehn aidehn added semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes labels Dec 1, 2025
@github-actions
Copy link

github-actions bot commented Dec 1, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 889 tests  ±0   21 075 ✅ ±0   6m 23s ⏱️ -3s
     1 suites ±0    1 814 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit af2b802. ± Comparison against base commit 049201f.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Dec 1, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 0s ⏱️ -20s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit af2b802. ± Comparison against base commit 049201f.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Dec 1, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files  ± 0      5 suites  ±0   2h 32m 50s ⏱️ - 11m 25s
5 494 tests +17  4 942 ✅ +17  552 💤 ±0  0 ❌ ±0 
5 500 runs  +17  4 942 ✅ +17  558 💤 ±0  0 ❌ ±0 

Results for commit af2b802. ± Comparison against base commit 049201f.

This pull request removes 1 and adds 18 tests. Note that renamed tests count towards both. 
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_list_buckets_region_validation_disabled

tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_eu_location_constraint
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_eu_location_constraint_raises
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_invalid_location_constraint[eu-west-1-bar]
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_invalid_location_constraint[us-east-1-foo]
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_invalid_location_constraint[us-east-1-us-east-1]
tests.aws.services.s3.test_s3.TestS3 ‑ test_response_structure_get_bucket_location[eu-central-1-eu-central-1-eu-central-1]
tests.aws.services.s3.test_s3.TestS3 ‑ test_response_structure_get_bucket_location[eu-west-1-EU-EU]
tests.aws.services.s3.test_s3.TestS3 ‑ test_response_structure_get_bucket_location[us-east-1-None-]
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_region_validation_non_standard_regions_enabled
tests.aws.services.stepfunctions.v2.test_state.test_state_mock_validation.TestStateMockValidation ‑ test_state_type_does_not_accept_mock[mock_error_output-FailState]
…

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Dec 1, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ± 0      2 suites  ±0   1h 56m 1s ⏱️ - 9m 6s
5 120 tests +17  4 728 ✅ +17  392 💤 ±0  0 ❌ ±0 
5 122 runs  +17  4 728 ✅ +17  394 💤 ±0  0 ❌ ±0 

Results for commit af2b802. ± Comparison against base commit 049201f.

This pull request removes 1 and adds 18 tests. Note that renamed tests count towards both. 
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_list_buckets_region_validation_disabled

tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_eu_location_constraint
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_eu_location_constraint_raises
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_invalid_location_constraint[eu-west-1-bar]
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_invalid_location_constraint[us-east-1-foo]
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_invalid_location_constraint[us-east-1-us-east-1]
tests.aws.services.s3.test_s3.TestS3 ‑ test_response_structure_get_bucket_location[eu-central-1-eu-central-1-eu-central-1]
tests.aws.services.s3.test_s3.TestS3 ‑ test_response_structure_get_bucket_location[eu-west-1-EU-EU]
tests.aws.services.s3.test_s3.TestS3 ‑ test_response_structure_get_bucket_location[us-east-1-None-]
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_region_validation_non_standard_regions_enabled
tests.aws.services.stepfunctions.v2.test_state.test_state_mock_validation.TestStateMockValidation ‑ test_state_type_does_not_accept_mock[mock_error_output-FailState]
…

♻️ This comment has been updated with latest results.

@aidehn aidehn force-pushed the aidehn/update/create-s3-bucket-eu-region branch from bca82d9 to 9d01ec9 Compare December 2, 2025 09:08
@github-actions
Copy link

github-actions bot commented Dec 2, 2025 •
edited
Loading

S3 Image Test Results (AMD64 / ARM64)

    2 files  ± 0      2 suites  ±0   8m 21s ⏱️ +8s
  552 tests + 8    500 ✅ + 8   52 💤 ±0  0 ❌ ±0 
1 104 runs  +16  1 000 ✅ +16  104 💤 ±0  0 ❌ ±0 

Results for commit af2b802. ± Comparison against base commit 049201f.

This pull request removes 1 and adds 9 tests. Note that renamed tests count towards both. 
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_list_buckets_region_validation_disabled

tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_eu_location_constraint
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_eu_location_constraint_raises
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_invalid_location_constraint[eu-west-1-bar]
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_invalid_location_constraint[us-east-1-foo]
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_with_invalid_location_constraint[us-east-1-us-east-1]
tests.aws.services.s3.test_s3.TestS3 ‑ test_response_structure_get_bucket_location[eu-central-1-eu-central-1-eu-central-1]
tests.aws.services.s3.test_s3.TestS3 ‑ test_response_structure_get_bucket_location[eu-west-1-EU-EU]
tests.aws.services.s3.test_s3.TestS3 ‑ test_response_structure_get_bucket_location[us-east-1-None-]
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_region_validation_non_standard_regions_enabled

♻️ This comment has been updated with latest results.

@aidehn aidehn marked this pull request as ready for review December 2, 2025 14:06
@aidehn aidehn requested review from bentsku and k-a-il as code owners December 2, 2025 14:06
bentsku
bentsku reviewed Dec 2, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

Nice work, I think storing the attribute is the right move! We get better parity this way with this weird pseudo-region, and we know we have users using this so better be good on parity!

I have some comments about us now returning the location constraint as a "None" string now in the GetBucketLocation, and we should maybe have better tests around that.

Once this is addressed, this is good to go! 🚀

@aidehn aidehn force-pushed the aidehn/update/create-s3-bucket-eu-region branch from 9d01ec9 to 88083fe Compare December 3, 2025 08:56
aidehn added 15 commits December 3, 2025 09:56
@aidehn
store location constraint on bucket model (might need to change if br…
33c1c60 
…eaks persistence)
@aidehn
update create bucket logic to support EU location contraint
0a26121
@aidehn
add bucket arn to list buckets response
5e13ab2
@aidehn
validated s3 integration test with snapshots
6c56e5a
@aidehn
add another test for using EU location constraint in region that isn'…
4b61ff6 
…t eu-west-1
@aidehn
format
d9fbbf7
@aidehn
bucket arn does not appear for other scenarios
b8b0af7
@aidehn
ignore bucket arn and leave comment
e6b68f5
@aidehn
add eu west 1 region to constants
6c35b95
@aidehn
add bucket location constraints constants
2cbd38d
@aidehn
create illegal location constraint exception
7fb915e
@aidehn
add location constraint to the s3 bucket model
b3b57c0
@aidehn
refactor to work logic on location constraint instead of bucket region
41b4519
@aidehn
use util for get bucket location
8eed55a
@aidehn
add util for get bucket location xml
96b87f1
@aidehn aidehn force-pushed the aidehn/update/create-s3-bucket-eu-region branch from 88083fe to 63f94da Compare December 3, 2025 10:40
@aidehn aidehn requested a review from bentsku December 3, 2025 13:32
aidehn
aidehn commented Dec 3, 2025
View reviewed changes
bentsku
bentsku reviewed Dec 3, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

This looks great, thanks a lot for going the extra mile here! I believe we should keep the deleted test, and add the behavior with config.ALLOW_NONSTANDARD_REGIONS just to be on the safe side. Once this is addressed, this good to go 👍

@aidehn aidehn requested a review from bentsku December 4, 2025 10:39
bentsku
bentsku approved these changes Dec 4, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks for addressing all the comments, this looks awesome! A quick fix actually was a bit more work, but ultimately the code looks much better now, thank you 🙇

Good to merge once the pipeline is all green 🚀

@aidehn
Copy link
Contributor Author

aidehn commented Dec 4, 2025

LGTM, thanks for addressing all the comments, this looks awesome! A quick fix actually was a bit more work, but ultimately the code looks much better now, thank you 🙇

Good to merge once the pipeline is all green 🚀

@bentsku All good! Thank you for the help and the reviews 🔥

@aidehn aidehn merged commit a62eb76 into main Dec 4, 2025
50 checks passed
@aidehn aidehn deleted the aidehn/update/create-s3-bucket-eu-region branch December 4, 2025 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bentsku bentsku bentsku approved these changes

@k-a-il k-a-il Awaiting requested review from k-a-il k-a-il is a code owner

Assignees

@aidehn aidehn

Labels

docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aidehn @bentsku