aws-cdk: separate dependencies from devDependencies to work with pnpm

[aphex]

Describe the bug

When using the CDK with pnpm it is prone to have package hoisting issues as all the packages are declared as devDependencies not dependencies. Looking at

https://unpkg.com/[email protected]/package.json

I am seeing all dependencies have been moved to devDependencies however in the code itself it looks like there are normal dependencies (https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk/package.json#L94).

When these are only declared as dev pnpm is not hoisting the dependency (as it is not declared as one that is needed to use the package), which causes aws-cdk to load [email protected] instead of [email protected] if there is another package in the monorepo that has a hard dependency on [email protected].

Expected Behavior

Dependencies and devDependencies should be maintained in the released npm package indicating packages needed to use the cdk vs packages just needed when developing it.

Current Behavior

All dependencies are merged into devDependencies when released

Reproduction Steps

Create a pnpm workspace where the main package.json depends on lint-staged as well as aws-cdk. Attmepting to use the cdk will result in yaml errors when trying to use yaml/types for example.

Possible Solution

Maintain dependencies when released.

Additional Information/Context

Workaround for this is to add the following to your pnpm config in paclage.json

"aws-cdk": {
  "dependencies": {
    "yaml": "1.10.2"
  }
}

CDK CLI Version

n/a

Framework Version

No response

Node.js Version

16

OS

WSL/Ubuntu

Language

Typescript

Language Version

No response

Other information

No response

[alistairstead]

I don't understand this being reclassified as a feature-request? The dependencies of the package are defined in a way that causes runtime errors when using package managers that do not hoist dependencies.

Even outside of how package managers work, surely this is a defect, not a feature request?

[corymhall]

Attmepting to use the cdk will result in yaml errors when trying to use yaml/types for example.

@aphex do you mean executing the CLI (i.e. cdk deploy) throws the error? Or are you importing aws-cdk somewhere in your code?

[mrgrain]

FYI the published version of aws-cdk is bundling all its dependencies and should not attempt to use any other deps. If it does, that's certainly undesired behavior.

[aphex]

@corymhall this came up while using SST (https://sst.dev/) with pnpm. It is using the CDK we are using it and liny-staged this combination seemed to cause a mix up with the yaml package where aws-cdk was getting linked up to v2 of the yaml package and not v1.

@mrgrain we were definitely getting a runtime error from the aws-cdk package trying to load the yaml package so maybe something is off in bundling there. That would make more sense on why the dependencies are moved.

[mrgrain]

@aphex I can't reproduce this with the instructions you've provided. Can you please setup a repo that reproduced the issue?

However, I strongly suspect however SST is using CDK is not something that we are currently aware of and thus likely don't support. Not that we strictly wouldn't but if we don't know what they are doing.... You might want to talk to them.

package.json

{
  "name": "pnpm-test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "cdk": "cdk"
  },
  "keywords": [],
  "author": "",
  "license": "MIT",
  "dependencies": {
    "aws-cdk-lib": "^2.53.0",
    "constructs": "^10.0.0"
  },
  "devDependencies": {
    "@types/node": "^18.11.11",
    "aws-cdk": "^2.53.0",
    "lint-staged": "^13.1.0",
    "ts-node": "^10.9.1",
    "typescript": "^4.9.3"
  }
}

app.ts

import * as cdk from "aws-cdk-lib";
import * as s3 from "aws-cdk-lib/aws-s3";

const app = new cdk.App();
const stack = new cdk.Stack(app);
new s3.Bucket(stack, 'MyBucket');
app.synth();

Running

pnpm cdk synth --app "ts-node app.ts"

[github-actions]

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

[aphex]

bump, I will try to get to a minimal example of this as soon as I can.

[douddle]

Hello
I'm crossposting this as I think both are linked:
aws/aws-sdk-js-v3#4264 (comment)

[mrgrain]

Thanks @douddle! Has anyone checked if there's an open issue with pnpm yet? I find it notable that this issues (apparently) started to occur fairly recently and independently in two different libraries. Both times it appears to be an issue with bundled dependencies.

[aphex]

@mrgrain sorry for the delay on this, holidays always pile everything on. I think I have a very minimal example showcase the issue of bundling.

This assumes you have node/pnpm installed. I am running node 16.17.0 and pnpm 7.18.2

Step 1: Project init

mkdir aws-cdk-23182
cd aws-cdk-23182
pnpm init
pnpm i aws-cdk
pnpm i yaml

Step 2: Change to module type
Edit package.json
add "type": "module",

Step 3: Create test
edit index.js

import { serialize } from 'aws-cdk/lib/util/yaml-cfn.js'

console.log(serialize)

Step 3: Run test

node .

Step 4: Expected Error

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './types' is not defined by "exports"

Step 5: Update package json
Edit package.json add

"pnpm": {
    "packageExtensions": {
      "aws-cdk": {
        "dependencies": {
          "yaml": "1.10.2"
        }
      }
    }
  }

Step 6: Kill and start over

pnpx npkill
pnpm i

Step 7: Run again

node .

Step 8: 🥳

Not totally following the bundle mentality for this package, so hoping you can help me understand. Is it simply the thought that tree shaking will make this a smaller install in ci environments? As package managers generally install all the devDependencies anyway I don't think there is any saving during dev, if anything its more as we download bundled code and then potentially all the devDependencies anyway. Hope this all makes sense, please let me know if there is more detail I can provide!

[mrgrain]

No worries! I feel you with the Christmas time 😅

Thanks for the repro. That will be super helpful. I guess the "type": "module" is necessary for this issue to manifest? Seems like a candidate for things to go wrong. But I will have a look.

---

The bundling strategy is driven from a supply chain security perspective. Every released CLI package will work within the tested parameters, no matter if a dependency has been compromised or vandalized.

[aphex]

@mrgrain Not sure about the module type, I didn't try it without but it's just what is being used in our projects. Though seeing that yaml-cfn.js is requiring yaml/types in code I would think we could end up in the same spot as pnpm is not adding yaml@1 to the mix at all.

Thanks for the bundle explanation, makes sense with issues like node-ipc or even leftpad. Though I wonder now if things are actually bundling as expected seeing that this package is reaching out into my node_modules.

However I would think pinning dependencies would also get you a similar result? If I remember correctly you cannot overwrite npm versions anymore, you can unpublish and republish but this has time restrictions, and unpublish after 72 hours requires manual intervention.

The security downside to this strategy is that users are not able to do patch/minor updates to dependent packages that have security fixes. So now its the responsibility of the aws-cdk package to ensure all its packages are updated to clear up any potential security issues that have been addressed.

[zackheil]

Might be worth noting that this is an issue for me with npm as a package manager as well (using the same sort of repro code). npm would never install the yaml package as it is nowhere in the dependency list for aws-cdk and instead gets "downgraded" to dev dependency.

[aphex]

@zackheil that is an odd one, any chance you have a ENV laying around that might make npm think its production only time? Or maybe this situation is when you do a ci or production build?

Either way this is a great point as it is totally possible this works fine in dev mode (when devDependencies are installed) but fails in a CI or production environment

ref:
https://docs.npmjs.com/cli/v9/commands/npm-install#:~:text=With%20the%20%2D%2Dproduction%20flag

[mrgrain]

FYI I'm on annual leave and will need more time to get to the bottom of some questions for this.

The solution likely won't be to stop bundling (at least on the short term, and I appreciate the feedback on it), but sorting out the dependencies in package.json and/or the bundle itself.

[rix0rrr]

The issue is here:

import { serialize } from 'aws-cdk/lib/util/yaml-cfn.js'

This was never actually supported. Some people did it, and it worked for them. We then moved to bundling the CLI sources, and left the original source in place to not break people for whom this happened to work. (That does not include making it work for new people!)

But the CLI package's contract is that it can be used as a CLI -- that's it.

The fact that the CLI package doesn't advertise a "main" entry point in the package.json (meaning you can't do import * as cli from 'aws-cdk'; or require('aws-cdk')) should tell you that it is not intended to be used this way.

