tools/lxdclient: use utils/proxy.DefaultConfig #7014

Merged
merged 1 commit into from Feb 21, 2017

Conversation

Projects
None yet
3 participants
Member

axw commented Feb 21, 2017

Description of change

Use the Juju-managed HTTP proxy configuration
when making LXD client connections, rather than
the default proxy handler that looks at the
environment variables once and only once.

As well as this, initialise the proxy config
for the client, so that $http_proxy and co. will
be picked up by the client.

QA steps

(Address of LXD host on lxdbr0 is 10.250.243.1)

  1. run an HTTP proxy, port 8080, on the LXD host; the proxy should reject attempts to proxy through to 10.250.243.1 (i.e. so if no_proxy doesn't include 10.250.243.1, the whole thing fails)
  2. export no_proxy=10.250.243.1 http_proxy=http://10.250.243.1:8080 https_proxy=http://10.250.243.1:8080
  3. create /tmp/config.yaml with the following contents:
apt-http-proxy: http://10.250.243.1:8080
apt-https-proxy: http://10.250.243.1:8080
http-proxy: http://10.250.243.1:8080
https-proxy: http://10.250.243.1:8080
no-proxy: 127.0.0.1,localhost,10.250.243.1
  1. juju bootstrap localhost --config /tmp/config.yaml. As soon as the bootstrap container machine starts, lxc exec bash to it, and run the following:
ufw default deny outgoing
ufw default allow incoming
ufw allow out to 10.250.243.1
ufw enable
  1. juju add-machine
  2. ensure there's no "juju/trusty/amd64" alias: lxc image alias delete juju/trusty/amd64 if necessary
  3. juju add-machine --series=trusty

The two machines should be added successfully.

Documentation changes

None.

Bug reference

Fixes https://bugs.launchpad.net/juju/+bug/1642385

Member

axw commented Feb 21, 2017

P.S. here's the proxy program I used for testing:

package main

import (
        "log"
        "net"
        "net/http"

        "github.com/elazarl/goproxy"
)

func main() {
        proxy := goproxy.NewProxyHttpServer()
        proxy.Verbose = true
        proxy.OnRequest().HandleConnectFunc(func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) {
                if addr, _, _ := net.SplitHostPort(host); addr == "10.250.243.1" {
                        return goproxy.RejectConnect, host
                }
                return goproxy.OkConnect, host
        })
        log.Fatal(http.ListenAndServe(":8080", proxy))
}

This feels like something where we need a more concrete upstream fix. Either a shared library that LXD and Juju use, or a patch towards Go's core library that can at least have a way to trigger "reread the ENV variables because I just set them."

I can live with this, though monkey-patching the internal transport of a client always feels like you're a bit lucky they exposed it at all.

tools/lxdclient/client.go
@@ -262,6 +264,12 @@ func newRawClient(remote Remote) (*lxd.Client, error) {
return nil, errors.Trace(err)
}
+ // Replace the proxy handler with the one managed
+ // by Juju's worker/proxyupdater.
+ if tr, ok := client.Http.Transport.(*http.Transport); ok && tr.Proxy != nil {
@jameinel

jameinel Feb 21, 2017

Owner

While this is a bit ugly, if we're going to do it, shouldn't we be universally setting tr.Proxy and not just setting it if there is already a proxy? (from what I can tell, its probably always setting ProxyFromEnvironment, which also does the read-only-once stuff.)

@axw

axw Feb 21, 2017

Member

Yes, it does always do that. I've made it unconditional though, in case LXD changes that.

tools/lxdclient: use utils/proxy.DefaultConfig
Use the Juju-managed HTTP proxy configuration
when making LXD client connections, rather than
the default proxy handler that looks at the
environment variables once and only once.

As well as this, initialise the proxy config
for the client, so that $http_proxy and co. will
be picked up by the client.

Fixes https://bugs.launchpad.net/juju/+bug/1642385
Member

axw commented Feb 21, 2017

I can live with this, though monkey-patching the internal transport of a client always feels like you're a bit lucky they exposed it at all.

Indeed. We have a test that should protect us, but it would be good to have a statement of support for this in the LXD client docs.

Member

axw commented Feb 21, 2017

$$merge$$

Contributor

jujubot commented Feb 21, 2017

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

@jujubot jujubot merged commit 1d44d9c into juju:2.1 Feb 21, 2017

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