-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(s3-notifications): multiple notifications doesn't work #28132
Conversation
When multiple bucket notifications are created it creates a race condition where only the last one processed gets applied. All bucket notifications created in a stack are given the same `stackId` prefix. This prefix is then used to filter out the notification created by the custom resource. If there are other notifications created in the same stack, but not by this custom resource, they get filtered out. This PR fixes that by filtering the notifications by the specific notification id. This ensures that only the notifications created by the individual custom resource are filter out and the rest (included those created by other custom resources) are marked external.
Hi Corey! Good to see you, and thanks! 😄 Is this in reference to #28120 ? |
👋! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a problem with this and it looks like it fixes a pretty egregious bug. Since it touches a custom resource I wanna get @rix0rrr to take a second look before we send it out to the wild
packages/@aws-cdk/custom-resource-handlers/lib/aws-s3/notifications-resource-handler/index.py
Outdated
Show resolved
Hide resolved
…tions-resource-handler/index.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both @rix0rrr and I agree with you that we gotta rewrite this to typescript, but short of getting a volunteer to do that we're just gonna merge this 🤷. Thanks @corymhall
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
When multiple bucket notifications are created it creates a race condition where only the last one processed gets applied. All bucket notifications created in a stack are given the same
stackId
prefix. This prefix is then used to filter out the notification created by the custom resource. If there are other notifications created in the same stack, but not by this custom resource, they get filtered out.This PR fixes that by filtering the notifications by the specific notification id. This ensures that only the notifications created by the individual custom resource are filter out and the rest (included those created by other custom resources) are marked external.
Note - I had to refactor some of the function code to make it fit the inline size limit. This should probably be rewritten in typescript...
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license