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

http[s] proxy support #204

Closed
wants to merge 1 commit into from
Closed

http[s] proxy support #204

wants to merge 1 commit into from

Conversation

wrightp
Copy link

@wrightp wrightp commented May 10, 2017

tl;dr add http and https proxy support to mixlib install api.

See Readme updates for how the options interact with different aspects of the application.

Proxy unit and functional tests run against a mock http proxy server.
The windows powershell changes were tested manually using Fiddler.

This change only includes http and https proxy settings. It does not include the full proxy gambit of passing credentials or ftp proxies. This can always be added later.

To add automated acceptance tests a proxy server will need to be provisioned for each proxy test (windows and linux). Let's get this out in the field and think about the best approach for proxy acceptance tests.

  • API calls to package router
  • install.sh
  • install.ps1
  • CLI downloads
  • acceptance tests (manually for Windows)

Signed-off-by: Patrick Wright patrick@chef.io

@wrightp
Copy link
Author

wrightp commented May 10, 2017

#201

@wrightp
Copy link
Author

wrightp commented May 10, 2017

@iancward Here's the PR for the http proxy work. It will take some time as I have to make this work for the installation scripts and download features. Super rough now, but it'll get there.

@wrightp wrightp force-pushed the pw/proxy branch 3 times, most recently from 9fe0f80 to 6fac3d0 Compare May 12, 2017 20:41
Net::HTTP.new(uri.host, nil, proxy_uri.host, proxy_uri.port, use_ssl: use_ssl).start do |http|
response = http.request(request)
end
rescue SocketError => e
Copy link
Author

Choose a reason for hiding this comment

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

I may need to add ConnectionError to this as well...

@wrightp wrightp force-pushed the pw/proxy branch 3 times, most recently from f6b471d to 3b33a1b Compare May 16, 2017 22:45
@wrightp wrightp changed the title [WIP] http_proxy support http/s_proxy support May 16, 2017
@wrightp wrightp changed the title http/s_proxy support http[s]_proxy support May 17, 2017
@wrightp wrightp changed the title http[s]_proxy support http[s] proxy support May 17, 2017
@wrightp wrightp force-pushed the pw/proxy branch 2 times, most recently from 029982e to 0fad8c2 Compare May 17, 2017 22:34
@wrightp wrightp requested review from schisamo, mwrock and smurawski May 17, 2017 22:40
@wrightp
Copy link
Author

wrightp commented May 17, 2017

@smurawski @mwrock - I added you to get your 👀 on the powershell proxy changes.

@wrightp
Copy link
Author

wrightp commented May 17, 2017

@chef/engineering-services

Copy link
Member

@mwrock mwrock left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@schisamo schisamo left a comment

Choose a reason for hiding this comment

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

One small suggestion but looks good!

