Add --no-proxy option by wjun · Pull Request #8201 · vmware/vic

wjun · 2018-08-10T06:44:12Z

The previous vic-machine proxy configuration behaviour has an issue in that http and https settings will overwrite each other if we only configure one each time. This issue has been fixed as well.

Fixes #8144

zjs · 2018-08-10T18:56:57Z

cmd/vic-machine/common/proxy.go

+		}
 	}
+
+	noproxy = p.NoProxy


It seems like we should validate this user input and add unit tests for that validation logic.

We may also need unit or integration tests to ensure that we handle edge cases properly, like a user specifying a --no-proxy setting without specifying either proxy.

The snippet below is how Golang http/transport process no_proxy value, and it is kind of fault tolerant.

for _, p := range strings.Split(no_proxy, ",") { p = strings.ToLower(strings.TrimSpace(p)) if len(p) == 0 { continue } if hasPort(p) { p = p[:strings.LastIndex(p, ":")] } if addr == p { return false } if len(p) == 0 { // There is no host part, likely the entry is malformed; ignore. continue } if p[0] == '.' && (strings.HasSuffix(addr, p) || addr == p[1:]) { // no_proxy ".foo.com" matches "bar.foo.com" or "foo.com" return false } if p[0] != '.' && strings.HasSuffix(addr, p) && addr[len(addr)-len(p)-1] == '.' { // no_proxy "foo.com" matches "bar.foo.com" return false } }

@wjun I think what @zjs is talking about is less the edge case parsing of the contents of the no-proxy option, but whether the no-proxy option is valid at all. If the user has not specified any proxies then setting no-proxy is likely a mistake.

I understand the point, but they are not necessarily set together, as we do when setting them through env variables. I also wonder if we can remove the field of proxies.IsSet field which looks redundant.

zjs · 2018-08-10T18:58:55Z

cmd/vic-machine/configure/configure.go

-		updateSessionEnv(vicAdminSession, config.VICAdminHTTPSProxy, sProxy)
+	}
+	nProxy := ""
+	if c.NoProxy != nil {


Since proxy exclusions don't make sense if proxies haven't been configured, it seems like this should be nested under the c.proxies.IsSet block.

I thought maybe we can align with other no_proxy settings which are not only for http protocol but also for other protocols, though we only use http protocols for now.

zjs · 2018-08-10T19:01:50Z

cmd/vic-machine/configure/configure.go

 		hProxy := ""
 		if c.HTTPProxy != nil {
 			hProxy = c.HTTPProxy.String()
+			updateSessionEnv(personaSession, config.GeneralHTTPProxy, hProxy)


This restructuring fixes one issue as noted in the commit message, but looks like it may introduce another: it no longer seems possible to unset a previously-configured proxy.

Good catch. Fixed the regression.

zjs · 2018-08-10T19:04:03Z

lib/apiservers/service/swagger.json

                  "format": "uri"
+                },
+                "no_proxy": {
+                  "type": "string"


It seems like this should take an array of URIs; we use "rich" data types in the API even if they're not feasible for the CLI. (This will require some changes to the API handler logic.)

According to the snippet above, no_proxy includes not only URIs, but also domain names.

If I recall correctly a URI should be able to encode a protocol-relative location (e.g //f.q.d.n) - I'd suggest seeing whether the url package encodes correctly when omitting the protocol (a sanity check in playground says it does). I believe you already test this in the unit test, but maybe not with the actual protocol relative form.

Regardless the data in this option is expected to be an array of valid URIs so we should define it as such.

Right, URIs can parse '*' or domain names too. I just updated the type to URI array.

hickeng · 2018-08-14T04:52:44Z

lib/apiservers/service/swagger.json

                  "format": "uri"
+                },
+                "no_proxy": {
+                  "type": "string"


If I recall correctly a URI should be able to encode a protocol-relative location (e.g //f.q.d.n) - I'd suggest seeing whether the url package encodes correctly when omitting the protocol (a sanity check in playground says it does). I believe you already test this in the unit test, but maybe not with the actual protocol relative form.

Regardless the data in this option is expected to be an array of valid URIs so we should define it as such.

hickeng · 2018-08-14T04:53:52Z

cmd/vic-machine/common/proxy.go

+		}
 	}
+
+	noproxy = p.NoProxy


@wjun I think what @zjs is talking about is less the edge case parsing of the contents of the no-proxy option, but whether the no-proxy option is valid at all. If the user has not specified any proxies then setting no-proxy is likely a mistake.

hickeng · 2018-08-14T05:04:14Z

lib/install/validate/config_to_data.go

 	}
 	for _, env := range persona.Cmd.Env {
-		if !strings.HasPrefix(env, httpProxy) && !strings.HasPrefix(env, httpsProxy) {
+		if !strings.HasPrefix(env, httpProxy) && !strings.HasPrefix(env, httpsProxy) && !strings.HasPrefix(env, noProxy) {


This is likely much neater if using switch - something like:

for _, env := range persona.Cmd.Env { val := strings.Split(env, "=") switch { case strings.HasPrefix(env, httpProxy): ... case strings.HasPrefix(env, httpsProxy): ... case strings.HasPrefix(env, noProxy): ... }

Good suggestion. Just refactored to use switch.

zjs

We should also ensure the API change is covered by an integration test.

zjs · 2018-08-17T17:18:35Z

cmd/vic-machine/common/proxy_test.go

-			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")
+				_, _, nproxy, err := gproxy.ProcessProxies()
+				assert.NoError(t, err, "Expected %s, %s, %s and %s to be accepted", ghttp, ghttps, uri, nproxy)


It seems like this will print the --no-proxy values twice: once from uri and once from nproxy.

I added uri and nproxy for comparison. nproxy will trim whitespaces of each uri.

zjs · 2018-08-17T17:19:14Z

cmd/vic-machine/common/proxy_test.go

 	}

+	gnproxies := [...]string{
+		"*",


Is this actually a valid value? If so, perhaps it should be mentioned in the help text (which currently says "This should be a comma-separated list of hostnames, domain names, or a mixture of both"),

Good catch. Just updated the information.

zjs · 2018-08-17T17:21:11Z

cmd/vic-machine/common/proxy_test.go


+	gnproxies := [...]string{
+		"*",
+		"localhost, .example.com",


Maybe we should also include a test case without a space. (E.g., "localhost,.example.com")

zjs · 2018-08-17T17:22:55Z

cmd/vic-machine/common/proxy.go

+		cli.GenericFlag{
+			Name:   "no-proxy",
+			Value:  flags.NewOptionalString(&p.NoProxy),
+			Usage:  "URLs that should be excluded from proxying. This should be a comma-separated list of hostnames, domain names, or a mixture of both",


In the example below, you have ".example.com". Presumably this means example.com and all sub-domains. It's not clear from this help output that this would be accepted. Perhaps @stuclem can suggest a concise way to explain this.

zjs · 2018-08-17T17:30:24Z

lib/apiservers/service/restapi/handlers/decode/networking.go

 func FromImageFetchProxy(p *models.VCHRegistryImageFetchProxy) common.Proxies {
 	http := string(p.HTTP)
 	https := string(p.HTTPS)
+	var nproxy *string


To be consistent with the other functions in this package (where each function converts one object), this should probably be extracted into its own function (e.g., fromProxyExclusionList).

noproxy is defined as one field of VCHRegistryImageFetchProxy, so I process it here.

zjs · 2018-08-17T17:31:22Z

lib/apiservers/service/restapi/handlers/decode/networking.go

+			buffer.WriteString(",")
+			buffer.WriteString(string(v))
+		}
+		nproxyStr := buffer.String()


I think this could be more clearly written as strings.Join(p.NoProxy, ",")

I tried initially, but looks p.NoProxy is []strfmt.URL type, and strings.Join does not accept it?

zjs · 2018-08-17T17:33:18Z

lib/apiservers/service/restapi/handlers/encode/networking.go

 		if strings.HasPrefix(env, https+"=") {
 			httpsProxy = strfmt.URI(strings.SplitN(env, "=", 2)[1])
 		}
+		if strings.HasPrefix(env, nproxy+"=") {


Like config_to_data.go (#8201 (comment)), I think this would be clearer if it used a switch.

Good catch. Done.

zjs · 2018-08-17T17:34:01Z

lib/apiservers/service/restapi/handlers/encode/networking.go

+			nProxyStrs := strings.Split(strings.SplitN(env, "=", 2)[1], ",")
+			nProxy = make([]strfmt.URI, len(nProxyStrs))
+			for i := range nProxy {
+				nProxy[i] = strfmt.URI(strings.TrimSpace(nProxyStrs[i]))


To be consistent with the other functions in this package (where each function converts one object), this should probably be extracted into its own function (e.g., asProxyExclusionList).

zjs · 2018-08-17T17:35:40Z

lib/config/virtual_container_host.go

+	GeneralNoProxy     = "NO_PROXY"
 	VICAdminHTTPProxy  = "VICADMIN_HTTP_PROXY"
 	VICAdminHTTPSProxy = "VICADMIN_HTTPS_PROXY"
+	VICAdminNoProxy    = "NO_PROXY"


This seems inconsistent with VICAdminHTTPProxy and VICAdminHTTPSProxy. Should this be VICADMIN_NO_PROXY?

NO_PROXY is used inside http.transport, so I need to put that env name

vic/lib/vicadmin/validate.go

Line 202 in 2119c41

client.Transport = &http.Transport{Proxy: http.ProxyURL(url)}

wjun · 2018-08-18T12:51:06Z

We should also ensure the API change is covered by an integration test.
It is covered in vch_create_test.go.

(cherry picked from commit 45a545a) Conflicts: lib/apiservers/service/restapi/handlers/decode/networking.go lib/apiservers/service/restapi/handlers/encode/networking.go lib/apiservers/service/restapi/handlers/vch_get.go

(cherry picked from commit 45a545a)

(cherry picked from commit 45a545a) Conflicts: deleted by us: lib/apiservers/service/restapi/handlers/decode/networking.go deleted by us: lib/apiservers/service/restapi/handlers/encode/networking.go both modified: lib/apiservers/service/restapi/handlers/vch_get.go modified: lib/apiservers/service/restapi/handlers/vch_create.go

wjun requested a review from a team as a code owner August 10, 2018 06:44

vmwclabot added the cla-not-required label Aug 10, 2018

wjun requested review from hickeng and zjs August 10, 2018 06:47

wjun force-pushed the proxy branch 3 times, most recently from 710ec58 to c3b156c Compare August 10, 2018 14:25

zjs requested changes Aug 10, 2018

View reviewed changes

wjun force-pushed the proxy branch from 88157a7 to 99c9e00 Compare August 13, 2018 03:57

hickeng reviewed Aug 14, 2018

View reviewed changes

wjun force-pushed the proxy branch 3 times, most recently from afeffe8 to c1fb11e Compare August 15, 2018 07:18

zjs requested changes Aug 17, 2018

View reviewed changes

wjun force-pushed the proxy branch from c1fb11e to 795571d Compare August 18, 2018 12:40

zjs approved these changes Aug 27, 2018

View reviewed changes

zjs assigned wjun Aug 28, 2018

Add --no-proxy option

e554bf0

wjun force-pushed the proxy branch from 795571d to e554bf0 Compare August 31, 2018 06:52

DanielXiao approved these changes Aug 31, 2018

View reviewed changes

wjun merged commit 45a545a into vmware:master Aug 31, 2018

wjun added a commit to wjun/vic that referenced this pull request Sep 3, 2018

Add --no-proxy option (vmware#8201)

964faaf

(cherry picked from commit 45a545a)

Conversation

wjun commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjun Aug 12, 2018 • edited by hickeng Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjun Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hickeng Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hickeng Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zjs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjun commented Aug 18, 2018

wjun commented Aug 10, 2018 •

edited

Loading

wjun Aug 12, 2018 •

edited by hickeng

Loading

wjun Aug 14, 2018 •

edited

Loading

hickeng Aug 14, 2018 •

edited

Loading

hickeng Aug 14, 2018 •

edited

Loading