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

Add proxy support for plugin installation #7967

Closed
wants to merge 4 commits into from

Conversation

barryib
Copy link

@barryib barryib commented Aug 9, 2016

  • Replace Wreck with the Request module. This introduce an extra http (HEAD method) request to get only headers (content length by exemple) and ensure that plugin exist before trying to donwload it.
  • Add proxy support with --proxy=http://proxy.exemple.com:8080/ option

@elasticmachine
Copy link
Contributor

Can one of the admins verify this patch?

@barryib
Copy link
Author

barryib commented Aug 9, 2016

Could probably address #5211 #5196 #5902

@epixa
Copy link
Contributor

epixa commented Aug 10, 2016

@barryib What's the value of the additional HEAD request before the GET request? By the very nature of HEAD, it should return exactly the same headers as the corresponding GET request, so if you know that you want to do the GET request, you might as well just try it.

@barryib
Copy link
Author

barryib commented Aug 10, 2016

@epixa Good point. It was the simplest way I found to do minimum changes within the code base, without going through callback hell and furthermore without decreasing performances.

But I'm opened to any suggestions.

@barryib
Copy link
Author

barryib commented Aug 12, 2016

@epixa I've removed the extra request and try to download directly.

@epixa epixa added the review label Aug 12, 2016
@epixa
Copy link
Contributor

epixa commented Aug 12, 2016

Awesome, thanks @barryib. I marked this for review so people can jump on it when they have a chance, otherwise I'll try to get to it myself in the next few days.

@epixa
Copy link
Contributor

epixa commented Aug 12, 2016

jenkins, test this

@epixa
Copy link
Contributor

epixa commented Aug 20, 2016

@barryib We were having some issues with our tests last week. Can you rebase or merge master into this to make sure it's running on the latest?

@barryib
Copy link
Author

barryib commented Aug 20, 2016

@epixa done. Is it ok for you?

@epixa
Copy link
Contributor

epixa commented Aug 20, 2016

jenkins, test this

@xkrt
Copy link

xkrt commented Sep 2, 2016

What about proxy authentication?

@barryib
Copy link
Author

barryib commented Sep 2, 2016

@xkrt It should works. I didn't test it in our infrastructure because we don't have proxy auth, but in request documentation, it says:

proxy - An HTTP proxy to be used. Supports proxy Auth with Basic Auth, identical to support for the url parameter (by embedding the auth info in the uri)

So you only need to pass auth like this --proxy=http://user:password@proxy.exemple.com:8080/


req.on('response', (resp) => {
Copy link
Member

Choose a reason for hiding this comment

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

resp can be used without () as its a single argument

Copy link
Author

Choose a reason for hiding this comment

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

@ppisljar good to know. But I think it's better this way for coding style consistency, as it's write like that in the rest of code base.

@ppisljar
Copy link
Member

ppisljar commented Sep 2, 2016

i added a few small style comments

@barryib
Copy link
Author

barryib commented Sep 2, 2016

thanks @ppisljar I fixed some coding style you pointed out.

@epixa
Copy link
Contributor

epixa commented Sep 13, 2016

jenkins, test this

@epixa
Copy link
Contributor

epixa commented Sep 13, 2016

@barryib Can you move wreck to devDependencies in package.json? Or bonus points if you update tasks/build/download_node_builds.js to use request instead and then remove wreck entirely.

@epixa
Copy link
Contributor

epixa commented Sep 13, 2016

@barryib Also, you probably need to rebase or merge master into this branch in order to get CI passing.

@epixa
Copy link
Contributor

epixa commented Sep 13, 2016

I'm not sure if this is because the branch needs to be updated with master or something, but I cannot get a standard (no-proxy) install to work properly. If I try to install timelion, for example:

$ bin/kibana-plugin install timelion
Attempting to transfer from timelion
Attempting to transfer from https://download.elastic.co/kibana/timelion/timelion-5.0.0-alpha5.zip
Transferring 5489534 bytes....................
Transfer complete
Retrieving metadata from plugin archive
Possibly unsupported ZIP platform type, 110
Possibly unsupported ZIP platform type, 105
Possibly unsupported ZIP platform type, 129
Possibly unsupported ZIP platform type, 115
Possibly unsupported ZIP platform type, 95
Possibly unsupported ZIP platform type, 101
Possibly unsupported ZIP platform type, 8
Possibly unsupported ZIP platform type, 109
Possibly unsupported ZIP platform type, 115
Error: Offset is out of bounds
Plugin installation was unsuccessful due to error "Error retrieving metadata from plugin archive"

@barryib
Copy link
Author

barryib commented Sep 19, 2016

@epixa I tried to remove wreck entirely from dependences.

Can you test it please?

@epixa
Copy link
Contributor

epixa commented Sep 19, 2016

jenkins, test this

@barryib
Copy link
Author

barryib commented Oct 6, 2016

@epixa any news or ETA for this PR?

@epixa
Copy link
Contributor

epixa commented Oct 6, 2016

@barryib Nothing to report :-/ We've been pretty caught up with trying to get 5.0 out the door, so feature PRs like this have unfortunately taken a back seat temporarily. We'll get a review on this asap though.

@mrlannigan
Copy link

Just a heads up, but the full conversion from wreck doesn't necessarily need to be done to add proxy support.

example usage.

const Wreck = require('wreck');
const HttpProxyAgent = require('http-proxy-agent');
const proxy = process.env.http_proxy || 'http://127.0.0.1:3128'; // this is placeholder logic
const agent = new HttpProxyAgent(proxy);

const wreck = Wreck.defaults({
    agent
});

Just an idea.

@epixa epixa added the Team:Operations Team label for Operations Team label Nov 23, 2016
@tronicum
Copy link

tronicum commented Jan 3, 2017

could we upgrade the PR to merge? would love to have it working, same as elastic/logstash#6044

@tronicum
Copy link

tronicum commented Jan 3, 2017

patching /usr/share/elasticsearch/bin/elasticsearch-plugin
with
ES_JAVA_OPTS="$ES_JAVA_OPTS -Dhttps.proxyHost=proxy.yourhost -Dhttps.proxyPort=3128"

works, too (and is easy)

@MartinKuhne
Copy link

Is there a workaround for this? I am behind a proxy. I can get x-pack installed into elasticsearch using the offline file method

PS C:\tools\elasticsearch> bin/elasticsearch-plugin install file:///d:/x-pack-5.1.1.zip
-> Downloading file:///d:/x-pack-5.1.1.zip

however

C:\Tools\kibana>bin\kibana-plugin install file:///D:/x-pack-5.1.1.zip
Attempting to transfer from file:///D:/x-pack-5.1.1.zip
Error: ENOTFOUND

Tried JAVA_OPTS to no avail. This is on Win10 RS2. Can't load data into it if I can't secure it :(

@tronicum
Copy link

tronicum commented Jan 6, 2017

@tronicum
Copy link

tronicum commented Jan 6, 2017

You can try to use my PR code

@MartinKuhne
Copy link

I am seeing a call into node.exe from the plugin installer. Does that really use java? I am not being successful despite setting a global JAVA proxy or setting the appropriate env variables.

@tronicum
Copy link

tronicum commented Jan 7, 2017

kind of. node.js = JAVAscript ;) try this one (http://superuser.com/questions/347476/how-to-install-npm-behind-authentication-proxy-on-windows)

@barryib
Copy link
Author

barryib commented Mar 17, 2017

@epixa @mrlannigan Juste came back from holidays. How should we handle the plugin installation through proxy?

@barryib
Copy link
Author

barryib commented Jun 15, 2017

Closing this PR. Refaire to #10946 for Plugin install through proxy.

@barryib barryib closed this Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Operations Team label for Operations Team updates_needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants