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

feat(lambda): allow running a build file #30196

Merged
merged 42 commits into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
ad1bb87
start work
bergjaak May 13, 2024
0b96fbb
feat(Lambda): allow running a build file
bergjaak May 14, 2024
52d3ce4
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/18470
bergjaak May 14, 2024
4ddf6e1
clean up
bergjaak May 15, 2024
94b6685
clean up
bergjaak May 15, 2024
5412aaf
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/18470
bergjaak May 15, 2024
9869ea9
allow user to specify arguments that are passed to spawnSync
bergjaak May 15, 2024
c3212e8
allow bundling options
bergjaak May 15, 2024
a162e59
lil comment
bergjaak May 15, 2024
63a1959
nice testing
bergjaak May 15, 2024
12134f9
nice testsss
bergjaak May 15, 2024
46da5e1
make error message more helpful
bergjaak May 15, 2024
7eef2ee
integ test
bergjaak May 15, 2024
573756f
integ test
bergjaak May 15, 2024
2e21017
integ test
bergjaak May 15, 2024
3e89cf1
reeead me
bergjaak May 15, 2024
df987a9
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/18470
bergjaak May 15, 2024
90fa9a4
integ test is working
bergjaak May 15, 2024
a0610f6
integ test is working
bergjaak May 15, 2024
731c363
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/18470
bergjaak May 16, 2024
83b4238
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/18470
bergjaak May 16, 2024
f83234a
try the integ test without deleting any files
bergjaak May 16, 2024
dc8f61e
use bash for integ test... plz work"
bergjaak May 16, 2024
7cccfd8
stuff
bergjaak May 16, 2024
fa4bde2
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/18470
bergjaak May 16, 2024
916f92d
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/18470
bergjaak May 28, 2024
c0c324a
stuff
bergjaak May 28, 2024
870090b
don't change things that shouldn't change
bergjaak May 28, 2024
c193beb
don't change things that shouldn't change
bergjaak May 28, 2024
1e59d4a
fix readme
bergjaak May 28, 2024
a66abc1
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/18470
bergjaak May 30, 2024
58d2c86
add documentation
bergjaak May 30, 2024
6e4b2cc
add documentation
bergjaak May 30, 2024
70469ea
add documentation
bergjaak May 30, 2024
e5de5ff
add documentation
bergjaak May 30, 2024
a6bcad2
make rosetta happy please
bergjaak May 30, 2024
26b4b15
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/18470
bergjaak May 30, 2024
7200961
changes
bergjaak May 30, 2024
54fe929
changes
bergjaak May 30, 2024
63d0788
changes
bergjaak May 30, 2024
2732c24
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/18470
bergjaak May 30, 2024
86c345b
changes
bergjaak May 30, 2024
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
20 changes: 8 additions & 12 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,15 @@
"version": "0.2.0",
"configurations": [
{
// Has convenient settings for attaching to a NodeJS process for debugging purposes
// that are NOT the default and otherwise every developers has to configure for
// themselves again and again.
"type": "node",
"request": "attach",
"name": "Attach to NodeJS",
// If we don't do this, every step-into into an async function call will go into
// NodeJS internals which are hard to step out of.
"skipFiles": [
"<node_internals>/**"
],
// Saves some button-pressing latency on attaching
"stopOnEntry": false
"request": "launch",
"args": ["jest", "--runTestsByPath", "/Users/bergjak/workplace/CDK/aws-cdk/packages/aws-cdk-lib/aws-lambda-nodejs/test/function.test.ts"],
"name": "debug unit test",
"program": "/opt/homebrew/bin/yarn",
"cwd": "/Users/bergjak/workplace/CDK/aws-cdk/packages/aws-cdk-lib",
"console": "integratedTerminal",
"sourceMaps": true,
"skipFiles": [ "<node_internals>/**/*" ],
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I'll remove this in my final revision

}
]
}
69 changes: 49 additions & 20 deletions packages/aws-cdk-lib/aws-lambda-nodejs/lib/function.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,12 @@ export interface NodejsFunctionProps extends lambda.FunctionOptions {
/**
* The name of the exported handler in the entry file.
*
* The handler is prefixed with `index.` unless the specified handler value contains a `.`,
* in which case it is used as-is.
* * If the `code` property is supplied, then you must include the `handler` property. The handler should be the name of the file
* that contains the exported handler and the function that should be called when the AWS Lambda is invoked. For example, if
* you had a file called `myLambda.js` and the function to be invoked was `myHandler`, then you should input `handler` property as `myLambda.myHandler`.
*
* * If the `code` property is not supplied and the handler input does not contain a `.`, then the handler is prefixed with `index.` (index period). Otherwise,
* the handler property is not modified.
*
* @default handler
*/
Expand Down Expand Up @@ -83,6 +87,17 @@ export interface NodejsFunctionProps extends lambda.FunctionOptions {
* @default - the directory containing the `depsLockFilePath`
*/
readonly projectRoot?: string;

/**
* The code that will be deployed to the Lambda Handler. If included, then properties related to
* bundling of the code are ignored. In the constructor of NodeJsFunction, where the Super is called,
* you can see which bundling properties are ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's re-phrase this. Maybe just leave it to "If included, then properties related to bundling of the code are ignored". The "In the constructor of the NodeJsFunction, where the Super is called you can see which bundling properties are ignored" isn't helpful since I wouldn't necessarily expect a user to have to go into our source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense, agreed

*
* * If the `code` field is specified, then you must include the `handler` property.
*
* @default - the code is bundled by esbuild
*/
readonly code?: lambda.Code;
}

/**
Expand All @@ -94,27 +109,41 @@ export class NodejsFunction extends lambda.Function {
throw new Error('Only `NODEJS` runtimes are supported.');
}

// Entry and defaults
const entry = path.resolve(findEntry(id, props.entry));
const handler = props.handler ?? 'handler';
const architecture = props.architecture ?? Architecture.X86_64;
const depsLockFilePath = findLockFile(props.depsLockFilePath);
const projectRoot = props.projectRoot ?? path.dirname(depsLockFilePath);
const runtime = getRuntime(scope, props);

super(scope, id, {
...props,
runtime,
code: Bundling.bundle(scope, {
...props.bundling ?? {},
entry,
if (props.code !== undefined) {
if (props.handler === undefined) {
throw new Error('Cannot determine handler when `code` property is specified. Use `handler` property to specify a handler.');
}

super(scope, id, {
...props,
runtime,
architecture,
depsLockFilePath,
projectRoot,
}),
handler: handler.indexOf('.') !== -1 ? `${handler}` : `index.${handler}`,
});
code: props.code,
handler: props.handler!,
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 remove the !? We're already checking and throwing an error if props.handler is undefined so we don't need to use ! here.

});
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand why we need this new code block here? Why can't we just determine the handler in the same way we are doing it right now? Couldn't we just keep the existing code and do something where if code is supplied we use it, otherwise we do Bundling.bundle? So something like:

  // Entry and defaults
  const entry = path.resolve(findEntry(id, props.entry));
  const architecture = props.architecture ?? Architecture.X86_64;
  const depsLockFilePath = findLockFile(props.depsLockFilePath);
  const projectRoot = props.projectRoot ?? path.dirname(depsLockFilePath);
  const handler = props.handler ?? 'handler';

  super(scope, id, {
    ...props,
    runtime,
    code: props.code ?? Bundling.bundle(scope, {
      ...props.bundling ?? {},
      entry,
      runtime,
      architecture,
      depsLockFilePath,
      projectRoot,
    }),
    handler: handler.indexOf('.') !== -1 ? `${handler}` : `index.${handler}`,
  });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I originally had, but I think it's a worse customer experience. Because there's nothing forcing the customer to name the file containing the exported handler index. It's entirely up to the customer to decide how they want to name that file, if they're using the Code.fromCustomCommand. Therefore, I think this is a better customer experience, even if it's a few more lines of code

Copy link
Contributor

Choose a reason for hiding this comment

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

I see where you're coming from but I'm not sure I agree. I think this construct has already established that the expected behavior is we will default the handler if it isn't supplied. Consider a customer that is using the nodejs function and they currently don't supply a handler and just use the default. Now, once this feature is released, they decide to use the new code property to run a custom build script. However, suddenly they're encountering errors since they don't set a handler. I recognize that it is trivial to just specify a handler, but it is a change from the expectation for the construct as a whole. Additionally, we generally try to avoid properties where we have to do a bunch of validation around if a property is set then another one must also be set. We consider it a bit of an anti-pattern. Our VPC construct is a more extreme example of this, but it shows where this can get to be hard to deal with when we have to start doing a ton of property validation - https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-ec2/lib/vpc.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, once this feature is released, they decide to use the new code property to run a custom build script. However, suddenly they're encountering errors since they don't set a handler.

My understanding is, if a customer does switch to using Code.fromCustomCommand and wants an error-free transition with default handler values, then their build script will need to create a file (either in a directory or a zip file) named index.js with an exported function called handler. Defaulting to handler makes sense in this case since it's unlikely that the customer would have changed the name of the function. However, if the customer never knew that the file that contained their lambda code, when deployed to AWS Lambda, was called index.js (since that's abstracted away by this construct), then wouldn't the customer be confused when their lambda fails to work because their build file didn't output the handler in a file called index.js? That seems more confusing to me. By throwing the Error here, we push the error that the customer may run into much earlier, rather than the customer's lambda failing at runtime after deploying successfully -- which could cause an outage if a customer is deploying directly to production.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically, to avoid having an outage in production, if a customer is switching to using this Code.fromCustomCommand, the customer would need to know that their build file must output a file called index.js that exports a function called handler. That's not something that I think should be hidden from the customer. By requiring handler, we don't abstract away a detail that could potentially lead to an outage

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 agree that this is more complicated, but the alternative is to push this complexity onto the customer after they've deployed their lambda

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to change my mind on this, and with the way you've explained it I agree. Doesn't make sense to let the customer fail during deploy time.

// Entry and defaults
const entry = path.resolve(findEntry(id, props.entry));
const architecture = props.architecture ?? Architecture.X86_64;
const depsLockFilePath = findLockFile(props.depsLockFilePath);
const projectRoot = props.projectRoot ?? path.dirname(depsLockFilePath);
const handler = props.handler ?? 'handler';

super(scope, id, {
...props,
runtime,
code: Bundling.bundle(scope, {
...props.bundling ?? {},
entry,
runtime,
architecture,
depsLockFilePath,
projectRoot,
}),
handler: handler.indexOf('.') !== -1 ? `${handler}` : `index.${handler}`,
});
}

// Enable connection reuse for aws-sdk
if (props.awsSdkConnectionReuse ?? true) {
Expand Down
47 changes: 46 additions & 1 deletion packages/aws-cdk-lib/aws-lambda-nodejs/test/function.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import * as child_process from 'child_process';
import { bockfs } from '@aws-cdk/cdk-build-tools';
import { Template, Match } from '../../assertions';
import { Vpc } from '../../aws-ec2';
import { CodeConfig, Runtime } from '../../aws-lambda';
import { Code, CodeConfig, Runtime } from '../../aws-lambda';
import { App, Stack } from '../../core';
import { LAMBDA_NODEJS_USE_LATEST_RUNTIME } from '../../cx-api';
import { NodejsFunction } from '../lib';
Expand Down Expand Up @@ -50,6 +51,7 @@ bockfs({
'/home/project/function.test.handler4.mts': '// nothing',
'/home/project/function.test.handler5.cts': '// nothing',
'/home/project/function.test.handler6.cjs': '// nothing',
'/home/project/function.test.handler7.zip': '// nothing',
'/home/project/aws-lambda-nodejs/lib/index.ts': '// nothing',
});
const bockPath = bockfs.workingDirectory('/home/project');
Expand Down Expand Up @@ -78,6 +80,49 @@ test('NodejsFunction with .ts handler', () => {
});
});

describe('lambda.Code.fromCustomCommand', () => {
// GIVEN
beforeEach(() => {
jest.spyOn(child_process, 'spawnSync').mockReturnValue({
status: 0,
stderr: Buffer.from('stderr'),
stdout: Buffer.from('stdout'),
pid: 123,
output: ['stdout', 'stderr'],
signal: null,
});
});
afterEach(() => {
jest.restoreAllMocks();
});

test('if code property is included without handler property, then error is thrown', () => {
// WHEN
const handlerName = undefined;

// THEN
expect(() => new NodejsFunction(stack, 'handler1', {
handler: handlerName,
code: Code.fromCustomCommand('function.test.handler7.zip', ['node'], undefined),
})).toThrow('Cannot determine handler when `code` property is specified. Use `handler` property to specify a handler.');
});

test('if code and handler properties are included, the template can be synthesized', () => {
// WHEN
new NodejsFunction(stack, 'handler1', {
handler: 'Random.Name',
runtime: Runtime.NODEJS_18_X,
code: Code.fromCustomCommand('function.test.handler7.zip', ['node'], undefined),
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Function', {
Handler: 'Random.Name',
Runtime: 'nodejs18.x',
});
});
});

test('NodejsFunction with overridden handler - no dots', () => {
// WHEN
new NodejsFunction(stack, 'handler1', {
Expand Down
39 changes: 39 additions & 0 deletions packages/aws-cdk-lib/aws-lambda/lib/code.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { spawnSync } from 'child_process';
import { Construct } from 'constructs';
import * as ecr from '../../aws-ecr';
import * as ecr_assets from '../../aws-ecr-assets';
Expand Down Expand Up @@ -54,6 +55,44 @@ export abstract class Code {
return new AssetCode(path, options);
}

/**
* Runs a command to build the code asset that will be used.
*
* @param output Where the output of the command will be directed, either a directory or a .zip file with the output Lambda code bundle
* * For example, if you use the command to run a build script (e.g., [ 'node', 'bundle_code.js' ]), and the build script generates a directory `/my/lambda/code`
* containing code that should be ran in a Lambda function, then output should be set to `/my/lambda/code`
* @param command The command which will be executed to generate the output, for example, [ 'node', 'bundle_code.js' ]
* @param assetOptions The same options that are available for `Code.fromAsset` -- but bundling options are not allowed
* @param commandOptions Options that are passed to the spawned process, which determine the characteristics of the spawned process.
* * See `child_process.SpawnSyncOptions` for possible inputs and defaults (https://nodejs.org/api/child_process.html#child_processspawnsynccommand-args-options).
*/
public static fromCustomCommand(
output: string,
command: string[],
commandOptions?: {[option: string]: any}, // jsii build fails if the type is SpawnSyncOptions... so best we can do is point user to those options.
assetOptions?: s3_assets.AssetOptions,
Copy link
Contributor

@colifran colifran May 16, 2024

Choose a reason for hiding this comment

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

I generally prefer to use interfaces where we can so that if there is ever a chance that a new argument is introduced we can just add it to the interface. What do you think about creating a new interface that just extends AssetOptions? We could call it CustomCommandOptions. Something like this:

export interface CustomCommandOptions extends AssetOptions {
	readonly commands?: { [options: string]: any },
}

Then the function definition could just be:

public static fromCustomCommand(output: string, command: string[], options: CustomCommandOptions = {})

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 agree with you, that's an improvement

): AssetCode {
if (command.length === 0) {
throw new Error('command must contain at least one argument. For example, ["node", "buildFile.js"].');
}

const cmd = command[0];
const commandArguments = command.splice(1);

const proc = commandOptions === undefined
? spawnSync(cmd, commandArguments) // use the default spawnSyncOptions
: spawnSync(cmd, commandArguments, commandOptions);

if (proc.error) {
throw new Error(`Failed to execute custom command: ${proc.error}`);
}
if (proc.status !== 0) {
throw new Error(`${command.join(' ')} exited with status: ${proc.status}\n\nstdout: ${proc.stdout?.toString().trim()}\n\nstderr: ${proc.stderr?.toString().trim()}`);
}

return new AssetCode(output, assetOptions);
}

/**
* Loads the function code from an asset created by a Docker build.
*
Expand Down
73 changes: 73 additions & 0 deletions packages/aws-cdk-lib/aws-lambda/test/code.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as child_process from 'child_process';
import * as path from 'path';
import { Match, Template } from '../../assertions';
import * as ecr from '../../aws-ecr';
Expand All @@ -15,6 +16,78 @@ describe('code', () => {
});
});

describe('lambda.Code.fromCustomCommand', () => {
let spawnSyncMock: jest.SpyInstance;
beforeEach(() => {
spawnSyncMock = jest.spyOn(child_process, 'spawnSync').mockReturnValue({
status: 0,
stderr: Buffer.from('stderr'),
stdout: Buffer.from('stdout'),
pid: 123,
output: ['stdout', 'stderr'],
signal: null,
});
});
afterEach(() => {
jest.restoreAllMocks();
});

test('fails if command is empty', () => {
// GIVEN
const command = [];

// THEN
expect(() => lambda.Code.fromCustomCommand('', command)).toThrow('command must contain at least one argument. For example, ["node", "buildFile.js"].');
});
test('properly splices arguments', () => {
// GIVEN
const command = 'node is a great command, wow'.split(' ');
lambda.Code.fromCustomCommand('', command);

// THEN
expect(spawnSyncMock).toHaveBeenCalledWith(
'node',
['is', 'a', 'great', 'command,', 'wow'],
);
});
test('properly splices arguments when commandOptions are included', () => {
// GIVEN
const command = 'node is a great command, wow'.split(' ');
const commandOptions = { cwd: '/tmp', env: { SOME_KEY: 'SOME_VALUE' } };
lambda.Code.fromCustomCommand('', command, commandOptions);

// THEN
expect(spawnSyncMock).toHaveBeenCalledWith(
'node',
['is', 'a', 'great', 'command,', 'wow'],
commandOptions,
);
});
test('throws custom error message when spawnSync errors', () => {
// GIVEN
jest.restoreAllMocks(); // use the real spawnSync, which doesn't work in unit tests.
const command = ['whatever'];

// THEN
expect(() => lambda.Code.fromCustomCommand('', command)).toThrow(/Failed to execute custom command: .*/);
});
test('throws custom error message when spawnSync exits with non-zero status code', () => {
// GIVEN
const command = ['whatever'];
spawnSyncMock = jest.spyOn(child_process, 'spawnSync').mockReturnValue({
status: 1,
stderr: Buffer.from('stderr'),
stdout: Buffer.from('stdout'),
pid: 123,
output: ['stdout', 'stderr'],
signal: null,
});

// THEN
expect(() => lambda.Code.fromCustomCommand('', command)).toThrow('whatever exited with status: 1\n\nstdout: stdout\n\nstderr: stderr');
});
});

describe('lambda.Code.fromAsset', () => {
test('fails if path is empty', () => {
// GIVEN
Expand Down
Loading