feat: support node 16; also test with node 15, drop testing of node 13 #2055

trentm · 2021-04-21T16:12:23Z

This borrows work from @kevcodez and myself on #1841.

Fixes: #2053

Checklist

Implement code
Add tests
Add CHANGELOG.asciidoc entry
Commit message follows commit guidelines

@kevcodez

This borrows work from @kevcodez and myself on #1841. Fixes: #2053

apmmachine · 2021-04-21T16:28:10Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #2055 updated
Start Time: 2021-04-30T02:10:58.978+0000
Duration: 17 min 42 sec
Commit: 4430f84

Test stats 🧪

Test	Results
Failed	0
Passed	20944
Skipped	0
Total	20944

Trends 🧪

trentm · 2021-04-21T17:26:53Z

^^^ node v10.0.0 test failed in node test/agent.js

 # #addTransactionFilter() - abort with 'null'
not ok 260 plan != count
  ---
    operator: fail
    expected: 1
    actual:   0
    at: Timeout._onTimeout (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/agent.js:713:13)
    stack: |-
      Error: plan != count
          at Test.assert [as _assert] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:304:54)
          at Test.bound [as _assert] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:91:32)
          at Test.fail (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:398:10)
          at Test.bound [as fail] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:91:32)
          at completeEnd (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:243:18)
          at next (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:217:13)
          at Test._end (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:236:5)
          at Test.bound [as _end] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:91:32)
          at Test.end (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:191:10)
          at Test.bound [as end] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:91:32)
          at Timeout._onTimeout (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/agent.js:713:13)
          at ontimeout (timers.js:427:11)
          at tryOnTimeout (timers.js:289:5)
          at listOnTimeout (timers.js:252:5)
          at Timer.processTimers (timers.js:212:10)
  ...

And some other similar errors. The only thing in my change that could possibly have effected this is the .npmrc change, and I can't imagine it did. I'll re-run the tests.

trentm · 2021-04-21T17:36:23Z

The apm-ci.elastic.co Jenkins tests have failed because a "node:16" docker image has not yet been published: https://hub.docker.com/_/node

nodejs/docker-node#1465 is the PR for this, I believe.

FWIW the tests for node 15 and other versions passed in the Jenkins tests: https://apm-ci.elastic.co/blue/organizations/jenkins/apm-agent-nodejs%2Fapm-agent-nodejs-mbp/detail/PR-2055/1/pipeline

trentm · 2021-04-21T17:38:03Z

The GitHub Actions 10.0 node tests passed on a re-run, so we have some possibly flaky tests there.

trentm · 2021-04-21T17:46:04Z

jenkins run the module tests

trentm · 2021-04-21T20:48:46Z

FWIW, a full tav test ran for me on macos with node 16.0.0:

% ./node_modules/.bin/tav -q
-- required packages ["[email protected]"]
-- installing ["[email protected]"]
-- running test "node test/instrumentation/modules/aws-sdk/sqs.js" with aws-sdk
-- required packages ["[email protected]"]
-- installing ["[email protected]"]
...
-- required packages ["[email protected]"]
-- installing ["[email protected]"]
-- running test "node test/instrumentation/modules/generic-pool.js" with generic-pool
-- required packages ["[email protected]"]
-- installing ["[email protected]"]
-- running test "node test/instrumentation/modules/generic-pool.js" with generic-pool
-- ok

% echo $?
0

% node --version
v16.0.0

trentm · 2021-04-22T15:08:20Z

jenkins run the module tests

trentm · 2021-04-22T15:29:46Z

Now waiting for this PR (docker-library/official-images#10030) to get merged and docker.io/library/node:16 to be published.

trentm · 2021-04-27T16:17:20Z

There are a couple test failures I don't grok yet.

trentm · 2021-04-27T17:58:24Z

One test failure is in our Jenkins tests with node 16:

% .ci/scripts/test.sh "16" "" "false"
...
node_tests_1     | # http2.createSecureServer compatibility mode
node_tests_1     | node:internal/tls:91
node_tests_1     |     context.setCert(cert);
node_tests_1     |             ^
node_tests_1     |
node_tests_1     | Error: error:140AB18F:SSL routines:SSL_CTX_use_certificate:ee key too small
node_tests_1     |     at node:internal/tls:91:13
node_tests_1     |     at Array.forEach (<anonymous>)
node_tests_1     |     at setCerts (node:internal/tls:89:3)
node_tests_1     |     at configSecureContext (node:internal/tls:178:5)
node_tests_1     |     at Object.createSecureContext (node:_tls_common:113:3)
node_tests_1     |     at Http2SecureServer.Server.setSecureContext (node:_tls_wrap:1353:27)
node_tests_1     |     at new Server (node:_tls_wrap:1209:8)
node_tests_1     |     at new Http2SecureServer (node:internal/http2/core:3107:5)
node_tests_1     |     at Object.createSecureServer (node:internal/http2/core:3298:10)
node_tests_1     |     at Object.wrappedCreateServer [as createSecureServer] (/app/lib/instrumentation/modules/http2.js:28:29)
node_tests_1     |     at Test.<anonymous> (/app/test/instrumentation/modules/http2.js:47:15)
node_tests_1     |     at Test.bound [as _cb] (/app/node_modules/tape/lib/test.js:91:32)
node_tests_1     |     at Test.run (/app/node_modules/tape/lib/test.js:108:31)
node_tests_1     |     at Test.bound [as run] (/app/node_modules/tape/lib/test.js:91:32)
node_tests_1     |     at Immediate.next (/app/node_modules/tape/lib/results.js:89:19)
node_tests_1     |     at Immediate.elasticAPMCallbackWrapper (/app/lib/instrumentation/index.js:358:27)
node_tests_1     |     at processImmediate (node:internal/timers:464:21) {
node_tests_1     |   library: 'SSL routines',
node_tests_1     |   function: 'SSL_CTX_use_certificate',
node_tests_1     |   reason: 'ee key too small',
node_tests_1     |   code: 'ERR_SSL_EE_KEY_TOO_SMALL'
node_tests_1     | }
node_tests_1     | /app/test/test.js:135
node_tests_1     |     if (err) throw err
node_tests_1     |              ^

I can reproduce with tests running in Docker, but not on my Mac via node --unhandled-rejections=strict test/instrumentation/modules/http2.js using node 16.

It turns out that OpenSSL's SSL_CTX_use_certificate() will return that "ee key too small" depending on the parameters of the given key/certificate measured against the OpenSSL security level ("SECLEVEL"). And notice that the new "node:16" default docker image changed to set the SECLEVEL to 2:

% docker run --rm -ti node:14 grep -r SECLEVEL /etc/ssl
% docker run --rm -ti node:16 grep -r SECLEVEL /etc/ssl
/etc/ssl/openssl.cnf:CipherString = DEFAULT@SECLEVEL=2

trentm · 2021-04-27T18:05:58Z

I can repro the "ee key too small" outside of docker with a custom openssl config passed to node:

% cat my-openssl-config.cnf
# Based on https://github.com/nodejs/node/issues/36655
openssl_conf = openssl_init

[openssl_init]
ssl_conf = ssl_sect

[ssl_sect]
system_default = system_default_sect

[system_default_sect]
CipherString = DEFAULT:@SECLEVEL=2
[11:04:47 trentm@pink:~/el/apm-agent-nodejs-node16 (n:16 git:trentm/node16)]
% node --openssl-config=./my-openssl-config.cnf  test/instrumentation/modules/http2.js
TAP version 13
# http2.createServer compatibility mode
ok 1 have current transaction
ok 2 should be strictly equal
ok 3 should be strictly equal
ok 4 should be strictly equal
ok 5 should be truthy
ok 6 should be truthy
ok 7 should be strictly equal
ok 8 should be strictly equal
ok 9 should be strictly equal
ok 10 should be strictly equal
ok 11 should be truthy
ok 12 should be truthy
ok 13 should be deeply equivalent
ok 14 should be loosely deeply equivalent
ok 15 should have expected body
# http2.createServer stream respond
ok 16 have current transaction
ok 17 should be strictly equal
ok 18 should be strictly equal
ok 19 should be strictly equal
ok 20 should be truthy
ok 21 should be truthy
ok 22 should be strictly equal
ok 23 should be strictly equal
ok 24 should be strictly equal
ok 25 should be strictly equal
ok 26 should be truthy
ok 27 should be truthy
ok 28 should be deeply equivalent
ok 29 should be loosely deeply equivalent
ok 30 should have expected body
# http2.createServer stream respondWithFD
ok 31 have current transaction
ok 32 should be strictly equal
ok 33 null
ok 34 should be strictly equal
ok 35 should be strictly equal
ok 36 should be truthy
ok 37 should be truthy
ok 38 should be strictly equal
ok 39 should be strictly equal
ok 40 should be strictly equal
ok 41 should be strictly equal
ok 42 should be truthy
ok 43 should be truthy
ok 44 should be deeply equivalent
ok 45 should be loosely deeply equivalent
ok 46 should have expected body
# http2.createServer stream respondWithFile
ok 47 have current transaction
ok 48 should be strictly equal
ok 49 should be strictly equal
ok 50 should be strictly equal
ok 51 should be truthy
ok 52 should be truthy
ok 53 should be strictly equal
ok 54 should be strictly equal
ok 55 should be strictly equal
ok 56 should be strictly equal
ok 57 should be truthy
ok 58 should be truthy
ok 59 should be deeply equivalent
ok 60 should be loosely deeply equivalent
ok 61 should have expected body
# http2.createServer ignore push streams
ok 62 have current transaction
ok 63 should be strictly equal
ok 64 null
ok 65 should be strictly equal
ok 66 should be strictly equal
ok 67 should be truthy
ok 68 should be truthy
ok 69 should be strictly equal
ok 70 should be strictly equal
ok 71 should be strictly equal
ok 72 should be strictly equal
ok 73 should be truthy
ok 74 should be truthy
ok 75 should be deeply equivalent
ok 76 should be loosely deeply equivalent
ok 77 should be strictly equal
ok 78 should have expected body
ok 79 should have expected body
ok 80 should have called function
ok 81 should have called function
# http2.request
ok 82 have current transaction
ok 83 should be strictly equal
ok 84 have current transaction
ok 85 should be strictly equal
ok 86 should be strictly equal
ok 87 should be strictly equal
ok 88 should be truthy
ok 89 should be truthy
ok 90 should be strictly equal
ok 91 should be strictly equal
ok 92 should be strictly equal
ok 93 should be strictly equal
ok 94 should be truthy
ok 95 should be truthy
ok 96 should be deeply equivalent
ok 97 should be loosely deeply equivalent
ok 98 should be truthy
ok 99 should be truthy
ok 100 should be strictly equal
ok 101 should be strictly equal
ok 102 should be strictly equal
ok 103 should be strictly equal
ok 104 should be truthy
ok 105 should be truthy
ok 106 should be deeply equivalent
ok 107 should be loosely deeply equivalent
ok 108 root transaction should have span
ok 109 should be strictly equal
ok 110 should be strictly equal
ok 111 should be strictly equal
ok 112 should be deeply equivalent
ok 113 should be deeply equivalent
ok 114 should have expected body
# http2.createSecureServer compatibility mode
node:internal/tls:91
    context.setCert(cert);
            ^

Error: error:140AB18F:SSL routines:SSL_CTX_use_certificate:ee key too small
    at node:internal/tls:91:13
    at Array.forEach (<anonymous>)
    at setCerts (node:internal/tls:89:3)
    at configSecureContext (node:internal/tls:178:5)
    at Object.createSecureContext (node:_tls_common:113:3)
    at Http2SecureServer.Server.setSecureContext (node:_tls_wrap:1353:27)
    at new Server (node:_tls_wrap:1209:8)
    at new Http2SecureServer (node:internal/http2/core:3107:5)
    at Object.createSecureServer (node:internal/http2/core:3298:10)
    at Object.wrappedCreateServer [as createSecureServer] (/Users/trentm/el/apm-agent-nodejs-node16/lib/instrumentation/modules/http2.js:28:29)
    at Test.<anonymous> (/Users/trentm/el/apm-agent-nodejs-node16/test/instrumentation/modules/http2.js:50:15)
    at Test.bound [as _cb] (/Users/trentm/el/apm-agent-nodejs-node16/node_modules/tape/lib/test.js:91:32)
    at Test.run (/Users/trentm/el/apm-agent-nodejs-node16/node_modules/tape/lib/test.js:108:31)
    at Test.bound [as run] (/Users/trentm/el/apm-agent-nodejs-node16/node_modules/tape/lib/test.js:91:32)
    at Immediate.next (/Users/trentm/el/apm-agent-nodejs-node16/node_modules/tape/lib/results.js:89:19)
    at processImmediate (node:internal/timers:464:21)
    at process.callbackTrampoline (node:internal/async_hooks:131:17) {
  library: 'SSL routines',
  function: 'SSL_CTX_use_certificate',
  reason: 'ee key too small',
  code: 'ERR_SSL_EE_KEY_TOO_SMALL'
}

trentm · 2021-04-27T18:14:14Z

My understanding is that I should be able to override that default configured SECLEVEL via the ciphers argument like this:

diff --git a/test/instrumentation/modules/http2.js b/test/instrumentation/modules/http2.js
index 46234e1..6e5c9c6 100644
--- a/test/instrumentation/modules/http2.js
+++ b/test/instrumentation/modules/http2.js
@@ -43,7 +46,7 @@ isSecure.forEach(secure => {

     var port
     var server = secure
-      ? http2.createSecureServer(pem, onRequest)
+      ? http2.createSecureServer({ ciphers: 'ALL@SECLEVEL=0', ...pem }, onRequest)
       : http2.createServer(onRequest)

     var onError = err => t.error(err)

However, that may be failing because of nodejs/node#36655

…ss in node:16 docker image

trentm · 2021-04-27T19:22:45Z

The other issue is there is some hang in the node v16 runs on GH Actions. The log includes no "not ok" test failures.

trentm · 2021-04-27T20:25:59Z

Jenkins tests for node 16 are now failing at the same thing.

I was able to repro the hang locally on macOS. It requires, @hapi/[email protected] and node v16, then:

% npm install --no-save @hapi/[email protected]
...

% node --version
v16.0.0

% node test/sanitize-field-names/hapi.js
TAP version 13
# Running fixtures with hapi
# tests default wildcard handling, with urlencode bodyparsing

small separate repro

Save this as "notsohapi.js":

const Hapi = require('@hapi/hapi')
async function setup () {
  const server = Hapi.server({ host: 'localhost', port: '12345' })
  server.route({
    method: 'GET',
    path: '/',
    handler: (request, h) => {
      // return 'hi' // this works
      return h.response('hi') // this hangs with node v16 + @hapi/[email protected]
    }
  })
  await server.start()
  console.log('server listening at', server.info.uri)
}

setup()

Ensure you have @hapi/[email protected]: npm install --no-save @hapi/[email protected].

Run that script with node 16 and attempt to: curl -i http://localhost:12345/ -X POST -d foo=bar. That will hang.

Fixes

add a timeout to the test case so we would not have had this indefinite hang
just update our base version of @hapi/hapi to v20. v18 is labelled as no longer supported. Note that our TAV tests that test with older hapi versions do not hit this: likely because the tests run for each version are not testing "POST" endpoints.

…move on to hapi v20

… v10 and below

…penssl support issues; should be fine as the only current need we had for it was for node:16; node v8 is EOL anyway

…checking and add to the set-framework-*.js test files

trentm · 2021-04-28T16:29:53Z

package.json

@@ -115,17 +115,16 @@
    "@babel/core": "^7.8.4",
    "@babel/preset-env": "^7.8.4",
    "@elastic/elasticsearch": "^7.10.0",
-    "body-parser": "^1.19.0",
-    "@hapi/hapi": "^18.4.1",
+    "@hapi/hapi": "^20.1.2",


The "POST" handler test in test/instrumentation/modules/hapi.js hangs with @hapi/[email protected] and node 16. Let's just move to testing with the latest hapi (which does mean only testing with node v12 and later). The TAV tests cover the rest.

trentm · 2021-04-28T18:53:20Z

jenkins run the module tests

trentm · 2021-04-29T02:11:17Z

All tests (regular, TAV, edge) passing: https://apm-ci.elastic.co/blue/organizations/jenkins/apm-agent-nodejs%2Fapm-agent-nodejs-mbp/detail/PR-2055/19/pipeline/

apmmachine · 2021-04-30T18:15:38Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #2055 updated
Start Time: 2021-04-30T17:57:51.627+0000
Duration: 17 min 28 sec
Commit: 38eea4b

Test stats 🧪

Test	Results
Failed	0
Passed	20944
Skipped	0
Total	20944

Trends 🧪

feat: support node 16; also test with node 15, drop testing of node 13

ce31964

This borrows work from @kevcodez and myself on #1841. Fixes: #2053

trentm self-assigned this Apr 21, 2021

github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Apr 21, 2021

trentm mentioned this pull request Apr 21, 2021

feta(node): add suppport to node 16 #2054

Closed

6 tasks

trentm mentioned this pull request Apr 22, 2021

Add support for Node.js 15/16 #1840

Closed

trentm added 2 commits April 22, 2021 11:03

enable node 16/15 testing for the GH Actions tests as well

a151e84

Merge branch 'master' into trentm/node16

d6a4085

trentm requested a review from astorm April 26, 2021 21:55

ensure OpenSSL SECLEVEL=0 for tests, required for http2.js test to pa…

c06f853

…ss in node:16 docker image

trentm removed the request for review from astorm April 27, 2021 19:23

fix hang with POST-handler + @hapi/[email protected] + node v16; we can just …

b621866

…move on to hapi v20

trentm added 7 commits April 27, 2021 14:37

switching to hapi@20 means we need to skip out of hapi tests for node…

c0b8ad5

… v10 and below

avoid the openssl-config fix in NODE_OPTIONS for node v8 because of o…

672a72a

…penssl support issues; should be fine as the only current need we had for it was for node:16; node v8 is EOL anyway

fix breakage

94ce459

yet another place to check hapi versions for skipping

563bda1

correct that hapi package version check

80ea59f

correct this bash test

7793282

fix logic for --openssl-config in NODE_OPTIONS; rejigger hapi compat …

dd960d6

…checking and add to the set-framework-*.js test files

trentm commented Apr 28, 2021

View reviewed changes

tyops

6748685

correct tav config for my test file renamings (serves me right)

7d8e67c

trentm requested a review from astorm April 29, 2021 02:11

trentm and others added 2 commits April 28, 2021 19:11

Merge branch 'master' into trentm/node16

5aa0889

Merge branch 'master' into trentm/node16

4430f84

astorm previously approved these changes Apr 30, 2021

View reviewed changes

trentm added 2 commits April 30, 2021 10:57

Use the common hapi-version-compat check in this test case as well.

0e04422

Add a changelog entry

38eea4b

trentm dismissed astorm’s stale review via 38eea4b April 30, 2021 17:57

trentm merged commit 7714701 into master Apr 30, 2021

trentm deleted the trentm/node16 branch April 30, 2021 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support node 16; also test with node 15, drop testing of node 13 #2055

feat: support node 16; also test with node 15, drop testing of node 13 #2055

trentm commented Apr 21, 2021 •

edited

Loading

apmmachine commented Apr 21, 2021 •

edited

Loading

Build stats

Test stats 🧪

Trends 🧪

trentm commented Apr 21, 2021 •

edited

Loading

trentm commented Apr 21, 2021 •

edited

Loading

trentm commented Apr 21, 2021

trentm commented Apr 21, 2021

trentm commented Apr 21, 2021

trentm commented Apr 22, 2021

trentm commented Apr 22, 2021

trentm commented Apr 27, 2021

trentm commented Apr 27, 2021

trentm commented Apr 27, 2021

trentm commented Apr 27, 2021

trentm commented Apr 27, 2021

trentm commented Apr 27, 2021

trentm Apr 28, 2021

trentm commented Apr 28, 2021

trentm commented Apr 29, 2021

apmmachine commented Apr 30, 2021

Build stats

Test stats 🧪

Trends 🧪

feat: support node 16; also test with node 15, drop testing of node 13 #2055

feat: support node 16; also test with node 15, drop testing of node 13 #2055

Conversation

trentm commented Apr 21, 2021 • edited Loading

Checklist

apmmachine commented Apr 21, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

trentm commented Apr 21, 2021 • edited Loading

trentm commented Apr 21, 2021 • edited Loading

trentm commented Apr 21, 2021

trentm commented Apr 21, 2021

trentm commented Apr 21, 2021

trentm commented Apr 22, 2021

trentm commented Apr 22, 2021

trentm commented Apr 27, 2021

trentm commented Apr 27, 2021

trentm commented Apr 27, 2021

trentm commented Apr 27, 2021

trentm commented Apr 27, 2021

trentm commented Apr 27, 2021

small separate repro

Fixes

trentm Apr 28, 2021

Choose a reason for hiding this comment

trentm commented Apr 28, 2021

trentm commented Apr 29, 2021

apmmachine commented Apr 30, 2021

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

trentm commented Apr 21, 2021 •

edited

Loading

apmmachine commented Apr 21, 2021 •

edited

Loading

trentm commented Apr 21, 2021 •

edited

Loading

trentm commented Apr 21, 2021 •

edited

Loading