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 awsRegion field in S3 notification #9370

Merged
merged 3 commits into from Oct 19, 2023
Merged

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Oct 17, 2023

Motivation

It had been shown in the multi-account tests that we've had an issue where the awsRegion was not correct. It was using the client request region, when it should have been the bucket region.

Also, per the documentation for events: https://docs.aws.amazon.com/AmazonS3/latest/userguide/ev-events.html, the region field for the event should be the bucket region, which means the events client needs to be created with the bucket region and not the client region, as the region field in the event is from the internal client.

Also validated the behaviour with AWS, that the region was of the bucket and not the client.

Changes

Fix the event payload for SQS/SNS/Lambda to use the region of the bucket for the awsRegion field.
Fix the region for the events client used in the notifier.

Add 2 tests to validate AWS behaviour (one created a bucket in another region than the default client for SQS, and one creating a client in a secondary region and the bucket in the default one).

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Oct 17, 2023
@bentsku bentsku self-assigned this Oct 17, 2023
@github-actions
Copy link

github-actions bot commented Oct 17, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 9m 12s ⏱️
2 258 tests 1 754 ✔️ 504 💤 0
2 259 runs  1 754 ✔️ 505 💤 0

Results for commit 3bd070d.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Oct 17, 2023

Coverage Status

coverage: 82.993% (-0.003%) from 82.996% when pulling 3bd070d on fix-s3-notification-region into 6662d91 on master.

Copy link
Contributor

@macnev2013 macnev2013 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 🚀 LGTM

@bentsku bentsku merged commit 5f326c7 into master Oct 19, 2023
29 checks passed
@bentsku bentsku deleted the fix-s3-notification-region branch October 19, 2023 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

3 participants