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

NodejsFunction: bundler fails with unclear error message when using node 12 runtime #25710

Open
0xdevalias opened this issue May 24, 2023 · 5 comments
Labels

Comments

@0xdevalias
Copy link
Contributor

0xdevalias commented May 24, 2023

Describe the bug

I'm in the process of upgrading from an old version of CDK 1.x, to the latest CDK 2.x (2.80.0).

I had some old NodejsFunction code that was using runtime: Runtime.NODEJS_12_X

Old code that caused error:

this.fnHandler = new NodejsFunction(this, 'Handler', {
  description: handlerDescription,
  entry: path.join(__dirname, '..', 'functions', handlerPath),
  handler: handlerExport,
  runtime: Runtime.NODEJS_12_X,
  memorySize: 128,
  timeout: Duration.seconds(60),
})

Expected Behavior

The framework would give me a more meaningful error message telling me what/where the issue was.

Current Behavior

⇒ yarn cdk-dev diff '*'
yarn run v1.22.15
$ yarn -s awsvault-dev:nosession cdk diff '*'
$ /Users/devalias/dev/REDACTED-infrastructure/node_modules/.bin/ts-node bin/REDACTED.ts
[+] Building 19.8s (11/13)
 => [internal] load .dockerignore                                                                                                                          0.0s
 => => transferring context: 2B                                                                                                                            0.0s
 => [internal] load build definition from Dockerfile                                                                                                       0.0s
 => => transferring dockerfile: 1.30kB                                                                                                                     0.0s
 => [internal] load metadata for public.ecr.aws/sam/build-nodejs12.x:latest                                                                                2.1s
 => [ 1/10] FROM public.ecr.aws/sam/build-nodejs12.x@sha256:4748f3e85ef16460a48f502433eec0b8282f374bd646553392e7dc27a1c20579                               0.0s
 => CACHED [ 2/10] RUN npm install --global yarn@1.22.5                                                                                                    0.0s
 => [ 3/10] RUN npm install --global pnpm@7.30.5                                                                                                           4.2s
 => [ 4/10] RUN npm install --global typescript                                                                                                            3.7s
 => [ 5/10] RUN npm install --global --unsafe-perm=true esbuild@0                                                                                          8.3s
 => [ 6/10] RUN mkdir /tmp/npm-cache &&     chmod -R 777 /tmp/npm-cache &&     npm config --global set cache /tmp/npm-cache                                0.5s
 => [ 7/10] RUN mkdir /tmp/yarn-cache &&     chmod -R 777 /tmp/yarn-cache &&     yarn config set cache-folder /tmp/yarn-cache                              0.7s
 => ERROR [ 8/10] RUN mkdir /tmp/pnpm-cache &&     chmod -R 777 /tmp/pnpm-cache &&     pnpm config --global set store-dir /tmp/pnpm-cache                  0.3s
------
 > [ 8/10] RUN mkdir /tmp/pnpm-cache &&     chmod -R 777 /tmp/pnpm-cache &&     pnpm config --global set store-dir /tmp/pnpm-cache:
#0 0.282 ERROR: This version of pnpm requires at least Node.js v14.6
#0 0.282 The current version of Node.js is v12.22.12
#0 0.282 Visit https://r.pnpm.io/comp to see the list of past pnpm versions with respective Node.js version support.
------
Dockerfile:31
--------------------
  30 |     # Ensure all users can write to pnpm cache
  31 | >>> RUN mkdir /tmp/pnpm-cache && \
  32 | >>>     chmod -R 777 /tmp/pnpm-cache && \
  33 | >>>     pnpm config --global set store-dir /tmp/pnpm-cache
  34 |
--------------------
ERROR: failed to solve: process "/bin/sh -c mkdir /tmp/pnpm-cache &&     chmod -R 777 /tmp/pnpm-cache &&     pnpm config --global set store-dir /tmp/pnpm-cache" did not complete successfully: exit code: 1

/Users/devalias/dev/REDACTED-infrastructure/node_modules/aws-cdk-lib/core/lib/private/asset-staging.js:2
`).map((line,idx)=>`${idx===0?firstLine:padding}${line}`)};const reason=proc.signal!=null?`signal ${proc.signal}`:`status ${proc.status}`,command=[prog,...args.map(arg=>/[^a-z0-9_-]/i.test(arg)?JSON.stringify(arg):arg)].join(" ");throw new Error([`${prog} exited with ${reason}`,...prependLines("--> STDOUT:  ",proc.stdout)??[],...prependLines("--> STDERR:  ",proc.stderr)??[],`--> Command: ${command}`].join(`
                                                                                                                                                                                                                                            ^
