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

aws-cloudwatch: Imported composite alarm ARN incorrect #24594

Closed
nakedible-p opened this issue Mar 13, 2023 · 3 comments · Fixed by #24604
Closed

aws-cloudwatch: Imported composite alarm ARN incorrect #24594

nakedible-p opened this issue Mar 13, 2023 · 3 comments · Fixed by #24604
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@nakedible-p
Copy link
Contributor

Describe the bug

It seems that the ARN used for CompositeAlarms is incorrectly generated when using CompositeAlarm.fromCompositeAlarmName. The resulting ARN has / as separator where as the actual alarm has :.

The whole thing is a bit wonky too:

  • In reality, ARNs for Alarm and CompositeAlarm instances are equivalent, yet in CDK they are different
  • There is no Alarm.fromAlarmName, even though CompositeAlarm.fromCompositeAlarmName exists
  • The resulting type for either is IAlarm anyway, so the distinction between Alarm and CompositeAlarm is lost

Fix is trivial, but might require more rework to fix this all.

Expected Behavior

I expected an ARN of the form:

arn:aws:cloudwatch:eu-central-1:219189227975:alarm:NpayGatewayRegionalSyteCentral-Stack-RollbackAlarm

Current Behavior

I got an ARN of the form:

arn:aws:cloudwatch:eu-central-1:219189227975:alarm/NpayGatewayRegionalSyteCentral-Stack-RollbackAlarm

I tried using this as a rollback trigger, and got the error:

RelativeId of a Rollback Trigger's ARN is incorrect

Reproduction Steps

Call code as such:

const testAlarm = CompositeAlarm.fromCompositeAlarmName(this, 'alarm', `TestAlarm`);
console.log(testAlarm.alarmArn);

Possible Solution

https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-cloudwatch/lib/composite-alarm.ts#L76-L80

Pass in arnFormat: ArnFormat.COLON_RESOURCE_NAME.

Also below:

https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-cloudwatch/lib/composite-alarm.ts#L93

Use ArnFormat.COLON_RESOURCE_NAME.

Suggest aligning these methods with Alarm class as well.

Additional Information/Context

No response

CDK CLI Version

2.68.0 (build 25fda51)

Framework Version

No response

Node.js Version

v19.7.0

OS

Linux codespaces-3481fc 5.4.0-1104-azure #110~18.04.1-Ubuntu SMP Sat Feb 11 17:41:21 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Language

Typescript

Language Version

Version 4.9.5

Other information

No response

@nakedible-p nakedible-p added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 13, 2023
@github-actions github-actions bot added the @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch label Mar 13, 2023
@pahud
Copy link
Contributor

pahud commented Mar 13, 2023

Thank you for the bug report. I'm making it p1 though small effort. Any PR submission would be welcome and appreciated.

@pahud pahud added p1 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 13, 2023
@nakedible-p
Copy link
Contributor Author

Added a quick PR, most likely still needs work to be complete.

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

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

Successfully merging a pull request may close this issue.

2 participants