Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(aws-cloudwatch-actions): LambdaAction fails if added to multiple action types #29514

Labels
@aws-cdk/aws-cloudwatch-actions bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@morrijm4
Copy link
Contributor

Describe the bug

Adding the same lambda as the action for multiple status changes (alarm, ok, insufficient data) causes an error because of logical id conflicts.

Expected Behavior

I expect to be able to add the same lambda for multiple action types on a CloudWatch alarm.

Current Behavior

/Users/james.morrison/work/sales-domain/node_modules/.pnpm/constructs@10.3.0/node_modules/constructs/src/construct.ts:447
      throw new Error(`There is already a Construct with name '${childName}' in ${typeName}${name.length > 0 ? ' [' + name + ']' : ''}`);
            ^
Error: There is already a Construct with name 'ApiGatewayClientErrorAlarmAlarmPermission' in Import [SlackLambda]
    at Node.addChild (/Users/james.morrison/work/sales-domain/node_modules/.pnpm/constructs@10.3.0/node_modules/constructs/src/construct.ts:447:13)
    at new Node (/Users/james.morrison/work/sales-domain/node_modules/.pnpm/constructs@10.3.0/node_modules/constructs/src/construct.ts:71:17)
    at new Construct (/Users/james.morrison/work/sales-domain/node_modules/.pnpm/constructs@10.3.0/node_modules/constructs/src/construct.ts:499:17)
    at new CfnElement (/Users/james.morrison/work/sales-domain/node_modules/.pnpm/aws-cdk-lib@2.126.0_constructs@10.3.0/node_modules/aws-cdk-lib/core/lib/cfn-element.js:1:743)
    at new CfnRefElement (/Users/james.morrison/work/sales-domain/node_modules/.pnpm/aws-cdk-lib@2.126.0_constructs@10.3.0/node_modules/aws-cdk-lib/core/lib/cfn-element.js:2:823)
    at new CfnResource (/Users/james.morrison/work/sales-domain/node_modules/.pnpm/aws-cdk-lib@2.126.0_constructs@10.3.0/node_modules/aws-cdk-lib/core/lib/cfn-resource.js:1:1399)
    at new CfnPermission (/Users/james.morrison/work/sales-domain/node_modules/.pnpm/aws-cdk-lib@2.126.0_constructs@10.3.0/node_modules/aws-cdk-lib/aws-lambda/lib/lambda.generated.js:1:113187)
    at Import.addPermission (/Users/james.morrison/work/sales-domain/node_modules/.pnpm/aws-cdk-lib@2.126.0_constructs@10.3.0/node_modules/aws-cdk-lib/aws-lambda/lib/function-base.js:2:1036)
    at LambdaAction.bind (/Users/james.morrison/work/sales-domain/node_modules/.pnpm/aws-cdk-lib@2.126.0_constructs@10.3.0/node_modules/aws-cdk-lib/aws-cloudwatch-actions/lib/lambda.js:1:947)
    at /Users/james.morrison/work/sales-domain/node_modules/.pnpm/aws-cdk-lib@2.126.0_constructs@10.3.0/node_modules/aws-cdk-lib/aws-cloudwatch/lib/alarm-base.js:1:1587

Reproduction Steps

declare const lambda: lambda.IFunction;
declare const alarm: cloudwatch.Alarm;

const action = new actions.LambdaAction(lambda);
alarm.addAlarmAction(action);
alarm.addOkAction(action);

Possible Solution

In the LambdaAction bind function, check if a permission with the same logical id has been added to the lambda using the this.lambdaFunction.permissionsNode.tryFindChild(idPrefix) function.

Additional Information/Context

No response

CDK CLI Version

2.126.0 (build fb74c41)

Framework Version

No response

Node.js Version

v18.18.0

OS

macOS Ventura Version 13.6.4 (22G513)

Language

TypeScript

Language Version

5.3.3

Other information

No response

@tim-finnigan
Copy link

Thanks for reporting this issue and creating the PR.

