Skip to content

Commit aee1b30

Browse files
gracelu0gracelu0
authored
fix(scheduler-targets-alpha): create a role per target instead of singleton schedule target role (#31895)
### Issue # (if applicable) Tracking #31785. ### Reason for this change The current logic for creating a schedule target execution role uses a hash on the `targetArn` to determine if there is an existing role in the stack. Currently if the `targetArn` contains token values (e.g. intrinsic functions), `stack.resolve(targetArn).toString()` is used to convert the tokenized ARN into a string. However this always results in `[object Object]` which then gets hashed, meaning the same role is used for any target where the ARN passed in is not a pure string. This does not follow principle of least privilege, and a singleton role used across multiple different targets/target types can be confusing for the customer to manage. ### Description of changes - Use `JSON.stringify()` instead of `.toString()` to produce unique hash (and thus create new role) per target. ### Description of how you validated changes Updated unit tests 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) BREAKING CHANGE: Schedule Target will reuse role if target is re-used across schedules. This change triggered replacement of existing roles for Schedule as logical ID of the roles are changed. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 85fd5f7 commit aee1b30

File tree

101 files changed

+236460
-214577
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

101 files changed

+236460
-214577
lines changed

‎packages/@aws-cdk/aws-scheduler-targets-alpha/lib/target.ts

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { ISchedule, ScheduleTargetConfig, ScheduleTargetInput } from '@aws-cdk/aws-scheduler-alpha';
2-
import { Annotations, Duration, Names, PhysicalName, Token, Stack } from 'aws-cdk-lib';
2+
import { Annotations, Duration, Names, PhysicalName, Stack, Token } from 'aws-cdk-lib';
33
import * as iam from 'aws-cdk-lib/aws-iam';
44
import { CfnSchedule } from 'aws-cdk-lib/aws-scheduler';
55
import * as sqs from 'aws-cdk-lib/aws-sqs';
@@ -76,7 +76,8 @@ export abstract class ScheduleTargetBase {
7676
protected abstract addTargetActionToRole(schedule: ISchedule, role: iam.IRole): void;
7777

7878
protected bindBaseTargetConfig(_schedule: ISchedule): ScheduleTargetConfig {
79-
const role: iam.IRole = this.baseProps.role ?? this.singletonScheduleRole(_schedule, this.targetArn);
79+
const role: iam.IRole = this.baseProps.role ?? this.createOrGetScheduleTargetRole(_schedule, this.targetArn);
80+
8081
this.addTargetActionToRole(_schedule, role);
8182

8283
if (this.baseProps.deadLetterQueue) {
@@ -85,7 +86,7 @@ export abstract class ScheduleTargetBase {
8586

8687
return {
8788
arn: this.targetArn,
88-
role: role,
89+
role,
8990
deadLetterConfig: this.baseProps.deadLetterQueue ? {
9091
arn: this.baseProps.deadLetterQueue.queueArn,
9192
} : undefined,
@@ -104,14 +105,14 @@ export abstract class ScheduleTargetBase {
104105
}
105106

106107
/**
107-
* Obtain the Role for the EventBridge Scheduler event
108+
* Get or create the Role for the EventBridge Scheduler event
108109
*
109110
* If a role already exists, it will be returned. This ensures that if multiple
110-
* events have the same target, they will share a role.
111+
* schedules have the same target, they will share a role.
111112
*/
112-
private singletonScheduleRole(schedule: ISchedule, targetArn: string): iam.IRole {
113+
private createOrGetScheduleTargetRole(schedule: ISchedule, targetArn: string): iam.IRole {
113114
const stack = Stack.of(schedule);
114-
const arn = Token.isUnresolved(targetArn) ? stack.resolve(targetArn).toString() : targetArn;
115+
const arn = Token.isUnresolved(targetArn) ? JSON.stringify(stack.resolve(targetArn)) : targetArn;
115116
const hash = md5hash(arn).slice(0, 6);
116117
const id = 'SchedulerRoleForTarget-' + hash;
117118
const existingRole = stack.node.tryFindChild(id) as iam.Role;
@@ -120,9 +121,17 @@ export abstract class ScheduleTargetBase {
120121
conditions: {
121122
StringEquals: {
122123
'aws:SourceAccount': schedule.env.account,
124+
'aws:SourceArn': schedule.group?.groupArn ?? Stack.of(schedule).formatArn({
125+
service: 'scheduler',
126+
resource: 'schedule-group',
127+
region: schedule.env.region,
128+
account: schedule.env.account,
129+
resourceName: schedule.group?.groupName ?? 'default',
130+
}),
123131
},
124132
},
125133
});
134+
126135
if (existingRole) {
127136
existingRole.assumeRolePolicy?.addStatements(new iam.PolicyStatement({
128137
effect: iam.Effect.ALLOW,

‎packages/@aws-cdk/aws-scheduler-targets-alpha/test/codebuild-start-build.test.ts

Lines changed: 124 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Schedule, ScheduleExpression } from '@aws-cdk/aws-scheduler-alpha';
1+
import { Group, Schedule, ScheduleExpression } from '@aws-cdk/aws-scheduler-alpha';
22
import { App, Duration, Stack } from 'aws-cdk-lib';
33
import { Template } from 'aws-cdk-lib/assertions';
44
import { BuildSpec, Project } from 'aws-cdk-lib/aws-codebuild';
@@ -14,6 +14,7 @@ describe('codebuild start build', () => {
1414
const codebuildArnRef = { 'Fn::GetAtt': ['ProjectC78D97AD', 'Arn'] };
1515
const codebuildAction = 'codebuild:StartBuild';
1616
const expr = ScheduleExpression.at(new Date(Date.UTC(1991, 2, 24, 0, 0, 0)));
17+
const roleId = 'SchedulerRoleForTarget27bd47517CF0F8';
1718

1819
beforeEach(() => {
1920
app = new App({ context: { '@aws-cdk/aws-iam:minimizePolicies': true } });
@@ -36,7 +37,7 @@ describe('codebuild start build', () => {
3637
Properties: {
3738
Target: {
3839
Arn: codebuildArnRef,
39-
RoleArn: { 'Fn::GetAtt': ['SchedulerRoleForTarget1441a743A31888', 'Arn'] },
40+
RoleArn: { 'Fn::GetAtt': [roleId, 'Arn'] },
4041
RetryPolicy: {},
4142
},
4243
},
@@ -52,7 +53,7 @@ describe('codebuild start build', () => {
5253
},
5354
],
5455
},
55-
Roles: [{ Ref: 'SchedulerRoleForTarget1441a743A31888' }],
56+
Roles: [{ Ref: roleId }],
5657
});
5758

5859
template.hasResourceProperties('AWS::IAM::Role', {
@@ -61,7 +62,23 @@ describe('codebuild start build', () => {
6162
Statement: [
6263
{
6364
Effect: 'Allow',
64-
Condition: { StringEquals: { 'aws:SourceAccount': '123456789012' } },
65+
Condition: {
66+
StringEquals: {
67+
'aws:SourceAccount': '123456789012',
68+
'aws:SourceArn': {
69+
'Fn::Join': [
70+
'',
71+
[
72+
'arn:',
73+
{
74+
Ref: 'AWS::Partition',
75+
},
76+
':scheduler:us-east-1:123456789012:schedule-group/default',
77+
],
78+
],
79+
},
80+
},
81+
},
6582
Principal: {
6683
Service: 'scheduler.amazonaws.com',
6784
},
@@ -113,8 +130,72 @@ describe('codebuild start build', () => {
113130
});
114131
});
115132

116-
test('reuses IAM role and IAM policy for two schedules from the same account', () => {
133+
test('reuses IAM role and IAM policy for two schedules with the same target from the same account', () => {
134+
const codeBuildTarget = new CodeBuildStartBuild(codebuildProject, {});
135+
136+
new Schedule(stack, 'MyScheduleDummy1', {
137+
schedule: expr,
138+
target: codeBuildTarget,
139+
});
140+
141+
new Schedule(stack, 'MyScheduleDummy2', {
142+
schedule: expr,
143+
target: codeBuildTarget,
144+
});
145+
146+
const template = Template.fromStack(stack);
147+
148+
template.resourcePropertiesCountIs('AWS::IAM::Role', {
149+
AssumeRolePolicyDocument: {
150+
Version: '2012-10-17',
151+
Statement: [
152+
{
153+
Effect: 'Allow',
154+
Condition: {
155+
StringEquals: {
156+
'aws:SourceAccount': '123456789012',
157+
'aws:SourceArn': {
158+
'Fn::Join': [
159+
'',
160+
[
161+
'arn:',
162+
{
163+
Ref: 'AWS::Partition',
164+
},
165+
':scheduler:us-east-1:123456789012:schedule-group/default',
166+
],
167+
],
168+
},
169+
},
170+
},
171+
Principal: {
172+
Service: 'scheduler.amazonaws.com',
173+
},
174+
Action: 'sts:AssumeRole',
175+
},
176+
],
177+
},
178+
}, 1);
179+
180+
template.resourcePropertiesCountIs('AWS::IAM::Policy', {
181+
PolicyDocument: {
182+
Statement: [
183+
{
184+
Action: codebuildAction,
185+
Effect: 'Allow',
186+
Resource: codebuildArnRef,
187+
},
188+
],
189+
},
190+
Roles: [{ Ref: roleId }],
191+
}, 1);
192+
});
193+
194+
test('creates IAM role and IAM policy for two schedules with the same target but different groups', () => {
117195
const codeBuildTarget = new CodeBuildStartBuild(codebuildProject, {});
196+
const group = new Group(stack, 'Group', {
197+
groupName: 'mygroup',
198+
});
118199

119200
new Schedule(stack, 'MyScheduleDummy1', {
120201
schedule: expr,
@@ -124,6 +205,7 @@ describe('codebuild start build', () => {
124205
new Schedule(stack, 'MyScheduleDummy2', {
125206
schedule: expr,
126207
target: codeBuildTarget,
208+
group,
127209
});
128210

129211
const template = Template.fromStack(stack);
@@ -134,7 +216,41 @@ describe('codebuild start build', () => {
134216
Statement: [
135217
{
136218
Effect: 'Allow',
137-
Condition: { StringEquals: { 'aws:SourceAccount': '123456789012' } },
219+
Condition: {
220+
StringEquals: {
221+
'aws:SourceAccount': '123456789012',
222+
'aws:SourceArn': {
223+
'Fn::Join': [
224+
'',
225+
[
226+
'arn:',
227+
{
228+
Ref: 'AWS::Partition',
229+
},
230+
':scheduler:us-east-1:123456789012:schedule-group/default',
231+
],
232+
],
233+
},
234+
},
235+
},
236+
Principal: {
237+
Service: 'scheduler.amazonaws.com',
238+
},
239+
Action: 'sts:AssumeRole',
240+
},
241+
{
242+
Effect: 'Allow',
243+
Condition: {
244+
StringEquals: {
245+
'aws:SourceAccount': '123456789012',
246+
'aws:SourceArn': {
247+
'Fn::GetAtt': [
248+
'GroupC77FDACD',
249+
'Arn',
250+
],
251+
},
252+
},
253+
},
138254
Principal: {
139255
Service: 'scheduler.amazonaws.com',
140256
},
@@ -154,7 +270,7 @@ describe('codebuild start build', () => {
154270
},
155271
],
156272
},
157-
Roles: [{ Ref: 'SchedulerRoleForTarget1441a743A31888' }],
273+
Roles: [{ Ref: roleId }],
158274
}, 1);
159275
});
160276

@@ -411,7 +527,7 @@ describe('codebuild start build', () => {
411527
Properties: {
412528
Target: {
413529
Arn: codebuildArnRef,
414-
RoleArn: { 'Fn::GetAtt': ['SchedulerRoleForTarget1441a743A31888', 'Arn'] },
530+
RoleArn: { 'Fn::GetAtt': [roleId, 'Arn'] },
415531
RetryPolicy: {
416532
MaximumEventAgeInSeconds: 10800,
417533
MaximumRetryAttempts: 5,

0 commit comments

Comments
 (0)