From b77e9379e8d1e6d653042a2be35efb18983e1973 Mon Sep 17 00:00:00 2001 From: Ian Hou <45278651+iankhou@users.noreply.github.com> Date: Thu, 23 Jan 2025 15:47:33 -0500 Subject: [PATCH] fix(sqs): does not print all failed validations for Queue props (#33070) ### Issue #33098 Closes #33098. ### Reason for this change When initializing a new SQS Queue, props validation throws an error at the first validation issue encountered. If there are multiple validation issues, the user is only informed of the first one. ### Description of changes Using `validateAllProps` presents all validation errors to the user at once. If `redriveAllowPolicy` is enabled, the policy will also be evaluated in the same way. ### Describe any new or updated permissions being added No permissions changes. ### Description of how you validated changes Adjusted and added unit tests. Ran integration tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/aws-cdk-lib/aws-sqs/lib/queue.ts | 16 +--- .../aws-cdk-lib/aws-sqs/lib/validate-props.ts | 20 ----- .../aws-sqs/lib/validate-queue-props.ts | 61 +++++++++++++++ packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts | 75 ++++++++++++++++++- 4 files changed, 137 insertions(+), 35 deletions(-) delete mode 100644 packages/aws-cdk-lib/aws-sqs/lib/validate-props.ts create mode 100644 packages/aws-cdk-lib/aws-sqs/lib/validate-queue-props.ts diff --git a/packages/aws-cdk-lib/aws-sqs/lib/queue.ts b/packages/aws-cdk-lib/aws-sqs/lib/queue.ts index 8b3390994a47e..6a93e2895cfd1 100644 --- a/packages/aws-cdk-lib/aws-sqs/lib/queue.ts +++ b/packages/aws-cdk-lib/aws-sqs/lib/queue.ts @@ -1,7 +1,7 @@ import { Construct } from 'constructs'; import { IQueue, QueueAttributes, QueueBase, QueueEncryption } from './queue-base'; import { CfnQueue } from './sqs.generated'; -import { validateProps } from './validate-props'; +import { validateQueueProps, validateRedriveAllowPolicy } from './validate-queue-props'; import * as iam from '../../aws-iam'; import * as kms from '../../aws-kms'; import { Duration, RemovalPolicy, Stack, Token, ArnFormat, Annotations } from '../../core'; @@ -383,20 +383,10 @@ export class Queue extends QueueBase { physicalName: props.queueName, }); - validateProps(this, props); + validateQueueProps(this, props); if (props.redriveAllowPolicy) { - const { redrivePermission, sourceQueues } = props.redriveAllowPolicy; - if (redrivePermission === RedrivePermission.BY_QUEUE) { - if (!sourceQueues || sourceQueues.length === 0) { - throw new ValidationError('At least one source queue must be specified when RedrivePermission is set to \'byQueue\'', this); - } - if (sourceQueues && sourceQueues.length > 10) { - throw new ValidationError('Up to 10 sourceQueues can be specified. Set RedrivePermission to \'allowAll\' to specify more', this); - } - } else if (redrivePermission && sourceQueues) { - throw new ValidationError('sourceQueues cannot be configured when RedrivePermission is set to \'allowAll\' or \'denyAll\'', this); - } + validateRedriveAllowPolicy(this, props.redriveAllowPolicy); } const redrivePolicy = props.deadLetterQueue diff --git a/packages/aws-cdk-lib/aws-sqs/lib/validate-props.ts b/packages/aws-cdk-lib/aws-sqs/lib/validate-props.ts deleted file mode 100644 index c0c52dc514137..0000000000000 --- a/packages/aws-cdk-lib/aws-sqs/lib/validate-props.ts +++ /dev/null @@ -1,20 +0,0 @@ -import { Construct } from 'constructs'; -import { QueueProps } from './index'; -import { Token } from '../../core'; -import { ValidationError } from '../../core/lib/errors'; - -export function validateProps(scope: Construct, props: QueueProps) { - validateRange(scope, 'delivery delay', props.deliveryDelay && props.deliveryDelay.toSeconds(), 0, 900, 'seconds'); - validateRange(scope, 'maximum message size', props.maxMessageSizeBytes, 1_024, 262_144, 'bytes'); - validateRange(scope, 'message retention period', props.retentionPeriod && props.retentionPeriod.toSeconds(), 60, 1_209_600, 'seconds'); - validateRange(scope, 'receive wait time', props.receiveMessageWaitTime && props.receiveMessageWaitTime.toSeconds(), 0, 20, 'seconds'); - validateRange(scope, 'visibility timeout', props.visibilityTimeout && props.visibilityTimeout.toSeconds(), 0, 43_200, 'seconds'); - validateRange(scope, 'dead letter target maximum receive count', props.deadLetterQueue && props.deadLetterQueue.maxReceiveCount, 1, +Infinity); -} - -function validateRange(scope: Construct, label: string, value: number | undefined, minValue: number, maxValue: number, unit?: string) { - if (value === undefined || Token.isUnresolved(value)) { return; } - const unitSuffix = unit ? ` ${unit}` : ''; - if (value < minValue) { throw new ValidationError(`${label} must be ${minValue}${unitSuffix} or more, but ${value} was provided`, scope); } - if (value > maxValue) { throw new ValidationError(`${label} must be ${maxValue}${unitSuffix} or less, but ${value} was provided`, scope); } -} diff --git a/packages/aws-cdk-lib/aws-sqs/lib/validate-queue-props.ts b/packages/aws-cdk-lib/aws-sqs/lib/validate-queue-props.ts new file mode 100644 index 0000000000000..cd17f9d12d9aa --- /dev/null +++ b/packages/aws-cdk-lib/aws-sqs/lib/validate-queue-props.ts @@ -0,0 +1,61 @@ +import { Construct } from 'constructs'; +import { Queue, QueueProps, RedriveAllowPolicy, RedrivePermission } from './index'; +import { Token } from '../../core'; +import { validateAllProps, ValidationRule } from '../../core/lib/helpers-internal'; + +function validateRange(value: number | undefined, minValue: number, maxValue: number): boolean { + return value !== undefined && !Token.isUnresolved(value) && (value < minValue || value > maxValue); +} + +const queueValidationRules: ValidationRule[] = [ + { + condition: (props) => validateRange(props.deliveryDelay?.toSeconds(), 0, 900), + message: (props) => `delivery delay must be between 0 and 900 seconds, but ${props.deliveryDelay?.toSeconds()} was provided`, + }, + { + condition: (props) => validateRange(props.maxMessageSizeBytes, 1_024, 262_144), + message: (props) => `maximum message size must be between 1,024 and 262,144 bytes, but ${props.maxMessageSizeBytes} was provided`, + }, + { + condition: (props) => validateRange(props.retentionPeriod?.toSeconds(), 60, 1_209_600), + message: (props) => `message retention period must be between 60 and 1,209,600 seconds, but ${props.retentionPeriod?.toSeconds()} was provided`, + }, + { + condition: (props) => validateRange(props.receiveMessageWaitTime?.toSeconds(), 0, 20), + message: (props) => `receive wait time must be between 0 and 20 seconds, but ${props.receiveMessageWaitTime?.toSeconds()} was provided`, + }, + { + condition: (props) => validateRange(props.visibilityTimeout?.toSeconds(), 0, 43_200), + message: (props) => `visibility timeout must be between 0 and 43,200 seconds, but ${props.visibilityTimeout?.toSeconds()} was provided`, + }, + { + condition: (props) => validateRange(props.deadLetterQueue?.maxReceiveCount, 1, Number.MAX_SAFE_INTEGER), + message: (props) => `dead letter target maximum receive count must be 1 or more, but ${props.deadLetterQueue?.maxReceiveCount} was provided`, + }, +]; + +const redriveValidationRules: ValidationRule[] = [ + { + condition: ({ redrivePermission, sourceQueues }) => + redrivePermission === RedrivePermission.BY_QUEUE && (!sourceQueues || sourceQueues.length === 0), + message: () => 'At least one source queue must be specified when RedrivePermission is set to \'byQueue\'', + }, + { + condition: ({ redrivePermission, sourceQueues }) => + !!(redrivePermission === RedrivePermission.BY_QUEUE && sourceQueues && sourceQueues.length > 10), + message: () => 'Up to 10 sourceQueues can be specified. Set RedrivePermission to \'allowAll\' to specify more', + }, + { + condition: ({ redrivePermission, sourceQueues }) => + !!((redrivePermission === RedrivePermission.ALLOW_ALL || redrivePermission === RedrivePermission.DENY_ALL) && sourceQueues), + message: () => 'sourceQueues cannot be configured when RedrivePermission is set to \'allowAll\' or \'denyAll\'', + }, +]; + +export function validateQueueProps(scope: Construct, props: QueueProps) { + validateAllProps(scope, Queue.name, props, queueValidationRules); +} + +export function validateRedriveAllowPolicy(scope: Construct, policy: RedriveAllowPolicy) { + validateAllProps(scope, Queue.name, policy, redriveValidationRules); +} diff --git a/packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts b/packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts index 690eb0ec275be..9b1cb00e87cc1 100644 --- a/packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts +++ b/packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts @@ -3,6 +3,7 @@ import * as iam from '../../aws-iam'; import * as kms from '../../aws-kms'; import { CfnParameter, Duration, Stack, App, Token } from '../../core'; import * as sqs from '../lib'; +import { validateRedriveAllowPolicy } from '../lib/validate-queue-props'; /* eslint-disable quote-props */ @@ -62,6 +63,17 @@ test('with a dead letter queue', () => { expect(queue.deadLetterQueue).toEqual(dlqProps); }); +test('multiple prop validation errors are presented to the user (out-of-range retentionPeriod and deliveryDelay)', () => { + // GIVEN + const stack = new Stack(); + + // THEN + expect(() => new sqs.Queue(stack, 'MyQueue', { + retentionPeriod: Duration.seconds(30), + deliveryDelay: Duration.minutes(16), + })).toThrow('Queue initialization failed due to the following validation error(s):\n- delivery delay must be between 0 and 900 seconds, but 960 was provided\n- message retention period must be between 60 and 1,209,600 seconds, but 30 was provided'); +}); + test('message retention period must be between 1 minute to 14 days', () => { // GIVEN const stack = new Stack(); @@ -69,11 +81,11 @@ test('message retention period must be between 1 minute to 14 days', () => { // THEN expect(() => new sqs.Queue(stack, 'MyQueue', { retentionPeriod: Duration.seconds(30), - })).toThrow(/message retention period must be 60 seconds or more/); + })).toThrow('Queue initialization failed due to the following validation error(s):\n- message retention period must be between 60 and 1,209,600 seconds, but 30 was provided'); expect(() => new sqs.Queue(stack, 'AnotherQueue', { retentionPeriod: Duration.days(15), - })).toThrow(/message retention period must be 1209600 seconds or less/); + })).toThrow('Queue initialization failed due to the following validation error(s):\n- message retention period must be between 60 and 1,209,600 seconds, but 1296000 was provided'); }); test('message retention period can be provided as a parameter', () => { @@ -155,6 +167,65 @@ test('addToPolicy will automatically create a policy for this queue', () => { }); }); +describe('validateRedriveAllowPolicy', () => { + test('does not throw for valid policy', () => { + // GIVEN + const stack = new Stack(); + + // WHEN + const redriveAllowPolicy = { redrivePermission: sqs.RedrivePermission.ALLOW_ALL }; + + // THEN + expect(() => validateRedriveAllowPolicy(stack, redriveAllowPolicy)).not.toThrow(); + }); + + test('throws when sourceQueues is provided with ALLOW_ALL permission', () => { + // GIVEN + const stack = new Stack(); + + // WHEN + const sourceQueue = new sqs.Queue(stack, 'SourceQueue'); + const redriveAllowPolicy = { + redrivePermission: sqs.RedrivePermission.ALLOW_ALL, + sourceQueues: [sourceQueue], + }; + + // THEN + expect(() => validateRedriveAllowPolicy(stack, redriveAllowPolicy)) + .toThrow("Queue initialization failed due to the following validation error(s):\n- sourceQueues cannot be configured when RedrivePermission is set to 'allowAll' or 'denyAll'"); + }); + + test('throws when sourceQueues is not provided with BY_QUEUE permission', () => { + // GIVEN + const stack = new Stack(); + + // WHEN + const redriveAllowPolicy = { + redrivePermission: sqs.RedrivePermission.BY_QUEUE, + }; + + // THEN + expect(() => validateRedriveAllowPolicy(stack, redriveAllowPolicy)) + .toThrow("Queue initialization failed due to the following validation error(s):\n- At least one source queue must be specified when RedrivePermission is set to 'byQueue'"); + }); + + test('throws when more than 10 sourceQueues are provided', () => { + // GIVEN + const stack = new Stack(); + + // WHEN + const sourceQueues = Array(11).fill(null).map((_, i) => new sqs.Queue(stack, `SourceQueue${i}`)); + const redriveAllowPolicy = { + redrivePermission: sqs.RedrivePermission.BY_QUEUE, + sourceQueues, + }; + + // THEN + expect(() => validateRedriveAllowPolicy(stack, redriveAllowPolicy)) + .toThrow("Queue initialization failed due to the following validation error(s):\n- Up to 10 sourceQueues can be specified. Set RedrivePermission to 'allowAll' to specify more"); + }); +}); + describe('export and import', () => { test('importing works correctly', () => { // GIVEN