fix(lambda): grantInvoke fails on second invocation

[njlynch]

If grantInvoke() is called twice for the same principal, the second call fails
due to attempting to create two CfnPermission nodes with the same id. This
(simple) fix skips the second creation if the node already exists.

A more robust check would be to check the existing CfnPermission, comparing
every field, skipping creation if the two are identical and throwing an error
otherwise, as well as handling that in the upstream grantInvoke call. I opted
for the simpler solution for now, but willing to take arguments for something
more complex.

I also nested the existing grantInvoke tests for future readability. The tests weren't changed,
just the last one added.

fixes #8553

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[nija-at]

Looking at the original ALB issue, this 'feels' like a case where the two ALBs should be (but are not) calling addPermission() with unique scope/id combination.

Can you point to the calling point where this error is occurring?

[njlynch]

Can you point to the calling point where this error is occurring?

It's actually not addPermission directly, but indirectly through grantInvoke:

aws-cdk/packages/@aws-cdk/aws-elasticloadbalancingv2-targets/lib/lambda-target.ts

Line 21 in 3c116f3

const grant = this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com'));

All ALBs will make the same grantInvoke call with elasticloadbalancing.amazonaws.com, which generates the same id for the addPermission call.

[eladb]

This doesn't feel like the right place to fix this bug. If two unaware 3rd party construct libraries call fn.addPermission() with the same id, the second will be silently ignored? Doesn't make sense.

Feels like the right place to fix this is at the higher level API where there is more context

[njlynch]

One other alternative I toyed with was fixing this just one level up, in the grantInvoke call. That feels like a slightly better compromise to me than making ELB have to deal with this. It makes reasonable sense to me that grantInvoke should be an idempotent operation, and not explode on duplicate calls.

If the ELB code deals with this, the ELB code will have to have implementation-level details of how the Lambda grant is implemented, either by looping through all possible associated permissions or looking for the Invoke${grantee}-id'ed node, both of which seem like abstraction leakage to me.

[eladb]

Maybe we can offer something like fn.hasPermission(id) or something to that affect to prevent the leakage?

Agreed that grantInvoke() should not explode on duplicate calls but we can't use the ID as the arbiter. It has to be the entire configuration.

[eladb]

Can you point to the calling point where this error is occurring?

It's actually not addPermission directly, but indirectly through grantInvoke:

aws-cdk/packages/@aws-cdk/aws-elasticloadbalancingv2-targets/lib/lambda-target.ts

Line 21 in 3c116f3

const grant = this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com'));

All ALBs will make the same grantInvoke call with elasticloadbalancing.amazonaws.com, which generates the same id for the addPermission call.

I think the solution should be in the ELB code, not lambda.

[nija-at]

Can you point to the calling point where this error is occurring?

It's actually not addPermission directly, but indirectly through grantInvoke:

aws-cdk/packages/@aws-cdk/aws-elasticloadbalancingv2-targets/lib/lambda-target.ts

Line 21 in 3c116f3

const grant = this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com'));

All ALBs will make the same grantInvoke call with elasticloadbalancing.amazonaws.com, which generates the same id for the addPermission call.

What I gather is happening is that - there are two ALBs using the same lambda function and hence need two AWS::Lambda::Permission resources. Did I get that correctly?

If so, I think the resource id should either be the ALB construct id (+ function construct id) scoped under the lambda function construct, or the other way around (i.e. id should be function construct, scope should be ALB).
I gather the second one would be better in preventing cyclic dependencies when used across stacks.

[njlynch]

What I gather is happening is that - there are two ALBs using the same lambda function and hence need two AWS::Lambda::Permission resources. Did I get that correctly?

Yes and no. The use case is two ALBs using the same Lambda function (yes); they do not need two Permission resources though (no). There is nothing resource-specific about the Permission; it is generally allowing ELB to invoke the Lambda, not a specific invocation by a specific load balancer.

We could create two resources (I think - haven't tested if this explodes for some reason), but there's no need to. Much better to detect that the permission already exists and re-use it. @eladb and I disagree on whether that should be Lambda's job or ELB's job. Feel free to act as a tie-breaker. :)

[nija-at]

Calling addPermission() with the same scope and id but with different parameters must throw an error. Silently succeeding is the wrong customer experience because it gives the sense that the permission grant worked but actually it didn't.

Either the check/cache needs to go higher up the stack, or the comparison needs to be deeper.
Going down the deep comparison route, keep in mind that the logic should take into account that the Permission struct, in the future, can expand to include more properties that may or may not have to be included in the deep comparison.
There may be caveats going up the stack.

[njlynch]

I moved the logic up one layer and used memoization to be able to retrieve the previous Grant. I believe this satisfies both sets of feedback.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 11f131b
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).