Merge lp:~axwalk/juju-core/lp1223277-check-provisioned-ignore-stderr into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

environs/manual: ignore stderr in checkProvisioned

ssh may issue warnings on stderr, for example if
the target host does not exist in known_hosts.
This was tripping up the checkProvisioned function,
which was incorrectly checking that both stdout and
stderr were empty.

Fixes #1223277

https://codereview.appspot.com/13477045/

environs/manual: ignore stderr in checkProvisioned

ssh may issue warnings on stderr, for example if
the target host does not exist in known_hosts.
This was tripping up the checkProvisioned function,
which was incorrectly checking that both stdout and
stderr were empty.

Fixes #1223277

https://codereview.appspot.com/13477045/

[Andrew Wilkins (axwalk)]

Reviewers: mp+184911_code.launchpad.net,

Message:
Please take a look.

Description:
environs/manual: ignore stderr in checkProvisioned

ssh may issue warnings on stderr, for example if
the target host does not exist in known_hosts.
This was tripping up the checkProvisioned function,
which was incorrectly checking that both stdout and
stderr were empty.

Fixes #1223277

https://code.launchpad.net/~axwalk/juju-core/lp1223277-check-provisioned-ignore-stderr/+merge/184911

(do not edit description out of merge proposal)

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

Affected files (+54, -8 lines):
   A [revision details]
   M environs/manual/detection.go
   M environs/manual/detection_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130910174617-1ib2wgm1qymgkvvs
+New revision: <email address hidden>

Index: environs/manual/detection.go
=== modified file 'environs/manual/detection.go'
--- environs/manual/detection.go 2013-08-23 06:57:09 +0000
+++ environs/manual/detection.go 2013-09-11 01:58:11 +0000
@@ -26,11 +26,16 @@
  func checkProvisioned(sshHost string) (bool, error) {
   cmd := exec.Command("ssh", sshHost, "bash")
   cmd.Stdin = bytes.NewBufferString(checkProvisionedScript)
- out, err := cmd.CombinedOutput()
- if err != nil {
+ var stdout, stderr bytes.Buffer
+ cmd.Stdout = &stdout
+ cmd.Stderr = &stderr
+ if err := cmd.Run(); err != nil {
+ if stderr.Len() != 0 {
+ err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(stderr.String()))
+ }
    return false, err
   }
- return len(strings.TrimSpace(string(out))) > 0, nil
+ return len(strings.TrimSpace(stdout.String())) > 0, nil
  }

  // detectSeriesHardwareCharacteristics detects the OS series

Index: environs/manual/detection_test.go
=== modified file 'environs/manual/detection_test.go'
--- environs/manual/detection_test.go 2013-08-29 03:58:19 +0000
+++ environs/manual/detection_test.go 2013-09-11 01:58:11 +0000
@@ -13,6 +13,7 @@
   gc "launchpad.net/gocheck"

   "launchpad.net/juju-core/testing"
