Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Node.js v16 Upgrade Plan #835

Closed
boktorbb opened this issue Sep 30, 2021 · 15 comments
Closed

[RFC] Node.js v16 Upgrade Plan #835

boktorbb opened this issue Sep 30, 2021 · 15 comments
Assignees
Labels
dependencies Pull requests that update a dependency file discuss RFC Substantial changes or new features that require community input to garner consensus.

Comments

@boktorbb
Copy link
Contributor

boktorbb commented Sep 30, 2021

Overview

OpenSearch Dashboards is currently running an EOL Node.js version, Node.js v10.24.1. This current version of Node carries with it several high severity security vulnerabilities, as well as forces us to use older version of critical dependencies that carry their own share security vulnerabilities. The intent of the core Dashboards team is to upgrade to Node.js v16.9.1 which goes into LTS support in October 2021. The OpenSearch project is following the SemVer standard for versioning; therefore, the Node.js v16 upgrade is considered a breaking change and will require a version bump of Dashboards to 2.0.0

Scope

The scope of the Node 16 upgrade is to do the minimum amount of dependency, code, and testing changes to allow for the core Dashboards team to address many of the CVEs that are present currently and blocked by our current Node.js version. While there may be many opportunities to make changes or upgrade packages because of the new version of Node, they would be out of scope of the work done to upgrade Dashboards to Node.js v16.

Upgrade Proposal

The Node.js v16 upgrade will take the form of 3 phases that will be cycled through for each new plugin and developer wanting to upgrade. The 3 phases are somewhat coupled which makes working on the phases asynchronously difficult.

Phase 1: Dependency Version Parity

This phase consists primarily of bringing all of Dashboards’ dependencies up to parity with what Node.js v16 expects/requires. Node v16 is many major versions higher than what we were working with before and will require upgrades to several dependencies. These changes will introduce cascading breaking changes to other dependencies, as well as to some code implementations in Dashboards.

Below is a table of dependencies that are required to upgrade to Node.js v16 for core Dashboards:

Dependencies Current Version Upgrade Version Notes Function in Dashboards
lmdb-store 0.6.10 1.6.7 The primary issue introduced by Node.js v16 upgrade is that the sub-dependency msgpackr-extract is not supported by Node 16 and requires lmdb-store to be bumped to latest. lmdb-store is a key-value store library being used by our osd-optimizer package for cluster caching
node-sass v5 6.0.1 node-sass is deprecated and should be changed to dart-sass; however, there was a version of node-sass that was supported by Node.js v16 and that was version 6.0.1. Bumping node-sass rather than swapping out to dart-sass seems to be less work and there are other open questions surrounding the use of dart-sass with regards to EUI. node-sass is the primary library for using Sass. Currently deprecated in favor of dart-sass
sass-loader 8.0.2 10.2.0 Bumping node-sass up to v6.0.1 causes the need to upgrade sass-loader to a newer version that supports both node-sass v6.0.1 and node.js v16.9.1. helper library for all things Sass
@types/webpack-env 1.15.2 1.15.3 This version bump was required because of a bug with Node.js interfacing with webpack. The error was: error TS2430: Interface 'NodeJS.Module' incorrectly extends interface '__WebpackModuleApi.Module Type definitions for typescript that correspond to webpack and are to be used in conjunction with webpack
tslib N/A 2.0.0 The tslib module is explicitly required for Node.js v16 and was not added in our package.json runtime library for typescript that contains all of the typescript helper functions
@types/node >=10.17.17 <10.20.0 16.9.1 It's required to update to the latest type definition package for node that corresponds to the Node.js major version being used Type definitions for typescript that correspond to Node major versions

The majority of the errors pertaining to dependency version errors and version mismatches will manifest themselves as build errors and package compatibility errors. Some of the errors will manifest as type/code logical errors but those will be handled in the next phase.

Phase 2: Breaking code changes

