implement S3 cross account across all operations #9121
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bentsku
commented
Sep 12, 2023
| if not context: | ||
| return s3_stores[get_aws_account_id()][aws_stack.get_region()] | ||
| def get_store(account_id: Optional[str] = None, region: Optional[str] = None) -> S3Store: | ||
| return s3_stores[account_id or DEFAULT_AWS_ACCOUNT_ID][region or AWS_REGION_US_EAST_1] |
There was a problem hiding this comment.
note: not sure this is needed, but as it was optional before and I didn't check for all usages, better be safe than sorry for now. The new provider doesn't allow optional values.
287eb57 to
6658e77
Compare
|
We will have some conflicts with #9023 where I also introduce the account ID in Could we wait until that one is merged? I'm happy to take this PR over the line after that. |
|
@viren-nadkarni sure, no problem. I'd like to keep the difference in name between |
viren-nadkarni
approved these changes
Sep 13, 2023
|
@bentsku: my PR will take a bit more work before it is merged. You can go ahead and merge this one, I'll fix any conflicts on my end 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Issues with S3 Website were found from #9017, which showed a deeper issue in our S3 cross/multi account implementation.
GetBucketWebsitewould returnNoSuchBucketif a request from account A was done to a bucket in account B, even though you could get an object from account B.We need to implement proper cross-account access for S3.
Changes
Make all S3 access cross-account
For the current provider:
Before, if we tried to access a bucket existing in account A from account B, we would get some weird issue, as we got the bucket from moto but we would then access the store from account B, which wouldn't have the bucket properties. We now use the found moto bucket account id and region to fetch the store.
For the v3 provider:
We now use the cross account access for every S3 operation, which will allow us to have some implementation of ACL if not using bucket policies, instead of returning
NoSuchBucketif the bucket didn't exist in the caller account. This will need multi account setup in our test suite at some point?Misc
Added a quick cross region test to verify it, and we didn't have one earlier for bucket operations.
Also sneaked in a little change for the request, I would get a big logging error that the account_id and region were not passed to the logging format string, following #9084 @whummer. This might just returndeleted as it's now part of another PR.None/None, not sure if that would still be helping or if we should refactor the format string?