Skip to content
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

fix(synthetics): updated handler validation #26569

Merged
merged 5 commits into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
42 changes: 26 additions & 16 deletions packages/@aws-cdk/aws-synthetics-alpha/lib/canary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,9 @@ export class Test {
* @param options The configuration options
*/
public static custom(options: CustomTestOptions): Test {
Test.validateHandler(options.handler);
return new Test(options.code, options.handler);
}

/**
* Verifies that the given handler ends in '.handler'. Returns the handler if successful and
* throws an error if not.
*
* @param handler - the handler given by the user
*/
private static validateHandler(handler: string) {
if (!handler.endsWith('.handler')) {
throw new Error(`Canary Handler must end in '.handler' (${handler})`);
}
if (handler.length > 21) {
throw new Error(`Canary Handler must be less than 21 characters (${handler})`);
}
}

/**
* Construct a Test property
*
Expand Down Expand Up @@ -422,6 +406,7 @@ export class Canary extends cdk.Resource implements ec2.IConnectable {
* Returns the code object taken in by the canary resource.
*/
private createCode(props: CanaryProps): CfnCanary.CodeProperty {
this.validateHandler(props.test.handler, props.runtime);
const codeConfig = {
handler: props.test.handler,
...props.test.code.bind(this, props.test.handler, props.runtime.family),
Expand All @@ -435,6 +420,31 @@ export class Canary extends cdk.Resource implements ec2.IConnectable {
};
}

private validateHandler(handler: string, runtime: Runtime) {
kaizencc marked this conversation as resolved.
Show resolved Hide resolved
if (
runtime === Runtime.SYNTHETICS_PYTHON_SELENIUM_1_0 ||
runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_0 ||
runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_1 ||
runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_2 ||
runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_3
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (
runtime === Runtime.SYNTHETICS_PYTHON_SELENIUM_1_0 ||
runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_0 ||
runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_1 ||
runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_2 ||
runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_3
) {
const oldRuntimes = [/*put those runtimes here */];
if (oldRuntimes.includes(runtime)) {

if (!handler.match(/^[0-9A-Za-z_\\-]+\.handler*$/)) {
throw new Error(`Canary Handler must be specified as \'fileName.handler\' for legacy runtimes, received ${handler}`);
}
} else if (runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_4) {
if (!handler.match(/^[0-9A-Za-z_\\-]+\.[A-Za-z_][A-Za-z0-9_]*$/)) {
throw new Error(`Canary Handler must be specified either as \'fileName.handler\' or \'fileName.functionName\' for the \'syn-nodejs-puppeteer-3.4'\ runtime, received ${handler}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did you get the info that 3.4 has it's own specific rule? From the cfn docs it seems this isn't the case:

For syn-python-selenium-1.1, syn-nodejs.puppeteer-3.4, and later runtimes, the handler can be specified as fileName.functionName , or you can specify a folder where canary scripts reside as folder/fileName.functionName .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. I think I misread this line.
I'll apply the suggested changes and add the required tests.

}
} else {
if (!handler.match(/^([0-9a-zA-Z_-]+\/)*[0-9A-Za-z_\\-]+\.[A-Za-z_][A-Za-z0-9_]*$/)) {
throw new Error(`Canary Handler must be specified either as \'fileName.handler\', \'fileName.functionName\', or \'folder/fileName.functionName\', received ${handler}`);
}
}
if (handler.length < 1 || handler.length > 128) {
throw new Error(`Canary Handler length must be between 1 and 128, received ${handler.length}`);
}
}

private createRunConfig(props: CanaryProps): CfnCanary.RunConfigProperty | undefined {
if (!props.environmentVariables) {
return undefined;
Expand Down
7 changes: 1 addition & 6 deletions packages/@aws-cdk/aws-synthetics-alpha/lib/code.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,7 @@ export class InlineCode extends Code {
}
}

public bind(_scope: Construct, handler: string, _family: RuntimeFamily): CodeConfig {

if (handler !== 'index.handler') {
throw new Error(`The handler for inline code must be "index.handler" (got "${handler}")`);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about removing this. Have you tested this to make sure that it works? IIRC lambda will bundle inline code into an index.js|py file, which means that it should at least always start with index.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I restored the original code since I'm unsure about the expected behavior and couldn't find documentation about it.
Let me know if you think we should dive deeper into this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see anyone complaining about this, and it looks right to me. So lets keep it for now


public bind(_scope: Construct, _handler: string, _family: RuntimeFamily): CodeConfig {
return {
inlineCode: this.code,
};
Expand Down
78 changes: 78 additions & 0 deletions packages/@aws-cdk/aws-synthetics-alpha/test/canary.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -703,3 +703,81 @@ testDeprecated('Role policy generated as expected', () => {
}],
});
});

testDeprecated('Should create handler with path for recent runtimes', () => {
// GIVEN
const stack = new Stack();

// WHEN
new synthetics.Canary(stack, 'Canary', {
canaryName: 'mycanary',
test: synthetics.Test.custom({
handler: 'folder/fileName.functionName',
code: synthetics.Code.fromInline('/* Synthetics handler code */'),
}),
runtime: synthetics.Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_8,
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Synthetics::Canary', {
Name: 'mycanary',
Code: {
Handler: 'folder/fileName.functionName',
},
RuntimeVersion: 'syn-nodejs-puppeteer-3.8',
});
});

describe('handler validation', () => {
testDeprecated('legacy runtimes', () => {
const stack = new Stack();
expect(() => {
new synthetics.Canary(stack, 'Canary', {
test: synthetics.Test.custom({
handler: 'index.functionName',
code: synthetics.Code.fromInline('/* Synthetics handler code'),
}),
runtime: synthetics.Runtime.SYNTHETICS_PYTHON_SELENIUM_1_0,
});
}).toThrow(/Canary Handler must be specified as 'fileName.handler' for legacy runtimes/);
});

testDeprecated('syn-nodejs-puppeteer-3.4', () => {
const stack = new Stack();
expect(() => {
new synthetics.Canary(stack, 'Canary', {
test: synthetics.Test.custom({
handler: 'folder/fileName.functionName',
code: synthetics.Code.fromInline('/* Synthetics handler code'),
}),
runtime: synthetics.Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_4,
});
}).toThrow(/Canary Handler must be specified either as 'fileName.handler' or 'fileName.functionName' for the 'syn-nodejs-puppeteer-3.4' runtime/);
});

testDeprecated('recent runtimes', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also unit test that fileName.functionName and folder/fileName.functionName passes the regex for recent runtimes

const stack = new Stack();
expect(() => {
new synthetics.Canary(stack, 'Canary', {
test: synthetics.Test.custom({
handler: 'invalidHandler',
code: synthetics.Code.fromInline('/* Synthetics handler code'),
}),
runtime: synthetics.Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_9,
});
}).toThrow(/Canary Handler must be specified either as 'fileName.handler', 'fileName.functionName', or 'folder\/fileName.functionName'/);
});

testDeprecated('handler length', () => {
const stack = new Stack();
expect(() => {
new synthetics.Canary(stack, 'Canary1', {
test: synthetics.Test.custom({
handler: 'longHandlerName'.repeat(10) + '.handler',
code: synthetics.Code.fromInline('/* Synthetics handler code'),
}),
runtime: synthetics.Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_9,
});
}).toThrow(/Canary Handler length must be between 1 and 128/);
});
});
9 changes: 0 additions & 9 deletions packages/@aws-cdk/aws-synthetics-alpha/test/code.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,6 @@ describe(synthetics.Code.fromInline, () => {
expect(() => synthetics.Code.fromInline(''))
.toThrowError('Canary inline code cannot be empty');
});

test('fails if handler is not "index.handler"', () => {
// GIVEN
const stack = new Stack(new App(), 'canaries');

// THEN
expect(() => synthetics.Code.fromInline('code').bind(stack, 'canary.handler', RuntimeFamily.NODEJS))
.toThrowError('The handler for inline code must be "index.handler" (got "canary.handler")');
});
});

describe(synthetics.Code.fromAsset, () => {
Expand Down