aws_sns_subscriptions: Naming topic `Default` breaks subscriptions

[nicolai0]

Describe the bug

If I set the ID of the Topic construct as Default then if I make LambdaSubscription on that topic it'll fail with an error like this:

Error: section 'Resources' already contains 'Function76856677'

Expected Behavior

Creating the subscription should work.

Current Behavior

Creating the subscription results in an error being thrown while synthesizing the template.

Reproduction Steps

The following code triggers the issue when synthesizing. If you change Default to anything else, it works fine:

import * as cdk from 'aws-cdk-lib'
import * as lambda from 'aws-cdk-lib/aws-lambda'
import * as lambdaEventSources from 'aws-cdk-lib/aws-lambda-event-sources'
import * as sns from 'aws-cdk-lib/aws-sns'
import { Construct } from 'constructs'

class MyStack extends cdk.Stack {
  constructor(scope: Construct, id: string) {
    super(scope, id)

    const topic = new sns.Topic(this, 'Default')

    new lambda.Function(this, 'Function', {
      code: lambda.Code.fromInline('export function handler() {}'),
      events: [new lambdaEventSources.SnsEventSource(topic)],
      handler: 'handler',
      runtime: lambda.Runtime.NODEJS_16_X
    })
  }
}

const app = new cdk.App()
new MyStack(app, 'MyStack')

Possible Solution

It looks like the issue resides in packages/@aws-cdk/aws-sns-subscriptions/lib/lambda.ts. The subscriberId (which is eventually used as the ID of the CfnSubscription construct) is set to topic.node.id, which, being Default, causes it to be raised out of its scope and collide with the name of the topic. The ID could be changed to include a prefix, for example:

subscriberId: `Subscriber-${topic.node.id}`,

Additional Information/Context

No response

CDK CLI Version

2.34.2

Framework Version

2.34.2

Node.js Version

16.13.1

OS

macOS 12.4

Language

Typescript

Language Version

4.7.2

Other information

Full error:

Error: section 'Resources' already contains 'Function76856677'
    at mergeObjectsWithoutDuplicates (/Users/nicolai/Repositories/Shape/backend-infrastructure/node_modules/aws-cdk-lib/core/lib/stack.js:2:854)
    at mergeSection (/Users/nicolai/Repositories/Shape/backend-infrastructure/node_modules/aws-cdk-lib/core/lib/stack.js:2:254)
    at merge (/Users/nicolai/Repositories/Shape/backend-infrastructure/node_modules/aws-cdk-lib/core/lib/stack.js:1:14478)
    at MyStack._toCloudFormation (/Users/nicolai/Repositories/Shape/backend-infrastructure/node_modules/aws-cdk-lib/core/lib/stack.js:1:12619)
    at MyStack._synthesizeTemplate (/Users/nicolai/Repositories/Shape/backend-infrastructure/node_modules/aws-cdk-lib/core/lib/stack.js:1:8262)
    at DefaultStackSynthesizer.synthesizeStackTemplate (/Users/nicolai/Repositories/Shape/backend-infrastructure/node_modules/aws-cdk-lib/core/lib/stack-synthesizers/default-synthesizer.js:1:5057)
    at DefaultStackSynthesizer.synthesize (/Users/nicolai/Repositories/Shape/backend-infrastructure/node_modules/aws-cdk-lib/core/lib/stack-synthesizers/default-synthesizer.js:1:5554)
    at /Users/nicolai/Repositories/Shape/backend-infrastructure/node_modules/aws-cdk-lib/core/lib/private/synthesis.js:1:2661
    at visit (/Users/nicolai/Repositories/Shape/backend-infrastructure/node_modules/aws-cdk-lib/core/lib/private/synthesis.js:3:179)
    at visit (/Users/nicolai/Repositories/Shape/backend-infrastructure/node_modules/aws-cdk-lib/core/lib/private/synthesis.js:3:141)

[peterwoodworth]

I can reproduce this, thanks for reporting!

What I don't understand is why the name Default is significant here, or why the error is regarding duplicate naming of the Lambda Function. Do you understand why this is exactly?

I think that unfortunately your proposed fix would be a breaking change, as adjusting how logical IDs are calculated will cause resource replacement. Maybe I could provide an alternate solution if I better understood exactly why this is happening

[nicolai0]

@peterwoodworth the significance stems from this piece of code (I think) packages/@aws-cdk/cloudformation-diff/lib/format.ts:350. I can't remember if it's properly documented anywhere else, but essentially if you name your construct Default then that construct will instead assume the ID of its parent resource.

I use this because I wrap some common constructs like buckets, queues, lambdas, etc... with another construct to provide some default properties. By naming it Default then I can freely swap between using my wrapped construct and the non-wrapped construct without any change to the CloudFormation. So there's no diff between the following two:

new s3.Bucket(this, 'Bucket')

and

new WrappedBucket(this, 'Bucket')

class WrappedBucket .... {
  ... {
    new Bucket(this, 'Default')
  }
}

The problem is then that when the Lambda subscription is created, the ID Default is used to create that subscription in the scope of the Lambda (because it uses the ID of the topic as its own ID) causing its construct path to go from MyStack/MyLambda/MyTopic => /MyStack/MyLambda/Default which gets reduced to /MyStack/MyLambda, which is taken by the Lambda itself.

I hope that explains it. Another potential fix could be checking if the ID is Default and only then providing a prefix.

[peterwoodworth]

This explains it very well @nicolai0, thanks for elaborating.

I'm not confident that handling special cases around this functionality is a precedent we want to set. If there are other occurrences of this bug throughout our codebase (there very likely are), that could get messy pretty quick. We should absolutely document this functionality however, @Jerry-AWS can you make sure the Default and Resource logical ID quirk gets documented?