launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/dns-config-permissions into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/maas/dns-config-permissions/+merge/122200

Pre-imped with Julian.  This updates atomic_write to accept a "mode" argument.  We wanted a leave-it-alone default, but due to how temporary files are created, the default is 0700 so we might as well pick a half-sensible default mode and set it unconditionally.

The branch also fixes some leaking of temporary files, and it takes the writing of the temporary file out of the locked region for simplicity.  This eliminates a potential savings for the case where a file already existed, but improves the normal case by reducing the time the lock is held.  We only used the non-overwrite mode of operation in one place, and it did not seem performance-critical.

I slapped on some documentation for good measure.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/dns-config-permissions/+merge/122200
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/dns-config-permissions into lp:maas.

=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2012-08-28 07:24:40 +0000
+++ src/provisioningserver/dns/config.py	2012-08-31 07:31:20 +0000
@@ -151,7 +151,8 @@
         template = self.get_template()
         kwargs.update(self.get_context())
         rendered = self.render_template(template, **kwargs)
-        atomic_write(rendered, self.target_path, overwrite=overwrite)
+        atomic_write(
+            rendered, self.target_path, overwrite=overwrite, mode=0744)
 
 
 class DNSConfig(DNSConfigBase):

=== modified file 'src/provisioningserver/dns/tests/test_config.py'
--- src/provisioningserver/dns/tests/test_config.py	2012-08-14 07:29:41 +0000
+++ src/provisioningserver/dns/tests/test_config.py	2012-08-31 07:31:20 +0000
@@ -14,6 +14,7 @@
 
 import os.path
 import random
+import stat
 
 from celery.conf import conf
 from maastesting.factory import factory
@@ -165,6 +166,15 @@
                         MAAS_NAMED_RNDC_CONF_NAME,
                     ])))
 
+    def test_write_config_makes_config_world_readable(self):
+        target_dir = self.make_dir()
+        self.patch(DNSConfig, 'target_dir', target_dir)
+        DNSConfig().write_config()
+        config_file = os.path.join(target_dir, MAAS_NAMED_CONF_NAME)
+        self.assertEqual(
+            stat.S_IROTH,
+            os.stat(config_file).st_mode & stat.S_IROTH)
+
     def test_get_include_snippet_returns_snippet(self):
         target_dir = self.make_dir()
         self.patch(DNSConfig, 'target_dir', target_dir)

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2012-08-23 16:36:52 +0000
+++ src/provisioningserver/tasks.py	2012-08-31 07:31:20 +0000
@@ -308,7 +308,7 @@
     :param **kwargs: Keyword args passed to dhcp.config.get_config()
     """
     output = config.get_config(**kwargs).encode("ascii")
-    atomic_write(output, DHCP_CONFIG_FILE)
+    atomic_write(output, DHCP_CONFIG_FILE, mode=0744)
     restart_dhcp_server()
 
 

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2012-08-24 08:39:18 +0000
+++ src/provisioningserver/tests/test_tasks.py	2012-08-31 07:31:20 +0000
@@ -15,6 +15,7 @@
 from datetime import datetime
 import os
 import random
+import stat
 from subprocess import CalledProcessError
 
 from apiclient.creds import convert_tuple_to_string
@@ -151,6 +152,21 @@
             recorder.extract_args()[0][0],
             ContainsAll(args))
 
+    def make_dhcp_config_params(self):
+        """Fake up a dict of dhcp configuration parameters."""
+        param_names = [
+             'omapi_key',
+             'subnet',
+             'subnet_mask',
+             'next_server',
+             'broadcast_ip',
+             'dns_servers',
+             'router_ip',
+             'ip_range_low',
+             'ip_range_high',
+             ]
+        return {param: factory.getRandomString() for param in param_names}
+
     def test_upload_dhcp_leases(self):
         self.patch(
             leases, 'parse_leases_file',
@@ -210,17 +226,11 @@
             ip, server_address, key)
 
     def test_write_dhcp_config_writes_config(self):
-        conf_file = self.make_file(contents=factory.getRandomString())
+        conf_file = self.make_file()
         self.patch(tasks, 'DHCP_CONFIG_FILE', conf_file)
         recorder = FakeMethod()
         self.patch(tasks, 'check_call', recorder)
-        param_names = [
-             'omapi_key', 'subnet', 'subnet_mask', 'next_server',
-             'broadcast_ip', 'dns_servers', 'router_ip', 'ip_range_low',
-             'ip_range_high',
-             ]
-        params = {param: factory.getRandomString() for param in param_names}
-        write_dhcp_config(**params)
+        write_dhcp_config(**self.make_dhcp_config_params())
         self.assertThat(
             conf_file,
             FileContains(
@@ -233,6 +243,15 @@
             (1, (['sudo', 'service', 'isc-dhcp-server', 'restart'],)),
             (recorder.call_count, recorder.extract_args()[0]))
 
+    def test_write_dhcp_config_writes_world_readable_config(self):
+        self.patch(tasks, 'check_call', Mock())
+        conf_file = self.make_file()
+        self.patch(tasks, 'DHCP_CONFIG_FILE', conf_file)
+        write_dhcp_config(**self.make_dhcp_config_params())
+        self.assertEqual(
+            stat.S_IROTH,
+            os.stat(conf_file).st_mode & stat.S_IROTH)
+
     def test_restart_dhcp_server_sends_command(self):
         recorder = FakeMethod()
         self.patch(tasks, 'check_call', recorder)

=== modified file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	2012-08-31 06:06:26 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-08-31 07:31:20 +0000
@@ -18,6 +18,7 @@
     )
 import os
 from random import randint
+import stat
 import StringIO
 from subprocess import CalledProcessError
 import sys
@@ -27,6 +28,7 @@
 from maastesting.factory import factory
 from maastesting.fakemethod import FakeMethod
 from maastesting.testcase import TestCase
+from mock import Mock
 from provisioningserver.utils import (
     ActionScript,
     atomic_write,
@@ -39,6 +41,7 @@
     ShellTemplate,
     )
 from testtools.matchers import FileContains
+from testtools.testcase import ExpectedException
 
 
 class TestSafe(TestCase):
@@ -77,17 +80,51 @@
         atomic_write(content, filename, overwrite=False)
         self.assertThat(filename, FileContains(content))
 
-    def test_atomic_write_cleans_up_temp_file(self):
-        # If the writing of the file is skipped because overwrite is
-        # False and the file already exists, the temporary file which
-        # would have been used for the copy operation is removed.
-        content = factory.getRandomString()
-        random_content = factory.getRandomString()
-        filename = self.make_file(contents=random_content)
-        atomic_write(content, filename, overwrite=False)
-        self.assertEqual(
-            [os.path.basename(filename)],
-            os.listdir(os.path.dirname(filename)))
+    def test_atomic_write_does_not_leak_temp_file_when_not_overwriting(self):
+        # If the file is not written because it already exists and
+        # overwriting was disabled, atomic_write does not leak its
+        # temporary file.
+        filename = self.make_file()
+        atomic_write(factory.getRandomString(), filename, overwrite=False)
+        self.assertEqual(
+            [os.path.basename(filename)],
+            os.listdir(os.path.dirname(filename)))
+
+    def test_atomic_write_does_not_leak_temp_file_on_failure(self):
+        # If the overwrite fails, atomic_write does not leak its
+        # temporary file.
+        self.patch(os, 'rename', Mock(side_effect=OSError()))
+        filename = self.make_file()
+        with ExpectedException(OSError):
+            atomic_write(factory.getRandomString(), filename)
+        self.assertEqual(
+            [os.path.basename(filename)],
+            os.listdir(os.path.dirname(filename)))
+
+    def test_atomic_write_sets_permissions(self):
+        atomic_file = self.make_file()
+        # Pick an unusual mode that is also likely to fall outside our
+        # umask.  We want this mode set, not treated as advice that may
+        # be tightened up by umask later.
+        mode = 0323
+        atomic_write(factory.getRandomString(), atomic_file, mode=mode)
+        self.assertEqual(mode, stat.S_IMODE(os.stat(atomic_file).st_mode))
+
+    def test_atomic_write_sets_permissions_before_moving_into_place(self):
+
+        recorded_modes = []
+
+        def record_mode(source, dest):
+            """Stub for os.rename: get source file's access mode."""
+            recorded_modes.append(os.stat(source).st_mode)
+
+        self.patch(os, 'rename', Mock(side_effect=record_mode))
+        playground = self.make_dir()
+        atomic_file = os.path.join(playground, factory.make_name('atomic'))
+        mode = 0323
+        atomic_write(factory.getRandomString(), atomic_file, mode=mode)
+        [recorded_mode] = recorded_modes
+        self.assertEqual(mode, stat.S_IMODE(recorded_mode))
 
 
 class TestIncrementalWrite(TestCase):

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-08-24 09:21:34 +0000
+++ src/provisioningserver/utils.py	2012-08-31 07:31:20 +0000
@@ -63,26 +63,34 @@
     return temp_file
 
 
-def atomic_write(content, filename, overwrite=True):
-    """Write `content` into the file `filename` in an atomic fashion."""
-    # If not overwrite: use filelock to gain an exclusive access to the
-    # destination file.
-    if not overwrite:
-        lock = FileLock(filename)
-        # Acquire an exclusive lock on this file.
-        lock.acquire()
-        try:
-            if not os.path.isfile(filename):
-                temp_file = _write_temp_file(content, filename)
-                os.rename(temp_file, filename)
-        finally:
-            # Release the lock.
-            lock.release()
-    else:
-        # Rename the temporary file to `filename`, that operation is atomic on
-        # POSIX systems.
-        temp_file = _write_temp_file(content, filename)
-        os.rename(temp_file, filename)
+def atomic_write(content, filename, overwrite=True, mode=0600):
+    """Write `content` into the file `filename` in an atomic fashion.
+
+    This requires write permissions to the directory that `filename` is in.
+    It creates a temporary file in the same directory (so that it will be
+    on the same filesystem as the destination) and then renames it to
+    replace the original, if any.  Such a rename is atomic in POSIX.
+
+    :param overwrite: Overwrite `filename` if it already exists?  Default
+        is True.
+    :param mode: Access permissions for the file, if written.
+    """
+    temp_file = _write_temp_file(content, filename)
+    os.chmod(temp_file, mode)
+    try:
+        if overwrite:
+            os.rename(temp_file, filename)
+        else:
+            lock = FileLock(filename)
+            lock.acquire()
+            try:
+                if not os.path.isfile(filename):
+                    os.rename(temp_file, filename)
+            finally:
+                lock.release()
+    finally:
+        if os.path.isfile(temp_file):
+            os.remove(temp_file)
 
 
 def incremental_write(content, filename):