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 support for global *_proxy environment variables #6

Merged
merged 8 commits into from
Aug 26, 2013
Merged

Conversation

tmatilai
Copy link
Owner

@tmatilai tmatilai commented Jul 1, 2013

Many programs (wget, curl, yum, etc.) can be configured to use proxies with <protocol>_proxy or <PROTOCOL>_PROXY environment variables. Based on the same environment variables on the host and Vagranfile configuration we'll generate /etc/profile.d/proxy.sh that will be evaluated by most shells.

All comments welcome!

@patcon
Copy link

patcon commented Jul 2, 2013

Nice idea! Also, I like that you wrote the docs first :)

@tmatilai
Copy link
Owner Author

tmatilai commented Jul 2, 2013

@patcon Thanks! I hate writing docs, but in this case I wanted to start with planning the syntax etc. And even secretly hoped there would be some other opinions. ;)

Speaking of documentation, please feel free to fix the language and typos. I'm far from a native English speaker as you should have noticed.

@tmatilai
Copy link
Owner Author

tmatilai commented Aug 1, 2013

Sigh, I have some spike code lying in my local repo. Hope to find time and motivation to clean it up and push it out soonish.

@tknerr
Copy link

tknerr commented Aug 1, 2013

👍 for this PR :-)

@tmatilai
Copy link
Owner Author

@tknerr: I agree that using HTTP_PROXY on the host environment is not a good idea in general. How about something like this: https://gist.github.com/tmatilai/6253077 ? I would reserve VAGRANT_HTTP_PROXY for #14, so we have to come up with something else for this.

/cc @patcon @fgrehm

@tknerr
Copy link

tknerr commented Aug 16, 2013

@tmatilai I'm 👍 for 'VAGRANT_ENV_HTTP_PROXY', see comment in the gist

@tmatilai
Copy link
Owner Author

OK, now there is a seemingly working implementation. There is now a lot of duplication (and no tests) in the Action classes, but I would like to get this feature out sooner than later. =)

The capability is now available only on Linuxes. But I think at least Free&OpenBSD have the profile.d included by default. Have to take a look...

Anyway, all testing welcome!

tknerr added a commit to tknerr/vagrant-proxy-test that referenced this pull request Aug 26, 2013
@tknerr
Copy link

tknerr commented Aug 26, 2013

Cool stuff!

I did some basic testing and it all looks good:

  • setting the proxy works as expected (tested on Ubuntu 13.04 guest), yay! :-)
  • like that the VAGRANT_ENV_* vars override the settings in the Vagrantfile
  • I had a hard time unsetting the proxy, but this was because I didn't read the README carefully enough. It works now... ;-)

@tmatilai
Copy link
Owner Author

@tknerr Thanks a lot for testing! All improvements to the README are highly welcome. Documentation is not my best skill. ;)

@tknerr
Copy link

tknerr commented Aug 26, 2013

@tmatilai no worries the docs are fine, it was just me not reading carefully ;-)

@tmatilai
Copy link
Owner Author

I'll merge this now. Action classes will get some refactoring love in #14.

tmatilai added a commit that referenced this pull request Aug 26, 2013
Add support for global `*_proxy` environment variables
@tmatilai tmatilai merged commit 3cbfe46 into master Aug 26, 2013
tmatilai added a commit that referenced this pull request Aug 27, 2013
@tknerr
Copy link

tknerr commented Aug 27, 2013

👍 thanks! :-)

tknerr referenced this pull request in gad0lin/vagrant-omnibus Aug 28, 2013
@fgrehm
Copy link

fgrehm commented Aug 30, 2013

Sorry for the "absence" (I was on vacations for the last week) but this looks good :) I'm planning to finally try out the plugin this weekend and I'll let u guys know how it goes :D

@tknerr
Copy link

tknerr commented Aug 31, 2013

@tmatilai re: unsetting the proxy

