Skip to content

Commit

Permalink
fix(ecs-patterns): add validation for queue and queue related props (a…
Browse files Browse the repository at this point in the history 
…ws#21717)

## Problem

When queue construct is set, queue related props (maxReceiveCount, visibilityTimeout, retentionPeriod) have no effect to the queue of the `QueueProcessingService`. I think this is a reasonable behavior, but it is difficult to notice when wrongly configured.

## Fix

This pull request adds a validation to prevent from setting both queue and queue related props at the same time, and notice users the configuration error. Also, updates the param docs for this behavior.

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.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
@rajyan @josephedward
rajyan authored and josephedward committed Aug 30, 2022
1 parent eeaab22 commit 71c4b0c
Show file tree
Hide file tree
Showing 3 changed files with 220 additions and 0 deletions.
7 changes: 7 additions & 0 deletions packages/@aws-cdk/aws-ecs-patterns/lib/base/queue-processing-service-base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export interface QueueProcessingServiceBaseProps {
* The maximum number of times that a message can be received by consumers.
* When this value is exceeded for a message the message will be automatically sent to the Dead Letter Queue.
*
* If the queue construct is specified, maxReceiveCount should be omitted.
* @default 3
*/
readonly maxReceiveCount?: number;
Expand All @@ -106,13 +107,15 @@ export interface QueueProcessingServiceBaseProps {
* Timeout of processing a single message. After dequeuing, the processor has this much time to handle the message and delete it from the queue
* before it becomes visible again for dequeueing by another processor. Values must be between 0 and (12 hours).
*
* If the queue construct is specified, visibilityTimeout should be omitted.
* @default Duration.seconds(30)
*/
readonly visibilityTimeout?: Duration;

/**
* The number of seconds that Dead Letter Queue retains a message.
*
* If the queue construct is specified, retentionPeriod should be omitted.
* @default Duration.days(14)
*/
readonly retentionPeriod?: Duration;
Expand Down Expand Up @@ -288,6 +291,10 @@ export abstract class QueueProcessingServiceBase extends Construct {
}
this.cluster = props.cluster || this.getDefaultCluster(this, props.vpc);

if (props.queue && (props.retentionPeriod || props.visibilityTimeout || props.maxReceiveCount)) {
const errorProps = ['retentionPeriod', 'visibilityTimeout', 'maxReceiveCount'].filter(prop => props.hasOwnProperty(prop));
throw new Error(`${errorProps.join(', ')} can be set only when queue is not set. Specify them in the QueueProps of the queue`);
}
// Create the SQS queue and it's corresponding DLQ if one is not provided
if (props.queue) {
this.sqsQueue = props.queue;
Expand Down
123 changes: 123 additions & 0 deletions packages/@aws-cdk/aws-ecs-patterns/test/ec2/queue-processing-ecs-service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as ec2 from '@aws-cdk/aws-ec2';
import { AsgCapacityProvider } from '@aws-cdk/aws-ecs';
import * as ecs from '@aws-cdk/aws-ecs';
import * as sqs from '@aws-cdk/aws-sqs';
import { Queue } from '@aws-cdk/aws-sqs';
import { testDeprecated, testLegacyBehavior } from '@aws-cdk/cdk-build-tools';
import * as cdk from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
Expand Down Expand Up @@ -505,3 +506,125 @@ test('can set capacity provider strategies', () => {
],
});
});

it('can set queue props by queue construct', () => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });
cluster.addAsgCapacityProvider(new AsgCapacityProvider(stack, 'DefaultAutoScalingGroupProvider', {
autoScalingGroup: new AutoScalingGroup(stack, 'DefaultAutoScalingGroup', {
vpc,
instanceType: new ec2.InstanceType('t2.micro'),
machineImage: MachineImage.latestAmazonLinux(),
}),
}));
const queue = new Queue(stack, 'Queue', {
queueName: 'custom-queue',
visibilityTimeout: cdk.Duration.seconds(200),
deadLetterQueue: {
queue: new Queue(stack, 'DeadLetterQueue', {
queueName: 'custom-dead-letter-queue',
retentionPeriod: cdk.Duration.seconds(100),
}),
maxReceiveCount: 10,
},
});

// WHEN
new ecsPatterns.QueueProcessingEc2Service(stack, 'Service', {
cluster: cluster,
memoryLimitMiB: 512,
image: ecs.ContainerImage.fromRegistry('test'),
queue: queue,
});

// Queue
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
QueueName: 'custom-queue',
VisibilityTimeout: 200,
RedrivePolicy: {
maxReceiveCount: 10,
},
});
// DLQ
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
QueueName: 'custom-dead-letter-queue',
MessageRetentionPeriod: 100,
});
});

it('can set queue props by QueueProcessingServiceBaseProps', () => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });
cluster.addAsgCapacityProvider(new AsgCapacityProvider(stack, 'DefaultAutoScalingGroupProvider', {
autoScalingGroup: new AutoScalingGroup(stack, 'DefaultAutoScalingGroup', {
vpc,
instanceType: new ec2.InstanceType('t2.micro'),
machineImage: MachineImage.latestAmazonLinux(),
}),
}));

// WHEN
new ecsPatterns.QueueProcessingEc2Service(stack, 'Service', {
cluster: cluster,
memoryLimitMiB: 512,
image: ecs.ContainerImage.fromRegistry('test'),
retentionPeriod: cdk.Duration.seconds(100),
visibilityTimeout: cdk.Duration.seconds(200),
maxReceiveCount: 10,
});

