Skip to content

Commit a12887b

Browse files
morrijm4morrijm4
authored
fix(cloudwatch-actions): LambdaAction fails if added to multiple action 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*
1 parent 70dc4e8 commit a12887b

File tree

9 files changed

+243
-12
lines changed

9 files changed

+243
-12
lines changed

‎packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch-actions/test/integ.lambda-alarm-action.js.snapshot/LambdaAlarmActionIntegrationTestStack.assets.json

+2-2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch-actions/test/integ.lambda-alarm-action.js.snapshot/LambdaAlarmActionIntegrationTestStack.template.json

+56
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,36 @@
7979
}
8080
],
8181
"EvaluationPeriods": 1,
82+
"InsufficientDataActions": [
83+
{
84+
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
85+
},
86+
{
87+
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
88+
},
89+
{
90+
"Fn::GetAtt": [
91+
"alarmLambdaFeatureD560800F",
92+
"Arn"
93+
]
94+
}
95+
],
8296
"MetricName": "Errors",
8397
"Namespace": "AWS/Lambda",
98+
"OKActions": [
99+
{
100+
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
101+
},
102+
{
103+
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
104+
},
105+
{
106+
"Fn::GetAtt": [
107+
"alarmLambdaFeatureD560800F",
108+
"Arn"
109+
]
110+
}
111+
],
84112
"Period": 60,
85113
"Statistic": "Sum",
86114
"Threshold": 1,
@@ -309,8 +337,36 @@
309337
}
310338
],
311339
"EvaluationPeriods": 1,
340+
"InsufficientDataActions": [
341+
{
342+
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
343+
},
344+
{
345+
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
346+
},
347+
{
348+
"Fn::GetAtt": [
349+
"alarmLambdaFeatureD560800F",
350+
"Arn"
351+
]
352+
}
353+
],
312354
"MetricName": "Errors",
313355
"Namespace": "AWS/Lambda",
356+
"OKActions": [
357+
{
358+
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
359+
},
360+
{
361+
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
362+
},
363+
{
364+
"Fn::GetAtt": [
365+
"alarmLambdaFeatureD560800F",
366+
"Arn"
367+
]
368+
}
369+
],
314370
"Period": 60,
315371
"Statistic": "Sum",
316372
"Threshold": 1,

‎packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch-actions/test/integ.lambda-alarm-action.js.snapshot/LambdaAlarmActionIntegrationTestStackWithFeatureFlag.assets.json

+2-2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch-actions/test/integ.lambda-alarm-action.js.snapshot/LambdaAlarmActionIntegrationTestStackWithFeatureFlag.template.json

+56
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,36 @@
7979
}
8080
],
8181
"EvaluationPeriods": 1,
82+
"InsufficientDataActions": [
83+
{
84+
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
85+
},
86+
{
87+
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
88+
},
89+
{
90+
"Fn::GetAtt": [
91+
"alarmLambdaFeatureD560800F",
92+
"Arn"
93+
]
94+
}
95+
],
8296
"MetricName": "Errors",
8397
"Namespace": "AWS/Lambda",
98+
"OKActions": [
99+
{
100+
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
101+
},
102+
{
103+
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
104+
},
105+
{
106+
"Fn::GetAtt": [
107+
"alarmLambdaFeatureD560800F",
108+
"Arn"
109+
]
110+
}
111+
],
84112
"Period": 60,
85113
"Statistic": "Sum",
86114
"Threshold": 1,
@@ -309,8 +337,36 @@
309337
}
310338
],
311339
"EvaluationPeriods": 1,
340+
"InsufficientDataActions": [
341+
{
342+
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
343+
},
344+
{
345+
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
346+
},
347+
{
348+
"Fn::GetAtt": [
349+
"alarmLambdaFeatureD560800F",
350+
"Arn"
351+
]
352+
}
353+
],
312354
"MetricName": "Errors",
313355
"Namespace": "AWS/Lambda",
356+
"OKActions": [
357+
{
358+
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
359+
},
360+
{
361+
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
362+
},
363+
{
364+
"Fn::GetAtt": [
365+
"alarmLambdaFeatureD560800F",
366+
"Arn"
367+
]
368+
}
369+
],
314370
"Period": 60,
315371
"Statistic": "Sum",
316372
"Threshold": 1,

‎packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch-actions/test/integ.lambda-alarm-action.js.snapshot/manifest.json

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch-actions/test/integ.lambda-alarm-action.js.snapshot/tree.json

+56
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch-actions/test/integ.lambda-alarm-action.ts

