Skip to content

Commit

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

`aws-s3*` 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*
  • Loading branch information
kaizencc authored Jan 23, 2025
1 parent 7b9f6c8 commit aea8f3b
Show file tree
Hide file tree
Showing 9 changed files with 39 additions and 24 deletions.
7 changes: 7 additions & 0 deletions packages/aws-cdk-lib/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ const enableNoThrowDefaultErrorIn = [
'aws-ssmincidents',
'aws-ssmquicksetup',
'aws-synthetics',
'aws-s3-assets',
'aws-s3-deployment',
'aws-s3-notifications',
'aws-s3express',
'aws-s3objectlambda',
'aws-s3outposts',
'aws-s3tables',
];
baseConfig.overrides.push({
files: enableNoThrowDefaultErrorIn.map(m => `./${m}/lib/**`),
Expand Down
3 changes: 2 additions & 1 deletion packages/aws-cdk-lib/aws-s3-assets/lib/asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as iam from '../../aws-iam';
import * as kms from '../../aws-kms';
import * as s3 from '../../aws-s3';
import * as cdk from '../../core';
import { ValidationError } from '../../core/lib/errors';
import * as cxapi from '../../cx-api';

export interface AssetOptions extends CopyOptions, cdk.FileCopyOptions, cdk.AssetOptions {
Expand Down Expand Up @@ -143,7 +144,7 @@ export class Asset extends Construct implements cdk.IAsset {
super(scope, id);

if (!props.path) {
throw new Error('Asset path cannot be empty');
throw new ValidationError('Asset path cannot be empty', this);
}

this.isBundled = props.bundling != null;
Expand Down
3 changes: 2 additions & 1 deletion packages/aws-cdk-lib/aws-s3-assets/lib/compat.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { FollowMode } from '../../assets';
import { SymlinkFollowMode } from '../../core';
import { UnscopedValidationError } from '../../core/lib/errors';

export function toSymlinkFollow(follow?: FollowMode): SymlinkFollowMode | undefined {
if (!follow) {
Expand All @@ -12,6 +13,6 @@ export function toSymlinkFollow(follow?: FollowMode): SymlinkFollowMode | undefi
case FollowMode.BLOCK_EXTERNAL: return SymlinkFollowMode.BLOCK_EXTERNAL;
case FollowMode.EXTERNAL: return SymlinkFollowMode.EXTERNAL;
default:
throw new Error(`unknown follow mode: ${follow}`);
throw new UnscopedValidationError(`unknown follow mode: ${follow}`);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import * as path from 'path';
import { Template } from '../../assertions';
import { StackSynthesizer, FileAssetSource, FileAssetLocation, DockerImageAssetSource, DockerImageAssetLocation, ISynthesisSession, App, Stack, AssetManifestBuilder, CfnParameter, CfnResource } from '../../core';
import { UnscopedValidationError } from '../../core/lib/errors';
import { AssetManifestArtifact } from '../../cx-api';
import { Asset } from '../lib';

Expand Down Expand Up @@ -84,7 +85,7 @@ class CustomSynthesizer extends StackSynthesizer {

addDockerImageAsset(asset: DockerImageAssetSource): DockerImageAssetLocation {
void(asset);
throw new Error('Docker images are not supported here');
throw new UnscopedValidationError('Docker images are not supported here');
}

synthesize(session: ISynthesisSession): void {
Expand Down
17 changes: 9 additions & 8 deletions packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as lambda from '../../aws-lambda';
import * as logs from '../../aws-logs';
import * as s3 from '../../aws-s3';
import * as cdk from '../../core';
import { ValidationError } from '../../core/lib/errors';
import { BucketDeploymentSingletonFunction } from '../../custom-resource-handlers/dist/aws-s3-deployment/bucket-deployment-provider.generated';
import { AwsCliLayer } from '../../lambda-layer-awscli';

Expand Down Expand Up @@ -304,17 +305,17 @@ export class BucketDeployment extends Construct {

if (props.distributionPaths) {
if (!props.distribution) {
throw new Error('Distribution must be specified if distribution paths are specified');
throw new ValidationError('Distribution must be specified if distribution paths are specified', this);
}
if (!cdk.Token.isUnresolved(props.distributionPaths)) {
if (!props.distributionPaths.every(distributionPath => cdk.Token.isUnresolved(distributionPath) || distributionPath.startsWith('/'))) {
throw new Error('Distribution paths must start with "/"');
throw new ValidationError('Distribution paths must start with "/"', this);
}
}
}

if (props.useEfs && !props.vpc) {
throw new Error('Vpc must be specified if useEfs is set');
throw new ValidationError('Vpc must be specified if useEfs is set', this);
}

this.destinationBucket = props.destinationBucket;
Expand Down Expand Up @@ -376,7 +377,7 @@ export class BucketDeployment extends Construct {
});

const handlerRole = handler.role;
if (!handlerRole) { throw new Error('lambda.SingletonFunction should have created a Role'); }
if (!handlerRole) { throw new ValidationError('lambda.SingletonFunction should have created a Role', this); }
this.handlerRole = handlerRole;

this.sources = props.sources.map((source: ISource) => source.bind(this, { handlerRole: this.handlerRole }));
Expand Down Expand Up @@ -455,7 +456,7 @@ export class BucketDeployment extends Construct {
// '/this/is/a/random/key/prefix/that/is/a/lot/of/characters/do/we/think/that/it/will/ever/be/this/long?????'
// better to throw an error here than wait for CloudFormation to fail
if (!cdk.Token.isUnresolved(tagKey) && tagKey.length > 128) {
throw new Error('The BucketDeployment construct requires that the "destinationKeyPrefix" be <=104 characters.');
throw new ValidationError('The BucketDeployment construct requires that the "destinationKeyPrefix" be <=104 characters.', this);
}

/*
Expand Down Expand Up @@ -567,7 +568,7 @@ export class BucketDeployment extends Construct {
// configurations since we have a singleton.
if (memoryLimit) {
if (cdk.Token.isUnresolved(memoryLimit)) {
throw new Error("Can't use tokens when specifying 'memoryLimit' since we use it to identify the singleton custom resource handler.");
throw new ValidationError("Can't use tokens when specifying 'memoryLimit' since we use it to identify the singleton custom resource handler.", this);
}

uuid += `-${memoryLimit.toString()}MiB`;
Expand All @@ -578,7 +579,7 @@ export class BucketDeployment extends Construct {
// configurations since we have a singleton.
if (ephemeralStorageSize) {
if (ephemeralStorageSize.isUnresolved()) {
throw new Error("Can't use tokens when specifying 'ephemeralStorageSize' since we use it to identify the singleton custom resource handler.");
throw new ValidationError("Can't use tokens when specifying 'ephemeralStorageSize' since we use it to identify the singleton custom resource handler.", this);
}

uuid += `-${ephemeralStorageSize.toMebibytes().toString()}MiB`;
Expand Down Expand Up @@ -660,7 +661,7 @@ export class DeployTimeSubstitutedFile extends BucketDeployment {

constructor(scope: Construct, id: string, props: DeployTimeSubstitutedFileProps) {
if (!fs.existsSync(props.source)) {
throw new Error(`No file found at 'source' path ${props.source}`);
throw new ValidationError(`No file found at 'source' path ${props.source}`, scope);
}
// Makes substitutions on the file
let fileData = fs.readFileSync(props.source, 'utf-8');
Expand Down
11 changes: 6 additions & 5 deletions packages/aws-cdk-lib/aws-s3-deployment/lib/render-data.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Construct } from 'constructs';
import { Stack } from '../../core';
import { ValidationError } from '../../core/lib/errors';

export interface Content {
readonly text: string;
Expand All @@ -22,7 +23,7 @@ export function renderData(scope: Construct, data: string): Content {
}

if (typeof(obj) !== 'object') {
throw new Error(`Unexpected: after resolve() data must either be a string or a CloudFormation intrinsic. Got: ${JSON.stringify(obj)}`);
throw new ValidationError(`Unexpected: after resolve() data must either be a string or a CloudFormation intrinsic. Got: ${JSON.stringify(obj)}`, scope);
}

let markerIndex = 0;
Expand All @@ -35,7 +36,7 @@ export function renderData(scope: Construct, data: string): Content {
const parts = fnJoin[1];

if (sep !== '') {
throw new Error(`Unexpected "Fn::Join", expecting separator to be an empty string but got "${sep}"`);
throw new ValidationError(`Unexpected "Fn::Join", expecting separator to be an empty string but got "${sep}"`, scope);
}

for (const part of parts) {
Expand All @@ -49,21 +50,21 @@ export function renderData(scope: Construct, data: string): Content {
continue;
}

throw new Error(`Unexpected "Fn::Join" part, expecting string or object but got ${typeof (part)}`);
throw new ValidationError(`Unexpected "Fn::Join" part, expecting string or object but got ${typeof (part)}`, scope);
}

} else if (obj.Ref || obj['Fn::GetAtt'] || obj['Fn::Select']) {
addMarker(obj);
} else {
throw new Error('Unexpected: Expecting `resolve()` to return "Fn::Join", "Ref" or "Fn::GetAtt"');
throw new ValidationError('Unexpected: Expecting `resolve()` to return "Fn::Join", "Ref" or "Fn::GetAtt"', scope);
}

function addMarker(part: Ref | GetAtt | FnSelect) {
const keys = Object.keys(part);
const acceptedCfnFns = ['Ref', 'Fn::GetAtt', 'Fn::Select'];
if (keys.length !== 1 || !acceptedCfnFns.includes(keys[0])) {
const stringifiedAcceptedCfnFns = acceptedCfnFns.map((fn) => `"${fn}"`).join(' or ');
throw new Error(`Invalid CloudFormation reference. Key must start with any of ${stringifiedAcceptedCfnFns}. Got ${JSON.stringify(part)}`);
throw new ValidationError(`Invalid CloudFormation reference. Key must start with any of ${stringifiedAcceptedCfnFns}. Got ${JSON.stringify(part)}`, scope);
}

const marker = `<<marker:0xbaba:${markerIndex++}>>`;
Expand Down
9 changes: 5 additions & 4 deletions packages/aws-cdk-lib/aws-s3-deployment/lib/source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as iam from '../../aws-iam';
import * as s3 from '../../aws-s3';
import * as s3_assets from '../../aws-s3-assets';
import { FileSystem, Stack } from '../../core';
import { ValidationError } from '../../core/lib/errors';

/**
* Source information.
Expand Down Expand Up @@ -98,9 +99,9 @@ export class Source {
*/
public static bucket(bucket: s3.IBucket, zipObjectKey: string): ISource {
return {
bind: (_: Construct, context?: DeploymentSourceContext) => {
bind: (scope: Construct, context?: DeploymentSourceContext) => {
if (!context) {
throw new Error('To use a Source.bucket(), context must be provided');
throw new ValidationError('To use a Source.bucket(), context must be provided', scope);
}

bucket.grantRead(context.handlerRole);
Expand All @@ -121,7 +122,7 @@ export class Source {
return {
bind(scope: Construct, context?: DeploymentSourceContext): SourceConfig {
if (!context) {
throw new Error('To use a Source.asset(), context must be provided');
throw new ValidationError('To use a Source.asset(), context must be provided', scope);
}

let id = 1;
Expand All @@ -133,7 +134,7 @@ export class Source {
...options,
});
if (!asset.isZipArchive) {
throw new Error('Asset path must be either a .zip file or a directory');
throw new ValidationError('Asset path must be either a .zip file or a directory', scope);
}
asset.grantRead(context.handlerRole);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import * as logs from '../../aws-logs';
import * as s3 from '../../aws-s3';
import * as sns from '../../aws-sns';
import * as cdk from '../../core';
import { UnscopedValidationError } from '../../core/lib/errors';
import * as cxapi from '../../cx-api';
import * as s3deploy from '../lib';

Expand Down Expand Up @@ -1678,7 +1679,7 @@ function readDataFile(casm: cxapi.CloudAssembly, relativePath: string): string {
}
}

throw new Error(`File ${relativePath} not found in any of the assets of the assembly`);
throw new UnscopedValidationError(`File ${relativePath} not found in any of the assets of the assembly`);
}

test('DeployTimeSubstitutedFile allows custom role to be supplied', () => {
Expand Down
7 changes: 4 additions & 3 deletions packages/aws-cdk-lib/aws-s3-notifications/lib/lambda.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 lambda from '../../aws-lambda';
import * as s3 from '../../aws-s3';
import { CfnResource, Names, Stack } from '../../core';
import { ValidationError } from '../../core/lib/errors';

/**
* Use a Lambda function as a bucket notification destination
Expand All @@ -11,12 +12,12 @@ export class LambdaDestination implements s3.IBucketNotificationDestination {
constructor(private readonly fn: lambda.IFunction) {
}

public bind(_scope: Construct, bucket: s3.IBucket): s3.BucketNotificationDestinationConfig {
public bind(scope: Construct, bucket: s3.IBucket): s3.BucketNotificationDestinationConfig {
const permissionId = `AllowBucketNotificationsTo${Names.nodeUniqueId(this.fn.permissionsNode)}`;

if (!(bucket instanceof Construct)) {
throw new Error(`LambdaDestination for function ${Names.nodeUniqueId(this.fn.permissionsNode)} can only be configured on a
bucket construct (Bucket ${bucket.bucketName})`);
throw new ValidationError(`LambdaDestination for function ${Names.nodeUniqueId(this.fn.permissionsNode)} can only be configured on a
bucket construct (Bucket ${bucket.bucketName})`, scope);
}

if (bucket.node.tryFindChild(permissionId) === undefined) {
Expand Down

0 comments on commit aea8f3b

Please sign in to comment.