Skip to content

Commit

Permalink
feat(rds): throw ValidationError instead of untyped errors (#33042)
Browse files Browse the repository at this point in the history
### Issue

`aws-rds` for #32569

### Description of changes

ValidationErrors everywhere

### Describe any new or updated permissions being added

n/a

### Description of how you validated changes

Existing tests. Exemptions granted as this is basically a refactor of existing code.

### 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*

(cherry picked from commit 0b2db62)
  • Loading branch information
kaizencc authored and moelasmar committed Jan 24, 2025
1 parent f532a92 commit c3779cf
Show file tree
Hide file tree
Showing 11 changed files with 111 additions and 101 deletions.
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ baseConfig.rules['import/no-extraneous-dependencies'] = [


// no-throw-default-error
const modules = ['aws-s3', 'aws-lambda'];
const modules = ['aws-s3', 'aws-lambda', 'aws-rds'];
baseConfig.overrides.push({
files: modules.map(m => `./${m}/lib/**`),
rules: { "@cdklabs/no-throw-default-error": ['error'] },
Expand Down
5 changes: 3 additions & 2 deletions packages/aws-cdk-lib/aws-rds/lib/aurora-cluster-instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as ec2 from '../../aws-ec2';
import { IRole } from '../../aws-iam';
import * as kms from '../../aws-kms';
import { IResource, Resource, Duration, RemovalPolicy, ArnFormat, FeatureFlags } from '../../core';
import { ValidationError } from '../../core/lib/errors';
import { AURORA_CLUSTER_CHANGE_SCOPE_OF_INSTANCE_PARAMETER_GROUP_WITH_EACH_PARAMETERS } from '../../cx-api';

/**
Expand Down Expand Up @@ -476,7 +477,7 @@ class AuroraClusterInstance extends Resource implements IAuroraClusterInstance {
});
this.tier = props.promotionTier ?? 2;
if (this.tier > 15) {
throw new Error('promotionTier must be between 0-15');
throw new ValidationError('promotionTier must be between 0-15', this);
}

const isOwnedResource = Resource.isOwnedResource(props.cluster);
Expand All @@ -499,7 +500,7 @@ class AuroraClusterInstance extends Resource implements IAuroraClusterInstance {
const enablePerformanceInsights = props.enablePerformanceInsights
|| props.performanceInsightRetention !== undefined || props.performanceInsightEncryptionKey !== undefined;
if (enablePerformanceInsights && props.enablePerformanceInsights === false) {
throw new Error('`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set');
throw new ValidationError('`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set', this);
}

this.performanceInsightsEnabled = enablePerformanceInsights;
Expand Down
9 changes: 5 additions & 4 deletions packages/aws-cdk-lib/aws-rds/lib/cluster-engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { EngineVersion } from './engine-version';
import { IParameterGroup, ParameterGroup } from './parameter-group';
import * as iam from '../../aws-iam';
import * as secretsmanager from '../../aws-secretsmanager';
import { ValidationError } from '../../core/lib/errors';

/**
* The extra options passed to the `IClusterEngine.bindToCluster` method.
Expand Down Expand Up @@ -1179,19 +1180,19 @@ class AuroraPostgresClusterEngine extends ClusterEngineBase {
// skip validation for unversioned as it might be supported/unsupported. we cannot reliably tell at compile-time
if (this.engineVersion?.fullVersion) {
if (options.s3ImportRole && !(config.features?.s3Import)) {
throw new Error(`s3Import is not supported for Postgres version: ${this.engineVersion.fullVersion}. Use a version that supports the s3Import feature.`);
throw new ValidationError(`s3Import is not supported for Postgres version: ${this.engineVersion.fullVersion}. Use a version that supports the s3Import feature.`, scope);
}
if (options.s3ExportRole && !(config.features?.s3Export)) {
throw new Error(`s3Export is not supported for Postgres version: ${this.engineVersion.fullVersion}. Use a version that supports the s3Export feature.`);
throw new ValidationError(`s3Export is not supported for Postgres version: ${this.engineVersion.fullVersion}. Use a version that supports the s3Export feature.`, scope);
}
}
return config;
}

protected defaultParameterGroup(scope: Construct): IParameterGroup | undefined {
if (!this.parameterGroupFamily) {
throw new Error('Could not create a new ParameterGroup for an unversioned aurora-postgresql cluster engine. ' +
'Please either use a versioned engine, or pass an explicit ParameterGroup when creating the cluster');
throw new ValidationError('Could not create a new ParameterGroup for an unversioned aurora-postgresql cluster engine. ' +
'Please either use a versioned engine, or pass an explicit ParameterGroup when creating the cluster', scope);
}
return ParameterGroup.fromParameterGroupName(scope, 'AuroraPostgreSqlDatabaseClusterEngineDefaultParameterGroup',
`default.${this.parameterGroupFamily}`);
Expand Down
79 changes: 40 additions & 39 deletions packages/aws-cdk-lib/aws-rds/lib/cluster.ts

Large diffs are not rendered by default.

9 changes: 5 additions & 4 deletions packages/aws-cdk-lib/aws-rds/lib/instance-engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { EngineVersion } from './engine-version';
import { IOptionGroup, OptionGroup } from './option-group';
import * as iam from '../../aws-iam';
import * as secretsmanager from '../../aws-secretsmanager';
import { ValidationError } from '../../core/lib/errors';

/**
* The options passed to `IInstanceEngine.bind`.
Expand Down Expand Up @@ -142,9 +143,9 @@ abstract class InstanceEngineBase implements IInstanceEngine {
this.engineFamily = props.engineFamily;
}

public bindToInstance(_scope: Construct, options: InstanceEngineBindOptions): InstanceEngineConfig {
public bindToInstance(scope: Construct, options: InstanceEngineBindOptions): InstanceEngineConfig {
if (options.timezone && !this.supportsTimezone) {
throw new Error(`timezone property can not be configured for ${this.engineType}`);
throw new ValidationError(`timezone property can not be configured for ${this.engineType}`, scope);
}
return {
features: this.features,
Expand Down Expand Up @@ -621,7 +622,7 @@ class MariaDbInstanceEngine extends InstanceEngineBase {

public bindToInstance(scope: Construct, options: InstanceEngineBindOptions): InstanceEngineConfig {
if (options.domain) {
throw new Error(`domain property cannot be configured for ${this.engineType}`);
throw new ValidationError(`domain property cannot be configured for ${this.engineType}`, scope);
}
return super.bindToInstance(scope, options);
}
Expand Down Expand Up @@ -2828,7 +2829,7 @@ abstract class SqlServerInstanceEngineBase extends InstanceEngineBase {
const s3Role = options.s3ImportRole ?? options.s3ExportRole;
if (s3Role) {
if (options.s3ImportRole && options.s3ExportRole && options.s3ImportRole !== options.s3ExportRole) {
throw new Error('S3 import and export roles must be the same for SQL Server engines');
throw new ValidationError('S3 import and export roles must be the same for SQL Server engines', scope);
}

if (!optionGroup) {
Expand Down
35 changes: 18 additions & 17 deletions packages/aws-cdk-lib/aws-rds/lib/instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import * as logs from '../../aws-logs';
import * as s3 from '../../aws-s3';
import * as secretsmanager from '../../aws-secretsmanager';
import { ArnComponents, ArnFormat, Duration, FeatureFlags, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, Tokenization } from '../../core';
import { ValidationError } from '../../core/lib/errors';
import * as cxapi from '../../cx-api';

/**
Expand Down Expand Up @@ -180,15 +181,15 @@ export abstract class DatabaseInstanceBase extends Resource implements IDatabase

public grantConnect(grantee: iam.IGrantable, dbUser?: string): iam.Grant {
if (this.enableIamAuthentication === false) {
throw new Error('Cannot grant connect when IAM authentication is disabled');
throw new ValidationError('Cannot grant connect when IAM authentication is disabled', this);
}

if (!this.instanceResourceId) {
throw new Error('For imported Database Instances, instanceResourceId is required to grantConnect()');
throw new ValidationError('For imported Database Instances, instanceResourceId is required to grantConnect()', this);
}

if (!dbUser) {
throw new Error('For imported Database Instances, the dbUser is required to grantConnect()');
throw new ValidationError('For imported Database Instances, the dbUser is required to grantConnect()', this);
}

this.enableIamAuthentication = true;
Expand Down Expand Up @@ -784,12 +785,12 @@ abstract class DatabaseInstanceNew extends DatabaseInstanceBase implements IData

this.vpc = props.vpc;
if (props.vpcSubnets && props.vpcPlacement) {
throw new Error('Only one of `vpcSubnets` or `vpcPlacement` can be specified');
throw new ValidationError('Only one of `vpcSubnets` or `vpcPlacement` can be specified', this);
}
this.vpcPlacement = props.vpcSubnets ?? props.vpcPlacement;

if (props.multiAz === true && props.availabilityZone) {
throw new Error('Requesting a specific availability zone is not valid for Multi-AZ instances');
throw new ValidationError('Requesting a specific availability zone is not valid for Multi-AZ instances', this);
}

const subnetGroup = props.subnetGroup ?? new SubnetGroup(this, 'SubnetGroup', {
Expand Down Expand Up @@ -820,12 +821,12 @@ abstract class DatabaseInstanceNew extends DatabaseInstanceBase implements IData
const storageType = props.storageType ?? StorageType.GP2;
const iops = defaultIops(storageType, props.iops);
if (props.storageThroughput && storageType !== StorageType.GP3) {
throw new Error(`The storage throughput can only be specified with GP3 storage type. Got ${storageType}.`);
throw new ValidationError(`The storage throughput can only be specified with GP3 storage type. Got ${storageType}.`, this);
}
if (storageType === StorageType.GP3 && props.storageThroughput && iops
&& !Token.isUnresolved(props.storageThroughput) && !Token.isUnresolved(iops)
&& props.storageThroughput/iops > 0.25) {
throw new Error(`The maximum ratio of storage throughput to IOPS is 0.25. Got ${props.storageThroughput/iops}.`);
throw new ValidationError(`The maximum ratio of storage throughput to IOPS is 0.25. Got ${props.storageThroughput/iops}.`, this);
}

this.cloudwatchLogGroups = {};
Expand All @@ -837,7 +838,7 @@ abstract class DatabaseInstanceNew extends DatabaseInstanceBase implements IData
const enablePerformanceInsights = props.enablePerformanceInsights
|| props.performanceInsightRetention !== undefined || props.performanceInsightEncryptionKey !== undefined;
if (enablePerformanceInsights && props.enablePerformanceInsights === false) {
throw new Error('`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set');
throw new ValidationError('`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set', this);
}

if (props.domain) {
Expand Down Expand Up @@ -1019,13 +1020,13 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
const engineFeatures = engineConfig.features;
if (s3ImportRole) {
if (!engineFeatures?.s3Import) {
throw new Error(`Engine '${engineDescription(props.engine)}' does not support S3 import`);
throw new ValidationError(`Engine '${engineDescription(props.engine)}' does not support S3 import`, this);
}
instanceAssociatedRoles.push({ roleArn: s3ImportRole.roleArn, featureName: engineFeatures?.s3Import });
}
if (s3ExportRole) {
if (!engineFeatures?.s3Export) {
throw new Error(`Engine '${engineDescription(props.engine)}' does not support S3 export`);
throw new ValidationError(`Engine '${engineDescription(props.engine)}' does not support S3 export`, this);
}
// only add the export feature if it's different from the import feature
if (engineFeatures.s3Import !== engineFeatures?.s3Export) {
Expand All @@ -1036,7 +1037,7 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
this.instanceType = props.instanceType ?? ec2.InstanceType.of(ec2.InstanceClass.M5, ec2.InstanceSize.LARGE);

if (props.parameterGroup && props.parameters) {
throw new Error('You cannot specify both parameterGroup and parameters');
throw new ValidationError('You cannot specify both parameterGroup and parameters', this);
}

const dbParameterGroupName = props.parameters
Expand Down Expand Up @@ -1069,13 +1070,13 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
*/
public addRotationSingleUser(options: RotationSingleUserOptions = {}): secretsmanager.SecretRotation {
if (!this.secret) {
throw new Error('Cannot add single user rotation for an instance without secret.');
throw new ValidationError('Cannot add single user rotation for an instance without secret.', this);
}

const id = 'RotationSingleUser';
const existing = this.node.tryFindChild(id);
if (existing) {
throw new Error('A single user rotation was already added to this instance.');
throw new ValidationError('A single user rotation was already added to this instance.', this);
}

return new secretsmanager.SecretRotation(this, id, {
Expand All @@ -1092,7 +1093,7 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
*/
public addRotationMultiUser(id: string, options: RotationMultiUserOptions): secretsmanager.SecretRotation {
if (!this.secret) {
throw new Error('Cannot add multi user rotation for an instance without secret.');
throw new ValidationError('Cannot add multi user rotation for an instance without secret.', this);
}

return new secretsmanager.SecretRotation(this, id, {
Expand All @@ -1115,7 +1116,7 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
public grantConnect(grantee: iam.IGrantable, dbUser?: string): iam.Grant {
if (!dbUser) {
if (!this.secret) {
throw new Error('A secret or dbUser is required to grantConnect()');
throw new ValidationError('A secret or dbUser is required to grantConnect()', this);
}

dbUser = this.secret.secretValueFromJson('username').unsafeUnwrap();
Expand Down Expand Up @@ -1248,7 +1249,7 @@ export class DatabaseInstanceFromSnapshot extends DatabaseInstanceSource impleme
let secret = credentials?.secret;
if (!secret && credentials?.generatePassword) {
if (!credentials.username) {
throw new Error('`credentials` `username` must be specified when `generatePassword` is set to true');
throw new ValidationError('`credentials` `username` must be specified when `generatePassword` is set to true', this);
}

secret = new DatabaseSecret(this, 'Secret', {
Expand Down Expand Up @@ -1351,7 +1352,7 @@ export class DatabaseInstanceReadReplica extends DatabaseInstanceNew implements
if (props.sourceDatabaseInstance.engine
&& !props.sourceDatabaseInstance.engine.supportsReadReplicaBackups
&& props.backupRetention) {
throw new Error(`Cannot set 'backupRetention', as engine '${engineDescription(props.sourceDatabaseInstance.engine)}' does not support automatic backups for read replicas`);
throw new ValidationError(`Cannot set 'backupRetention', as engine '${engineDescription(props.sourceDatabaseInstance.engine)}' does not support automatic backups for read replicas`, this);
}

// The read replica instance always uses the same engine as the source instance
Expand Down
5 changes: 3 additions & 2 deletions packages/aws-cdk-lib/aws-rds/lib/option-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { IInstanceEngine } from './instance-engine';
import { CfnOptionGroup } from './rds.generated';
import * as ec2 from '../../aws-ec2';
import { IResource, Lazy, Resource } from '../../core';
import { ValidationError } from '../../core/lib/errors';

/**
* An option group
Expand Down Expand Up @@ -126,7 +127,7 @@ export class OptionGroup extends Resource implements IOptionGroup {

const majorEngineVersion = props.engine.engineVersion?.majorVersion;
if (!majorEngineVersion) {
throw new Error("OptionGroup cannot be used with an engine that doesn't specify a version");
throw new ValidationError("OptionGroup cannot be used with an engine that doesn't specify a version", this);
}

props.configurations.forEach(config => this.addConfiguration(config));
Expand All @@ -146,7 +147,7 @@ export class OptionGroup extends Resource implements IOptionGroup {

if (configuration.port) {
if (!configuration.vpc) {
throw new Error('`port` and `vpc` must be specified together.');
throw new ValidationError('`port` and `vpc` must be specified together.', this);
}

const securityGroups = configuration.securityGroups && configuration.securityGroups.length > 0
Expand Down
3 changes: 2 additions & 1 deletion packages/aws-cdk-lib/aws-rds/lib/parameter-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Construct } from 'constructs';
import { IEngine } from './engine';
import { CfnDBClusterParameterGroup, CfnDBParameterGroup } from './rds.generated';
import { IResource, Lazy, RemovalPolicy, Resource } from '../../core';
import { ValidationError } from '../../core/lib/errors';

/**
* Options for `IParameterGroup.bindToCluster`.
Expand Down Expand Up @@ -143,7 +144,7 @@ export class ParameterGroup extends Resource implements IParameterGroup {

const family = props.engine.parameterGroupFamily;
if (!family) {
throw new Error("ParameterGroup cannot be used with an engine that doesn't specify a version");
throw new ValidationError("ParameterGroup cannot be used with an engine that doesn't specify a version", this);
}
this.family = family;
this.description = props.description;
Expand Down
7 changes: 4 additions & 3 deletions packages/aws-cdk-lib/aws-rds/lib/private/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as ec2 from '../../../aws-ec2';
import * as iam from '../../../aws-iam';
import * as s3 from '../../../aws-s3';
import { RemovalPolicy } from '../../../core';
import { ValidationError } from '../../../core/lib/errors';
import { DatabaseSecret } from '../database-secret';
import { IEngine } from '../engine';
import { CommonRotationUserOptions, Credentials, SnapshotCredentials } from '../props';
Expand Down Expand Up @@ -43,7 +44,7 @@ export function setupS3ImportExport(

if (props.s3ImportBuckets && props.s3ImportBuckets.length > 0) {
if (props.s3ImportRole) {
throw new Error('Only one of s3ImportRole or s3ImportBuckets must be specified, not both.');
throw new ValidationError('Only one of s3ImportRole or s3ImportBuckets must be specified, not both.', scope);
}

s3ImportRole = (combineRoles && s3ExportRole) ? s3ExportRole : new iam.Role(scope, 'S3ImportRole', {
Expand All @@ -56,7 +57,7 @@ export function setupS3ImportExport(

if (props.s3ExportBuckets && props.s3ExportBuckets.length > 0) {
if (props.s3ExportRole) {
throw new Error('Only one of s3ExportRole or s3ExportBuckets must be specified, not both.');
throw new ValidationError('Only one of s3ExportRole or s3ExportBuckets must be specified, not both.', scope);
}

s3ExportRole = (combineRoles && s3ImportRole) ? s3ImportRole : new iam.Role(scope, 'S3ExportRole', {
Expand Down Expand Up @@ -117,7 +118,7 @@ export function renderSnapshotCredentials(scope: Construct, credentials?: Snapsh
let secret = renderedCredentials?.secret;
if (!secret && renderedCredentials?.generatePassword) {
if (!renderedCredentials.username) {
throw new Error('`snapshotCredentials` `username` must be specified when `generatePassword` is set to true');
throw new ValidationError('`snapshotCredentials` `username` must be specified when `generatePassword` is set to true', scope);
}

renderedCredentials = SnapshotCredentials.fromSecret(
Expand Down
Loading

0 comments on commit c3779cf

Please sign in to comment.