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

Open
morrijm4 opened this issue Mar 16, 2024 · 1 comment
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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudwatch-actions bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests

2 participants