Might be a misunderstanding in the README, or a small bug:

  • set VAGRANT_HTTP_PROXY="" unsets the proxy
  • set VAGRANT_HTTP_PROXY= does not

I would have expected that the latter worked as well. Is this as intended?

@tknerr
Copy link

tknerr commented Aug 31, 2013

A bit further testing reveals that set VAGRANT_HTTP_PROXY= becomes effective later after I set VAGRANT_HTTPS_PROXY=to_some_value (notice the HTTPS).

The set VAGRANT_HTTP_PROXY="" reliably works though

@tmatilai
Copy link
Owner Author

@tknerr Which shell are you using? Some c shell? In Posix shells (e.g. bash) set is used for manipulating shell options, positional parameters etc., so it doesn't make much sense here. To be honest I don't know what set VAGRANT_HTTP_PROXY= even does... Without set it assigns an empty string so it should work.

@tknerr
Copy link

tknerr commented Sep 1, 2013

Oh, this was on windows... should have mentioned that ;-)

@tmatilai
Copy link
Owner Author

tmatilai commented Sep 2, 2013

Ha ha, sorry, I should have guessed that. =)

It been more than 10 years that I last scripted or even used Windows, but doesn't set VAGRANT_HTTP_PROXY= unset the whole variable? We can not disable the proxy if the env var is not specified at all, as that would mean we require the env var always to be set...

@tknerr
Copy link

tknerr commented Sep 3, 2013

You must be a happy man, I still have to use Windows at work... ;-)

AFAIK there is no unset under Windows, i.e. the env var always remains and you can only change it's value. For some reason there seems to a difference between set VAGRANT_HTTP_PROXY= and set VAGRANT_HTTP_PROXY="" though, as the latter reliably works.

I'm on vacation now, but I can debug a bit more on my windows system when I'm back. In the meantime set VAGRANT_HTTP_PROXY="" works on windows.

@tmatilai
Copy link
Owner Author

tmatilai commented Sep 3, 2013

Well, this page says:

Delete a variable

Type SET with just the variable name and an equals sign:

SET _department=

That would confirm your experience. You can test it with something like:

ruby -e "puts ENV.fetch('VAGRANT_HTTP_PROXY', 'not set').inspect"

@tknerr
Copy link

tknerr commented Sep 3, 2013

@tmatilai you are totally right, that explains it. Thanks for the clarification! :-)

@hak8or
Copy link

hak8or commented Feb 9, 2014

I want to point out that in the readme.md it does say that this does work with wget but confusion may arrise for some using the example variables.

For example, it says config.env_proxy.http = "http://192.168.33.200:8888/" in the readme, but that won't work with wget giving you this:

vagrant@vagrant-ubuntu-raring-64:~$ wget google.com
Error in proxy URL ftp://squid/[email protected]:3128: Must be HTTP.

You instead need config.proxy.http = "http://squid:[email protected]:3128", specifically the http:// section.

If I may recommend, putting in a comment about this in the readme.md would be immensly helpful for those like me who are confused as to why wget wasn't working.

@tmatilai
Copy link
Owner Author

tmatilai commented Feb 9, 2014

Hi @hak8or,

Maybe I'm missing something, but the readme already has http:// in the examples. And especially:

A proxy should be specified in the form of http://[user:pass@]host:port.

The only place where it has been possible to drop the schema is the apt_proxy, but I'm planning to deprecate even that. True enough, there's one example of apt_proxy still without the http:// prefix. I'll fix that too.

If you have concrete suggestions how to improve the readme, I'm all ears. Documentation (especially in foreign language) is not my strongest skill. =)

The readme in general is bloated and should be split. Have to think a bit...

@hak8or
Copy link

hak8or commented Feb 9, 2014

I have utterly no idea what happened, you are indeed correct. Terribly sorry for wasting your time on this post, I think I started seeing things from lack of sleep or something, hah.

