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.

Sign up for GitHub

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

Jump to bottom

[WIP] build a Function's runtime with a Docker container #1884

Closed
wants to merge 2 commits into from
Closed

[WIP] build a Function's runtime with a Docker container #1884

wants to merge 2 commits into from

Conversation

sam-goodwin
Copy link
Contributor

This change adds a new BuildAsset concept that can build an asset from a project directory by running a command within a Docker container. The primary use-case is to integrate with aws-lambda-builders (see the design PR: aws/aws-lambda-builders#88) for building lambda functions, but the intent is to be extensible enough to support BYO and project types other than Lambda Functions.

When we launch the Docker container, we mount the source code at /tmp/cdk.out and set the working directory to that same location. The command will then run against the project's code and is expected to deposit artifacts under .cdk.staging within the source directory. The standard FileAsset process is continued after the build completes.

This change also adds a JvmFunction and NodeFunction extension of lambda.Function that provides a narrow interface for building functions built with their respective platforms.

Putting this out to get early feedback - there is still a bunch of work to do:

  • Re-factor Assets to better support workflows - the metadata hack is messy
  • Extract the 'project inference' logic duplicated from SAM CLI into a separate library. See [WIP] Design for generic builder interface aws-lambda-builders#88 for details.
  • Create separate PRs for aws-assets change and aws-lambda change.
  • Add support for the remaining languages: go, dotnet, python, custom?
  • Optimize so we only build when necessary?
  • Unit and Integration tests
  • Documentation (README and Code)

Example experience:

const app = new cdk.App();
const stack = new cdk.Stack(app, 'hello-builders');

const gradle = new lambda.JvmFunction(stack, 'Gradle', {
  version: lambda.JvmVersion.Java8,
  path: './gradle-project',
  handler: {
    className: 'gradle.project.App',
    methodName: 'getGreeting'
  }
});
const npm = new lambda.NodeFunction(stack, 'Node', {
  version: lambda.NodeVersion.NodeJS810,
  path: './node-project',
  handler: 'index.handle',
});

app.run();

Closes #1435

Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@sam-goodwin sam-goodwin requested a review from a team as a code owner February 26, 2019 06:54
@sam-goodwin
Copy link
Contributor Author

sam-goodwin commented Feb 26, 2019 •
edited
Loading

@jfuss

@sam-goodwin sam-goodwin mentioned this pull request Feb 26, 2019
Closed
eladb
eladb approved these changes Feb 26, 2019
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

looks promising

packages/@aws-cdk/assets/lib/asset.ts
*/
export class Asset extends cdk.Construct {
export abstract class BaseAsset extends cdk.Construct {
Copy link
Contributor

Choose a reason for hiding this comment

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

AssetBase

packages/@aws-cdk/assets/lib/asset.ts
@@ -180,6 +158,54 @@ export class Asset extends cdk.Construct {
}
}

export interface GenericAssetProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Props type name should be XxxProps

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 - i found it strange that we had a Generic prefix

packages/@aws-cdk/assets/lib/asset.ts
@@ -224,6 +250,53 @@ export class ZipDirectoryAsset extends Asset {
}
}

export interface BuildAssetProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

jsdocs

packages/@aws-cdk/assets/lib/asset.ts
@@ -224,6 +250,53 @@ export class ZipDirectoryAsset extends Asset {
}
}

export interface BuildAssetProps {
codePath: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

projectRoot?

packages/@aws-cdk/assets/lib/asset.ts
@@ -224,6 +250,53 @@ export class ZipDirectoryAsset extends Asset {
}
}

