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

Proxy support for plugin installer #12753

Merged
merged 15 commits into from
Jul 26, 2017
Merged
Show file tree
Hide file tree
Changes from 3 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
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -129,15 +129,16 @@
"expose-loader": "0.7.0",
"extract-text-webpack-plugin": "0.8.2",
"file-loader": "0.8.4",
"font-awesome": "4.4.0",
"flot-charts": "0.8.3",
"font-awesome": "4.4.0",
"glob": "5.0.13",
"glob-all": "3.0.1",
"good-squeeze": "2.1.0",
"gridster": "0.5.6",
"h2o2": "5.1.1",
"handlebars": "4.0.5",
"hapi": "14.2.0",
"http-proxy-agent": "1.0.0",
"imports-loader": "0.6.4",
"inert": "4.0.2",
"jade": "1.11.0",
Expand Down
42 changes: 39 additions & 3 deletions src/cli_plugin/install/downloaders/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,47 @@ import Wreck from 'wreck';
import Progress from '../progress';
import { fromNode as fn } from 'bluebird';
import { createWriteStream } from 'fs';
import HttpProxyAgent from 'http-proxy-agent';
import URL from 'url';

function sendRequest({ sourceUrl, timeout }) {
function getProxyAgent(sourceUrl, logger) {
const httpProxy = process.env.http_proxy || process.env.HTTP_PROXY;
Copy link
Contributor

@kimjoar kimjoar Jul 11, 2017

Choose a reason for hiding this comment

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

nitpick: Maybe extract a getEnv:

const getEnv = env => process.env[env.toLowerCase()] || process.env[env.toUpperCase()]

// we have a proxy detected, lets use it
if (httpProxy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change to early return instead?

if (!httpProxy) {
  return undefined;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have some styleguide about early return? I am personally not a fan of early return, if it's not the very first statement and just checks function parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

logger.log(`Picked up proxy ${httpProxy} from http_proxy environment variable.`);
// get the hostname of the sourceUrl
const hostname = URL.parse(sourceUrl).hostname;
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, maybe:

const { hostname } = URL.parse(sourceUrl);

const noProxy = process.env.no_proxy || process.env.NO_PROXY || '';
const excludedHosts = noProxy.split(',');

// proxy if the hostname is not in noProxy
const shouldProxy = (excludedHosts.indexOf(hostname) === -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

excludedHosts.includes(hostname)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also: might need to handle whitespace in excludedHosts:

'foo, bar'.split(',')
> ["foo", " bar"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done the first. For the second: no_proxy can have a rather complex syntax and lots of tools don't support it all. @tylersmalley and me discussed if we should make an easy implementation at the beginning or try to support it as good as possible and decided to just check for plain hostnames now. We don't support ports at the moment (e.g. no_proxy="artifcacts.elastic.co:443" wouldn't neither match. I would also count this trimming of spaces as "advanced" handling at the moment, since we need to process the all entries again. So we should discuss whether or not we would also enhance the no_proxy support in general then? Discuss plz :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me, just make it super-clear in the docs :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, saw https://github.com/Rob--W/proxy-from-env. If it makes our lives easier we could just fork it and bring it into the codebase and make the necessary changes. It also has a good test suite we can use as a starting point: https://github.com/Rob--W/proxy-from-env/blob/master/test.js


if (shouldProxy) {
logger.log(`Use proxy to download plugin.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use -> Using?

logger.log(`Hint: you can add ${hostname} to the no_proxy environment variable, `
+ `to exclude that host from proxying.`);
return new HttpProxyAgent(httpProxy);
} else {
logger.log(`Found exception for host ${hostname} in no_proxy environment variable. `
+ `Skipping proxy.`);
}
}

return null;
}

function sendRequest({ sourceUrl, timeout }, logger) {
const maxRedirects = 11; //Because this one goes to 11.
return fn(cb => {
const req = Wreck.request('GET', sourceUrl, { timeout, redirects: maxRedirects }, (err, resp) => {
const reqOptions = { timeout, redirects: maxRedirects };
const proxyAgent = getProxyAgent(sourceUrl, logger);

if (proxyAgent) {
reqOptions.agent = proxyAgent;
}

const req = Wreck.request('GET', sourceUrl, reqOptions, (err, resp) => {
if (err) {
if (err.code === 'ECONNREFUSED') {
err = new Error('ENOTFOUND');
Expand Down Expand Up @@ -50,7 +86,7 @@ Responsible for managing http transfers
*/
export default async function downloadUrl(logger, sourceUrl, targetPath, timeout) {
try {
const { req, resp } = await sendRequest({ sourceUrl, timeout });
const { req, resp } = await sendRequest({ sourceUrl, timeout }, logger);

try {
const totalSize = parseFloat(resp.headers['content-length']) || 0;
Expand Down