I do agree, it is somewhat convoluted. I think that maybe it is worth it to make a wiki page and put the less common usage scenario examples in it. Quick Start could be moved into Usage.

I feel that Global *_proxy environment variables and Default/global configuration are similar enough that one or the other could be dropped, especially due to how long it is.

Possible value's are pretty much identical across all three sections, they might be able to be merged into one easily. Environment variables are also capable of being merged across the various sections.

Going further with the wiki page, I would recommend that you combine the Global *_proxy environment variables, Default/global configuration, and apt sections into one, using the same Example Vagrant file for them all.

Like this,

Vagrant.configure("2") do |config|

  # Do not enable this plugin if using the cloud provider
  config.vm.provider :my_cloud do |cloud, override|
      override.proxy.enabled = false
  end

  # The proxy for HTTP URIs
  config.env_proxy.http     = "http://192.168.33.200:8888/" 
  config.env_proxy.https    = "http://192.168.33.200:8888/"

  # A comma separated list of hosts or domains which do not use proxies.
  config.env_proxy.no_proxy = "localhost,127.0.0.1,.example.com" 

  # Proxy for Apt-get queries
  config.apt_proxy.http  = "http://192.168.33.1:3142" 

  # Do not use proxy for apt-get queries using HTTPS
  config.apt_proxy.https = "DIRECT" 

  # Do not use proxy for apt-get queries using FTP
  config.apt_proxy.ftp = "DIRECT" 
end

and if you want to enable the plugin only if it is installed

Vagrant.configure("2") do |config|
  if Vagrant.has_plugin?("vagrant-proxyconf")
    config.proxy.http     = "http://192.168.0.2:3128/"
    config.proxy.https    = "http://192.168.0.2:3128/"
    config.proxy.no_proxy = "localhost,127.0.0.1,.example.com"
  end
  # ... other stuff
end

I feel that users should be able to figure pretty much all usage cases from that alone, with you being able to put all the rest of your readme including specifics like possible values for environment variables, into the wiki. This way your readme would be shortened drastically while still giving users the exact info they need to get up and running, and if they need more information then just click on the wiki link and viola.

Hopefully this helps and makes up for me overlooking how all of them already have http in front!

@tmatilai
Copy link
Owner Author

@hak8or not a problem at all, and thanks a lot for the ideas! Thinking of it, I guess we could even deprecate the env_proxy altogether. I can't think of any case when someone would want to specify only the env vars and not everything else too, as they affect in so many places.

The downside of using wiki is that it's not in sync with code nor releases. So I would prefer having the documentation still generated from master either to github pages or somewhere else. This way the documentation changes can also be committed with code PRs.

@johnbellone
Copy link
Contributor

@tmatilai I would actually probably be a great candidate for writing the wiki if you want me to take a stab at it? Since we have an unfortunate situation with our corporate proxies I have a lot of extensive documentation (and patterns we use) that I am sure would be extremely helpful. I literally probably have every single edge case that you could throw at me (internal mirrors for aptitude, requirements for GitHub Enterprise, GitHub.com, MITM SSL issues, etc).

@johnbellone
Copy link
Contributor

I'll slam out some examples and throw them into a PR.

@tmatilai
Copy link
Owner Author

Oh, wiki is perfect for examples, edge cases and war stories. And we can always pick things from there to the "official" documentation. So go ahead!

@hak8or
Copy link

hak8or commented Feb 11, 2014

You guys are all awesome!

Edge cases and more examples in the wiki all sound like fantastic ideas, all of which I feel is the way to go. Maybe at the bottom of a wiki include a small note like "Checked for relevance at Version XX" and then someone like myself can always do a quick test of various examples to see if they need updating, with a successful test bumping the version checked with up to the current one.

@johnbellone
Copy link
Contributor

Take a look at this Wiki entry. I will expand on it a little bit more this afternoon but it gives you a general idea of how I am composing together environment values.

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.

6 participants