Skip to content
This repository was archived by the owner on Jan 21, 2026. It is now read-only.

Commit 3ac0b90

Browse files
kjinJustinBeckwith
authored andcommitted
fix: force http and https clients to be patched (#1084)
1 parent c24faa3 commit 3ac0b90

7 files changed

Lines changed: 185 additions & 36 deletions

File tree

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@
6666
"@types/mongoose": "^5.3.26",
6767
"@types/nock": "^10.0.0",
6868
"@types/node": "~10.7.2",
69+
"@types/node-fetch": "^2.5.0",
6970
"@types/once": "^1.4.0",
7071
"@types/proxyquire": "^1.3.28",
7172
"@types/request": "^2.48.1",

src/tracing.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,11 @@ export class Tracing implements Component {
154154
.create(this.config.pluginLoaderConfig, tracerComponents)
155155
.activate();
156156

157+
// Require http and https again, now that the plugin loader is activated.
158+
// This forces them to be patched.
159+
require('http');
160+
require('https');
161+
157162
if (
158163
typeof this.config.writerConfig.authOptions.projectId !== 'string' &&
159164
typeof this.config.writerConfig.authOptions.projectId !== 'undefined'

test/fixtures/plugin-fixtures.json

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,11 @@
151151
"mysql2": "^1.5.1"
152152
}
153153
},
154+
"node-fetch2": {
155+
"dependencies": {
156+
"node-fetch": "^2.6.0"
157+
}
158+
},
154159
"pg6": {
155160
"dependencies": {
156161
"pg": "^6.1.2"

test/plugins/test-trace-http.ts

Lines changed: 1 addition & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,8 @@
1616

1717
import * as assert from 'assert';
1818
import {EventEmitter} from 'events';
19-
import * as fs from 'fs';
2019
import * as httpModule from 'http';
2120
import * as httpsModule from 'https';
22-
import {AddressInfo} from 'net';
23-
import * as path from 'path';
2421
import * as stream from 'stream';
2522
import {URL} from 'url';
2623

@@ -30,6 +27,7 @@ import {parseContextFromHeader, TraceContext} from '../../src/util';
3027
import * as testTraceModule from '../trace';
3128
import {assertSpanDuration, DEFAULT_SPAN_DURATION} from '../utils';
3229
import {Express4} from '../web-frameworks/express';
30+
import {Express4Secure} from '../web-frameworks/express-secure';
3331

3432
// This type describes (http|https).(get|request).
3533
type HttpRequest = (
@@ -80,39 +78,6 @@ class WaitForResponse {
8078

8179
const wait = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));
8280

83-
/**
84-
* A modification of the Express4 test server that uses HTTPS instead.
85-
*/
86-
class Express4Secure extends Express4 {
87-
static key = fs.readFileSync(
88-
path.join(__dirname, '..', 'fixtures', 'key.pem')
89-
);
90-
static cert = fs.readFileSync(
91-
path.join(__dirname, '..', 'fixtures', 'cert.pem')
92-
);
93-
private https: typeof httpsModule;
94-
95-
constructor() {
96-
super();
97-
this.https = require('https');
98-
}
99-
100-
listen(port: number): number {
101-
// The types of (http|https).Server are not compatible, but we don't
102-
// access any properties that aren't present on both in the test.
103-
this.server = (this.https.createServer(
104-
{key: Express4Secure.key, cert: Express4Secure.cert},
105-
this.app
106-
) as {}) as httpModule.Server;
107-
this.server.listen(port);
108-
return (this.server.address() as AddressInfo).port;
109-
}
110-
111-
shutdown() {
112-
this.server!.close();
113-
}
114-
}
115-
11681
// Server abstraction class definitions. These are borrowed from web framework
11782
// tests -- which are useful because they already expose a Promise API.
11883
const servers = {
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/**
2+
* Copyright 2019 Google Inc. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
import * as fetchTypes from 'node-fetch'; // For types only.
18+
import * as testTraceModule from '../trace';
19+
import * as assert from 'assert';
20+
import {describeInterop} from '../utils';
21+
import {Express4} from '../web-frameworks/express';
22+
import {Express4Secure} from '../web-frameworks/express-secure';
23+
import {Agent} from 'https';
24+
import {SpanKind} from '../../src/trace';
25+
26+
// Server abstraction class definitions. These are borrowed from web framework
27+
// tests -- which are useful because they already expose a Promise API.
28+
const servers = {
29+
http: Express4,
30+
https: Express4Secure,
31+
};
32+
33+
/**
34+
* This test is needed because @google-cloud/common uses node-fetch under the
35+
* covers, so there is a possibility that we miss the opportunity to patch
36+
* http/https core modules. This occurs when the user requires `node-fetch`,
37+
* and never transitively requires (one of) `http` or `https` outside of
38+
* `node-fetch`, because then the plugin loader will never get the chance to
39+
* hook into a `http` or `https` module require.
40+
*/
41+
describeInterop<typeof fetchTypes & typeof fetchTypes.default>(
42+
'node-fetch',
43+
fixture => {
44+
before(() => {
45+
testTraceModule.setPluginLoaderForTest();
46+
testTraceModule.setCLSForTest();
47+
});
48+
49+
after(() => {
50+
testTraceModule.setPluginLoaderForTest(testTraceModule.TestPluginLoader);
51+
testTraceModule.setCLSForTest(testTraceModule.TestCLS);
52+
});
53+
54+
beforeEach(() => {
55+
testTraceModule.clearTraceData();
56+
});
57+
58+
for (const protocol of Object.keys(servers) as Array<
59+
keyof typeof servers
60+
>) {
61+
it(`works with the Trace Agent, ${protocol}`, async () => {
62+
// Set up a server. To preserve the condition described in the top-level
63+
// description of this test, we ensure that this constructor is called
64+
// before the Trace Agent is started, so that the Trace Agent never has
65+
// an opportunity to patch http or https upon user require.
66+
const server = new servers[protocol]();
67+
// Require node-fetch once before starting the Trace Agent. We do this
68+
// in lieu of letting it be required when the Trace Agent is started,
69+
// because we've mocked out the Trace Writer instance that would
70+
// require node-fetch in typical usage.
71+
fixture.require();
72+
const tracer = testTraceModule.start();
73+
const fetch = fixture.require();
74+
75+
// Set up the server.
76+
server.addHandler({
77+
path: '/',
78+
hasResponse: true,
79+
fn: async () => ({statusCode: 200, message: 'OK'}),
80+
});
81+
const port = server.listen(0);
82+
83+
// Allow self-signed certificates.
84+
let agent: Agent | undefined;
85+
if (protocol === 'https') {
86+
agent = new Agent({
87+
rejectUnauthorized: false,
88+
});
89+
}
90+
91+
try {
92+
// Make a request against the above server.
93+
await tracer.runInRootSpan({name: 'outer'}, async span => {
94+
assert.ok(tracer.isRealSpan(span));
95+
const response = await fetch(`${protocol}://localhost:${port}`, {
96+
agent,
97+
});
98+
assert.strictEqual(await response.text(), 'OK');
99+
span.endSpan();
100+
});
101+
102+
// Get the trace that represents the root span from above..
103+
const traces = testTraceModule.getOneTrace(trace =>
104+
trace.spans.some(span => span.name === 'outer')
105+
);
106+
// There should be an HTTP client span.
107+
assert.ok(
108+
traces.spans.some(span => span.kind === SpanKind.RPC_CLIENT)
109+
);
110+
} finally {
111+
server.shutdown();
112+
}
113+
});
114+
}
115+
}
116+
);
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/**
2+
* Copyright 2019 Google LLC
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
import {AddressInfo} from 'net';
18+
19+
import * as fs from 'fs';
20+
import * as path from 'path';
21+
import {Express4} from './express';
22+
import * as httpModule from 'http';
23+
import * as httpsModule from 'https';
24+
25+
/**
26+
* A modification of the Express4 test server that uses HTTPS instead.
27+
*/
28+
export class Express4Secure extends Express4 {
29+
static key = fs.readFileSync(
30+
path.join(__dirname, '..', 'fixtures', 'key.pem')
31+
);
32+
static cert = fs.readFileSync(
33+
path.join(__dirname, '..', 'fixtures', 'cert.pem')
34+
);
35+
private https: typeof httpsModule;
36+
37+
constructor() {
38+
super();
39+
this.https = require('https');
40+
}
41+
42+
listen(port: number): number {
43+
// The types of (http|https).Server are not compatible, but we don't
44+
// access any properties that aren't present on both in the test.
45+
this.server = (this.https.createServer(
46+
{key: Express4Secure.key, cert: Express4Secure.cert},
47+
this.app
48+
) as {}) as httpModule.Server;
49+
this.server.listen(port);
50+
return (this.server.address() as AddressInfo).port;
51+
}
52+
53+
shutdown() {
54+
this.server!.close();
55+
}
56+
}

tsconfig.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
"test/plugins/test-trace-http2.ts",
2727
"test/plugins/test-trace-knex.ts",
2828
"test/plugins/test-trace-mongoose-async-await.ts",
29+
"test/plugins/test-trace-node-fetch.ts",
2930
"test/logger.ts",
3031
"test/nocks.ts",
3132
"test/test-cls.ts",

0 commit comments

Comments
 (0)