fix: handle Node 10 style http requests by AaronFriel · Pull Request #1233 · googleapis/cloud-trace-nodejs

AaronFriel · 2020-04-10T02:08:07Z

In Node 10 and above and libraries which use the request(url, options, cb?) overload of the request function such as got 10.x, request tracing fails.

Environment details

OS: any
Node.js version: 10.x
npm version: 6.x
@google-cloud/trace-agent version: 4.5.2

Steps to reproduce

Use got version 10.x or manually invoke a request with a separate URL and options argument
In a root span, try to do a got.post or any other request as above and log the x-cloud-trace-header in the server receiving the request
Observe that the request isn't traced and the trace header is absent

Bug

The bug is in three parts:

If condition filters out valid requests:

cloud-trace-nodejs/src/plugins/plugin-http.ts

Lines 95 to 113 in ac7e886

    
           function makeRequestTrace( 
        
             protocol: string, 
        
             request: RequestFunction, 
        
             api: Tracer 
        
           ): RequestFunction { 
        
             // On Node 8+ we use the following function to patch both request and get. 
        
             // Here `request` may also happen to be `get`. 
        
             return function requestTrace( 
        
               this: never, 
        
               url: httpModule.RequestOptions | string | URL, 
        
               options?: 
        
                 | httpModule.RequestOptions 
        
                 | ((res: httpModule.IncomingMessage) => void), 
        
               callback?: (res: httpModule.IncomingMessage) => void 
        
             ): ClientRequest { 
        
               // These are error conditions; defer to http.request and don't trace. 
        
               if (!url || (typeof url === 'object' && typeof options === 'object')) { 
        
                 return request.apply(this, arguments); 
        
               }

When got 10 makes a request, it sends three arguments: url (as an object), options (as an object), and callback (as a function). This if condition blocks tracing all requests from got 10.

Object.assign does not work for URL properties because URL properties are not enumerable.

cloud-trace-nodejs/src/plugins/plugin-http.ts

Lines 114 to 126 in ac7e886

    
           let urlString; 
        
           if (typeof url === 'string') { 
        
             // save the value of uri so we don't have to reconstruct it later 
        
             urlString = url; 
        
             url = urlParse(url); 
        
           } 
        
           if (typeof options === 'function') { 
        
             callback = options; 
        
             options = url; 
        
           } else { 
        
             options = Object.assign({}, url, options); 
        
           }

If the if condition is changed above so that we can fall through to line 125, we hit a second bug. This last options = Object.assign(...) does not work because URL properties are not enumerable. The result is that properties such as "hostname", "host", etc. are not copied into the options.

The wrapped request is called with the deficient object

cloud-trace-nodejs/src/plugins/plugin-http.ts

Line 171 in ac7e886

const req = request(options, res => {

Given the incorrect Object.assign, even if we repair that defect, we end up sending this rewritten object to the destination and we destroy any non-enumerable symbols or other properties assigned to the url or options object we were passed.

Fix

Looking at the source for CreateRequest, the function called by http.request, we see that the "input" argument is always replaced by an object, as opposed to a URL instance, before being ObjectAssigned

https://github.com/nodejs/node/blob/dccdc51788bd5337f9fd80441ef52932383a2441/lib/_http_client.js#L88-L123

We can do the same. Our needs are a bit different, but we can ensure that before the object assign the url value is always converted to a RequestOptions object.

AaronFriel · 2020-04-10T02:09:32Z

@kjin I believe this will unblock the got 10.x upgrade.

AaronFriel · 2020-04-10T18:41:26Z

CC @bcoe, if this could be reviewed and released it'd be great not to rely on a fork!

bcoe · 2020-04-13T22:18:03Z

@AaronFriel thanks for the patch 🙏 I'm not the most well versed in this library, and it would be helpful if we added a few tests.

As it stands today, you can't use the currently published version of @gogole-cloud/trace-agent with got?

In Node 10 and above and libraries which use the `request(url, options, cb?)` overload of the request function such as `got` 10.x, request tracing fails. Environment details ------------------- - OS: any - Node.js version: 10.x - npm version: 6.x - `@google-cloud/trace-agent` version: 4.5.2 Steps to reproduce ------------------ 1. Use `got` version `10.x` or manually invoke a request with a separate URL and options argument 2. In a root span, try to do a `got.post` or any other request as above and log the `x-cloud-trace-header` in the server receiving the request 3. Observe that the request isn't traced and the trace header is absent Bug --- The bug is in three parts: 1. If condition filters out valid requests: https://github.com/googleapis/cloud-trace-nodejs/blob/ac7e886c178ca9c34502e9baa9eb190d23104347/src/plugins/plugin-http.ts#L95-L113 When got 10 makes a request, it sends three arguments: url (as an object), options (as an object), and callback (as a function). This if condition blocks tracing all requests from got 10. 2. `Object.assign` does not work for URL properties because URL properties are not enumerable. https://github.com/googleapis/cloud-trace-nodejs/blob/ac7e886c178ca9c34502e9baa9eb190d23104347/src/plugins/plugin-http.ts#L114-L126 If the if condition is changed above so that we can fall through to line 125, we hit a second bug. This last `options = Object.assign(...)` does not work because URL properties are not enumerable. The result is that properties such as "hostname", "host", etc. are not copied into the options. 3. The wrapped request is called with the deficient object https://github.com/googleapis/cloud-trace-nodejs/blob/ac7e886c178ca9c34502e9baa9eb190d23104347/src/plugins/plugin-http.ts#L171 Given the incorrect Object.assign, even if we repair that defect, we end up sending this rewritten object to the destination and we destroy any non-enumerable symbols or other properties assigned to the url or options object we were passed. Fix --- Looking at the source for CreateRequest, the function called by `http.request`, we see that the "input" argument is always replaced by an object, as opposed to a URL instance, before being `ObjectAssigned` https://github.com/nodejs/node/blob/dccdc51788bd5337f9fd80441ef52932383a2441/lib/_http_client.js#L88-L123 We can do the same. Our needs are a bit different, but we can ensure that before the object assign the url value is always converted to a `RequestOptions` object.

Co-Authored-By: Kelvin Jin <[email protected]>

AaronFriel · 2020-04-14T17:33:55Z

Rebased and resolved conflicts.

bcoe · 2020-04-14T18:38:57Z

@kjin it sounds like Kelvin has good technical feedback 😄

Might I still press you for a test, otherwise it looks like this is coming along great.

AaronFriel · 2020-04-15T00:56:06Z

@bcoe that is correct, got 10 throws an exception in the http plugin source code. And I'm not sure who Kelvin is.

I wrote three tests to be very explicit about how the request function is called, two of which failed (string and URL class instance) as expected, rebasing my branch on that failing test commit resulted in the tests passing. The "parsed URL" test never failed because Object.assign behaves as expected when the URL-ish object is passed in, but should help prevent bitrot.

kjin

Thanks! (I'm Kelvin, by the way)

gcf-merge-on-green · 2020-04-15T22:46:07Z

Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR

gcf-merge-on-green · 2020-04-15T22:48:08Z

Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR

gcf-merge-on-green · 2020-04-16T01:46:07Z

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

bcoe · 2020-04-16T21:29:27Z

@AaronFriel awesome, thank you for adding the regression tests, and for the contribution.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 10, 2020

AaronFriel mentioned this pull request Apr 13, 2020

chore: release 5.0.0 #1241

Merged

kjin self-requested a review April 13, 2020 17:47

kjin reviewed Apr 14, 2020

View reviewed changes

Comment thread src/plugins/plugin-http.ts

Comment thread src/plugins/plugin-http.ts Outdated

AaronFriel and others added 2 commits April 14, 2020 10:33

Update src/plugins/plugin-http.ts

9ccefb7

Co-Authored-By: Kelvin Jin <[email protected]>

AaronFriel force-pushed the fix-got-10 branch from 402bd6e to f2d9f50 Compare April 14, 2020 17:33

AaronFriel force-pushed the fix-got-10 branch from f2d9f50 to 0671c4b Compare April 14, 2020 17:53

AaronFriel added 2 commits April 14, 2020 17:47

chore: failing tests for Node 10.x style http.request calls

149f83d

fix: let http.request throw error on undefined/falsey URL

4e62c63

AaronFriel force-pushed the fix-got-10 branch from 0671c4b to 4e62c63 Compare April 15, 2020 00:47

kjin approved these changes Apr 15, 2020

View reviewed changes

JustinBeckwith added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. labels Apr 15, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 15, 2020

kjin merged commit 511b21c into googleapis:master Apr 16, 2020

release-please Bot mentioned this pull request Jun 30, 2022

chore(main): release 7.0.0 #1452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle Node 10 style http requests#1233

fix: handle Node 10 style http requests#1233
kjin merged 4 commits intogoogleapis:masterfrom
two-chairs:fix-got-10

AaronFriel commented Apr 10, 2020

Uh oh!

AaronFriel commented Apr 10, 2020

Uh oh!

AaronFriel commented Apr 10, 2020

Uh oh!

bcoe commented Apr 13, 2020

Uh oh!

Uh oh!

Uh oh!

AaronFriel commented Apr 14, 2020

Uh oh!

bcoe commented Apr 14, 2020

Uh oh!

AaronFriel commented Apr 15, 2020

Uh oh!

kjin left a comment

Uh oh!

gcf-merge-on-green Bot commented Apr 15, 2020

Uh oh!

gcf-merge-on-green Bot commented Apr 15, 2020

Uh oh!

gcf-merge-on-green Bot commented Apr 16, 2020

Uh oh!

bcoe commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	function makeRequestTrace(
	protocol: string,
	request: RequestFunction,
	api: Tracer
	): RequestFunction {
	// On Node 8+ we use the following function to patch both request and get.
	// Here `request` may also happen to be `get`.
	return function requestTrace(
	this: never,
	url: httpModule.RequestOptions \| string \| URL,
	options?:
	\| httpModule.RequestOptions
	\| ((res: httpModule.IncomingMessage) => void),
	callback?: (res: httpModule.IncomingMessage) => void
	): ClientRequest {
	// These are error conditions; defer to http.request and don't trace.
	if (!url \|\| (typeof url === 'object' && typeof options === 'object')) {
	return request.apply(this, arguments);
	}


	let urlString;
	if (typeof url === 'string') {
	// save the value of uri so we don't have to reconstruct it later
	urlString = url;
	url = urlParse(url);
	}
	if (typeof options === 'function') {
	callback = options;
	options = url;
	} else {
	options = Object.assign({}, url, options);
	}

Conversation

AaronFriel commented Apr 10, 2020

Environment details

Steps to reproduce

Bug

Fix

Uh oh!

AaronFriel commented Apr 10, 2020

Uh oh!

AaronFriel commented Apr 10, 2020

Uh oh!

bcoe commented Apr 13, 2020

Uh oh!

Uh oh!

Uh oh!

AaronFriel commented Apr 14, 2020

Uh oh!

bcoe commented Apr 14, 2020

Uh oh!

AaronFriel commented Apr 15, 2020

Uh oh!

kjin left a comment

Choose a reason for hiding this comment

Uh oh!

gcf-merge-on-green Bot commented Apr 15, 2020

Uh oh!

gcf-merge-on-green Bot commented Apr 15, 2020

Uh oh!

gcf-merge-on-green Bot commented Apr 16, 2020

Uh oh!

bcoe commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants