fix(otlp-exporter-base): honor env proxy settings#6660
Conversation
Signed-off-by: cyphercodes <cyphercodes@users.noreply.github.com>
cyphercodes
force-pushed
the
fix-otlp-http-env-proxy-6638
branch
from
9289f34 to
8a164e5
Compare
raphael-theriault-swi
left a comment
raphael-theriault-swi
left a comment
There was a problem hiding this comment.
LGTM but I'll let @pichlermarc take a look
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6660 +/- ##
=======================================
Coverage 95.51% 95.52%
=======================================
Files 372 372
Lines 12361 12375 +14
Branches 2836 2843 +7
=======================================
+ Hits 11807 11821 +14
Misses 554 554
🚀 New features to boost your workflow:
|
| function hasEnvProxyConfiguration(): boolean { | ||
| return ( | ||
| process.env.NODE_USE_ENV_PROXY === '1' || | ||
| hasEnvProxyEnvVar('HTTP_PROXY') || |
There was a problem hiding this comment.
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:
| 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
|
|
||
| function hasEnvProxyConfiguration(): boolean { | ||
| return ( | ||
| process.env.NODE_USE_ENV_PROXY === '1' || |
There was a problem hiding this comment.
| 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.
| // `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 }; |
There was a problem hiding this comment.
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:
| // `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.
| 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); | ||
| }); |
There was a problem hiding this comment.
Drop this test, assuming agreement with dropping using NODE_USE_ENV_PROXY above.
| ...envProxyAgentOptions, | ||
| } as http.AgentOptions); | ||
| } | ||
| return new Agent(options); | ||
| return new Agent({ | ||
| ...options, | ||
| ...envProxyAgentOptions, |
There was a problem hiding this comment.
Should envProxyAgentOptions be applied before the passed in options? That would allow the caller of httpAgentFactoryFromOptions to specify its own custom proxyEnv.
4 participants
Which problem is this PR solving?
Fixes #6638.
Short description of the changes
Pass Node's
proxyEnvagent option when env proxy settings are present so OTLP HTTP exporters honorHTTP_PROXY/HTTPS_PROXY/NO_PROXYwithout dropping existing agent options such askeepAlive.Type of change
How Has This Been Tested?
npm run compileinexperimental/packages/otlp-exporter-basenpm testinexperimental/packages/otlp-exporter-basenpm run lintinexperimental/packages/otlp-exporter-baseHTTPS_PROXYChecklist: