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(s3-notifications): multiple notifications doesn't work #28132

Merged
merged 11 commits into from
Dec 15, 2023
Merged

fix(s3-notifications): multiple notifications doesn't work #28132

merged 11 commits into from
Dec 15, 2023

Conversation

corymhall
Copy link
Contributor

@corymhall corymhall commented Nov 25, 2023

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

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.
@aws-cdk-automation aws-cdk-automation requested a review from a team November 25, 2023 10:49
@github-actions github-actions bot added the p2 label Nov 25, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 25, 2023
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 27, 2023

Hi Corey! Good to see you, and thanks! 😄

Is this in reference to #28120 ?

@corymhall
Copy link
Contributor Author

Is this in reference to #28120 ?

@rix0rrr I'm not sure, but I don't think they are exactly the same. This PR should fix the issue where there are multiple notifications in the same stack. It shouldn't be possible for the issue to occur if there is a single notification created per stack.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 27, 2023
@kaizencc
Copy link
Contributor

👋! contribution/core LOL. We gotta get you your real merit badge.

Copy link
Contributor

@kaizencc kaizencc left a 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

@kaizencc kaizencc added the pr/do-not-merge This PR should not be merged at this time. label Dec 12, 2023
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 12, 2023
@kaizencc kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label Dec 14, 2023
Copy link
Contributor

@kaizencc kaizencc left a 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

Copy link
Contributor

mergify bot commented Dec 14, 2023

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: ef86a72
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 37be7b9 into aws:main Dec 15, 2023
10 checks passed
Copy link
Contributor

mergify bot commented Dec 15, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants