Skip to content

Commit

Permalink
fix(scheduler-targets-alpha): add dlq policy to execution role instea…
Browse files Browse the repository at this point in the history 
…d of queue policy (#32032)

### Issue # (if applicable)

Tracking #31785.

### Reason for this change

Currently if a dead letter queue (DLQ) is specified then a queue policy is created for the DLQ which allows the schedule to send messages. This is incorrect and the permissions should be added to the schedule's execution role instead.  

### Description of changes

Add `sqs:SendMessage` permission to execution role's policy statement if dead letter queue is specified. This follows the [service docs](https://docs.aws.amazon.com/scheduler/latest/UserGuide/configuring-schedule-dlq.html#configuring-schedule-dlq-permissions) for configuring a schedule DLQ.

Also removed cross-region validation as the deployment will fail fast for this case so the validation is unnecessary.

### Description of how you validated changes

Updated unit tests and added a new integration test with dead letter queue setup on the schedule

### 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*
  • Loading branch information
@gracelu0
gracelu0 authored Nov 7, 2024
1 parent 8a3734d commit b953b2a
Show file tree
Hide file tree
Showing 22 changed files with 37,252 additions and 617 deletions.
35 changes: 8 additions & 27 deletions packages/@aws-cdk/aws-scheduler-targets-alpha/lib/target.ts
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { ISchedule, ScheduleTargetConfig, ScheduleTargetInput } from '@aws-cdk/aws-scheduler-alpha';
import { Annotations, Duration, Names, PhysicalName, Stack, Token } from 'aws-cdk-lib';
import { 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';
import { md5hash } from 'aws-cdk-lib/core/lib/helpers-internal';
import { sameEnvDimension } from './util';

/**
* Base properties for a Schedule Target
Expand Down Expand Up @@ -81,7 +80,7 @@ export abstract class ScheduleTargetBase {
this.addTargetActionToRole(_schedule, role);

if (this.baseProps.deadLetterQueue) {
this.addToDeadLetterQueueResourcePolicy(_schedule, this.baseProps.deadLetterQueue);
this.addDeadLetterQueueActionToRole(role, this.baseProps.deadLetterQueue);
}

return {
Expand Down Expand Up @@ -148,31 +147,13 @@ 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.`);
}

// 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,
actions: ['sqs:SendMessage'],
resources: [queue.queueArn],
}));
} else {
Annotations.of(schedule).addWarning(`Cannot add a resource policy to your dead letter queue associated with schedule ${schedule.scheduleName} because the queue is in a different account. You must add the resource policy manually to the dead letter queue in account ${queue.env.account}.`);
}
private addDeadLetterQueueActionToRole(role: iam.IRole, queue: sqs.IQueue) {
role.addToPrincipalPolicy(new iam.PolicyStatement({
actions: ['sqs:SendMessage'],
resources: [queue.queueArn],
}));
}

private renderRetryPolicy(maximumEventAge: Duration | undefined, maximumRetryAttempts: number | undefined): CfnSchedule.RetryPolicyProperty {
Expand Down
79 changes: 26 additions & 53 deletions packages/@aws-cdk/aws-scheduler-targets-alpha/test/codebuild-start-build.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand All @@ -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, {
Expand All @@ -485,31 +466,23 @@ describe('codebuild start build', () => {
target: codebuildProjectTarget,
});

Template.fromStack(stack).resourceCountIs('AWS::SQS::QueuePolicy', 0);
});

test('does not create a queue policy when DLQ is created in a different account', () => {
const stack2 = new Stack(app, 'Stack2', {
env: {
region: 'us-east-1',
account: '234567890123',
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 }],
});

const queue = new Queue(stack2, 'DummyDeadLetterQueue', {
queueName: 'DummyDeadLetterQueue',
});

const codebuildProjectTarget = new CodeBuildStartBuild(codebuildProject, {
deadLetterQueue: queue,
});

new Schedule(stack, 'MyScheduleDummy', {
schedule: expr,
target: codebuildProjectTarget,
});

Template.fromStack(stack).resourceCountIs('AWS::SQS::QueuePolicy', 0);
});

test('renders expected retry policy', () => {
Expand Down
77 changes: 24 additions & 53 deletions ...s/@aws-cdk/aws-scheduler-targets-alpha/test/codepipeline-start-pipeline-execution.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand All @@ -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, {
Expand All @@ -505,31 +484,23 @@ describe('codepipeline start execution', () => {
target: codepipelineTarget,
});

Template.fromStack(stack).resourceCountIs('AWS::SQS::QueuePolicy', 0);
});

test('does not create a queue policy when DLQ is created in a different account', () => {
const stack2 = new Stack(app, 'Stack2', {
env: {
region: 'us-east-1',
account: '234567890123',
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 }],
});

const queue = new sqs.Queue(stack2, 'DummyDeadLetterQueue', {
queueName: 'DummyDeadLetterQueue',
});

const codepipelineTarget = new CodePipelineStartPipelineExecution(codepipeline, {
deadLetterQueue: queue,
});

new Schedule(stack, 'MyScheduleDummy', {
schedule: expr,
target: codepipelineTarget,
});

Template.fromStack(stack).resourceCountIs('AWS::SQS::QueuePolicy', 0);
});

test('renders expected retry policy', () => {
Expand Down
Loading

0 comments on commit b953b2a

Please sign in to comment.