Skip to content

Commit 7e9bd7d

Browse files
rajyanrajyan
authored
fix(ecs-patterns): add validation for queue and queue related props (#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*
1 parent 489d130 commit 7e9bd7d

File tree

3 files changed

+220
-0
lines changed

3 files changed

+220
-0
lines changed

‎packages/@aws-cdk/aws-ecs-patterns/lib/base/queue-processing-service-base.ts

+7
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ export interface QueueProcessingServiceBaseProps {
9898
* The maximum number of times that a message can be received by consumers.
9999
* When this value is exceeded for a message the message will be automatically sent to the Dead Letter Queue.
100100
*
101+
* If the queue construct is specified, maxReceiveCount should be omitted.
101102
* @default 3
102103
*/
103104
readonly maxReceiveCount?: number;
@@ -106,13 +107,15 @@ export interface QueueProcessingServiceBaseProps {
106107
* Timeout of processing a single message. After dequeuing, the processor has this much time to handle the message and delete it from the queue
107108
* before it becomes visible again for dequeueing by another processor. Values must be between 0 and (12 hours).
108109
*
110+
* If the queue construct is specified, visibilityTimeout should be omitted.
109111
* @default Duration.seconds(30)
110112
*/
111113
readonly visibilityTimeout?: Duration;
112114

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

294+
if (props.queue && (props.retentionPeriod || props.visibilityTimeout || props.maxReceiveCount)) {
295+
const errorProps = ['retentionPeriod', 'visibilityTimeout', 'maxReceiveCount'].filter(prop => props.hasOwnProperty(prop));
296+
throw new Error(`${errorProps.join(', ')} can be set only when queue is not set. Specify them in the QueueProps of the queue`);
297+
}
291298
// Create the SQS queue and it's corresponding DLQ if one is not provided
292299
if (props.queue) {
293300
this.sqsQueue = props.queue;

‎packages/@aws-cdk/aws-ecs-patterns/test/ec2/queue-processing-ecs-service.test.ts

+123
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import * as ec2 from '@aws-cdk/aws-ec2';
66
import { AsgCapacityProvider } from '@aws-cdk/aws-ecs';
77
import * as ecs from '@aws-cdk/aws-ecs';
88
import * as sqs from '@aws-cdk/aws-sqs';
9+
import { Queue } from '@aws-cdk/aws-sqs';
910
import { testDeprecated, testLegacyBehavior } from '@aws-cdk/cdk-build-tools';
1011
import * as cdk from '@aws-cdk/core';
1112
import * as cxapi from '@aws-cdk/cx-api';
@@ -505,3 +506,125 @@ test('can set capacity provider strategies', () => {
505506
],
506507
});
507508
});
509+
510+
it('can set queue props by queue construct', () => {
511+
// GIVEN
512+
const stack = new cdk.Stack();
513+
const vpc = new ec2.Vpc(stack, 'VPC');
514+
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });
515+
cluster.addAsgCapacityProvider(new AsgCapacityProvider(stack, 'DefaultAutoScalingGroupProvider', {
516+
autoScalingGroup: new AutoScalingGroup(stack, 'DefaultAutoScalingGroup', {
517+
vpc,
518+
instanceType: new ec2.InstanceType('t2.micro'),
519+
machineImage: MachineImage.latestAmazonLinux(),
520+
}),
521+
}));
522+
const queue = new Queue(stack, 'Queue', {
523+
queueName: 'custom-queue',
524+
visibilityTimeout: cdk.Duration.seconds(200),
525+
deadLetterQueue: {
526+
queue: new Queue(stack, 'DeadLetterQueue', {
527+
queueName: 'custom-dead-letter-queue',
528+
retentionPeriod: cdk.Duration.seconds(100),
529+
}),
530+
maxReceiveCount: 10,
531+
},
532+
});
533+
534+
// WHEN
535+
new ecsPatterns.QueueProcessingEc2Service(stack, 'Service', {
536+
cluster: cluster,
537+
memoryLimitMiB: 512,
538+
image: ecs.ContainerImage.fromRegistry('test'),
539+
queue: queue,
540+
});
541+
542+
// Queue
543+
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
544+
QueueName: 'custom-queue',
545+
VisibilityTimeout: 200,
546+
RedrivePolicy: {
547+
maxReceiveCount: 10,
548+
},
549+
});
550+
// DLQ
551+
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
552+
QueueName: 'custom-dead-letter-queue',
553+
MessageRetentionPeriod: 100,
554+
});
555+
});
556+
557+
it('can set queue props by QueueProcessingServiceBaseProps', () => {
558+
// GIVEN
559+
const stack = new cdk.Stack();
560+
const vpc = new ec2.Vpc(stack, 'VPC');
561+
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });
562+
cluster.addAsgCapacityProvider(new AsgCapacityProvider(stack, 'DefaultAutoScalingGroupProvider', {
563+
autoScalingGroup: new AutoScalingGroup(stack, 'DefaultAutoScalingGroup', {
564+
vpc,
565+
instanceType: new ec2.InstanceType('t2.micro'),
566+
machineImage: MachineImage.latestAmazonLinux(),
567+
}),
568+
}));
569+
570+
// WHEN
571+
new ecsPatterns.QueueProcessingEc2Service(stack, 'Service', {
572+
cluster: cluster,
573+
memoryLimitMiB: 512,
574+
image: ecs.ContainerImage.fromRegistry('test'),
575+
retentionPeriod: cdk.Duration.seconds(100),
576+
visibilityTimeout: cdk.Duration.seconds(200),
577+
maxReceiveCount: 10,
578+
});
579+
580+
// Queue
581+
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
582+
QueueName: Match.absent(),
583+
VisibilityTimeout: 200,
584+
RedrivePolicy: {
585+
maxReceiveCount: 10,
586+
},
587+
});
588+
// DLQ
589+
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
590+
QueueName: Match.absent(),
591+
MessageRetentionPeriod: 100,
592+
});
593+
});
594+
595+
it('throws validation errors of the specific queue prop, when setting queue and queue related props at same time', () => {
596+
// GIVEN
597+
const stack = new cdk.Stack();
598+
const queue = new Queue(stack, 'Queue');
599+
const vpc = new ec2.Vpc(stack, 'VPC');
600+
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });
601+
cluster.addAsgCapacityProvider(new AsgCapacityProvider(stack, 'DefaultAutoScalingGroupProvider', {
602+
autoScalingGroup: new AutoScalingGroup(stack, 'DefaultAutoScalingGroup', {
603+
vpc,
604+
instanceType: new ec2.InstanceType('t2.micro'),
605+
machineImage: MachineImage.latestAmazonLinux(),
606+
}),
607+
}));
608+
609+
// Setting all retentionPeriod, visibilityTimeout and maxReceiveCount
610+
expect(() => {
611+
new ecsPatterns.QueueProcessingEc2Service(stack, 'Service1', {
612+
cluster: cluster,
613+
memoryLimitMiB: 512,
614+
image: ecs.ContainerImage.fromRegistry('test'),
615+
queue: queue,
616+
retentionPeriod: cdk.Duration.seconds(100),
617+
visibilityTimeout: cdk.Duration.seconds(200),
618+
maxReceiveCount: 10,
619+
});
620+
}).toThrow(new Error('retentionPeriod, visibilityTimeout, maxReceiveCount can be set only when queue is not set. Specify them in the QueueProps of the queue'));
621+
622+
// Setting only visibilityTimeout
623+
expect(() => {
624+
new ecsPatterns.QueueProcessingFargateService(stack, 'Service2', {
625+
image: ecs.ContainerImage.fromRegistry('test'),
626+
queue: queue,
627+
visibilityTimeout: cdk.Duration.seconds(200),
628+
});
629+
}).toThrow(new Error('visibilityTimeout can be set only when queue is not set. Specify them in the QueueProps of the queue'));
630+
});