The next major obstacle is the plethora of breaking code changes introduced both by the new version of Node.js itself and the new versions of other dependencies. This phase requires a good amount of familiarity with breaking changes across major versions of dependencies. The three major types of code changes are listed below:

  1. Syntax strictness with regards to null and undefined behavior
    1. Node.js v16 has become much more strict with the handling of possible null and undefined behaviors.
    2. The majority of these types of errors are fixed by passing the non-null assertion operator if you are sure that the value being passed in code is not going to be null
      1. e.g.
      2. childProcess.stdin!.end(stdin, 'utf8'); // TypeScript note: As long as the proc stdio[1] is 'pipe', then stdin will not be null
      3. Node has an issue with stdin and stderr null possibilities and requires the non-null assertion operator to no longer break the build
  2. Type mismatches and type strictness
    1. There are lambdas throughout the code base that are using variables with no explicit type
    2. Variables being typed as unknown so they break type definitions for certain interfaces
    3. Generally the error messages take the form of Type<unknown> cannot be assigned to Type<type>
    4. There are cases of the type definitions expected by @types/node not matching the actual types
      1. E.g.
      2. urlUtils.encodeQuery expects key/value pairs with values of type string | string[] | null,
        however @types/node says that url.query has values of type string | string[] | undefined.
        After investigating this, it seems that no matter what the values will be of type string | string[]
  3. Feature deprecation and new data interfaces
    1. Webpack config file has deprecated an object property in favor of another in newer versions
      1. webpack-config: prependData changed to additionalData
      2. The additionalData property requires an additional content parameter
    2. The lmdb-store version bump leads to a required rewrite of our cache.ts file in the osd-optimizer package
      1. This is required because in v0.6.10, there was no data interface for the store. This lead to elastic creating their own data interface abstraction
      2. interface Lmdb<T> {
        get(key: string): T | undefined;
        put(key: string, value: T, version?: number, ifVersion?: number): Promise<boolean>;
        remove(key: string, ifVersion?: number): Promise<boolean>;
        openDB<T>(options: { name: string; encoding: 'msgpack' | 'string' | 'json' | 'binary' }): Lmdb<T>;
        getRange(options?: {
        start?: T;
        end?: T;
        reverse?: boolean;
        limit?: number;
        versions?: boolean;
        }): Iterable<{ key: string; value: T }>;
        }
      3. In lmdb-store v1.6.7, they have introduced new interfaces that must be used in our caching implementation.
      4. The lmdb-store types are:
        1. LmdbStore.RootDatabase
        2. LmdbStore.Database
      5. A rewrite of some of the member methods of the Cache class is required to satisfy the more strict exception handling requirements of Node

Phase 3: Runtime Errors

Runtime errors tend to be the hardest types of errors to track down in this process and they usually occur either as you attempt to run Dashboards using yarn start or they crop up while Dashboards is running itself.

Node 16 Cluster error:

There seems to be an issue with importing the cluster module in the worker.ts file in core.

 log   [16:54:15.757] [fatal][root] TypeError: _cluster.default.setupPrimary is not a function
When switching the import from import cluster from 'cluster' to import * as cluster from 'cluster' we get the following error.
Unhandled Promise rejection detected:

TypeError: Cannot read properties of undefined (reading 'apply')
at BinderFor.on (/home/ubuntu/OpenSearch-Dashboards/src/cli/cluster/binder.ts:47:8)
at BinderFor.on (/home/ubuntu/OpenSearch-Dashboards/src/cli/cluster/binder_for.ts:41:11)
at Worker.start (/home/ubuntu/OpenSearch-Dashboards/src/cli/cluster/worker.ts:228:24)
at ClusterManager.startCluster (/home/ubuntu/OpenSearch-Dashboards/src/cli/cluster/cluster_manager.ts:181:14)
at FSWatcher.<anonymous> (/home/ubuntu/OpenSearch-Dashboards/src/cli/cluster/cluster_manager.ts:249:12)

API Error:

The following 2 errors occur when starting dashboards with yarn start after upgrading lmdb-store, node-sass, and sass-loader. It stops Dashboards from loading the home app because the capabilities api call never succeeds.
NetworkError when attempting to fetch resource.
Version
: 1.1.0
Build: 9007199254740991
Error: NetworkError when attempting to fetch resource.
HttpFetchError@http://localhost:5603/ime/9007199254740991/bundles/core/core.entry.js:18398:5
fetchResponse@http://localhost:5603/ime/9007199254740991/bundles/core/core.entry.js:18274:13
Uncaught (in promise) TypeError: NetworkError when attempting to fetch resource.
HttpFetchError http_fetch_error.ts:38
fetchResponse fetch.ts:156
osdBootstrap osd_bootstrap.ts:57
AsyncFunctionThrow self-hosted:697
Looks like the above errors pertain to the api/core/capabilities API request
POST http://localhost:5603/azc/api/core/capabilities net::ERR_EMPTY_RESPONSE

@boktorbb boktorbb added dependencies Pull requests that update a dependency file v2.0.0 labels Sep 30, 2021
@boktorbb boktorbb self-assigned this Sep 30, 2021
@dblock
Copy link
Member

dblock commented Oct 1, 2021

Please make sure to include changes to https://github.com/opensearch-project/opensearch-build when node is upgraded. It likely means building different docker images.

@seanneumann seanneumann pinned this issue Oct 12, 2021
@boktorbb boktorbb changed the title Node.js v16 Upgrade Plan [RFC] Node.js v16 Upgrade Plan Oct 12, 2021
@dbbaughe
Copy link

