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

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...

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/

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.

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/

Please take a look.

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

LGTM

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