Merge lp:~smoser/cloud-init/trunk.mcollective-cleanup into lp:~cloud-init-dev/cloud-init/trunk

[Scott Moser]

[Sergii Golovatiuk (sgolovatiuk)]

Refactoring looks good to me. It doesn't break anything. Also, Scott added more unit tests which is really good.

review: Approve

[lp:~smoser/cloud-init/trunk.mcollective-cleanup]

1258. By Scott Moser on 2016-07-19

move most of handle to apply_config, change tests.

1259. By Scott Moser on 2016-07-19

test dictionary in config

1260. By Scott Moser on 2016-07-19

whitespace

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:1260
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~smoser/cloud-init/trunk.mcollective-cleanup/+merge/300394/+edit-commit-message

https://server-team-jenkins.canonical.com/job/cloud-init-ci/27/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/lp-vote-on-merge/1/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/cloud-init-ci/27/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

this got merged at 4264783ae982e48d184e5991a67deab6f63a9054

=== modified file 'cloudinit/config/cc_mcollective.py'
--- cloudinit/config/cc_mcollective.py	2016-07-14 15:28:46 +0000
+++ cloudinit/config/cc_mcollective.py	2016-07-19 15:17:25 +0000
@@ -36,67 +36,75 @@
 LOG = logging.getLogger(__name__)
 
 
-def configure(config):
+def configure(config, server_cfg=SERVER_CFG,
+              pubcert_file=PUBCERT_FILE, pricert_file=PRICERT_FILE):
     # Read server.cfg values from the
     # original file in order to be able to mix the rest up
     try:
-        mcollective_config = ConfigObj(SERVER_CFG, file_error=True)
+        mcollective_config = ConfigObj(server_cfg, file_error=True)
+        existed = True
     except IOError:
-        LOG.warn("Did not find file %s", SERVER_CFG)
-        mcollective_config = ConfigObj(config)
-    else:
-        for (cfg_name, cfg) in config.items():
-            if cfg_name == 'public-cert':
-                util.write_file(PUBCERT_FILE, cfg, mode=0o644)
-                mcollective_config[
-                    'plugin.ssl_server_public'] = PUBCERT_FILE
-                mcollective_config['securityprovider'] = 'ssl'
-            elif cfg_name == 'private-cert':
-                util.write_file(PRICERT_FILE, cfg, mode=0o600)
-                mcollective_config[
-                    'plugin.ssl_server_private'] = PRICERT_FILE
-                mcollective_config['securityprovider'] = 'ssl'
+        LOG.debug("Did not find file %s", server_cfg)
+        mcollective_config = ConfigObj()
+        existed = False
+
+    for (cfg_name, cfg) in config.items():
+        if cfg_name == 'public-cert':
+            util.write_file(pubcert_file, cfg, mode=0o644)
+            mcollective_config[
+                'plugin.ssl_server_public'] = pubcert_file
+            mcollective_config['securityprovider'] = 'ssl'
+        elif cfg_name == 'private-cert':
+            util.write_file(pricert_file, cfg, mode=0o600)
+            mcollective_config[
+                'plugin.ssl_server_private'] = pricert_file
+            mcollective_config['securityprovider'] = 'ssl'
+        else:
+            if isinstance(cfg, six.string_types):
+                # Just set it in the 'main' section
+                mcollective_config[cfg_name] = cfg
+            elif isinstance(cfg, (dict)):
+                # Iterate through the config items, create a section if
+                # it is needed and then add/or create items as needed
+                if cfg_name not in mcollective_config.sections:
+                    mcollective_config[cfg_name] = {}
+                for (o, v) in cfg.items():
+                    mcollective_config[cfg_name][o] = v
             else:
-                if isinstance(cfg, six.string_types):
-                    # Just set it in the 'main' section
-                    mcollective_config[cfg_name] = cfg
-                elif isinstance(cfg, (dict)):
-                    # Iterate through the config items, create a section if
-                    # it is needed and then add/or create items as needed
-                    if cfg_name not in mcollective_config.sections:
-                        mcollective_config[cfg_name] = {}
-                    for (o, v) in cfg.items():
-                        mcollective_config[cfg_name][o] = v
-                else:
-                    # Otherwise just try to convert it to a string
-                    mcollective_config[cfg_name] = str(cfg)
+                # Otherwise just try to convert it to a string
+                mcollective_config[cfg_name] = str(cfg)
+
+    if existed:
         # We got all our config as wanted we'll rename
         # the previous server.cfg and create our new one
