fix(cloudwatch): AlarmRule.concat cover empty operands
#34757
Conversation
github-actions
bot
added
bug
aws-cdk-automation
added
the
pr/needs-community-review
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| @@ -111,6 +111,10 @@ export class AlarmRule { | |||
| private static concat(operator: Operator, ...operands: IAlarmRule[]): IAlarmRule { | |||
| return new class implements IAlarmRule { | |||
| public renderAlarmRule(): string { | |||
| if (operands.length === 0) { | |||
There was a problem hiding this comment.
Overall the idea looks good but I have a few questions :
- When creating empty alarms it might be better to return an empty string instead of something that cannot be interpreted.
- Will this lead to composite alarms being created but with empty details, for example will the 'FALSE' value returned keep alarm in an alarmed state ?
- Can we have a warning or a log statement so that the CDK user can track/debug as to why an alarm was created with no underlying alarm ?
All of this is assuming it does create an alarm in the user's account, if Cloudformation ignore such alarms and doesn't create any resources behind it
There was a problem hiding this comment.
Thank you for taking a look at this. Here are the explanations:
- We cannot put empty string since AlarmRule is required with minimum length of 1. Ref https://docs.aws.amazon.com/AWSCloudFormation/latest/TemplateReference/aws-resource-cloudwatch-compositealarm.html#cfn-cloudwatch-compositealarm-alarmrule
- FALSE value makes the alarm remains in OK state forever. We can even say its useless 😅
- I've added a
console.warn. I'm fully aware about theAnnotations.of(construct).addWarningV2()pattern, but we cannot do that here since there is no construct to pass in. Do you know any better way to do this thanconsole.warn?
There was a problem hiding this comment.
I think the main reason behind the reported issue was :
We could consider having CDK throw an error if expression is undefined so users don't need to wait till deployment time to find out the alarm rule is invalid. Maybe something like
The current changes would create an alarm which will not be usable or will not be useful, maybe changing the concat function to handle the operands.length === 0 check and then not generating a IAlarmRule instance might be better choice. Again I am not aware of the full implication, so open to have a discussion around it(I assume you might know better as you have looked at the code)
Main point being that we don't want users to have a new unused/(not useful) alarm unknowingly showing up in their AWS account
There was a problem hiding this comment.
After reconsider, i agree with that. Throwing when empty is definitely a better option. Updated.
| @@ -59,6 +59,14 @@ class CompositeAlarmIntegrationTest extends Stack { | |||
| alarmRule, | |||
| actionsSuppressor: alarm5, | |||
| }); | |||
|
|
|||
| new CompositeAlarm(this, 'EmptyAnyOf', { | |||
There was a problem hiding this comment.
With the empty alarm cases can we have assertions on the CfnOutput and/or on api calls ?
For example if these alarms only exist in template and not actually create a resource we can have an assertion that checks that these alarms are not created at all(since they are basically empty shells)
There was a problem hiding this comment.
Of course, I've added API assertion that composite alarm with FALSE rule must be created, and the state is OK.
phuhung273
force-pushed
the
composite-alarm-empty-rule
branch
from
318e90e to
8aaa94d
Compare
phuhung273
force-pushed
the
composite-alarm-empty-rule
branch
from
8aaa94d to
3600e13
Compare
|
Exemption Request: throwing error doesn't need integration |
aws-cdk-automation
added
the
pr-linter/exemption-requested
kumvprat
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
removed
the
pr/needs-community-review
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
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). |
|
@phuhung273 Please edit the PR description to remove the line : |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #34662
Reason for this change
Invalid template for empty operands
Description of changes
Set to
FALSEwhen empty operands. Alarm is in OK stateDescribe any new or updated permissions being added
Description of how you validated changes
Unit + Integ
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license