// Queue
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
QueueName: Match.absent(),
VisibilityTimeout: 200,
RedrivePolicy: {
maxReceiveCount: 10,
},
});
// DLQ
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
QueueName: Match.absent(),
MessageRetentionPeriod: 100,
});
});

it('throws validation errors of the specific queue prop, when setting queue and queue related props at same time', () => {
// GIVEN
const stack = new cdk.Stack();
const queue = new Queue(stack, 'Queue');
const vpc = new ec2.Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });
cluster.addAsgCapacityProvider(new AsgCapacityProvider(stack, 'DefaultAutoScalingGroupProvider', {
autoScalingGroup: new AutoScalingGroup(stack, 'DefaultAutoScalingGroup', {
vpc,
instanceType: new ec2.InstanceType('t2.micro'),
machineImage: MachineImage.latestAmazonLinux(),
}),
}));

// Setting all retentionPeriod, visibilityTimeout and maxReceiveCount
expect(() => {
new ecsPatterns.QueueProcessingEc2Service(stack, 'Service1', {
cluster: cluster,
memoryLimitMiB: 512,
image: ecs.ContainerImage.fromRegistry('test'),
queue: queue,
retentionPeriod: cdk.Duration.seconds(100),
visibilityTimeout: cdk.Duration.seconds(200),
maxReceiveCount: 10,
});
}).toThrow(new Error('retentionPeriod, visibilityTimeout, maxReceiveCount can be set only when queue is not set. Specify them in the QueueProps of the queue'));

// Setting only visibilityTimeout
expect(() => {
new ecsPatterns.QueueProcessingFargateService(stack, 'Service2', {
image: ecs.ContainerImage.fromRegistry('test'),
queue: queue,
visibilityTimeout: cdk.Duration.seconds(200),
});
}).toThrow(new Error('visibilityTimeout can be set only when queue is not set. Specify them in the QueueProps of the queue'));
});
90 changes: 90 additions & 0 deletions packages/@aws-cdk/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { MachineImage } from '@aws-cdk/aws-ec2';
import * as ecs from '@aws-cdk/aws-ecs';
import { AsgCapacityProvider } from '@aws-cdk/aws-ecs';
import * as sqs from '@aws-cdk/aws-sqs';
import { Queue } from '@aws-cdk/aws-sqs';
import { testDeprecated, testFutureBehavior } from '@aws-cdk/cdk-build-tools';
import * as cdk from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
Expand Down Expand Up @@ -653,3 +654,92 @@ test('can set capacity provider strategies', () => {
],
});
});

it('can set queue props by queue construct', () => {
// GIVEN
const stack = new cdk.Stack();
const queue = new Queue(stack, 'Queue', {
queueName: 'custom-queue',
visibilityTimeout: cdk.Duration.seconds(200),
deadLetterQueue: {
queue: new Queue(stack, 'DeadLetterQueue', {
queueName: 'custom-dead-letter-queue',
retentionPeriod: cdk.Duration.seconds(100),
}),
maxReceiveCount: 10,
},
});

// WHEN
new ecsPatterns.QueueProcessingFargateService(stack, 'Service', {
image: ecs.ContainerImage.fromRegistry('test'),
queue: queue,
});

// Queue
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
QueueName: 'custom-queue',
VisibilityTimeout: 200,
RedrivePolicy: {
maxReceiveCount: 10,
},
});
// DLQ
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
QueueName: 'custom-dead-letter-queue',
MessageRetentionPeriod: 100,
});
});

it('can set queue props by QueueProcessingServiceBaseProps', () => {
// GIVEN
const stack = new cdk.Stack();

// WHEN
new ecsPatterns.QueueProcessingFargateService(stack, 'Service', {
image: ecs.ContainerImage.fromRegistry('test'),
retentionPeriod: cdk.Duration.seconds(100),
visibilityTimeout: cdk.Duration.seconds(200),
maxReceiveCount: 10,
});

// Queue
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
QueueName: Match.absent(),
VisibilityTimeout: 200,
RedrivePolicy: {
maxReceiveCount: 10,
},
});
// DLQ
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
QueueName: Match.absent(),
MessageRetentionPeriod: 100,
});
});

it('throws validation errors of the specific queue prop, when setting queue and queue related props at same time', () => {
// GIVEN
const stack = new cdk.Stack();
const queue = new Queue(stack, 'Queue');

// Setting all retentionPeriod, visibilityTimeout and maxReceiveCount
expect(() => {
new ecsPatterns.QueueProcessingFargateService(stack, 'Service1', {
image: ecs.ContainerImage.fromRegistry('test'),
queue: queue,
retentionPeriod: cdk.Duration.seconds(100),
visibilityTimeout: cdk.Duration.seconds(200),
maxReceiveCount: 10,
});
}).toThrow(new Error('retentionPeriod, visibilityTimeout, maxReceiveCount can be set only when queue is not set. Specify them in the QueueProps of the queue'));

// Setting only visibilityTimeout
expect(() => {
new ecsPatterns.QueueProcessingFargateService(stack, 'Service2', {
image: ecs.ContainerImage.fromRegistry('test'),
queue: queue,
visibilityTimeout: cdk.Duration.seconds(200),
});
}).toThrow(new Error('visibilityTimeout can be set only when queue is not set. Specify them in the QueueProps of the queue'));
});

0 comments on commit 71c4b0c

Please sign in to comment.