OpenSearch-Dashboards icon indicating copy to clipboard operation
OpenSearch-Dashboards copied to clipboard

[ENHANCEMENT] Ability to start OS Dashboards with a newer and compatible nodejs version

Open sipopo opened this issue 3 years ago • 8 comments

Describe the bug

The application doesn't start with nodejs version 14.20.0

To Reproduce Upgrade nodejs to version 14.20.0 and run the application.

Expected behavior The application should start.

OpenSearch Version OpenSearch-Dashboard 2.1.0

Additional context Errors in log: OpenSearch Dashboards was built with v14.19.1 and does not support the current Node.js version v14.20.0. Please use Node.js v14.19.1 or a higher patch version.

sipopo avatar Aug 08 '22 15:08 sipopo

Hello @sipopo,

Thanks for opening. I would be hesitant to call this a bug because it's quite intentional. As indicated by the message, OpenSearch Dashboards was built with 14.19 and we are semi-confident that there won't be any unforeseen issues by having different patch versions. But that risk increases even more when we allow for minor versions to be different.

In my opinion I'd think it's more safe to leave this behavior the same. At this point I'd be more comfortable with this being implemented: https://github.com/opensearch-project/OpenSearch-Dashboards/issues/1219 and have a warning message about using a higher version.

But I would like to hear from the other @opensearch-project/opensearch-dashboards-core.

kavilla avatar Aug 08 '22 16:08 kavilla

I'm curious to know if we have any documented examples of using higher minor versions of node breaking anything in OSD. The entire purpose of semver is to introduce non breaking backward compatible changes in minor version bumps (And node does follow semver). To not use that and lock down the major, minor and patch versions of NodeJS used seems over cautious imo.

Like you called out, i'd be open to compromising and adding a warning to higher minor versions, but I'd like to hear some arguments about why we'd even need that.

ashwin-pc avatar Aug 09 '22 01:08 ashwin-pc

:+1: for honoring SemVer: currently we actively reject expected to be compatible versions. Also if a security issue is found in node 14, it would be fixed as 14.20.1 (now that 14.20.0 has been released), this mean that the fixed version could not be used without prior patching.

smortex avatar Aug 09 '22 04:08 smortex

Also if a security issue is found in node 14, it would be fixed as 14.20.1

My bad, 14.20.0 already fix three medium severity issues and two high severity issues according to https://nodejs.org/en/blog/vulnerability/july-2022-security-releases/ :upside_down_face:

smortex avatar Aug 09 '22 04:08 smortex

I'm curious to know if we have any documented examples of using higher minor versions of node breaking anything in OSD. The entire purpose of semver is to introduce non breaking backward compatible changes in minor version bumps (And node does follow semver). To not use that and lock down the major, minor and patch versions of NodeJS used seems over cautious imo.

Like you called out, i'd be open to compromising and adding a warning to higher minor versions, but I'd like to hear some arguments about why we'd even need that.

I think that's one of the issues that we still have not done enough research in why this check was required and carried over from the fork. The original expected to use the bundled Node version and if the Node executable isn't available it will default to use the env global version, and expected the same version.

Historically I can only find that the reason why this logic existed was that they said Node adheres to semver but the legacy app said they can't guarantee it Node does without bumping the application version as well.

I still think in the end the overall goal should match what OpenSearch is doing with OPENSEARCH_JAVA_HOME, where there's a check but basically setting env variable path will override the checking of the pre-bundled Java regardless if the file is there or not.

At which point, this will solve what @smortex is referencing because the vulnerabilities would still exist if we built the application with Node 14.20.0 and folks don't manually delete the pre-bundled Node.

kavilla avatar Aug 09 '22 05:08 kavilla

I still think in the end the overall goal should match what OpenSearch is doing with OPENSEARCH_JAVA_HOME, where there's a check but basically setting env variable path will override the checking of the pre-bundled Java regardless if the file is there or not.

I think this is a separate conversation

At which point, this will solve what @smortex is referencing because the vulnerabilities would still exist if we built the application with Node 14.20.0 and folks don't manually delete the pre-bundled Node.

I think the ask here is much simpler. It's just to allow the user to use minor versions greater than the one defined in .nvmrc. The bundled node versions will always contain the version we specify, but if a user has a higher node minor version on their system, we dont need to fail the build, but instead just give a warning. We already do the same thing with our npm dependencies when the minor versions dont match.

ashwin-pc avatar Aug 09 '22 18:08 ashwin-pc

#2101 was merged and update nodejs to 14.20.0 so this Issue title is not true anymore. I suggest to update it to better match the discussion, something like "[BUG] Not able to start OS Dashboards with a newer and compatible nodejs version"

smortex avatar Aug 12 '22 17:08 smortex

Updated the issue title.

joshuarrrr avatar Oct 04 '22 20:10 joshuarrrr