Plugin installer proxy support #10946
Conversation
|
Can one of the admins verify this patch? |
|
Thanks for the PR @thomasv314. If you get a chance, would you mind signing the CLA? |
|
@jbudz No problem, I've just signed the agreement. |
|
Excellent. This is the way to go, at least on unix-like platforms. Any idea when this will make it into mainstream? |
|
@jbudz any chance this will get reviewed anytime soon? It would be super helpful for our ELKv5 rollout. Thanks! |
| const noProxy = process.env.NO_PROXY || ''; | ||
|
|
||
| // proxy if the hostname is not in noProxy | ||
| const shouldProxy = (noProxy.indexOf(hostname) === -1); |
There was a problem hiding this comment.
We actually need to split the entries of no_proxy by comma before checking this, since there exist TLD domains, that are the prefix of another. So downloading a plugin from foo.ac would also not use the proxy if your env looks like no_proxy="foo.academy,localhost" even though it should be.
So we should split no_proxy by comma and check for an exact entry.
Also this way we don't support the more complex syntax of no_proxy, with ports and subdomain wildcards. We should think if these are important enough for the beginning that they must be there, or it's a nice to have to implement later.
| import URL from 'url'; | ||
|
|
||
| function getProxyAgent(sourceUrl) { | ||
| const httpProxy = process.env.HTTP_PROXY; |
There was a problem hiding this comment.
Unfortunately the case of the env variable is not really specified. On Linux most programs use the lower case variant http_proxy (and same for no_proxy). Most programs support either lower or upper case and I think we should also try to use lower-case (preferred) and fallback to upper-case.
|
After chatting with @timroes, I like the idea of using To assist with debugging for us and the users we should log when the proxy is used.
|
|
@thomasv314, apologies for taking so long to get to this PR - we do appreciate your contribution. We would like to get this into 6.0, which would require some modifications related to the feedback provided. If you have the bandwidth to do this over the next couple days, that would be great otherwise we can take it over. |
|
@timroes and @tylersmalley, thanks for the feedback. I intend to make those changes and will update this PR in the next few days. |
|
@thomasv314 Since we have a feature freeze on Tuesday for the upcoming version, I would continue working on your pull request in around 10 hours. Please feel free to also commit broken work-in-progress changes, if you already started working at this. Thanks again for the great contribution and sorry that this pull request went unnoticed for so long. |
|
I made the required changes in #12753 . Closing this PR in favor of the new one. |
|
@timroes awesome! thank you, looking forward to these changes :D |
This commit fixes #5211 #5196 #5902 and possibly a few others.
Instead of removing the Wreck dependency and changing the plugin CLI API like in PR #7967, it depends on
HTTP_PROXYandNO_PROXYenvvars.I did not include any tests for this only because I could not find them in the kibana repo. If you can point me to the right place I'd be more than happy to write a unit or feature test.