Merge lp:~axwalk/juju-core/delete-sudo-chown into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

utils: remove sudo/chown helpers

We no longer need these, as we no longer require
sudo around juju for local. The one remaining user,
utils/ssh, has been updated to no longer use the
helpers.

https://codereview.appspot.com/58160043/

utils: remove sudo/chown helpers

We no longer need these, as we no longer require
sudo around juju for local. The one remaining user,
utils/ssh, has been updated to no longer use the
helpers.

https://codereview.appspot.com/58160043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+203665_code.launchpad.net,

Message:
Please take a look.

Description:
utils: remove sudo/chown helpers

We no longer need these, as we no longer require
sudo around juju for local. The one remaining user,
utils/ssh, has been updated to no longer use the
helpers.

https://code.launchpad.net/~axwalk/juju-core/delete-sudo-chown/+merge/203665

(do not edit description out of merge proposal)

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

Affected files (+3, -244 lines):
   A [revision details]
   M utils/ssh/clientkeys.go
   D utils/sudo.go
   D utils/sudo_test.go

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/58160043/

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/delete-sudo-chown into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.899s
ok launchpad.net/juju-core/agent/tools 0.282s
ok launchpad.net/juju-core/bzr 7.431s
ok launchpad.net/juju-core/cert 3.252s
ok launchpad.net/juju-core/charm 0.621s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.039s
ok launchpad.net/juju-core/cloudinit/sshinit 1.070s
ok launchpad.net/juju-core/cmd 0.241s
ok launchpad.net/juju-core/cmd/charm-admin 0.837s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 234.786s
ok launchpad.net/juju-core/cmd/jujud 61.784s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 12.225s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.029s
ok launchpad.net/juju-core/container 0.043s
ok launchpad.net/juju-core/container/factory 0.051s
ok launchpad.net/juju-core/container/kvm 0.347s
ok launchpad.net/juju-core/container/kvm/mock 0.054s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.333s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.280s
ok launchpad.net/juju-core/environs 3.180s
ok launchpad.net/juju-core/environs/bootstrap 4.563s
ok launchpad.net/juju-core/environs/cloudinit 0.626s
ok launchpad.net/juju-core/environs/config 3.506s
ok launchpad.net/juju-core/environs/configstore 0.040s
ok launchpad.net/juju-core/environs/filestorage 0.031s
ok launchpad.net/juju-core/environs/httpstorage 0.956s
ok launchpad.net/juju-core/environs/imagemetadata 0.534s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.058s
ok launchpad.net/juju-core/environs/jujutest 0.260s
ok launchpad.net/juju-core/environs/manual 9.348s
ok launchpad.net/juju-core/environs/simplestreams 0.335s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.224s
ok launchpad.net/juju-core/environs/storage 1.266s
ok launchpad.net/juju-core/environs/sync 34.852s
ok launchpad.net/juju-core/environs/testing 0.201s
ok launchpad.net/juju-core/environs/tools 6.982s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 22.854s
ok launchpad.net/juju-core/juju/osenv 0.019s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.022s
ok launchpad.net/juju-core/names 0.028s
? launchpad.net/juju-core/...

=== modified file 'utils/ssh/clientkeys.go'
--- utils/ssh/clientkeys.go	2014-01-16 00:51:05 +0000
+++ utils/ssh/clientkeys.go	2014-01-29 04:49:36 +0000
@@ -61,7 +61,7 @@
 		// Directory exists but contains no keys;
 		// fall through and create one.
 	}
-	if err := utils.MkdirAllForUser(dir, 0700); err != nil {
+	if err := os.MkdirAll(dir, 0700); err != nil {
 		return err
 	}
 	keyfile, key, err := generateClientKey(dir)
@@ -97,11 +97,6 @@
 		os.Remove(privkeyFilename)
 		return "", nil, err
 	}
-	if err := utils.ChownToUser(privkeyFilename, privkeyFilename+PublicKeySuffix); err != nil {
-		os.Remove(privkeyFilename)
-		os.Remove(privkeyFilename + PublicKeySuffix)
-		return "", nil, err
-	}
 	return privkeyFilename, clientPrivateKey, nil
 }
 