Error: docker exited with status 1
--> Command: docker build -t cdk-56873b35ad5918ca4be680982cd873a337a6409face1e9609c1043d328c6fd66 --platform "linux/amd64" --build-arg "IMAGE=public.ecr.aws/sam/build-nodejs12.x" --build-arg "ESBUILD_VERSION=0" "/Users/devalias/dev/REDACTED-infrastructure/node_modules/aws-cdk-lib/aws-lambda-nodejs/lib"
    at dockerExec (/Users/devalias/dev/REDACTED-infrastructure/node_modules/aws-cdk-lib/core/lib/private/asset-staging.js:2:237)
    at Function.fromBuild (/Users/devalias/dev/REDACTED-infrastructure/node_modules/aws-cdk-lib/core/lib/bundling.js:1:4085)
    at new Bundling (/Users/devalias/dev/REDACTED-infrastructure/node_modules/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.js:1:2315)
    at Function.bundle (/Users/devalias/dev/REDACTED-infrastructure/node_modules/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.js:1:599)
    at new NodejsFunction (/Users/devalias/dev/REDACTED-infrastructure/node_modules/aws-cdk-lib/aws-lambda-nodejs/lib/function.js:1:1310)
    at new CustomNodeLambdaEventHandler (/Users/devalias/dev/REDACTED-infrastructure/lib/constructs/CustomNodeLambdaEventHandler.ts:44:22)
    at new REDACTEDEventStack (/Users/devalias/dev/REDACTED-infrastructure/lib/REDACTED-event-stack.ts:100:43)
    at Object.<anonymous> (/Users/devalias/dev/REDACTED-infrastructure/bin/REDACTED.ts:46:30)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Module.m._compile (/Users/devalias/dev/REDACTED-infrastructure/node_modules/ts-node/src/index.ts:837:23)
error Command failed with exit code 1.

Reproduction Steps

See above

Possible Solution

Throw a more useful error/warning at the CDK framework level that describes the issue, rather than it being surfaced through arbitrary warnings within a related docker build container.

Workaround

Manually figure out that the error relates to using runtime: Runtime.NODEJS_12_X, find it in your code, and update it to something supported such as runtime: Runtime.NODEJS_14_X

Additional Information/Context

N/A

CDK CLI Version

2.80.0 (build bbdb16a)

Framework Version

2.80.0

Node.js Version

v16.15.1

OS

macOS Ventura 13.3.1

Language

Typescript

Language Version

No response

Other information

N/A

@0xdevalias 0xdevalias added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 24, 2023
@peterwoodworth
Copy link
Contributor

The node 12 runtime option is marked as deprecated in CDK, but it's still a valid configuration for functions that were created on node 12, so we can't just throw an error if someone uses node 12. I'm not sure how we can throw a clear warning message in this exact case of container failure either, that could be pretty tricky. Thanks for letting us know this could be clearer

@peterwoodworth peterwoodworth added p2 and removed needs-triage This issue or PR still needs to be triaged. labels May 24, 2023
@peterwoodworth peterwoodworth changed the title NodejsFunction: ERROR: This version of pnpm requires at least Node.js v14.6, The current version of Node.js is v12.22.12 NodejsFunction: bundler fails with unclear error message when using node 12 runtime May 24, 2023
@0xdevalias
Copy link
Contributor Author

0xdevalias commented May 24, 2023

The node 12 runtime option is marked as deprecated in CDK, but it's still a valid configuration for functions that were created on node 12, so we can't just throw an error if someone uses node 12.

@peterwoodworth Yeah, that's fair enough; though I guess in this case, when CDK chooses to use the container to build the function, it's failing outright.. so perhaps that is an error in the container maybe then? I'm not really sure of the internals of it all, but the fact that a Node 12.x build container is failing with a "requires node 14.x+ error seems like a bug to me.

I'm not sure if that bug is in the build container itself, or in something that CDK is passing to that container. If the former, it seems like a bug to be raised upstream with the build container repo. If it's something that CDK is passing to the build container, then either it should be fixed up so it doesn't cause the error (probably the ideal case, since you say Node 12.x is still supported for older functions), or marked that after version 2.XX of the CDK, Node 12.x functions will no longer be supported when using a docker build container.

