fix dlq policy and update unit tests

aws · Nov 6, 2024 · 5eebbf8 · 5eebbf8
1 parent 5fca268
commit 5eebbf8
Show file tree

Hide file tree

Showing 12 changed files with 372 additions and 396 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, Stack, Token } from 'aws-cdk-lib';
+import { Annotations, Duration, 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';
@@ -81,7 +81,7 @@ export abstract class ScheduleTargetBase {
     this.addTargetActionToRole(_schedule, role);
 
     if (this.baseProps.deadLetterQueue) {
-      this.addToDeadLetterQueueResourcePolicy(_schedule, this.baseProps.deadLetterQueue);
+      this.addDeadLetterQueueActionToRole(_schedule, role, this.baseProps.deadLetterQueue);
     }
 
     return {
@@ -148,25 +148,14 @@ export abstract class ScheduleTargetBase {
   }
 
   /**
-   * Allow a schedule to send events with failed invocation to an Amazon SQS queue.
-   * @param schedule schedule to add DLQ to
-   * @param queue the DLQ
+   * Allow schedule to send events with failed invocation to an Amazon SQS queue.
    */
-  private addToDeadLetterQueueResourcePolicy(schedule: ISchedule, queue: sqs.IQueue) {
-    if (!sameEnvDimension(schedule.env.region, queue.env.region)) {
-      throw new Error(`Cannot assign Dead Letter Queue in region ${queue.env.region} to the schedule ${Names.nodeUniqueId(schedule.node)} in region ${schedule.env.region}. Both the queue and the schedule must be in the same region.`);
-    }
-
+  private addDeadLetterQueueActionToRole(schedule: ISchedule, role: iam.IRole, queue: sqs.IQueue) {
     // Skip Resource Policy creation if the Queue is not in the same account.
     // There is no way to add a target onto an imported schedule, so we can assume we will run the following code only
     // in the account where the schedule is created.
     if (sameEnvDimension(schedule.env.account, queue.env.account)) {
-      const policyStatementId = `AllowSchedule${Names.nodeUniqueId(schedule.node)}`;
-
-      queue.addToResourcePolicy(new iam.PolicyStatement({
-        sid: policyStatementId,
-        principals: [new iam.ServicePrincipal('scheduler.amazonaws.com')],
-        effect: iam.Effect.ALLOW,
+      role.addToPrincipalPolicy(new iam.PolicyStatement({
         actions: ['sqs:SendMessage'],
         resources: [queue.queueArn],
       }));

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,6 +1,6 @@
 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 { Annotations, Template } from 'aws-cdk-lib/assertions';
 import { BuildSpec, Project } from 'aws-cdk-lib/aws-codebuild';
 import { AccountRootPrincipal, Role } from 'aws-cdk-lib/aws-iam';
 import { Queue } from 'aws-cdk-lib/aws-sqs';
@@ -419,7 +419,7 @@ describe('codebuild start build', () => {
       })).toThrow(/Both the target and the execution role must be in the same account/);
   });
 
-  test('adds permissions to DLQ', () => {
+  test('adds permissions to execution role for sending messages to DLQ', () => {
     const dlq = new Queue(stack, 'DummyDeadLetterQueue');
 
     const codebuildProjectTarget = new CodeBuildStartBuild(codebuildProject, {
@@ -431,49 +431,30 @@ describe('codebuild start build', () => {
       target: codebuildProjectTarget,
     });
 
-    Template.fromStack(stack).hasResourceProperties('AWS::SQS::QueuePolicy', {
+    const template = Template.fromStack(stack);
+
+    template.hasResourceProperties('AWS::IAM::Policy', {
       PolicyDocument: {
         Statement: [
+          {
+            Action: codebuildAction,
+            Effect: 'Allow',
+            Resource: codebuildArnRef,
+          },
           {
             Action: 'sqs:SendMessage',
-            Principal: {
-              Service: 'scheduler.amazonaws.com',
-            },
             Effect: 'Allow',
             Resource: {
               'Fn::GetAtt': ['DummyDeadLetterQueueCEBF3463', 'Arn'],
             },
           },
         ],
       },
-      Queues: [
-        {
-          Ref: 'DummyDeadLetterQueueCEBF3463',
-        },
-      ],
-    });
-  });
-
-  test('throws when adding permissions to DLQ from a different region', () => {
-    const stack2 = new Stack(app, 'Stack2', {
-      env: {
-        region: 'eu-west-2',
-      },
-    });
-    const queue = new Queue(stack2, 'DummyDeadLetterQueue');
-
-    const codebuildProjectTarget = new CodeBuildStartBuild(codebuildProject, {
-      deadLetterQueue: queue,
+      Roles: [{ Ref: roleId }],
     });
-
-    expect(() =>
-      new Schedule(stack, 'MyScheduleDummy', {
-        schedule: expr,
-        target: codebuildProjectTarget,
-      })).toThrow(/Both the queue and the schedule must be in the same region./);
   });
 
-  test('does not create a queue policy when DLQ is imported', () => {
+  test('adds permission to execution role when imported DLQ is in same account', () => {
     const importedQueue = Queue.fromQueueArn(stack, 'ImportedQueue', 'arn:aws:sqs:us-east-1:123456789012:queue1');
 
     const codebuildProjectTarget = new CodeBuildStartBuild(codebuildProject, {
@@ -485,10 +466,26 @@ describe('codebuild start build', () => {
       target: codebuildProjectTarget,
     });
 
-    Template.fromStack(stack).resourceCountIs('AWS::SQS::QueuePolicy', 0);
+    Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
+      PolicyDocument: {
+        Statement: [
+          {
+            Action: codebuildAction,
+            Effect: 'Allow',
+            Resource: codebuildArnRef,
+          },
+          {
+            Action: 'sqs:SendMessage',
+            Effect: 'Allow',
+            Resource: importedQueue.queueArn,
+          },
+        ],
+      },
+      Roles: [{ Ref: roleId }],
+    });
   });
 
-  test('does not create a queue policy when DLQ is created in a different account', () => {
+  test('does not add IAM policy when DLQ is created in a different account', () => {
     const stack2 = new Stack(app, 'Stack2', {
       env: {
         region: 'us-east-1',
@@ -509,7 +506,7 @@ describe('codebuild start build', () => {
       target: codebuildProjectTarget,
     });
 
-    Template.fromStack(stack).resourceCountIs('AWS::SQS::QueuePolicy', 0);
+    Annotations.fromStack(stack).hasWarning('/Stack/MyScheduleDummy', 'Cannot add a resource policy to your dead letter queue associated with schedule undefined because the queue is in a different account. You must add the resource policy manually to the dead letter queue in account 234567890123.');
   });
 
   test('renders expected retry policy', () => {

diff --git a/...s/@aws-cdk/aws-scheduler-targets-alpha/test/codepipeline-start-pipeline-execution.test.ts b/...s/@aws-cdk/aws-scheduler-targets-alpha/test/codepipeline-start-pipeline-execution.test.ts
@@ -1,6 +1,6 @@
 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 { Annotations, Template } from 'aws-cdk-lib/assertions';
 import { Artifact, Pipeline } from 'aws-cdk-lib/aws-codepipeline';
 import { ManualApprovalAction, S3SourceAction } from 'aws-cdk-lib/aws-codepipeline-actions';
 import { AccountRootPrincipal, Role } from 'aws-cdk-lib/aws-iam';
@@ -439,7 +439,7 @@ describe('codepipeline start execution', () => {
       })).toThrow(/Both the target and the execution role must be in the same account/);
   });
 
-  test('adds permissions to DLQ', () => {
+  test('adds permissions to execution role for sending messages to DLQ', () => {
     const dlq = new sqs.Queue(stack, 'DummyDeadLetterQueue');
 
     const codepipelineTarget = new CodePipelineStartPipelineExecution(codepipeline, {
@@ -451,49 +451,28 @@ describe('codepipeline start execution', () => {
       target: codepipelineTarget,
     });
 
-    Template.fromStack(stack).hasResourceProperties('AWS::SQS::QueuePolicy', {
+    Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
       PolicyDocument: {
         Statement: [
+          {
+            Action: 'codepipeline:StartPipelineExecution',
+            Effect: 'Allow',
+            Resource: pipelineArn,
+          },
           {
             Action: 'sqs:SendMessage',
-            Principal: {
-              Service: 'scheduler.amazonaws.com',
-            },
             Effect: 'Allow',
             Resource: {
               'Fn::GetAtt': ['DummyDeadLetterQueueCEBF3463', 'Arn'],
             },
           },
         ],
       },
-      Queues: [
-        {
-          Ref: 'DummyDeadLetterQueueCEBF3463',
-        },
-      ],
-    });
-  });
-
-  test('throws when adding permissions to DLQ from a different region', () => {
-    const stack2 = new Stack(app, 'Stack2', {
-      env: {
-        region: 'eu-west-2',
-      },
-    });
-    const queue = new sqs.Queue(stack2, 'DummyDeadLetterQueue');
-
-    const codepipelineTarget = new CodePipelineStartPipelineExecution(codepipeline, {
-      deadLetterQueue: queue,
+      Roles: [{ Ref: roleId }],
     });
-
-    expect(() =>
-      new Schedule(stack, 'MyScheduleDummy', {
-        schedule: expr,
-        target: codepipelineTarget,
-      })).toThrow(/Both the queue and the schedule must be in the same region./);
   });
 
-  test('does not create a queue policy when DLQ is imported', () => {
+  test('adds permission to execution role when imported DLQ is in same account', () => {
     const importedQueue = sqs.Queue.fromQueueArn(stack, 'ImportedQueue', 'arn:aws:sqs:us-east-1:123456789012:queue1');
 
     const codepipelineTarget = new CodePipelineStartPipelineExecution(codepipeline, {
@@ -505,10 +484,26 @@ describe('codepipeline start execution', () => {
       target: codepipelineTarget,
     });
 
-    Template.fromStack(stack).resourceCountIs('AWS::SQS::QueuePolicy', 0);
+    Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
+      PolicyDocument: {
+        Statement: [
+          {
+            Action: 'codepipeline:StartPipelineExecution',
+            Effect: 'Allow',
+            Resource: pipelineArn,
+          },
+          {
+            Action: 'sqs:SendMessage',
+            Effect: 'Allow',
+            Resource: importedQueue.queueArn,
+          },
+        ],
+      },
+      Roles: [{ Ref: roleId }],
+    });
   });
 
-  test('does not create a queue policy when DLQ is created in a different account', () => {
+  test('does not add IAM policy when DLQ is created in a different account', () => {
     const stack2 = new Stack(app, 'Stack2', {
       env: {
         region: 'us-east-1',
@@ -529,7 +524,7 @@ describe('codepipeline start execution', () => {
       target: codepipelineTarget,
     });
 
-    Template.fromStack(stack).resourceCountIs('AWS::SQS::QueuePolicy', 0);
+    Annotations.fromStack(stack).hasWarning('/Stack/MyScheduleDummy', 'Cannot add a resource policy to your dead letter queue associated with schedule undefined because the queue is in a different account. You must add the resource policy manually to the dead letter queue in account 234567890123.');
   });
 
   test('renders expected retry policy', () => {

diff --git a/packages/@aws-cdk/aws-scheduler-targets-alpha/test/event-bridge-put-events.test.ts b/packages/@aws-cdk/aws-scheduler-targets-alpha/test/event-bridge-put-events.test.ts
@@ -1,6 +1,6 @@
 import { Group, Schedule, ScheduleExpression, ScheduleTargetInput } from '@aws-cdk/aws-scheduler-alpha';
 import { App, Duration, Stack } from 'aws-cdk-lib';
-import { Template } from 'aws-cdk-lib/assertions';
+import { Annotations, Template } from 'aws-cdk-lib/assertions';
 import * as events from 'aws-cdk-lib/aws-events';
 import { AccountRootPrincipal, Role } from 'aws-cdk-lib/aws-iam';
 import * as sqs from 'aws-cdk-lib/aws-sqs';
@@ -515,7 +515,7 @@ describe('eventBridge put events', () => {
       })).toThrow(/Both the target and the execution role must be in the same account/);
   });
 
-  test('adds permissions to DLQ', () => {
+  test('adds permissions to execution role for sending messages to DLQ', () => {
     const dlq = new sqs.Queue(stack, 'DummyDeadLetterQueue');
 
     const eventBusTarget = new EventBridgePutEvents(eventBusEventEntry, {
@@ -527,49 +527,33 @@ describe('eventBridge put events', () => {
       target: eventBusTarget,
     });
 
-    Template.fromStack(stack).hasResourceProperties('AWS::SQS::QueuePolicy', {
+    Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
       PolicyDocument: {
         Statement: [
           {
-            Action: 'sqs:SendMessage',
-            Principal: {
-              Service: 'scheduler.amazonaws.com',
+            Action: 'events:PutEvents',
+            Effect: 'Allow',
+            Resource: {
+              'Fn::GetAtt': [
+                'MyEventBus251E60F8',
+                'Arn',
+              ],
             },
+          },
+          {
+            Action: 'sqs:SendMessage',
             Effect: 'Allow',
             Resource: {
               'Fn::GetAtt': ['DummyDeadLetterQueueCEBF3463', 'Arn'],
             },
           },
         ],
       },
-      Queues: [
-        {
-          Ref: 'DummyDeadLetterQueueCEBF3463',
-        },
-      ],
+      Roles: [{ Ref: roleId }],
     });
   });
 
-  test('throws when adding permissions to DLQ from a different region', () => {
-    const stack2 = new Stack(app, 'Stack2', {
-      env: {
-        region: 'eu-west-2',
-      },
-    });
-    const queue = new sqs.Queue(stack2, 'DummyDeadLetterQueue');
-
-    const eventBusTarget = new EventBridgePutEvents(eventBusEventEntry, {
-      deadLetterQueue: queue,
-    });
-
-    expect(() =>
-      new Schedule(stack, 'MyScheduleDummy', {
-        schedule: expr,
-        target: eventBusTarget,
-      })).toThrow(/Both the queue and the schedule must be in the same region./);
-  });
-
-  test('does not create a queue policy when DLQ is imported', () => {
+  test('adds permission to execution role when imported DLQ is in same account', () => {
     const importedQueue = sqs.Queue.fromQueueArn(stack, 'ImportedQueue', 'arn:aws:sqs:us-east-1:123456789012:queue1');
 
     const eventBusTarget = new EventBridgePutEvents(eventBusEventEntry, {
@@ -581,10 +565,30 @@ describe('eventBridge put events', () => {
       target: eventBusTarget,
     });
 
-    Template.fromStack(stack).resourceCountIs('AWS::SQS::QueuePolicy', 0);
+    Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
+      PolicyDocument: {
+        Statement: [
+          {
+            Action: 'events:PutEvents',
+            Effect: 'Allow',
+            Resource: {
+              'Fn::GetAtt': [
+                'MyEventBus251E60F8',
+                'Arn',
+              ],
+            },
+          },
+          {
+            Action: 'sqs:SendMessage',
+            Effect: 'Allow',
+            Resource: importedQueue.queueArn,
+          },
+        ],
+      },
+      Roles: [{ Ref: roleId }],
+    });
   });
-
-  test('does not create a queue policy when DLQ is created in a different account', () => {
+  test('does not add IAM policy when DLQ is created in a different account', () => {
     const stack2 = new Stack(app, 'Stack2', {
       env: {
         region: 'us-east-1',
@@ -605,7 +609,7 @@ describe('eventBridge put events', () => {
       target: eventBusTarget,
     });
 
-    Template.fromStack(stack).resourceCountIs('AWS::SQS::QueuePolicy', 0);
+    Annotations.fromStack(stack).hasWarning('/Stack/MyScheduleDummy', 'Cannot add a resource policy to your dead letter queue associated with schedule undefined because the queue is in a different account. You must add the resource policy manually to the dead letter queue in account 234567890123.');
   });
 
   test('renders expected retry policy', () => {