Skip to content
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.

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

S3: Implement cross-account access #8395

Merged
merged 5 commits into from Jun 2, 2023
Merged

Conversation

viren-nadkarni
Copy link
Member

@viren-nadkarni viren-nadkarni commented May 30, 2023

This PR adds tests for cross-account support to S3 for following operations:

  • GetObject
  • PutObject
  • ListObjects

The actual implementation was done in Moto in below linked PRs. This also introduces a shared namespace for buckets.

Depends on:

cc: @dfangl

oh yeah test driven development baby
contingent to Moto PR being merged
@viren-nadkarni viren-nadkarni self-assigned this May 30, 2023
@viren-nadkarni viren-nadkarni added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label May 30, 2023
@coveralls
Copy link

Coverage Status

Coverage: 82.685% (+0.002%) from 82.682% when pulling 72c9aad on s3-cross-accounts into 8ab1a66 on master.

@github-actions
Copy link

github-actions bot commented Jun 1, 2023

LocalStack Community integration with Pro

2 081 tests   1 799 ✔️  1h 18m 52s ⏱️
       2 suites     282 💤
       2 files           0

Results for commit 72c9aad.

@viren-nadkarni viren-nadkarni marked this pull request as ready for review June 1, 2023 10:27
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.

LGTM! I've read the change in moto, and it's not only GetObject, PutObject and ListObjects anymore, right, but it's effectively every single S3 operation that is now cross-account enabled?
This might actually fix #7084!

This is very cool, I'm not sure how ACL are enforced on the moto side, but it should be okay. Awesome, thanks a lot for tackling this!

Bucket="foo",
CreateBucketConfiguration={"LocationConstraint": SECONDARY_TEST_AWS_REGION_NAME},
)
exc.match("BucketAlreadyExists")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I've tested against AWS and its missing a dot at the end of the exception message. Shouldn't be so bad, but we know some IaC/SDK are very picky about this message. I guess we should fix it in moto?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this should be fixed in Moto. I'll try to remember to sneak a fix for this in my next moto PR

@viren-nadkarni
Copy link
Member Author

I've read the change in moto, and it's not only GetObject, PutObject and ListObjects anymore, right, but it's effectively every single S3 operation that is now cross-account enabled?

It may, but this definitely needs further testing before we can claim so!

@viren-nadkarni viren-nadkarni merged commit 58b4fbc into master Jun 2, 2023
28 checks passed
@viren-nadkarni viren-nadkarni deleted the s3-cross-accounts branch June 2, 2023 09:43
@dfangl dfangl added this to the 2.2 milestone Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants