fix(scheduler-targets-alpha): create a role per target instead of sin…

…gleton 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*
aws · Nov 1, 2024 · aee1b30 · aee1b30
1 parent 85fd5f7
commit aee1b30
Show file tree

Hide file tree

Showing 101 changed files with 236,460 additions and 214,577 deletions.
diff --git a/packages/@aws-cdk/aws-scheduler-targets-alpha/lib/target.ts b/packages/@aws-cdk/aws-scheduler-targets-alpha/lib/target.ts
@@ -1,5 +1,5 @@
 import { ISchedule, ScheduleTargetConfig, ScheduleTargetInput } from '@aws-cdk/aws-scheduler-alpha';
-import { Annotations, Duration, Names, PhysicalName, Token, Stack } from 'aws-cdk-lib';
+import { Annotations, Duration, Names, PhysicalName, Stack, Token } from 'aws-cdk-lib';
 import * as iam from 'aws-cdk-lib/aws-iam';
 import { CfnSchedule } from 'aws-cdk-lib/aws-scheduler';
 import * as sqs from 'aws-cdk-lib/aws-sqs';
@@ -76,7 +76,8 @@ export abstract class ScheduleTargetBase {
   protected abstract addTargetActionToRole(schedule: ISchedule, role: iam.IRole): void;
 
   protected bindBaseTargetConfig(_schedule: ISchedule): ScheduleTargetConfig {
-    const role: iam.IRole = this.baseProps.role ?? this.singletonScheduleRole(_schedule, this.targetArn);
+    const role: iam.IRole = this.baseProps.role ?? this.createOrGetScheduleTargetRole(_schedule, this.targetArn);
+
     this.addTargetActionToRole(_schedule, role);
 
     if (this.baseProps.deadLetterQueue) {
@@ -85,7 +86,7 @@ export abstract class ScheduleTargetBase {
 
     return {
       arn: this.targetArn,
-      role: role,
+      role,
       deadLetterConfig: this.baseProps.deadLetterQueue ? {
         arn: this.baseProps.deadLetterQueue.queueArn,
       } : undefined,
@@ -104,14 +105,14 @@ export abstract class ScheduleTargetBase {
   }
 
   /**
-   * Obtain the Role for the EventBridge Scheduler event
+   * Get or create the Role for the EventBridge Scheduler event
    *
    * If a role already exists, it will be returned. This ensures that if multiple
-   * events have the same target, they will share a role.
+   * schedules have the same target, they will share a role.
    */
-  private singletonScheduleRole(schedule: ISchedule, targetArn: string): iam.IRole {
+  private createOrGetScheduleTargetRole(schedule: ISchedule, targetArn: string): iam.IRole {
     const stack = Stack.of(schedule);
-    const arn = Token.isUnresolved(targetArn) ? stack.resolve(targetArn).toString() : targetArn;
+    const arn = Token.isUnresolved(targetArn) ? JSON.stringify(stack.resolve(targetArn)) : targetArn;
     const hash = md5hash(arn).slice(0, 6);
     const id = 'SchedulerRoleForTarget-' + hash;
     const existingRole = stack.node.tryFindChild(id) as iam.Role;
@@ -120,9 +121,17 @@ export abstract class ScheduleTargetBase {
       conditions: {
         StringEquals: {
           'aws:SourceAccount': schedule.env.account,
+          'aws:SourceArn': schedule.group?.groupArn ?? Stack.of(schedule).formatArn({
+            service: 'scheduler',
+            resource: 'schedule-group',
+            region: schedule.env.region,
+            account: schedule.env.account,
+            resourceName: schedule.group?.groupName ?? 'default',
+          }),
         },
       },
     });
+
     if (existingRole) {
       existingRole.assumeRolePolicy?.addStatements(new iam.PolicyStatement({
         effect: iam.Effect.ALLOW,

diff --git a/packages/@aws-cdk/aws-scheduler-targets-alpha/test/codebuild-start-build.test.ts b/packages/@aws-cdk/aws-scheduler-targets-alpha/test/codebuild-start-build.test.ts
@@ -1,4 +1,4 @@
-import { Schedule, ScheduleExpression } from '@aws-cdk/aws-scheduler-alpha';
+import { Group, Schedule, ScheduleExpression } from '@aws-cdk/aws-scheduler-alpha';
 import { App, Duration, Stack } from 'aws-cdk-lib';
 import { Template } from 'aws-cdk-lib/assertions';
 import { BuildSpec, Project } from 'aws-cdk-lib/aws-codebuild';
@@ -14,6 +14,7 @@ describe('codebuild start build', () => {
   const codebuildArnRef = { 'Fn::GetAtt': ['ProjectC78D97AD', 'Arn'] };
   const codebuildAction = 'codebuild:StartBuild';
   const expr = ScheduleExpression.at(new Date(Date.UTC(1991, 2, 24, 0, 0, 0)));
+  const roleId = 'SchedulerRoleForTarget27bd47517CF0F8';
 
   beforeEach(() => {
     app = new App({ context: { '@aws-cdk/aws-iam:minimizePolicies': true } });
@@ -36,7 +37,7 @@ describe('codebuild start build', () => {
       Properties: {
         Target: {
           Arn: codebuildArnRef,
-          RoleArn: { 'Fn::GetAtt': ['SchedulerRoleForTarget1441a743A31888', 'Arn'] },
+          RoleArn: { 'Fn::GetAtt': [roleId, 'Arn'] },
           RetryPolicy: {},
         },
       },
@@ -52,7 +53,7 @@ describe('codebuild start build', () => {
           },
         ],
       },
-      Roles: [{ Ref: 'SchedulerRoleForTarget1441a743A31888' }],
+      Roles: [{ Ref: roleId }],
     });
 
     template.hasResourceProperties('AWS::IAM::Role', {
@@ -61,7 +62,23 @@ describe('codebuild start build', () => {
         Statement: [
           {
             Effect: 'Allow',
-            Condition: { StringEquals: { 'aws:SourceAccount': '123456789012' } },
+            Condition: {
+              StringEquals: {
+                'aws:SourceAccount': '123456789012',
+                'aws:SourceArn': {
+                  'Fn::Join': [
+                    '',
+                    [
+                      'arn:',
+                      {
+                        Ref: 'AWS::Partition',
+                      },
+                      ':scheduler:us-east-1:123456789012:schedule-group/default',
+                    ],
+                  ],
+                },
+              },
+            },
             Principal: {
               Service: 'scheduler.amazonaws.com',
             },
@@ -113,8 +130,72 @@ describe('codebuild start build', () => {
     });
   });
 
-  test('reuses IAM role and IAM policy for two schedules from the same account', () => {
+  test('reuses IAM role and IAM policy for two schedules with the same target from the same account', () => {
+    const codeBuildTarget = new CodeBuildStartBuild(codebuildProject, {});
+
+    new Schedule(stack, 'MyScheduleDummy1', {
+      schedule: expr,
+      target: codeBuildTarget,
+    });
+
+    new Schedule(stack, 'MyScheduleDummy2', {
+      schedule: expr,
+      target: codeBuildTarget,
+    });
+
+    const template = Template.fromStack(stack);
+
+    template.resourcePropertiesCountIs('AWS::IAM::Role', {
+      AssumeRolePolicyDocument: {
+        Version: '2012-10-17',
+        Statement: [
+          {
+            Effect: 'Allow',
+            Condition: {
+              StringEquals: {
+                'aws:SourceAccount': '123456789012',
+                'aws:SourceArn': {
+                  'Fn::Join': [
+                    '',
+                    [
+                      'arn:',
+                      {
+                        Ref: 'AWS::Partition',
+                      },
+                      ':scheduler:us-east-1:123456789012:schedule-group/default',
+                    ],
+                  ],
+                },
+              },
+            },
+            Principal: {
+              Service: 'scheduler.amazonaws.com',
+            },
+            Action: 'sts:AssumeRole',
+          },
+        ],
+      },
+    }, 1);
+
+    template.resourcePropertiesCountIs('AWS::IAM::Policy', {
+      PolicyDocument: {
+        Statement: [
+          {
+            Action: codebuildAction,
+            Effect: 'Allow',
+            Resource: codebuildArnRef,
+          },
+        ],
+      },
+      Roles: [{ Ref: roleId }],
+    }, 1);
+  });
+
+  test('creates IAM role and IAM policy for two schedules with the same target but different groups', () => {
     const codeBuildTarget = new CodeBuildStartBuild(codebuildProject, {});
+    const group = new Group(stack, 'Group', {
+      groupName: 'mygroup',
+    });
 
     new Schedule(stack, 'MyScheduleDummy1', {
       schedule: expr,
@@ -124,6 +205,7 @@ describe('codebuild start build', () => {
     new Schedule(stack, 'MyScheduleDummy2', {
       schedule: expr,
       target: codeBuildTarget,
+      group,
     });
 
     const template = Template.fromStack(stack);
@@ -134,7 +216,41 @@ describe('codebuild start build', () => {
         Statement: [
           {
             Effect: 'Allow',
-            Condition: { StringEquals: { 'aws:SourceAccount': '123456789012' } },
+            Condition: {
+              StringEquals: {
+                'aws:SourceAccount': '123456789012',
+                'aws:SourceArn': {
+                  'Fn::Join': [
+                    '',
+                    [
+                      'arn:',
+                      {
+                        Ref: 'AWS::Partition',
+                      },
+                      ':scheduler:us-east-1:123456789012:schedule-group/default',
+                    ],
+                  ],
+                },
+              },
+            },
+            Principal: {
+              Service: 'scheduler.amazonaws.com',
+            },
+            Action: 'sts:AssumeRole',
+          },
+          {
+            Effect: 'Allow',
+            Condition: {
+              StringEquals: {
+                'aws:SourceAccount': '123456789012',
+                'aws:SourceArn': {
+                  'Fn::GetAtt': [
+                    'GroupC77FDACD',
+                    'Arn',
+                  ],
+                },
+              },
+            },
             Principal: {
               Service: 'scheduler.amazonaws.com',
             },
@@ -154,7 +270,7 @@ describe('codebuild start build', () => {
           },
         ],
       },
-      Roles: [{ Ref: 'SchedulerRoleForTarget1441a743A31888' }],
+      Roles: [{ Ref: roleId }],
     }, 1);
   });
 
@@ -411,7 +527,7 @@ describe('codebuild start build', () => {
       Properties: {
         Target: {
           Arn: codebuildArnRef,
-          RoleArn: { 'Fn::GetAtt': ['SchedulerRoleForTarget1441a743A31888', 'Arn'] },
+          RoleArn: { 'Fn::GetAtt': [roleId, 'Arn'] },
           RetryPolicy: {
             MaximumEventAgeInSeconds: 10800,
             MaximumRetryAttempts: 5,