fix(appconfig): scope generated alarm role policy to '*' for composit…

…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*
aws · Feb 22, 2024 · 424d89d · 424d89d
1 parent 0b9d3d9
commit 424d89d
Show file tree

Hide file tree

Showing 10 changed files with 397 additions and 258 deletions.
diff --git a/packages/@aws-cdk/aws-appconfig-alpha/lib/environment.ts b/packages/@aws-cdk/aws-appconfig-alpha/lib/environment.ts
@@ -5,6 +5,7 @@ import * as iam from 'aws-cdk-lib/aws-iam';
 import { Construct } from 'constructs';
 import { IApplication } from './application';
 import { ActionPoint, IEventDestination, ExtensionOptions, IExtension, IExtensible, ExtensibleBase } from './extension';
+import { getHash } from './private/hash';
 
 /**
  * Attributes of an existing AWS AppConfig environment to import it.
@@ -252,11 +253,11 @@ export class Environment extends EnvironmentBase {
       applicationId: this.applicationId,
       name: this.name,
       description: this.description,
-      monitors: this.monitors?.map((monitor, index) => {
+      monitors: this.monitors?.map((monitor) => {
         return {
           alarmArn: monitor.alarmArn,
           ...(monitor.monitorType === MonitorType.CLOUDWATCH
-            ? { alarmRoleArn: monitor.alarmRoleArn || this.createAlarmRole(monitor, index).roleArn }
+            ? { alarmRoleArn: monitor.alarmRoleArn || this.createOrGetAlarmRole().roleArn }
             : { alarmRoleArn: monitor.alarmRoleArn }),
         };
       }),
@@ -274,24 +275,20 @@ export class Environment extends EnvironmentBase {
     this.application.addExistingEnvironment(this);
   }
 
-  private createAlarmRole(monitor: Monitor, index: number): iam.IRole {
-    const logicalId = monitor.isCompositeAlarm ? 'RoleCompositeAlarm' : `Role${index}`;
+  private createOrGetAlarmRole(): iam.IRole {
+    // the name is guaranteed to be set in line 243
+    const logicalId = `Role${getHash(this.name!)}`;
     const existingRole = this.node.tryFindChild(logicalId) as iam.IRole;
     if (existingRole) {
       return existingRole;
     }
-    const alarmArn = monitor.isCompositeAlarm
-      ? this.stack.formatArn({
-        service: 'cloudwatch',
-        resource: 'alarm',
-        resourceName: '*',
-        arnFormat: ArnFormat.COLON_RESOURCE_NAME,
-      })
-      : monitor.alarmArn;
+    // this scope is fine for cloudwatch:DescribeAlarms since it is readonly
+    // and it is required for composite alarms
+    // https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_DescribeAlarms.html
     const policy = new iam.PolicyStatement({
       effect: iam.Effect.ALLOW,
       actions: ['cloudwatch:DescribeAlarms'],
-      resources: [alarmArn],
+      resources: ['*'],
     });
     const document = new iam.PolicyDocument({
       statements: [policy],
@@ -338,7 +335,6 @@ export abstract class Monitor {
       alarmArn: alarm.alarmArn,
       alarmRoleArn: alarmRole?.roleArn,
       monitorType: MonitorType.CLOUDWATCH,
-      isCompositeAlarm: alarm instanceof cloudwatch.CompositeAlarm,
     };
   }
 

diff --git a/packages/@aws-cdk/aws-appconfig-alpha/test/environment.test.ts b/packages/@aws-cdk/aws-appconfig-alpha/test/environment.test.ts
@@ -140,7 +140,7 @@ describe('environment', () => {
           },
           AlarmRoleArn: {
             'Fn::GetAtt': [
-              'MyEnvironmentRole01C8C013F',
+              'MyEnvironmentRole1E6113D2F07A1',
               'Arn',
             ],
           },
@@ -154,12 +154,7 @@ describe('environment', () => {
             Statement: [
               {
                 Effect: iam.Effect.ALLOW,
-                Resource: {
-                  'Fn::GetAtt': [
-                    'Alarm7103F465',
-                    'Arn',
-                  ],
-                },
+                Resource: '*',
                 Action: 'cloudwatch:DescribeAlarms',
               },
             ],
@@ -272,7 +267,7 @@ describe('environment', () => {
           },
           AlarmRoleArn: {
             'Fn::GetAtt': [
-              'MyEnvironmentRoleCompositeAlarm8C2A0542',
+              'MyEnvironmentRole1E6113D2F07A1',
               'Arn',
             ],
           },
@@ -286,20 +281,7 @@ describe('environment', () => {
             Statement: [
               {
                 Effect: iam.Effect.ALLOW,
-                Resource: {
-                  'Fn::Join': [
-                    '',
-                    [
-                      'arn:',
-                      { Ref: 'AWS::Partition' },
-                      ':cloudwatch:',
-                      { Ref: 'AWS::Region' },
-                      ':',
-                      { Ref: 'AWS::AccountId' },
-                      ':alarm:*',
-                    ],
-                  ],
-                },
+                Resource: '*',
                 Action: 'cloudwatch:DescribeAlarms',
               },
             ],
@@ -357,7 +339,7 @@ describe('environment', () => {
           },
           AlarmRoleArn: {
             'Fn::GetAtt': [
-              'MyEnvironmentRoleCompositeAlarm8C2A0542',
+              'MyEnvironmentRole1E6113D2F07A1',
               'Arn',
             ],
           },
@@ -371,7 +353,7 @@ describe('environment', () => {
           },
           AlarmRoleArn: {
             'Fn::GetAtt': [
-              'MyEnvironmentRoleCompositeAlarm8C2A0542',
+              'MyEnvironmentRole1E6113D2F07A1',
               'Arn',
             ],
           },
@@ -385,20 +367,7 @@ describe('environment', () => {
             Statement: [
               {
                 Effect: iam.Effect.ALLOW,
-                Resource: {
-                  'Fn::Join': [
-                    '',
-                    [
-                      'arn:',
-                      { Ref: 'AWS::Partition' },
-                      ':cloudwatch:',
-                      { Ref: 'AWS::Region' },
-                      ':',
-                      { Ref: 'AWS::AccountId' },
-                      ':alarm:*',
-                    ],
-                  ],
-                },
+                Resource: '*',
                 Action: 'cloudwatch:DescribeAlarms',
               },
             ],
@@ -409,7 +378,7 @@ describe('environment', () => {
     });
   });
 
-  test('environment with monitors with two alarms', () => {
+  test('environment with monitors with multiple alarms', () => {
     const stack = new cdk.Stack();
     const app = new Application(stack, 'MyAppConfig');
     const alarm1 = new Alarm(stack, 'Alarm1', {
@@ -432,17 +401,28 @@ describe('environment', () => {
         },
       ),
     });
+    const alarm3 = new Alarm(stack, 'Alarm3', {
+      threshold: 5,
+      evaluationPeriods: 5,
+      metric: new Metric(
+        {
+          namespace: 'aws',
+          metricName: 'myMetric',
+        },
+      ),
+    });
     new Environment(stack, 'MyEnvironment', {
       environmentName: 'TestEnv',
       application: app,
       monitors: [
         Monitor.fromCloudWatchAlarm(alarm1),
         Monitor.fromCloudWatchAlarm(alarm2),
+        Monitor.fromCloudWatchAlarm(alarm3),
       ],
     });
 
-    Template.fromStack(stack).resourceCountIs('AWS::CloudWatch::Alarm', 2);
-    Template.fromStack(stack).resourceCountIs('AWS::IAM::Role', 2);
+    Template.fromStack(stack).resourceCountIs('AWS::CloudWatch::Alarm', 3);
+    Template.fromStack(stack).resourceCountIs('AWS::IAM::Role', 1);
     Template.fromStack(stack).hasResourceProperties('AWS::AppConfig::Environment', {
       Name: 'TestEnv',
       ApplicationId: {
@@ -458,7 +438,7 @@ describe('environment', () => {
           },
           AlarmRoleArn: {
             'Fn::GetAtt': [
-              'MyEnvironmentRole01C8C013F',
+              'MyEnvironmentRole1E6113D2F07A1',
               'Arn',
             ],
           },
@@ -472,7 +452,21 @@ describe('environment', () => {
           },
           AlarmRoleArn: {
             'Fn::GetAtt': [
-              'MyEnvironmentRole135A2CEE4',
+              'MyEnvironmentRole1E6113D2F07A1',
+              'Arn',
+            ],
+          },
+        },
+        {
+          AlarmArn: {
+            'Fn::GetAtt': [
+              'Alarm32341D8D9',
+              'Arn',
+            ],
+          },
+          AlarmRoleArn: {
+            'Fn::GetAtt': [
+              'MyEnvironmentRole1E6113D2F07A1',
               'Arn',
             ],
           },

diff --git a/...g.environment.js.snapshot/appconfigenvironmentDefaultTestDeployAssert75BD28E7.assets.json b/...g.environment.js.snapshot/appconfigenvironmentDefaultTestDeployAssert75BD28E7.assets.json
diff --git a/...-appconfig-alpha/test/integ.environment.js.snapshot/aws-appconfig-environment.assets.json b/...-appconfig-alpha/test/integ.environment.js.snapshot/aws-appconfig-environment.assets.json