launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/scripted_dhcp_config_writing into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/scripted_dhcp_config_writing/+merge/122512

This branch changes the call to atomic_write to write the dhcpd config so that it uses the new script version with a sudo.

I had to replace the tests for this to not actually do the call out any more since we can't expect the tests to sudo.  Instead it uses copious mocking to check the calls to subprocess.

While I was at it, I also fixed test_script_executable so that it doesn't create a temp file, and refactored some common test code into a get_mocked_script() helper.

Finally, the script was not taking a --mode argument, so I added that too, with tests of course.
-- 
https://code.launchpad.net/~julian-edwards/maas/scripted_dhcp_config_writing/+merge/122512
Your team MAAS Maintainers is requested to review the proposed merge of lp:~julian-edwards/maas/scripted_dhcp_config_writing into lp:maas.

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2012-08-31 07:17:29 +0000
+++ src/provisioningserver/tasks.py	2012-09-03 13:07:30 +0000
@@ -26,6 +26,8 @@
 from subprocess import (
     CalledProcessError,
     check_call,
+    PIPE,
+    Popen,
     )
 
 from celery.task import task
@@ -47,7 +49,6 @@
     PowerAction,
     PowerActionFail,
     )
-from provisioningserver.utils import atomic_write
 
 # For each item passed to refresh_secrets, a refresh function to give it to.
 refresh_functions = {
@@ -308,7 +309,10 @@
     :param **kwargs: Keyword args passed to dhcp.config.get_config()
     """
     output = config.get_config(**kwargs).encode("ascii")
-    atomic_write(output, DHCP_CONFIG_FILE, mode=0744)
+    proc = Popen(
+        ["sudo", "maas-provsion", "atomic-write", "--filename",
+        DHCP_CONFIG_FILE, "--mode", "744"], stdin=PIPE)
+    proc.communicate(output)
     restart_dhcp_server()
 
 

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2012-08-31 07:17:29 +0000
+++ src/provisioningserver/tests/test_tasks.py	2012-09-03 13:07:30 +0000
@@ -15,10 +15,10 @@
 from datetime import datetime
 import os
 import random
-import stat
 from subprocess import CalledProcessError
 
 from apiclient.creds import convert_tuple_to_string
+from celeryconfig import DHCP_CONFIG_FILE
 from maastesting.celery import CeleryFixture
 from maastesting.factory import factory
 from maastesting.fakemethod import (
@@ -33,7 +33,10 @@
     cache,
     tasks,
     )
-from provisioningserver.dhcp import leases
+from provisioningserver.dhcp import (
+    config,
+    leases,
+    )
 from provisioningserver.dns.config import (
     conf,
     DNSZoneConfig,
@@ -64,7 +67,6 @@
 from testresources import FixtureResource
 from testtools.matchers import (
     Equals,
-    FileContains,
     FileExists,
     MatchesListwise,
     )
@@ -225,32 +227,31 @@
             CalledProcessError, remove_dhcp_host_map.delay,
             ip, server_address, key)
 
-    def test_write_dhcp_config_writes_config(self):
-        conf_file = self.make_file()
-        self.patch(tasks, 'DHCP_CONFIG_FILE', conf_file)
-        recorder = FakeMethod()
-        self.patch(tasks, 'check_call', recorder)
-        write_dhcp_config(**self.make_dhcp_config_params())
-        self.assertThat(
-            conf_file,
-            FileContains(
-                matcher=ContainsAll(
-                    [
-                        "next-server ",
-                        "option subnet-mask",
-                    ])))
-        self.assertEqual(
-            (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_write_dhcp_config_invokes_script_correctly(self):
+        mocked_proc = Mock()
+        mocked_popen = self.patch(
+            tasks, "Popen", Mock(return_value=mocked_proc))
+        mocked_check_call = self.patch(tasks, "check_call")
+
+        config_params = self.make_dhcp_config_params()
+        write_dhcp_config(**config_params)
+
+        # It should construcy Popen with the right parameters.
+        popen_args = mocked_popen.call_args[0][0]
+        self.assertEqual(
+            popen_args,
+            ["sudo", "maas-provsion", "atomic-write", "--filename",
+            DHCP_CONFIG_FILE, "--mode", "744"])
+
+        # It should then pass the content to communicate()
+        content = config.get_config(**config_params).encode("ascii")
+        mocked_proc.communicate.assert_called_once_with(content)
+
+        # Finally it should restart the dhcp server.
+        check_call_args = mocked_check_call.call_args
+        self.assertEqual(
+            check_call_args[0][0],
+            ['sudo', 'service', 'isc-dhcp-server', 'restart'])
 
     def test_restart_dhcp_server_sends_command(self):
         recorder = FakeMethod()

=== modified file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	2012-09-03 05:20:39 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-09-03 13:07:30 +0000
@@ -440,16 +440,27 @@
         AtomicWriteScript.add_arguments(parser)
         return parser
 
+    def get_mocked_script(self, content, filename, *args):
+        self.patch(sys, "stdin", StringIO.StringIO(content))
+        parser = self.get_parser()
+        parsed_args = parser.parse_args(*args)
+        mocked_atomic_write = self.patch(
+            provisioningserver.utils, 'atomic_write')
+        AtomicWriteScript.run(parsed_args)
+        return mocked_atomic_write
+
     def test_arg_setup(self):
         parser = self.get_parser()
         filename = factory.getRandomString()
         args = parser.parse_args((
             '--no-overwrite',
-            '--filename', filename))
+            '--filename', filename,
+            '--mode', "111"))
         self.assertThat(
             args, MatchesStructure.byEquality(
                 no_overwrite=True,
-                filename=filename))
+                filename=filename,
+                mode="111"))
 
     def test_filename_arg_required(self):
         parser = self.get_parser()
@@ -466,26 +477,42 @@
             os.path.dirname(provisioningserver.__file__),
             os.pardir, os.pardir)
         content = factory.getRandomString()
-        test_data_file = self.make_file(contents=content)
         script = ["%s/bin/maas-provision" % dev_root, 'atomic-write']
         target_file = self.make_file()
-        script.extend(('--filename', target_file))
-        with open(test_data_file, "rb") as stdin:
-            cmd = Popen(
-                script, stdin=stdin,
-                env=dict(PYTHONPATH=":".join(sys.path)))
-            cmd.communicate()
+        script.extend(('--filename', target_file, '--mode', '615'))
+        cmd = Popen(
+            script, stdin=PIPE,
+            env=dict(PYTHONPATH=":".join(sys.path)))
+        cmd.communicate(content)
         self.assertThat(target_file, FileContains(content))
+        self.assertEqual(0615, stat.S_IMODE(os.stat(target_file).st_mode))
 
     def test_passes_overwrite_flag(self):
         content = factory.getRandomString()
-        self.patch(sys, "stdin", StringIO.StringIO(content))
-        parser = self.get_parser()
-        filename = factory.getRandomString()
-        args = parser.parse_args(('--filename', filename, '--no-overwrite'))
-        mocked_atomic_write = self.patch(
-            provisioningserver.utils, 'atomic_write')
-        AtomicWriteScript.run(args)
-
-        mocked_atomic_write.assert_called_once_with(
-            content, filename, overwrite=False)
+        filename = factory.getRandomString()
+        mocked_atomic_write = self.get_mocked_script(
+            content, filename,
+            ('--filename', filename, '--no-overwrite'))
+
+        mocked_atomic_write.assert_called_once_with(
+            content, filename, mode=0600, overwrite=False)
+
+    def test_passes_mode_flag(self):
+        content = factory.getRandomString()
+        filename = factory.getRandomString()
+        mocked_atomic_write = self.get_mocked_script(
+            content, filename,
+            ('--filename', filename, '--mode', "744"))
+
+        mocked_atomic_write.assert_called_once_with(
+            content, filename, mode=0744, overwrite=True)
+
+    def test_default_mode(self):
+        content = factory.getRandomString()
+        filename = factory.getRandomString()
+        mocked_atomic_write = self.get_mocked_script(
+            content, filename,
+            ('--filename', filename))
+
+        mocked_atomic_write.assert_called_once_with(
+            content, filename, mode=0600, overwrite=True)

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-08-31 23:16:59 +0000
+++ src/provisioningserver/utils.py	2012-09-03 13:07:30 +0000
@@ -333,10 +333,19 @@
         parser.add_argument(
             "--filename", action="store", required=True, help=(
             "The name of the file in which to store contents of stdin"))
+        parser.add_argument(
+            "--mode", action="store", required=False, default=None, help=(
+            "The permissions to set on the file. If not set will be r/w only "
+            "to owner"))
 
     @staticmethod
     def run(args):
         """Take content from stdin and write it atomically to a file."""
         content = sys.stdin.read()
+        if args.mode is not None:
+            mode = int(args.mode, 8)
+        else:
+            mode = 0600
         atomic_write(
-            content, args.filename, overwrite=not args.no_overwrite)
+            content, args.filename, overwrite=not args.no_overwrite,
+            mode=mode)