One discussion point I'd like to see some opinions about is the versioning..

Since this is an upgrade of the nodejs major version.. it's expected that the OpenSearch Dashboards will bump to 2.0.

So we can either:

  1. Hold off on this release until OpenSearch core is ready for 2.0 and release them both together and keep them in sync
  2. Do an early release of OpenSearch core for 2.0 when Dashboards is ready to keep them in sync
  3. Or let the versions diverge so we have OpenSearch core 1.3 (? or 1.4 or whenever) and OpenSearch Dashboards 2.0

I'm assuming Dashboards is leaning towards #3 as I don't think core is ready for a 2.0 release yet with all the breaking changes intended to go in.

So if we choose #3 we'll then have to manage the confusion around the different compatibility matrix of the Dashboard and core versions. And we also would need to decide on what to do with plugins... as we have not yet decided to decouple the plugin versions from core/Dashboards.

So if Dashboards 2.0 releases and we still have core 1.3 will we also bump all of our dashboard plugins to 2.0 and keep our backend plugins on 1.3? And then also have to deal with the confusion around our plugins having a version compatibility matrix now?

@AMoo-Miki
Copy link
Collaborator

Upgrading to use Node v16 would provide OpenSearch Dashboards a welcome performance and security boost and its customers a very long-term stability. However, Node v16 just entering LTS in a few weeks could prevent some from adopting the newer versions of Dashboards due to policy or technical barriers.

Node v14 might not have all the newer performance optimizations of Node v16 but has been around long enough to not raise as many technical or policy barriers. Moreover, Node v14 will be in LTS Maintenance till the end of Apr 2023 which would allow for a decent expectation of stability for Dashboards customers.

Additionally, while the current dependency of Dashboards on Node v10 might have been an adoption blocker for some, Node v16 could be an adoption blocker for the same reasons. While some might have the luxury of running Dashboards on a dedicated box, I believe many would welcome the ability to host several other web apps on the same box, ease of which could be dependent on the version of Node available. While there are workarounds to this problem, policy and technical barriers could still impact their adoption.

As such, I believe Node v14 should also be considered as a possible upgrade target. In a perfect world, a version of Dashboards that runs on v14 shouldn't or could be made to not have much or any trouble running of Node v16 but if a difference exists, to maximize adoption, I would call for two releases, one for each version. However, if that is not an option, Dashboards should spin up a poll to gather information about the version of Node that its customers are more likely to adopt, in order to make an informed decision.

@stockholmux
Copy link
Member

@AMoo-Miki My personal PoV here is that we need as long of a runway as possible - v16 gives us a longer runway and it will give us more active development.

In your use case (running dashboards on a shared box), I think most people use some sort of tool to run multiple versions (nvm as an example).

@AMoo-Miki
Copy link
Collaborator

@stockholmux, while nvm and its likes would address the second point for some, I know at least one organization that had a policy against using nvm in production environments. In fact, they had two or three specific versions of Node engine that you could opt for. The same organization had a policy against using Node before certain number of months had passed from its entry to LTS; this was the first point in my argument.

Also, I love Dashboards striving to offer long-term stability; Node v14 would offer 18 months of it and Node v16, 30 months. That reason alone could excite many about Dashboards on Node v16 but if their organizations have policies barring them from using Node v16 for some time, they wouldn't adopt the newer versions of Dashboards yet. That's why I proposed using a poll to assess if current and potential customers can use Dashboards on Node v16 in (1) 3 months, (2) 6 months, or (3) maybe later.

On a side note, it is possible that if Dashboards targets Node v14, it would only need to bump some dependencies to use Node v16. If at all that works, Dashboards' install scripts could intelligently install the needed deps to make it compatible with both versions of Node.

@stockholmux
Copy link
Member

stockholmux commented Oct 14, 2021

@AMoo-Miki So, v16 will be Active LTS on 2021-10-26 which will be before the time we follow through on this RFC. If an organization wants to disallow active LTS, that seems like their problem, not ours.

Every time Dashboard's adopts a new version of Node it's a new major version (according to SemVer). Every new major can cause heartburn for our users and those who want to develop plugins for Dashboards. The least we can do is give them the longest interval between needing to take TUMS.

@AMoo-Miki
Copy link
Collaborator

@stockholmux, I hear you; it helps that the upgrade will come some time after v16 goes to LTS.

Node will always have 2 or 3 LTS versions simultaneously and because I would love to avoid heartburn for everyone, I would prefer to invest a bit more effort and make Dashboards run on all the LTS versions that people might be running with.

@boktorbb
Copy link
Contributor Author

boktorbb commented Oct 26, 2021

@AMoo-Miki I totally understand wanting to include as many people and organizations as possible. Unfortunately, we are not set up to support all LTS versions available. It would be a massive project for our team that would include reworking the build system. There are other priorities that have larger impact for our users.