=== removed file 'utils/sudo.go'
--- utils/sudo.go	2014-01-16 03:48:15 +0000
+++ utils/sudo.go	1970-01-01 00:00:00 +0000
@@ -1,89 +0,0 @@
-// Copyright 2013 Canonical Ltd.
-// Licensed under the AGPLv3, see LICENCE file for details.
-
-package utils
-
-import (
-	"fmt"
-	"os"
-	"path/filepath"
-	"strconv"
-)
-
-// CheckIfRoot is a simple function that we can use to determine if
-// the ownership of files and directories we create.
-var CheckIfRoot = func() bool {
-	return os.Getuid() == 0
-}
-
-// SudoCallerIds returns the user id and group id of the SUDO caller.
-// If either is unset, it returns zero for both values.
-// An error is returned if the relevant environment variables
-// are not valid integers.
-func SudoCallerIds() (uid int, gid int, err error) {
-	uidStr := os.Getenv("SUDO_UID")
-	gidStr := os.Getenv("SUDO_GID")
-
-	if uidStr == "" || gidStr == "" {
-		return 0, 0, nil
-	}
-	uid, err = strconv.Atoi(uidStr)
-	if err != nil {
-		return 0, 0, fmt.Errorf("invalid value %q for SUDO_UID", uidStr)
-	}
-	gid, err = strconv.Atoi(gidStr)
-	if err != nil {
-		return 0, 0, fmt.Errorf("invalid value %q for SUDO_GID", gidStr)
-	}
-	return
-}
-
-// MkdirForUser will call down to os.Mkdir and if the user is running as root,
-// the ownership will be changed to the sudo user.
-func MkdirForUser(dir string, perm os.FileMode) error {
-	if err := os.Mkdir(dir, perm); err != nil {
-		return err
-	}
-	return ChownToUser(dir)
-}
-
-// MkdirAllForUser will call down to os.MkdirAll and if the user is running as
-// root, the ownership will be changed to the sudo user for each directory
-// that was created.
-func MkdirAllForUser(dir string, perm os.FileMode) error {
-	toCreate := []string{}
-	path := dir
-	for {
-		_, err := os.Lstat(path)
-		if os.IsNotExist(err) {
-			toCreate = append(toCreate, path)
-		} else {
-			break
-		}
-		path = filepath.Dir(path)
-	}
-
-	if err := os.MkdirAll(dir, perm); err != nil {
-		return err
-	}
-	return ChownToUser(toCreate...)
-}
-
-// ChownToUser will attempt to change the ownership of all the paths
-// to the user returned by the SudoCallerIds method.  Ownership change
-// will only be attempted if we are running as root.
-func ChownToUser(paths ...string) error {
-	if !CheckIfRoot() {
-		return nil
-	}
-	uid, gid, err := SudoCallerIds()
-	if err != nil {
-		return err
-	}
-	for _, path := range paths {
-		if err := os.Chown(path, uid, gid); err != nil {
-			return err
-		}
-	}
-	return nil
-}

=== removed file 'utils/sudo_test.go'
--- utils/sudo_test.go	2014-01-16 03:48:15 +0000
+++ utils/sudo_test.go	1970-01-01 00:00:00 +0000
@@ -1,149 +0,0 @@
-// Copyright 2013 Canonical Ltd.
-// Licensed under the AGPLv3, see LICENCE file for details.
-
-package utils_test
-
-import (
-	"os"
-	"os/user"
-	"path/filepath"
-
-	gc "launchpad.net/gocheck"
-
-	jc "launchpad.net/juju-core/testing/checkers"
-	"launchpad.net/juju-core/testing/testbase"
-	"launchpad.net/juju-core/utils"
-)
-
-type sudoSuite struct {
-	testbase.LoggingSuite
-}
-
-var _ = gc.Suite(&sudoSuite{})
-
-func (s *sudoSuite) TestSudoCallerIds(c *gc.C) {
-	s.PatchEnvironment("SUDO_UID", "0")
-	s.PatchEnvironment("SUDO_GID", "0")
-	for _, test := range []struct {
-		uid         string
-		gid         string
-		errString   string
-		expectedUid int
-		expectedGid int
-	}{{
-		uid: "",
-		gid: "",
-	}, {
-		uid:         "1001",
-		gid:         "1002",
-		expectedUid: 1001,
-		expectedGid: 1002,
-	}, {
-		uid:       "1001",
-		gid:       "foo",
-		errString: `invalid value "foo" for SUDO_GID`,
-	}, {
-		uid:       "foo",
-		gid:       "bar",
-		errString: `invalid value "foo" for SUDO_UID`,
-	}} {
-		os.Setenv("SUDO_UID", test.uid)
-		os.Setenv("SUDO_GID", test.gid)
-		uid, gid, err := utils.SudoCallerIds()
-		if test.errString == "" {
-			c.Assert(err, gc.IsNil)
-			c.Assert(uid, gc.Equals, test.expectedUid)
-			c.Assert(gid, gc.Equals, test.expectedGid)
-		} else {
-			c.Assert(err, gc.ErrorMatches, test.errString)
-			c.Assert(uid, gc.Equals, 0)
-			c.Assert(gid, gc.Equals, 0)
-		}
-	}
-}
-
-func (s *sudoSuite) TestMkDirForUserAsUser(c *gc.C) {
-	dir := filepath.Join(c.MkDir(), "new-dir")
-	err := utils.MkdirForUser(dir, 0755)
-	c.Assert(err, gc.IsNil)
-	c.Assert(dir, jc.IsDirectory)
-}
-
-func (s *sudoSuite) setupBadSudoUid() {
-	s.PatchEnvironment("SUDO_UID", "omg")
-	s.PatchEnvironment("SUDO_GID", "omg")
-	s.PatchValue(&utils.CheckIfRoot, func() bool { return true })
-}
-
-func (s *sudoSuite) setupGoodSudoUid(c *gc.C) {
-	user, err := user.Current()
-	c.Assert(err, gc.IsNil)
-	s.PatchEnvironment("SUDO_UID", user.Uid)
-	s.PatchEnvironment("SUDO_GID", user.Gid)
-	s.PatchValue(&utils.CheckIfRoot, func() bool { return true })
-}
-
-func (s *sudoSuite) TestMkDirForUserRoot(c *gc.C) {
-	s.setupGoodSudoUid(c)
-	dir := filepath.Join(c.MkDir(), "new-dir")
-	err := utils.MkdirForUser(dir, 0755)
-	c.Assert(err, gc.IsNil)
-	c.Assert(dir, jc.IsDirectory)
-}
-
-func (s *sudoSuite) TestMkDirForUserWithError(c *gc.C) {
-	s.setupBadSudoUid()
-	dir := filepath.Join(c.MkDir(), "new-dir")
-	err := utils.MkdirForUser(dir, 0755)
-	c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
-	// The directory is still there.
-	c.Assert(dir, jc.IsDirectory)
-}
-
-func (s *sudoSuite) TestMkDirAllForUserAsUser(c *gc.C) {
-	dir := filepath.Join(c.MkDir(), "new-dir", "and-another")
-	err := utils.MkdirAllForUser(dir, 0755)
-	c.Assert(err, gc.IsNil)
-	c.Assert(dir, jc.IsDirectory)
-}
-
-func (s *sudoSuite) TestMkDirAllForUserRoot(c *gc.C) {
-	s.setupGoodSudoUid(c)
-
-	dir := filepath.Join(c.MkDir(), "new-dir", "and-another")
-	err := utils.MkdirAllForUser(dir, 0755)
-	c.Assert(err, gc.IsNil)
-	c.Assert(dir, jc.IsDirectory)
-}
-
-func (s *sudoSuite) TestMkDirAllForUserWithError(c *gc.C) {
-	s.setupBadSudoUid()
-
-	base := c.MkDir()
-	dir := filepath.Join(base, "new-dir", "and-another")
-	err := utils.MkdirAllForUser(dir, 0755)
-	c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
-	// The directory is still there.
-	c.Assert(dir, jc.IsDirectory)
-}
-
-func (s *sudoSuite) TestChownToUserNotRoot(c *gc.C) {
-	// Just in case someone runs the suite as root, we don't want to fail.
-	s.PatchValue(&utils.CheckIfRoot, func() bool { return false })
-
-	nonExistant := filepath.Join(c.MkDir(), "non-existant")
-	err := utils.ChownToUser(nonExistant)
-	c.Assert(err, gc.IsNil)
-}
-
-func (s *sudoSuite) TestChownToUserBadUid(c *gc.C) {
-	s.setupBadSudoUid()
-	err := utils.ChownToUser(c.MkDir())
-	c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
-}
-
-func (s *sudoSuite) TestChownToUser(c *gc.C) {
-	s.setupGoodSudoUid(c)
-	err := utils.ChownToUser(c.MkDir(), c.MkDir())
-	c.Assert(err, gc.IsNil)
-}