Skip to content

Commit

Permalink
fix(sqs): does not print all failed validations for Queue props (#33070)
Browse files Browse the repository at this point in the history
### 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*
  • Loading branch information
iankhou authored Jan 23, 2025
1 parent c0ed449 commit b77e937
Show file tree
Hide file tree
Showing 4 changed files with 137 additions and 35 deletions.
16 changes: 3 additions & 13 deletions packages/aws-cdk-lib/aws-sqs/lib/queue.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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
Expand Down
20 changes: 0 additions & 20 deletions packages/aws-cdk-lib/aws-sqs/lib/validate-props.ts

This file was deleted.

61 changes: 61 additions & 0 deletions packages/aws-cdk-lib/aws-sqs/lib/validate-queue-props.ts
Original file line number Diff line number Diff line change
@@ -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<QueueProps>[] = [
{
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<RedriveAllowPolicy>[] = [
{
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);
}
75 changes: 73 additions & 2 deletions packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

Expand Down Expand Up @@ -62,18 +63,29 @@ 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();

// 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', () => {
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit b77e937

Please sign in to comment.