Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2

### :bug: Bug Fixes

* fix(otlp-exporter-base): honor Node.js environment proxy settings in HTTP agent factory [#6660](https://github.com/open-telemetry/opentelemetry-js/pull/6660) @cyphercodes
* fix(configuration): do not validate `OTEL_CONFIG_FILE` value before using it for file config [#6643](https://github.com/open-telemetry/opentelemetry-js/pull/6643) @trentm
* fix(configuration): improve how 'additionalProperties' in JSON schema is translated to TS types [#6650](https://github.com/open-telemetry/opentelemetry-js/pull/6650) @trentm
* fix(sampler-jaeger-remote): add missing axios dep [#6656](https://github.com/open-telemetry/opentelemetry-js/pull/6656) @trentm
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,54 @@ export interface OtlpNodeHttpConfiguration extends OtlpHttpConfiguration {
userAgent?: string;
}

function hasEnvProxyEnvVar(name: string): boolean {
return process.env[name] != null && process.env[name] !== '';
}

function hasEnvProxyConfiguration(): boolean {
return (
process.env.NODE_USE_ENV_PROXY === '1' ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
process.env.NODE_USE_ENV_PROXY === '1' ||

This env var is for a user to tell node to read proxy-related envvars, if there are any. This isn't an indication that there is proxy-related env.

hasEnvProxyEnvVar('HTTP_PROXY') ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node's process.env values are always strings (https://nodejs.org/api/process.html#processenv), e.g.:

> process.env.FOO = 42
42
> process.env.FOO
'42'

so these hasEnvProxyEnvVar usages can be:

Suggested change
hasEnvProxyEnvVar('HTTP_PROXY') ||
process.env.HTTP_PROXY ||

as Node.js core is doing: https://github.com/nodejs/node/blob/9fe7634c12a6ee7491729dc1e0a8c2020f87f8c4/lib/internal/process/pre_execution.js#L201-L204

hasEnvProxyEnvVar('http_proxy') ||
hasEnvProxyEnvVar('HTTPS_PROXY') ||
hasEnvProxyEnvVar('https_proxy')
);
}

function getEnvProxyAgentOptions():
| { proxyEnv: NodeJS.ProcessEnv }
| undefined {
if (hasEnvProxyConfiguration()) {
// `proxyEnv` is used by Node.js' HTTP agents to honor HTTP(S)_PROXY
// while preserving exporter-specific agent options such as keepAlive.
return { proxyEnv: process.env };
Comment on lines +57 to +59
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we could pass in the full process.env, I don't think we should. The Node.js docs for proxyEnv suggest only the well-known 5 values are used: https://nodejs.org/api/http.html#new-agentoptions

Perhaps:

Suggested change
// `proxyEnv` is used by Node.js' HTTP agents to honor HTTP(S)_PROXY
// while preserving exporter-specific agent options such as keepAlive.
return { proxyEnv: process.env };
return {
proxyEnv: {
HTTP_PROXY: process.env.HTTP_PROXY,
HTTPS_PROXY: process.env.HTTPS_PROXY,
NO_PROXY: process.env.NO_PROXY,
http_proxy: process.env.http_proxy,
https_proxy: process.env.https_proxy,
no_proxy: process.env.no_proxy,
}
};

I suppose a fair argument is that passing in process.env works, and avoid creating another object.

}

return undefined;
}

export function httpAgentFactoryFromOptions(
options: http.AgentOptions | https.AgentOptions
): HttpAgentFactory {
return async protocol => {
const isInsecure = protocol === 'http:';
const module = isInsecure ? import('http') : import('https');
const { Agent } = await module;
const envProxyAgentOptions = getEnvProxyAgentOptions();

if (isInsecure) {
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- these props should not be used in agent options
const { ca, cert, key, ...insecureOptions } =
options as https.AgentOptions;
return new Agent(insecureOptions);
return new Agent({
...insecureOptions,
...envProxyAgentOptions,
} as http.AgentOptions);
}
return new Agent(options);
return new Agent({
...options,
...envProxyAgentOptions,
Comment on lines +80 to +85
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should envProxyAgentOptions be applied before the passed in options? That would allow the caller of httpAgentFactoryFromOptions to specify its own custom proxyEnv.

} as https.AgentOptions);
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,93 @@
* SPDX-License-Identifier: Apache-2.0
*/
import * as assert from 'assert';
import { mergeOtlpNodeHttpConfigurationWithDefaults } from '../../../src/configuration/otlp-node-http-configuration';
import * as http from 'http';
import * as https from 'https';
import {
httpAgentFactoryFromOptions,
mergeOtlpNodeHttpConfigurationWithDefaults,
} from '../../../src/configuration/otlp-node-http-configuration';
import type { OtlpNodeHttpConfiguration } from '../../../src/configuration/otlp-node-http-configuration';
import { VERSION } from '../../../src/version';

const ENV_PROXY_VARIABLES = [
'NODE_USE_ENV_PROXY',
'HTTP_PROXY',
'http_proxy',
'HTTPS_PROXY',
'https_proxy',
];

function agentOptions(agent: http.Agent | https.Agent): any {
return (agent as any).options;
}

describe('httpAgentFactoryFromOptions', function () {
let originalEnv: Record<string, string | undefined>;

beforeEach(function () {
originalEnv = {};
for (const envVar of ENV_PROXY_VARIABLES) {
originalEnv[envVar] = process.env[envVar];
delete process.env[envVar];
}
});

afterEach(function () {
for (const envVar of ENV_PROXY_VARIABLES) {
if (originalEnv[envVar] == null) {
delete process.env[envVar];
} else {
process.env[envVar] = originalEnv[envVar];
}
}
});

it('creates protocol-specific agents with the provided options', async function () {
const factory = httpAgentFactoryFromOptions({ keepAlive: true });

const httpAgent = (await factory('http:')) as http.Agent;
const httpsAgent = (await factory('https:')) as https.Agent;

assert.ok(httpAgent instanceof http.Agent);
assert.ok(httpsAgent instanceof https.Agent);
assert.strictEqual(agentOptions(httpAgent).keepAlive, true);
assert.strictEqual(agentOptions(httpsAgent).keepAlive, true);
assert.strictEqual(agentOptions(httpAgent).proxyEnv, undefined);
assert.strictEqual(agentOptions(httpsAgent).proxyEnv, undefined);
});

it('passes proxyEnv to http agents when HTTP_PROXY is configured', async function () {
process.env.HTTP_PROXY = 'http://proxy.example:3128';
const factory = httpAgentFactoryFromOptions({ keepAlive: true });

const agent = (await factory('http:')) as http.Agent;

assert.strictEqual(agentOptions(agent).keepAlive, true);
assert.strictEqual(agentOptions(agent).proxyEnv, process.env);
});

it('passes proxyEnv to https agents when HTTPS_PROXY is configured', async function () {
process.env.HTTPS_PROXY = 'http://proxy.example:3128';
const factory = httpAgentFactoryFromOptions({ keepAlive: true });

const agent = (await factory('https:')) as https.Agent;

assert.strictEqual(agentOptions(agent).keepAlive, true);
assert.strictEqual(agentOptions(agent).proxyEnv, process.env);
});

it('passes proxyEnv to agents when NODE_USE_ENV_PROXY is enabled', async function () {
process.env.NODE_USE_ENV_PROXY = '1';
const factory = httpAgentFactoryFromOptions({ keepAlive: true });

const agent = (await factory('https:')) as https.Agent;

assert.strictEqual(agentOptions(agent).keepAlive, true);
assert.strictEqual(agentOptions(agent).proxyEnv, process.env);
});
Comment on lines +82 to +90
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this test, assuming agreement with dropping using NODE_USE_ENV_PROXY above.

});

describe('mergeOtlpNodeHttpConfigurationWithDefaults', function () {
const testDefaults: OtlpNodeHttpConfiguration = {
url: 'http://default.example.test',
Expand Down
Loading