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

fix inter services IAM for S3 Notifications #9411

Merged
merged 2 commits into from Oct 20, 2023
Merged

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Oct 20, 2023

Motivation

Follow up from #9370, to add proper region support to S3 notifications, I've added the region to the client's creation, but also added the account id as done in #9023 for sending notifications. I'll trigger an -ext run on this PR and link it here. Sorry!

https://github.com/localstack/localstack-ext/actions/runs/6582528350

https://github.com/localstack/localstack-ext/runs/17885245621
S3 notifications are fixed, only appsync remains

Changes

Remove the account id from the client creation to trigger test event.

Also removed the account id from the client's creation to send to SQS/Lambda/SNS. But this might create issue again with multi account? \cc @viren-nadkarni why was this change made in the first place?

I believe that SNS/Lambda/SQS will retrieve the account id from the ARN passed and not from the credentials of the request to then retrieve the resource in their store, is that right?

@bentsku bentsku added aws:iam AWS Identity and Access Management aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Oct 20, 2023
@bentsku bentsku requested a review from dfangl October 20, 2023 01:45
@bentsku bentsku self-assigned this Oct 20, 2023
@github-actions
Copy link

LocalStack Community integration with Pro

       2 files         2 suites   1h 12m 43s ⏱️
2 261 tests 1 686 ✔️ 575 💤 0
2 262 runs  1 686 ✔️ 576 💤 0

Results for commit 3de10b0.

Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing it :)

@bentsku bentsku merged commit 030e879 into master Oct 20, 2023
33 checks passed
@bentsku bentsku deleted the fix-s3-notification-iam branch October 20, 2023 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:iam AWS Identity and Access Management aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants