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 1 commit
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
21 changes: 5 additions & 16 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,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';
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)) {
gracelu0 marked this conversation as resolved.
Show resolved Hide resolved
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],
}));
Expand Down
65 changes: 31 additions & 34 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
@@ -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';
Expand Down 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,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', () => {
gracelu0 marked this conversation as resolved.
Show resolved Hide resolved
const stack2 = new Stack(app, 'Stack2', {
env: {
region: 'us-east-1',
Expand All @@ -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', () => {
Expand Down
63 changes: 29 additions & 34 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
@@ -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';
Expand Down 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,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',
Expand All @@ -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', () => {
Expand Down
74 changes: 39 additions & 35 deletions packages/@aws-cdk/aws-scheduler-targets-alpha/test/event-bridge-put-events.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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, {
Expand All @@ -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, {
Expand All @@ -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',
Expand All @@ -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', () => {
Expand Down
Loading
Loading