From 7e9bd7d8419313c333b7a0fffdc489363046e4e2 Mon Sep 17 00:00:00 2001 From: Yohta Kimura <38206553+rajyan@users.noreply.github.com> Date: Tue, 30 Aug 2022 12:58:28 +0900 Subject: [PATCH] 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* --- .../lib/base/queue-processing-service-base.ts | 7 + .../ec2/queue-processing-ecs-service.test.ts | 123 ++++++++++++++++++ .../queue-processing-fargate-service.test.ts | 90 +++++++++++++ 3 files changed, 220 insertions(+) diff --git a/packages/@aws-cdk/aws-ecs-patterns/lib/base/queue-processing-service-base.ts b/packages/@aws-cdk/aws-ecs-patterns/lib/base/queue-processing-service-base.ts index 9b73999ee866f..9347a7649d87b 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/lib/base/queue-processing-service-base.ts +++ b/packages/@aws-cdk/aws-ecs-patterns/lib/base/queue-processing-service-base.ts @@ -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; @@ -106,6 +107,7 @@ 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; @@ -113,6 +115,7 @@ export interface QueueProcessingServiceBaseProps { /** * 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; @@ -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; diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/ec2/queue-processing-ecs-service.test.ts b/packages/@aws-cdk/aws-ecs-patterns/test/ec2/queue-processing-ecs-service.test.ts index df70409b293d2..4a9089ccb8c6c 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/ec2/queue-processing-ecs-service.test.ts +++ b/packages/@aws-cdk/aws-ecs-patterns/test/ec2/queue-processing-ecs-service.test.ts @@ -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'; @@ -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')); +}); diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts index 0164356061d61..baabcb914289e 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts @@ -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'; @@ -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')); +});