export interface BuildAssetProps {
codePath: string;
artifactName: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this important and who cares? Can you use construct.node.uniqueId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aws-lambda-builders doesn't offer a way to specify the name of the archive file. I'll make sure that aws/aws-lambda-builders#88 calls this out as a requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even though, who cares? I believe construct.node.uniqueId should be fine, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not all languages compile to a single artifact. So the aws-lambda-builders allows you to specify the artifacts_dir on where the artifacts are placed. For Python, this is the customers function code with all the dependencies laid out how Lambda expects. On the other hand, Go only has the binary in the artifacts_dir. For Go (both the Dep and Modules workflows), you can control the binary name by passing {artifact_executable_name: binary_name} into the options dictionary (which should match the handler name in the template).

packages/@aws-cdk/aws-lambda/lib/languages/jvm.ts
}
export interface JvmHandler {
className: string;
methodName: string;
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 provide a default for methodName, why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

handle seems reasonable - that's what the docs usually use i think.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

packages/@aws-cdk/aws-lambda/lib/languages/lambda-builder.ts
import { CfnFunction } from '../lambda.generated';
import { Runtime } from '../runtime';

interface LambdaBuilderRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

reference to where this spec comes from

packages/aws-cdk/lib/assets.ts
debug('Building project asset:', asset.codePath);

// working directory within container
const wd = '/tmp/cdk.out/';
Copy link
Contributor

Choose a reason for hiding this comment

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

I would mount to /var/code or something like that, not /tmp

packages/aws-cdk/lib/assets.ts
async function prepareZipAsset(asset: FileAssetMetadataEntry, toolkitInfo: ToolkitInfo): Promise<CloudFormation.Parameter[]> {
debug('Preparing zip asset from directory:', asset.path);
const staging = await fs.mkdtemp(path.join(os.tmpdir(), 'cdk-assets'));
// const staging = await fs.mkdtemp(path.join(os.tmpdir(), 'cdk-assets'));
Copy link
Contributor

Choose a reason for hiding this comment

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

?

packages/aws-cdk/lib/os.ts
child.once('error', reject);
child.once('error', (err) => {
debug('error', err);
reject(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should result in an exception so not sure debug is needed before, but that's fine

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 added it as a debug, will remove.

jfuss
jfuss reviewed Feb 26, 2019
View reviewed changes
Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

A side question I have is, when would build happen in the CDK workflow? Synth? Deploy? I ask because of the integrations between SAM CLI and CDK that were recently done. If we wanted to allow customers to debug and run their functions locally (through SAM CLI), we would need the building to happen during synth. Otherwise the function code is incomplete and not runnable due to all dependencies for a function being available during invoke.

packages/@aws-cdk/aws-lambda/lib/languages/lambda-builder.ts
}

public bind(construct: cdk.Construct) {
// If the same AssetCode is used multiple times, retain only the first instantiation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not super familiar with the inner workings of CDK so excuse my naive-ness here.

If you are building many functions, won't this place everything into the same artifacts_dir without any way to untangle the directory? I am not sure if you want this retained over different builds. aws-lambda-builders will place the artifact into into artifacts_dir. This is the isolation model for a function. So if you are building two Go functions, the artifacts_dir will have two binaries. Instead, you will want to create sub directories in some model CDK understands. This will allow you to map back artifacts to functions without clobbering them all together.

packages/@aws-cdk/aws-lambda/lib/languages/lambda-builder.ts
public bind(construct: cdk.Construct) {
// If the same AssetCode is used multiple times, retain only the first instantiation.
if (!this.asset) {
const mount = '/tmp/cdk.out/';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the directory that will mounted into the container? Do you need to handle windows paths here?

Be careful with /tmp directories on the host. These are not shared by default with docker and has caused some pain with SAM CLI customers (especially on Windows).

If you are bind mounting in 'ro' mode (like SAM CLI does), you will need to be aware that Go likes to write files into the source directory on the container. There is a change I am working on to copy code instead of bind mount to get around these. We are looking at this as an alternative/fall back for when bind mounts fail. We have seen the mount fail on Windows when using VPNs. Something to keep in mind while making design decisions here.

packages/@aws-cdk/aws-lambda/lib/languages/node.ts

export class NodeVersion {
public static readonly NodeJS = new NodeVersion(Runtime.NodeJS);
public static readonly NodeJS43 = new NodeVersion(Runtime.NodeJS43);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is probably not worth adding support for Node4.3 here as it is depreciated on Lambda: https://docs.aws.amazon.com/lambda/latest/dg/runtime-support-policy.html

packages/aws-cdk/lib/assets.ts
'docker', 'run',
...(asset.stdin !== undefined ? ['-i'] : []),
'-w', wd,
'--mount', `type=bind,source=${asset.codePath},target=${wd}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Mounts default to be writable back to the host system. Is that what you want to do?

@sam-goodwin sam-goodwin mentioned this pull request Mar 7, 2019
Closed
@sam-goodwin sam-goodwin closed this Apr 1, 2019
@amcdnl
Copy link

amcdnl commented Jul 30, 2019

Is there any docs or example repos for this?

@RomainMuller RomainMuller deleted the samgood/build-asset branch August 10, 2019 00:38
@jfuss jfuss mentioned this pull request Apr 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfuss jfuss jfuss left review comments

@eladb eladb eladb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lambda "build" assets
4 participants
@sam-goodwin @amcdnl @eladb @jfuss