-        util.rename(SERVER_CFG, "%s.old" % (SERVER_CFG))
+        util.rename(server_cfg, "%s.old" % (server_cfg))
 
     # Now we got the whole file, write to disk...
     contents = BytesIO()
     mcollective_config.write(contents)
-    contents = contents.getvalue()
-    util.write_file(SERVER_CFG, contents, mode=0o644)
+    util.write_file(server_cfg, contents.getvalue(), mode=0o644)
+
+
+def apply_config(config, install_packages):
+    # Start by installing the mcollective package ...
+    install_packages(("mcollective",))
+
+    # ... and then update the mcollective configuration
+    if 'conf' in config:
+        configure(config=config['conf'])
+
+    # restart mcollective to handle updated config
+    util.subp(['service', 'mcollective', 'restart'], capture=False)
+    return True
 
 
 def handle(name, cfg, cloud, log, _args):
-
     # If there isn't a mcollective key in the configuration don't do anything
     if 'mcollective' not in cfg:
         log.debug(("Skipping module named %s, "
                    "no 'mcollective' key in configuration"), name)
         return
 
-    mcollective_cfg = cfg['mcollective']
-
-    # Start by installing the mcollective package ...
-    cloud.distro.install_packages(("mcollective",))
-
-    # ... and then update the mcollective configuration
-    if 'conf' in mcollective_cfg:
-        configure(config=mcollective_cfg['conf'])
-
-    # Start mcollective
-    util.subp(['service', 'mcollective', 'start'], capture=False)
+    return apply_config(
+        config=cfg['mcollective'],
+        install_packages=cloud.distro.install_packages)

=== modified file 'tests/unittests/test_handler/test_handler_mcollective.py'
--- tests/unittests/test_handler/test_handler_mcollective.py	2016-07-14 15:28:46 +0000
+++ tests/unittests/test_handler/test_handler_mcollective.py	2016-07-19 15:17:25 +0000
@@ -1,10 +1,11 @@
 from cloudinit.config import cc_mcollective
 from cloudinit import util
 
-from .. import helpers
+from ..helpers import mock, FilesystemMockingTestCase, TestCase
 
 import configobj
 import logging
+import os
 import shutil
 from six import BytesIO
 import tempfile
@@ -12,11 +13,43 @@
 LOG = logging.getLogger(__name__)
 
 
-class TestConfig(helpers.FilesystemMockingTestCase):
+STOCK_CONFIG = """\
+main_collective = mcollective
+collectives = mcollective
+libdir = /usr/share/mcollective/plugins
+logfile = /var/log/mcollective.log
+loglevel = info
+daemonize = 1
+
+# Plugins
+securityprovider = psk
+plugin.psk = unset
+
+connector = activemq
+plugin.activemq.pool.size = 1
+plugin.activemq.pool.1.host = stomp1
+plugin.activemq.pool.1.port = 61613
+plugin.activemq.pool.1.user = mcollective
+plugin.activemq.pool.1.password = marionette
+
+# Facts
+factsource = yaml
+plugin.yaml = /etc/mcollective/facts.yaml
+"""
+
+
+class TestConfig(FilesystemMockingTestCase):
     def setUp(self):
         super(TestConfig, self).setUp()
         self.tmp = tempfile.mkdtemp()
         self.addCleanup(shutil.rmtree, self.tmp)
+        # "./": make os.path.join behave correctly with abs path as second arg
+        self.server_cfg = os.path.join(
+            self.tmp, "./" + cc_mcollective.SERVER_CFG)
+        self.pubcert_file = os.path.join(
+            self.tmp, "./" + cc_mcollective.PUBCERT_FILE)
+        self.pricert_file = os.path.join(
+            self.tmp, self.tmp, "./" + cc_mcollective.PRICERT_FILE)
 
     def test_basic_config(self):
         cfg = {
@@ -38,23 +71,96 @@
                 },
             },
         }
+        expected = cfg['mcollective']['conf']
+
         self.patchUtils(self.tmp)
         cc_mcollective.configure(cfg['mcollective']['conf'])
-        contents = util.load_file("/etc/mcollective/server.cfg", decode=False)
+        contents = util.load_file(cc_mcollective.SERVER_CFG, decode=False)
         contents = configobj.ConfigObj(BytesIO(contents))
