fix(sns): race condition exists between sqs queue policy and sns subscription #21259
Conversation
|
|
github-actions
bot
added
bug
TheRealAmazonKendra
changed the title
|
|
||
| // Add the subscription policy as a dependency for the subscription | ||
| const subscriptionDependencies: IDependable[] = []; | ||
| if (queuePolicyDependable) { |
There was a problem hiding this comment.
You're setting this a few lines above. Under what conditions would this not be defined?
There was a problem hiding this comment.
It's not defined if the queue is imported or if autoCreatePolicy is set to false while no queue policy exists: https://github.com/aws/aws-cdk/blob/v1.159.0/packages/%40aws-cdk/aws-sqs/lib/queue-base.ts#L144
| * | ||
| * @default - empty list | ||
| */ | ||
| readonly subscriptionDependencies?: IDependable[]; |
There was a problem hiding this comment.
You're only adding one dependable here so why make it an array? Also, your logic above doesn't seem to have a scenario where subscriptionDependencies is undefined so I'm not sure why this would be optional.
There was a problem hiding this comment.
I made it an array to make it generic, but I guess it's not backwards incompatible to change this in the future to a list if needed.
It needs to be undefined because only the SQS subscription needs to set this, the lambda and email subscription don't need this: https://github.com/aws/aws-cdk/blob/v1.159.0/packages/%40aws-cdk/aws-sns-subscriptions/lib/email.ts, https://github.com/aws/aws-cdk/blob/v1.159.0/packages/%40aws-cdk/aws-sns-subscriptions/lib/sms.ts, https://github.com/aws/aws-cdk/blob/v1.159.0/packages/%40aws-cdk/aws-sns-subscriptions/lib/lambda.ts
There was a problem hiding this comment.
I don't really see any value to make it an array when we only expect one value here. It's adding unnecessary logic elsewhere.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| * | ||
| * @default - empty list | ||
| */ | ||
| readonly subscriptionDependencies?: IDependable[]; |
There was a problem hiding this comment.
I don't really see any value to make it an array when we only expect one value here. It's adding unnecessary logic elsewhere.
|
This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
aws-cdk-automation
added
the
closed-for-staleness
…cription (#21797) ---- Fixes #20665 by adding a dependency to sqs policy for sns subscriptions. ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* This is a follow up to #21259, which got closed for failing for too long
…cription (aws#21797) ---- Fixes aws#20665 by adding a dependency to sqs policy for sns subscriptions. ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* This is a follow up to aws#21259, which got closed for failing for too long
Fixes #20665 by adding a dependency to sqs policy for sns subscriptions.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license