Skip to content

Conversation

trentm
Copy link
Member

@trentm trentm commented Jan 14, 2023

This change updates the argument munging that is done for the wrapping
of http.request et al to fix some cases where the instrumentation
would change the behaviour of the function. The main fix here is
that before this change the conversion of a URL instance to request
options would accidentally drop basic auth info (the 'username' and
'password' fields).

This issue has existed since v2.17.0. It was introduced in commit dd60b12
when switching directly from url.parse() to new URL(). Recent
versions of node have a url.urlToHttpOptions() to help with this
conversion.

Fixes: #2044
Fixes: #2382
Refs: https://discuss.elastic.co/t/issue-apm-agent-with-backend-requests-username-password-url/322903

This change updates the argument munging that is done for the wrapping
of `http.request` et al to fix some cases where the instrumentation
would *change the behaviour* of the function.  The main fix here is
that before this change the conversion of a URL instance to request
options would accidentally drop basic auth info (the 'username' and
'password' fields).

This issue has existed since v2.17.0. It was introduced in commit dd60b12
when switching directly from `url.parse()` to `new URL()`. Recent
versions of node have a `url.urlToHttpOptions()` to help with this
conversion.

Fixes: #2044
Refs: https://discuss.elastic.co/t/issue-apm-agent-with-backend-requests-username-password-url/322903
@trentm trentm self-assigned this Jan 14, 2023
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jan 14, 2023
@trentm
Copy link
Member Author

trentm commented Jan 14, 2023

The cleaning up http.request options handling also fixes #2382.

repro

Run this server:

% cat issue2382-server.example.js
const express = require('express')
const app = express()
app.get('/ping', (req, res) => {
  res.send('pong')
})
app.listen(3000)

% node issue2382-server.example.js

Then run this client request script (issue2382.example.js):

require('.').start({ // elastic-apm-node
  serviceName: 'example-outgoing-http-ipv6-boom',
  cloudProvider: 'none',
  metricsInterval: '0s',
  centralConfig: false,
  captureExceptions: false,
  logUncaughtExceptions: true,
  disableSend: true
})

var http = require('http')
var theUrl = 'http://[::1]:3000/ping'
http.get(theUrl, function (res) {
  console.log('res:', res.statusCode, res.headers)
  res.on('data', (chunk) => { console.log('data: %s', chunk) })
  res.on('end', () => { console.log('end') })
})

without the fix:

% node issue2382.example.js
node:events:491
      throw er; // Unhandled 'error' event
      ^

Error: getaddrinfo ENOTFOUND [::1]
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:109:26)
Emitted 'error' event on ClientRequest instance at:
    at Socket.socketErrorListener (node:_http_client:494:9)
    at Socket.emit (node:events:513:28)
    at emitErrorNT (node:internal/streams/destroy:157:8)
    at emitErrorCloseNT (node:internal/streams/destroy:122:3)
    at processTicksAndRejections (node:internal/process/task_queues:83:21) {
  errno: -3008,
  code: 'ENOTFOUND',
  syscall: 'getaddrinfo',
  hostname: '[::1]'
}

and with this PR:

% node issue2382.example.js
res: 200 {
  'x-powered-by': 'Express',
  'content-type': 'text/html; charset=utf-8',
  'content-length': '4',
  etag: 'W/"4-DlFKBmK8tp3IY5U9HOJuPUDoGoc"',
  date: 'Sat, 14 Jan 2023 00:35:18 GMT',
  connection: 'close'
}
data: pong
end

@trentm trentm changed the title fix: instrumentation of http|https.request|get has a few edge cases fix: instrumentation of http.request et al has a few edge cases Jan 14, 2023
@ghost
Copy link

ghost commented Jan 14, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-01-17T00:15:02.398+0000

  • Duration: 23 min 19 sec

Test stats 🧪

Test Results
Failed 0
Passed 321772
Skipped 0
Total 321772

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

  • run benchmark tests : Run the benchmark test only.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@trentm trentm added this to the 8.7 milestone Jan 16, 2023
trentm and others added 7 commits January 16, 2023 10:31
…oosey-goosey usage of 'http.request(...)' et al
…3100)

The test for `error.exception.module` for a reported APM error using
`@elastic/elasticsearch` broke with the recent `8.6.0` version...  as
installed by the npm in node v14.

The issue was that the install tree changed such that the error stack
top frame was in:
    node_modules/@elastic/elasticsearch/node_modules/@elastic/transport/lib/Transport.js
rather than:
    node_modules/@elastic/transport/lib/Transport.js
for reasons independent of the `@elastic/[email protected]` release.
The `_moduleNameFromFrames` utility didn't handle a nested
`node_modules/...` dir.  This change fixes that.
Support for tracing/monitoring Azure Functions.
Supported triggers/bindings: HTTP (spec'd), Timer (not spec'd).

Spec: elastic/apm#716
Closes: #3015
Co-authored-by: Brandon Morelli <[email protected]>
@trentm trentm merged commit ef26a69 into main Jan 17, 2023
@trentm trentm deleted the trentm/fix-broken-http-request-with-auth branch January 17, 2023 00:43
trentm added a commit that referenced this pull request Jan 18, 2023
The 'http.request' instrumentation fix in #3090 uncovered a bug in our
tests for @hapi/hapi when with node v8 (a version before
`http.request(url, opts, cb)` was supported in node). This fixes that.

This also updates the .tav.yml section for '@hapi/hapi' to align with
logic in "_is_hapi_incompat.js" so the TAV tests don't waste time
installing a version of Hapi for which tests will be skipped anyway.

Refs: #3090
trentm added a commit that referenced this pull request Jan 18, 2023
The 'http.request' instrumentation fix in #3090 uncovered a bug in our
tests for @hapi/hapi when with node v8 (a version before
`http.request(url, opts, cb)` was supported in node). This fixes that.

This also updates the .tav.yml section for '@hapi/hapi' to align with
logic in "_is_hapi_incompat.js" so the TAV tests don't waste time
installing a version of Hapi for which tests will be skipped anyway.

Refs: #3090
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
…astic#3090)

This change updates the argument munging that is done for the wrapping
of `http.request` et al to fix some cases where the instrumentation
would *change the behaviour* of the function.  The main fix here is
that before this change the conversion of a URL instance to request
options would accidentally drop basic auth info (the 'username' and
'password' fields).

This issue has existed since v2.17.0. It was introduced in commit dd60b12
when switching directly from `url.parse()` to `new URL()`. Recent
versions of node have a `url.urlToHttpOptions()` to help with this
conversion.

Fixes: elastic#2044
Fixes: elastic#2382
Refs: https://discuss.elastic.co/t/issue-apm-agent-with-backend-requests-username-password-url/322903
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
The 'http.request' instrumentation fix in elastic#3090 uncovered a bug in our
tests for @hapi/hapi when with node v8 (a version before
`http.request(url, opts, cb)` was supported in node). This fixes that.

This also updates the .tav.yml section for '@hapi/hapi' to align with
logic in "_is_hapi_incompat.js" so the TAV tests don't waste time
installing a version of Hapi for which tests will be skipped anyway.

Refs: elastic#3090
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

outgoing http.request instrumentation exacerbates node issue with ipv6 addresses instrumentation of http{s}.request() has a few edge case issues
1 participant