‎packages/@aws-cdk/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts

+90
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { MachineImage } from '@aws-cdk/aws-ec2';
55
import * as ecs from '@aws-cdk/aws-ecs';
66
import { AsgCapacityProvider } from '@aws-cdk/aws-ecs';
77
import * as sqs from '@aws-cdk/aws-sqs';
8+
import { Queue } from '@aws-cdk/aws-sqs';
89
import { testDeprecated, testFutureBehavior } from '@aws-cdk/cdk-build-tools';
910
import * as cdk from '@aws-cdk/core';
1011
import * as cxapi from '@aws-cdk/cx-api';
@@ -653,3 +654,92 @@ test('can set capacity provider strategies', () => {
653654
],
654655
});
655656
});
657+
658+
it('can set queue props by queue construct', () => {
659+
// GIVEN
660+
const stack = new cdk.Stack();
661+
const queue = new Queue(stack, 'Queue', {
662+
queueName: 'custom-queue',
663+
visibilityTimeout: cdk.Duration.seconds(200),
664+
deadLetterQueue: {
665+
queue: new Queue(stack, 'DeadLetterQueue', {
666+
queueName: 'custom-dead-letter-queue',
667+
retentionPeriod: cdk.Duration.seconds(100),
668+
}),
669+
maxReceiveCount: 10,
670+
},
671+
});
672+
673+
// WHEN
674+
new ecsPatterns.QueueProcessingFargateService(stack, 'Service', {
675+
image: ecs.ContainerImage.fromRegistry('test'),
676+
queue: queue,
677+
});
678+
679+
// Queue
680+
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
681+
QueueName: 'custom-queue',
682+
VisibilityTimeout: 200,
683+
RedrivePolicy: {
684+
maxReceiveCount: 10,
685+
},
686+
});
687+
// DLQ
688+
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
689+
QueueName: 'custom-dead-letter-queue',
690+
MessageRetentionPeriod: 100,
691+
});
692+
});
693+
694+
it('can set queue props by QueueProcessingServiceBaseProps', () => {
695+
// GIVEN
696+
const stack = new cdk.Stack();
697+
698+
// WHEN
699+
new ecsPatterns.QueueProcessingFargateService(stack, 'Service', {
700+
image: ecs.ContainerImage.fromRegistry('test'),
701+
retentionPeriod: cdk.Duration.seconds(100),
702+
visibilityTimeout: cdk.Duration.seconds(200),
703+
maxReceiveCount: 10,
704+
});
705+
706+
// Queue
707+
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
708+
QueueName: Match.absent(),
709+
VisibilityTimeout: 200,
710+
RedrivePolicy: {
711+
maxReceiveCount: 10,
712+
},
713+
});
714+
// DLQ
715+
Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', {
716+
QueueName: Match.absent(),
717+
MessageRetentionPeriod: 100,
718+
});
719+
});
720+
721+
it('throws validation errors of the specific queue prop, when setting queue and queue related props at same time', () => {
722+
// GIVEN
723+
const stack = new cdk.Stack();
724+
const queue = new Queue(stack, 'Queue');
725+
726+
// Setting all retentionPeriod, visibilityTimeout and maxReceiveCount
727+
expect(() => {
728+
new ecsPatterns.QueueProcessingFargateService(stack, 'Service1', {
729+
image: ecs.ContainerImage.fromRegistry('test'),
730+
queue: queue,
731+
retentionPeriod: cdk.Duration.seconds(100),
732+
visibilityTimeout: cdk.Duration.seconds(200),
733+
maxReceiveCount: 10,
734+
});
735+
}).toThrow(new Error('retentionPeriod, visibilityTimeout, maxReceiveCount can be set only when queue is not set. Specify them in the QueueProps of the queue'));
736+
737+
// Setting only visibilityTimeout
738+
expect(() => {
739+
new ecsPatterns.QueueProcessingFargateService(stack, 'Service2', {
740+
image: ecs.ContainerImage.fromRegistry('test'),
741+
queue: queue,
742+
visibilityTimeout: cdk.Duration.seconds(200),
743+
});
744+
}).toThrow(new Error('visibilityTimeout can be set only when queue is not set. Specify them in the QueueProps of the queue'));
745+
});

0 commit comments

Comments
 (0)