+12
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,12 @@ class LambdaAlarmActionIntegrationTestStack extends Stack {
4040
alarm.addAlarmAction(new cloudwatchActions.LambdaAction(version));
4141
alarm.addAlarmAction(new cloudwatchActions.LambdaAction(aliasName));
4242
alarm.addAlarmAction(new cloudwatchActions.LambdaAction(alarmLambda));
43+
alarm.addOkAction(new cloudwatchActions.LambdaAction(version));
44+
alarm.addOkAction(new cloudwatchActions.LambdaAction(aliasName));
45+
alarm.addOkAction(new cloudwatchActions.LambdaAction(alarmLambda));
46+
alarm.addInsufficientDataAction(new cloudwatchActions.LambdaAction(version));
47+
alarm.addInsufficientDataAction(new cloudwatchActions.LambdaAction(aliasName));
48+
alarm.addInsufficientDataAction(new cloudwatchActions.LambdaAction(alarmLambda));
4349

4450
if (isFeature) {
4551
const alarm2 = new cloudwatch.Alarm(this, `Alarm${lambdaIdSuffix}`, {
@@ -53,6 +59,12 @@ class LambdaAlarmActionIntegrationTestStack extends Stack {
5359
alarm2.addAlarmAction(new cloudwatchActions.LambdaAction(version));
5460
alarm2.addAlarmAction(new cloudwatchActions.LambdaAction(aliasName));
5561
alarm2.addAlarmAction(new cloudwatchActions.LambdaAction(alarmLambda));
62+
alarm2.addOkAction(new cloudwatchActions.LambdaAction(version));
63+
alarm2.addOkAction(new cloudwatchActions.LambdaAction(aliasName));
64+
alarm2.addOkAction(new cloudwatchActions.LambdaAction(alarmLambda));
65+
alarm2.addInsufficientDataAction(new cloudwatchActions.LambdaAction(version));
66+
alarm2.addInsufficientDataAction(new cloudwatchActions.LambdaAction(aliasName));
67+
alarm2.addInsufficientDataAction(new cloudwatchActions.LambdaAction(alarmLambda));
5668
}
5769
}
5870
}

‎packages/aws-cdk-lib/aws-cloudwatch-actions/lib/lambda.ts

+14-6
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,20 @@ export class LambdaAction implements cloudwatch.IAlarmAction {
2323
*/
2424
bind(scope: Construct, alarm: cloudwatch.IAlarm): cloudwatch.AlarmActionConfig {
2525
const idPrefix = FeatureFlags.of(scope).isEnabled(LAMBDA_PERMISSION_LOGICAL_ID_FOR_LAMBDA_ACTION) ? alarm.node.id : '';
26-
this.lambdaFunction.addPermission(`${idPrefix}AlarmPermission`, {
27-
sourceAccount: Stack.of(scope).account,
28-
action: 'lambda:InvokeFunction',
29-
sourceArn: alarm.alarmArn,
30-
principal: new iam.ServicePrincipal('lambda.alarms.cloudwatch.amazonaws.com'),
31-
});
26+
const permissionId = `${idPrefix}AlarmPermission`;
27+
const permissionNode = this.lambdaFunction.permissionsNode.tryFindChild(permissionId) as lambda.CfnPermission | undefined;
28+
29+
// If the Lambda permission has already been added to this function
30+
// we skip adding it to avoid an exception being thrown
31+
// see https://github.com/aws/aws-cdk/issues/29514
32+
if (permissionNode?.sourceArn !== alarm.alarmArn) {
33+
this.lambdaFunction.addPermission(permissionId, {
34+
sourceAccount: Stack.of(scope).account,
35+
action: 'lambda:InvokeFunction',
36+
sourceArn: alarm.alarmArn,
37+
principal: new iam.ServicePrincipal('lambda.alarms.cloudwatch.amazonaws.com'),
38+
});
39+
}
3240

3341
return {
3442
alarmActionArn: this.lambdaFunction.functionArn,

‎packages/aws-cdk-lib/aws-cloudwatch-actions/test/lambda.test.ts

+44-1
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,12 @@ def handler(event, context):
139139
handler: 'index.handler',
140140
});
141141
alarm1.addAlarmAction(new actions.LambdaAction(alarmLambda));
142+
alarm1.addOkAction(new actions.LambdaAction(alarmLambda));
143+
alarm1.addInsufficientDataAction(new actions.LambdaAction(alarmLambda));
144+
142145
alarm2.addAlarmAction(new actions.LambdaAction(alarmLambda));
146+
alarm2.addOkAction(new actions.LambdaAction(alarmLambda));
147+
alarm2.addInsufficientDataAction(new actions.LambdaAction(alarmLambda));
143148

144149
// THEN
145150
Template.fromStack(stack).resourceCountIs('AWS::CloudWatch::Alarm', 2);
@@ -173,9 +178,47 @@ def handler(event, context):
173178
handler: 'index.handler',
174179
});
175180
alarm1.addAlarmAction(new actions.LambdaAction(alarmLambda));
181+
alarm1.addOkAction(new actions.LambdaAction(alarmLambda));
182+
alarm1.addInsufficientDataAction(new actions.LambdaAction(alarmLambda));
176183

177184
// THEN
178185
expect(() => {
179186
alarm2.addAlarmAction(new actions.LambdaAction(alarmLambda));
180187
}).toThrow(/There is already a Construct with name 'AlarmPermission' in Function \[alarmLambda\]/);
181-
});
188+
});
189+
190+
test('can use same lambda for same action multiple time', () => {
191+
const stack = new Stack();
192+
const alarm = new cloudwatch.Alarm(stack, 'Alarm', {
193+
metric: new cloudwatch.Metric({ namespace: 'AWS', metricName: 'Test' }),
194+
evaluationPeriods: 3,
195+
threshold: 100,
196+
});
197+
198+
// WHEN
199+
const alarmLambda = new lambda.Function(stack, 'alarmLambda', {
200+
runtime: lambda.Runtime.PYTHON_3_12,
201+
functionName: 'alarmLambda',
202+
code: lambda.Code.fromInline(`
203+
def handler(event, context):
204+
print('event:', event)
205+
print('.............................................')
206+
print('context:', context)`),
207+
handler: 'index.handler',
208+
});
209+
alarm.addAlarmAction(new actions.LambdaAction(alarmLambda));
210+
alarm.addAlarmAction(new actions.LambdaAction(alarmLambda));
211+
212+
// THEN
213+
Template.fromStack(stack).resourceCountIs('AWS::Lambda::Permission', 1);
214+
Template.fromStack(stack).hasResourceProperties('AWS::CloudWatch::Alarm', {
215+
AlarmActions: [
216+
{
217+
'Fn::GetAtt': ['alarmLambda131DB691', 'Arn'],
218+
},
219+
{
220+
'Fn::GetAtt': ['alarmLambda131DB691', 'Arn'],
221+
},
222+
],
223+
});
224+
});

0 commit comments

Comments
 (0)