Skip to content

fix(cloudwatch): AlarmRule.concat cover empty operands #34757

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 27, 2025
Merged

fix(cloudwatch): AlarmRule.concat cover empty operands #34757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3600e13
fix(cloudwatch): AlarmRule.concat throws on empty operands
phuhung273 Aug 27, 2025
5d96939
Merge branch 'main' into composite-alarm-empty-rule
mergify[bot] Aug 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/aws-cdk-lib/aws-cloudwatch/lib/alarm-rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { IAlarm, IAlarmRule } from './alarm-base';
import { UnscopedValidationError } from '../../core';

/**
* Enumeration indicates state of Alarm used in building Alarm Rule.
Expand Down Expand Up @@ -111,6 +112,10 @@ export class AlarmRule {
private static concat(operator: Operator, ...operands: IAlarmRule[]): IAlarmRule {
return new class implements IAlarmRule {
public renderAlarmRule(): string {
if (operands.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@phuhung273 phuhung273 Aug 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for taking a look at this. Here are the explanations:

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reconsider, i agree with that. Throwing when empty is definitely a better option. Updated.

throw new UnscopedValidationError(`Did not detect any operands for AlarmRule.${operator === Operator.AND ? 'allOf' : 'anyOf'}()`);
}

const expression = operands
.map(operand => `${operand.renderAlarmRule()}`)
.join(` ${operator} `);
Expand Down
12 changes: 12 additions & 0 deletions packages/aws-cdk-lib/aws-cloudwatch/test/composite-alarm.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,4 +204,16 @@ describe('CompositeAlarm', () => {
expect(alarmFromName.alarmName).toEqual('TestAlarmName');
expect(alarmFromName.alarmArn).toMatch(/:alarm:TestAlarmName$/);
});

test('empty anyOf', () => {
expect(() => new CompositeAlarm(new Stack(), 'alarm', {
alarmRule: AlarmRule.anyOf(),
})).toThrow('Did not detect any operands for AlarmRule.anyOf');
});

test('empty allOf', () => {
expect(() => new CompositeAlarm(new Stack(), 'alarm', {
alarmRule: AlarmRule.allOf(),
})).toThrow('Did not detect any operands for AlarmRule.allOf');
});
});
Loading