From 344d916480f6facc437841033cd3d072ebc07010 Mon Sep 17 00:00:00 2001 From: Ian Hou <45278651+iankhou@users.noreply.github.com> Date: Thu, 23 Jan 2025 11:59:28 -0500 Subject: [PATCH] fix(rds): does not print all failed validations for DatabaseCluster props (#32841) ### Issue #32840 Closes #32840 ### Reason for this change When initializing a new RDS DB Cluster, the [current implementation](https://github.com/aws/aws-cdk/pull/32151/files#diff-49b4a9e1bf0b7db3ab71f4f08580da0cb2191d84605dc82a70c324bd122d5cf7R805-R828) fails on the first validation error, making it possible for the user to encounter another failure after fixing known validation issues. ### Description of changes Implemented a [validation function](https://github.com/aws/aws-cdk/pull/32841/files#diff-5d08d37e744e173239879212c59fd45cb9a279349f3dfb1c66923cb015ed3a3a) that collects all validation errors and presents them to the user. Used this function in RDS Database Cluster initialization. I will implement this separately for SQS Queue initialization as a POC for usability. There are several other places that can make use of this shared code, to show users all validation errors at once. Here's a non-exhaustive list: - [aws-ec2](https://github.com/aws/aws-cdk/blob/3e4f3773bfa48b75bf0adc7d53d46bbec7714a9e/packages/aws-cdk-lib/aws-ec2/lib/volume.ts#L672-L743) - [eventbridge-scheduler](https://github.com/aws/aws-cdk/blob/3e4f3773bfa48b75bf0adc7d53d46bbec7714a9e/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts#L324-L362) - [aws-fsx](https://github.com/aws/aws-cdk/blob/3e4f3773bfa48b75bf0adc7d53d46bbec7714a9e/packages/aws-cdk-lib/aws-fsx/lib/lustre-file-system.ts#L360-L380) ### Describe any new or updated permissions being added No permissions changes. ### Description of how you validated changes Added unit tests and modified existing unit tests. Screenshot 2025-01-16 at 14 51 47 ### 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-rds/lib/cluster.ts | 31 +------ .../lib/validate-database-cluster-props.ts | 54 +++++++++++ .../aws-cdk-lib/aws-rds/test/cluster.test.ts | 18 ++-- .../core/lib/helpers-internal/index.ts | 1 + .../helpers-internal/validate-all-props.ts | 42 +++++++++ .../validate-all-props.test.ts | 90 +++++++++++++++++++ 6 files changed, 199 insertions(+), 37 deletions(-) create mode 100644 packages/aws-cdk-lib/aws-rds/lib/validate-database-cluster-props.ts create mode 100644 packages/aws-cdk-lib/core/lib/helpers-internal/validate-all-props.ts create mode 100644 packages/aws-cdk-lib/core/test/helpers-internal/validate-all-props.test.ts diff --git a/packages/aws-cdk-lib/aws-rds/lib/cluster.ts b/packages/aws-cdk-lib/aws-rds/lib/cluster.ts index c7261601c8e6a..9870753a8017a 100644 --- a/packages/aws-cdk-lib/aws-rds/lib/cluster.ts +++ b/packages/aws-cdk-lib/aws-rds/lib/cluster.ts @@ -11,6 +11,7 @@ import { BackupProps, Credentials, InstanceProps, PerformanceInsightRetention, R import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy'; import { CfnDBCluster, CfnDBClusterProps, CfnDBInstance } from './rds.generated'; import { ISubnetGroup, SubnetGroup } from './subnet-group'; +import { validateDatabaseClusterProps } from './validate-database-cluster-props'; import * as cloudwatch from '../../aws-cloudwatch'; import * as ec2 from '../../aws-ec2'; import * as iam from '../../aws-iam'; @@ -830,36 +831,10 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase { }); } + validateDatabaseClusterProps(this, props); + const enablePerformanceInsights = props.enablePerformanceInsights || props.performanceInsightRetention !== undefined || props.performanceInsightEncryptionKey !== undefined; - if (enablePerformanceInsights && props.enablePerformanceInsights === false) { - throw new ValidationError('`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set', this); - } - - if (props.clusterScalabilityType === ClusterScalabilityType.LIMITLESS || props.clusterScailabilityType === ClusterScailabilityType.LIMITLESS) { - if (!props.enablePerformanceInsights) { - throw new ValidationError('Performance Insights must be enabled for Aurora Limitless Database.', this); - } - if (!props.performanceInsightRetention || props.performanceInsightRetention < PerformanceInsightRetention.MONTHS_1) { - throw new ValidationError('Performance Insights retention period must be set at least 31 days for Aurora Limitless Database.', this); - } - if (!props.monitoringInterval || !props.enableClusterLevelEnhancedMonitoring) { - throw new ValidationError('Cluster level enhanced monitoring must be set for Aurora Limitless Database. Please set \'monitoringInterval\' and enable \'enableClusterLevelEnhancedMonitoring\'.', this); - } - if (props.writer || props.readers) { - throw new ValidationError('Aurora Limitless Database does not support readers or writer instances.', this); - } - if (!props.engine.engineVersion?.fullVersion?.endsWith('limitless')) { - throw new ValidationError(`Aurora Limitless Database requires an engine version that supports it, got ${props.engine.engineVersion?.fullVersion}`, this); - } - if (props.storageType !== DBClusterStorageType.AURORA_IOPT1) { - throw new ValidationError(`Aurora Limitless Database requires I/O optimized storage type, got: ${props.storageType}`, this); - } - if (props.cloudwatchLogsExports === undefined || props.cloudwatchLogsExports.length === 0) { - throw new ValidationError('Aurora Limitless Database requires CloudWatch Logs exports to be set.', this); - } - } - this.performanceInsightsEnabled = enablePerformanceInsights; this.performanceInsightRetention = enablePerformanceInsights ? (props.performanceInsightRetention || PerformanceInsightRetention.DEFAULT) diff --git a/packages/aws-cdk-lib/aws-rds/lib/validate-database-cluster-props.ts b/packages/aws-cdk-lib/aws-rds/lib/validate-database-cluster-props.ts new file mode 100644 index 0000000000000..23b3896385a3e --- /dev/null +++ b/packages/aws-cdk-lib/aws-rds/lib/validate-database-cluster-props.ts @@ -0,0 +1,54 @@ +import { Construct } from 'constructs'; +import { ClusterScailabilityType, DatabaseCluster, DatabaseClusterProps, DBClusterStorageType } from './cluster'; +import { PerformanceInsightRetention } from './props'; +import { validateAllProps, ValidationRule } from '../../core/lib/helpers-internal'; + +const standardDatabaseRules: ValidationRule[] = [ + { + condition: (props) => props.enablePerformanceInsights === false && + (props.performanceInsightRetention !== undefined || props.performanceInsightEncryptionKey !== undefined), + message: () => '`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set', + + }, +]; + +const limitlessDatabaseRules: ValidationRule[] = [ + { + condition: (props) => !props.enablePerformanceInsights, + message: () => 'Performance Insights must be enabled for Aurora Limitless Database', + }, + { + condition: (props) => !props.performanceInsightRetention + || props.performanceInsightRetention < PerformanceInsightRetention.MONTHS_1, + message: () => 'Performance Insights retention period must be set to at least 31 days for Aurora Limitless Database', + }, + { + condition: (props) => !props.monitoringInterval || !props.enableClusterLevelEnhancedMonitoring, + message: () => 'Cluster level enhanced monitoring must be set for Aurora Limitless Database. Please set \'monitoringInterval\' and enable \'enableClusterLevelEnhancedMonitoring\'', + }, + { + condition: (props) => !!(props.writer || props.readers), + message: () => 'Aurora Limitless Database does not support reader or writer instances', + }, + { + condition: (props) => !props.engine.engineVersion?.fullVersion?.endsWith('limitless'), + message: (props) => `Aurora Limitless Database requires an engine version that supports it, got: ${props.engine.engineVersion?.fullVersion}`, + }, + { + condition: (props) => props.storageType !== DBClusterStorageType.AURORA_IOPT1, + message: (props) => `Aurora Limitless Database requires I/O optimized storage type, got: ${props.storageType}`, + }, + { + condition: (props) => props.cloudwatchLogsExports === undefined || props.cloudwatchLogsExports.length === 0, + message: () => 'Aurora Limitless Database requires CloudWatch Logs exports to be set', + }, +]; + +export function validateDatabaseClusterProps(scope: Construct, props: DatabaseClusterProps): void { + const isLimitlessCluster = props.clusterScailabilityType === ClusterScailabilityType.LIMITLESS; + const applicableRules = isLimitlessCluster + ? [...standardDatabaseRules, ...limitlessDatabaseRules] + : standardDatabaseRules; + + validateAllProps(scope, DatabaseCluster.name, props, applicableRules); +} diff --git a/packages/aws-cdk-lib/aws-rds/test/cluster.test.ts b/packages/aws-cdk-lib/aws-rds/test/cluster.test.ts index 9c340a6e41378..ce16e4fd5dfc2 100644 --- a/packages/aws-cdk-lib/aws-rds/test/cluster.test.ts +++ b/packages/aws-cdk-lib/aws-rds/test/cluster.test.ts @@ -268,7 +268,7 @@ describe('cluster new api', () => { storageType: DBClusterStorageType.AURORA_IOPT1, cloudwatchLogsExports: ['postgresql'], }); - }).toThrow('Performance Insights must be enabled for Aurora Limitless Database.'); + }).toThrow('DatabaseCluster initialization failed due to the following validation error(s):\n- Performance Insights must be enabled for Aurora Limitless Database\n- Performance Insights retention period must be set to at least 31 days for Aurora Limitless Database'); }); test('throw error for invalid performance insights retention period', () => { @@ -292,7 +292,7 @@ describe('cluster new api', () => { storageType: DBClusterStorageType.AURORA_IOPT1, cloudwatchLogsExports: ['postgresql'], }); - }).toThrow('Performance Insights retention period must be set at least 31 days for Aurora Limitless Database.'); + }).toThrow('DatabaseCluster initialization failed due to the following validation error(s):\n- Performance Insights retention period must be set to at least 31 days for Aurora Limitless Database'); }); test('throw error for not specifying monitoring interval', () => { @@ -316,7 +316,7 @@ describe('cluster new api', () => { storageType: DBClusterStorageType.AURORA_IOPT1, cloudwatchLogsExports: ['postgresql'], }); - }).toThrow('Cluster level enhanced monitoring must be set for Aurora Limitless Database. Please set \'monitoringInterval\' and enable \'enableClusterLevelEnhancedMonitoring\'.'); + }).toThrow('DatabaseCluster initialization failed due to the following validation error(s):\n- Cluster level enhanced monitoring must be set for Aurora Limitless Database. Please set \'monitoringInterval\' and enable \'enableClusterLevelEnhancedMonitoring\''); }); test.each([false, undefined])('throw error for configuring enhanced monitoring at the instance level', (enableClusterLevelEnhancedMonitoring) => { @@ -341,7 +341,7 @@ describe('cluster new api', () => { cloudwatchLogsExports: ['postgresql'], instances: 1, }); - }).toThrow('Cluster level enhanced monitoring must be set for Aurora Limitless Database. Please set \'monitoringInterval\' and enable \'enableClusterLevelEnhancedMonitoring\'.'); + }).toThrow('Cluster level enhanced monitoring must be set for Aurora Limitless Database. Please set \'monitoringInterval\' and enable \'enableClusterLevelEnhancedMonitoring\''); }); test('throw error for specifying writer instance', () => { @@ -366,7 +366,7 @@ describe('cluster new api', () => { cloudwatchLogsExports: ['postgresql'], writer: ClusterInstance.serverlessV2('writer'), }); - }).toThrow('Aurora Limitless Database does not support readers or writer instances.'); + }).toThrow('DatabaseCluster initialization failed due to the following validation error(s):\n- Aurora Limitless Database does not support reader or writer instances'); }); test.each([ @@ -395,7 +395,7 @@ describe('cluster new api', () => { storageType: DBClusterStorageType.AURORA_IOPT1, cloudwatchLogsExports: ['postgresql'], }); - }).toThrow(`Aurora Limitless Database requires an engine version that supports it, got ${engine.engineVersion?.fullVersion}`); + }).toThrow(`DatabaseCluster initialization failed due to the following validation error(s):\n- Aurora Limitless Database requires an engine version that supports it, got: ${engine.engineVersion?.fullVersion}`); }); test('throw error for invalid storage type', () => { @@ -443,7 +443,7 @@ describe('cluster new api', () => { storageType: DBClusterStorageType.AURORA_IOPT1, cloudwatchLogsExports, }); - }).toThrow('Aurora Limitless Database requires CloudWatch Logs exports to be set.'); + }).toThrow('DatabaseCluster initialization failed due to the following validation error(s):\n- Aurora Limitless Database requires CloudWatch Logs exports to be set'); }); }); @@ -2130,7 +2130,7 @@ describe('cluster', () => { enablePerformanceInsights: false, performanceInsightRetention: PerformanceInsightRetention.DEFAULT, }); - }).toThrow(/`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set/); + }).toThrow('DatabaseCluster initialization failed due to the following validation error(s):\n- `enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set'); }); test('throws if performanceInsightEncryptionKey is set but performance insights is disabled', () => { @@ -2142,7 +2142,7 @@ describe('cluster', () => { enablePerformanceInsights: false, performanceInsightRetention: PerformanceInsightRetention.DEFAULT, }); - }).toThrow(/`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set/); + }).toThrow('DatabaseCluster initialization failed due to the following validation error(s):\n- `enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set'); }); test('warn if performance insights is enabled at cluster level but disabled on writer and reader instances', () => { diff --git a/packages/aws-cdk-lib/core/lib/helpers-internal/index.ts b/packages/aws-cdk-lib/core/lib/helpers-internal/index.ts index 50003af38ab8f..5d85b9399403d 100644 --- a/packages/aws-cdk-lib/core/lib/helpers-internal/index.ts +++ b/packages/aws-cdk-lib/core/lib/helpers-internal/index.ts @@ -3,4 +3,5 @@ export * from './cfn-parse'; export { md5hash } from '../private/md5'; export * from './customize-roles'; export * from './string-specializer'; +export * from './validate-all-props'; export { constructInfoFromConstruct, constructInfoFromStack } from '../private/runtime-info'; diff --git a/packages/aws-cdk-lib/core/lib/helpers-internal/validate-all-props.ts b/packages/aws-cdk-lib/core/lib/helpers-internal/validate-all-props.ts new file mode 100644 index 0000000000000..59f580d12754d --- /dev/null +++ b/packages/aws-cdk-lib/core/lib/helpers-internal/validate-all-props.ts @@ -0,0 +1,42 @@ +import { Construct } from 'constructs'; +import { ValidationError } from '../errors'; + +/** + * Represents a validation rule for props of type T. + * @template T The type of the props being validated. + */ +export type ValidationRule = { + /** + * A function that checks if the validation rule condition is met. + * @param {T} props - The props object to validate. + * @returns {boolean} True if the condition is met (i.e., validation fails), false otherwise. + */ + condition: (props: T) => boolean; + + /** + * A function that returns an error message if the validation fails. + * @param {T} props - The props that failed validation. + * @returns {string} The error message. + */ + message: (props: T) => string; +}; + +/** + * Validates props against a set of rules and throws an error if any validations fail. + * + * @template T The type of the props being validated. + * @param {string} className - The name of the class being validated, used in the error message. Ex. for SQS, might be Queue.name + * @param {T} props - The props object to validate. + * @param {ValidationRule[]} rules - An array of validation rules to apply. + * @throws {Error} If any validation rules fail, with a message detailing all failures. + */ +export function validateAllProps(scope: Construct, className: string, props: T, rules: ValidationRule[]): void { + const validationErrors = rules + .filter(rule => rule.condition(props)) + .map(rule => rule.message(props)); + + if (validationErrors.length > 0) { + const errorMessage = `${className} initialization failed due to the following validation error(s):\n${validationErrors.map(error => `- ${error}`).join('\n')}`; + throw new ValidationError(errorMessage, scope); + } +} diff --git a/packages/aws-cdk-lib/core/test/helpers-internal/validate-all-props.test.ts b/packages/aws-cdk-lib/core/test/helpers-internal/validate-all-props.test.ts new file mode 100644 index 0000000000000..78948be563825 --- /dev/null +++ b/packages/aws-cdk-lib/core/test/helpers-internal/validate-all-props.test.ts @@ -0,0 +1,90 @@ +import { Construct } from 'constructs'; +import { ValidationError } from '../../lib/errors'; +import { validateAllProps, ValidationRule } from '../../lib/helpers-internal/validate-all-props'; + +class TestConstruct extends Construct { + constructor() { + super(undefined as any, 'TestConstruct'); + } +} + +describe('validateAllProps', () => { + let testScope: Construct; + + beforeEach(() => { + testScope = new TestConstruct(); + }); + + it('should not throw an error when all validations pass', () => { + const props = { value: 5 }; + const rules: ValidationRule[] = [ + { + condition: (p) => p.value < 0, + message: (p) => `Value ${p.value} should be non-negative`, + }, + ]; + + expect(() => validateAllProps(testScope, 'TestClass', props, rules)).not.toThrow(); + }); + + it('should throw a ValidationError when a validation fails', () => { + const props = { value: -5 }; + const rules: ValidationRule[] = [ + { + condition: (p) => p.value < 0, + message: (p) => `Value ${p.value} should be non-negative`, + }, + ]; + + expect(() => validateAllProps(testScope, 'TestClass', props, rules)).toThrow(ValidationError); + }); + + it('should include all failed validation messages in the error', () => { + const props = { value1: -5, value2: 15 }; + const rules: ValidationRule[] = [ + { + condition: (p) => p.value1 < 0, + message: (p) => `Value1 ${p.value1} should be non-negative`, + }, + { + condition: (p) => p.value2 > 10, + message: (p) => `Value2 ${p.value2} should be 10 or less`, + }, + ]; + + expect(() => validateAllProps(testScope, 'TestClass', props, rules)).toThrow(ValidationError); + try { + validateAllProps(testScope, 'TestClass', props, rules); + } catch (error) { + if (error instanceof ValidationError) { + expect(error.message).toBe( + 'TestClass initialization failed due to the following validation error(s):\n' + + '- Value1 -5 should be non-negative\n' + + '- Value2 15 should be 10 or less', + ); + } + } + }); + + it('should work with complex object structures', () => { + const props = { nested: { value: 'invalid' } }; + const rules: ValidationRule[] = [ + { + condition: (p) => p.nested.value !== 'valid', + message: (p) => `Nested value "${p.nested.value}" is not valid`, + }, + ]; + + expect(() => validateAllProps(testScope, 'TestClass', props, rules)).toThrow(ValidationError); + try { + validateAllProps(testScope, 'TestClass', props, rules); + } catch (error) { + if (error instanceof ValidationError) { + expect(error.message).toBe( + 'TestClass initialization failed due to the following validation error(s):\n' + + '- Nested value "invalid" is not valid', + ); + } + } + }); +});