From 0cc11caa52f29b82a9ce52b680a1b5afe5d798b2 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 6 Feb 2019 15:56:12 +0100 Subject: [PATCH] fix(sns): create subscription object under subscriber (#1645) Create the SNS Subscription object under the subscriber instead of the subscribee, avoiding cyclic stack dependencies if those objects would be in different stacks. Fixes #1643, fixes #1534. DEPLOYMENT IMPACT: this will recreate any subscription objects under new names. It should not have visible impact on your deployments. --- .../test/integ.project-events.expected.json | 2 +- .../ec2/integ.event-task.lit.expected.json | 4 +- .../test/ec2/integ.lb-awsvpc-nw.expected.json | 2 +- .../test/ec2/integ.lb-bridge-nw.expected.json | 2 +- .../test/integ.sns.expected.json | 4 +- packages/@aws-cdk/aws-sns/lib/topic-base.ts | 25 ++++++++--- ...teg.sns-bucket-notifications.expected.json | 2 +- .../integ.sns-event-rule-target.expected.json | 34 +++++++------- .../test/integ.sns-lambda.expected.json | 45 ++++++++++++------- .../test/integ.sns-sqs.lit.expected.json | 8 ++-- packages/@aws-cdk/aws-sns/test/test.sns.ts | 8 ++-- 11 files changed, 79 insertions(+), 57 deletions(-) diff --git a/packages/@aws-cdk/aws-codebuild/test/integ.project-events.expected.json b/packages/@aws-cdk/aws-codebuild/test/integ.project-events.expected.json index d1c87af113950..21263f56bfa29 100644 --- a/packages/@aws-cdk/aws-codebuild/test/integ.project-events.expected.json +++ b/packages/@aws-cdk/aws-codebuild/test/integ.project-events.expected.json @@ -345,7 +345,7 @@ "MyTopic86869434": { "Type": "AWS::SNS::Topic" }, - "MyTopicMyQueueSubscription3245B11E": { + "MyQueueMyTopicSubscriptionEB66AD1B": { "Type": "AWS::SNS::Subscription", "Properties": { "Endpoint": { diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/integ.event-task.lit.expected.json b/packages/@aws-cdk/aws-ecs/test/ec2/integ.event-task.lit.expected.json index d7925a2d328ec..c0fc2a4bc026d 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/integ.event-task.lit.expected.json +++ b/packages/@aws-cdk/aws-ecs/test/ec2/integ.event-task.lit.expected.json @@ -345,7 +345,7 @@ "EcsClusterDefaultAutoScalingGroupDrainECSHookTopicC705BD25": { "Type": "AWS::SNS::Topic" }, - "EcsClusterDefaultAutoScalingGroupDrainECSHookTopicFunctionSubscription4313BD38": { + "EcsClusterDefaultAutoScalingGroupDrainECSHookFunctionTopicSubscriptionDA5F8A10": { "Type": "AWS::SNS::Subscription", "Properties": { "Endpoint": { @@ -1090,4 +1090,4 @@ "Description": "S3 key for asset version \"aws-ecs-integ-ecs/AdoptEcrRepositorydbc60defc59544bcaa5c28c95d68f62c/Code\"" } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json b/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json index c70b4bdd68f30..b915396a9c18f 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json +++ b/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json @@ -501,7 +501,7 @@ "EcsClusterDefaultAutoScalingGroupDrainECSHookTopicC705BD25": { "Type": "AWS::SNS::Topic" }, - "EcsClusterDefaultAutoScalingGroupDrainECSHookTopicFunctionSubscription4313BD38": { + "EcsClusterDefaultAutoScalingGroupDrainECSHookFunctionTopicSubscriptionDA5F8A10": { "Type": "AWS::SNS::Subscription", "Properties": { "Endpoint": { diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json b/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json index 0d9d1497b6d86..4bb3eea3c4626 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json +++ b/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json @@ -522,7 +522,7 @@ "EcsClusterDefaultAutoScalingGroupDrainECSHookTopicC705BD25": { "Type": "AWS::SNS::Topic" }, - "EcsClusterDefaultAutoScalingGroupDrainECSHookTopicFunctionSubscription4313BD38": { + "EcsClusterDefaultAutoScalingGroupDrainECSHookFunctionTopicSubscriptionDA5F8A10": { "Type": "AWS::SNS::Subscription", "Properties": { "Endpoint": { diff --git a/packages/@aws-cdk/aws-lambda-event-sources/test/integ.sns.expected.json b/packages/@aws-cdk/aws-lambda-event-sources/test/integ.sns.expected.json index 94a40b16aada1..b5e5c86e85067 100644 --- a/packages/@aws-cdk/aws-lambda-event-sources/test/integ.sns.expected.json +++ b/packages/@aws-cdk/aws-lambda-event-sources/test/integ.sns.expected.json @@ -66,7 +66,7 @@ "TD925BC7E": { "Type": "AWS::SNS::Topic" }, - "TFSubscription47A24A95": { + "FTSubscription775EAF05": { "Type": "AWS::SNS::Subscription", "Properties": { "Endpoint": { @@ -82,4 +82,4 @@ } } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-sns/lib/topic-base.ts b/packages/@aws-cdk/aws-sns/lib/topic-base.ts index b3f189571f29b..bc99502e63b54 100644 --- a/packages/@aws-cdk/aws-sns/lib/topic-base.ts +++ b/packages/@aws-cdk/aws-sns/lib/topic-base.ts @@ -136,14 +136,19 @@ export abstract class TopicBase extends cdk.Construct implements ITopic { * @param queue The target queue */ public subscribeQueue(queue: sqs.IQueue): Subscription { - const subscriptionName = queue.node.id + 'Subscription'; - if (this.node.tryFindChild(subscriptionName)) { + if (!cdk.Construct.isConstruct(queue)) { + throw new Error(`The supplied Queue object must be an instance of Construct`); + } + + const subscriptionName = this.node.id + 'Subscription'; + if (queue.node.tryFindChild(subscriptionName)) { throw new Error(`A subscription between the topic ${this.node.id} and the queue ${queue.node.id} already exists`); } // we use the queue name as the subscription's. there's no meaning to subscribing - // the same queue twice on the same topic. - const sub = new Subscription(this, subscriptionName, { + // the same queue twice on the same topic. Create subscription under *consuming* + // construct to make sure it ends up in the correct stack in cases of cross-stack subscriptions. + const sub = new Subscription(queue, subscriptionName, { topic: this, endpoint: queue.queueArn, protocol: SubscriptionProtocol.Sqs @@ -170,13 +175,19 @@ export abstract class TopicBase extends cdk.Construct implements ITopic { * @param lambdaFunction The Lambda function to invoke */ public subscribeLambda(lambdaFunction: lambda.IFunction): Subscription { - const subscriptionName = lambdaFunction.id + 'Subscription'; + if (!cdk.Construct.isConstruct(lambdaFunction)) { + throw new Error(`The supplied lambda Function object must be an instance of Construct`); + } + + const subscriptionName = this.node.id + 'Subscription'; - if (this.node.tryFindChild(subscriptionName)) { + if (lambdaFunction.node.tryFindChild(subscriptionName)) { throw new Error(`A subscription between the topic ${this.node.id} and the lambda ${lambdaFunction.id} already exists`); } - const sub = new Subscription(this, subscriptionName, { + // Create subscription under *consuming* construct to make sure it ends up + // in the correct stack in cases of cross-stack subscriptions. + const sub = new Subscription(lambdaFunction, subscriptionName, { topic: this, endpoint: lambdaFunction.functionArn, protocol: SubscriptionProtocol.Lambda diff --git a/packages/@aws-cdk/aws-sns/test/integ.sns-bucket-notifications.expected.json b/packages/@aws-cdk/aws-sns/test/integ.sns-bucket-notifications.expected.json index 6e70240782358..b6d814cc219f7 100644 --- a/packages/@aws-cdk/aws-sns/test/integ.sns-bucket-notifications.expected.json +++ b/packages/@aws-cdk/aws-sns/test/integ.sns-bucket-notifications.expected.json @@ -204,4 +204,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-sns/test/integ.sns-event-rule-target.expected.json b/packages/@aws-cdk/aws-sns/test/integ.sns-event-rule-target.expected.json index afa32e0b0348a..39a497da7ea9a 100644 --- a/packages/@aws-cdk/aws-sns/test/integ.sns-event-rule-target.expected.json +++ b/packages/@aws-cdk/aws-sns/test/integ.sns-event-rule-target.expected.json @@ -3,28 +3,12 @@ "MyTopic86869434": { "Type": "AWS::SNS::Topic" }, - "MyTopicMyQueueSubscription3245B11E": { - "Type": "AWS::SNS::Subscription", - "Properties": { - "Endpoint": { - "Fn::GetAtt": [ - "MyQueueE6CA6235", - "Arn" - ] - }, - "Protocol": "sqs", - "TopicArn": { - "Ref": "MyTopic86869434" - } - } - }, "MyTopicPolicy12A5EC17": { "Type": "AWS::SNS::TopicPolicy", "Properties": { "PolicyDocument": { "Statement": [ { - "Sid": "0", "Action": "sns:Publish", "Effect": "Allow", "Principal": { @@ -32,7 +16,8 @@ }, "Resource": { "Ref": "MyTopic86869434" - } + }, + "Sid": "0" } ], "Version": "2012-10-17" @@ -62,6 +47,21 @@ "MyQueueE6CA6235": { "Type": "AWS::SQS::Queue" }, + "MyQueueMyTopicSubscriptionEB66AD1B": { + "Type": "AWS::SNS::Subscription", + "Properties": { + "Endpoint": { + "Fn::GetAtt": [ + "MyQueueE6CA6235", + "Arn" + ] + }, + "Protocol": "sqs", + "TopicArn": { + "Ref": "MyTopic86869434" + } + } + }, "MyQueuePolicy6BBEDDAC": { "Type": "AWS::SQS::QueuePolicy", "Properties": { diff --git a/packages/@aws-cdk/aws-sns/test/integ.sns-lambda.expected.json b/packages/@aws-cdk/aws-sns/test/integ.sns-lambda.expected.json index 800d7778834ae..c4ee7b98f7edf 100644 --- a/packages/@aws-cdk/aws-sns/test/integ.sns-lambda.expected.json +++ b/packages/@aws-cdk/aws-sns/test/integ.sns-lambda.expected.json @@ -3,21 +3,6 @@ "MyTopic86869434": { "Type": "AWS::SNS::Topic" }, - "MyTopicEchoSubscription021036AD": { - "Type": "AWS::SNS::Subscription", - "Properties": { - "Endpoint": { - "Fn::GetAtt": [ - "Echo11F3FB29", - "Arn" - ] - }, - "Protocol": "lambda", - "TopicArn": { - "Ref": "MyTopic86869434" - } - } - }, "EchoServiceRoleBE28060B": { "Type": "AWS::IAM::Role", "Properties": { @@ -34,7 +19,18 @@ "Version": "2012-10-17" }, "ManagedPolicyArns": [ - {"Fn::Join":["",["arn:",{"Ref":"AWS::Partition"},":iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"]]} + { + "Fn::Join": [ + "", + [ + "arn:", + { + "Ref": "AWS::Partition" + }, + ":iam::aws:policy/service-role/AWSLambdaBasicExecutionRole" + ] + ] + } ] } }, @@ -57,6 +53,21 @@ "EchoServiceRoleBE28060B" ] }, + "EchoMyTopicSubscriptionA634C6D7": { + "Type": "AWS::SNS::Subscription", + "Properties": { + "Endpoint": { + "Fn::GetAtt": [ + "Echo11F3FB29", + "Arn" + ] + }, + "Protocol": "lambda", + "TopicArn": { + "Ref": "MyTopic86869434" + } + } + }, "EchoMyTopicF6EBB45F": { "Type": "AWS::Lambda::Permission", "Properties": { @@ -71,4 +82,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-sns/test/integ.sns-sqs.lit.expected.json b/packages/@aws-cdk/aws-sns/test/integ.sns-sqs.lit.expected.json index 40b6fbae330d0..e0dd645a519ea 100644 --- a/packages/@aws-cdk/aws-sns/test/integ.sns-sqs.lit.expected.json +++ b/packages/@aws-cdk/aws-sns/test/integ.sns-sqs.lit.expected.json @@ -3,7 +3,10 @@ "MyTopic86869434": { "Type": "AWS::SNS::Topic" }, - "MyTopicMyQueueSubscription3245B11E": { + "MyQueueE6CA6235": { + "Type": "AWS::SQS::Queue" + }, + "MyQueueMyTopicSubscriptionEB66AD1B": { "Type": "AWS::SNS::Subscription", "Properties": { "Endpoint": { @@ -18,9 +21,6 @@ } } }, - "MyQueueE6CA6235": { - "Type": "AWS::SQS::Queue" - }, "MyQueuePolicy6BBEDDAC": { "Type": "AWS::SQS::QueuePolicy", "Properties": { diff --git a/packages/@aws-cdk/aws-sns/test/test.sns.ts b/packages/@aws-cdk/aws-sns/test/test.sns.ts index 6befe1dd706e4..bcbf6398d3bbe 100644 --- a/packages/@aws-cdk/aws-sns/test/test.sns.ts +++ b/packages/@aws-cdk/aws-sns/test/test.sns.ts @@ -149,7 +149,7 @@ export = { "TopicName": "topicName" } }, - "MyTopicMyQueueSubscription3245B11E": { + "MyQueueMyTopicSubscriptionEB66AD1B": { "Type": "AWS::SNS::Subscription", "Properties": { "Endpoint": { @@ -233,7 +233,7 @@ export = { "TopicName": "topicName" } }, - "MyTopicMyFuncSubscriptionEAF54A3F": { + "MyFuncMyTopicSubscription708A6535": { "Type": "AWS::SNS::Subscription", "Properties": { "Endpoint": { @@ -368,7 +368,7 @@ export = { "TopicName": "topicName" } }, - "MyTopicMyQueueSubscription3245B11E": { + "MyQueueMyTopicSubscriptionEB66AD1B": { "Type": "AWS::SNS::Subscription", "Properties": { "Endpoint": { @@ -383,7 +383,7 @@ export = { } } }, - "MyTopicMyFuncSubscriptionEAF54A3F": { + "MyFuncMyTopicSubscription708A6535": { "Type": "AWS::SNS::Subscription", "Properties": { "Endpoint": {