+ jc "launchpad.net/juju-core/testing/checkers"
  )

  type detectionSuite struct {
@@ -34,7 +35,10 @@
          echo "ERROR: did not match expected input" >&2
          exit $exitcode
      fi
-%s
+ # stdout
+ %s
+ # stderr
+ %s
      exit %d
  else
      export PATH=${PATH#*:}
@@ -44,14 +48,25 @@
  // sshresponse creates a fake "ssh" command in a new $PATH,
  // updates $PATH, and returns a function to reset $PATH to
  // its original value when called.
-func sshresponse(c *gc.C, input, output string, rc int) func() {
+//
+// output may be:
+// - nil (no output)
+// - a string (stdout)
+// - a slice of strings, of length two (stdout, stderr)
+func sshresponse(c *gc.C, input string, output interface{}, rc int) func()
{
   fakebin := c.MkDir()
   ssh := filepath.Join(fakebin, "ssh")
   sshexpectedinput := ssh + ".expected-input"
- if output != "" {
- output = fmt.Sprintf("cat<<EOF\n%s\nEOF", output)
+ var stdout, stderr string
+ switch output := output.(typ...

[William Reade (fwereade)]

On 2013/09/11 02:21:17, dfc wrote:
> On 2013/09/11 02:07:37, axw wrote:
> > Please take a look.

> Can I suggest a different solution.

> 1. Use cmd.Output, not CombinedOutput

> 2. pass "-o", "StrictHostKeyChecking no", we do that elsewhere in
cmd/juju so it
> has a precident.

+1. Isn't there a common set of ssh flags somewhere?

https://codereview.appspot.com/13477045/

[Dave Cheney (dave-cheney)]

Well, there is only that one, but your argument continues to be valid.

On 12/09/2013, at 0:24, William Reade <email address hidden> wrote:

> On 2013/09/11 02:21:17, dfc wrote:
>> On 2013/09/11 02:07:37, axw wrote:
>>> Please take a look.
>
>> Can I suggest a different solution.
>
>> 1. Use cmd.Output, not CombinedOutput
>
>> 2. pass "-o", "StrictHostKeyChecking no", we do that elsewhere in
> cmd/juju so it
>> has a precident.
>
> +1. Isn't there a common set of ssh flags somewhere?
>
> https://codereview.appspot.com/13477045/
>
> --
> https://code.launchpad.net/~axwalk/juju-core/lp1223277-check-provisioned-ignore-stderr/+merge/184911
> You are subscribed to branch lp:juju-core.

[Andrew Wilkins (axwalk)]

On 2013/09/11 14:24:04, fwereade wrote:
> On 2013/09/11 02:21:17, dfc wrote:
> > On 2013/09/11 02:07:37, axw wrote:
> > > Please take a look.
> >
> > Can I suggest a different solution.
> >
> > 1. Use cmd.Output, not CombinedOutput

I want stderr still (for the error), so I need to set Stderr. So I'm
setting Stdout for consistency.

> > 2. pass "-o", "StrictHostKeyChecking no", we do that elsewhere in
cmd/juju so
> it
> > has a precident.

Okay. The stderr is getting squashed anyway, unless there's a real
error, so it's not a real security concern.

> +1. Isn't there a common set of ssh flags somewhere?

No. I'll create a tech-debt bug.

https://codereview.appspot.com/13477045/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13477045/

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/13477045/

=== modified file 'environs/manual/agent.go'
--- environs/manual/agent.go	2013-09-03 09:03:02 +0000
+++ environs/manual/agent.go	2013-09-12 02:07:53 +0000
@@ -7,7 +7,6 @@
 	"encoding/base64"
 	"fmt"
 	"os"
-	"os/exec"
 	"strings"
 
 	corecloudinit "launchpad.net/juju-core/cloudinit"
@@ -45,12 +44,11 @@
 	}
 	scriptBase64 := base64.StdEncoding.EncodeToString([]byte(script))
 	script = fmt.Sprintf(`F=$(mktemp); echo %s | base64 -d > $F; . $F`, scriptBase64)
-	sshArgs := []string{
+	cmd := sshCommand(
 		args.host,
-		"-t", // allocate a pseudo-tty
-		"--", fmt.Sprintf("sudo bash -c '%s'", script),
-	}
-	cmd := exec.Command("ssh", sshArgs...)
+		fmt.Sprintf("sudo bash -c '%s'", script),
+		allocateTTY,
+	)
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 	cmd.Stdin = os.Stdin
 
=== modified file 'environs/manual/detection.go'
--- environs/manual/detection.go	2013-08-23 06:57:09 +0000
+++ environs/manual/detection.go	2013-09-12 02:07:53 +0000
@@ -6,7 +6,6 @@
 import (
 	"bytes"
 	"fmt"
-	"os/exec"
 	"regexp"
 	"strconv"
 	"strings"
@@ -24,20 +23,25 @@
 // checkProvisioned checks if any juju upstart jobs already
 // exist on the host machine.
 func checkProvisioned(sshHost string) (bool, error) {
-	cmd := exec.Command("ssh", sshHost, "bash")
+	cmd := sshCommand(sshHost, "bash")
+	var stdout, stderr bytes.Buffer
 	cmd.Stdin = bytes.NewBufferString(checkProvisionedScript)
-	out, err := cmd.CombinedOutput()
-	if err != nil {
+	cmd.Stdout = &stdout
+	cmd.Stderr = &stderr
+	if err := cmd.Run(); err != nil {
+		if stderr.Len() != 0 {
+			err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(stderr.String()))
+		}
 		return false, err
 	}
-	return len(strings.TrimSpace(string(out))) > 0, nil
+	return len(strings.TrimSpace(stdout.String())) > 0, nil
 }
 
 // detectSeriesHardwareCharacteristics detects the OS series
 // and hardware characteristics of the remote machine by
 // connecting to the machine and executing a bash script.
 func detectSeriesAndHardwareCharacteristics(sshHost string) (hc instance.HardwareCharacteristics, series string, err error) {
-	cmd := exec.Command("ssh", sshHost, "bash")
+	cmd := sshCommand(sshHost, "bash")
 	cmd.Stdin = bytes.NewBufferString(detectionScript)
 	out, err := cmd.CombinedOutput()
 	if err != nil {
 
=== modified file 'environs/manual/detection_test.go'
--- environs/manual/detection_test.go	2013-08-29 03:58:19 +0000
+++ environs/manual/detection_test.go	2013-09-12 02:07:53 +0000
@@ -13,6 +13,7 @@
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
 )
 
 type detectionSuite struct {
@@ -34,7 +35,10 @@
         echo "ERROR: did not match expected input" >&2
         exit $exitcode
     fi
-%s
+    # stdout
+    %s
+    # stderr
+    %s
     exit %d
 else
     export PATH=${PATH#*:}
@@ -44,14 +48,25 @@
 // sshresponse creates a fake "ssh" command in a new $PATH,
 // updates $PATH, and returns a function to reset $PATH to
 // its original value when called.
-func sshresponse(c *gc.C, input, output string, rc int) func() {
+//
+// output may be:
+//    - nil (no output)
+//    - a string (stdout)
+//    - a slice of strings, of length two (stdout, stderr)
+func sshresponse(c *gc.C, input string, output interface{}, rc int) func() {
 	fakebin := c.MkDir()
 	ssh := filepath.Join(fakebin, "ssh")
 	sshexpectedinput := ssh + ".expected-input"
-	if output != "" {
-		output = fmt.Sprintf("cat<<EOF\n%s\nEOF", output)
+	var stdout, stderr string
+	switch output := output.(type) {
+	case nil:
+	case string:
+		stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output)
+	case []string:
+		stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output[0])
+		stderr = fmt.Sprintf("cat>&2<<EOF\n%s\nEOF", output[1])
 	}
-	script := fmt.Sprintf(sshscript, output, rc)
+	script := fmt.Sprintf(sshscript, stdout, stderr, rc)
 	err := ioutil.WriteFile(ssh, []byte(script), 0777)
 	c.Assert(err, gc.IsNil)
 	err = ioutil.WriteFile(sshexpectedinput, []byte(input), 0644)
@@ -139,3 +154,27 @@
 		c.Assert(hc.String(), gc.Equals, test.expectedHc)
 	}
 }
+
+func (s *detectionSuite) TestCheckProvisioned(c *gc.C) {
+	defer sshresponse(c, checkProvisionedScript, "", 0)()
+	provisioned, err := checkProvisioned("example.com")
+	c.Assert(err, gc.IsNil)
+	c.Assert(provisioned, jc.IsFalse)
+
+	defer sshresponse(c, checkProvisionedScript, "non-empty", 0)()
+	provisioned, err = checkProvisioned("example.com")
+	c.Assert(err, gc.IsNil)
+	c.Assert(provisioned, jc.IsTrue)
+
+	// stderr should not affect result.
+	defer sshresponse(c, checkProvisionedScript, []string{"", "non-empty-stderr"}, 0)()
+	provisioned, err = checkProvisioned("example.com")
+	c.Assert(err, gc.IsNil)
+	c.Assert(provisioned, jc.IsFalse)
+
+	// if the script fails for whatever reason, then checkProvisioned
+	// will return an error. stderr will be included in the error message.
+	defer sshresponse(c, checkProvisionedScript, []string{"non-empty-stdout", "non-empty-stderr"}, 255)()
+	_, err = checkProvisioned("example.com")
+	c.Assert(err, gc.ErrorMatches, "exit status 255 \\(non-empty-stderr\\)")
+}
 
=== added file 'environs/manual/ssh.go'
--- environs/manual/ssh.go	1970-01-01 00:00:00 +0000
+++ environs/manual/ssh.go	2013-09-12 02:07:53 +0000
@@ -0,0 +1,25 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package manual
+
+import (
+	"os/exec"
+)
+
+type sshOption []string
+
+var allocateTTY sshOption = []string{"-t"}
+
+// TODO(axw) 2013-09-12 bug #1224230
+// Move this to a common package for use in cmd/juju, and others.
+var commonSSHOptions = []string{"-o", "StrictHostKeyChecking no"}
+
+func sshCommand(host string, command string, options ...sshOption) *exec.Cmd {
+	args := append([]string{}, commonSSHOptions...)
+	for _, option := range options {
+		args = append(args, option...)
+	}
+	args = append(args, host, "--", command)
+	return exec.Command("ssh", args...)
+}