feat: add no_proxy env variable #361

sofisl · 2020-11-06T04:05:30Z

Fixes #69

Refs #69

sofisl · 2020-11-06T04:08:32Z

src/gaxios.ts

@@ -55,7 +74,6 @@ function loadProxy() {
  }
  return proxy;
 }


@bcoe, I want to ask about the default behavior. If a URL comes in that is not listed under no_proxy or proxy, should it be proxied? Currently that's the way it is with the code as it stands now, but I feel like the default should be the other way around. In other words, we should maybe do some kind of checking to see if the URL equals any of the HTTPS_PROXY env variables.

codecov · 2020-11-06T04:10:04Z

Codecov Report

Merging #361 (2e9d695) into master (132568f) will decrease coverage by 0.00%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
- Coverage   96.31%   96.31%   -0.01%     
==========================================
  Files           6        6              
  Lines         705      732      +27     
  Branches       99      106       +7     
==========================================
+ Hits          679      705      +26     
- Misses         26       27       +1

Impacted Files	Coverage Δ
src/gaxios.ts	`98.67% <96.66%> (-0.24%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 132568f...2e9d695. Read the comment docs.

JustinBeckwith · 2020-11-06T04:08:17Z

src/gaxios.ts

@@ -44,7 +44,26 @@ let HttpsProxyAgent: any;

 // Figure out if we should be using a proxy. Only if it's required, load
 // the https-proxy-agent module as it adds startup cost.
-function loadProxy() {
+function loadProxy(url: string) {
+  const noProxy = (process.env.no_proxy ?? process.env.NO_PROXY)?.split(',');


This line of code is hurting my brain. Using the ?? means if process.env.no_proxy is an empty string, we would accept that value. Is that our intent? I would imagine || is more appropriate here.

Ah, I didn't realize it would accept empty strings. Changed!

JustinBeckwith · 2020-11-06T04:16:04Z

src/gaxios.ts

-function loadProxy() {
+function loadProxy(url: string) {
+  const noProxy = (process.env.no_proxy ?? process.env.NO_PROXY)?.split(',');
+  if (noProxy) {


Should we also check that noProxy.length > 0?

JustinBeckwith · 2020-11-06T04:23:57Z

src/gaxios.ts

@@ -55,7 +74,6 @@ function loadProxy() {
  }
  return proxy;
 }
-loadProxy();


So thinking back through this - I think I was front-loading this call so that the require call would always happen on app start, instead of potentially happening during a request and synchronously blocking the thread. By removing this, we could introduce a slower HTTP call here, and kind of degrade performance at a random point in app execution, no?

(sorry @bcoe I know you asked earlier, but it didn't make sense until I re-looked at the codes)

@JustinBeckwith, given that the function now depends on checking against a URL, wouldn't we have to wait for it to be called during a request?

Just had a discussion with @sofisl about a potential refactor here, to get the best of both worlds.

JustinBeckwith · 2020-11-06T04:24:48Z

test/test.getch.ts

+      const res = await request({url});
+      scope.done();
+      assert.deepStrictEqual(res.data, body);
+      assert.ok(res.config.agent instanceof HttpsProxyAgent);


I simply love the number of tests you wrote to cover this

sofisl · 2020-11-10T20:29:41Z

@JustinBeckwith @bcoe, refactored the code to separate out loadProxy() and getProxy(). The default behavior will be to load proxy if there's a proxy env variable, but will only use it if there is a match for proxy variable.

bcoe · 2020-11-16T18:59:53Z

src/gaxios.ts

    if (proxy) {
+      loadProxy();


shouldn't need this line anymore

src/gaxios.ts

bcoe · 2020-11-30T20:28:08Z

src/gaxios.ts

+    return matchingProxyStrings(
+      process.env.HTTPS_PROXY,
+      process.env.https_proxy,
+      process.env.HTTP_PROXY,
+      process.env.http_proxy,
+      url
+    );


we should just return the proxy here, the logic is different for http_proxy, vs., no_proxy (one contains a list of URLs, the other is just a single URL representing the proxy).

bcoe

Noticed a slight logic hiccup.

src/gaxios.ts

Co-authored-by: Benjamin E. Coe <bencoe@google.com>

bcoe · 2020-12-02T19:38:18Z

test/test.getch.ts

+      assert.strictEqual(res.config.agent, undefined);
+    });
+
+    it('should proxy if url is an exact match of proxy, but not of no_proxy', async () => {


we should also update this test to reflect the fact that there's no correlation between the URL and the https_proxy variable.

It's no_proxy that matches a list of URLs.

bcoe · 2020-12-02T19:38:43Z

test/test.getch.ts

+    });
+
+    it('should allow comma-separated lists for proxy env variables', async () => {
+      const url = 'https://api.google.com';


I think this test can be dropped.

bcoe and others added 2 commits November 5, 2020 11:44

feat: add support for no_proxy environment variable

9b60efd

Refs #69

feat: add no_proxy env variable

5185e05

sofisl requested a review from a team as a code owner November 6, 2020 04:05

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2020

sofisl requested a review from bcoe November 6, 2020 04:05

sofisl commented Nov 6, 2020

View reviewed changes

JustinBeckwith reviewed Nov 6, 2020

View reviewed changes

sofisl added 2 commits November 9, 2020 10:54

chore: address review comments re string logic

3d0bdd5

fix: separate out get proxy and load proxy;

f2ab41a

sofisl requested a review from JustinBeckwith November 11, 2020 18:01

sofisl added 2 commits November 11, 2020 10:01

remove logging

3807cf2

fix: npm run lint

472236d

bcoe reviewed Nov 16, 2020

View reviewed changes

src/gaxios.ts Outdated

if (proxy) {

loadProxy();

Copy link

Contributor

bcoe Nov 16, 2020

Choose a reason for hiding this comment

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

shouldn't need this line anymore

bcoe reviewed Nov 16, 2020

View reviewed changes

src/gaxios.ts Outdated Show resolved Hide resolved

fix: call loadProxy in getProxy

e05f52c

sofisl requested a review from bcoe November 24, 2020 22:23

Merge branch 'master' into no-proxy

00c27de

bcoe approved these changes Nov 30, 2020

View reviewed changes

bcoe reviewed Nov 30, 2020

View reviewed changes

bcoe suggested changes Nov 30, 2020

View reviewed changes

sofisl added 3 commits November 30, 2020 13:46

fix: logic for proxy env var

58f78a0

fix test

d433894

Merge branch 'no-proxy' of github.com:googleapis/gaxios into no-proxy

ea5fc09

sofisl requested a review from bcoe November 30, 2020 21:48

fix: return loadProxy() value

a16855f

bcoe reviewed Dec 2, 2020

View reviewed changes

src/gaxios.ts Show resolved Hide resolved

Update src/gaxios.ts

5a8f7c6

Co-authored-by: Benjamin E. Coe <bencoe@google.com>

bcoe reviewed Dec 2, 2020

View reviewed changes

sofisl added 2 commits December 5, 2020 00:53

fix: refactor gaxios tests

a8e02e4

Merge branch 'no-proxy' of github.com:googleapis/gaxios into no-proxy

32b7d6a

sofisl requested a review from bcoe December 5, 2020 08:58

bcoe added 2 commits December 8, 2020 16:07

Merge branch 'master' into no-proxy

94a7f3a

refactor: simplify code a bit

2e9d695

bcoe approved these changes Dec 8, 2020

View reviewed changes

sofisl merged commit efe72a7 into master Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add no_proxy env variable #361

feat: add no_proxy env variable #361

sofisl commented Nov 6, 2020

sofisl Nov 6, 2020

codecov bot commented Nov 6, 2020 •

edited

JustinBeckwith Nov 6, 2020

sofisl Nov 9, 2020

JustinBeckwith Nov 6, 2020

JustinBeckwith Nov 6, 2020

JustinBeckwith Nov 6, 2020

sofisl Nov 9, 2020

bcoe Nov 9, 2020

JustinBeckwith Nov 6, 2020

sofisl commented Nov 10, 2020

bcoe Nov 16, 2020

bcoe Nov 30, 2020

bcoe left a comment

bcoe Dec 2, 2020

bcoe Dec 2, 2020

feat: add no_proxy env variable #361

feat: add no_proxy env variable #361

Conversation

sofisl commented Nov 6, 2020

Choose a reason for hiding this comment

codecov bot commented Nov 6, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofisl commented Nov 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 6, 2020 •

edited