fix(sns): require topic name for fifo topic #12386 (#12437)

Require topicName property for FIFO SNS topics as a workaround to [issue 681](aws-cloudformation/cloudformation-coverage-roadmap#681) reported in the CloudFormation coverage roadmap. Also adding additional logic to append '.fifo' to FIFO topic name if not provided explicitly by the user. Fixes #12386 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Jan 13, 2021 · 37d8ccc · 37d8ccc
1 parent cd28c29
commit 37d8ccc
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 3 deletions.
diff --git a/packages/@aws-cdk/aws-sns/README.md b/packages/@aws-cdk/aws-sns/README.md
@@ -30,9 +30,12 @@ const topic = new sns.Topic(this, 'Topic', {
     contentBasedDeduplication: true,
     displayName: 'Customer subscription topic',
     fifo: true,
+    topicName: 'customerTopic',
 });
 ```
 
+Note that FIFO topics require a topic name to be provided. The required `.fifo` suffix will be automatically added to the topic name if it is not explicitly provided.
+
 ## Subscriptions
 
 Various subscriptions can be added to the topic by calling the

diff --git a/packages/@aws-cdk/aws-sns/lib/topic.ts b/packages/@aws-cdk/aws-sns/lib/topic.ts
@@ -80,13 +80,26 @@ export class Topic extends TopicBase {
       physicalName: props.topicName,
     });
 
+    if (props.fifo && !props.topicName) {
+      // NOTE: Workaround for CloudFormation problem reported in CDK issue 12386
+      // see https://github.com/aws/aws-cdk/issues/12386
+      throw new Error('FIFO SNS topics must be given a topic name.');
+    }
+
     if (props.contentBasedDeduplication && !props.fifo) {
       throw new Error('Content based deduplication can only be enabled for FIFO SNS topics.');
     }
 
+    let cfnTopicName: string;
+    if (props.fifo && props.topicName && !props.topicName.endsWith('.fifo')) {
+      cfnTopicName = this.physicalName + '.fifo';
+    } else {
+      cfnTopicName = this.physicalName;
+    }
+
     const resource = new CfnTopic(this, 'Resource', {
       displayName: props.displayName,
-      topicName: this.physicalName,
+      topicName: cfnTopicName,
       kmsMasterKeyId: props.masterKey && props.masterKey.keyArn,
       contentBasedDeduplication: props.contentBasedDeduplication,
       fifoTopic: props.fifo,

diff --git a/packages/@aws-cdk/aws-sns/test/integ.sns-fifo.ts b/packages/@aws-cdk/aws-sns/test/integ.sns-fifo.ts
@@ -6,7 +6,7 @@ class SNSFifoInteg extends Stack {
     super(scope, id, props);
 
     new Topic(this, 'MyTopic', {
-      topicName: 'fooTopic.fifo',
+      topicName: 'fooTopic',
       displayName: 'fooDisplayName',
       contentBasedDeduplication: true,
       fifo: true,

diff --git a/packages/@aws-cdk/aws-sns/test/test.sns.ts b/packages/@aws-cdk/aws-sns/test/test.sns.ts
@@ -81,7 +81,7 @@ export = {
       test.done();
     },
 
-    'specify both'(test: Test) {
+    'specify displayName and topicName'(test: Test) {
       const stack = new cdk.Stack();
 
       new sns.Topic(stack, 'MyTopic', {
@@ -104,11 +104,70 @@ export = {
       test.done();
     },
 
+    // NOTE: This test case should be invalid when CloudFormation problem reported in CDK issue 12386 is resolved
+    // see https://github.com/aws/aws-cdk/issues/12386
+    'throw with missing topicName on fifo topic'(test: Test) {
+      const stack = new cdk.Stack();
+
+      test.throws(() => new sns.Topic(stack, 'MyTopic', {
+        fifo: true,
+      }), /FIFO SNS topics must be given a topic name./);
+
+      test.done();
+    },
+
+    'specify fifo without .fifo suffix in topicName'(test: Test) {
+      const stack = new cdk.Stack();
+
+      new sns.Topic(stack, 'MyTopic', {
+        fifo: true,
+        topicName: 'topicName',
+      });
+
+      expect(stack).toMatch({
+        'Resources': {
+          'MyTopic86869434': {
+            'Type': 'AWS::SNS::Topic',
+            'Properties': {
+              'FifoTopic': true,
+              'TopicName': 'topicName.fifo',
+            },
+          },
+        },
+      });
+
+      test.done();
+    },
+
+    'specify fifo with .fifo suffix in topicName'(test: Test) {
+      const stack = new cdk.Stack();
+
+      new sns.Topic(stack, 'MyTopic', {
+        fifo: true,
+        topicName: 'topicName.fifo',
+      });
+
+      expect(stack).toMatch({
+        'Resources': {
+          'MyTopic86869434': {
+            'Type': 'AWS::SNS::Topic',
+            'Properties': {
+              'FifoTopic': true,
+              'TopicName': 'topicName.fifo',
+            },
+          },
+        },
+      });
+
+      test.done();
+    },
+
     'specify fifo without contentBasedDeduplication'(test: Test) {
       const stack = new cdk.Stack();
 
       new sns.Topic(stack, 'MyTopic', {
         fifo: true,
+        topicName: 'topicName',
       });
 
       expect(stack).toMatch({
@@ -117,6 +176,7 @@ export = {
             'Type': 'AWS::SNS::Topic',
             'Properties': {
               'FifoTopic': true,
+              'TopicName': 'topicName.fifo',
             },
           },
         },
@@ -131,6 +191,7 @@ export = {
       new sns.Topic(stack, 'MyTopic', {
         contentBasedDeduplication: true,
         fifo: true,
+        topicName: 'topicName',
       });
 
       expect(stack).toMatch({
@@ -140,6 +201,7 @@ export = {
             'Properties': {
               'ContentBasedDeduplication': true,
               'FifoTopic': true,
+              'TopicName': 'topicName.fifo',
             },
           },
         },