Merge lp:~sidnei/juju-core/lxc-mirror into lp:~go-bot/juju-core/trunk

[Sidnei da Silva]

Fetch apt proxy config via host apt-config

Since we're passing custom userdata to the ubuntu-cloud lxc template, it ends
up not setting the apt_mirror to the MIRROR setting. Read apt proxy
configuration from the container's host instead via the apt-config command and
inject that into the container via apt_proxy cloud init setting for the http
proxy plus a runcmd that writes /etc/apt/apt.conf.d/99proxy-extra with the
same config as the host for the remaing settings not supported explicitly by
cloud-init at the moment.

https://codereview.appspot.com/12143043/

R=dimitern, rogpeppe, smoser

Fetch apt proxy config via host apt-config

Since we're passing custom userdata to the ubuntu-cloud lxc template, it ends
up not setting the apt_mirror to the MIRROR setting. Read apt proxy
configuration from the container's host instead via the apt-config command and
inject that into the container via apt_proxy cloud init setting for the http
proxy plus a runcmd that writes /etc/apt/apt.conf.d/99proxy-extra with the
same config as the host for the remaing settings not supported explicitly by
cloud-init at the moment.

https://codereview.appspot.com/12143043/

[Sidnei da Silva (sidnei)]

Reviewers: mp+177737_code.launchpad.net,

Message:
Please take a look.

Description:
Fetch MIRROR setting from /etc/default/lxc

Since we're passing custom userdata to the ubuntu-cloud lxc template, it
ends
up not setting the apt_mirror to the MIRROR setting. One could argue
that the
template should be fixed instead to always set the apt_mirror if it's
unset in
the custom userdata, but that's a separate discussion.

https://code.launchpad.net/~sidnei/juju-core/lxc-mirror/+merge/177737

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/12143043/

Affected files:
   A [revision details]
   M agent/agent.go
   M cloudinit/cloudinit_test.go
   M cloudinit/options.go
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go
   M environs/cloudinit/cloudinit.go
   M environs/cloudinit/cloudinit_test.go
   M utils/apt.go
   M utils/apt_test.go

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/

[Dimiter Naydenov (dimitern)]

LGTM, although I think I reviewed part of that as a previous CL
(AddFile, AddScripts, etc.) did that one get merged here?

https://codereview.appspot.com/12143043/diff/7001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/7001/container/lxc/lxc_test.go#newcode46
container/lxc/lxc_test.go:46: []byte(aptConfig), 0755)

Please put 0755 on a separate line, like the other args.

https://codereview.appspot.com/12143043/

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/diff/7001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/7001/container/lxc/lxc_test.go#newcode46
container/lxc/lxc_test.go:46: []byte(aptConfig), 0755)
On 2013/08/05 10:46:08, dimitern wrote:

> Please put 0755 on a separate line, like the other args.

Done.

https://codereview.appspot.com/12143043/

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/

[Roger Peppe (rogpeppe)]

Looks OK to me, although I'm not qualified to
review it - I have no knowledge of apt proxy
settings, and given that we can't have tests that
actually test this for real, I'd be happier if it's
reviewed by someone that does.

Some suggestions below.

https://codereview.appspot.com/12143043/diff/15002/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/15002/container/lxc/lxc_test.go#newcode124
container/lxc/lxc_test.go:124: c.Assert(scripts[len(scripts)-4],
gc.Equals, "start jujud-machine-1-lxc-0")
c.Assert(scripts[len(scripts)-4:], gc.DeepEquals, []string{
     "start juju-machine-1-lxc-0",
     "install -m 600 /dev/null '/etc/apt.conf.d/99proxy-extra'",
     etc
})

?

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode32
utils/apt.go:32: var commandOutput = osCommandOutput

var commandOutput = exec.Cmd.Output

