fix(appconfig): scope generated alarm role policy to '*' for composite alarm support #29171
Conversation
github-actions
bot
added
admired-contributor
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
| const policy = new iam.PolicyStatement({ | ||
| effect: iam.Effect.ALLOW, | ||
| actions: ['cloudwatch:DescribeAlarms'], | ||
| resources: [alarmArn], | ||
| resources: ['*'], |
There was a problem hiding this comment.
Why '*' and not monitor.alarmArn? I don't like the default to all resources from a security perspective, so if this has to stay, we have to document exactly why we are doing it this way. * sends off alarm bells in a lot of customer use cases.
There was a problem hiding this comment.
The integ test already tests this because there is only one policy being created now with resource set to *.
Yeah, I discussed this with a senior engineer on my team and it is only being scoped to all resources for DescribeAlarms so I think this should be okay. But, after doing some personal testing, I found out that the original policy we had for composite alarms actually works when scoped narrower than * so the CW docs must be incorrect here. We could change this to that if you want too
https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_DescribeAlarms.html
There was a problem hiding this comment.
re: integ test
the test checks if the policy is being created with the resource set to *. it does not yet test that that policy can be used successfully. i argue that the latter is the important part, the former is simply a unit test.
edit: you know what I'm fine with this as is. disregard the above statement.
re: resource
since it is just a scope for describeAlarms, i.e. a readonly prop, we should be fine with *. Can you document in the code that we are okay with this for the readonly permissions and link to the doc page?
kaizencc
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| private createAlarmRole(monitor: Monitor, index: number): iam.IRole { | ||
| const logicalId = monitor.isCompositeAlarm ? 'RoleCompositeAlarm' : `Role${index}`; | ||
| private createOrGetAlarmRole(): iam.IRole { | ||
| const logicalId = 'Role'; |
There was a problem hiding this comment.
Ooh, we should have a much more descriptive logicalId here if we are searching for it in the tree. This name feels far too generic and can result in collisions.
There was a problem hiding this comment.
Good catch! Updated
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
aws-cdk-automation
added
the
pr/needs-community-review
|
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). |
aws-cdk-automation
removed
the
pr/needs-community-review
…e alarm support (#29171) ### Reason for this change Customers could create an alarm of type `IAlarm` where the alarm would be a composite alarm (for ex, when importing composite alarms, they return a type `IAlarm` instead of `CompositeAlarm`. This caused the logic that differentiates a composite alarm to mistakenly categorize the alarm incorrectly. ### Description of changes Add one policy scoped to `*` for any alarm passed to an environment. ### Description of how you validated changes Unit and integration 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*
Reason for this change
Customers could create an alarm of type
IAlarmwhere the alarm would be a composite alarm (for ex, when importing composite alarms, they return a typeIAlarminstead ofCompositeAlarm. This caused the logic that differentiates a composite alarm to mistakenly categorize the alarm incorrectly.Description of changes
Add one policy scoped to
*for any alarm passed to an environment.Description of how you validated changes
Unit and integration tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license