/juju

Don't rely on environment vars for proxy settings #6974

Merged
merged 4 commits into from Feb 14, 2017

Conversation

Reviewers

@mjs mjs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@babbageclunk @jujubot @mjs
@babbageclunk
Member

babbageclunk commented Feb 14, 2017 •
Edited 1 time

Description of change

At the moment we update environment variables with the proxy settings,
intending for them to apply to web requests made from the running
process (like charm downloads). Unfortunately the built-in
implementation that reads proxy info from the environment only reads the
variables once - if a request has already been made the variable will
have been read and subsequent changes won't be seen.

Instead, store proxy settings in a package variable and configure the process's
default transport so it will use the values from there. Change the proxy updater
to update those config settings as well as the other places it currently sets - we
want any processes started by jujud to still get the right values in their env.

The proxy selection code was adapted from the stdlib function
net/http.ProxyFromEnvironment.

QA steps

  • Run a proxy (like tinyproxy) and check that you can see requests that it handles.
  • Bootstrap a controller and add a model.
  • Deploy a charm to the model - the charm download shouldn't go via the proxy.
  • Update the controller proxy settings to point to the proxy.
  • Deploy another different charm that uses resources (like mattermost) - the download of the charm and resources should go via the proxy.

Bug reference

Fixes https://bugs.launchpad.net/juju/2.1/+bug/1654591

babbageclunk added some commits Feb 13, 2017

@babbageclunk
Add InProcessUpdate to proxyupdateworker 
At the moment we update environment variables with the proxy settings,
intending for them to apply to web requests made from the running
process (like charm downloads). Unfortunately the built-in
implementation that reads proxy info from the environment only reads the
variables once - if a request has already been made the variable will
have been read and subsequent changes won't be seen.

We still want the variables to be set in the environment so they'll be
right in processes that get started from this one, but for the
in-process requests we use a different default transport. This will get
the proxy settings from a package variable instead of env vars. The
function passed into the proxy updater will set this package variable.
339af23
@babbageclunk
Implement util.proxy.ProxyConfig 
This stores default proxy settings in a package variable. The manifolds
that run the proxyupdater now pass the default config's Set function to
the worker so that it will keep the proxy config
up-to-date. InstallInDefaultTransport sets the default http transport's
proxy resolution method to use these config settings (with logic adapted
from the stdlib net/http.ProxyFromEnvironment function).
c11aaed
@babbageclunk
Install the default proxy config from jujud main
11a4abb
@babbageclunk
Member

babbageclunk commented Feb 14, 2017

!!chittychitty!!
@mjs

mjs approved these changes Feb 14, 2017
View changes

Good stuff.

utils/proxy/proxyconfig.go
+}
+
+// useProxy reports whether requests to addr should use a proxy,
+// according to the NO_PROXY or no_proxy environment variable.
@mjs

mjs Feb 14, 2017

Contributor

this isn't quite right, those environment variables don't come into it here

@babbageclunk

babbageclunk Feb 14, 2017

Member

Fixed

utils/proxy/proxyconfig.go
+
+// useProxy reports whether requests to addr should use a proxy,
+// according to the NO_PROXY or no_proxy environment variable.
+// addr is always a canonicalAddr with a host and port.
@mjs

mjs Feb 14, 2017

Contributor

Maybe mention that this is copied/adapted too (and where from)

@babbageclunk

babbageclunk Feb 14, 2017

Member

Yup

utils/proxy/proxyconfig.go
+func canonicalAddr(url *url.URL) string {
+ addr := url.Host
+ if !hasPort(addr) {
+ return addr + ":" + portMap[url.Scheme]
@mjs

mjs Feb 14, 2017

Contributor

This is technically wrong (in the stdlib too!). It generates invalid IPv6 address:port strings (we've screwed this up before). It looks like the code gets away with it because SplitHostPort allows the badly formatted values.

Use https://golang.org/pkg/net/#JoinHostPort instead

I see it's fixed upstream: https://go.googlesource.com/go/+/master/src/net/http/transport.go#1990

worker/proxyupdater/proxyupdater_test.go
+ case inProcSettings = <-s.inProcSettings:
+ if c.Check(inProcSettings, gc.Equals, expected) {
+ gotInProc = true
+ }
case <-time.After(10 * time.Millisecond):
@mjs

mjs Feb 14, 2017

Contributor

This should probably be ShortWait

@babbageclunk

babbageclunk Feb 14, 2017

Member

Fixed

@babbageclunk
Member

babbageclunk commented Feb 14, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Feb 14, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 541a664 into juju:2.1 Feb 14, 2017

@babbageclunk babbageclunk deleted the babbageclunk:proxy-settings branch Feb 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment