Merge lp:~allenap/maas/use-atomic-delete-and-write-scripts into lp:maas/trunk

Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: 5840
Merged at revision: 5850
Proposed branch: lp:~allenap/maas/use-atomic-delete-and-write-scripts
Merge into: lp:maas/trunk
Prerequisite: lp:~allenap/maas/atomic-delete-and-write-scripts
Diff against target: 283 lines (+115/-21)
4 files modified
scripts/maas-delete-file (+13/-0)
scripts/maas-write-file (+13/-0)
src/provisioningserver/utils/fs.py (+37/-13)
src/provisioningserver/utils/tests/test_fs.py (+52/-8)
To merge this branch: bzr merge lp:~allenap/maas/use-atomic-delete-and-write-scripts
Reviewer Review Type Date Requested Status
Blake Rouse (community) Approve
Review via email: mp+320769@code.launchpad.net

Commit message

Use the new maas-delete-file and maas-write-file commands.

To post a comment you must log in.
Revision history for this message
Blake Rouse (blake-rouse) wrote :

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'scripts/maas-delete-file'
2--- scripts/maas-delete-file 2017-03-23 08:41:59 +0000
3+++ scripts/maas-delete-file 2017-03-23 08:41:59 +0000
4@@ -9,6 +9,7 @@
5 """
6
7 import argparse
8+import os
9 import pipes
10
11 from provisioningserver.utils.fs import atomic_delete
12@@ -20,6 +21,18 @@
13 }
14
15
16+# For DEVELOPMENT ONLY update the paths in the white list to all be prefixed
17+# with MAAS_ROOT, if defined. Check real and effective UIDs to be super extra
18+# paranoid (only the latter actually matters).
19+if os.getuid() != 0 and os.geteuid() != 0:
20+ root = os.environ.get("MAAS_ROOT")
21+ if root is not None:
22+ whitelist = {
23+ os.path.abspath(root + os.sep + path)
24+ for path in whitelist
25+ }
26+
27+
28 arg_parser = argparse.ArgumentParser(description=__doc__)
29 arg_parser.add_argument("filename", help="The file to delete.")
30
31
32=== modified file 'scripts/maas-write-file'
33--- scripts/maas-write-file 2017-03-23 08:41:59 +0000
34+++ scripts/maas-write-file 2017-03-23 08:41:59 +0000
35@@ -10,6 +10,7 @@
36 """
37
38 import argparse
39+import os
40 import pipes
41 import sys
42
43@@ -26,6 +27,18 @@
44 }
45
46
47+# For DEVELOPMENT ONLY update the paths in the white list to all be prefixed
48+# with MAAS_ROOT, if defined. Check real and effective UIDs to be super extra
49+# paranoid (only the latter actually matters).
50+if os.getuid() != 0 and os.geteuid() != 0:
51+ root = os.environ.get("MAAS_ROOT")
52+ if root is not None:
53+ whitelist = {
54+ os.path.abspath(root + os.sep + path)
55+ for path in whitelist
56+ }
57+
58+
59 def octal(string):
60 """Parse `string` as an octal number."""
61 return int(string, 8)
62
63=== modified file 'src/provisioningserver/utils/fs.py'
64--- src/provisioningserver/utils/fs.py 2017-01-28 00:51:47 +0000
65+++ src/provisioningserver/utils/fs.py 2017-03-23 08:41:59 +0000
66@@ -9,6 +9,7 @@
67 'atomic_symlink',
68 'atomic_write',
69 'FileLock',
70+ 'get_library_script_path',
71 'incremental_write',
72 'NamedLock',
73 'read_text_file',
74@@ -70,6 +71,28 @@
75 return "maas-rack"
76
77
78+def get_library_script_path(name):
79+ """Return path to a "library script".
80+
81+ By convention (here) scripts are always installed to ``/usr/lib/maas`` on
82+ the target machine.
83+
84+ The FHS (Filesystem Hierarchy Standard) defines ``/usr/lib/`` as the
85+ location for libraries used by binaries in ``/usr/bin`` and ``/usr/sbin``,
86+ hence the term "library script".
87+
88+ In production mode this will return ``/usr/lib/maas/$name``, but in
89+ development mode it will return ``$root/scripts/$name``.
90+ """
91+ # Avoid circular imports.
92+ from provisioningserver.config import is_dev_environment
93+ if is_dev_environment():
94+ from maastesting import root
95+ return os.path.join(root, "scripts", name)
96+ else:
97+ return os.path.join("/usr/lib/maas", name)
98+
99+
100 def _write_temp_file(content, filename):
101 """Write the given `content` in a temporary file next to `filename`."""
102 # Write the file to a temporary place (next to the target destination,
103@@ -261,6 +284,16 @@
104 atomic_write(content, filename, mode=mode)
105
106
107+def _with_dev_python(*command):
108+ # Avoid circular imports.
109+ from provisioningserver.config import is_dev_environment
110+ if is_dev_environment():
111+ from maastesting import root
112+ interpreter = os.path.join(root, "bin", "py")
113+ command = interpreter, *command
114+ return command
115+
116+
117 def sudo_write_file(filename, contents, mode=0o644):
118 """Write (or overwrite) file as root. USE WITH EXTREME CARE.
119
120@@ -271,13 +304,8 @@
121 """
122 if not isinstance(contents, bytes):
123 raise TypeError("Content must be bytes, got: %r" % (contents, ))
124- maas_provision_cmd = get_maas_provision_command()
125- command = [
126- maas_provision_cmd,
127- 'atomic-write',
128- '--filename', filename,
129- '--mode', "%.4o" % mode,
130- ]
131+ maas_write_file = get_library_script_path("maas-write-file")
132+ command = _with_dev_python(maas_write_file, filename, "%.4o" % mode)
133 proc = Popen(sudo(command), stdin=PIPE)
134 stdout, stderr = proc.communicate(contents)
135 if proc.returncode != 0:
136@@ -290,12 +318,8 @@
137 Runs an atomic update using non-interactive `sudo`. This will fail if
138 it needs to prompt for a password.
139 """
140- maas_provision_cmd = get_maas_provision_command()
141- command = [
142- maas_provision_cmd,
143- 'atomic-delete',
144- '--filename', filename,
145- ]
146+ maas_delete_file = get_library_script_path("maas-delete-file")
147+ command = _with_dev_python(maas_delete_file, filename)
148 proc = Popen(sudo(command))
149 stdout, stderr = proc.communicate()
150 if proc.returncode != 0:
151
152=== modified file 'src/provisioningserver/utils/tests/test_fs.py'
153--- src/provisioningserver/utils/tests/test_fs.py 2017-03-23 08:41:59 +0000
154+++ src/provisioningserver/utils/tests/test_fs.py 2017-03-23 08:41:59 +0000
155@@ -40,13 +40,17 @@
156 from maastesting.testcase import MAASTestCase
157 from maastesting.utils import age_file
158 import provisioningserver.config
159-from provisioningserver.path import get_tentative_path
160+from provisioningserver.path import (
161+ get_path,
162+ get_tentative_path,
163+)
164 from provisioningserver.utils.fs import (
165 atomic_copy,
166 atomic_delete,
167 atomic_symlink,
168 atomic_write,
169 FileLock,
170+ get_library_script_path,
171 get_maas_provision_command,
172 incremental_write,
173 NamedLock,
174@@ -380,6 +384,26 @@
175 get_maas_provision_command())
176
177
178+class TestGetLibraryScriptPath(MAASTestCase):
179+ """Tests for `get_library_script_path`."""
180+
181+ def test__returns_usr_lib_maas_name_for_production(self):
182+ self.patch(provisioningserver.config, "is_dev_environment")
183+ provisioningserver.config.is_dev_environment.return_value = False
184+ script_name = factory.make_name("script")
185+ self.assertEqual(
186+ "/usr/lib/maas/" + script_name,
187+ get_library_script_path(script_name))
188+
189+ def test__returns_full_path_for_development(self):
190+ self.patch(provisioningserver.config, "is_dev_environment")
191+ provisioningserver.config.is_dev_environment.return_value = True
192+ script_name = factory.make_name("script")
193+ self.assertEqual(
194+ root.rstrip("/") + "/scripts/" + script_name,
195+ get_library_script_path(script_name))
196+
197+
198 def patch_popen(test, returncode=0):
199 process = test.patch_autospec(fs_module, 'Popen').return_value
200 process.communicate.return_value = 'output', 'error output'
201@@ -388,27 +412,32 @@
202
203
204 def patch_sudo(test):
205- # Ensure that the `sudo` function always prepends a call to `sudo -n`
206- # to the command; is_develoo_mode and is_dev_environment will
207- # otherwise influence it.
208+ # Ensure that the `sudo` function always prepends a call to `sudo -n` to
209+ # the command; is_dev_environment will otherwise influence it.
210 sudo = test.patch_autospec(fs_module, "sudo")
211 sudo.side_effect = lambda command: ["sudo", "-n", *command]
212
213
214+def patch_dev(test, is_dev_environment):
215+ ide = test.patch_autospec(provisioningserver.config, "is_dev_environment")
216+ ide.return_value = bool(is_dev_environment)
217+
218+
219 class TestSudoWriteFile(MAASTestCase):
220 """Testing for `sudo_write_file`."""
221
222 def test_calls_atomic_write(self):
223 patch_popen(self)
224 patch_sudo(self)
225+ patch_dev(self, False)
226
227 path = os.path.join(self.make_dir(), factory.make_name('file'))
228 contents = factory.make_bytes()
229 sudo_write_file(path, contents)
230
231 self.assertThat(fs_module.Popen, MockCalledOnceWith(
232- ['sudo', '-n', get_maas_provision_command(), 'atomic-write',
233- '--filename', path, '--mode', '0644'], stdin=PIPE))
234+ ['sudo', '-n', get_library_script_path("maas-write-file"),
235+ path, "0644"], stdin=PIPE))
236
237 def test_rejects_non_bytes_contents(self):
238 self.assertRaises(
239@@ -421,6 +450,14 @@
240 CalledProcessError,
241 sudo_write_file, self.make_file(), factory.make_bytes())
242
243+ def test_can_write_file_in_development(self):
244+ filename = get_path("/var/lib/maas/dhcpd.conf")
245+ contents = factory.make_bytes() # Binary safe.
246+ mode = random.randint(0o000, 0o777) | 0o400 # Always u+r.
247+ sudo_write_file(filename, contents, mode)
248+ self.assertThat(filename, FileContains(contents))
249+ self.assertThat(os.stat(filename).st_mode & 0o777, Equals(mode))
250+
251
252 class TestSudoDeleteFile(MAASTestCase):
253 """Testing for `sudo_delete_file`."""
254@@ -428,13 +465,13 @@
255 def test_calls_atomic_delete(self):
256 patch_popen(self)
257 patch_sudo(self)
258+ patch_dev(self, False)
259
260 path = os.path.join(self.make_dir(), factory.make_name('file'))
261 sudo_delete_file(path)
262
263 self.assertThat(fs_module.Popen, MockCalledOnceWith(
264- ['sudo', '-n', get_maas_provision_command(), 'atomic-delete',
265- '--filename', path]))
266+ ['sudo', '-n', get_library_script_path("maas-delete-file"), path]))
267
268 def test_catches_failures(self):
269 patch_popen(self, 1)
270@@ -442,6 +479,13 @@
271 CalledProcessError,
272 sudo_delete_file, self.make_file())
273
274+ def test_can_delete_file_in_development(self):
275+ filename = get_path("/var/lib/maas/dhcpd.conf")
276+ with open(filename, "wb") as fd:
277+ fd.write(factory.make_bytes())
278+ sudo_delete_file(filename)
279+ self.assertThat(filename, Not(FileExists()))
280+
281
282 def load_script(filename):
283 """Load the Python script at `filename` into a new module."""