I agree with what @stockholmux said previously regarding external organization policies. We can't control what they choose to disallow and it would be difficult to please everyone

@tmarkley tmarkley added the RFC Substantial changes or new features that require community input to garner consensus. label Nov 1, 2021
@boktorbb
Copy link
Contributor Author

boktorbb commented Nov 7, 2021

Node upgrade progress update:

Overview

OpenSearch Dashboards needs to upgrade the Node.js version from the current version, 10.24.1, to mitigate high severity CVEs affecting the application. The upgrade process intended to move to Node.js v16.9.1. During the upgrade process, it became clear that the amount of investigation and work required to upgrade to v16 was quite large and the team wanted to prioritize mitigating CVEs for the next OpenSearch release. Since the Node v16 upgrade wouldn’t be finished in time for the next release, the team decided on a move to Node.js v14 instead.

Issues faced

The jump to v16 is several major versions higher than the current Dashboards version. Particularly, the jump from v14 to v16 is a much bigger jump in terms of breaking changes than even the jump to v10 to v14. Node v16 has many breaking changes and we encountered the following issues:

  • The first major issue is dependency mismatches. Dependencies have silent errors and breaking changes when we update the mandatory dependencies that are required to match Node.js v16.
    • One of the major issues of these dependency updates is the changes in types and interfaces. When they change, it requires us to refactor code and modules in Dashboards. This is mitigated by moving back to Node v14 where these breaking interface changes are minimal
    • The other major issue faced was silent failures during runtime. Certain dependencies are mismatched with others but the mismatch doesn’t manifest in errors or log information but causes silent runtime crashes and failures. This is mitigated by moving to Node v14 where the package changes and updates are minimized
  • The next major issue is code refactor in the Dashboards repo
    • The cluster module in the Dashboards core would require a refactor to mitigate the changes in the Node.js v16 built-in cluster module API changes.
      • A refactor of the way we determine worker types and scheduling would be required
      • A refactor of the osd-optimizer process that parses plugin paths and loads plugins would be required
  • Another issue is the extra effort that is required from individual plugin owners to upgrade their plugins for compatibility with Node v16. The jump from v10 to v16 is big enough that the amount of work required from each plugin can be large. With 9 internal Dashboards plugins being affected, moving to Node v14 would reduce effort on this front significantly.

Possible paths forward

Moving to Node 14 (preferred solution)

Node.js v14 is currently in LTS and will continue to be in LTS until 2023. This gives us an easier and earlier target to hit since we’re prioritizing mitigating CVEs. It also gives us time to plan and prioritize work for a future upgrade to Node v16 where we aren’t crunched for time because of CVEs and meeting predetermined release schedules.

Continuing Node 16 and missing OpenSearch release

Another option would be bringing on more people to help with the Node 16 upgrade. This would take more effort and would require us to miss the OpenSearch 1.3 release and wait until the following release. Since mitigating the high severity CVEs in Dashboards motivated the whole Node upgrade project, pushing off the CVE fixes further out while Node v10 is EOL is not a good option

Continuing Node 16 and delaying OpenSearch release

This option is the same as the above option but instead of missing the release date and waiting until the next one, we have the OpenSearch 1.3 release delayed until Node.js v16 upgrade is complete.

@seanneumann
Copy link
Contributor

Thanks for the research and prototyping @boktorbb-amzn. I think moving to 14 sounds like a good next step.

@stockholmux
Copy link
Member

So, 14 and 16 are breaking changes, so Dashboards will jump majors twice to get to the latest version?

What's the timeline/target distribution version for 16?

@seanneumann
Copy link
Contributor

According to Node.js LTS schedule, 14 is EOL April 2023. My hunch is we'd upgrade in Q1 of 2023.

@stockholmux
Copy link
Member

@seanneumann That makes sense, if we could advance it a tad, there would be a nice symmetry of V14 = 2022, V16 = 2023.

@stockholmux
Copy link
Member

And naturally, I think everyone would like to avoid releasing Dashboards with a EOL Node ever again, so leaving to the last part of the support window is probably not preferred.

@boktorbb boktorbb mentioned this issue Nov 8, 2021
15 tasks
@seanneumann seanneumann unpinned this issue Nov 8, 2021
@tmarkley tmarkley removed the v2.0.0 label Jan 4, 2022
@boktorbb
Copy link
Contributor Author

Node upgrade to version 14.18.2 is complete. Will close this issue for now and will reopen when the time comes to evaluate the next version to upgrade to for OpenSearch Dashboards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file discuss RFC Substantial changes or new features that require community input to garner consensus.
Projects
None yet
Development

No branches or pull requests

7 participants