[thdxr]

Chiming in here as I work on SST and wrote the code that is impacted by this. We solved this for our users by making these packages direct dependencies of our package.

For everyone else in this thread, there's plenty of issues across this repo where AWS has made it clear they're not interested in supporting programmatic access (for example #20196) . We made the decision at SST to take the risk and call into it programmatically anyway so keeping it working is probably our responsibility.

The reason this is worth it for us is there are a lot of performance benefits - which are important because CDK has many performance issues to begin with so every little improvement makes a meaningful difference.

I will say the approach the CDK project takes to bundling feels complicated and non-standard. Is there really a good reason to deviate from how 99% of other npm packages work?

The security concern seems odd to me unless I'm misunderstanding as you can pin versions - it only protects against compromising the immutable constraints of npm - which CDK itself is still subject to. Very few projects seem to opt for this tradeoff.

[rix0rrr]

The reason this is worth it for us is there are a lot of performance benefits - which are important because CDK has many performance issues to begin with so every little improvement makes a meaningful difference.

Honestly surprising to me. Most of the time spent in a CDK operation will be in app synthesis, not in starting the CLI. Are you avoiding JVM cold start in some way by doing this?

I will say the approach the CDK project takes to bundling feels complicated and non-standard. Is there really a good reason to deviate from how 99% of other npm packages work?

You didn't specify, so I'm not quite sure what the 99% standard is you are referring to. Is it bundledDependencies ? If so, Yarn does not support bundledDependencies, and Yarn is popular enough as a package manager that we need to reckon with it.

The security concern seems odd to me unless I'm misunderstanding as you can pin versions - it only protects against compromising the immutable constraints of npm - which CDK itself is still subject to. Very few projects seem to opt for this tradeoff.

The NPM ecosystem supports pinning, but pinning happens at the wrong time. Version pinning doesn't happen when we publish the CLI, pinning happens when you install the CLI.

If we publish the CLI with a dependency:

// CDK CLI package.json
{ 
  "dependencies": {
    "our-dependency": "1.2.3" // Very conscientiously avoiding a ^ or ~ here
  }
}

// our-dependency package.json
{
  "dependencies": {
    "leftpad": "^2.3.16"  // latest greatest
  }
}

Then immediately after the moment we test against [email protected] and publish the CLI, a new version [email protected] may be released that can do anything. That's the version you will get. That new version can do anything. It may not work (in the bad case), or send all your AWS keys to a remote server (in the worst). There is nothing we can do to guard against that, except bundling the tested, known-good version of leftpad into our CLI so that it is not left up to your NPM and the state of the world what happens to your machine.

In other words, pinning works for website builders (people who have a package.json and a package-lock.json sitting in a directory somewhere, and ship a build of that directory to their servers to run there), not app vendors (people who publish a CLI to npmjs.com).

---

You might be tempted to say "aha, I know about npm-shrinkwrap.json!"... yes, that would have worked if it had been widely supported. I don't know if you remember the colors debacle from last year, but we were in fact vending a npm-shrinkwrap.json and it did not protect us.

We asked Yarn to reconsider their position, but they have plans to support shrinkwrapping, leaving us no other choice than to bundle.

[rix0rrr]

By the way, you should have a look at this PR: #22836

[aphex]

@rix0rrr thanks for the explanation and the details. I can understand why you all are trying to keep security a priority and package manipulation/tampering is clearly a concern for a CLI that deals with a companies full infrastructure.

However my original issue was with pnpm support for this package. Please correct me if I am wrong but I believe it does clean up some of your security concern by default as prefer-frozen-lockfile is the default and a user needs to opt into doing an install that would change versions in the lock file. Though I suppose one could argue that during initial install/create of a project (prior to lock existing) the nested package versions could drift from the AWS officially vetted hierarchy of dependencies.

Then again if the official stance is simply "using this package as a code dependency is not supported, it is meant to be a CLI only" then I guess that also ends this conversation. This does seem like a roadblock for the community though, clearly the code here is important and building on top of the aws-cdk allows for projects like SST to expand the AWS community, innovate and provide these abstractions.

I also noticed your team distributes a signed zip version of the CDK, I was thinking this seems like a great way to provide a fully secure option to those who want a completely locked down version of the CLI and not use this as a code library. Right now your team is using npm as a kind of "app store" for your cli and just disregarding the fact that most of the community uses it as a way to share code amongst the javascript community. Truth of the matter is anyone using the aws-cdk as a library within there application is likely using other packages as well that still would suffer from nested dependency tampering. They are taking on this risk regardless.

If the official choice from your team is to not officially support programmatic access to this package I guess our best bet is to provide workarounds in projects like SST for pnpm as mentioned here #23182 (comment) and take this on as a risk of a project expanding on the aws-cdk. The other option is to wait for the programmatic cli api mentioned above that is currently experimental.

@thdxr I have no doubt this will be a bit frustrating but if your team doesn't want to move towards the programmatic cli api maybe create-sst could be aware of the package manager used to launch it and write some pnpm extensions into the new projects package.json. I guess this could fall under the responsibility you mentioned in your comment reaching into a package that doesn't officially support code access. This is of course if SST wants to officially support pnpm, which in my opinion should be the only package manager people use anymore.

[thdxr]

@rix0rrr ah right forgot about the nested dependency scenario thanks for clarifying

Performance issues are challenging to list out here but I spent a few months profiling every place SST was slow and needed to reach into cdk directly to get the performance we wanted. It's actually still not what we consider good enough but it's a drastic improvement over spawning the CLI many times through the course of a single command

@aphex I'm fairly certain the latest rc of sst doesn't need that pnpm workaround, try removing it and seeing if it's still a problem

[mrgrain]

@thdxr and @aphex - putting the dependency issue aside, can you explain what your use case is to use serialize from aws-cdk/lib/util/yaml-cfn.js directly? It's not one we have really considered and programmatic CLI access wouldn't concern itself with that either.

[thdxr]

I think that was a minimal reproduction so not a real world scenario

let me explain what we're actually doing - it's fairly monstrous so don't judge us too hard!

we've forked the following two files from cdk: https://github.com/serverless-stack/sst/tree/sst2/packages/sst/src/cdk
and modified them slightly to suit our needs which involves programmatically uploading assets and deploying stacks

these files import directly from aws-cdk/lib/* and so we have to make sure we include any other dependencies they have in our package.json since some of them are removed at publish time (as the above conversation)

effectively we're importing modules that aren't meant to be imported since we want programmatic access to CDK functionality without forking the whole CDK project

[mrgrain]

@thdxr Thanks for the insight! Based on this, would #22836 address your needs? If not, could you let me know your requirements?

[thdxr]

that's definitely a step in the right direction, but we're not sure if just 1:1 access to CLI commands will be enough

we need some granular control over what happens (eg suppressing all console output, splitting up asset upload from deploying stacks, etc)

we have some specialized CI functionality in https://seed.run/ that coordinates these steps in an efficient way so simply running the CLI probably will lose significant performance

[mrgrain]

we need some granular control over what happens (eg suppressing all console output, splitting up asset upload from deploying stacks, etc)

I'd love to hear these granular control requirements, whenever they pop up for you. No promises obviously.
At least "suppressing all console output" has been firmly on the board for me as well.

[thdxr]

cool!

we're wrapping up SST v2 and SEED integration and once we've done that we should be able to look back and summarize what exactly we needed to do

[zvictor]

EDIT: it actually didn't work... 😌

For anyone looking for a quick workaround while this gets fixed, try adding this to your package.json (thanks @aphex!):

"pnpm": {
  "packageExtensions": {
    "aws-cdk": {
      "dependencies": {
        "yaml": "1.10.2"
      }
    }
  }
}

[RichiCoder1]

I just ran into both this and the ./types error separately and had to nudge npm in the right direction in order to resolve this as it was transitively picking up the wrong versions of both.

[mrgrain]

I just ran into both this and the ./types error separately and had to nudge npm in the right direction in order to resolve this as it was transitively picking up the wrong versions of both.

@RichiCoder1 Thanks for the report. Can you confirm if you were also using a subpath import like others in the issue? Possibly via a third-party library.

[thdxr]

the issue was from SST as well

we're getting an increasing number of people having issues from this so we're likely going to have to fork aws-cdk so we can make sure package.json is defined correctly

would really love not to do this