Skip to content

feat(custom-resources): throw ValidationError instead of untyped Errors #33392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/aws-cdk-lib/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,22 @@ baseConfig.rules['@cdklabs/no-throw-default-error'] = ['error'];
// not yet supported
const noThrowDefaultErrorNotYetSupported = [
'core',
'custom-resources',
'region-info',
];
baseConfig.overrides.push({
files: [
// Build and test files can have whatever error they like
"./scripts/**",
"./*/build-tools/**",
"./*/test/**",

// Lambda Runtime code should use regular errors
"./custom-resources/lib/provider-framework/runtime/**",

// Not yet supported modules
...noThrowDefaultErrorNotYetSupported.map(m => `./${m}/lib/**`)
],
rules: { "@cdklabs/no-throw-default-error": "off" },
});


module.exports = baseConfig;
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ export class AwsCustomResource extends Construct implements iam.IGrantable {
private static breakIgnoreErrorsCircuit(sdkCalls: Array<AwsSdkCall | undefined>, caller: string) {
for (const call of sdkCalls) {
if (call?.ignoreErrorCodesMatching) {
throw new Error(`\`${caller}\`` + ' cannot be called along with `ignoreErrorCodesMatching`.');
throw new cdk.UnscopedValidationError(`\`${caller}\`` + ' cannot be called along with `ignoreErrorCodesMatching`.');
}
}
}
Expand All @@ -471,19 +471,19 @@ export class AwsCustomResource extends Construct implements iam.IGrantable {
super(scope, id);

if (!props.onCreate && !props.onUpdate && !props.onDelete) {
throw new Error('At least `onCreate`, `onUpdate` or `onDelete` must be specified.');
throw new cdk.ValidationError('At least `onCreate`, `onUpdate` or `onDelete` must be specified.', this);
}

if (!props.role && !props.policy) {
throw new Error('At least one of `policy` or `role` (or both) must be specified.');
throw new cdk.ValidationError('At least one of `policy` or `role` (or both) must be specified.', this);
}

if (props.onCreate && !props.onCreate.physicalResourceId) {
throw new Error("'physicalResourceId' must be specified for 'onCreate' call.");
throw new cdk.ValidationError("'physicalResourceId' must be specified for 'onCreate' call.", this);
}

if (!props.onCreate && props.onUpdate && !props.onUpdate.physicalResourceId) {
throw new Error("'physicalResourceId' must be specified for 'onUpdate' call when 'onCreate' is omitted.");
throw new cdk.ValidationError("'physicalResourceId' must be specified for 'onUpdate' call when 'onCreate' is omitted.", this);
}

for (const call of [props.onCreate, props.onUpdate, props.onDelete]) {
Expand All @@ -493,7 +493,7 @@ export class AwsCustomResource extends Construct implements iam.IGrantable {
}

if (includesPhysicalResourceIdRef(props.onCreate?.parameters)) {
throw new Error('`PhysicalResourceIdReference` must not be specified in `onCreate` parameters.');
throw new cdk.ValidationError('`PhysicalResourceIdReference` must not be specified in `onCreate` parameters.', this);
}

this.props = props;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ export class Provider extends Construct implements ICustomResourceProvider {
|| props.waiterStateMachineLogOptions
|| props.disableWaiterStateMachineLogging !== undefined
) {
throw new Error('"queryInterval", "totalTimeout", "waiterStateMachineLogOptions", and "disableWaiterStateMachineLogging" '
throw new ValidationError('"queryInterval", "totalTimeout", "waiterStateMachineLogOptions", and "disableWaiterStateMachineLogging" '
+ 'can only be configured if "isCompleteHandler" is specified. '
+ 'Otherwise, they have no meaning');
+ 'Otherwise, they have no meaning', this);
}
}

Expand Down Expand Up @@ -258,7 +258,7 @@ export class Provider extends Construct implements ICustomResourceProvider {
const isCompleteFunction = this.createFunction(consts.FRAMEWORK_IS_COMPLETE_HANDLER_NAME, undefined, props.frameworkCompleteAndTimeoutRole);
const timeoutFunction = this.createFunction(consts.FRAMEWORK_ON_TIMEOUT_HANDLER_NAME, undefined, props.frameworkCompleteAndTimeoutRole);

const retry = calculateRetryPolicy(props);
const retry = calculateRetryPolicy(this, props);
const waiterStateMachine = new WaiterStateMachine(this, 'waiter-state-machine', {
isCompleteHandler: isCompleteFunction,
timeoutHandler: timeoutFunction,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
import { Duration } from '../../../core';
import type { IConstruct } from 'constructs';
import { Duration, ValidationError } from '../../../core';

const DEFAULT_TIMEOUT = Duration.minutes(30);
const DEFAULT_INTERVAL = Duration.seconds(5);

export function calculateRetryPolicy(props: { totalTimeout?: Duration; queryInterval?: Duration } = { }) {
export function calculateRetryPolicy(scope: IConstruct, props: { totalTimeout?: Duration; queryInterval?: Duration } = { }) {
const totalTimeout = props.totalTimeout || DEFAULT_TIMEOUT;
const interval = props.queryInterval || DEFAULT_INTERVAL;
const maxAttempts = totalTimeout.toSeconds() / interval.toSeconds();

if (Math.round(maxAttempts) !== maxAttempts) {
throw new Error(`Cannot determine retry count since totalTimeout=${totalTimeout.toSeconds()}s is not integrally dividable by queryInterval=${interval.toSeconds()}s`);
throw new ValidationError(`Cannot determine retry count since totalTimeout=${totalTimeout.toSeconds()}s is not integrally dividable by queryInterval=${interval.toSeconds()}s`, scope);
}

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -352,30 +352,32 @@ test('fails if "queryInterval" or "totalTimeout" or "waiterStateMachineLogOption
});

describe('retry policy', () => {
const stack = new Stack();

it('default is 30 minutes timeout in 5 second intervals', () => {
const policy = util.calculateRetryPolicy();
const policy = util.calculateRetryPolicy(stack);
expect(policy.backoffRate).toStrictEqual(1);
expect(policy.interval && policy.interval.toSeconds()).toStrictEqual(5);
expect(policy.maxAttempts).toStrictEqual(360);
});

it('if total timeout and query interval are the same we will have one attempt', () => {
const policy = util.calculateRetryPolicy({
const policy = util.calculateRetryPolicy(stack, {
totalTimeout: Duration.minutes(5),
queryInterval: Duration.minutes(5),
});
expect(policy.maxAttempts).toStrictEqual(1);
});

it('fails if total timeout cannot be integrally divided', () => {
expect(() => util.calculateRetryPolicy({
expect(() => util.calculateRetryPolicy(stack, {
totalTimeout: Duration.seconds(100),
queryInterval: Duration.seconds(75),
})).toThrow(/Cannot determine retry count since totalTimeout=100s is not integrally dividable by queryInterval=75s/);
});

it('fails if interval > timeout', () => {
expect(() => util.calculateRetryPolicy({
expect(() => util.calculateRetryPolicy(stack, {
totalTimeout: Duration.seconds(5),
queryInterval: Duration.seconds(10),
})).toThrow(/Cannot determine retry count since totalTimeout=5s is not integrally dividable by queryInterval=10s/);
Expand Down
Loading