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

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

Merged
merged 18 commits into from
Jan 17, 2023
Merged

Conversation

watson
Copy link
Contributor

@watson watson commented Oct 26, 2022

Closes #134930

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:

@watson watson force-pushed the bump-node-18 branch 8 times, most recently from a9ea88a to 9816942 Compare November 2, 2022 13:45
@watson 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 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 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 watson force-pushed the bump-node-18 branch 4 times, most recently from d18da9e to 7237481 Compare January 9, 2023 13:51
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 || '';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@watson watson force-pushed the bump-node-18 branch 2 times, most recently from 3acd0e0 to c56c392 Compare January 11, 2023 07:32
@watson watson added backport:prev-major Backport to (8.x, 8.16, 8.15) the previous major branch and all later branches still in development release_note:skip Skip the PR/issue when compiling release notes labels Jan 11, 2023
@watson watson added v7.17.9 and removed backport:prev-major Backport to (8.x, 8.16, 8.15) the previous major branch and all later branches still in development labels Jan 24, 2023
@kibanamachine
Copy link
Contributor

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 watson added backport:skip This commit does not require backporting and removed v7.17.9 labels Jan 25, 2023
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 25, 2023
@watson
Copy link
Contributor Author

watson commented Feb 15, 2023

💚 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

watson pushed a commit to watson/kibana that referenced this pull request Feb 15, 2023
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)

# Conflicts:
#	.buildkite/package-lock.json
#	.buildkite/package.json
#	docs/developer/advanced/upgrading-nodejs.asciidoc
#	package.json
#	packages/kbn-monaco/BUILD.bazel
#	packages/kbn-optimizer/src/optimizer/observe_worker.ts
#	packages/kbn-ui-shared-deps-npm/BUILD.bazel
#	packages/kbn-ui-shared-deps-src/BUILD.bazel
#	src/dev/build/tasks/patch_native_modules_task.ts
#	x-pack/plugins/actions/server/lib/request_oauth_jwt_token.test.ts
#	x-pack/plugins/actions/server/lib/request_oauth_token.test.ts
#	x-pack/plugins/reporting/server/lib/tasks/execute_report.ts
#	x-pack/plugins/synthetics/server/synthetics_service/service_api_client.test.ts
#	yarn.lock
@kqualters-elastic
Copy link
Contributor

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
Copy link
Contributor Author

watson commented Feb 16, 2023

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?

@watson watson mentioned this pull request Jul 18, 2023
4 tasks
delanni added a commit that referenced this pull request Jul 27, 2023
## 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>
delanni added a commit to delanni/kibana that referenced this pull request Jul 28, 2023
## 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>
jbudz added a commit that referenced this pull request Aug 1, 2023
\>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
jbudz added a commit to jbudz/kibana that referenced this pull request Aug 1, 2023
\>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
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 1, 2023
\>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)
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
## 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>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
\>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
jbudz added a commit that referenced this pull request Aug 1, 2023
Backports #162833
Partially backports #144012 via
`x-pack/plugins/fleet/server/services/epm/archive/extract.ts`
Needed for #162722
delanni added a commit that referenced this pull request Aug 15, 2023
Closes #162695

# Backport

This will backport the following commits from `main` to `7.17`:
- [[Ops] Bump Node.js to version 18
(#160289)](#160289)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Alex
Szabo","email":"alex.szabo@elastic.co"},"sourceCommit":{"committedDate":"2023-07-27T12:12:48Z","message":"[Ops]
Bump Node.js to version 18 (#160289)\n\n## Summary\r\n\r\nBumps node.js
to 18.17.0 (replacement for PR #144012 which was
later\r\nreverted)\r\n\r\nAs a result, these categorical additions were
needed: \r\n- `node` evocations will need the
`--openssl-legacy-provider` flag,\r\nwherever it would use certain
crypto functionalities\r\n- tests required updating of the expected
HTTPS Agent call arguments,\r\n`noDelay` seems to be a default\r\n -
`window.[NAME]` fields cannot be written directly\r\n - some stricter
typechecks\r\n\r\nThis is using our in-house built node.js 18 versions
through the URLs\r\nthe proxy-cache. (built
with\r\nhttps://github.com/elastic/kibana-custom-nodejs-builds/pull/4)\r\n\r\nThese
urls are served from a bucket, where the RHEL7/Centos7
compatible\r\nnode distributables are.
(see:\r\nhttps://github.com/elastic/kibana-ci-proxy-cache/pull/7)\r\n\r\nFurther
todos: \r\n - [x] check docs wording and consistency\r\n - [ ] update
the dependency report\r\n - [x] explain custom builds in
documentation\r\n - [x] node_sass
prebuilts\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Tiago Costa <tiago.costa@elastic.co>\r\nCo-authored-by: Thomas Watson
<w@tson.dk>","sha":"8cf68dc6ba8f010e36538c1e7c92601a341efcf4","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Operations","Team:uptime","backport:skip","release_note:feature","ci:all-cypress-suites","v8.10.0"],"number":160289,"url":"https://github.com/elastic/kibana/pull/160289","mergeCommit":{"message":"[Ops]
Bump Node.js to version 18 (#160289)\n\n## Summary\r\n\r\nBumps node.js
to 18.17.0 (replacement for PR #144012 which was
later\r\nreverted)\r\n\r\nAs a result, these categorical additions were
needed: \r\n- `node` evocations will need the
`--openssl-legacy-provider` flag,\r\nwherever it would use certain
crypto functionalities\r\n- tests required updating of the expected
HTTPS Agent call arguments,\r\n`noDelay` seems to be a default\r\n -
`window.[NAME]` fields cannot be written directly\r\n - some stricter
typechecks\r\n\r\nThis is using our in-house built node.js 18 versions
through the URLs\r\nthe proxy-cache. (built
with\r\nhttps://github.com/elastic/kibana-custom-nodejs-builds/pull/4)\r\n\r\nThese
urls are served from a bucket, where the RHEL7/Centos7
compatible\r\nnode distributables are.
(see:\r\nhttps://github.com/elastic/kibana-ci-proxy-cache/pull/7)\r\n\r\nFurther
todos: \r\n - [x] check docs wording and consistency\r\n - [ ] update
the dependency report\r\n - [x] explain custom builds in
documentation\r\n - [x] node_sass
prebuilts\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Tiago Costa <tiago.costa@elastic.co>\r\nCo-authored-by: Thomas Watson
<w@tson.dk>","sha":"8cf68dc6ba8f010e36538c1e7c92601a341efcf4"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/160289","number":160289,"mergeCommit":{"message":"[Ops]
Bump Node.js to version 18 (#160289)\n\n## Summary\r\n\r\nBumps node.js
to 18.17.0 (replacement for PR #144012 which was
later\r\nreverted)\r\n\r\nAs a result, these categorical additions were
needed: \r\n- `node` evocations will need the
`--openssl-legacy-provider` flag,\r\nwherever it would use certain
crypto functionalities\r\n- tests required updating of the expected
HTTPS Agent call arguments,\r\n`noDelay` seems to be a default\r\n -
`window.[NAME]` fields cannot be written directly\r\n - some stricter
typechecks\r\n\r\nThis is using our in-house built node.js 18 versions
through the URLs\r\nthe proxy-cache. (built
with\r\nhttps://github.com/elastic/kibana-custom-nodejs-builds/pull/4)\r\n\r\nThese
urls are served from a bucket, where the RHEL7/Centos7
compatible\r\nnode distributables are.
(see:\r\nhttps://github.com/elastic/kibana-ci-proxy-cache/pull/7)\r\n\r\nFurther
todos: \r\n - [x] check docs wording and consistency\r\n - [ ] update
the dependency report\r\n - [x] explain custom builds in
documentation\r\n - [x] node_sass
prebuilts\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Tiago Costa <tiago.costa@elastic.co>\r\nCo-authored-by: Thomas Watson
<w@tson.dk>","sha":"8cf68dc6ba8f010e36538c1e7c92601a341efcf4"}}]}]
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:all-cypress-suites ci:build-all-platforms release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Node 18.x