loose http_proxy and https_proxy check

[wjun]

Fixes #6767

[zjs]

A few comments below. Also, could you update the commit message to be more useful? (Some advice here.)

[zjs]

As this validation logic is the same as above, can we extract it into a private utility method to avoid duplication?

Having the validation logic in a private method would also allow a unit test to ensure that it behaves as expected.

[zjs]

hproxy.Scheme != "http" || hproxy.Scheme != "https"

I don't think this is going to achieve the intended result. (Any string will either not be "http" or not be "https", so we'll always throw an error.)

Do you mean (hproxy.Scheme != "http" && hproxy.Scheme != "https")?

[zjs]

It seems like we should update these to be positive tests that you can configure "mismatched" proxies instead of removing them entirely.

[wjun]

Good point. I add them into another test case where we test http(s) proxy settings.

[hickeng]

I'd prefer to have the proxy pulled from the environment as this test will fail if not on the vmware internal network.
I know the test is already flawed in that regard but lets fix it up while we're making improvements.

I'm interested in moving to a situation where the test logic is independent of the infrastructure it runs on. While the bulk of that is around abstracting the topology of vSphere environments it should also include the direct manipulation of vSphere elements in tests and the infrastructure specific URIs so as this proxy server.

[wjun]

As far as I understand, these cases only configure proxy settings but not use them, so any correct proxy urls will be OK.

[hickeng]

You're correct. Which suggests we should have an additional test that makes sure that we actually use the configured proxy value. That's going to be a little more involved as it would require deployment of a proxy (could be a container in a different VCH if we want to deploy as part of the test) and checking proxy logs to ensure that traffic was routed through it.

Please open a follow up issue for the functional test (as opposed to this configuration test) if that's too involved for this PR.

[wjun]

According to #8144, proxy settings are used as environment variables, so that is the very common usage of http proxies.

[hickeng]

Your're correct. If/when we add configuration for per destination proxy configurations it will become much more involved, but for now the port layer does not use the proxy configuration so we can safely assume normal Linux proxy environment behaviour.

[hickeng]

Can we have a unit test that ensures the validation is successful for the various cases?
We do not have a test for omitting the scheme currently and I recall there was a url.Parse problem if scheme was omitted - the host portion could end up as the scheme.

I suggest testing at a minimum:

https://fully.qualified.domain.name
https://fully.qualified.domain.name:443
http://fully.qualified.domain.name
http://fully.qualified.domain.name:80
fully.qualified.domain.name
fully.qualified.domain.name:80
raw-host
raw-host:80

[wjun]

Good point. I will add one. Please note our current format is http(s)://hostname, so we will get OK for the first 4 urls above, and failure for the last 4 urls.

My review comments have been addressed.

[hickeng]

Approved conditional on follow up issue for verification of proxy use and investigation of the MITM proxy test failure in https://ci-vic.vmware.com/vmware/vic/19687/7

[hickeng]

You're correct. Which suggests we should have an additional test that makes sure that we actually use the configured proxy value. That's going to be a little more involved as it would require deployment of a proxy (could be a container in a different VCH if we want to deploy as part of the test) and checking proxy logs to ensure that traffic was routed through it.

Please open a follow up issue for the functional test (as opposed to this configuration test) if that's too involved for this PR.

[zjs]

Can we put the good urls and bad urls into separate slices so that we can test combinations where HTTPProxy != HTTPSProxy? Something like this would also allow us to check combinations more combinations, including combinations of valid & invalid proxies.

This also makes it easier to add new URLs without needing to update the logic later in the test.

I think it also makes sense to replace the example domains suggested by George with domain names that are reserved for such use, to avoid using potentially valid domains that we do not own. (For IP addresses, I refer to subnets reserved in RFC 3849 and RFC 5737.)

Lastly, I think it's helpful to use the optional msg arguments to ensure that a helpful message is printed if the test fails. (Debugging failures in CI can be annoying if all that's logged is something like true != false.)

Example of what I mean:

	gurls := [...]string{
		"https://fully.qualified.example.com",
		"https://fully.qualified.example.com:443",
		"http://fully.qualified.example.com",
		"http://fully.qualified.example.com:80", 
		"http://203.0.113.123", 
		"http://[2001:DB8:0123::]", 
	}

	burls := [...]string{
		"example.com",
		"example.com:80",
		"localhost",
		"localhost:80",
		"ftp://example.com",
		"httpd://example.com",
	}

	for _, ghttp := range gproxies {
		for _, ghttps := range gproxies {
			gproxy := Proxies{HTTPProxy: &ghttp, HTTPSProxy: &ghttps}

			_, _, err := gproxy.ProcessProxies()
			assert.NoError(t, err, "Expected %s and %s to be accepted", ghttp, ghttps)
			assert.True(t, gproxy.IsSet, "Expected proxy to be marked as set")
		}
 	} 

	for _, ghttp := range gproxies {
		for _, bhttps := range gproxies {
			bproxy := Proxies{HTTPProxy: &ghttp, HTTPSProxy: &bhttps}

			_, _, err := bproxy.ProcessProxies()
			assert.Error(t, err, "Expected %s to be rejected", bhttps)
		}
 	} 

	for _, bhttp := range gproxies {
		for _, ghttps := range gproxies {
			bproxy := Proxies{HTTPProxy: &bhttp, HTTPSProxy: &ghttps}

			_, _, err := bproxy.ProcessProxies()
			assert.Error(t, err, "Expected %s to be rejected", bhttp)
		}
 	} 

	for _, bhttp := range gproxies {
		for _, bhttps := range gproxies {
			bproxy := Proxies{HTTPProxy: &bhttp, HTTPSProxy: &bhttps}

			_, _, err := bproxy.ProcessProxies()
			assert.Error(t, err, "Expected %s and %s to be rejected", bhttp, bhttps)
		}
 	} 

[wjun]

Thanks for the suggestions. Initially, I thought most users will set http/https proxy to the same proxy server, and this refactoring definitely covers all cases and easier to extend.

[hickeng]

Your're correct. If/when we add configuration for per destination proxy configurations it will become much more involved, but for now the port layer does not use the proxy configuration so we can safely assume normal Linux proxy environment behaviour.