tmatilai / vagrant-proxyconf

Vagrant plugin that configures the virtual machine to use proxies
MIT License
531 stars 74 forks source link

Cannot force to write but without configuration #188

Closed haxorof closed 5 years ago

haxorof commented 5 years ago

Hi,

I noticed that if I try to overwrite with values but do not want to set any configuration for https and http I do not get the outcome I expected:

config.proxy.http = ""
config.proxy.https = ""
config.proxy.no_proxy = "localhost,127.0.0.1,.localdomain"

It write incorrect values into the Docker config file, example below: /etc/docker/config.json

{
  "proxies": {
    "default": {
      "httpProxy": false,
      "httpsProxy": false,
      "noProxy": "localhost,127.0.0.1,.localdomain"
    }
  }
}

According to documentation this might be a bug, or is this expected behavior?

Empty string ("") or false in any setting also force the configuration files to be written, but without configuration for that key. Can be used to clear the old configuration and/or override a global setting.

Cheers!

codylane commented 5 years ago

Thanks for reporting, this looks like a bug to me. I ran across this yesterday because I had the NO_PROXY environment variable set prior to running vagrant commands and it caused me many hours of troubleshooting. Anyway,I hoped would be an easy fix but I'm having a difficult time trying to decide if not configuring config.http and config.httpsare disabled should also automatically disable config.no_proxy. I'm thinking the answer is yes, but I'm having a hard time convincing myself it should be the case.

I'd appreciate some additional input on what you might expect the intended behavior to be? I appreciate constructive feedback and I'd love additional thoughts. To me, it's the only way this thing will get better is with honest feedback on intended behavior.

It's not a lot of work to fix it should be fairly straight forward but I'm a little busy right no so I may not be able to address this concern for a week or so.

haxorof commented 5 years ago

So for me it is not obvious to disable config.no_proxy if config.http_proxy etc also are disabled. But I have never had any issues by having the NO_PROXY configured in general when other proxy environments are set to empty value. Can I ask you what type of problem you bumped into when only having the NO_PROXY configured? Just to understand better.

Cheers!

codylane commented 5 years ago

Thanks @haxorof for your reply and for your continued help sorting out these types of issues. It means a lot to me.

Basically, I cannot decide either what the correct behavior is because when I leave the proxy environment variable NO_PROXY set and I have unset either of these HTTPS_PROXY and HTTP_PROXY environment variables or both variables null.

The currently implementation is such that if config.proxy.enabled = false it should disable all service providers listed in the README. However, what I found out over the weekend is that if I have config.proxy.http = "" and config.proxy.https = "" and config.proxy.no_proxy = ENV["NO_PROXY"] and ENV["NO_PROXY"] = "*.example.com" then this plugin thinks it should configure all service providers that it supports no_proxy configuration values to *.example.com which to me feels wrong.

I'm just trying to decide what the true and easiest course of action(s) should be without writing any more crazy logic conditions about how to disable this plugin. In my humble opinion I feel like the current conditional logic for disabling this plugin is way to complex and is in need of a refactor but it would take a lot of time to truly fix.

I'm in the process as I have spare time to try and to put some small proxyconf environments together to replicate and reproduce this behavior and commit them to this repo so that we can reference them inside issues like this. It's pretty rough draft right now, but I feel it might be necessary since this plugin does not have traditional integration or functional integration tests, it only has simulated mock style type tests which only go so far. I'll respond with commit message when I add them to this repo.

haxorof commented 5 years ago

Aha. I did not thing think that it enables the proxy settings even if config.proxy.enabled = false. That for me would really be unexpected behavior. In my mind I just thought you have the "big switch which is config.proxy.enable and after that point it will use the environment variables and configuration defined in the documentation. Maybe a little to naive thinking. 😄

codylane commented 5 years ago

Hey @haxorof - Sorry for the delay getting back to you. I believe I've got a fix in place for the issue reported. Indeed it was a bug in the implementation logic as described above.

I should be committing the fix shortly. I've created a new issue on github #189 that links related docker issues. I'll link fixing commits to that issue.

The scenario you reported:

config.proxy.http = ""
config.proxy.https = ""
config.proxy.no_proxy = "localhost,127.0.0.1,.localdomain"

Now configures /etc/docker/config.json thus disabling the http and https proxy settings both docker and system wide.

{
  "proxies": {
    "default": {
      "noProxy": "localhost,127.0.0.1,.localdomain"
    }
  }
codylane commented 5 years ago

2.0.1 has been released. Please re-open if this is still an issue.