fix(lambda-nodejs): docker build container attempts to use unsupported pnpm version for node runtime

[0xdevalias]

fixes #25710

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

When running the CDK build, it used the Docker build container, which tried to use a version of pnpm that doesn't support running on Node 12.x, producing the following error:

..snip..

 > [ 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.

..snip..

• https://r.pnpm.io/comp

This PR introduces the PNPM_VERSION ARG into the Dockerfile used for bundling (following existing CDK patterns used to inject the ESBUILD_VERSION ARG), which defines a switch/case of pnpm versions that should work with the corresponding Runtime.NODEJS_* versions.

You can see far more details of the error + working the proposed solution in:

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

Previous PRs (potentially non-exhaustive) related to the pnpm version + runtime compatibility are:

• chore(lambda-nodejs): pin pnpm version to 7 #24837
• fix(lambda-nodejs): pnpm no longer supports nodejs14.x #24821
• feat(lambda-nodejs): pnpm support #14772

TODO

• add/update any relevant unit and/or integration tests
  • see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-3-work-your-magic
  • I theorise on what some useful tests might be that would have caught/prevented this in the following comment (Ref)

---

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

[gitpod-io]

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[vinayak-kukreja]

Hey, is there a reason you want to run node 12?

[0xdevalias]

is there a reason you want to run node 12

@vinayak-kukreja Not overly, this fixes a bug/framework deficiency I noted as part of an upgrade/modernisation of a legacy stack.

But as was pointed out to me on the original issue I opened:

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

Originally posted by @peterwoodworth in #25710 (comment)

As I see it, the 2 paths forward are either fully deprecate/drop all support for Node 12.x, which as noted above (if correct), isn't a viable path right now; or alternately, make a small change to the existing code that allows it to continue to work bug free (the approach I took here), until the point that it is deemed fully unsupported and dropped entirely.

As things are currently, a stack that uses Node 12.x will fail to build when the docker build container is invoked, due to a pnpm version that doesn't support Node 12.x

---

This is the only part that needs to be finished off for this to be ready to go I believe:

• add/update any relevant unit and/or integration tests

• see main/CONTRIBUTING.md#step-3-work-your-magic
• I theorise on what some useful tests might be that would have caught/prevented this in the following comment (Ref)

I'd need to look deeper into those existing tests again to be sure of the best way to write one that would have identified this issue ahead of time + ensured it never broke in the first place though. Would seemingly have to be an actual run of the build container though, not just a mock.. so not sure how viable/desirable that is in the current test suite.

It would be good to get some guidance as to the best/most acceptable way to add some tests around this.

[0xdevalias]

Clarification Request

[vinayak-kukreja]

Hey, thank you for clarifying. It makes sense to me.

For unit tests, you can take a look at tests in this file.

And, for integration tests, you can take a look at DeployCommand and create a synth only integ test since stack deployment with Node 12 would fail.

Let me know if you have any questions.

[0xdevalias]

Let me know if you have any questions.

@vinayak-kukreja I guess my main question RE: tests was around the nuances of my musings here:

• https://github.com/aws/aws-cdk/blob/v2.80.0/packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts#L81-L101
  • Adding a test for Node 12.x here might be a useful way to surface this error, and catch any future regressions. It'd probably be worth doing this for all versions that are considered 'supported'.
  • Though I guess that test may not actually be running the build itself.. just testing that the right command is output.. so it might not be sufficient for this..
• https://github.com/aws/aws-cdk/blob/v2.80.0/packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts#L249-L258
  • This test looks like it's actually running the bundle, and expecting an error to be thrown.. so perhaps similar could be done to run it within the docker container and expect no error to be thrown?

Originally posted by @0xdevalias in #25710 (comment)

As well as this part:

I'd need to look deeper into those existing tests again to be sure of the best way to write one that would have identified this issue ahead of time + ensured it never broke in the first place though. Would seemingly have to be an actual run of the build container though, not just a mock.. so not sure how viable/desirable that is in the current test suite.

It would be good to get some guidance as to the best/most acceptable way to add some tests around this.

Originally posted by @0xdevalias in #25731 (comment)

@vinayak-kukreja Basically.. it feels like there is only sort of limited usefulness that I could get out of implementing unit tests with mocks/etc around this (eg. I could probably ensure that the version supplied is within the expected range for support I suppose, but it's still basically only testing the things I have already added in my code), whereas what feels like a 'proper' useful test would be to actually run the docker build itself, for each of the supported versions of Node, and then not see the failure anymore.

The clarity I was seeking (before I put the effort into writing those tests) is whether running a full proper docker build container for each supported version of node as part of these tests would be too 'heavy' to be accepted/acceptable.

Most of the existing tests within https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts are just checking that the appropriate function was called with the appropriate parameters (eg. Ref, which never would have surfaced the original error since the container wasn't actually being run.

For just checking that the params I define in the code get passed through appropriately for the given node version I could use a pattern like this one (Ref)

But re-skimming through https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts I don't see any examples where the docker build container is actually run, for us to then be able to see it error. For that part of it, is that where you were suggesting writing the integration tests with DeployCommand?

And, for integration tests, you can take a look at DeployCommand and create a synth only integ test since stack deployment with Node 12 would fail.

If so, can you recommend any good examples of using DeployCommand for something similar to this? (I haven't actually searched the existing code myself to try and find any yet)

---

Edit:

Looking at the other unit tests here, it looks like the ones in docker.test.ts may actually run the docker container, so perhaps I can use something from there (eg. Ref):

• https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda-nodejs/test/docker.test.ts

It references the Dockerfile from here:

• https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda-nodejs/lib/Dockerfile

Which defines the image to use as:

aws-cdk/packages/aws-cdk-lib/aws-lambda-nodejs/lib/Dockerfile

Lines 1 to 4 in 830e6d3

	# The correct AWS SAM build image based on the runtime of the function will be
	# passed as build arg. The default allows to do `docker build .` when testing.
	ARG IMAGE=public.ecr.aws/sam/build-nodejs18.x
	FROM $IMAGE

So adding some tests in docker.test.ts that tell it to use the appropriate image for each supported node runtime + try and run a pnpm build should probably trigger the original error before this fix was implemented.

Then that means I could continue to just have the tests in bundling.test.ts ensure that the correct docker args / pnpm versions would be output for the various node runtimes as per the logic of the fixes in this PR.

The pnpm tests in package-manager.test.ts seem like they're more related to bundling outside of docker, so probably don't need to do anything with them:

• https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda-nodejs/test/package-manager.test.ts#L36-L49

---

Edit 2:

Looking a little deeper, I came across these old remnants of integration tests:

• https://github.com/aws/aws-cdk/tree/main/packages/aws-cdk-lib/aws-lambda-nodejs/test/deactivated-integ-tests
  • https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda-nodejs/test/deactivated-integ-tests/dependencies-pnpm.ts
    • chore: remodel the aws-cdk repo to support v2 development only #24376

      • Removed all of the integration tests and put them into their own package to avoid cyclic dependencies. Integ tests for alpha packages remain with their package source.

    • https://github.com/aws/aws-cdk/tree/main/packages/aws-cdk-lib/aws-lambda-nodejs/test/integ-handlers/pnpm

Which seem like they have been moved here since #24376:

• https://github.com/aws/aws-cdk/tree/main/packages/%40aws-cdk-testing/framework-integ/test/aws-lambda-nodejs
  • https://github.com/aws/aws-cdk/tree/main/packages/%40aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test
    • https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.dependencies-pnpm.ts

@vinayak-kukreja Looking at that, it seems I could use integ.dependencies-pnpm.ts as a basis for implementing the integration test; though I wonder if I should just add additional cases to that file (since it's all technically pnpm related), or if a new standalone file would make more sense?

This integration test seems to give a pattern of testing multiple stack variations within the same integration test file, so maybe that would also be helpful for solving this:

• https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.modules.ts

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 3a1a238
• Result: FAILED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[aws-cdk-automation]

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

[aws-cdk-automation]

The pull request linter fails with the following errors:

❌ Fixes must contain a change to a test file.
❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

[0xdevalias]

sigh closed for lack of being able to add a couple of brief tests when the bulk of the work/fix is already done... what a waste.