Skip to content

Commit c17879d

Browse files
chenjane-devchenjane-dev
authored
fix(appconfig): scope generated alarm role policy to '*' for composite 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*
1 parent 74335f6 commit c17879d

File tree

10 files changed

+397
-258
lines changed

10 files changed

+397
-258
lines changed

‎packages/@aws-cdk/aws-appconfig-alpha/lib/environment.ts

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import * as iam from 'aws-cdk-lib/aws-iam';
55
import { Construct } from 'constructs';
66
import { IApplication } from './application';
77
import { ActionPoint, IEventDestination, ExtensionOptions, IExtension, IExtensible, ExtensibleBase } from './extension';
8+
import { getHash } from './private/hash';
89

910
/**
1011
* Attributes of an existing AWS AppConfig environment to import it.
@@ -252,11 +253,11 @@ export class Environment extends EnvironmentBase {
252253
applicationId: this.applicationId,
253254
name: this.name,
254255
description: this.description,
255-
monitors: this.monitors?.map((monitor, index) => {
256+
monitors: this.monitors?.map((monitor) => {
256257
return {
257258
alarmArn: monitor.alarmArn,
258259
...(monitor.monitorType === MonitorType.CLOUDWATCH
259-
? { alarmRoleArn: monitor.alarmRoleArn || this.createAlarmRole(monitor, index).roleArn }
260+
? { alarmRoleArn: monitor.alarmRoleArn || this.createOrGetAlarmRole().roleArn }
260261
: { alarmRoleArn: monitor.alarmRoleArn }),
261262
};
262263
}),
@@ -274,24 +275,20 @@ export class Environment extends EnvironmentBase {
274275
this.application.addExistingEnvironment(this);
275276
}
276277

277-
private createAlarmRole(monitor: Monitor, index: number): iam.IRole {
278-
const logicalId = monitor.isCompositeAlarm ? 'RoleCompositeAlarm' : `Role${index}`;
278+
private createOrGetAlarmRole(): iam.IRole {
279+
// the name is guaranteed to be set in line 243
280+
const logicalId = `Role${getHash(this.name!)}`;
279281
const existingRole = this.node.tryFindChild(logicalId) as iam.IRole;
280282
if (existingRole) {
281283
return existingRole;
282284
}
283-
const alarmArn = monitor.isCompositeAlarm
284-
? this.stack.formatArn({
285-
service: 'cloudwatch',
286-
resource: 'alarm',
287-
resourceName: '*',
288-
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
289-
})
290-
: monitor.alarmArn;
285+
// this scope is fine for cloudwatch:DescribeAlarms since it is readonly
286+
// and it is required for composite alarms
287+
// https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_DescribeAlarms.html
291288
const policy = new iam.PolicyStatement({
292289
effect: iam.Effect.ALLOW,
293290
actions: ['cloudwatch:DescribeAlarms'],
294-
resources: [alarmArn],
291+
resources: ['*'],
295292
});
296293
const document = new iam.PolicyDocument({
297294
statements: [policy],
@@ -338,7 +335,6 @@ export abstract class Monitor {
338335
alarmArn: alarm.alarmArn,
339336
alarmRoleArn: alarmRole?.roleArn,
340337
monitorType: MonitorType.CLOUDWATCH,
341-
isCompositeAlarm: alarm instanceof cloudwatch.CompositeAlarm,
342338
};
343339
}
344340

‎packages/@aws-cdk/aws-appconfig-alpha/test/environment.test.ts

Lines changed: 37 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ describe('environment', () => {
140140
},
141141
AlarmRoleArn: {
142142
'Fn::GetAtt': [
143-
'MyEnvironmentRole01C8C013F',
143+
'MyEnvironmentRole1E6113D2F07A1',
144144
'Arn',
145145
],
146146
},
@@ -154,12 +154,7 @@ describe('environment', () => {
154154
Statement: [
155155
{
156156
Effect: iam.Effect.ALLOW,
157-
Resource: {
158-
'Fn::GetAtt': [
159-
'Alarm7103F465',
160-
'Arn',
161-
],
162-
},
157+
Resource: '*',
163158
Action: 'cloudwatch:DescribeAlarms',
164159
},
165160
],
@@ -272,7 +267,7 @@ describe('environment', () => {
272267
},
273268
AlarmRoleArn: {
274269
'Fn::GetAtt': [
275-
'MyEnvironmentRoleCompositeAlarm8C2A0542',
270+
'MyEnvironmentRole1E6113D2F07A1',
276271
'Arn',
277272
],
278273
},
@@ -286,20 +281,7 @@ describe('environment', () => {
286281
Statement: [
287282
{
288283
Effect: iam.Effect.ALLOW,
289-
Resource: {
290-
'Fn::Join': [
291-
'',
292-
[
293-
'arn:',
294-
{ Ref: 'AWS::Partition' },
295-
':cloudwatch:',
296-
{ Ref: 'AWS::Region' },
297-
':',
298-
{ Ref: 'AWS::AccountId' },
299-
':alarm:*',
300-
],
301-
],
302-
},
284+
Resource: '*',
303285
Action: 'cloudwatch:DescribeAlarms',
304286
},
305287
],
@@ -357,7 +339,7 @@ describe('environment', () => {
357339
},
358340
AlarmRoleArn: {
359341
'Fn::GetAtt': [
360-
'MyEnvironmentRoleCompositeAlarm8C2A0542',
342+
'MyEnvironmentRole1E6113D2F07A1',
361343
'Arn',
362344
],
363345
},
@@ -371,7 +353,7 @@ describe('environment', () => {
371353
},
372354
AlarmRoleArn: {
373355
'Fn::GetAtt': [
374-
'MyEnvironmentRoleCompositeAlarm8C2A0542',
356+
'MyEnvironmentRole1E6113D2F07A1',
375357
'Arn',
376358
],
377359
},
@@ -385,20 +367,7 @@ describe('environment', () => {
385367
Statement: [
386368
{
387369
Effect: iam.Effect.ALLOW,
388-
Resource: {
389-
'Fn::Join': [
390-
'',
391-
[
392-
'arn:',
393-
{ Ref: 'AWS::Partition' },
394-
':cloudwatch:',
395-
{ Ref: 'AWS::Region' },
396-
':',
397-
{ Ref: 'AWS::AccountId' },
398-
':alarm:*',
399-
],
400-
],
401-
},
370+
Resource: '*',
402371
Action: 'cloudwatch:DescribeAlarms',
403372
},
404373
],
@@ -409,7 +378,7 @@ describe('environment', () => {
409378
});
410379
});
411380

412-
test('environment with monitors with two alarms', () => {
381+
test('environment with monitors with multiple alarms', () => {
413382
const stack = new cdk.Stack();
414383
const app = new Application(stack, 'MyAppConfig');
415384
const alarm1 = new Alarm(stack, 'Alarm1', {
@@ -432,17 +401,28 @@ describe('environment', () => {
432401
},
433402
),
434403
});
404+
const alarm3 = new Alarm(stack, 'Alarm3', {
405+
threshold: 5,
406+
evaluationPeriods: 5,
407+
metric: new Metric(
408+
{
409+
namespace: 'aws',
410+
metricName: 'myMetric',
411+
},
412+
),
413+
});
435414
new Environment(stack, 'MyEnvironment', {
436415
environmentName: 'TestEnv',
437416
application: app,
438417
monitors: [
439418
Monitor.fromCloudWatchAlarm(alarm1),
440419
Monitor.fromCloudWatchAlarm(alarm2),
420+
Monitor.fromCloudWatchAlarm(alarm3),
441421
],
442422
});
443423

444-
Template.fromStack(stack).resourceCountIs('AWS::CloudWatch::Alarm', 2);
445-
Template.fromStack(stack).resourceCountIs('AWS::IAM::Role', 2);
424+
Template.fromStack(stack).resourceCountIs('AWS::CloudWatch::Alarm', 3);
425+
Template.fromStack(stack).resourceCountIs('AWS::IAM::Role', 1);
446426
Template.fromStack(stack).hasResourceProperties('AWS::AppConfig::Environment', {
447427
Name: 'TestEnv',
448428
ApplicationId: {
@@ -458,7 +438,7 @@ describe('environment', () => {
458438
},
459439
AlarmRoleArn: {
460440
'Fn::GetAtt': [
461-
'MyEnvironmentRole01C8C013F',
441+
'MyEnvironmentRole1E6113D2F07A1',
462442
'Arn',
463443
],
464444
},
@@ -472,7 +452,21 @@ describe('environment', () => {
472452
},
473453
AlarmRoleArn: {
474454
'Fn::GetAtt': [
475-
'MyEnvironmentRole135A2CEE4',
455+
'MyEnvironmentRole1E6113D2F07A1',
456+
'Arn',
457+
],
458+
},
459+
},
460+
{
461+
AlarmArn: {
462+
'Fn::GetAtt': [
463+
'Alarm32341D8D9',
464+
'Arn',
465+
],
466+
},
467+
AlarmRoleArn: {
468+
'Fn::GetAtt': [
469+
'MyEnvironmentRole1E6113D2F07A1',
476470
'Arn',
477471
],
478472
},

‎packages/@aws-cdk/aws-appconfig-alpha/test/integ.environment.js.snapshot/appconfigenvironmentDefaultTestDeployAssert75BD28E7.assets.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/@aws-cdk/aws-appconfig-alpha/test/integ.environment.js.snapshot/aws-appconfig-environment.assets.json

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)