build-image icon indicating copy to clipboard operation
build-image copied to clipboard

feat: install always latest minor and patch version for each node major to get backports and security fixes

Open lukasholzer opened this issue 1 year ago โ€ข 5 comments

๐ŸŽ‰ Thanks for submitting a pull request! ๐ŸŽ‰

Summary

Node provides even for older LTS versions security fixes (patch version) and some important features that are back ported like corepack that got back ported to 14.19.0.

Currently if a user specifies to build with node version 14.12.0 we will install exactly this version. But we should provide the latest available patch and minor to cover all the security patches. And to securely rely on features that getting back ported for a LTS version.

As we freeze the node version it could be hard to update it for users when they created it long time ago.

This should now provide transparency though logging a message that we installed a newer version if they have specified an older version.

for example if someone specifies Node version 17.0.0 we will install the latest available 17 version at this point in time:

Downloading and installing node v17.9.1...
Downloading https://nodejs.org/dist/v17.9.1/node-v17.9.1-linux-arm64.tar.xz...
Computing checksum with sha256sum
Checksums matched!
Now using node v17.9.1 (npm v8.11.0)
We've installed a newer node version v17.9.1 as the one you've requested 17.0.0.
This is done to provide you with the latest security patches and features to ensure a safe build! ๐Ÿซก

For us to review and ship your PR efficiently, please perform the following steps:

  • [ ] Open a bug/issue before writing your code ๐Ÿง‘โ€๐Ÿ’ป. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire ๐Ÿ”ฅ (e.g. incident related), you can skip this step.
  • [ ] Read the contribution guidelines ๐Ÿ“–. This ensures your code follows our style guide and passes our tests.
  • [ ] Update or add tests (if any source code was changed or added) ๐Ÿงช
  • [ ] Update the included software doc (if you updated included software) ๐Ÿ“„
  • [ ] Update or add documentation (if features were changed or added) ๐Ÿ“
  • [ ] Make sure the status checks below are successful โœ…

A picture of a cute animal (not mandatory, but encouraged)

lukasholzer avatar Oct 13 '22 14:10 lukasholzer

cc @ascorbic @JGAntunes @kitop would love to discuss with you about that!

lukasholzer avatar Oct 13 '22 14:10 lukasholzer

I like this plan! Especially love the logging. I would like a way for people to opt out, since invariably some third party dependency still requires some very broken node version and someone will want to use it despite the warts.

I wonder if it makes more sense to only do this if user says version "17" or "17.0" rather than explicit "17.0.0" which could install exactly what they asked for? This would (in my opinion) avoid the need to leave a feature flag to turn this feature off available for the long term, since there would be an obvious, directly-customer-usable workaround which we could document (right here: https://docs.netlify.com/configure-builds/manage-dependencies/#node-js-and-javascript) for someone who no damnit really needs the broken version, for whatever reason.

fool avatar Oct 13 '22 15:10 fool

I wonder if it makes more sense to only do this if user says version "17" or "17.0" rather than explicit "17.0.0" which could install exactly what they asked for?

So this is the default behaviour right now - https://docs.netlify.com/configure-builds/manage-dependencies/#node-js-and-javascript - if a user sets the NODE_VERSION to a major or a major and minor pair we'll install the latest version for that given value.

Example:

  • https://github.com/JGAntunes/netlify-plugin-nextjs/blob/f42a7a93b3291a4a41f59978f4b776656f110620/demos/default/netlify.toml#L12
  • https://app.netlify.com/sites/aws-test-build-nextjs/deploys/63483a285c2ac800099d3a2f#L21-L22 Follow up build:
  • https://app.netlify.com/sites/aws-test-build-nextjs/deploys/63483add3fa0f20050a0d1a2#L21-L22

A lot of folks have been using that strategy as a way to automatically receive updates.

My main concern here is the inability to opt-out if for wtv reason a customer requires a locked version of node. Ideally I would see this has being controlled by some sort of UI setting where customers can disable our version management if they want to (winking at that https://github.com/netlify/bitballoon/issues/9481 too)

I was also thinking that since this is injected from Buildbot, if instead of adding more logic to our run-build-functions methods we could perform this operation at the Buildbot level before injecting the env var (easier to maintain). The downside being that we would be further spreading the version management logic, but something that can hopefully tackle as part of these https://github.com/netlify/pillar-workflow/issues/883 https://github.com/netlify/pillar-workflow/issues/228

JGAntunes avatar Oct 13 '22 16:10 JGAntunes

I think if as @JGAntunes says the user can already do this by setting NODE_VERSION to a major version (eg: "17"), maybe instead adding more logic to add this and then implement an opt-in/opt-out mechanism it would be enough to document that behavior for those who want it (setting NODE_VERSION to a major version only)?

4xposed avatar Oct 14 '22 09:10 4xposed

Agreeing with @4xposed.

jobala avatar Oct 14 '22 09:10 jobala

I'm going to close the PR as we came up with a different solution

lukasholzer avatar Oct 17 '22 09:10 lukasholzer