Added EnvHttpProxyAgent to support HTTP_PROXY #2994

10xLaCroixDrinker · 2024-03-25T16:01:02Z

This relates to...

Changes

Added EnvHttpProxyAgent and made it the default global dispatcher. The addition to index-fetch.js was required to keep those tests passing after changing the global dispatcher to a ProxyAgent

Features

Automatically detect/support HTTP_PROXY, HTTPS_PROXY, & NO_PROXY

Bug Fixes

N/A

Breaking Changes and Deprecations

Using these env vars by default could result in unexpected behavior

Status

lib/dispatcher/env-http-proxy-agent.js

metcoder95

Why not just detect the env var and set a ProxyAgent as globalDispatcher? ref:

undici/lib/global.js

Line 10 in ef25209

setGlobalDispatcher(new Agent())

I'm not really in sync with the addition of the new Agent as does not seems needed to enable this feature, we can keep it simple and just use the ProxyAgent if the feature is enabled or disabled otherwise.

Given the nature of the feature, I believe this might qualify as major as can cause unexpected side-effects if there's usage of HTTP_PROXY or similar already; unless we provide an option to ignore to force ignore the env var.

10xLaCroixDrinker · 2024-03-26T09:11:53Z

The three separate agents are needed to fully support the HTTP_PROXY, HTTPS_PROXY, NO_PROXY use case.

When only HTTP_PROXY is set, all requests go through it. When HTTPS_PROXY is set, https requests go through that one. And when NO_PROXY is set, requests that match are not proxied.

Creating a new agent is what appeared agreed upon in #1650

#1650 (comment)

KhafraDev · 2024-03-26T16:49:19Z

I think it shouldn't be set as the default agent, making it a semver-minor change.

index-fetch.js

lib/global.js

metcoder95 · 2024-03-27T09:04:20Z

test/env-http-proxy-agent.js

+  agentSymbols.forEach((agent) => {
+    sinon.spy(dispatcher[agent], 'dispatch')
+  })


Don't believe there's a need for mocks, we can test it pretty similar to how we test ProxyAgent, but depending on the environment vars

test/node-fetch/main.js

mcollina · 2024-03-28T21:00:59Z

I would prefer just adding this first, then we can turn it on by default. It seems the safer option.

mcollina

Good job! Could you also add some docs?

lib/dispatcher/env-http-proxy-agent.js

codecov-commenter · 2024-03-28T21:06:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.17%. Comparing base (2d5cbdf) to head (c322c33).
Report is 78 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2994      +/-   ##
==========================================
+ Coverage   93.51%   94.17%   +0.65%     
==========================================
  Files          89       90       +1     
  Lines       24227    24321      +94     
==========================================
+ Hits        22657    22905     +248     
+ Misses       1570     1416     -154

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Closes #1650

…ions

10xLaCroixDrinker · 2024-04-02T10:05:49Z

I've just got docs and refactoring tests away from mocks remaining. I believe all the other feedback is in now.

metcoder95

Let's not forget about the types and documentation

mcollina · 2024-04-17T13:49:49Z

@10xLaCroixDrinker I think it would be great to ship this. Any ETA on docs and types?

10xLaCroixDrinker · 2024-04-18T23:50:57Z

I can push those up today or tomorrow, but I haven't had time to work on rewriting the tests.

KhafraDev

I don't necessarily like the name since it also handles https proxies, but I don't have any alternative suggestions. Good work on this, I never got around to implementing it. 😄

lib/dispatcher/env-http-proxy-agent.js

metcoder95

LGTM, nice 👍

docs/docs/api/EnvHttpProxyAgent.md

mcollina

Lgtm

mcollina · 2024-04-19T12:08:59Z

The tests are failing on Windows

mcollina · 2024-04-19T14:26:12Z

@KhafraDev are you blocking this somehow? Can it land?

silverwind · 2024-04-19T21:01:58Z

docs/docs/api/EnvHttpProxyAgent.md

