From 3ac0b90442804cff6fcf21fa2a6731cc38a66030 Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Mon, 5 Aug 2019 14:24:10 -0700 Subject: [PATCH] fix: force http and https clients to be patched (#1084) --- package.json | 1 + src/tracing.ts | 5 ++ test/fixtures/plugin-fixtures.json | 5 ++ test/plugins/test-trace-http.ts | 37 +------- test/plugins/test-trace-node-fetch.ts | 116 ++++++++++++++++++++++++++ test/web-frameworks/express-secure.ts | 56 +++++++++++++ tsconfig.json | 1 + 7 files changed, 185 insertions(+), 36 deletions(-) create mode 100644 test/plugins/test-trace-node-fetch.ts create mode 100644 test/web-frameworks/express-secure.ts diff --git a/package.json b/package.json index 56a57e2a5..758cbbae0 100644 --- a/package.json +++ b/package.json @@ -66,6 +66,7 @@ "@types/mongoose": "^5.3.26", "@types/nock": "^10.0.0", "@types/node": "~10.7.2", + "@types/node-fetch": "^2.5.0", "@types/once": "^1.4.0", "@types/proxyquire": "^1.3.28", "@types/request": "^2.48.1", diff --git a/src/tracing.ts b/src/tracing.ts index 05d176203..dd8abbfbe 100644 --- a/src/tracing.ts +++ b/src/tracing.ts @@ -154,6 +154,11 @@ export class Tracing implements Component { .create(this.config.pluginLoaderConfig, tracerComponents) .activate(); + // Require http and https again, now that the plugin loader is activated. + // This forces them to be patched. + require('http'); + require('https'); + if ( typeof this.config.writerConfig.authOptions.projectId !== 'string' && typeof this.config.writerConfig.authOptions.projectId !== 'undefined' diff --git a/test/fixtures/plugin-fixtures.json b/test/fixtures/plugin-fixtures.json index 1a23ccf05..98597d1c9 100644 --- a/test/fixtures/plugin-fixtures.json +++ b/test/fixtures/plugin-fixtures.json @@ -151,6 +151,11 @@ "mysql2": "^1.5.1" } }, + "node-fetch2": { + "dependencies": { + "node-fetch": "^2.6.0" + } + }, "pg6": { "dependencies": { "pg": "^6.1.2" diff --git a/test/plugins/test-trace-http.ts b/test/plugins/test-trace-http.ts index 285f10675..a549704fb 100644 --- a/test/plugins/test-trace-http.ts +++ b/test/plugins/test-trace-http.ts @@ -16,11 +16,8 @@ import * as assert from 'assert'; import {EventEmitter} from 'events'; -import * as fs from 'fs'; import * as httpModule from 'http'; import * as httpsModule from 'https'; -import {AddressInfo} from 'net'; -import * as path from 'path'; import * as stream from 'stream'; import {URL} from 'url'; @@ -30,6 +27,7 @@ import {parseContextFromHeader, TraceContext} from '../../src/util'; import * as testTraceModule from '../trace'; import {assertSpanDuration, DEFAULT_SPAN_DURATION} from '../utils'; import {Express4} from '../web-frameworks/express'; +import {Express4Secure} from '../web-frameworks/express-secure'; // This type describes (http|https).(get|request). type HttpRequest = ( @@ -80,39 +78,6 @@ class WaitForResponse { const wait = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); -/** - * A modification of the Express4 test server that uses HTTPS instead. - */ -class Express4Secure extends Express4 { - static key = fs.readFileSync( - path.join(__dirname, '..', 'fixtures', 'key.pem') - ); - static cert = fs.readFileSync( - path.join(__dirname, '..', 'fixtures', 'cert.pem') - ); - private https: typeof httpsModule; - - constructor() { - super(); - this.https = require('https'); - } - - listen(port: number): number { - // The types of (http|https).Server are not compatible, but we don't - // access any properties that aren't present on both in the test. - this.server = (this.https.createServer( - {key: Express4Secure.key, cert: Express4Secure.cert}, - this.app - ) as {}) as httpModule.Server; - this.server.listen(port); - return (this.server.address() as AddressInfo).port; - } - - shutdown() { - this.server!.close(); - } -} - // Server abstraction class definitions. These are borrowed from web framework // tests -- which are useful because they already expose a Promise API. const servers = { diff --git a/test/plugins/test-trace-node-fetch.ts b/test/plugins/test-trace-node-fetch.ts new file mode 100644 index 000000000..36195e230 --- /dev/null +++ b/test/plugins/test-trace-node-fetch.ts @@ -0,0 +1,116 @@ +/** + * Copyright 2019 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as fetchTypes from 'node-fetch'; // For types only. +import * as testTraceModule from '../trace'; +import * as assert from 'assert'; +import {describeInterop} from '../utils'; +import {Express4} from '../web-frameworks/express'; +import {Express4Secure} from '../web-frameworks/express-secure'; +import {Agent} from 'https'; +import {SpanKind} from '../../src/trace'; + +// Server abstraction class definitions. These are borrowed from web framework +// tests -- which are useful because they already expose a Promise API. +const servers = { + http: Express4, + https: Express4Secure, +}; + +/** + * This test is needed because @google-cloud/common uses node-fetch under the + * covers, so there is a possibility that we miss the opportunity to patch + * http/https core modules. This occurs when the user requires `node-fetch`, + * and never transitively requires (one of) `http` or `https` outside of + * `node-fetch`, because then the plugin loader will never get the chance to + * hook into a `http` or `https` module require. + */ +describeInterop( + 'node-fetch', + fixture => { + before(() => { + testTraceModule.setPluginLoaderForTest(); + testTraceModule.setCLSForTest(); + }); + + after(() => { + testTraceModule.setPluginLoaderForTest(testTraceModule.TestPluginLoader); + testTraceModule.setCLSForTest(testTraceModule.TestCLS); + }); + + beforeEach(() => { + testTraceModule.clearTraceData(); + }); + + for (const protocol of Object.keys(servers) as Array< + keyof typeof servers + >) { + it(`works with the Trace Agent, ${protocol}`, async () => { + // Set up a server. To preserve the condition described in the top-level + // description of this test, we ensure that this constructor is called + // before the Trace Agent is started, so that the Trace Agent never has + // an opportunity to patch http or https upon user require. + const server = new servers[protocol](); + // Require node-fetch once before starting the Trace Agent. We do this + // in lieu of letting it be required when the Trace Agent is started, + // because we've mocked out the Trace Writer instance that would + // require node-fetch in typical usage. + fixture.require(); + const tracer = testTraceModule.start(); + const fetch = fixture.require(); + + // Set up the server. + server.addHandler({ + path: '/', + hasResponse: true, + fn: async () => ({statusCode: 200, message: 'OK'}), + }); + const port = server.listen(0); + + // Allow self-signed certificates. + let agent: Agent | undefined; + if (protocol === 'https') { + agent = new Agent({ + rejectUnauthorized: false, + }); + } + + try { + // Make a request against the above server. + await tracer.runInRootSpan({name: 'outer'}, async span => { + assert.ok(tracer.isRealSpan(span)); + const response = await fetch(`${protocol}://localhost:${port}`, { + agent, + }); + assert.strictEqual(await response.text(), 'OK'); + span.endSpan(); + }); + + // Get the trace that represents the root span from above.. + const traces = testTraceModule.getOneTrace(trace => + trace.spans.some(span => span.name === 'outer') + ); + // There should be an HTTP client span. + assert.ok( + traces.spans.some(span => span.kind === SpanKind.RPC_CLIENT) + ); + } finally { + server.shutdown(); + } + }); + } + } +); diff --git a/test/web-frameworks/express-secure.ts b/test/web-frameworks/express-secure.ts new file mode 100644 index 000000000..1fd77af19 --- /dev/null +++ b/test/web-frameworks/express-secure.ts @@ -0,0 +1,56 @@ +/** + * Copyright 2019 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {AddressInfo} from 'net'; + +import * as fs from 'fs'; +import * as path from 'path'; +import {Express4} from './express'; +import * as httpModule from 'http'; +import * as httpsModule from 'https'; + +/** + * A modification of the Express4 test server that uses HTTPS instead. + */ +export class Express4Secure extends Express4 { + static key = fs.readFileSync( + path.join(__dirname, '..', 'fixtures', 'key.pem') + ); + static cert = fs.readFileSync( + path.join(__dirname, '..', 'fixtures', 'cert.pem') + ); + private https: typeof httpsModule; + + constructor() { + super(); + this.https = require('https'); + } + + listen(port: number): number { + // The types of (http|https).Server are not compatible, but we don't + // access any properties that aren't present on both in the test. + this.server = (this.https.createServer( + {key: Express4Secure.key, cert: Express4Secure.cert}, + this.app + ) as {}) as httpModule.Server; + this.server.listen(port); + return (this.server.address() as AddressInfo).port; + } + + shutdown() { + this.server!.close(); + } +} diff --git a/tsconfig.json b/tsconfig.json index 8088cd5ad..e0dc646ec 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -26,6 +26,7 @@ "test/plugins/test-trace-http2.ts", "test/plugins/test-trace-knex.ts", "test/plugins/test-trace-mongoose-async-await.ts", + "test/plugins/test-trace-node-fetch.ts", "test/logger.ts", "test/nocks.ts", "test/test-cls.ts",