(and you can delete the osCommandOutput definition
and probably move most of the comment onto commandOutput)

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode60
utils/apt.go:60: var (

No need to declare these here.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode70
utils/apt.go:70: if out, err = commandOutput(cmd); err != nil {

out, err := commandOutput(cmd)
if err != nil {
     return "", err
}
return strings.TrimSpace(string(out)), nil

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode71
utils/apt.go:71: return "", err
If the command has failed for some reason other
than a missing apt-config command, this error
will be highly opaque (it will contain only
the apt-config exit code).

I suggest that you acquire the standard error output
too, and parcel it up into an error if
the command fails.

You could use the same runCommand hook mechanism that AptGetInstall
uses (and in fact, AptGetInstall suffers from the same issue, but that's
probably something to fix in another CL)

https://codereview.appspot.com/12143043/

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/diff/15002/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/15002/container/lxc/lxc_test.go#newcode124
container/lxc/lxc_test.go:124: c.Assert(scripts[len(scripts)-4],
gc.Equals, "start jujud-machine-1-lxc-0")
On 2013/08/05 16:05:34, rog wrote:
> c.Assert(scripts[len(scripts)-4:], gc.DeepEquals, []string{
> "start juju-machine-1-lxc-0",
> "install -m 600 /dev/null '/etc/apt.conf.d/99proxy-extra'",
> etc
> })

> ?

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode32
utils/apt.go:32: var commandOutput = osCommandOutput
On 2013/08/05 16:05:34, rog wrote:

> var commandOutput = exec.Cmd.Output

> (and you can delete the osCommandOutput definition
> and probably move most of the comment onto commandOutput)

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode60
utils/apt.go:60: var (
On 2013/08/05 16:05:34, rog wrote:

> No need to declare these here.

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode70
utils/apt.go:70: if out, err = commandOutput(cmd); err != nil {
On 2013/08/05 16:05:34, rog wrote:

> out, err := commandOutput(cmd)
> if err != nil {
> return "", err
> }
> return strings.TrimSpace(string(out)), nil

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode70
utils/apt.go:70: if out, err = commandOutput(cmd); err != nil {
On 2013/08/05 16:05:34, rog wrote:

> out, err := commandOutput(cmd)
> if err != nil {
> return "", err
> }
> return strings.TrimSpace(string(out)), nil

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode71
utils/apt.go:71: return "", err
On 2013/08/05 16:05:34, rog wrote:
> If the command has failed for some reason other
> than a missing apt-config command, this error
> will be highly opaque (it will contain only
> the apt-config exit code).

> I suggest that you acquire the standard error output
> too, and parcel it up into an error if
> the command fails.

> You could use the same runCommand hook mechanism that AptGetInstall
> uses (and in fact, AptGetInstall suffers from the same issue, but
that's
> probably something to fix in another CL)

Done, fixed in this branch.

https://codereview.appspot.com/12143043/

[Roger Peppe (rogpeppe)]

LGTM with a couple of trivials.
Thanks a lot.

https://codereview.appspot.com/12143043/diff/22001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/22001/container/lxc/lxc_test.go#newcode128
container/lxc/lxc_test.go:128: "ifconfig"})

We'd usually put the }) on a new line here, so each string gets its own
line.

https://codereview.appspot.com/12143043/diff/22001/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/12143043/diff/22001/utils/apt.go#newcode61
utils/apt.go:61: "Acquire::ftp::Proxy"}

newline before } please

https://codereview.appspot.com/12143043/

[Scott Moser (smoser)]

so i think this is reasonable.
the only comments (that I think Sidnei is aware of) is that https_proxy set via runcmnd will be too late to affect 'apt-get installs' from cloud-config.

they should work fine for subsequent juju charm installs. though.

the principle seems sound here.

review: Approve

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/diff/22001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/22001/container/lxc/lxc_test.go#newcode128
container/lxc/lxc_test.go:128: "ifconfig"})
On 2013/08/05 17:35:39, rog wrote:

> We'd usually put the }) on a new line here, so each string gets its
own line.

Done.

https://codereview.appspot.com/12143043/diff/22001/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/12143043/diff/22001/utils/apt.go#newcode61
utils/apt.go:61: "Acquire::ftp::Proxy"}
On 2013/08/05 17:35:39, rog wrote:

> newline before } please

Done.

https://codereview.appspot.com/12143043/

=== modified file 'cloudinit/cloudinit_test.go'
--- cloudinit/cloudinit_test.go	2013-08-02 17:03:19 +0000
+++ cloudinit/cloudinit_test.go	2013-08-05 17:47:24 +0000
@@ -49,6 +49,13 @@
 		},
 	},
 	{
+		"AptProxy",
+		"apt_proxy: http://foo.com\n",
+		func(cfg *cloudinit.Config) {
+			cfg.SetAptProxy("http://foo.com")
+		},
+	},
+	{
 		"AptMirror",
 		"apt_mirror: http://foo.com\n",
 		func(cfg *cloudinit.Config) {

=== modified file 'cloudinit/options.go'
--- cloudinit/options.go	2013-08-02 15:50:58 +0000
+++ cloudinit/options.go	2013-08-05 17:47:24 +0000
@@ -37,6 +37,12 @@
 	cfg.set("apt_update", yes, yes)
 }
 
+// SetAptProxy sets the URL to be used as the apt
+// proxy.
+func (cfg *Config) SetAptProxy(url string) {
+	cfg.set("apt_proxy", url != "", url)
+}
+
 // SetAptMirror sets the URL to be used as the apt
 // mirror site. If not set, the URL is selected based
 // on cloud metadata in EC2 - <region>.archive.ubuntu.com

=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	2013-08-02 12:37:43 +0000
+++ container/lxc/lxc.go	2013-08-05 17:47:24 +0000
@@ -8,6 +8,7 @@
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 
 	"launchpad.net/golxc"
@@ -22,6 +23,7 @@
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
+	"launchpad.net/juju-core/utils"
 )
 
 var logger = loggo.GetLogger("juju.container.lxc")
@@ -33,6 +35,7 @@
 	lxcContainerDir     = "/var/lib/lxc"
 	lxcRestartDir       = "/etc/lxc/auto"
 	lxcObjectFactory    = golxc.Factory()
+	aptHTTPProxyRE      = regexp.MustCompile(`(?i)^Acquire::HTTP::Proxy\s+"([^"]+)";$`)
 )
 
 const (
@@ -340,9 +343,36 @@
 	if err != nil {
 		return nil, err
 	}
+
+	// Run apt-config to fetch proxy settings from host. If no proxy
+	// settings are configured, then we don't set up any proxy information
+	// on the container.
+	proxyConfig, err := utils.AptConfigProxy()
+	if err != nil {
+		return nil, err
+	}
+	if proxyConfig != "" {
+		var proxyLines []string
+		for _, line := range strings.Split(proxyConfig, "\n") {
+			line = strings.TrimSpace(line)
+			if m := aptHTTPProxyRE.FindStringSubmatch(line); m != nil {
+				cloudConfig.SetAptProxy(m[1])
+			} else {
+				proxyLines = append(proxyLines, line)
+			}
+		}
+		if len(proxyLines) > 0 {
+			cloudConfig.AddFile(
+				"/etc/apt/apt.conf.d/99proxy-extra",
+				strings.Join(proxyLines, "\n"),
+				0600)
+		}
+	}
+
 	// Run ifconfig to get the addresses of the internal container at least
 	// logged in the host.
 	cloudConfig.AddRunCmd("ifconfig")
+
 	data, err := cloudConfig.Render()
 	if err != nil {
 		return nil, err

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2013-07-31 23:07:56 +0000
+++ container/lxc/lxc_test.go	2013-08-05 17:47:24 +0000
@@ -30,6 +30,7 @@
 type LxcSuite struct {
 	testing.LoggingSuite
 	lxc.TestSuite
+	oldPath string
 }
 
 var _ = gc.Suite(&LxcSuite{})
@@ -37,9 +38,18 @@
 func (s *LxcSuite) SetUpSuite(c *gc.C) {
 	s.LoggingSuite.SetUpSuite(c)
 	s.TestSuite.SetUpSuite(c)
+	tmpDir := c.MkDir()
+	s.oldPath = os.Getenv("PATH")
+	os.Setenv("PATH", tmpDir)
+	err := ioutil.WriteFile(
+		filepath.Join(tmpDir, "apt-config"),
+		[]byte(aptConfigScript),
+		0755)
+	c.Assert(err, gc.IsNil)
 }
 
 func (s *LxcSuite) TearDownSuite(c *gc.C) {
+	os.Setenv("PATH", s.oldPath)
 	s.TestSuite.TearDownSuite(c)
 	s.LoggingSuite.TearDownSuite(c)
 }
@@ -55,6 +65,17 @@
 	s.LoggingSuite.TearDownTest(c)
 }
 
+const (
+	aptHTTPProxy     = "http://1.2.3.4:3142"
+	configProxyExtra = `Acquire::https::Proxy "false";
+Acquire::ftp::Proxy "false";`
+)
+
+var (
+	configHttpProxy = fmt.Sprintf(`Acquire::http::Proxy "%s";`, aptHTTPProxy)
+	aptConfigScript = fmt.Sprintf("#!/bin/sh\n echo '%s\n%s'", configHttpProxy, configProxyExtra)
+)
+
 func StartContainer(c *gc.C, manager lxc.ContainerManager, machineId string) instance.Instance {
 	config := testing.EnvironConfig(c)
 	stateInfo := jujutesting.FakeStateInfo(machineId)
@@ -93,13 +114,19 @@
 	err = goyaml.Unmarshal(data, &x)
 	c.Assert(err, gc.IsNil)
 
+	c.Assert(x["apt_proxy"], gc.Equals, aptHTTPProxy)
+
 	var scripts []string
 	for _, s := range x["runcmd"].([]interface{}) {
 		scripts = append(scripts, s.(string))
 	}
 
-	c.Assert(scripts[len(scripts)-2], gc.Equals, "start jujud-machine-1-lxc-0")
-	c.Assert(scripts[len(scripts)-1], gc.Equals, "ifconfig")
+	c.Assert(scripts[len(scripts)-4:], gc.DeepEquals, []string{
+		"start jujud-machine-1-lxc-0",
+		"install -m 600 /dev/null '/etc/apt/apt.conf.d/99proxy-extra'",
+		fmt.Sprintf("echo '%s' > '/etc/apt/apt.conf.d/99proxy-extra'", configProxyExtra),
+		"ifconfig",
+	})
 
 	// Check the mount point has been created inside the container.
 	c.Assert(filepath.Join(s.LxcDir, name, "rootfs/var/log/juju"), jc.IsDirectory)

=== modified file 'utils/apt.go'
--- utils/apt.go	2013-07-11 15:51:06 +0000
+++ utils/apt.go	2013-08-05 17:47:24 +0000
@@ -4,9 +4,12 @@
 package utils
 
 import (
+	"fmt"
 	"os"
 	"os/exec"
+	"strings"
 
+	"launchpad.net/juju-core/log"
 	"launchpad.net/loggo"
 )
 
@@ -14,13 +17,9 @@
 
 // Some helpful functions for running apt in a sane way
 
-// osRunCommand calls cmd.Run, this is used as an overloading point so we can
-// test what *would* be run without actually executing another program
-func osRunCommand(cmd *exec.Cmd) error {
-	return cmd.Run()
-}
-
-var runCommand = osRunCommand
+// commandOutput calls cmd.Output, this is used as an overloading point so we
+// can test what *would* be run without actually executing another program
+var commandOutput = (*exec.Cmd).CombinedOutput
 
 // This is the default apt-get command used in cloud-init, the various settings
 // mean that apt won't actually block waiting for a prompt from the user.
@@ -41,5 +40,32 @@
 	aptLogger.Infof("Running: %s", cmdArgs)
 	cmd := exec.Command(cmdArgs[0], cmdArgs[1:]...)
 	cmd.Env = append(os.Environ(), aptGetEnvOptions...)
-	return runCommand(cmd)
+	out, err := commandOutput(cmd)
+	if err != nil {
+		log.Errorf("utils/apt: apt-get command failed: %v\nargs: %#v\n%s",
+			err, cmdArgs, string(out))
+		return fmt.Errorf("apt-get failed: %v", err)
+	}
+	return nil
+}
+
+// AptConfigProxy will consult apt-config about the configured proxy
+// settings. If there are no proxy settings configured, an empty string is
+// returned.
+func AptConfigProxy() (string, error) {
+	cmdArgs := []string{
+		"apt-config",
+		"dump",
+		"Acquire::http::Proxy",
+		"Acquire::https::Proxy",
+		"Acquire::ftp::Proxy",
+	}
+	cmd := exec.Command(cmdArgs[0], cmdArgs[1:]...)
+	out, err := commandOutput(cmd)
+	if err != nil {
+		log.Errorf("utils/apt: apt-config command failed: %v\nargs: %#v\n%s",
+			err, cmdArgs, string(out))
+		return "", fmt.Errorf("apt-config failed: %v", err)
+	}
+	return strings.TrimSpace(string(out)), nil
 }

=== modified file 'utils/apt_test.go'
--- utils/apt_test.go	2013-07-11 13:35:37 +0000
+++ utils/apt_test.go	2013-08-05 17:47:24 +0000
@@ -4,6 +4,7 @@
 package utils
 
 import (
+	"fmt"
 	"os/exec"
 
 	gc "launchpad.net/gocheck"
@@ -13,24 +14,25 @@
 
 var _ = gc.Suite(&AptSuite{})
 
-// hookRunCommand intercepts runCommand to a function that passes the actual
-// command back via a channel, and returns the error passed into this function.
-// It also returns a cleanup function so you can restore the original function
-func (s *AptSuite) hookRunCommand(err error) (<-chan *exec.Cmd, func()) {
+// hookCommandOutput intercepts commandOutput to a function that passes the
+// actual command and it's output back via a channel, and returns the error
+// passed into this function.  It also returns a cleanup function so you can
+// restore the original function
+func (s *AptSuite) hookCommandOutput(output []byte, err error) (<-chan *exec.Cmd, func()) {
 	cmdChan := make(chan *exec.Cmd, 1)
-	origRunCommand := runCommand
+	origCommandOutput := commandOutput
 	cleanup := func() {
-		runCommand = origRunCommand
+		commandOutput = origCommandOutput
 	}
-	runCommand = func(cmd *exec.Cmd) error {
+	commandOutput = func(cmd *exec.Cmd) ([]byte, error) {
 		cmdChan <- cmd
-		return err
+		return output, err
 	}
 	return cmdChan, cleanup
 }
 
 func (s *AptSuite) TestOnePackage(c *gc.C) {
-	cmdChan, cleanup := s.hookRunCommand(nil)
+	cmdChan, cleanup := s.hookCommandOutput([]byte{}, nil)
 	defer cleanup()
 	err := AptGetInstall("test-package")
 	c.Assert(err, gc.IsNil)
@@ -42,3 +44,63 @@
 	})
 	c.Assert(cmd.Env[len(cmd.Env)-1], gc.Equals, "DEBIAN_FRONTEND=noninteractive")
 }
+
+func (s *AptSuite) TestAptGetError(c *gc.C) {
+	const expected = `E: frobnicator failure detected`
+	cmdError := fmt.Errorf("error")
+	cmdExpectedError := fmt.Errorf("apt-get failed: error")
+	cmdChan, cleanup := s.hookCommandOutput([]byte(expected), cmdError)
+	defer cleanup()
+	err := AptGetInstall("foo")
+	c.Assert(err, gc.DeepEquals, cmdExpectedError)
+	cmd := <-cmdChan
+	c.Assert(cmd.Args, gc.DeepEquals, []string{
+		"apt-get", "--option=Dpkg::Options::=--force-confold",
+		"--option=Dpkg::options::=--force-unsafe-io", "--assume-yes", "--quiet",
+		"install", "foo",
+	})
+}
+
+func (s *AptSuite) TestConfigProxyEmpty(c *gc.C) {
+	cmdChan, cleanup := s.hookCommandOutput([]byte{}, nil)
+	defer cleanup()
+	out, err := AptConfigProxy()
+	c.Assert(err, gc.IsNil)
+	cmd := <-cmdChan
+	c.Assert(cmd.Args, gc.DeepEquals, []string{
+		"apt-config", "dump", "Acquire::http::Proxy",
+		"Acquire::https::Proxy", "Acquire::ftp::Proxy",
+	})
+	c.Assert(out, gc.Equals, "")
+}
+
+func (s *AptSuite) TestConfigProxyConfigured(c *gc.C) {
+	const expected = `Acquire::http::Proxy "10.0.3.1:3142";
+Acquire::https::Proxy "false";`
+	cmdChan, cleanup := s.hookCommandOutput([]byte(expected), nil)
+	defer cleanup()
+	out, err := AptConfigProxy()
+	c.Assert(err, gc.IsNil)
+	cmd := <-cmdChan
+	c.Assert(cmd.Args, gc.DeepEquals, []string{
+		"apt-config", "dump", "Acquire::http::Proxy",
+		"Acquire::https::Proxy", "Acquire::ftp::Proxy",
+	})
+	c.Assert(out, gc.Equals, expected)
+}
+
+func (s *AptSuite) TestConfigProxyError(c *gc.C) {
+	const expected = `E: frobnicator failure detected`
+	cmdError := fmt.Errorf("error")
+	cmdExpectedError := fmt.Errorf("apt-config failed: error")
+	cmdChan, cleanup := s.hookCommandOutput([]byte(expected), cmdError)
+	defer cleanup()
+	out, err := AptConfigProxy()
+	c.Assert(err, gc.DeepEquals, cmdExpectedError)
+	cmd := <-cmdChan
+	c.Assert(cmd.Args, gc.DeepEquals, []string{
+		"apt-config", "dump", "Acquire::http::Proxy",
+		"Acquire::https::Proxy", "Acquire::ftp::Proxy",
+	})
+	c.Assert(out, gc.Equals, "")
+}