-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
This will now also accept http_proxy/no_proxy (lower case) env variables. It also logs information about the used (or skipped) proxy to the console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks, otherwise it would be great with updated docs + tests that cover getProxyAgent
const shouldProxy = (excludedHosts.indexOf(hostname) === -1); | ||
|
||
if (shouldProxy) { | ||
logger.log(`Use proxy to download plugin.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
-> Using
?
function getProxyAgent(sourceUrl, logger) { | ||
const httpProxy = process.env.http_proxy || process.env.HTTP_PROXY; | ||
// we have a proxy detected, lets use it | ||
if (httpProxy) { |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const excludedHosts = noProxy.split(','); | ||
|
||
// proxy if the hostname is not in noProxy | ||
const shouldProxy = (excludedHosts.indexOf(hostname) === -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excludedHosts.includes(hostname)
?
There was a problem hiding this comment.
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"]
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
|
||
function sendRequest({ sourceUrl, timeout }) { | ||
function getProxyAgent(sourceUrl, logger) { | ||
const httpProxy = process.env.http_proxy || process.env.HTTP_PROXY; |
There was a problem hiding this comment.
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()]
if (httpProxy) { | ||
logger.log(`Picked up proxy ${httpProxy} from http_proxy environment variable.`); | ||
// get the hostname of the sourceUrl | ||
const hostname = URL.parse(sourceUrl).hostname; |
There was a problem hiding this comment.
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);
Extract getEnv method and use destructuring.
Pulling this out from the comment, as it was "hidden" because of another fix: 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 |
@kjbekkelund we have now again two options. We could either use - as discussed with @tylersmalley - the very limited implementation of |
Unless it's very important for 6.0 I prefer doing it properly. 6.1 isn't that long after 6.0. If it's an important feature to get in, though, I'm open to getting it in before ff. |
I would also like to have tests and docs for the limited implementation, so I would say, postpone it till 6.1, unless someone vetoes against it :-) |
++ Focus on doing this right and it'll get released when it gets released |
@kjbekkelund Why do you think we should fork it? I looked through the sources and it seems to do exactly what we need. Could we just pull it from npm? In that case we also wouldn't "need" to write test cases for that part, since the library is covered pretty well with tests. |
@timroes Yeah, we don't need to fork if it does exactly what we want it to do. However, of it's not a perfect fit we can use it as a starting point instead. |
We do want tests on the behaviors we're exposing though, even if they happen to be tested in the upstream project, otherwise a gap in coverage or a breaking change in the dependency means a regression in Kibana |
++ every time we directly expose a third-party dep we need to cover our use of that dep with tests. |
@kjbekkelund changed to use the library now. Would be glad about another review :-) |
Jenkins, please test this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a test that has http_proxy
specified, but does an https request, and the other way around. Those would both be expectNoProxyHit
, right?
@kjbekkelund Good catch. will do so! |
Do you know how standardized these environment variables are? My only thought with this is someone using these for some other application and not being able to shut it off without unsetting. I haven't done any research yet but my gut would be to prefix all environment variable usage with kibana. Anyone have thoughts on this? /cc @elastic/kibana-operations, @elastic/kibana-platform |
From my experience they are pretty common (even though not standardized). Applications using them are e.g. So using Linux since quite some time (and already managing some servers) I was always frustrated when an application doesn't use this env vars - which are mainly Java and node. So if I manage a server where I have set Update: If you look at the famous ArchLinux Wiki these environment variables make the first chapter of "Proxy Settings". |
I second @timroes's statement. I'm setting up an ELK stack using deviantony/docker-elk and landed on this PR when I looked for kibana plugin install proxy support. I eventually found a workaround to install it offline, but it would have been much less work if kibana's plugin installer supported these "standard" In comparison with another Elastic product, this would be way better than what elasticsearch-plugin install requires. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me. LGTM
Proxy support for plugin installer
This pull request supersedes #10946. It adds basic proxy support via the (kind of but not really standard) env variables
http_proxy
and basic support for exclusion of hosts viano_proxy
(though no ports or wildcards are supported.Release Note: Kibana now respects the
http_proxy
,https_proxy
andno_proxy
environment variables to download plugins via a proxy.