Upgrade Node.js from v16.18.1 to v18.13.0 #144012

watson · 2022-10-26T10:05:08Z

Breaking changes in Node.js majors:

Note to reviewers

Failing cypress test suites

The Security Solution Tests suites are known to be very flaky. They are triggered by the ci:all-cypress-suites label. Please ignore these test failures - they actually do pass (cc @elastic/security-solution).

Dates

This PR includes a bunch of Date related test changes. This is because Node.js v18.13.0 ships with a newer version of ICU (i18n library), which includes some changes to how dates are rendered:

Dates that include an AM/PM postfix now uses a non-breaking space between the time and the AM/PM part.
~~Some dates, including en-CA formatted dates which we use in one of our tests, now render as 10/13/2021 instead of 2021-10-13.~~ Based on feedback I've reverted the formatting of these dates to back to the Node.js 16 behaviour (or as close to it as I could get).

I'm not sure if any of these changes are blockers for this PR?

For details see: nodejs/node#45945

Stream race condition

There's a race condition in the report generator code where it's waiting for a stream to finish using the built in finished function from Node.js core. However, it seems that we hit a race condition with this function. I've reported it to the Node.js project (nodejs/node#46170). However, I feel confident that we can land this PR using my workaround added in this commit: ca2a1c3

Upgraded dependencies

re2: 1.17.4 -> 1.17.7 - We could technically have kept version 1.17.4, but we needed to recompile it against the newer Node.js version anyway, so I took the chance to upgrade it while I was at it.
~~lmdb-store: 1.6.11 -> 2.6.9 - The old version didn't seem to work with Node.js 18. The package also changed name from lmdb-store to just lmdb.~~ Extracted into separate PR: chore(NA): updates from lmdb-store to lmdb #145891
source-map: 0.7.3 -> 0.7.4 - The old version didn't seem to work with Node.js 18.
tar: 6.1.11 -> 6.1.13 - Includes a bug fix for a race condition that triggered when the ci:build-all-platforms label was added in Node.js 18 but not in Node.js 16

I also had to upgrade a few @types/* packages to be compatible with Node.js 18, but I've not listed them above as they don't influence the runtime.

Blockers:

x-pack/plugins/fleet/server/services/epm/archive/extract.ts

watson · 2023-01-10T05:38:01Z

x-pack/plugins/fleet/server/services/epm/archive/extract.ts

-  const inflateStream = tar.list().on('entry', (entry: tar.FileStat) => {
-    const path = entry.header.path || '';
+  const inflateStream = tar.list().on('entry', (entry) => {
+    const path = entry.path || '';


Accessing the path via the header works but isn't supported according to the types. Instead it seems that you're supposed to access it directly via the root path property where it's copied to in the constructor:

https://github.com/npm/node-tar/blob/26a496e5fa74eeaa0c3539511560fc181ef56557/lib/read-entry.js#L50

x-pack/plugins/fleet/server/services/epm/archive/extract.ts

watson · 2023-01-10T05:41:02Z

x-pack/plugins/fleet/server/services/epm/archive/extract.ts

    if (!filter({ path })) return;
-    streamToBuffer(entry).then((entryBuffer) => onEntry({ buffer: entryBuffer, path }));
+    streamToBuffer(entry as unknown as NodeJS.ReadableStream).then((entryBuffer) =>


streamToBuffer expects a ReadableStream, but entry isn't one. The types seem to be a little to strict though and casting like this should appease TS.

kibanamachine · 2023-01-25T15:50:49Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 144012 locally

watson · 2023-02-15T08:17:21Z

💚 All backports created successfully

Status	Branch	Result
✅	7.17

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kqualters-elastic · 2023-02-15T22:42:17Z

I think the main reason the security solution tests are so flaky is precisely because they don't run on prs like this one, and it shouldn't be a surprise that dependencies changing underneath of a code base without the bulk of the test coverage being run would lead to flakiness. We should probably add

        /^package\.json/,
        /^packages\/*/,

to https://github.com/elastic/kibana/blob/main/.buildkite/scripts/pipelines/pull_request/pipeline.ts#L71 and the vast majority of the flakiness would go away. If the tests are prohibitively slow, just bump https://github.com/elastic/kibana/blob/main/.buildkite/pipelines/pull_request/security_solution.yml#L8 by a factor of 2 or so. I'm not sure what the historical reasons are for limiting when they run really are, but it's a problem that's only going to get worse (and likely become a big issue for other code bases in that pipeline.ts file) with so much functionality being moved to packages, and integration tests not running when oft used dependencies change.

watson · 2023-02-16T08:18:52Z

