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

Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 2270
Proposed branch: lp:~axwalk/juju-core/delete-sudo-chown
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 272 lines (+1/-244)
3 files modified
utils/ssh/clientkeys.go (+1/-6)
utils/sudo.go (+0/-89)
utils/sudo_test.go (+0/-149)
To merge this branch: bzr merge lp:~axwalk/juju-core/delete-sudo-chown
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+203665@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :

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

Revision history for this message
Ian Booth (wallyworld) wrote :
Revision history for this message
Go Bot (go-bot) wrote :
Download full text (16.7 KiB)

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'utils/ssh/clientkeys.go'
2--- utils/ssh/clientkeys.go 2014-01-16 00:51:05 +0000
3+++ utils/ssh/clientkeys.go 2014-01-29 04:49:36 +0000
4@@ -61,7 +61,7 @@
5 // Directory exists but contains no keys;
6 // fall through and create one.
7 }
8- if err := utils.MkdirAllForUser(dir, 0700); err != nil {
9+ if err := os.MkdirAll(dir, 0700); err != nil {
10 return err
11 }
12 keyfile, key, err := generateClientKey(dir)
13@@ -97,11 +97,6 @@
14 os.Remove(privkeyFilename)
15 return "", nil, err
16 }
17- if err := utils.ChownToUser(privkeyFilename, privkeyFilename+PublicKeySuffix); err != nil {
18- os.Remove(privkeyFilename)
19- os.Remove(privkeyFilename + PublicKeySuffix)
20- return "", nil, err
21- }
22 return privkeyFilename, clientPrivateKey, nil
23 }
24
25
26=== removed file 'utils/sudo.go'
27--- utils/sudo.go 2014-01-16 03:48:15 +0000
28+++ utils/sudo.go 1970-01-01 00:00:00 +0000
29@@ -1,89 +0,0 @@
30-// Copyright 2013 Canonical Ltd.
31-// Licensed under the AGPLv3, see LICENCE file for details.
32-
33-package utils
34-
35-import (
36- "fmt"
37- "os"
38- "path/filepath"
39- "strconv"
40-)
41-
42-// CheckIfRoot is a simple function that we can use to determine if
43-// the ownership of files and directories we create.
44-var CheckIfRoot = func() bool {
45- return os.Getuid() == 0
46-}
47-
48-// SudoCallerIds returns the user id and group id of the SUDO caller.
49-// If either is unset, it returns zero for both values.
50-// An error is returned if the relevant environment variables
51-// are not valid integers.
52-func SudoCallerIds() (uid int, gid int, err error) {
53- uidStr := os.Getenv("SUDO_UID")
54- gidStr := os.Getenv("SUDO_GID")
55-
56- if uidStr == "" || gidStr == "" {
57- return 0, 0, nil
58- }
59- uid, err = strconv.Atoi(uidStr)
60- if err != nil {
61- return 0, 0, fmt.Errorf("invalid value %q for SUDO_UID", uidStr)
62- }
63- gid, err = strconv.Atoi(gidStr)
64- if err != nil {
65- return 0, 0, fmt.Errorf("invalid value %q for SUDO_GID", gidStr)
66- }
67- return
68-}
69-
70-// MkdirForUser will call down to os.Mkdir and if the user is running as root,
71-// the ownership will be changed to the sudo user.
72-func MkdirForUser(dir string, perm os.FileMode) error {
73- if err := os.Mkdir(dir, perm); err != nil {
74- return err
75- }
76- return ChownToUser(dir)
77-}
78-
79-// MkdirAllForUser will call down to os.MkdirAll and if the user is running as
80-// root, the ownership will be changed to the sudo user for each directory
81-// that was created.
82-func MkdirAllForUser(dir string, perm os.FileMode) error {
83- toCreate := []string{}
84- path := dir
85- for {
86- _, err := os.Lstat(path)
87- if os.IsNotExist(err) {
88- toCreate = append(toCreate, path)
89- } else {
90- break
91- }
92- path = filepath.Dir(path)
93- }
94-
95- if err := os.MkdirAll(dir, perm); err != nil {
96- return err
97- }
98- return ChownToUser(toCreate...)
99-}
100-
101-// ChownToUser will attempt to change the ownership of all the paths
102-// to the user returned by the SudoCallerIds method. Ownership change
103-// will only be attempted if we are running as root.
104-func ChownToUser(paths ...string) error {
105- if !CheckIfRoot() {
106- return nil
107- }
108- uid, gid, err := SudoCallerIds()
109- if err != nil {
110- return err
111- }
112- for _, path := range paths {
113- if err := os.Chown(path, uid, gid); err != nil {
114- return err
115- }
116- }
117- return nil
118-}
119
120=== removed file 'utils/sudo_test.go'
121--- utils/sudo_test.go 2014-01-16 03:48:15 +0000
122+++ utils/sudo_test.go 1970-01-01 00:00:00 +0000
123@@ -1,149 +0,0 @@
124-// Copyright 2013 Canonical Ltd.
125-// Licensed under the AGPLv3, see LICENCE file for details.
126-
127-package utils_test
128-
129-import (
130- "os"
131- "os/user"
132- "path/filepath"
133-
134- gc "launchpad.net/gocheck"
135-
136- jc "launchpad.net/juju-core/testing/checkers"
137- "launchpad.net/juju-core/testing/testbase"
138- "launchpad.net/juju-core/utils"
139-)
140-
141-type sudoSuite struct {
142- testbase.LoggingSuite
143-}
144-
145-var _ = gc.Suite(&sudoSuite{})
146-
147-func (s *sudoSuite) TestSudoCallerIds(c *gc.C) {
148- s.PatchEnvironment("SUDO_UID", "0")
149- s.PatchEnvironment("SUDO_GID", "0")
150- for _, test := range []struct {
151- uid string
152- gid string
153- errString string
154- expectedUid int
155- expectedGid int
156- }{{
157- uid: "",
158- gid: "",
159- }, {
160- uid: "1001",
161- gid: "1002",
162- expectedUid: 1001,
163- expectedGid: 1002,
164- }, {
165- uid: "1001",
166- gid: "foo",
167- errString: `invalid value "foo" for SUDO_GID`,
168- }, {
169- uid: "foo",
170- gid: "bar",
171- errString: `invalid value "foo" for SUDO_UID`,
172- }} {
173- os.Setenv("SUDO_UID", test.uid)
174- os.Setenv("SUDO_GID", test.gid)
175- uid, gid, err := utils.SudoCallerIds()
176- if test.errString == "" {
177- c.Assert(err, gc.IsNil)
178- c.Assert(uid, gc.Equals, test.expectedUid)
179- c.Assert(gid, gc.Equals, test.expectedGid)
180- } else {
181- c.Assert(err, gc.ErrorMatches, test.errString)
182- c.Assert(uid, gc.Equals, 0)
183- c.Assert(gid, gc.Equals, 0)
184- }
185- }
186-}
187-
188-func (s *sudoSuite) TestMkDirForUserAsUser(c *gc.C) {
189- dir := filepath.Join(c.MkDir(), "new-dir")
190- err := utils.MkdirForUser(dir, 0755)
191- c.Assert(err, gc.IsNil)
192- c.Assert(dir, jc.IsDirectory)
193-}
194-
195-func (s *sudoSuite) setupBadSudoUid() {
196- s.PatchEnvironment("SUDO_UID", "omg")
197- s.PatchEnvironment("SUDO_GID", "omg")
198- s.PatchValue(&utils.CheckIfRoot, func() bool { return true })
199-}
200-
201-func (s *sudoSuite) setupGoodSudoUid(c *gc.C) {
202- user, err := user.Current()
203- c.Assert(err, gc.IsNil)
204- s.PatchEnvironment("SUDO_UID", user.Uid)
205- s.PatchEnvironment("SUDO_GID", user.Gid)
206- s.PatchValue(&utils.CheckIfRoot, func() bool { return true })
207-}
208-
209-func (s *sudoSuite) TestMkDirForUserRoot(c *gc.C) {
210- s.setupGoodSudoUid(c)
211- dir := filepath.Join(c.MkDir(), "new-dir")
212- err := utils.MkdirForUser(dir, 0755)
213- c.Assert(err, gc.IsNil)
214- c.Assert(dir, jc.IsDirectory)
215-}
216-
217-func (s *sudoSuite) TestMkDirForUserWithError(c *gc.C) {
218- s.setupBadSudoUid()
219- dir := filepath.Join(c.MkDir(), "new-dir")
220- err := utils.MkdirForUser(dir, 0755)
221- c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
222- // The directory is still there.
223- c.Assert(dir, jc.IsDirectory)
224-}
225-
226-func (s *sudoSuite) TestMkDirAllForUserAsUser(c *gc.C) {
227- dir := filepath.Join(c.MkDir(), "new-dir", "and-another")
228- err := utils.MkdirAllForUser(dir, 0755)
229- c.Assert(err, gc.IsNil)
230- c.Assert(dir, jc.IsDirectory)
231-}
232-
233-func (s *sudoSuite) TestMkDirAllForUserRoot(c *gc.C) {
234- s.setupGoodSudoUid(c)
235-
236- dir := filepath.Join(c.MkDir(), "new-dir", "and-another")
237- err := utils.MkdirAllForUser(dir, 0755)
238- c.Assert(err, gc.IsNil)
239- c.Assert(dir, jc.IsDirectory)
240-}
241-
242-func (s *sudoSuite) TestMkDirAllForUserWithError(c *gc.C) {
243- s.setupBadSudoUid()
244-
245- base := c.MkDir()
246- dir := filepath.Join(base, "new-dir", "and-another")
247- err := utils.MkdirAllForUser(dir, 0755)
248- c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
249- // The directory is still there.
250- c.Assert(dir, jc.IsDirectory)
251-}
252-
253-func (s *sudoSuite) TestChownToUserNotRoot(c *gc.C) {
254- // Just in case someone runs the suite as root, we don't want to fail.
255- s.PatchValue(&utils.CheckIfRoot, func() bool { return false })
256-
257- nonExistant := filepath.Join(c.MkDir(), "non-existant")
258- err := utils.ChownToUser(nonExistant)
259- c.Assert(err, gc.IsNil)
260-}
261-
262-func (s *sudoSuite) TestChownToUserBadUid(c *gc.C) {
263- s.setupBadSudoUid()
264- err := utils.ChownToUser(c.MkDir())
265- c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
266-}
267-
268-func (s *sudoSuite) TestChownToUser(c *gc.C) {
269- s.setupGoodSudoUid(c)
270- err := utils.ChownToUser(c.MkDir(), c.MkDir())
271- c.Assert(err, gc.IsNil)
272-}

Subscribers

People subscribed via source and target branches

to status/vote changes: