GoogleCloudPlatform / buildpacks

Builders and buildpacks designed to run on Google Cloud's container platforms
Apache License 2.0
991 stars 147 forks source link

Don't set GOOGLE_EXPERIMENTAL_NODEJS_NPM_BUILD_ENABLED by default #290

Closed alexnault closed 1 year ago

alexnault commented 1 year ago

We've suddenly started seeing GOOGLE_EXPERIMENTAL_NODEJS_NPM_BUILD_ENABLED yesterday. Since then, Cloud Build w/ GAE has started running with NODE_ENV=development and caused npm run build to execute by default (breaking changes).

This commit seems to be the cause.

@anniefu How can we revert back/disable this behavior?

Build logs

``` Pulling image: us.gcr.io/gae-runtimes/buildpacks/google-gae-18/nodejs/builder:nodejs_20230417_RC00 nodejs_20230417_RC00: Pulling from gae-runtimes/buildpacks/google-gae-18/nodejs/builder 53e5e158da5a: Already exists 7577c9c60d3f: Already exists 3c2cba919283: Already exists 9eca6d02dcc5: Already exists 29cb268e5dbb: Already exists 458389fe2563: Already exists da4189bee6c8: Already exists 3db879c9bca7: Already exists 36e4468bec21: Pulling fs layer 959d7410f226: Pulling fs layer b84b29d601e1: Pulling fs layer 085da138c6a1: Pulling fs layer ef0f779a74e0: Pulling fs layer 06f428a8446f: Pulling fs layer 682fcfb7094c: Pulling fs layer aee114829cdb: Pulling fs layer 81eefa1bdfb5: Pulling fs layer e628db1aeec0: Pulling fs layer dced04efdb1a: Pulling fs layer 90fbe208248b: Pulling fs layer 1c7cd3add17c: Pulling fs layer ba710ad9381a: Pulling fs layer c60377f6894d: Pulling fs layer 3ba6fda60b47: Pulling fs layer 1d6d9c88fb34: Pulling fs layer fe4ee7f11b40: Pulling fs layer e7a09445ee13: Pulling fs layer 4f4fb700ef54: Pulling fs layer 085da138c6a1: Waiting ef0f779a74e0: Waiting 06f428a8446f: Waiting 682fcfb7094c: Waiting aee114829cdb: Waiting 81eefa1bdfb5: Waiting e628db1aeec0: Waiting dced04efdb1a: Waiting 90fbe208248b: Waiting 1c7cd3add17c: Waiting ba710ad9381a: Waiting c60377f6894d: Waiting 3ba6fda60b47: Waiting 1d6d9c88fb34: Waiting fe4ee7f11b40: Waiting 4f4fb700ef54: Waiting e7a09445ee13: Waiting 36e4468bec21: Verifying Checksum 36e4468bec21: Download complete b84b29d601e1: Download complete 959d7410f226: Verifying Checksum 959d7410f226: Download complete ef0f779a74e0: Verifying Checksum ef0f779a74e0: Download complete 06f428a8446f: Verifying Checksum 06f428a8446f: Download complete 36e4468bec21: Pull complete 085da138c6a1: Download complete 959d7410f226: Pull complete aee114829cdb: Verifying Checksum aee114829cdb: Download complete 81eefa1bdfb5: Verifying Checksum 81eefa1bdfb5: Download complete 682fcfb7094c: Verifying Checksum 682fcfb7094c: Download complete b84b29d601e1: Pull complete e628db1aeec0: Verifying Checksum e628db1aeec0: Download complete dced04efdb1a: Download complete 90fbe208248b: Verifying Checksum 90fbe208248b: Download complete 085da138c6a1: Pull complete 1c7cd3add17c: Download complete c60377f6894d: Verifying Checksum c60377f6894d: Download complete ef0f779a74e0: Pull complete ba710ad9381a: Verifying Checksum ba710ad9381a: Download complete fe4ee7f11b40: Verifying Checksum fe4ee7f11b40: Download complete 06f428a8446f: Pull complete 3ba6fda60b47: Verifying Checksum 3ba6fda60b47: Download complete 1d6d9c88fb34: Verifying Checksum 1d6d9c88fb34: Download complete e7a09445ee13: Verifying Checksum e7a09445ee13: Download complete 4f4fb700ef54: Verifying Checksum 4f4fb700ef54: Download complete 682fcfb7094c: Pull complete aee114829cdb: Pull complete 81eefa1bdfb5: Pull complete e628db1aeec0: Pull complete dced04efdb1a: Pull complete 90fbe208248b: Pull complete 1c7cd3add17c: Pull complete ba710ad9381a: Pull complete c60377f6894d: Pull complete 3ba6fda60b47: Pull complete 1d6d9c88fb34: Pull complete fe4ee7f11b40: Pull complete e7a09445ee13: Pull complete 4f4fb700ef54: Pull complete Digest: sha256:603207502a99d0f5b3f615db0a0289a961d9c3f6fdd090dcad4a5077379bd230 Status: Downloaded newer image for us.gcr.io/gae-runtimes/buildpacks/google-gae-18/nodejs/builder:nodejs_20230417_RC00 us.gcr.io/gae-runtimes/buildpacks/google-gae-18/nodejs/builder:nodejs_20230417_RC00 ===> ANALYZING Previous image with name "us.gcr.io/b12-staging-2/app-engine-tmp/app/frontend-gcs/ttl-18h:ce951a65-286b-4471-93e4-9197270202ae" not found ===> DETECTING google.nodejs.runtime 1.0.0 google.nodejs.npm 1.0.0 google.nodejs.appengine 0.9.0 google.utils.label-image 0.0.2 ===> RESTORING Restoring metadata for "google.nodejs.runtime:node" from cache Restoring metadata for "google.nodejs.npm:npm_modules" from cache Restoring data for "google.nodejs.runtime:node" from cache Restoring data for "google.nodejs.npm:npm_modules" from cache ===> BUILDING === Node.js - Runtime (google.nodejs.runtime@1.0.0) === Using runtime version from GOOGLE_RUNTIME_VERSION: 16.19.1 DEBUG: ***** CACHE HIT: "nodejs" Node.js v16.19.1 cache hit, skipping installation. Warning: BOM table is deprecated in this buildpack api version, though it remains supported for backwards compatibility. Buildpack authors should write BOM information to .sbom., launch.sbom., or build.sbom.. Warning: BOM table is deprecated in this buildpack api version, though it remains supported for backwards compatibility. Buildpack authors should write BOM information to .sbom., launch.sbom., or build.sbom.. Warning: BOM table is deprecated in this buildpack api version, though it remains supported for backwards compatibility. Buildpack authors should write BOM information to .sbom., launch.sbom., or build.sbom.. === Node.js - Npm (google.nodejs.npm@1.0.0) === -------------------------------------------------------------------------------- Running "node -v" v16.19.1 Done "node -v" (41.749213ms) DEBUG: Current dependency hash: "eaf02f554ec7e71e044f4fa9036222c393618e748c3932ac0ed4b41002bc8c80" DEBUG: Cache dependency hash: "d3a7b1a978a9d7900e53ec257802fe9e2e6435216dd791cdb26c1f64e008e7ae" DEBUG: ***** CACHE MISS: "npm_modules" Installing application dependencies. -------------------------------------------------------------------------------- Running "node -v" v16.19.1 Done "node -v" (7.066094ms) -------------------------------------------------------------------------------- Running "npm --version" 8.19.3 Done "npm --version" (427.512147ms) -------------------------------------------------------------------------------- Running "npm ci --quiet (NODE_ENV=development)" npm WARN skipping integrity check for git dependency ssh://git@github.com/kalmecak/moment-business-days.git npm WARN deprecated popper.js@1.16.1: You can find the new Popper v2 at @popperjs/core, this package is dedicated to the legacy v1 added 1392 packages, and audited 1393 packages in 57s 16 high severity vulnerabilities To address issues that do not require attention, run: npm audit fix To address all issues possible (including breaking changes), run: npm audit fix --force Some issues need review, and may require choosing a different dependency. Run `npm audit` for details. Done "npm ci --quiet (NODE_ENV=development)" (57.454913521s) -------------------------------------------------------------------------------- Running "cp --archive node_modules /layers/google.nodejs.npm/npm_modules/node_modules" Done "cp --archive node_modules /layers/google.nodejs.npm/npm_modu..." (8.845788982s) -------------------------------------------------------------------------------- Running "npm run build" > web@3.152.0 build > NODE_OPTIONS=--max-old-space-size=4096 vite build vite v4.1.1 building for production... transforming... ✓ 15110 modules transformed. rendering chunks... Killed Done "npm run build" (2m11.155397397s) NOTE: Running the default build script can be skipped by passing the empty environment variable `GOOGLE_NODE_RUN_SCRIPTS=` to the build. Failure: (ID: 1a2262f3) > web@3.152.0 build > NODE_OPTIONS=--max-old-space-size=4096 vite build vite v4.1.1 building for production... transforming... ✓ 15110 modules transformed. rendering chunks... Killed -------------------------------------------------------------------------------- Running "mv -f /builder/outputs/output-3370200833676305946 /builder/outputs/output" Done "mv -f /builder/outputs/output-3370200833676305946 /builder/o..." (90.744224ms) ERROR: failed to build: exit status 1 ```

anniefu commented 1 year ago

Hello, we made this change to support more Node.js applications types on various GCP platforms, though we recognize this may not work for everyone. There are a couple of options for workarounds:

elyobo commented 1 year ago

The additional functionality seems very useful, but the switch of default from "production dependencies, no build" to "dev dependencies, try to run npm run build" - shouldn't the new behaviour be opt in not opt out?

Edit: not noted in the GAE notes there is the change to npm ci behaviour, installing with NODE_ENV=development instead - is that automatically switched due to the presence of the build script as well? Can that be documented as such?

I picked this up last night working on a migration of CI platforms for one project and had assumed it was something I was doing wrong, but coming back this morning this has broken automatic deploy of several applications overnight due to the change in default behaviour :-/

elyobo commented 1 year ago

Setting the environment variable did not work (set via app.yaml); adding the gcp-build improved things, but it then attempts to run npm start with a development node env again (which makes our particular app fail because the launch behaviour is, unsurprisingly, different in development mode than in production 🤦 ).

Is there some additional work required tell everything to run in production mode as it used to?

These are node 18 apps running in app engine flex.

anniefu commented 1 year ago

We chose to make this the default behavior with the long term view that running npm run build is the more idiomatic behavior for Node.js users than our previous behavior of ignoring build scripts and requiring GCP-specific scripts or env vars. We did our best to try and announce the changes across the impacted platforms, though we apologize for the inconvenience.

By default, if the buildpack is going to run a script on behalf of the user, dependencies will be installed first. This is noted in https://cloud.google.com/docs/buildpacks/nodejs#executing_custom_build_steps_during_deployment

By default, when you configure custom build steps, both the dependencies and devDependencies in your package.json file are installed first before any scripts or commands are executed. To override the default behavior, you can use the NODE_ENV environment variable.

Using any of the workarounds to prevent running a script will prevent installing development dependencies too. As part of the recent changes, passing NODE_ENV=production will also be respected.

For App Engine in particular, note that setting environment variables actually uses a colon instead of an equals sign. Sorry, that's a bit confusing in the release notes.

app.yaml

runtime: nodejs18

build_env_variables:
  GOOGLE_NODE_RUN_SCRIPTS: ""
elyobo commented 1 year ago

Thanks; I missed these breaking changes, where would be the best place to sign up / follow / whatever to ensure I don't miss them in the future?

By default, when you configure custom build steps

The guidance here is misleading, because we have not configured any custom build steps, but this is happening anyway. Presumably "custom build steps" includes "having an npm build script" but that's not an obvious interpretation of the sentence!

Thanks for the additional advice. I had just set them in the env_variables, I'll adjust to build_env_variables. Can confirm that adding a gcp-build script to package.json did also work as advertised.

Dev dependencies were still installed via npm ci with NODE_ENV=development but were later removed using npm prune --production so that seems reasonable; I guess you could infer that the build script isn't going to be run and avoid installing them in the first place as an optimisation (edit: and as a security enhancement; fewer dependencies installed, fewer potential "supply chain" attacks). Edit 2: setting NODE_ENV to production in the build_env_variables does change this npm ci behaviour to only install production, so the behaviour appears to differ between setting the env var and adding the gcp-build script.

So if we want to run in production mode we should be manually setting NODE_ENV to production in the env_variables (or the build_env_variables) as well now?

elyobo commented 1 year ago

Checking back in, this is the update that gets us back to the old "install production dependencies, do not build stuff, launch app with NODE_ENV production" in app.yaml for a node 18 app in app engine flex.

build_env_variables:
  GOOGLE_NODE_RUN_SCRIPTS: ''

Would very much appreciate info on where exactly to sign up to that we get advance notice of breaking changes like this in the future!

Edit: updated above, NODE_ENV override removed as no longer required.

alexnault commented 1 year ago

Adding the following to our app.yaml got us back to the previous behavior:

build_env_variables:
    GOOGLE_NODE_RUN_SCRIPTS: ''

Thanks @anniefu!

elyobo commented 1 year ago

We had to add the override to NODE_ENV as well, not entirely sure why but it looked like it was running npm start in the build stage for some reason (and we do things in npm start that depend on the NODE_ENV which fail without the dev dependencies installed).

alexnault commented 1 year ago

@elyobo I'm not entirely sure why it runs npm start for you. NODE_ENV is properly set to production for us even without specifying it in our build_env_variables.

This part of the source code might provide an answer?

elyobo commented 1 year ago

Could be; we weren't setting anything, relying on the documented default behaviour in the past that it's production all the way through and so not setting anything. Works as it used to so long as we set it, but otherwise running additional steps that were surprising (e.g. not sure why we're seeing an npm start in the build step before it actually rolls out 🤷 ).

We've updated our apps to accommodate it but still not sure why the extra steps are required or why the default behaviour changed in such a breaking fashion.

anniefu commented 1 year ago

Glad everyone was able to get unblocked!

@elyobo you can keep an eye out for future breaking changes on the GAE release notes page: https://cloud.google.com/appengine/docs/standard/nodejs/release-notes

You shouldn't need to explicitly pass NODE_ENV: 'production' if you also have GOOGLE_NODE_RUN_SCRIPTS: ''. As long as no scripts npm run commands are executed, NODE_ENV defaults to production.

If npm run build did run, we change the default at launch time to NODE_ENV: 'development'. That's actually unintentional, and I'll push a fix for that.

elyobo commented 1 year ago

@anniefu thanks, looking through what happened was that in the empty gcp-build version it ended up deploying in development mode as you described above. In our case that causes different (undesirable in production) behaviour (tries to launch with development features enabled, fails because the dependencies are not deployed to GAE).

It sounds like your fix to NODE_ENV above addresses this.

It seems like my NODE_ENV override is unnecessary then, I'll confirm! (Edit: confirmed, the NODE_ENV override is not required with GOOGLE_NODE_RUN_SCRIPTS: '').

In terms of breaking changes, checking in on release notes isn't really sufficient for an opt-out breaking change! It's fine for opt in, but there needs to be some sort of push channel for notifications where an absence of any action will result in things breaking. GAE has been very stable for us in general (thanks!) and so it's not realistic that we will be regularly checking in on the release notes on the off chance that breaking changes have suddenly turned up.

Petersdavis commented 1 year ago

These changes also break the deployment of NextJS app using experimental:webframeworks. I haven't gotten it to work again yet -- but will try to follow ^ steps. posting because I've had a very frustrating day trying to figure out what I broke.

Mankee commented 1 year ago

Where do I send the bill to google for time wasted on this?