-        expected = {
-            'loglevel': 'debug',
-            'connector': 'rabbitmq',
-            'logfile': '/var/log/mcollective.log',
-            'ttl': '4294957',
-            'collectives': 'mcollective',
-            'main_collective': 'mcollective',
-            'securityprovider': 'psk',
-            'daemonize': '1',
-            'factsource': 'yaml',
-            'direct_addressing': '1',
-            'plugin.psk': 'unset',
-            'libdir': '/usr/share/mcollective/plugins',
-            'identity': '1',
-        }
         self.assertEqual(expected, dict(contents))
+
+    def test_existing_config_is_saved(self):
+        cfg = {'loglevel': 'warn'}
+        util.write_file(self.server_cfg, STOCK_CONFIG)
+        cc_mcollective.configure(config=cfg, server_cfg=self.server_cfg)
+        self.assertTrue(os.path.exists(self.server_cfg))
+        self.assertTrue(os.path.exists(self.server_cfg + ".old"))
+        self.assertEqual(util.load_file(self.server_cfg + ".old"),
+                         STOCK_CONFIG)
+
+    def test_existing_updated(self):
+        cfg = {'loglevel': 'warn'}
+        util.write_file(self.server_cfg, STOCK_CONFIG)
+        cc_mcollective.configure(config=cfg, server_cfg=self.server_cfg)
+        cfgobj = configobj.ConfigObj(self.server_cfg)
+        self.assertEqual(cfg['loglevel'], cfgobj['loglevel'])
+
+    def test_dictionary_in_config(self):
+        cfg = {'loglevel': 'debug', 'subfoo': {'subkey': 'subval'}}
+        cc_mcollective.configure(config=cfg, server_cfg=self.server_cfg)
+        cfgobj = configobj.ConfigObj(self.server_cfg)
+        self.assertEqual(cfg['loglevel'], cfgobj['loglevel'])
+        contents = util.load_file(self.server_cfg)
+        self.assertIn('[subfoo]', contents)
+        self.assertEqual(cfgobj['subfoo']['subkey'], 'subval')
+
+    def test_certificats_written(self):
+        # check public-cert and private-cert keys in config get written
+        cfg = {'loglevel': 'debug',
+               'public-cert': "this is my public-certificate",
+               'private-cert': "secret private certificate"}
+
+        cc_mcollective.configure(
+            config=cfg, server_cfg=self.server_cfg,
+            pricert_file=self.pricert_file, pubcert_file=self.pubcert_file)
+
+        found = configobj.ConfigObj(self.server_cfg)
+
+        # make sure these didnt get written in
+        self.assertFalse('public-cert' in found)
+        self.assertFalse('private-cert' in found)
+
+        # these need updating to the specified paths
+        self.assertEqual(found['plugin.ssl_server_public'], self.pubcert_file)
+        self.assertEqual(found['plugin.ssl_server_private'], self.pricert_file)
+
+        # and the security provider should be ssl
+        self.assertEqual(found['securityprovider'], 'ssl')
+
+        self.assertEqual(
+            util.load_file(self.pricert_file), cfg['private-cert'])
+        self.assertEqual(
+            util.load_file(self.pubcert_file), cfg['public-cert'])
+
+
+class TestApplyConfig(TestCase):
+    def setUp(self):
+        super(TestApplyConfig, self).setUp()
+        self.installed_packages = []
+
+    def _install_packages(self, pkglist):
+        self.installed_packages.extend(pkglist)
+
+    @mock.patch("cloudinit.config.cc_mcollective.util.subp")
+    @mock.patch("cloudinit.config.cc_mcollective.configure")
+    def test_with_conf_configure_called(self, configure, subp):
+        mycfg = {'conf': {'loglevel': 'warn'}}
+        cc_mcollective.apply_config(
+            mycfg, install_packages=self._install_packages)
+        self.assertTrue(configure.called_once)
+        self.assertEqual(configure.call_args_list,
+                         [mock.call(config=mycfg['conf'])])
+        self.assertEqual(subp.call_args_list[0][0][0],
+                         ['service', 'mcollective', 'restart'])
+        self.assertEqual(self.installed_packages, ['mcollective'])
+
+    @mock.patch("cloudinit.config.cc_mcollective.util.subp")
+    @mock.patch("cloudinit.config.cc_mcollective.configure")
+    def test_without_conf_configure_not_called(self, configure, subp):
+        mycfg = {'foo': 'bar'}
+        cc_mcollective.apply_config(
+            mycfg, install_packages=self._install_packages)
+        self.assertFalse(configure.called)
+        self.assertEqual(subp.call_args_list[0][0][0],
+                         ['service', 'mcollective', 'restart'])
+        self.assertEqual(self.installed_packages, ['mcollective'])