If running the security solutions tests for each change to package.json (if I'm reading your proposal correctly), then I'd definitely vote for splitting up the tests into more parallel runners so that a single flaky test doesn't require us to re-run 45 minutes of already tested tests. If so, I'm all for running them more often, as long as work is being done to ensure the flakiness is addressed 👍

I'm not sure if the security solution tests are part of the regular "flaky tests" program where they are skipped if considered flaky and an issue opened to address it, so they don't hold up all PRs just because they happen to be flaky?

Closes elastic#134930 Breaking changes in Node.js majors: - `17.0.0`: https://github.com/nodejs/node/blob/main/doc/changelogs/CHANGELOG_V17.md#17.0.0 - `18.0.0`: https://github.com/nodejs/node/blob/main/doc/changelogs/CHANGELOG_V18.md#18.0.0 (cherry picked from commit b345f75)

## Summary Bumps node.js to 18.17.0 (replacement for PR #144012 which was later reverted) As a result, these categorical additions were needed: - `node` evocations will need the `--openssl-legacy-provider` flag, wherever it would use certain crypto functionalities - tests required updating of the expected HTTPS Agent call arguments, `noDelay` seems to be a default - `window.[NAME]` fields cannot be written directly - some stricter typechecks This is using our in-house built node.js 18 versions through the URLs the proxy-cache. (built with elastic/kibana-custom-nodejs-builds#4) These urls are served from a bucket, where the RHEL7/Centos7 compatible node distributables are. (see: elastic/kibana-ci-proxy-cache#7) Further todos: - [x] check docs wording and consistency - [ ] update the dependency report - [x] explain custom builds in documentation - [x] node_sass prebuilts --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Tiago Costa <tiago.costa@elastic.co> Co-authored-by: Thomas Watson <w@tson.dk>

## Summary Bumps node.js to 18.17.0 (replacement for PR elastic#144012 which was later reverted) As a result, these categorical additions were needed: - `node` evocations will need the `--openssl-legacy-provider` flag, wherever it would use certain crypto functionalities - tests required updating of the expected HTTPS Agent call arguments, `noDelay` seems to be a default - `window.[NAME]` fields cannot be written directly - some stricter typechecks This is using our in-house built node.js 18 versions through the URLs the proxy-cache. (built with elastic/kibana-custom-nodejs-builds#4) These urls are served from a bucket, where the RHEL7/Centos7 compatible node distributables are. (see: elastic/kibana-ci-proxy-cache#7) Further todos: - [x] check docs wording and consistency - [ ] update the dependency report - [x] explain custom builds in documentation - [x] node_sass prebuilts --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Tiago Costa <tiago.costa@elastic.co> Co-authored-by: Thomas Watson <w@tson.dk>

\>6.1.11 is needed for a bug fix in #162722 Noted in #144012: > tar: 6.1.11 -> 6.1.13 - Includes a bug fix for a race condition that triggered when the ci:build-all-platforms label was added in Node.js 18 but not in Node.js 16 This upgrades to the latest patch on main before backporting to 7.17

\>6.1.11 is needed for a bug fix in elastic#162722 Noted in elastic#144012: > tar: 6.1.11 -> 6.1.13 - Includes a bug fix for a race condition that triggered when the ci:build-all-platforms label was added in Node.js 18 but not in Node.js 16 This upgrades to the latest patch on main before backporting to 7.17

\>6.1.11 is needed for a bug fix in elastic#162722 Noted in elastic#144012: > tar: 6.1.11 -> 6.1.13 - Includes a bug fix for a race condition that triggered when the ci:build-all-platforms label was added in Node.js 18 but not in Node.js 16 This upgrades to the latest patch on main before backporting to 7.17 (cherry picked from commit 8ffd4fb)

## Summary Bumps node.js to 18.17.0 (replacement for PR elastic#144012 which was later reverted) As a result, these categorical additions were needed: - `node` evocations will need the `--openssl-legacy-provider` flag, wherever it would use certain crypto functionalities - tests required updating of the expected HTTPS Agent call arguments, `noDelay` seems to be a default - `window.[NAME]` fields cannot be written directly - some stricter typechecks This is using our in-house built node.js 18 versions through the URLs the proxy-cache. (built with elastic/kibana-custom-nodejs-builds#4) These urls are served from a bucket, where the RHEL7/Centos7 compatible node distributables are. (see: elastic/kibana-ci-proxy-cache#7) Further todos: - [x] check docs wording and consistency - [ ] update the dependency report - [x] explain custom builds in documentation - [x] node_sass prebuilts --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Tiago Costa <tiago.costa@elastic.co> Co-authored-by: Thomas Watson <w@tson.dk>

\>6.1.11 is needed for a bug fix in elastic#162722 Noted in elastic#144012: > tar: 6.1.11 -> 6.1.13 - Includes a bug fix for a race condition that triggered when the ci:build-all-platforms label was added in Node.js 18 but not in Node.js 16 This upgrades to the latest patch on main before backporting to 7.17

Backports #162833 Partially backports #144012 via `x-pack/plugins/fleet/server/services/epm/archive/extract.ts` Needed for #162722

Closes #162695 # Backport This will backport the following commits from `main` to `7.17`: - [[Ops] Bump Node.js to version 18 (#160289)](#160289)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Tiago Costa <tiago.costa@elastic.co> Co-authored-by: Thomas Watson <w@tson.dk> Co-authored-by: Jonathan Budzenski <jon@elastic.co>

watson self-assigned this Oct 26, 2022

legrego mentioned this pull request Oct 26, 2022

Upgrade to Node 18.x #134930

Closed

watson force-pushed the bump-node-18 branch from 652439d to eac10f1 Compare October 26, 2022 15:28

watson commented Oct 27, 2022

View reviewed changes

x-pack/plugins/fleet/server/services/epm/archive/extract.ts Outdated Show resolved Hide resolved

watson force-pushed the bump-node-18 branch 8 times, most recently from a9ea88a to 9816942 Compare November 2, 2022 13:45

watson force-pushed the bump-node-18 branch from 9816942 to 2db7308 Compare November 7, 2022 10:23

watson changed the title ~~Upgrade Node.js from v16.17.1 to v18.12.0~~ Upgrade Node.js from v16.17.1 to v18.12.1 Nov 7, 2022

watson force-pushed the bump-node-18 branch from 2db7308 to e24c27b Compare November 17, 2022 12:42

watson changed the title ~~Upgrade Node.js from v16.17.1 to v18.12.1~~ Upgrade Node.js from v16.18.1 to v18.12.1 Nov 17, 2022

watson force-pushed the bump-node-18 branch from e24c27b to fa548ef Compare November 21, 2022 10:58

watson force-pushed the bump-node-18 branch from fa548ef to 0f5e63c Compare January 9, 2023 08:45

watson changed the title ~~Upgrade Node.js from v16.18.1 to v18.12.1~~ Upgrade Node.js from v16.18.1 to v18.13.0 Jan 9, 2023

watson force-pushed the bump-node-18 branch 4 times, most recently from d18da9e to 7237481 Compare January 9, 2023 13:51

watson commented Jan 10, 2023

View reviewed changes

x-pack/plugins/fleet/server/services/epm/archive/extract.ts Outdated Show resolved Hide resolved

watson commented Jan 10, 2023

View reviewed changes

watson force-pushed the bump-node-18 branch 2 times, most recently from 3acd0e0 to c56c392 Compare January 11, 2023 07:32

watson added backport:prev-major Backport to (8.x, 8.17, 8.16) the previous major branch and other branches in development release_note:skip Skip the PR/issue when compiling release notes labels Jan 11, 2023

hop-dev mentioned this pull request Jan 24, 2023

[Fleet] Local Kibana can't connect to local EPR #149335

Closed

watson added v7.17.9 and removed backport:prev-major Backport to (8.x, 8.17, 8.16) the previous major branch and other branches in development labels Jan 24, 2023

marius-dr mentioned this pull request Jan 24, 2023

Kibana 8.7.0-snapshot with Node 18.x does not install on Ubuntu 18.04 #149438

Closed

jsanz mentioned this pull request Jan 24, 2023

Upgrade to Node 18 elastic/ems-client#143

Closed

watson added backport:skip This commit does not require backporting and removed v7.17.9 labels Jan 25, 2023

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 25, 2023

watson mentioned this pull request Feb 15, 2023

[7.17] Upgrade Node.js from v16.19.1 to v18.13.0 (#144012) #151252

Closed

kibanamachine mentioned this pull request Feb 21, 2023

Upgrade Node.js to 16.19.1 #151527

Merged

watson mentioned this pull request Jul 18, 2023

[Ops] Bump Node.js to version 18 #160289

Merged

4 tasks

This was referenced Aug 1, 2023

[7.17] Bump types/tar to ^6.1.5 (#162833) #162893

Merged

Bump tar to ^6.1.15 #162897

Merged

jbudz added a commit that referenced this pull request Aug 1, 2023

[7.17] Bump types/tar to ^6.1.5 (#162833) (#162893)

91d4c51

Backports #162833 Partially backports #144012 via `x-pack/plugins/fleet/server/services/epm/archive/extract.ts` Needed for #162722

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Node.js from v16.18.1 to v18.13.0 #144012

Upgrade Node.js from v16.18.1 to v18.13.0 #144012

watson commented Oct 26, 2022 •

edited by kibanamachine

Loading

watson Jan 10, 2023

watson Jan 10, 2023

kibanamachine commented Jan 25, 2023

watson commented Feb 15, 2023

kqualters-elastic commented Feb 15, 2023

watson commented Feb 16, 2023

Upgrade Node.js from v16.18.1 to v18.13.0 #144012

Upgrade Node.js from v16.18.1 to v18.13.0 #144012

Conversation

watson commented Oct 26, 2022 • edited by kibanamachine Loading

Note to reviewers

Failing cypress test suites

Dates

Stream race condition

Upgraded dependencies

Blockers:

watson Jan 10, 2023

Choose a reason for hiding this comment

watson Jan 10, 2023

Choose a reason for hiding this comment

kibanamachine commented Jan 25, 2023

watson commented Feb 15, 2023

💚 All backports created successfully

Questions ?

kqualters-elastic commented Feb 15, 2023

watson commented Feb 16, 2023

watson commented Oct 26, 2022 •

edited by kibanamachine

Loading