S3: fix tagging with cross region calls#13834
Conversation
bentsku
added
aws:s3
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 8m 9s ⏱️ Results for commit e38de90. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 1h 37m 23s ⏱️ Results for commit e38de90. ♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files 2 suites 1h 15m 39s ⏱️ Results for commit e38de90. ♻️ This comment has been updated with latest results. |
| key_id = get_unique_key_id(bucket, object_key, version_id) | ||
| self._remove_all_object_tags(store, key_id) | ||
| store.tags.delete_all_tags(key_id) | ||
| print(f"{tagging=}") |
There was a problem hiding this comment.
(also on line 4613, 4615, 3373, 3374)
aidehn
left a comment
aidehn
left a comment
There was a problem hiding this comment.
Thank you for picking this up! (and sorry for introducing the regression in the last PR 😅) - the logic looks a lot cleaner now too without all the tagging methods 👍🏼
Just some comments to remove the remaining print statements! Also, out of curiosity do we know why the integration tests didn't pick this up on the PR merged?
Yes, this wasn't an issue because our regular integration tests do not run tests in non default account/region, so this case only happened when we don't use |
2 participants
Motivation
Following #13821, we switch S3 tags to be a local attribute instead of a cross account one, which is the right behavior, tagging should be scoped per region, as testing with RGTA showed.
However, our S3 code was not fully ready for this change: when fetching the bucket and the store associated with it, we did not take the region into account, and would we return only a store that had the same account as the bucket. This wasn't an issue before because all attributes were cross-region. Now that tagging is local, this started to become an issue, as if you sent a request from
us-east-1, you wouldn't be able to see the tags if the bucket came fromeu-west-1for example.The fix is in the
_get_cross_account_bucketchange.The rest is just refactoring following the RGTA architectural change, to use the new clean
TagsAPI directly 👍All the
_buckettagging utility functions that were there already had the right behavior and were re-fetching the right store, but we should have the right store from the beginning available in the provider operation 👍Changes
_get_cross_account_bucketto return the right regional store with the bucketTests
Related