+
+`NO_PROXY` is a comma or space-separated list of hostnames that should not be proxied. The list may contain leading wildcard characters (`*`). If `NO_PROXY` is set, the EnvHttpProxyAgent will bypass the proxy for requests to hosts that match the list. If `NO_PROXY` is set to `"*"`, the EnvHttpProxyAgent will bypass the proxy for all requests.
+
+Lower case environment variables are also supported: `http_proxy`, `https_proxy`, and `no_proxy`. However, if both the lower case and upper case environment variables are set, the lower case environment variables will be ignored.


I'm late to the party, but I strongly recommend to reconsider the case preference as per the proxy var "spec", lowercase form should be preferred:

Use lowercase form. HTTP_PROXY is not always supported or recommended.
If you absolutely must use the uppercase form as well, be sure they share the same value.

cc @10xLaCroixDrinker

I can make the change some time in the next couple days.

Done: #3152

10xLaCroixDrinker changed the title ~~feat: added EnvHttpProxyAgent~~ Added EnvHttpProxyAgent to support HTTP_PROXY by default Mar 25, 2024

Uzlopak reviewed Mar 25, 2024

View reviewed changes

lib/dispatcher/env-http-proxy-agent.js Outdated Show resolved Hide resolved

metcoder95 reviewed Mar 26, 2024

View reviewed changes

metcoder95 reviewed Mar 27, 2024

View reviewed changes

mcollina requested changes Mar 28, 2024

View reviewed changes

10xLaCroixDrinker changed the title ~~Added EnvHttpProxyAgent to support HTTP_PROXY by default~~ Added EnvHttpProxyAgent to support HTTP_PROXY Apr 2, 2024

10xLaCroixDrinker added 6 commits April 1, 2024 22:33

feat: added EnvHttpProxyAgent

5f346b5

Closes #1650

refactor(env-http-proxy-agent): parse NO_PROXY in constructor

3b7cd3b

don't use EnvHttpProxyAgent by default

f31d3f9

refactor: use for loop when checking NO_PROXY entries

e497cb4

feat(env-http-proxy-agent): added httpProxy, httpsProxy & noProxy opt…

7384e45

…ions

feat(env-http-proxy-agent): handle changes to NO_PROXY

6937a51

metcoder95 reviewed Apr 3, 2024

View reviewed changes

docs: added types for EnvHttpProxyAgent

8dddd82

KhafraDev requested changes Apr 19, 2024

View reviewed changes

lib/dispatcher/env-http-proxy-agent.js Show resolved Hide resolved

metcoder95 approved these changes Apr 19, 2024

View reviewed changes

docs/docs/api/EnvHttpProxyAgent.md Show resolved Hide resolved

mcollina approved these changes Apr 19, 2024

View reviewed changes

10xLaCroixDrinker added 3 commits April 19, 2024 06:31

test: resolve windows issues, mark experimental, update doc

f85f825

docs: fix typo

f42599a

docs: fetch for EnvHttpProxyAgent

c322c33

KhafraDev approved these changes Apr 19, 2024

View reviewed changes

Uzlopak approved these changes Apr 19, 2024

View reviewed changes

mcollina merged commit ead42cd into nodejs:main Apr 19, 2024
26 checks passed

10xLaCroixDrinker deleted the feature/env-http-proxy-agent branch April 19, 2024 17:11

danielbankhead mentioned this pull request Apr 19, 2024

feat!: fetch (POC) googleapis/gaxios#618

Draft

silverwind reviewed Apr 19, 2024

View reviewed changes

github-actions bot mentioned this pull request Apr 22, 2024

[Release] v6.14.0 #3147

Merged

10xLaCroixDrinker mentioned this pull request Apr 22, 2024

fix(EnvHttpProxyAgent): prefer lowercase env vars #3152

Merged

7 tasks

benjdlambert mentioned this pull request May 29, 2024

🐛 Bug Report: Techdocs returning 401 after enabling permissions flag backstage/backstage#24791

Closed

2 tasks

paulfitz mentioned this pull request May 30, 2024

OIDC issuer behind a proxy cannot be accessed gristlabs/grist-core#942

Open

wojtekmaj mentioned this pull request Jul 11, 2024

Remove undici dependency when minimum supported Node.js version is 22.3.0 nodejs/corepack#516

Open

pgcalixto mentioned this pull request Jul 31, 2024

[BUG] Connection fail when using behing a corporate proxy (request to https://rhda.rhcloud.com/api/v4/analysis failed, reason: read ECONNRESET) fabric8-analytics/fabric8-analytics-vscode-extension#712

Open

nilreml mentioned this pull request Aug 16, 2024

ERROR initialize:initialize: failed to add schemas from catalog tamasfe/taplo#585

Open

parkerbxyz mentioned this pull request Sep 4, 2024

fix: remove custom proxy handling actions/create-github-app-token#143

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added EnvHttpProxyAgent to support HTTP_PROXY #2994

Added EnvHttpProxyAgent to support HTTP_PROXY #2994

10xLaCroixDrinker commented Mar 25, 2024 •

edited

Loading

metcoder95 left a comment

10xLaCroixDrinker commented Mar 26, 2024

KhafraDev commented Mar 26, 2024

metcoder95 Mar 27, 2024

mcollina commented Mar 28, 2024

mcollina left a comment

codecov-commenter commented Mar 28, 2024 •

edited

Loading

10xLaCroixDrinker commented Apr 2, 2024

metcoder95 left a comment

mcollina commented Apr 17, 2024 •

edited

Loading

10xLaCroixDrinker commented Apr 18, 2024

KhafraDev left a comment

metcoder95 left a comment

mcollina left a comment

mcollina commented Apr 19, 2024

mcollina commented Apr 19, 2024

silverwind Apr 19, 2024 •

edited

Loading

metcoder95 Apr 21, 2024

10xLaCroixDrinker Apr 21, 2024

10xLaCroixDrinker Apr 22, 2024


		`NO_PROXY` is a comma or space-separated list of hostnames that should not be proxied. The list may contain leading wildcard characters (``). If `NO_PROXY` is set, the EnvHttpProxyAgent will bypass the proxy for requests to hosts that match the list. If `NO_PROXY` is set to `""`, the EnvHttpProxyAgent will bypass the proxy for all requests.

		Lower case environment variables are also supported: `http_proxy`, `https_proxy`, and `no_proxy`. However, if both the lower case and upper case environment variables are set, the lower case environment variables will be ignored.

Added EnvHttpProxyAgent to support HTTP_PROXY #2994

Added EnvHttpProxyAgent to support HTTP_PROXY #2994

Conversation

10xLaCroixDrinker commented Mar 25, 2024 • edited Loading

This relates to...

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

metcoder95 left a comment

Choose a reason for hiding this comment

10xLaCroixDrinker commented Mar 26, 2024

KhafraDev commented Mar 26, 2024

metcoder95 Mar 27, 2024

Choose a reason for hiding this comment

mcollina commented Mar 28, 2024

mcollina left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 28, 2024 • edited Loading

Codecov Report

10xLaCroixDrinker commented Apr 2, 2024

metcoder95 left a comment

Choose a reason for hiding this comment

mcollina commented Apr 17, 2024 • edited Loading

10xLaCroixDrinker commented Apr 18, 2024

KhafraDev left a comment

Choose a reason for hiding this comment

metcoder95 left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Apr 19, 2024

mcollina commented Apr 19, 2024

silverwind Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

metcoder95 Apr 21, 2024

Choose a reason for hiding this comment

10xLaCroixDrinker Apr 21, 2024

Choose a reason for hiding this comment

10xLaCroixDrinker Apr 22, 2024

Choose a reason for hiding this comment

10xLaCroixDrinker commented Mar 25, 2024 •

edited

Loading

codecov-commenter commented Mar 28, 2024 •

edited

Loading

mcollina commented Apr 17, 2024 •

edited

Loading

silverwind Apr 19, 2024 •

edited

Loading