Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add no_proxy env variable #361

Merged
merged 17 commits into from Dec 8, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
66 changes: 59 additions & 7 deletions src/gaxios.ts
Expand Up @@ -42,8 +42,6 @@ function hasFetch() {

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() {
const proxy =
process.env.HTTPS_PROXY ||
Expand All @@ -53,10 +51,63 @@ function loadProxy() {
if (proxy) {
HttpsProxyAgent = require('https-proxy-agent');
}
return proxy;
}
loadProxy();
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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


sofisl marked this conversation as resolved.
Show resolved Hide resolved
function matchingProxyStrings(
envVarHTTPS: string | undefined,
envVarHTTP: string | undefined,
envVarhttps: string | undefined,
envVarhttp: string | undefined,
url: string
) {
const arrayOfEnvVariables = (
envVarHTTPS ||
envVarHTTP ||
envVarhttps ||
envVarhttp
)?.split(',');

let isMatch;
if (arrayOfEnvVariables && arrayOfEnvVariables.length > 0) {
const parsedURL = new URL(url);
isMatch = arrayOfEnvVariables.find(url => {
if (url.startsWith('*.') || url.startsWith('.')) {
url = url.replace('*', '');
return parsedURL.hostname.endsWith(url);
} else {
return url === parsedURL.origin || url === parsedURL.hostname;
}
});
}
return isMatch;
}

// 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 getProxy(url: string) {
const shouldThisBeNoProxy = matchingProxyStrings(
process.env.no_proxy,
process.env.no_proxy,
undefined,
undefined,
url
);
// If there is a match between the no_proxy env variables and the url, then do not proxy
if (shouldThisBeNoProxy) {
return undefined;
// If there is not a match between the no_proxy env variables and the url, check to see if there should be a proxy
} else {
return matchingProxyStrings(
process.env.HTTPS_PROXY,
process.env.https_proxy,
process.env.HTTP_PROXY,
process.env.http_proxy,
url
);
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.


export class Gaxios {
private agentCache = new Map<
string,
Expand Down Expand Up @@ -219,13 +270,14 @@ export class Gaxios {
}
opts.method = opts.method || 'GET';

const proxy = loadProxy();
const proxy = getProxy(opts.url);
if (proxy) {
loadProxy();
Copy link
Contributor

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

sofisl marked this conversation as resolved.
Show resolved Hide resolved
if (this.agentCache.has(proxy)) {
opts.agent = this.agentCache.get(proxy);
opts.agent = this.agentCache.get(opts.url);
} else {
opts.agent = new HttpsProxyAgent(proxy);
this.agentCache.set(proxy, opts.agent!);
opts.agent = new HttpsProxyAgent(opts.url);
this.agentCache.set(opts.url, opts.agent!);
}
}

Expand Down
183 changes: 175 additions & 8 deletions test/test.getch.ts
Expand Up @@ -249,14 +249,181 @@ describe('🥁 configuration options', () => {
assert.deepStrictEqual(res.data, {});
});

it('should use an https proxy if asked nicely', async () => {
sandbox.stub(process, 'env').value({https_proxy: 'https://fake.proxy'});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.ok(res.config.agent instanceof HttpsProxyAgent);
describe('proxying', () => {
it('should use an https proxy if asked nicely', async () => {
const url = 'https://fake.proxy';
sandbox.stub(process, 'env').value({https_proxy: 'https://fake.proxy'});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.ok(res.config.agent instanceof HttpsProxyAgent);
});

it('should not proxy when url matches no_proxy', async () => {
const url = 'https://example.com';
sandbox.stub(process, 'env').value({
https_proxy: 'https://fake.proxy',
no_proxy: 'https://example.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.strictEqual(res.config.agent, undefined);
});

it('should not proxy if url does not match either env variable', async () => {
const url = 'https://example2.com';
sandbox.stub(process, 'env').value({
https_proxy: 'https://fake.proxy',
no_proxy: 'https://example.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.strictEqual(res.config.agent, undefined);
});

it('should not proxy if no_proxy URL matches the origin or hostname of the URL', async () => {
const url = 'https://example2.com';
sandbox.stub(process, 'env').value({
https_proxy: 'https://fake.proxy',
no_proxy: 'example2.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.strictEqual(res.config.agent, undefined);
});

it('should proxy if proxy URL matches the origin or hostname of the URL', async () => {
const url = 'https://example2.com';
sandbox.stub(process, 'env').value({
https_proxy: 'example2.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.ok(res.config.agent instanceof HttpsProxyAgent);
});

it('should not proxy if no_proxy env variable has asterisk', async () => {
const url = 'https://domain.example.com';
sandbox.stub(process, 'env').value({
https_proxy: 'https://fake.proxy',
no_proxy: '*.example.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.strictEqual(res.config.agent, undefined);
});

it('should proxy if proxy env variable has asterisk', async () => {
const url = 'https://domain.example.com';
sandbox.stub(process, 'env').value({
https_proxy: '*.example.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.ok(res.config.agent instanceof HttpsProxyAgent);
});

it('should not proxy if no_proxy env variable starts with a dot', async () => {
const url = 'https://domain.example.com';
sandbox.stub(process, 'env').value({
https_proxy: 'https://fake.proxy',
no_proxy: '.example.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.strictEqual(res.config.agent, undefined);
});

it('should proxy if proxy env variable starts with a dot', async () => {
const url = 'https://domain.example.com';
sandbox.stub(process, 'env').value({
https_proxy: '.example.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.ok(res.config.agent instanceof HttpsProxyAgent);
});

it('should allow comma-separated lists for no_proxy env variables', async () => {
const url = 'https://api.google.com';
sandbox.stub(process, 'env').value({
https_proxy: 'https://fake.proxy',
no_proxy: 'example.com,*.google.com,hello.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.strictEqual(res.config.agent, undefined);
});

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

Choose a reason for hiding this comment

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

I think this test can be dropped.

sandbox.stub(process, 'env').value({
https_proxy: 'example.com,*.google.com,hello.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.ok(res.config.agent instanceof HttpsProxyAgent);
});

it('should let no_proxy take precedence, if url is in both proxy and no_proxy env variables', async () => {
const url = 'https://example.com';
sandbox.stub(process, 'env').value({
https_proxy: 'example.com',
no_proxy: 'example.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.strictEqual(res.config.agent, undefined);
});

it('should proxy if url is an exact match of proxy, but not of no_proxy', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const url = 'https://hello.example.com';
sandbox.stub(process, 'env').value({
https_proxy: 'https://hello.example.com',
no_proxy: 'example.com',
});
const body = {hello: '🌎'};
const scope = nock(url).get('/').reply(200, body);
const res = await request({url});
scope.done();
assert.deepStrictEqual(res.data, body);
assert.ok(res.config.agent instanceof HttpsProxyAgent);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

});
});

it('should load the proxy from the cache', async () => {
Expand Down