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

elasticsearch_plugin install lacks proxy support #415

Closed
dbaggott opened this issue Dec 28, 2015 · 8 comments · Fixed by #420
Closed

elasticsearch_plugin install lacks proxy support #415

dbaggott opened this issue Dec 28, 2015 · 8 comments · Fixed by #420

Comments

@dbaggott
Copy link

The underlying ES install tool supports proxies. This cookbook does not.

It would be awesome if the cookbook supported it too. I think ideally it would automatically proxy if the Chef run is configured with a proxy.

Alternatively, I can imagine making the wrapping cookbook handle it by (optionally) passing in a proxy:

elasticsearch_plugin 'analysis-icu' do
  action :install
  proxy_host 'proxy.host.com'
  proxy_port 3128
end
@dbaggott
Copy link
Author

And, with apologies to all that's decent, here's the horrific hack of a work around until a fix is made and released.

Note that this temporary work-around is dependent on the specifics of how the call to the install tool is currently implemented. It's fragile.

Also note that as of ES 2.0 core plugins (like cloud-aws) are now versioned with ES and, if you are forced to adopt this hack, care should be taken that the version of the supplied url matches the version of ES.

  if Chef::Config['http_proxy']
    proxy_host = Chef::Config['http_proxy'][/\/+([^:]+):/, 1]
    proxy_port = Chef::Config['http_proxy'][/:(\d+)/, 1]
    optional_proxy_settings = "-DproxyHost=#{proxy_host} -DproxyPort=#{proxy_port}"
  else
    optional_proxy_settings = ''
  end

  elasticsearch_plugin 'cloud-aws' do
    action :install
    # appending the proxy settings to the url is an unsupported hack!
    url "https://download.elastic.co/elasticsearch/release/org/elasticsearch/plugin/cloud-aws/2.1.1/cloud-aws-2.1.1.zip #{optional_proxy_settings}"
  end

@dbaggott
Copy link
Author

Thoughts on the approach? If interrogating whether chef is configured with a proxy (ie along the lines of what's in that hack) makes sense to you, I should be able to provide a PR for consideration in the near future.

Personally, the automatic proxying approach makes more sense to me but maybe I'm missing a scenario in which manually specifying a proxy is necessary...

martinb3 added a commit that referenced this issue Dec 30, 2015
Adds sniffing out proxy settings to pass to `bin/plugin` when installing ES plugins via Chef resource.

Fixes #415.
@martinb3
Copy link
Contributor

I think it's a perfectly reasonable approach. I was actually just doing a branch with that built-in... what do you think? ecf427d

@dbaggott
Copy link
Author

Looks great, thank you!

Suggestions for your consideration below:

I think URI is a standard library in Ruby so is probably a cleaner approach than the regex stuff from my hack. I'm not a native ruby speaker so this might need massaging but what do you think about a modification of the code for DRYness along these lines?

def get_java_proxy_arguments
  configured_proxy = get_configured_proxy
  if configured_proxy
    uri = URI.parse(configured_proxy)
    port = uri.port ? uri.port : '80'
    "-DproxyHost=#{uri.host} -DproxyPort=#{port}"
  else
    '' # no proxy available 
  end
end

def get_configured_proxy
  if Chef::Config['http_proxy'] && !Chef::Config['http_proxy'].empty?
    Chef::Config['http_proxy']
  elsif Chef::Config['https_proxy'] && !Chef::Config['https_proxy'].empty?
    Chef::Config['https_proxy']
  else
    nil
  end
end

# respect chef proxy settings unless they have been disabled explicitly
proxy_argument = new_resource.chef_proxy ? " #{get_java_proxy_arguments}" : ''

It's not a straight refactoring as the port is explicitly set no matter what but that's probably ok?

Edit to Comment: I noticed/fixed a bug in the original get_java_proxy_arguments code (it was always returning the empty string)

@dbaggott
Copy link
Author

Also, +1 for the disabling -- I think it's then reasonable to ignore the Chef::Config['no_proxy'] configuration...

martinb3 added a commit that referenced this issue Jan 2, 2016
Adds sniffing out proxy settings to pass to `bin/plugin` when installing ES plugins via Chef resource.

Fixes #415.
@martinb3
Copy link
Contributor

martinb3 commented Jan 2, 2016

Hello! I made some tweaks based on your suggestion -- it's much cleaner now :) Let me know if you think it looks good and I'll merge it in... 09cdd78

@dbaggott
Copy link
Author

dbaggott commented Jan 3, 2016

Lovely! Thank you!

@martinb3
Copy link
Contributor

martinb3 commented Jan 4, 2016

Merged to master. Thanks again for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants