Merge pull request #5871 from dimitern/lp-1575676-lxd-bridge

Fixes lp:1575676: Detect LXD default profile's bridge name and use it

Changed tools/lxdclient code not to assume the default LXD bridge name
is always "lxdbr0". Instead, the "default" profile's bridge device name
is determined and used in Client.

See http://pad.lv/1575676 for details.

QA steps to verify the fix:

1. LXD provider (where it appears):
```
   $ sudo dpkg-reconfigure -p medium lxd
   # rename the lxdbr0 to something else, keep the rest the same.
   $ juju bootstrap --upload-tools lxd localhost
   # expect success, before this fix it failed to complete due to lxdbr0
   # not existing. 
   $ juju deploy ubuntu # also OK
```

2. Manual provider (for completeness):
```
   $ lxc launch ubuntu:xenial man-x1
   $ lxc info man-x1 | grep eth0    # find eth0's address, e.g. 10.0.1.2
   $ lxc file push ~/.ssh/<ssh-key.pub> man-x1/home/ubuntu/.ssh/authorized_keys
   # to allow the below to work
   $ ssh ubuntu@10.0.1.2            # should work now
   $ juju bootstrap --upload-tools man manual/10.0.1.2
```

tools/lxdclient/client.go

@@ -16,11 +16,12 @@ import (
 	"strings"
 	"syscall"
 
+	"github.com/juju/errors"
+	"github.com/juju/loggo"
 	"github.com/lxc/lxd"
 	lxdshared "github.com/lxc/lxd/shared"
 
-	"github.com/juju/errors"
-	"github.com/juju/loggo"
+	"github.com/juju/juju/network"
 )
 
 var logger = loggo.GetLogger("juju.tools.lxdclient")
@@ -96,34 +97,51 @@ type Client struct {
 	*profileClient
 	*instanceClient
 	*imageClient
-	baseURL string
+	baseURL                  string
+	defaultProfileBridgeName string
 }
 
 func (c Client) String() string {
 	return fmt.Sprintf("Client(%s)", c.baseURL)
 }
 
+func (c Client) DefaultProfileBridgeName() string {
+	return c.defaultProfileBridgeName
+}
+
 // Connect opens an API connection to LXD and returns a high-level
 // Client wrapper around that connection.
 func Connect(cfg Config) (*Client, error) {
 	if err := cfg.Validate(); err != nil {
 		return nil, errors.Trace(err)
 	}
 
-	remote := cfg.Remote.ID()
+	remoteID := cfg.Remote.ID()
 
 	raw, err := newRawClient(cfg.Remote)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
 
+	// If this is the LXD provider on the localhost, let's do an extra check to
+	// make sure the default profile has a correctly configured bridge, and
+	// which one is it.
+	var bridgeName string
+	if remoteID == remoteIDForLocal {
+		bridgeName, err = verifyDefaultProfileBridgeConfig(raw)
+		if err != nil {
+			return nil, errors.Trace(err)
+		}
+	}
+
 	conn := &Client{
-		serverConfigClient: &serverConfigClient{raw},
-		certClient:         &certClient{raw},
-		profileClient:      &profileClient{raw},
-		instanceClient:     &instanceClient{raw, remote},
-		imageClient:        &imageClient{raw, connectToRaw},
-		baseURL:            raw.BaseURL,
+		serverConfigClient:       &serverConfigClient{raw},
+		certClient:               &certClient{raw},
+		profileClient:            &profileClient{raw},
+		instanceClient:           &instanceClient{raw, remoteID},
+		imageClient:              &imageClient{raw, connectToRaw},
+		baseURL:                  raw.BaseURL,
+		defaultProfileBridgeName: bridgeName,
 	}
 	return conn, nil
 }
@@ -169,7 +187,6 @@ func isSupportedLxdVersion(version string) bool {
 // newRawClient connects to the LXD host that is defined in Config.
 func newRawClient(remote Remote) (*lxd.Client, error) {
 	host := remote.Host
-	logger.Debugf("connecting to LXD remote %q: %q", remote.ID(), host)
 
 	if remote.ID() == remoteIDForLocal && host == "" {
 		host = "unix://" + lxdshared.VarPath("unix.socket")
@@ -180,6 +197,7 @@ func newRawClient(remote Remote) (*lxd.Client, error) {
 			host = net.JoinHostPort(host, lxdshared.DefaultPort)
 		}
 	}
+	logger.Debugf("connecting to LXD remote %q: %q", remote.ID(), host)
 
 	clientCert := ""
 	if remote.Cert != nil && remote.Cert.CertPEM != nil {
@@ -229,39 +247,63 @@ func newRawClient(remote Remote) (*lxd.Client, error) {
 		}
 	}
 
-	/* If this is the LXD provider on the localhost, let's do an extra
-	 * check to make sure that lxdbr0 is configured.
-	 */
-	if remote.ID() == remoteIDForLocal {
-		profile, err := client.ProfileConfig("default")
-		if err != nil {
-			return nil, errors.Trace(err)
-		}
+	return client, nil
+}
 
-		/* If the default profile doesn't have eth0 in it, then the
-		 * user has messed with it, so let's just use whatever they set up.
-		 *
-		 * Otherwise, if it looks like it's pointing at our lxdbr0,
-		 * let's check and make sure that is configured.
-		 */
-		eth0, ok := profile.Devices["eth0"]
-		if ok && eth0["type"] == "nic" && eth0["nictype"] == "bridged" && eth0["parent"] == "lxdbr0" {
-			conf, err := ioutil.ReadFile(LXDBridgeFile)
-			if err != nil {
-				if !os.IsNotExist(err) {
-					return nil, errors.Trace(err)
-				} else {
-					return nil, bridgeConfigError("lxdbr0 configured but no config file found at " + LXDBridgeFile)
-				}
-			}
+// verifyDefaultProfileBridgeConfig takes a LXD API client and extracts the
+// network bridge configured on the "default" profile. Additionally, if the
+// default bridge bridge is used, its configuration in LXDBridgeFile is also
+// inspected to make sure it has a chance to work.
+func verifyDefaultProfileBridgeConfig(client *lxd.Client) (string, error) {
+	const (
+		defaultProfileName = "default"
+		configTypeKey      = "type"
+		configTypeNic      = "nic"
+		configNicTypeKey   = "nictype"
+		configBridged      = "bridged"
+		configEth0         = "eth0"
+		configParentKey    = "parent"
+	)
+
+	config, err := client.ProfileConfig(defaultProfileName)
+	if err != nil {
+		return "", errors.Trace(err)
+	}
 
-			if err = checkLXDBridgeConfiguration(string(conf)); err != nil {
-				return nil, errors.Trace(err)
-			}
-		}
+	// If the default profile doesn't have eth0 in it, then the user has messed
+	// with it, so let's just use whatever they set up.
+	eth0, ok := config.Devices[configEth0]
+	if !ok {
+		return "", errors.Errorf("unexpected LXD %q profile config without eth0: %+v", defaultProfileName, config)
 	}
 
-	return client, nil
+	// If eth0 is there, but not with the expected attributes, likewise fail
+	// early.
+	if eth0[configTypeKey] != configTypeNic || eth0[configNicTypeKey] != configBridged {
+		return "", errors.Errorf("unexpected LXD %q profile config: %+v", defaultProfileName, config)
+	}
+
+	bridgeName := eth0[configParentKey]
+	logger.Infof(`LXD "default" profile uses network bridge %q`, bridgeName)
+
+	if bridgeName != network.DefaultLXDBridge {
+		// When the user changed which bridge to use, just return its name and
+		// check no further.
+		return bridgeName, nil
+	}
+
+	bridgeConfig, err := ioutil.ReadFile(LXDBridgeFile)
+	if os.IsNotExist(err) {
+		return "", bridgeConfigError("lxdbr0 configured but no config file found at " + LXDBridgeFile)
+	} else if err != nil {
+		return "", errors.Trace(err)
+	}
+
+	if err := checkLXDBridgeConfiguration(string(bridgeConfig)); err != nil {
+		return "", errors.Trace(err)
+	}
+
+	return bridgeName, nil
 }
 
 func bridgeConfigError(err string) error {
@@ -292,7 +334,7 @@ func checkLXDBridgeConfiguration(conf string) error {
 			 * because the default profile that juju will use says
 			 * lxdbr0. So let's fail if it doesn't.
 			 */
-			if name != "lxdbr0" {
+			if name != network.DefaultLXDBridge {
 				return bridgeConfigError(fmt.Sprintf(LXDBridgeFile+" has a bridge named %s, not lxdbr0", name))
 			}
 		} else if strings.HasPrefix(line, "LXD_IPV4_ADDR=") || strings.HasPrefix(line, "LXD_IPV6_ADDR=") {

tools/lxdclient/client_profile.go

@@ -13,7 +13,8 @@ import (
 type rawProfileClient interface {
 	ProfileCreate(name string) error
 	ListProfiles() ([]string, error)
-	SetProfileConfigItem(name, key, value string) error
+	SetProfileConfigItem(profile, key, value string) error
+	GetProfileConfig(profile string) (map[string]string, error)
 	ProfileDelete(profile string) error
 	ProfileDeviceAdd(profile, devname, devtype string, props []string) (*lxd.Response, error)
 }
@@ -70,3 +71,21 @@ func (p profileClient) HasProfile(name string) (bool, error) {
 	}
 	return false, nil
 }
+
+// SetProfileConfigItem updates the given profile config key to the given value.
+func (p profileClient) SetProfileConfigItem(profile, key, value string) error {
+	if err := p.raw.SetProfileConfigItem(profile, key, value); err != nil {
+		return errors.Trace(err)
+	}
+	return nil
+}
+
+// GetProfileConfig returns a map with all keys and values for the given
+// profile.
+func (p profileClient) GetProfileConfig(profile string) (map[string]string, error) {
+	config, err := p.raw.GetProfileConfig(profile)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	return config, nil
+}

tools/lxdclient/config.go

@@ -58,23 +58,19 @@ func (cfg Config) UsingTCPRemote() (Config, error) {
 		return cfg, errors.Trace(err)
 	}
 
-	/* UsingTCP will try to figure out the network name of the lxdbr0,
-	 * which means that lxdbr0 needs to be up. If this lxd has never had
-	 * anything done to it, it hasn't been socket activated yet, and lxdbr0
-	 * won't exist. So, we rely on this poke to get lxdbr0 started.
-	 */
-	_, err = client.ServerStatus()
-	if err != nil {
+	if _, err := client.ServerStatus(); err != nil {
 		return cfg, errors.Trace(err)
 	}
 
-	remote, err := cfg.Remote.UsingTCP()
+	// If the default profile's bridge was never used before, the next call with
+	// also activate it and get its address.
+	remote, err := cfg.Remote.UsingTCP(client.defaultProfileBridgeName)
 	if err != nil {
 		return cfg, errors.Trace(err)
 	}
 
 	// Update the server config and authorized certs.
-	serverCert, err := prepareRemote(client, *remote.Cert)
+	serverCert, err := prepareRemote(client, remote.Cert)
 	if err != nil {
 		return cfg, errors.Trace(err)
 	}
@@ -88,7 +84,7 @@ func (cfg Config) UsingTCPRemote() (Config, error) {
 	return cfg, nil
 }
 
-func prepareRemote(client *Client, newCert Cert) (string, error) {
+func prepareRemote(client *Client, newCert *Cert) (string, error) {
 	// Make sure the LXD service is configured to listen to local https
 	// requests, rather than only via the Unix socket.
 	// TODO: jam 2016-02-25 This tells LXD to listen on all addresses,
@@ -99,8 +95,12 @@ func prepareRemote(client *Client, newCert Cert) (string, error) {
 		return "", errors.Trace(err)
 	}
 
+	if newCert == nil {
+		return "", nil
+	}
+
 	// Make sure the LXD service will allow our certificate to connect
-	if err := client.AddCert(newCert); err != nil {
+	if err := client.AddCert(*newCert); err != nil {
 		return "", errors.Trace(err)
 	}
 

tools/lxdclient/remote.go

@@ -9,13 +9,11 @@ import (
 	"github.com/juju/errors"
 	"github.com/juju/utils"
 	lxdshared "github.com/lxc/lxd/shared"
-
-	"github.com/juju/juju/network"
 )
 
 const (
 	// remoteLocalName is a specific remote name in the default LXD config.
-	// See https://github.com/lxc/lxd/blob/master/config.go:defaultRemote.
+	// See https://github.com/lxc/lxd/blob/master/config.go:DefaultRemotes.
 	remoteLocalName  = "local"
 	remoteIDForLocal = remoteLocalName
 )
@@ -130,8 +128,6 @@ func (r Remote) withLocalDefaults() Remote {
 		r.Protocol = LXDProtocol
 	}
 
-	// TODO(ericsnow) Set r.Cert to nil?
-
 	return r
 }
 
@@ -176,35 +172,31 @@ func (r Remote) validateLocal() error {
 	return nil
 }
 
-// UsingTCP converts the remote into a non-local version. For
-// non-local remotes this is a no-op.
+// UsingTCP converts the remote into a non-local version. For non-local remotes
+// this is a no-op.
 //
-// For a "local" remote (see Local), the remote is changed to a one
-// with the host set to the IP address of the local lxcbr0 bridge
-// interface. The remote is also set up for remote access, setting
-// the cert if not already set.
-func (r Remote) UsingTCP() (Remote, error) {
+// For a "local" remote (see Local), the remote is changed to a one with the
+// host set to the first IPv4 address assigned to the given bridgeName. The
+// remote is also set up for remote access, setting the cert if not already set.
+func (r Remote) UsingTCP(bridgeName string) (Remote, error) {
 	// Note that r is a value receiver, so it is an implicit copy.
 
 	if !r.isLocal() {
 		return r, nil
 	}
 
-	// TODO: jam 2016-02-25 This should be updated for systems that are
-	// 	 space aware, as we may not be just using the default LXC
-	// 	 bridge.
-	addr, err := utils.GetAddressForInterface(network.DefaultLXDBridge)
+	address, err := utils.GetAddressForInterface(bridgeName)
 	if err != nil {
 		return r, errors.Trace(err)
 	}
-	r.Host = addr
+	r.Host = address
+
+	// TODO(ericsnow) Change r.Name if "local"? Prepend "juju-"?
 
 	r, err = r.WithDefaults()
 	if err != nil {
 		return r, errors.Trace(err)
 	}
 
-	// TODO(ericsnow) Change r.Name if "local"? Prepend "juju-"?
-
 	return r, nil
 }

tools/lxdclient/remote_test.go

@@ -6,15 +6,12 @@
 package lxdclient_test
 
 import (
-	"fmt"
 	"net"
 
 	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
-	"github.com/juju/utils"
 	gc "gopkg.in/check.v1"
 
-	"github.com/juju/juju/network"
 	"github.com/juju/juju/tools/lxdclient"
 )
 
@@ -416,7 +413,7 @@ func (s *remoteSuite) TestUsingTCPNoop(c *gc.C) {
 		Protocol: lxdclient.LXDProtocol,
 		Cert:     s.Cert,
 	}
-	nonlocal, err := remote.UsingTCP()
+	nonlocal, err := remote.UsingTCP("")
 	c.Assert(err, jc.ErrorIsNil)
 
 	c.Check(nonlocal, jc.DeepEquals, remote)
@@ -427,20 +424,14 @@ type remoteFunctionalSuite struct {
 }
 
 func (s *remoteFunctionalSuite) TestUsingTCP(c *gc.C) {
-	if _, err := net.InterfaceByName(network.DefaultLXDBridge); err != nil {
-		c.Skip("network bridge interface not found")
-	}
-	if _, err := utils.GetAddressForInterface(network.DefaultLXDBridge); err != nil {
-		c.Skip(fmt.Sprintf("no IPv4 address available for %s", network.DefaultLXDBridge))
-	}
 	lxdclient.PatchGenerateCertificate(&s.CleanupSuite, testingCert, testingKey)
 
 	remote := lxdclient.Remote{
 		Name: "my-remote",
 		Host: "",
 		Cert: nil,
 	}
-	nonlocal, err := remote.UsingTCP()
+	nonlocal, err := remote.UsingTCP("lo")
 	c.Assert(err, jc.ErrorIsNil)
 
 	checkValidRemote(c, &nonlocal)