@tim-finnigan tim-finnigan added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 18, 2024
@tim-finnigan tim-finnigan removed their assignment Mar 18, 2024
mergify bot pushed a commit that referenced this issue Apr 2, 2024
…tion types (#29515)

Closes. #29514


### Reason for this change

Adding the same lambda as the action for multiple status changes (alarm, ok, insufficient data) causes an error because of logical id conflicts.

### Description of changes

Before adding the `lambda:InvokeFunction` permission to the lambda's resource policy, it checks to see if one already exists.

I considered not including this change under the `LAMBDA_PERMISSION_LOGICAL_ID_FOR_LAMBDA_ACTION` feature flag but, it breaks the `throws when multiple alarms are created for the same lambda if feature flag is set to false` test because it no longer throws. I understand that a major goal of the project is to keep behavior consistent however, it seems like it would be beneficial to fix an undesirable behavior without the need of configuring a feature flag.

This is my first contribution so I am new to this, could my change warrant its own feature flag?

### Description of how you validated changes

Expanded upon existing unit tests. 

### Checklist
- [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@khushail
Copy link
Contributor

khushail commented May 21, 2024

As the related PR is merged successfully, closing this issue.

Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@Flavioli
Copy link

Is this actually resolved? I'm still running into this issue. With the flag @aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction I can use the same LambdaAction on multiple alarms, but I still cannot use the same LambdaAction on both addAlarmAction and addOkAction of the same alarm.

A workaround is to duplicate the alarm just for the purpose of having a different alarm for each of addAlarmAction and addOkAction with the same lambda, or two use two different lambdas, one for each alarm action type. Neither scenario is ideal. But on my end this particular issue still seems to be present.

@khushail
Copy link
Contributor

khushail commented Jul 24, 2024

Is this actually resolved? I'm still running into this issue. With the flag @aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction I can use the same LambdaAction on multiple alarms, but I still cannot use the same LambdaAction on both addAlarmAction and addOkAction of the same alarm.

A workaround is to duplicate the alarm just for the purpose of having a different alarm for each of addAlarmAction and addOkAction with the same lambda, or two use two different lambdas, one for each alarm action type. Neither scenario is ideal. But on my end this particular issue still seems to be present.

There was a similar reported issue -#30264, and suggested solution on that one.Could you please check and get back if the issue is still not resolved. Also pls see that comments on closed issue are hard to track , so if you are facing the issue, please feel free to open a new github issue ,sharing all the details, Team will look into it. Hope that helps!
Thanks.

@Flavioli
Copy link

@khushail thanks for the reply!

I did look at #30264 (and basically all related issues I could find; I spent many hours investigating this). It's very close to this issue, and related in some ways, but it's not identical. That reported issue identifies a bug when attempting to use the same LambdaAction for two separate alarms. On the other hand, this issue corresponds to using one LambdaAction on a single alarm, but registering it on both addAlarmAction and addOkAction of that single alarm.

Interestingly, 30264 actually works correctly for me if I use the feature flag indicated at the bottom (@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction). In fact, the fix for 30264 (using that feature flag) is what I ultimately had to use to work around my problems with this issue (29514), by using two separate copies of the same alarm to register the LambdaAction for each of addAlarmAction and addOkAction.

Ironically, issue 30264 is still marked as Open even though it seems to be resolved, while this issue (29514) is marked as Closed even though it appears to still be unresolved... at least for me.

I can point exactly to why this one is still broken while 30264 is resolved when the feature flag is applied:

  • When registering a LambdaAction on both addAlarmAction and addOkAction of single alarm, there is an error because both attempt to create a Construct named AlarmPermission (or MyAlarmNameAlarmPermission if you enable the feature flag, where MyAlarmName is the name of the one alarm).
  • When registering a LambdaAction on 2 separate alarms (addAlarmAction on one alarm and addOkAction on a copy of that alarm), it works properly with the feature flag applied because there is no conflicting Construct name. If the alarms are named MyAlarmName1 and MyAlarmName2, you end up with MyAlarmName1AlarmPermission and MyAlarmName2AlarmPermission, which is fine. Of course, without the feature flag enabled you'll have an error because both will instead attempt to create a construct called AlarmPermission.

Let me know if I should post this followup anywhere else!

@khushail
Copy link
Contributor

@Flavioli, Thanks for the detailed analysis and sharing your insights. Please feel free to open a new issue as we don't track comments on closed issues. You could refer to this old issue in the new one and share your analysis for the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment