aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.38k stars 3.79k forks source link

cloudwatch-actions: cannot add LambdaActions to alarms with the same id but different addresses #30754

Open tmokmss opened 1 week ago

tmokmss commented 1 week ago

Describe the bug

See the reproduction code below. When we try to add LambdaAction to a different Alarm with the same id, synthesize fails. This pattern often appears when we define Alarm constructs as chidren of various constructs and use the same Lambda function to notify them.

This behavior forces users to set the id of the alarms unique across the entire stack, making it difficult to reuse a construct with alarms.

Expected Behavior

Synthesize successes.

Current Behavior

Synthesize fails.

Reproduction Steps

Synthesize the below code:

import * as cdk from 'aws-cdk-lib';
import { AlarmReproStack } from '../lib/alarm-repro-stack';
import { Code, Function, Runtime } from 'aws-cdk-lib/aws-lambda';
import { Construct } from 'constructs';
import { Alarm } from 'aws-cdk-lib/aws-cloudwatch';
import { LambdaAction } from 'aws-cdk-lib/aws-cloudwatch-actions';

const app = new cdk.App();
const stack = new AlarmReproStack(app, 'AlarmReproStack', {});
// handler to notify alarms
const handler = new Function(stack, 'Handler', {
  code: Code.fromInline('code'),
  runtime: Runtime.NODEJS_20_X,
  handler: 'index.handler',
});
{
  const child = new Construct(stack, 'Child1');
  const func = new Function(child, 'Handler', {
    code: Code.fromInline('code'),
    runtime: Runtime.NODEJS_20_X,
    handler: 'index.handler',
  });
  const alarm = new Alarm(child, 'Alarm', {
    metric: func.metricErrors(),
    threshold: 1,
    evaluationPeriods: 1,
  });
  alarm.addAlarmAction(new LambdaAction(handler));
}
{
  const child = new Construct(stack, 'Child2');
  const func = new Function(child, 'Handler', {
    code: Code.fromInline('code'),
    runtime: Runtime.NODEJS_20_X,
    handler: 'index.handler',
  });
  // Alarm with the same id
  // changing this id to some unique value, e.g. Alarm2, will make synth success
  const alarm = new Alarm(child, 'Alarm', {
    metric: func.metricErrors(),
    threshold: 1,
    evaluationPeriods: 1,
  });
  alarm.addAlarmAction(new LambdaAction(handler));
}

Possible Solution

The problem derives from here:

https://github.com/aws/aws-cdk/blob/358ceadd3352b4c692438b9d9061354556fc5bac/packages/aws-cdk-lib/aws-cloudwatch-actions/lib/lambda.ts#L25-L26

I think the permissionId should really be AlarmPermission${alarm.node.addr}. node.addr is a globally unique id across a stack, so safe to use here.

Also, because lambda permission is stateless and changes to logical ids will not cause any disruption, we should not need a feature flag to introduce this change.

Additional Information/Context

No response

CDK CLI Version

2.147.3

Framework Version

2.147.3

Node.js Version

v20.10.0

OS

macOS

Language

TypeScript

Language Version

No response

Other information

No response

ashishdhingra commented 1 week ago

Reproducible using customer's provided code. Perhaps the fix is similar to the approach used here, i.e., change the line here to below:

const idPrefix = FeatureFlags.of(scope).isEnabled(LAMBDA_PERMISSION_LOGICAL_ID_FOR_LAMBDA_ACTION) ? `${alarm.node.id}-${alarm.node.addr}` : '';

@tmokmss Please advise if there is a business use case for using the same ID for alarms.

tmokmss commented 1 week ago

@ashishdhingra Thanks.

Please advise if there is a business use case for using the same ID for alarms.

It is a common practice to reuse CDK constructs, which is not limited to specific use cases but widely used, whereas this issue currently makes it impossible to reuse them.

For example, say we have the below construct:

export class SomeConstructWithAlarm extends Construct {
  public readonly alarm: Alarm;
  constructor(scope: Construct, id: string) {
    super(scope, id);

    const func = new Function(this, 'Handler', {
      code: Code.fromInline('code'),
      runtime: Runtime.NODEJS_20_X,
      handler: 'index.handler',
    });
    // the definition of alarm is encapsulated into this construct
    this.alarm = new Alarm(this, 'Alarm', {
      metric: func.metricErrors(),
      threshold: 1,
      evaluationPeriods: 1,
    });
  }
}

but we cannot reuse this construct like the following code due to the issue:

const app = new cdk.App();
const stack = new AlarmReproStack(app, 'Stack', {});

const c1 = new SomeConstructWithAlarm(stack, "C1");
const c2 = new SomeConstructWithAlarm(stack, "C2");

const handler = new Function(stack, 'Handler', {
  code: Code.fromInline('code'),
  runtime: Runtime.NODEJS_20_X,
  handler: 'index.handler',
});
c1.alarm.addAlarmAction(new LambdaAction(handler));
c2.alarm.addAlarmAction(new LambdaAction(handler)); // ERROR!