fix(cloudwatch-actions): lambda permission use unique prefix id

[phuhung273]

Issue # (if applicable)

Closes #33958

Reason for this change

Duplicate id of alarm of multiple StepScalingPolicy

Description of changes

Update Lambda permission prefix to use Names.uniqueId

Considering this requires resource destruction, please let me know if i need a feature flag for the fix. As there was another feature flag in the exact same place.

Description of how you validated changes

Unit + Integ

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[scorbiere]

Thanks for your contribution. I would recommend the following changes:

• Add an optional LambdaActionProps parameter in the constructor. This new props will include a boolean useLongPermissionId (or a better name ;) ).
• Use the new property in the construction of the idPrefix.
• after if (permissionNode?.sourceArn !== alarm.alarmArn) {, add a test that will show a warning letting the user know about the property useLongPermissionId, in case permissionNode is not undefined (which should case the next line .addPermission to throw the exception)

---

Why not another feature flag? Because I think user may not want this behaviour to be at a global (or stack) level.

---

Why not fixing the actual logic? Because it will trigger the replacement of the existing permissions, and also, I am concerned about the risk of transient permission issues.

Pull request has been modified.

[phuhung273]

Thanks @scorbiere for your instruction. I can see some similar implementation in the codebase but since this is the first time adding a warning, I must have misunderstand your point.

Below pseudocode is what I'm understanding about your suggestion

if (permissionNode?.sourceArn !== alarm.alarmArn) {
  if (permissionNode !== undefined && !useUniquePermissionId) {
    Annotations.of(scope).addWarningV2(permissionId, 'Please use useUniquePermissionId');
  }

  this.lambdaFunction.addPermission(permissionId, {...});
}

Is my understanding correct ?

[scorbiere]

Thanks @scorbiere for your instruction. I can see some similar implementation in the codebase but since this is the first time adding a warning, I must have misunderstand your point.

Below pseudocode is what I'm understanding about your suggestion

if (permissionNode?.sourceArn !== alarm.alarmArn) {
  if (permissionNode !== undefined && !useUniquePermissionId) {
    Annotations.of(scope).addWarningV2(permissionId, 'Please use useUniquePermissionId');
  }

  this.lambdaFunction.addPermission(permissionId, {...});
}

Is my understanding correct ?

Yes, I would add the following:

• The warning id is usually in a form like @aws-cdk/aws-cloudwatch-actions:LambdaActionPermissionId. see this example:

  aws-cdk/packages/aws-cdk-lib/aws-ses-actions/lib/s3.ts

  Line 68 in c50cb87

  cdk.Annotations.of(rule).addWarningV2('@aws-cdk/s3:AddBucketPermissions', 'This rule is using a S3 action with an imported bucket. Ensure permission is given to SES to write to that bucket.');

• In order to help the user, the error message could mention LambdaAction and include this.lambdaFunction.functionName and alarm.alarmArn.
• Annotations.of(scope). will add the warning on the Alarm construct, but I think this is fine since the class LambdaAction is not a construct.

[phuhung273]

The problem with if (permissionNode !== undefined && !useUniquePermissionId) { is we cannot unit-test the warning message

const annotations = Annotations.fromStack(stack); // throw construct id collision exception here
annotations.hasWarning('*', Match.stringLikeRegexp('message here')); // never reach here

But we can with if (!useUniquePermissionId) {

Let me know your thought on whether we should check permissionNode is not undefined. If we should, I'll remove that unit test.

[scorbiere]

I think it is important to keep on checking the permissionNode is not undefined so that the warning is only displayed when it should be useful, otherwise users may acknowledge it and miss the hint when they would have needed it.
I see why the unit test check is a challenge. Before removing it, could you try these options?

1. Add try/catch block around Annotations.fromStack(stack) and see if the metadata of the alarm contains the warning (maybe in something like policy1.lowerAlarm.node.metadata)
2. Try to override the method alarmLambda.permissionsNode.tryFindChild. Here is a proposed example generated with Amazon Q:

test('warning is shown when useUniquePermissionId is not set and collision would occur', () => {
  // GIVEN
  const stack = new Stack();
  
  // Create a lambda function
  const alarmLambda = new lambda.Function(stack, 'alarmLambda', {
    runtime: lambda.Runtime.PYTHON_3_12,
    functionName: 'alarmLambda',
    code: lambda.Code.fromInline(`def handler(event, context): pass`),
    handler: 'index.handler',
  });
  
  // Create an alarm
  const alarm = new cloudwatch.Alarm(stack, 'TestAlarm', {
    metric: new cloudwatch.Metric({
      namespace: 'Test',
      metricName: 'TestMetric',
    }),
    threshold: 1,
    evaluationPeriods: 1,
  });
  
  // Save the original tryFindChild method
  const originalTryFindChild = alarmLambda.permissionsNode.tryFindChild;
  
  // Override tryFindChild to simulate a collision
  alarmLambda.permissionsNode.tryFindChild = (id: string) => {
    if (id === 'TestAlarmAlarmPermission') {
      // Return a mock permission with a different sourceArn to trigger the warning
      return {
        sourceArn: 'arn:aws:cloudwatch:us-east-1:123456789012:alarm:DifferentAlarm',
      } as unknown as lambda.CfnPermission;
    }
    return originalTryFindChild.call(alarmLambda.permissionsNode, id);
  };
  
  // Create a LambdaAction without useUniquePermissionId
  const lambdaAction = new actions.LambdaAction(alarmLambda);
  
  // WHEN
  alarm.addAlarmAction(lambdaAction);
  
  // THEN
  const annotations = Annotations.fromStack(stack);
  annotations.hasWarning('*', Match.stringLikeRegexp('Please use \'useUniquePermissionId\' to generate unique Lambda Permission Id'));
  
  // Restore the original method
  alarmLambda.permissionsNode.tryFindChild = originalTryFindChild;
});

I haven't tested these options myself, if they are not working let's remove the test of the warning 'throw warning when useUniquePermissionId is not set'

[phuhung273]

The try catch works great, i never think of it. Thanks for your suggestion @scorbiere

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 98d660e
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.