I'm not sure how we can throw a clear warning message in this exact case of container failure either, that could be pretty tricky.

@peterwoodworth Mm, fair enough. Not too sure of the specifics on that side of things, so can't provide any further guidance.

@0xdevalias
Copy link
Contributor Author

0xdevalias commented May 24, 2023

The following docs/references seem relevant here:

@0xdevalias
Copy link
Contributor Author

0xdevalias commented May 25, 2023

Looking deeper at this we can see that the Dockerfile passes in an ARG for the esbuild version:

# Install esbuild
# (unsafe-perm because esbuild has a postinstall script)
ARG ESBUILD_VERSION=0
RUN npm install --global --unsafe-perm=true esbuild@$ESBUILD_VERSION

This is then passed in from cdk.DockerImage.fromBuild in packages/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.ts:

const shouldBuildImage = props.forceDockerBundling || !Bundling.esbuildInstallation;
this.image = shouldBuildImage ? props.dockerImage ?? cdk.DockerImage.fromBuild(path.join(__dirname, '../lib'),
{
buildArgs: {
...props.buildArgs ?? {},
// If runtime isn't passed use regional default, lowest common denominator is node14
IMAGE: (props.runtime ?? Runtime.NODEJS_14_X).bundlingImage.image,
ESBUILD_VERSION: props.esbuildVersion ?? ESBUILD_MAJOR_VERSION,
},
platform: props.architecture.dockerPlatform,
})
: cdk.DockerImage.fromRegistry('dummy'); // Do not build if we don't need to

While that part isn't being used to switch based on the Runtime, it demonstrates an existing pattern for passing in a version from the bundling code.

Just above it, we can see that the IMAGE is using Runtime.NODEJS_14_X as part of it's default.

Based on these 2 patterns, we could easily derive a 'use the correct/supported pnpm version for the runtime' pattern. It could look something like this:

const runtime = props.runtime ?? Runtime.NODEJS_14_X

const pnpmVersion = ((runtime) => {
  switch (runtime) {
    case Runtime.NODEJS_4_3:
      return '^1.27.0-1'
    case Runtime.NODEJS_6_10:
      return '^2.24.0-0'
    case Runtime.NODEJS_8_10:
      return '^3.8.1'
    case Runtime.NODEJS_10_X:
      return '^5.18.10'
    case Runtime.NODEJS_12_X:
      return '^6.35.1'
    case Runtime.NODEJS_14_X:
      return '^7.32.5'
    default:
      return '^8.5.1'
  }
})(runtime)

Then in the cdk.DockerImage.fromBuild:

// Docker bundling
const shouldBuildImage = props.forceDockerBundling || !Bundling.esbuildInstallation;
this.image = shouldBuildImage ? props.dockerImage ?? cdk.DockerImage.fromBuild(path.join(__dirname, '../lib'),
  {
    buildArgs: {
      ...props.buildArgs ?? {},
      // If runtime isn't passed use regional default, lowest common denominator is node14
-      IMAGE: (props.runtime ?? Runtime.NODEJS_14_X).bundlingImage.image,
+      IMAGE: runtime.bundlingImage.image, 
      ESBUILD_VERSION: props.esbuildVersion ?? ESBUILD_MAJOR_VERSION,
+     PNPM_VERSION: pnpmVersion 
    },
    platform: props.architecture.dockerPlatform,
  })
  : cdk.DockerImage.fromRegistry('dummy'); // Do not build if we don't need to

Then finally, in the Dockerfile:

# Install pnpm
- RUN npm install --global pnpm@7.30.5
+ ARG PNPM_VERSION=0
+ RUN npm install --global pnpm@$PNPM_VERSION

We can see the changelog's for various versions dropping pnpm support here:

And we can get the latest version of each major release here:

As new major versions are released the default case in the example code can be updated, or new case statements can be added as those new versions drop support for other runtimes.

Details on using the ^ prefix to get the latest minor/patch version of the dependency (rather than pinning to an exact version):

Though if you want to pin to the exact latest versions at time of writing, just strip the leading ^ from the example code above.

@0xdevalias
Copy link
Contributor Author

I've submitted a PR with the above proposed solution here:

0xdevalias added a commit to 0xdevalias/aws-cdk that referenced this issue Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants