[sns] support for tokens in EmailSubscription

[strazeadin]

🐛 Bug Report

What is the problem?

Creating an email subscription to an SNS topic when using an email address that is a token (for example, a CfnParameter) will fail with the following error:
"Cannot use tokens in construct ID"

Reproduction Steps

The below code will reproduce the error:

const emailParam = new CfnParameter(this, "EmailParam", {
   type: "String"
})

const testTopic = new Topic(this, "TestTopic", {
   displayName: "TestTopic"
});

testTopic.addSubscription(new EmailSubscription(emailParam.valueAsString));

Environment

• CDK CLI Version: 1.7.0 (build 8870695)
• Module Version: 1.7.0 (@aws-cdk/aws-sns-subscriptions)
• OS: OSX Mojave
• Language: TypeScript

Other information

I believe the issue is coming from

aws-cdk/packages/@aws-cdk/aws-sns-subscriptions/lib/email.ts

Line 28 in 8870695

subscriberId: this.emailAddress,

as the token is used as the subscriberId.

I'm not sure if there are other implications to this, the following code change solves the bug on my end:

subscriberId: _topic.node.uniqueId,

this is a similar approach to

aws-cdk/packages/@aws-cdk/aws-sns-subscriptions/lib/sqs.ts

Line 48 in 8870695

subscriberId: topic.node.uniqueId,

Final note

Looking at the code for UrlSubscription:

aws-cdk/packages/@aws-cdk/aws-sns-subscriptions/lib/url.ts

Line 56 in 8870695

subscriberId: this.unresolvedUrl ? 'UnresolvedUrl' : this.url,

it will have the same exact issue as above. If there are no down-sides to all subscriptions using the unique node ID based on the recommendation here:

aws-cdk/packages/@aws-cdk/aws-sns/lib/subscriber.ts

Line 23 in 8870695

* cases, it is recommended to use the `uniqueId` of the topic you are

potentially it would be good to make all the subscription types consistent.

[nija-at]

This sounds good to me. We should make all the subscriptions consistent with using uniqueId.

@strazeadin - are you able to send a PR to this effect? 😊

[strazeadin]

Hey,

I've looked into implementing this as a PR and stumbled into an issue with my proposed solution.

Utilizing the topic uniqueId will result in naming conflicts when you try and add multiple URL or Email subscriptions to the same topic (since they are using the same topic as the scope and the same unique ID as the identifier). SQS and Lambda can utilize the topic UniqueID due to the fact that they pass the queue or function respectively as the subscriberScope and therefore avoid conflicts that way.

I also believe PR #2938 introduced a bug into the UrlSubscription similar to what my proposed solution would have done. If you subscribe to more than one unresolved URL you'll get the following error:

A subscription with id "UnresolvedUrl" already exists under the scope MyTopic

I'm currently unsure how to resolve this and looking for ideas around how to resolve both issues. Let me know if the above is clear or if I have missed anything obvious. Happy to hear any and all thoughts.

[nija-at]

You've identified a problem there for sure with EmailSubscription and UrlSubscription. @rix0rrr and I had a conversation around how we could go about this.

The full solution we came up with looks something like below -

In topic-base.ts, where the subscription is being bound to the topic, the code would now change to

let id;
if (Token.isUnresolved(subscriptionConfig.subscriberId)) {
  id = `${this.node.nextTokenChildPrefix()}:urlsubscription`;
} else {
  id = subscriptionConfig.subscriberId;
}

The nextTokenChildPrefix() method would have to be added to ConstructNode.
This would go through the list of the node's children and filter them by a special prefix on their ids (for example, 'unresolved') and would return the prefix that the next child should hold.
So, in the case of the first child, it will be 'unresolved:0', followed by 'unresolved:1'. The logic would identify what value return by looking at the children with the special prefix.

Note that: nextTokenChildPrefix() and unresolved are not the literals I would use (need to come up with better names), and neither is the code snippet. I'm only using those to convey the idea.

On the other hand, you have the option to use the solution that we've used for UrlSubscription with the caveat that you can't have multiple subscriptions with tokens involved.

[strazeadin]

Thanks for following up with a potential solution!

Is it possible to go with a less intrusive solution of adding a simple private numberOfsubscriptions: number = 0 field to topic-base.ts?

At that point the id resolution will change to:

this.numberOfsubscriptions++;
const id = `subscription${this.numberOfsubscriptions}`;

I believe this will have much less impact on the code base. The side-effect of this change is that you won't be able to provide an error message in case of subscribing the same subscriber to the same topic (which isn't ideal but doesn't cause any errors in deployment).

Let me know your thoughts on my proposed solution.

[nija-at]

With the solution I suggested, I was hoping to use existing states to achieve this and avoid adding additional state. I was also hoping to make this reusable for other constructs that may have similar requirements.

However, I'm ok with going with what you've suggested to begin with. Do note that this logic should apply ONLY to subscriptions that use tokens, and not otherwise.