Update CodeDeploy default policy to AWSCodeDeployRoleForLambdaLimited

[vrr-21]

Proposed changes

The managed policy AWSCodeDeployRoleForLambda used for Lambda deployments has broad permissions, providing publish access to all SNS topics within the customer's accounts.
This change replaces that with a new policy AWSCodeDeployRoleForLambdaLimited which removes those permissions.
To handle customers who may have triggers set up for SNS notifications on CodeDeploy, this change also attaches the AmazonSNSFullAccess managed policy when the deployment preferences include trigger configurations.

Types of changes

What types of changes does your code introduce to the plugin?
Put an x in the boxes that apply

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• Lint and unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

[philstrong]

👍

[davidgf]

Hi there! First of all, thank you for your contribution. A couple of questions though, I'm trying to understand the new logic introduced, but after not looking at this plugin's code in quite a while, it's being a bit difficult to follow. Why are we checking precisely the first key of functionResources? Is it because by chance that's the place where the resource you're looking for is located? Would the logic break if someone changed the order of that array? Another thing that feels a bit odd is that the codeDeployRole is being modified right after being created. The responsibility of generating the appropriate role should fall on the buildCodeDeploy() method. A possible solution to this that springs to mind is just passing a new argument to that method that indicates whether SNS access is needed, something that can be determined by another function whose sole responsibility is figuring out whether SNS is involved or not, that way we would also maintain the declarative style we've tried to maintain across the repo. Last but not least, why do you consider the PR introduces a breaking change? As far as I can see, it would make the plugin use more accurate permissions, but it wouldn't break a deployment on any existing serverless project, is that correct?

[philstrong]

@davidgf thanks for your response. I'll let @vrr-21 cover some topics and I'll answer one here:

Last but not least, why do you consider the PR introduces a breaking change? As far as I can see, it would make the plugin use more accurate permissions, but it wouldn't break a deployment on any existing serverless project, is that correct?

That seems accurate to me. It should not break if this change is merged. The new policy is scoped down but has been tested to work by the CodeDeploy team. Furthermore, AWS will be setting a deprecation date on the old policy at some point soon. So it will be a breaking change not to update to new policy.

[vrr-21]

Hi @davidgf

Thank you for your response, I will answer the questions below:

Why are we checking precisely the first key of functionResources? Is it because by chance that's the place where the resource you're looking for is located?

I was checking the first key to find if TriggerConfigurations have been set. Yes, it is the place where the resource is located.

Would the logic break if someone changed the order of that array?

Yes, if I am not wrong. I agree that there can be a better way to do this.

Another thing that feels a bit odd is that the codeDeployRole is being modified right after being created. The responsibility of generating the appropriate role should fall on the buildCodeDeploy() method. A possible solution to this that springs to mind is just passing a new argument to that method that indicates whether SNS access is needed, something that can be determined by another function whose sole responsibility is figuring out whether SNS is involved or not, that way we would also maintain the declarative style we've tried to maintain across the repo.

That is correct. I agree to the fact that generating the appropriate role should fall on the buildCodeDeploy() method. I did it like this because it was the only place where TriggerConfigurations and the role can be found together. But I am all in with passing an argument to the method.

Last but not least, why do you consider the PR introduces a breaking change? As far as I can see, it would make the plugin use more accurate permissions, but it wouldn't break a deployment on any existing serverless project, is that correct?

This PR will not break any deployments currently, but I was not sure if changing permissions would constitute a breaking change so I marked it as a breaking change (with the intent of working from there). @philstrong is correct.

[Helen1987]

@davidgf any further comments?

[davidgf]

Hey @Helen1987, no further comments, but the suggested changes haven't been included. I'm happy to add them myself, but I couldn't give an ETA, due to professional commitments

[vrr-21]

Hi @davidgf Thanks for your response. I can add the changes.

[vrr-21]

Hi @davidgf
I have made the changes. Please let me know if there's anything else that may be needed.

[davidgf]

Amazing, thanks for your contribution!