Skip to content

Commit

Permalink
fix(cloudwatch-actions): LambdaAction fails if added to multiple ac…
Browse files Browse the repository at this point in the history
…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*
  • Loading branch information
morrijm4 authored Apr 2, 2024
1 parent 70dc4e8 commit a12887b
Show file tree
Hide file tree
Showing 9 changed files with 243 additions and 12 deletions.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,36 @@
}
],
"EvaluationPeriods": 1,
"InsufficientDataActions": [
{
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
},
{
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
},
{
"Fn::GetAtt": [
"alarmLambdaFeatureD560800F",
"Arn"
]
}
],
"MetricName": "Errors",
"Namespace": "AWS/Lambda",
"OKActions": [
{
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
},
{
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
},
{
"Fn::GetAtt": [
"alarmLambdaFeatureD560800F",
"Arn"
]
}
],
"Period": 60,
"Statistic": "Sum",
"Threshold": 1,
Expand Down Expand Up @@ -309,8 +337,36 @@
}
],
"EvaluationPeriods": 1,
"InsufficientDataActions": [
{
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
},
{
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
},
{
"Fn::GetAtt": [
"alarmLambdaFeatureD560800F",
"Arn"
]
}
],
"MetricName": "Errors",
"Namespace": "AWS/Lambda",
"OKActions": [
{
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
},
{
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
},
{
"Fn::GetAtt": [
"alarmLambdaFeatureD560800F",
"Arn"
]
}
],
"Period": 60,
"Statistic": "Sum",
"Threshold": 1,
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,36 @@
}
],
"EvaluationPeriods": 1,
"InsufficientDataActions": [
{
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
},
{
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
},
{
"Fn::GetAtt": [
"alarmLambdaFeatureD560800F",
"Arn"
]
}
],
"MetricName": "Errors",
"Namespace": "AWS/Lambda",
"OKActions": [
{
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
},
{
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
},
{
"Fn::GetAtt": [
"alarmLambdaFeatureD560800F",
"Arn"
]
}
],
"Period": 60,
"Statistic": "Sum",
"Threshold": 1,
Expand Down Expand Up @@ -309,8 +337,36 @@
}
],
"EvaluationPeriods": 1,
"InsufficientDataActions": [
{
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
},
{
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
},
{
"Fn::GetAtt": [
"alarmLambdaFeatureD560800F",
"Arn"
]
}
],
"MetricName": "Errors",
"Namespace": "AWS/Lambda",
"OKActions": [
{
"Ref": "alarmLambdaFeatureCurrentVersionCF39751979501d2f67eaf906b2ef0c378303873b"
},
{
"Ref": "alarmLambdaFeatureAliasaliasName16F91D34"
},
{
"Fn::GetAtt": [
"alarmLambdaFeatureD560800F",
"Arn"
]
}
],
"Period": 60,
"Statistic": "Sum",
"Threshold": 1,
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ class LambdaAlarmActionIntegrationTestStack extends Stack {
alarm.addAlarmAction(new cloudwatchActions.LambdaAction(version));
alarm.addAlarmAction(new cloudwatchActions.LambdaAction(aliasName));
alarm.addAlarmAction(new cloudwatchActions.LambdaAction(alarmLambda));
alarm.addOkAction(new cloudwatchActions.LambdaAction(version));
alarm.addOkAction(new cloudwatchActions.LambdaAction(aliasName));
alarm.addOkAction(new cloudwatchActions.LambdaAction(alarmLambda));
alarm.addInsufficientDataAction(new cloudwatchActions.LambdaAction(version));
alarm.addInsufficientDataAction(new cloudwatchActions.LambdaAction(aliasName));
alarm.addInsufficientDataAction(new cloudwatchActions.LambdaAction(alarmLambda));

if (isFeature) {
const alarm2 = new cloudwatch.Alarm(this, `Alarm${lambdaIdSuffix}`, {
Expand All @@ -53,6 +59,12 @@ class LambdaAlarmActionIntegrationTestStack extends Stack {
alarm2.addAlarmAction(new cloudwatchActions.LambdaAction(version));
alarm2.addAlarmAction(new cloudwatchActions.LambdaAction(aliasName));
alarm2.addAlarmAction(new cloudwatchActions.LambdaAction(alarmLambda));
alarm2.addOkAction(new cloudwatchActions.LambdaAction(version));
alarm2.addOkAction(new cloudwatchActions.LambdaAction(aliasName));
alarm2.addOkAction(new cloudwatchActions.LambdaAction(alarmLambda));
alarm2.addInsufficientDataAction(new cloudwatchActions.LambdaAction(version));
alarm2.addInsufficientDataAction(new cloudwatchActions.LambdaAction(aliasName));
alarm2.addInsufficientDataAction(new cloudwatchActions.LambdaAction(alarmLambda));
}
}
}
Expand Down
20 changes: 14 additions & 6 deletions packages/aws-cdk-lib/aws-cloudwatch-actions/lib/lambda.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,20 @@ export class LambdaAction implements cloudwatch.IAlarmAction {
*/
bind(scope: Construct, alarm: cloudwatch.IAlarm): cloudwatch.AlarmActionConfig {
const idPrefix = FeatureFlags.of(scope).isEnabled(LAMBDA_PERMISSION_LOGICAL_ID_FOR_LAMBDA_ACTION) ? alarm.node.id : '';
this.lambdaFunction.addPermission(`${idPrefix}AlarmPermission`, {
sourceAccount: Stack.of(scope).account,
action: 'lambda:InvokeFunction',
sourceArn: alarm.alarmArn,
principal: new iam.ServicePrincipal('lambda.alarms.cloudwatch.amazonaws.com'),
});
const permissionId = `${idPrefix}AlarmPermission`;
const permissionNode = this.lambdaFunction.permissionsNode.tryFindChild(permissionId) as lambda.CfnPermission | undefined;

// If the Lambda permission has already been added to this function
// we skip adding it to avoid an exception being thrown
// see https://github.com/aws/aws-cdk/issues/29514
if (permissionNode?.sourceArn !== alarm.alarmArn) {
this.lambdaFunction.addPermission(permissionId, {
sourceAccount: Stack.of(scope).account,
action: 'lambda:InvokeFunction',
sourceArn: alarm.alarmArn,
principal: new iam.ServicePrincipal('lambda.alarms.cloudwatch.amazonaws.com'),
});
}

return {
alarmActionArn: this.lambdaFunction.functionArn,
Expand Down
45 changes: 44 additions & 1 deletion packages/aws-cdk-lib/aws-cloudwatch-actions/test/lambda.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,12 @@ def handler(event, context):
handler: 'index.handler',
});
alarm1.addAlarmAction(new actions.LambdaAction(alarmLambda));
alarm1.addOkAction(new actions.LambdaAction(alarmLambda));
alarm1.addInsufficientDataAction(new actions.LambdaAction(alarmLambda));

alarm2.addAlarmAction(new actions.LambdaAction(alarmLambda));
alarm2.addOkAction(new actions.LambdaAction(alarmLambda));
alarm2.addInsufficientDataAction(new actions.LambdaAction(alarmLambda));

// THEN
Template.fromStack(stack).resourceCountIs('AWS::CloudWatch::Alarm', 2);
Expand Down Expand Up @@ -173,9 +178,47 @@ def handler(event, context):
handler: 'index.handler',
});
alarm1.addAlarmAction(new actions.LambdaAction(alarmLambda));
alarm1.addOkAction(new actions.LambdaAction(alarmLambda));
alarm1.addInsufficientDataAction(new actions.LambdaAction(alarmLambda));

// THEN
expect(() => {
alarm2.addAlarmAction(new actions.LambdaAction(alarmLambda));
}).toThrow(/There is already a Construct with name 'AlarmPermission' in Function \[alarmLambda\]/);
});
});

test('can use same lambda for same action multiple time', () => {
const stack = new Stack();
const alarm = new cloudwatch.Alarm(stack, 'Alarm', {
metric: new cloudwatch.Metric({ namespace: 'AWS', metricName: 'Test' }),
evaluationPeriods: 3,
threshold: 100,
});

// WHEN
const alarmLambda = new lambda.Function(stack, 'alarmLambda', {
runtime: lambda.Runtime.PYTHON_3_12,
functionName: 'alarmLambda',
code: lambda.Code.fromInline(`
def handler(event, context):
print('event:', event)
print('.............................................')
print('context:', context)`),
handler: 'index.handler',
});
alarm.addAlarmAction(new actions.LambdaAction(alarmLambda));
alarm.addAlarmAction(new actions.LambdaAction(alarmLambda));

// THEN
Template.fromStack(stack).resourceCountIs('AWS::Lambda::Permission', 1);
Template.fromStack(stack).hasResourceProperties('AWS::CloudWatch::Alarm', {
AlarmActions: [
{
'Fn::GetAtt': ['alarmLambda131DB691', 'Arn'],
},
{
'Fn::GetAtt': ['alarmLambda131DB691', 'Arn'],
},
],
});
});

0 comments on commit a12887b

Please sign in to comment.