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

fix(cloudwatch-actions): LambdaAction fails if added to multiple action types #29515

Conversation

morrijm4
Copy link
Contributor

@morrijm4 morrijm4 commented Mar 16, 2024

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


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

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Mar 16, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team March 16, 2024 02:34
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@morrijm4 morrijm4 changed the title fix(cloudwatch-actions): LambdaAction fails if added to multiple acti… fix(cloudwatch-actions): LambdaAction fails if added to multiple acti… Mar 16, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review March 16, 2024 03:44

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 16, 2024
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 16, 2024
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#29515 (comment)

should the CDK be a blocker for architectural decisions

Nope, but I think it should help enforce good development practices.
Anyway, it seems possible to define the same Lambda for different actions on the same alarm from the Console so I guess it should be allowed here as well.

I left a note to adjust the test cases and to add some comments on the new condition.
Also, can you please fix the title to not be truncated?

Thanks for your quick follow-ups and clarifications 👍🙂

@morrijm4 morrijm4 changed the title fix(cloudwatch-actions): LambdaAction fails if added to multiple acti… fix(cloudwatch-actions): LambdaAction fails if added to multiple action types Mar 17, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@morrijm4 morrijm4 changed the title fix(cloudwatch-actions): LambdaAction fails if added to multiple action types fix(cloudwatch-actions): LambdaAction fails if added to multiple action types Mar 17, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review March 17, 2024 19:23

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@morrijm4
Copy link
Contributor Author

morrijm4 commented Mar 17, 2024

@lpizzinidev Just to clarify, you want me to change it back to my original implementation with it behind the feature flag? Would you mind elaborating why you prefer that over the current one?

@lpizzinidev
Copy link
Contributor

@morrijm4 Will the current one throw with the feature flag disabled?

@morrijm4
Copy link
Contributor Author

@lpizzinidev With the feature flag disabled it will throw if the same lambda is added to multiple alarms because it will try to add multiple permissions with the same 'AlarmPermission' logical ID. If the same lambda is added to different actions for one alarm it will not throw.

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#29515 (comment) Makes sense (unless I'm missing something).
Thanks for clarifying 👍
Left some comments for test coverage and documentation (please also address #29515 (comment) from the previous review)

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Mar 23, 2024
@GavinZZ GavinZZ self-requested a review March 28, 2024 22:47
GavinZZ
GavinZZ previously approved these changes Mar 28, 2024
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks for fixing it!

Copy link
Contributor

mergify bot commented Mar 28, 2024

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

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Mar 28, 2024
Copy link
Contributor

mergify bot commented Mar 28, 2024

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

@morrijm4
Copy link
Contributor Author

@Mergifyio refresh

Copy link
Contributor

mergify bot commented Mar 31, 2024

refresh

✅ Pull request refreshed

@mergify mergify bot dismissed GavinZZ’s stale review March 31, 2024 23:40

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 1, 2024
@morrijm4
Copy link
Contributor Author

morrijm4 commented Apr 1, 2024

Hey @GavinZZ, one of the mergify actions failed because it said it did not have update permissions to merge this PR. I verified I checked the "Allow edits by maintainers" box so I wanted to refresh the action. As you can see, I tried the refresh comment command and then updating the branch.

Sorry for the inconvenience and ignoring the clear message by the mergify bot. I see why it is there now...

Do you have any other troubleshooting advice? Tagging @lpizzinidev too. Thanks for the help!

@GavinZZ
Copy link
Contributor

GavinZZ commented Apr 2, 2024

@mergify update

Copy link
Contributor

mergify bot commented Apr 2, 2024

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/repo-metrics-monthly.yml without workflows permission

@GavinZZ
Copy link
Contributor

GavinZZ commented Apr 2, 2024

No worry, will update the branch and retry the mergify process. Will monitor it and retry if any of the actions failed.

GavinZZ
GavinZZ previously approved these changes Apr 2, 2024
@mergify mergify bot dismissed GavinZZ’s stale review April 2, 2024 20:19

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 2, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 3b222f3
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

mergify bot commented Apr 2, 2024

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

@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.

@jamescarter-le
Copy link

jamescarter-le commented Sep 30, 2024

Agree, it is annoying we cannot disable this functionality.

I have permissions for any alarm to call my reporting Lambda, and want to create many Alarms via the CDK, without them trying to create their own permissions.

I grant all my CloudWatch alarms permission to invoke my Lambda, in the C# CDK.

function.AddPermission("InvokeByCloudWatch", new Permission
{
    Principal = new ServicePrincipal("lambda.alarms.cloudwatch.amazonaws.com"),
    Action = "lambda:InvokeFunction",
    SourceArn = $"arn:aws:cloudwatch:{this.Region}:{this.Account}:alarm:*"
});

Now when calling AddAlarmAction, it should not try and create the Permissions, or we should have a parameter to prevent this behavior.

alarm.AddAlarmAction(new LambdaAction(AlarmReportingFunction));

Because the Alarm tries to add permissions, when attaching all my Lambdas clashes with the Permission being added. I do not want to add the AlarmName to the Action Permission using the existing feature flag, because it means I cannot attach the same Function to both AlarmAction and OkAction.

Error: There is already a Construct with name 'AlarmPermission' in Function [Lambda_CloudWatchAlarms]

I cannot find another way of attaching an Action, without using these helpers which automatically create the Permission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants