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

fix(scheduler-targets-alpha): add dlq policy to execution role instead of queue policy #32032

Merged
merged 4 commits into from
Nov 7, 2024
Merged

fix(scheduler-targets-alpha): add dlq policy to execution role instead of queue policy #32032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5eebbf8
fix dlq policy and update unit tests
gracelu0 Nov 5, 2024
c99e6ed
remove check for account since dlq can be in diff account
gracelu0 Nov 7, 2024
ea85ed4
add integ test
gracelu0 Nov 7, 2024
fe451ba
Merge branch 'main' into scheduler-dlq-policy-fix
gracelu0 Nov 7, 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
35 changes: 8 additions & 27 deletions packages/@aws-cdk/aws-scheduler-targets-alpha/lib/target.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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