Skip to content
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.

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

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

Open
tmokmss opened this issue Jul 4, 2024 · 2 comments · May be fixed by #32057
Open
Labels
@aws-cdk/aws-cloudwatch-actions bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@tmokmss
Copy link
Contributor

tmokmss commented Jul 4, 2024

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:

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

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

@tmokmss tmokmss added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 4, 2024
@ashishdhingra ashishdhingra self-assigned this Jul 5, 2024
@ashishdhingra ashishdhingra added needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Jul 5, 2024
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Jul 5, 2024

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.

@ashishdhingra ashishdhingra added p2 effort/small Small work item – less than a day of effort and removed needs-reproduction This issue needs reproduction. labels Jul 5, 2024
@ashishdhingra ashishdhingra removed their assignment Jul 5, 2024
@tmokmss
Copy link
Contributor Author

tmokmss commented Jul 8, 2024

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudwatch-actions bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
2 participants