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.

Sign up for GitHub

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

Jump to bottom

fix(appconfig): scope generated alarm role policy to '*' for composite alarm support #29171

Merged
merged 11 commits into from
Feb 20, 2024
Merged

fix(appconfig): scope generated alarm role policy to '*' for composite alarm support #29171

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e3f1873
fix(appconfig): correctly scope composite alarm policy to '*'
chenjane-dev Feb 19, 2024
24577ff
fix(appconfig): scope generated alarm role policy to '*' for composit…
chenjane-dev Feb 19, 2024
41c3922
rename private method
chenjane-dev Feb 19, 2024
0f0b773
update integ tests and add reasoning for full scope policy
chenjane-dev Feb 20, 2024
3c1ef5b
Merge branch 'main' into composite-alarm
chenjane-dev Feb 20, 2024
8f419ff
update role logical id
chenjane-dev Feb 20, 2024
c07e6ea
fix eslint
chenjane-dev Feb 20, 2024
316d421
Merge branch 'aws:main' into composite-alarm
chenjane-dev Feb 20, 2024
c393c86
update integ tests and add reasoning for full scope policy
chenjane-dev Feb 20, 2024
b778fb4
fix eslint
chenjane-dev Feb 20, 2024
7aa65c9
update snapshot
chenjane-dev Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 5 additions & 14 deletions packages/@aws-cdk/aws-appconfig-alpha/lib/environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,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 }),
};
}),
Expand All @@ -274,24 +274,16 @@ 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 {
const logicalId = 'Role';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Updated

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;
const policy = new iam.PolicyStatement({
effect: iam.Effect.ALLOW,
actions: ['cloudwatch:DescribeAlarms'],
resources: [alarmArn],
resources: ['*'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kaizencc kaizencc Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

});
const document = new iam.PolicyDocument({
statements: [policy],
Expand Down Expand Up @@ -338,7 +330,6 @@ export abstract class Monitor {
alarmArn: alarm.alarmArn,
alarmRoleArn: alarmRole?.roleArn,
monitorType: MonitorType.CLOUDWATCH,
isCompositeAlarm: alarm instanceof cloudwatch.CompositeAlarm,
};
}

Expand Down
80 changes: 37 additions & 43 deletions packages/@aws-cdk/aws-appconfig-alpha/test/environment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ describe('environment', () => {
},
AlarmRoleArn: {
'Fn::GetAtt': [
'MyEnvironmentRole01C8C013F',
'MyEnvironmentRoleC08961D3',
'Arn',
],
},
Expand All @@ -154,12 +154,7 @@ describe('environment', () => {
Statement: [
{
Effect: iam.Effect.ALLOW,
Resource: {
'Fn::GetAtt': [
'Alarm7103F465',
'Arn',
],
},
Resource: '*',
Action: 'cloudwatch:DescribeAlarms',
},
],
Expand Down Expand Up @@ -272,7 +267,7 @@ describe('environment', () => {
},
AlarmRoleArn: {
'Fn::GetAtt': [
'MyEnvironmentRoleCompositeAlarm8C2A0542',
'MyEnvironmentRoleC08961D3',
'Arn',
],
},
Expand All @@ -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',
},
],
Expand Down Expand Up @@ -357,7 +339,7 @@ describe('environment', () => {
},
AlarmRoleArn: {
'Fn::GetAtt': [
'MyEnvironmentRoleCompositeAlarm8C2A0542',
'MyEnvironmentRoleC08961D3',
'Arn',
],
},
Expand All @@ -371,7 +353,7 @@ describe('environment', () => {
},
AlarmRoleArn: {
'Fn::GetAtt': [
'MyEnvironmentRoleCompositeAlarm8C2A0542',
'MyEnvironmentRoleC08961D3',
'Arn',
],
},
Expand All @@ -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',
},
],
Expand All @@ -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', {
Expand All @@ -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: {
Expand All @@ -458,7 +438,7 @@ describe('environment', () => {
},
AlarmRoleArn: {
'Fn::GetAtt': [
'MyEnvironmentRole01C8C013F',
'MyEnvironmentRoleC08961D3',
'Arn',
],
},
Expand All @@ -472,7 +452,21 @@ describe('environment', () => {
},
AlarmRoleArn: {
'Fn::GetAtt': [
'MyEnvironmentRole135A2CEE4',
'MyEnvironmentRoleC08961D3',
'Arn',
],
},
},
{
AlarmArn: {
'Fn::GetAtt': [
'Alarm32341D8D9',
'Arn',
],
},
AlarmRoleArn: {
'Fn::GetAtt': [
'MyEnvironmentRoleC08961D3',
'Arn',
],
},
Expand Down
2 changes: 1 addition & 1 deletion ...g.environment.js.snapshot/appconfigenvironmentDefaultTestDeployAssert75BD28E7.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions ...-appconfig-alpha/test/integ.environment.js.snapshot/aws-appconfig-environment.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

64 changes: 4 additions & 60 deletions ...ppconfig-alpha/test/integ.environment.js.snapshot/aws-appconfig-environment.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
}
}
},
"MyEnvironmentRole01C8C013F": {
"MyEnvironmentRoleC08961D3": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
Expand All @@ -78,63 +78,7 @@
{
"Action": "cloudwatch:DescribeAlarms",
"Effect": "Allow",
"Resource": {
"Fn::GetAtt": [
"MyAlarm696658B6",
"Arn"
]
}
}
],
"Version": "2012-10-17"
},
"PolicyName": "AllowAppConfigMonitorAlarmPolicy"
}
]
}
},
"MyEnvironmentRoleCompositeAlarm8C2A0542": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": "appconfig.amazonaws.com"
}
}
],
"Version": "2012-10-17"
},
"Policies": [
{
"PolicyDocument": {
"Statement": [
{
"Action": "cloudwatch:DescribeAlarms",
"Effect": "Allow",
"Resource": {
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":cloudwatch:",
{
"Ref": "AWS::Region"
},
":",
{
"Ref": "AWS::AccountId"
},
":alarm:*"
]
]
}
"Resource": "*"
}
],
"Version": "2012-10-17"
Expand All @@ -161,7 +105,7 @@
},
"AlarmRoleArn": {
"Fn::GetAtt": [
"MyEnvironmentRole01C8C013F",
"MyEnvironmentRoleC08961D3",
"Arn"
]
}
Expand Down Expand Up @@ -189,7 +133,7 @@
},
"AlarmRoleArn": {
"Fn::GetAtt": [
"MyEnvironmentRoleCompositeAlarm8C2A0542",
"MyEnvironmentRoleC08961D3",
"Arn"
]
}
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-appconfig-alpha/test/integ.environment.js.snapshot/cdk.out
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-appconfig-alpha/test/integ.environment.js.snapshot/integ.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading