Using notifications overwrites exisiting resouce permissions.

[dgem]

This is a Bug Report

Description

When using a pre-existing SNS topic for notifications, any existing policies on the topic are replaced with a new one. If, like us, you have multiple alarms & notifications using the same queue (for fan-in) then it stop alarms from being able to publish to the topic.

Expected behaviour:

• The existing policy is be amended, rather than replaced.

Similar or dependent issues:

• none afaik (nothing in issues)

Additional Data

• https://github.com/horike37/serverless-step-functions/blob/master/lib/deploy/stepFunctions/compileNotifications.js#L89 shows how the policy is generated
• https://github.com/horike37/serverless-step-functions/blob/master/lib/deploy/stepFunctions/compileNotifications.js#L264 being where it is built (I'm sure you know that...)

[dgem]

Hello, any thoughts on this?

[theburningmonk]

@dgem hey, so I had a look at this recently and the problem is that both SNS and SQS allows just one policy and there's no way to add permissions to it through CloudFormation currently, you'd have to use the corresponding add-permission and remove-permission API calls.

I've raised feature requests on the CloudFormation roadmaps repo to support adding permissions to SNS topic and SQS queue but have no idea if and when they'll be picked up and implemented.

In the meantime, there are two approaches we can take off the top of my head:

1. create custom resources and essentially implement those SNS::TopicPolicyStatement and SQS::QueuePolicyStatement resources ourselves (there's precedent for this approach too, the Serverless framework does this for EventBridge trigger for Lambda)
2. make direct API calls during the deploy or post deploy hooks

Option 1 would be quite involved and I don't really fancy it. Option 2 is fairly straightforward, so if I had to do it then that's the approach I'd pick.

Do you know how the serverless framework works and the structure of this plugin enough to put together a PR for this?

[dgem]

Hi @theburningmonk, Funnily enough I was just looking into this earlier and thought I'd check the PR. The case I'm looking at, the SNS is defined in another serverless.yaml. Originally I was going to see if it was possible to resolve the Policy attached there, however this would be more of an edge case than the generic. I've done a little work with serverless plugins, so will take a look at Option 2 and post an update here. Thanks for the advice :)

[dgem]

I took a look and it seems like the fix is not quite a trivial as I'd hoped. Currently the name of the policy contains a randomly generated part. Using the AWS SDK it would, I think, require the name to be predictable as during the delete phase you need to specify the name of the policy.

afaics both approaches would require this approach...