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

[Gavin Panella]

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

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

=== modified file 'scripts/maas-delete-file'
--- scripts/maas-delete-file	2017-03-23 08:41:59 +0000
+++ scripts/maas-delete-file	2017-03-23 08:41:59 +0000
@@ -9,6 +9,7 @@
 """
 
 import argparse
+import os
 import pipes
 
 from provisioningserver.utils.fs import atomic_delete
@@ -20,6 +21,18 @@
 }
 
 
+# For DEVELOPMENT ONLY update the paths in the white list to all be prefixed
+# with MAAS_ROOT, if defined. Check real and effective UIDs to be super extra
+# paranoid (only the latter actually matters).
+if os.getuid() != 0 and os.geteuid() != 0:
+    root = os.environ.get("MAAS_ROOT")
+    if root is not None:
+        whitelist = {
+            os.path.abspath(root + os.sep + path)
+            for path in whitelist
+        }
+
+
 arg_parser = argparse.ArgumentParser(description=__doc__)
 arg_parser.add_argument("filename", help="The file to delete.")
 

=== modified file 'scripts/maas-write-file'
--- scripts/maas-write-file	2017-03-23 08:41:59 +0000
+++ scripts/maas-write-file	2017-03-23 08:41:59 +0000
@@ -10,6 +10,7 @@
 """
 
 import argparse
+import os
 import pipes
 import sys
 
@@ -26,6 +27,18 @@
 }
 
 
+# For DEVELOPMENT ONLY update the paths in the white list to all be prefixed
+# with MAAS_ROOT, if defined. Check real and effective UIDs to be super extra
+# paranoid (only the latter actually matters).
+if os.getuid() != 0 and os.geteuid() != 0:
+    root = os.environ.get("MAAS_ROOT")
+    if root is not None:
+        whitelist = {
+            os.path.abspath(root + os.sep + path)
+            for path in whitelist
+        }
+
+
 def octal(string):
     """Parse `string` as an octal number."""
     return int(string, 8)

=== modified file 'src/provisioningserver/utils/fs.py'
--- src/provisioningserver/utils/fs.py	2017-01-28 00:51:47 +0000
+++ src/provisioningserver/utils/fs.py	2017-03-23 08:41:59 +0000
@@ -9,6 +9,7 @@
     'atomic_symlink',
     'atomic_write',
     'FileLock',
+    'get_library_script_path',
     'incremental_write',
     'NamedLock',
     'read_text_file',
@@ -70,6 +71,28 @@
         return "maas-rack"
 
 
+def get_library_script_path(name):
+    """Return path to a "library script".
+
+    By convention (here) scripts are always installed to ``/usr/lib/maas`` on
+    the target machine.
+
+    The FHS (Filesystem Hierarchy Standard) defines ``/usr/lib/`` as the
+    location for libraries used by binaries in ``/usr/bin`` and ``/usr/sbin``,
+    hence the term "library script".
+
+    In production mode this will return ``/usr/lib/maas/$name``, but in
+    development mode it will return ``$root/scripts/$name``.
+    """
+    # Avoid circular imports.
+    from provisioningserver.config import is_dev_environment
+    if is_dev_environment():
+        from maastesting import root
+        return os.path.join(root, "scripts", name)
+    else:
+        return os.path.join("/usr/lib/maas", name)
+
+
 def _write_temp_file(content, filename):
     """Write the given `content` in a temporary file next to `filename`."""
     # Write the file to a temporary place (next to the target destination,
@@ -261,6 +284,16 @@
     atomic_write(content, filename, mode=mode)
 
 
+def _with_dev_python(*command):
+    # Avoid circular imports.
+    from provisioningserver.config import is_dev_environment
+    if is_dev_environment():
+        from maastesting import root
+        interpreter = os.path.join(root, "bin", "py")
+        command = interpreter, *command
+    return command
+
+
 def sudo_write_file(filename, contents, mode=0o644):
     """Write (or overwrite) file as root.  USE WITH EXTREME CARE.
 
@@ -271,13 +304,8 @@
     """
     if not isinstance(contents, bytes):
         raise TypeError("Content must be bytes, got: %r" % (contents, ))
-    maas_provision_cmd = get_maas_provision_command()
-    command = [
-        maas_provision_cmd,
-        'atomic-write',
-        '--filename', filename,
-        '--mode', "%.4o" % mode,
-    ]
+    maas_write_file = get_library_script_path("maas-write-file")
+    command = _with_dev_python(maas_write_file, filename, "%.4o" % mode)
     proc = Popen(sudo(command), stdin=PIPE)
     stdout, stderr = proc.communicate(contents)
     if proc.returncode != 0:
@@ -290,12 +318,8 @@
     Runs an atomic update using non-interactive `sudo`.  This will fail if
     it needs to prompt for a password.
     """
-    maas_provision_cmd = get_maas_provision_command()
-    command = [
-        maas_provision_cmd,
-        'atomic-delete',
-        '--filename', filename,
-    ]
+    maas_delete_file = get_library_script_path("maas-delete-file")
+    command = _with_dev_python(maas_delete_file, filename)
     proc = Popen(sudo(command))
     stdout, stderr = proc.communicate()
     if proc.returncode != 0:

=== modified file 'src/provisioningserver/utils/tests/test_fs.py'
--- src/provisioningserver/utils/tests/test_fs.py	2017-03-23 08:41:59 +0000
+++ src/provisioningserver/utils/tests/test_fs.py	2017-03-23 08:41:59 +0000
@@ -40,13 +40,17 @@
 from maastesting.testcase import MAASTestCase
 from maastesting.utils import age_file
 import provisioningserver.config
-from provisioningserver.path import get_tentative_path
+from provisioningserver.path import (
+    get_path,
+    get_tentative_path,
+)
 from provisioningserver.utils.fs import (
     atomic_copy,
     atomic_delete,
     atomic_symlink,
     atomic_write,
     FileLock,
+    get_library_script_path,
     get_maas_provision_command,
     incremental_write,
     NamedLock,
@@ -380,6 +384,26 @@
             get_maas_provision_command())
 
 
+class TestGetLibraryScriptPath(MAASTestCase):
+    """Tests for `get_library_script_path`."""
+
+    def test__returns_usr_lib_maas_name_for_production(self):
+        self.patch(provisioningserver.config, "is_dev_environment")
+        provisioningserver.config.is_dev_environment.return_value = False
+        script_name = factory.make_name("script")
+        self.assertEqual(
+            "/usr/lib/maas/" + script_name,
+            get_library_script_path(script_name))
+
+    def test__returns_full_path_for_development(self):
+        self.patch(provisioningserver.config, "is_dev_environment")
+        provisioningserver.config.is_dev_environment.return_value = True
+        script_name = factory.make_name("script")
+        self.assertEqual(
+            root.rstrip("/") + "/scripts/" + script_name,
+            get_library_script_path(script_name))
+
+
 def patch_popen(test, returncode=0):
     process = test.patch_autospec(fs_module, 'Popen').return_value
     process.communicate.return_value = 'output', 'error output'
@@ -388,27 +412,32 @@
 
 
 def patch_sudo(test):
-    # Ensure that the `sudo` function always prepends a call to `sudo -n`
-    # to the command; is_develoo_mode and is_dev_environment will
-    # otherwise influence it.
+    # Ensure that the `sudo` function always prepends a call to `sudo -n` to
+    # the command; is_dev_environment will otherwise influence it.
     sudo = test.patch_autospec(fs_module, "sudo")
     sudo.side_effect = lambda command: ["sudo", "-n", *command]
 
 
+def patch_dev(test, is_dev_environment):
+    ide = test.patch_autospec(provisioningserver.config, "is_dev_environment")
+    ide.return_value = bool(is_dev_environment)
+
+
 class TestSudoWriteFile(MAASTestCase):
     """Testing for `sudo_write_file`."""
 
     def test_calls_atomic_write(self):
         patch_popen(self)
         patch_sudo(self)
+        patch_dev(self, False)
 
         path = os.path.join(self.make_dir(), factory.make_name('file'))
         contents = factory.make_bytes()
         sudo_write_file(path, contents)
 
         self.assertThat(fs_module.Popen, MockCalledOnceWith(
-            ['sudo', '-n', get_maas_provision_command(), 'atomic-write',
-             '--filename', path, '--mode', '0644'], stdin=PIPE))
+            ['sudo', '-n', get_library_script_path("maas-write-file"),
+             path, "0644"], stdin=PIPE))
 
     def test_rejects_non_bytes_contents(self):
         self.assertRaises(
@@ -421,6 +450,14 @@
             CalledProcessError,
             sudo_write_file, self.make_file(), factory.make_bytes())
 
+    def test_can_write_file_in_development(self):
+        filename = get_path("/var/lib/maas/dhcpd.conf")
+        contents = factory.make_bytes()  # Binary safe.
+        mode = random.randint(0o000, 0o777) | 0o400  # Always u+r.
+        sudo_write_file(filename, contents, mode)
+        self.assertThat(filename, FileContains(contents))
+        self.assertThat(os.stat(filename).st_mode & 0o777, Equals(mode))
+
 
 class TestSudoDeleteFile(MAASTestCase):
     """Testing for `sudo_delete_file`."""
@@ -428,13 +465,13 @@
     def test_calls_atomic_delete(self):
         patch_popen(self)
         patch_sudo(self)
+        patch_dev(self, False)
 
         path = os.path.join(self.make_dir(), factory.make_name('file'))
         sudo_delete_file(path)
 
         self.assertThat(fs_module.Popen, MockCalledOnceWith(
-            ['sudo', '-n', get_maas_provision_command(), 'atomic-delete',
-             '--filename', path]))
+            ['sudo', '-n', get_library_script_path("maas-delete-file"), path]))
 
     def test_catches_failures(self):
         patch_popen(self, 1)
@@ -442,6 +479,13 @@
             CalledProcessError,
             sudo_delete_file, self.make_file())
 
+    def test_can_delete_file_in_development(self):
+        filename = get_path("/var/lib/maas/dhcpd.conf")
+        with open(filename, "wb") as fd:
+            fd.write(factory.make_bytes())
+        sudo_delete_file(filename)
+        self.assertThat(filename, Not(FileExists()))
+
 
 def load_script(filename):
     """Load the Python script at `filename` into a new module."""