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

Ensure the proxies are configured before vagrant-omnibus etc. #13

Merged
merged 2 commits into from
Aug 30, 2013

Conversation

tmatilai
Copy link
Owner

Try to kick in before other plugins so that the proxy settings would take effect for them, too. This would be especially useful with #6 and e.g. vagrant-omnibus plugin.

Thanks to @tknerr for the idea.

@tmatilai
Copy link
Owner Author

After trying to understand how the things really work, this seems to be quite complicated. :/

We are now hooking to the Provision action, which gets called before the machine is booted. Thus we need to pass the control forward and wait for it to return before configuring the guest. And as the plugins are loaded in alphabetical order, and due to how the Hook class is implemented, we end up doing our magic only after vagrant-omnibus, which hooks to the same action.

The only solution (besides renaming the plugin) that I can think of, is to hook to all Boot or Create actions on up and reload commands. But the class is different in all providers. And then we should still handle at least provision and rebuild commands separately. So overall it would get quite complicated and even fragile. And support for new providers needs to be added case by case.

@mitchellh, sorry to poke you, but would you have any better ideas?
Or someone else?

@tknerr
Copy link

tknerr commented Aug 26, 2013

@tmatilai can we maybe hook in before one of the actions defined by vagrant-omnibus?

@fgrehm some more ideas?

@tknerr
Copy link

tknerr commented Aug 26, 2013

e.g. something like this:

if VagrantPlugins.const_defined?('Omnibus')
  hook.before VagrantPlugins::Omnibus::Action::InstallChef, Action::ConfigureEnvProxy
else
  hook.after Vagrant::Action::Builtin::Provision, Action::ConfigureEnvProxy
end

@tmatilai
Copy link
Owner Author

can we maybe hook in before one of the actions defined by vagrant-omnibus?

Sure, but then we will need more and more logic for other (combinations) of plugins that need the proxy configurations to be set. OK, I admit that at the moment I know none that download something on the guest, but with the future of thin boxes I guess it gets more common.

Maybe we could just ask (via a PR) @schisamo to hook vagrant-omnibus before the Provision action? That way we can set the configuration before it.

[...]

Sigh, after testing that a bit it seems that for some reason the configuration is not loaded even if the shell should be bash -l. :(
A shell provisioner works as expected. Too much magic for monday morning.

So yet another option. What if Vagrant offered plugins a way to inject environment variable settings in all commands in the SSH communicator plugin, similar to TERM? Then we wouldn't be so dependent on the execution order as we could configure those in a much earlier phase. /cc @mitchellh @fgrehm

@tmatilai
Copy link
Owner Author

Ugh, stupid me. I didn't set https proxy when testing. So vagrant-omnibus uses the profile.d settings as expected. Sorry. =)

@tmatilai
Copy link
Owner Author

I created chef-boneyard/vagrant-omnibus#34.

@tmatilai
Copy link
Owner Author

And closed it too. Of course it doesn't work correctly, as it would wait for the whole Provision action to finnish.

So the options seem to be to hook to either VM creation actions or to the omnibus action. I don't like much either one.

(Quite epic chat with myself. Hope you have popcorn.)

@tknerr
Copy link

tknerr commented Aug 27, 2013

@tmatilai I'm in favor of adding the 80% solution by hooking to the omnibus action, unless @mitchellh or @fgrehm have some better ideas how to do this in a more generic way.

It's the same arguments for hooking to the TimedProvision action if the vagrant-aws plugin is installed: it's not generic and in theory you would have to do that for every other plugin which extends the Provision action, but it's so immensely useful because so many people are using vagrant-aws. The same argumentation holds for vagrant-omnibus imho.

btw: +1 for so much transparency by chatting with yourself (no popcorn but pizza is in the oven now ;-))

@fgrehm
Copy link

fgrehm commented Aug 30, 2013

@tknerr @tmatilai sorry guys, unfortunately I can't think of anything else apart from hooking to the omnibus action right now. I'll keep this on my "radar" and will let u guys know in case I have a better idea

@tmatilai
Copy link
Owner Author

Yeah, lets start with that and change later if we get better ideas, or if Vagrant starts to supports injecting env vars for the commands.

I hope to get #14 and this done in the next couple of days, and at least an RC release out right after that. Lets see if the kids and life agree. ;)

@tknerr
Copy link

tknerr commented Aug 30, 2013

@tmatilai 👍 that woud be awesome :-)

tmatilai added a commit that referenced this pull request Aug 30, 2013
Ensure the proxies are configured before vagrant-omnibus etc.
@tmatilai tmatilai merged commit 946deb0 into master Aug 30, 2013
@tmatilai tmatilai deleted the hook-before-omnibus branch August 30, 2013 19:10
@tmatilai
Copy link
Owner Author

Ok folks, I just released v0.4.0.rc1. Can be installed with command:

vagrant plugin install --plugin-source https://rubygems.org/ --plugin-prerelease vagrant-proxyconf

All testing appreciated. =)

@tknerr
Copy link

tknerr commented Aug 31, 2013

Thanks @tmatilai!

It works as expected, but unfortunately the env proxy settings are not honored with the latest 1.1.0 release of vagrant-omnibus. The culprit lies in this line where the environment is not preserved in the sudo context (it's not sudo -E).

However, the latest version in master now omits the anyways unecessary sudo and thus it works fine there.

As soon as the next version of vagrant-omnibus is released the proxy settings should take effect. I have created chef-boneyard/vagrant-omnibus#35 requesting an 1.1.1 release -- should be coming out soon (thanks @schisamo!)

@tmatilai
Copy link
Owner Author

Ah, might be that I tested with the master version only...
I think it also depends on the system's and root user's shell setup. On some cases the profile.d script should be evaluated also with sudo, so it wouldn't be necessary to pass the env vars through processes.

@tknerr
Copy link

tknerr commented Sep 1, 2013

Afaik the bevaviour of env vars when using sudo can be configured in '/etc/sudoers' only (via 'env_reset' and 'env_keep'), at least on Ubuntu. But now that vagrant-omnibus does not pipe to 'sudo bash' anymore it's not an issue anymore. Not sure if there are shell settings for this as well...

Anyways, great work 👍,thanks!

@tknerr
Copy link

tknerr commented Sep 1, 2013

Ah. Now I understand. Yes, I 'm wondering too why /etc/profile.d was not read when 'sudo bash' was called. That's probably a '.profile' or '.bashrc' thing...

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.

3 participants