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.

Sign up for GitHub

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(sns): race condition exists between sqs queue policy and sns subscription #21259

Closed
wants to merge 3 commits into from
+34 −7
Closed

fix(sns): race condition exists between sqs queue policy and sns subscription #21259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8ac13b
fix(sns): add dependency to sqs policy for sns subscription
jonnekaunisto Jul 20, 2022
dde620f
Fix linting error
jonnekaunisto Jul 20, 2022
87a1b3d
Update CloudFormation package snapshot
jonnekaunisto Jul 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ...-cdk/aws-cloudformation/test/nested-stack.integ.snapshot/nested-stacks-test.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
},
"SubscriberQueuenestedstackstestNestedStack1topic089C5EB1396F65087": {
"Type": "AWS::SNS::Subscription",
"DependsOn": "SubscriberQueuePolicy25A0799E",
"Properties": {
"Protocol": "sqs",
"TopicArn": {
Expand All @@ -109,6 +110,7 @@
},
"SubscriberQueuenestedstackstestNestedStack1topic1150E1A929A2C267E": {
"Type": "AWS::SNS::Subscription",
"DependsOn": "SubscriberQueuePolicy25A0799E",
"Properties": {
"Protocol": "sqs",
"TopicArn": {
Expand All @@ -127,6 +129,7 @@
},
"SubscriberQueuenestedstackstestNestedStack1topic209B8719858511914": {
"Type": "AWS::SNS::Subscription",
"DependsOn": "SubscriberQueuePolicy25A0799E",
"Properties": {
"Protocol": "sqs",
"TopicArn": {
Expand Down
13 changes: 10 additions & 3 deletions packages/@aws-cdk/aws-sns-subscriptions/lib/sqs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as sns from '@aws-cdk/aws-sns';
import * as sqs from '@aws-cdk/aws-sqs';
import { ArnFormat, FeatureFlags, Names, Stack, Token } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import { Construct, IDependable } from 'constructs';
import { SubscriptionProps } from './subscription';

/**
Expand Down Expand Up @@ -40,14 +40,20 @@ export class SqsSubscription implements sns.ITopicSubscription {

// add a statement to the queue resource policy which allows this topic
// to send messages to the queue.
this.queue.addToResourcePolicy(new iam.PolicyStatement({
const queuePolicyDependable = this.queue.addToResourcePolicy(new iam.PolicyStatement({
resources: [this.queue.queueArn],
actions: ['sqs:SendMessage'],
principals: [snsServicePrincipal],
conditions: {
ArnEquals: { 'aws:SourceArn': topic.topicArn },
},
}));
})).policyDependable;

// Add the subscription policy as a dependency for the subscription
const subscriptionDependencies: IDependable[] = [];
if (queuePolicyDependable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're setting this a few lines above. Under what conditions would this not be defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

subscriptionDependencies.push(queuePolicyDependable);
}

// if the queue is encrypted, add a statement to the key resource policy
// which allows this topic to decrypt KMS keys
Expand Down Expand Up @@ -77,6 +83,7 @@ export class SqsSubscription implements sns.ITopicSubscription {
filterPolicy: this.props.filterPolicy,
region: this.regionFromArn(topic),
deadLetterQueue: this.props.deadLetterQueue,
subscriptionDependencies,
};
}

Expand Down
1 change: 1 addition & 0 deletions ...s-cdk/aws-sns-subscriptions/test/sns-sqs.lit.integ.snapshot/aws-cdk-sns-sqs.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
},
"MyQueueawscdksnssqsMyTopic9361DEA223429051": {
"Type": "AWS::SNS::Subscription",
"DependsOn" : "MyQueuePolicy6BBEDDAC",
"Properties": {
"Protocol": "sqs",
"TopicArn": {
Expand Down
11 changes: 10 additions & 1 deletion packages/@aws-cdk/aws-sns/lib/subscriber.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Construct } from 'constructs';
import { Construct, IDependable } from 'constructs';
import { SubscriptionOptions } from './subscription';
import { ITopic } from './topic-base';

Expand All @@ -24,6 +24,15 @@ export interface TopicSubscriptionConfig extends SubscriptionOptions {
* subscribing to.
*/
readonly subscriberId: string;

/**
* The resources that need to be created before the subscription can be safely created.
* For example for SQS subscription, the subscription needs to have a dependency on the SQS queue policy
* in order for the subscription to successfully deliver messages.
*
* @default - empty list
*/
readonly subscriptionDependencies?: IDependable[];
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

/**
Expand Down
13 changes: 10 additions & 3 deletions packages/@aws-cdk/aws-sns/lib/topic-base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ export abstract class TopicBase extends Resource implements ITopic {
/**
* Subscribe some endpoint to this topic
*/
public addSubscription(subscription: ITopicSubscription): Subscription {
const subscriptionConfig = subscription.bind(this);
public addSubscription(topicSubscription: ITopicSubscription): Subscription {
const subscriptionConfig = topicSubscription.bind(this);

const scope = subscriptionConfig.subscriberScope || this;
let id = subscriptionConfig.subscriberId;
Expand All @@ -95,10 +95,17 @@ export abstract class TopicBase extends Resource implements ITopic {
throw new Error(`A subscription with id "${id}" already exists under the scope ${scope.node.path}`);
}

return new Subscription(scope, id, {
const subscription = new Subscription(scope, id, {
topic: this,
...subscriptionConfig,
});

// Add dependencies for the subscription
subscriptionConfig.subscriptionDependencies?.forEach(subscriptionDependency => {
subscription.node.addDependency(subscriptionDependency);
});

return subscription;
}

/**
Expand Down