@@ -64,11 +64,13 @@ def available_versions
#
# @return [Array<String>] list of available versions for the given
# product_name and channel.
def self.available_versions(product_name, channel)
def self.available_versions(product_name, channel, https_proxy = nil, http_proxy = nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it makes sense to make the 3rd parameter an opts hash? This would allow easy adding of more options in the future:

#
# List available versions
#
# @param [String] product name
#
# @param [String, Symbol] channel
#
# @param [Hash] opts
# @option opts [String] :https_proxy 
# @option opts [String] :http_proxy 
#
# @return [Array<String>] list of available versions for the given
# product_name and channel.
def self.available_versions(product_name, channel, opts = {})
  Backend.available_versions(
    Mixlib::Install::Options.new(
      product_name: product_name,
      channel: channel.to_sym,
      https_proxy: opts[:https_proxy],
      http_proxy: opts[:http_proxy]
    )
  )
end

@schisamo schisamo requested review from rhass and tduffield May 19, 2017 14:52
Signed-off-by: Patrick Wright <patrick@chef.io>
@wrightp
Copy link
Author

wrightp commented May 19, 2017

@iancward Any chance that you could try out this brach and see if the http proxy options behave as you would expect?

@iancward
Copy link

@wrightp wow, I didn't realize how extensive a change like this would be.

The first thing I noticed is that it appears that if you don't specify a URI scheme with the http_proxy (or https_proxy), it defaults to https. I had actually put http:// at the front of the proxy URL when using an http proxy.

I was able to get the list versions feature to work both via pry/irb and the CLI (which I wish was documented a bit better in the README); however, when I attempted to use the CLI to download the artifact, it failed. It was able to identify what to download (it fetched the latest version just fine), but it looks like the actual download method just uses Net::HTTP without any proxy options.

I was able to use Mixlib::Install.new(options).available_versions with only the http_proxy option specified, but when I tried to use Mixlib::ShellOut to use the Mixlib::Install.new(options).install_command, it didn't work unless I specified the https_proxy option (regardless of whether http_proxy was specified).

I have a windows machine accessible to me; however, trying to test this there is proving challenging. (yay for trying to use ruby, bundler, chef, and git on windows). I'll keep working at it and try to give you a response by early next week.

@wrightp
Copy link
Author

wrightp commented May 19, 2017

@iancward Thank you for the response! Incredibly valuable! I'll try to structure my response as best as I can.

URI scheme requirement:

The way the code is currently written the URI scheme (http://) would be required. The previous code base used all internal URLs so we knew they would all exist, and in fact all be https. That was a missed assumption and something I can fix.

CLI download command

I totally missed that part! This bit of code is removed from the rest of the application (and in fact the only place we download anything.) I will fix that as well. My tests missed it because my test proxy server catches the metadata query, but has free access to the internet to download :/

CLI docs

We were hoping the help command would be sufficient, but in reality the tool has grown much in complexity since inception. We could certainly add docs for it's capabilities.

http vs https proxy combinations

I mimicked much of how the chef client uses proxies. Which may not be the same functionality we really need. There could be an inherent design issue more than a bug itself. See the section below for how I think I need to fix this moving forward.

My thoughts on splitting proxy setting responsibilities

I think something that I may just have to give up on is trying to get the proxy options for the API to also magically work for the scripts. Round peg, square hole kind of thing. I'm thinking moving forward the proxy options will have to be set differently for API vs install script generation. The install_command_options option is designed exactly for that. For example,

http_proxy: "pserver:80" # API
install_command_options: { http_proxy: "pserver:80" } # Install script param/var

I'm worried that this will feel like double-setting a value when in fact they are quite different. Perhaps good documentation would solve the issue.

@iancward
Copy link

iancward commented May 22, 2017

Is install_command_options a Mixlib::Install thing or a Chef API thing?
I think the separate http_proxy and https_proxy options make sense, because curl (and I believe yum as well) will use environment variables for each if they are set; however, I don't remember if curl requires https_proxy to be set for for https requests.

When I used the https_proxy option, I was able to successfully install chef-client on a RHEL host behind a firewall.
Maybe instead of an install_command_options hash, it would be easier to call it command_options that could be used across the board (e.g. CLI for available version fetch, install; bash/ps1 script generator). I'm uncertain what might be useful beyond proxy support, but potentially whatever is added would be useful (or even necessary) for everything else, but still should exist outside of the "normal" options (e.g. release channel, product name, platform, architecture).

@iancward
Copy link

I upgraded chefdk to 1.0.3 on a windows machine to get the required version of ruby for chefstyle (I was hoping to have Mixlib::Install update it for me), and I was able to run chef exec bundle exec rake console and actually get Mixlib::Install.new(options).available_versions to work when using the proxy.

However, when I tried to use Mixlib::ShellOut to run the install_command, I got an error on execution that "The command line is too long". This is on a 64-bit Windows 2008R2 machine. I've received this error running rake console from both a cmd prompt and a powershell prompt, and both with and without calling #detect_platform prior to instantiating Mixlib::ShellOut.

I found a few issues for test kitchen regarding similar errors; however, I'm running the mixlib-install bits directly on the server.
test-kitchen/test-kitchen#811
test-kitchen/test-kitchen#854

The install_command powershell command is only 413 lines long, so I'm not quite sure why I'm getting this error. If this was "normal" on Windows, I'd expect to see an issue or something in the chef_client_updater cookbook, but there's no mention of this error there.

I'm also unable to run the mixlib-install CLI via bundler (bundler: not executable: bin\mixlib-install). I'm not sure if Thor supports Windows; however, there's a CHANGELOG entry from 2008 that claims to have "improved" support for it..

@iancward
Copy link

iancward commented May 22, 2017

Ah I see. There are existing install_command_options to override the download URL, and that really only applies to downloading the artifact and not searching for available versions. However, if someone is overriding the download URL, is the version search even relevant? Maybe just rename the parameter/hash with an alias and simplify things a little?

I suppose there may also be a scenario where someone would want to use both a web proxy and override the download URL (probably unlikely, since the overridden download URL is probably used where packages.chef.io is unreachable; however, a possibility nonetheless).

@wrightp
Copy link
Author

wrightp commented May 22, 2017

This PR is not the correct solution. It seemed straight forward at first, but has become convoluted Before trying to work this further I'm going create a design doc with requirements and go from there.

@wrightp wrightp closed this May 22, 2017
@iancward
Copy link

@wrightp thanks for the update. Please let me know if there's any assistance I can provide in terms of requirements, etc.

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 this pull request may close these issues.

5 participants