cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Chad Smith has proposed merging ~chad.smith/cloud-init:cc-landscape-py3-config-fix into cloud-init:master.

Requested reviews:
  cloud-init commiters (cloud-init-dev)
Related bugs:
  Bug #1699282 in cloud-init: "cloud-init process fails to configure puppet"
  https://bugs.launchpad.net/cloud-init/+bug/1699282

For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/329087

cc_landscape & cc_puppet: Fix six.StringIO use in writing configs

Both landscape and puppet modules had issues with the way they wrote /etc/landscape/client.conf or /etc/puppet/puppet.conf on either python3 or python2. This branch adds initial unit tests for both modules which will get better exercise under both python2 and python3.

The unit tests shed light on a few issues:
   - In the cc_landscape module py3 can't provide six.StringIO content to ConfigParser.write, so we need to ue six.BytesIO instead
   - In the cc_puppet module, python <= 2.7 doesn't support using six.StringIO as a context manager, so we drop the context manager fanciness and directly outputstream = StringIO() directly.
   - The docstring in cc_puppet is fixed to represent that the conf sub-key needs to contain valid puppet section names enclosing each key-value list.


he
This br
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:cc-landscape-py3-config-fix into cloud-init:master.

diff --git a/cloudinit/config/cc_landscape.py b/cloudinit/config/cc_landscape.py
index 86b7138..8f9f1ab 100644
--- a/cloudinit/config/cc_landscape.py
+++ b/cloudinit/config/cc_landscape.py
@@ -57,7 +57,7 @@ The following default client config is provided, but can be overridden::
 
 import os
 
-from six import StringIO
+from six import BytesIO
 
 from configobj import ConfigObj
 
@@ -109,7 +109,7 @@ def handle(_name, cfg, cloud, log, _args):
         ls_cloudcfg,
     ]
     merged = merge_together(merge_data)
-    contents = StringIO()
+    contents = BytesIO()
     merged.write(contents)
 
     util.ensure_dir(os.path.dirname(LSC_CLIENT_CFG_FILE))
diff --git a/cloudinit/config/cc_puppet.py b/cloudinit/config/cc_puppet.py
index dc11561..28b1d56 100644
--- a/cloudinit/config/cc_puppet.py
+++ b/cloudinit/config/cc_puppet.py
@@ -15,21 +15,23 @@ This module handles puppet installation and configuration. If the ``puppet``
 key does not exist in global configuration, no action will be taken. If a
 config entry for ``puppet`` is present, then by default the latest version of
 puppet will be installed. If ``install`` is set to ``false``, puppet will not
-be installed. However, this may result in an error if puppet is not already
+be installed. However, this will result in an error if puppet is not already
 present on the system. The version of puppet to be installed can be specified
 under ``version``, and defaults to ``none``, which selects the latest version
 in the repos. If the ``puppet`` config key exists in the config archive, this
 module will attempt to start puppet even if no installation was performed.
 
-Puppet configuration can be specified under the ``conf`` key. The configuration
-is specified as a dictionary which is converted into ``<key>=<value>`` format
-and appended to ``puppet.conf`` under the ``[puppetd]`` section. The
+Puppet configuration can be specified under the ``conf`` key. The
+configuration is specified as a dictionary containing high-level ``<section>``
+keys and lists of ``<key>=<value>`` pairs within each section. Each section
+name and ``<key>=<value>`` pair is written directly to ``puppet.conf``. As
+such,  section names should be one of: ``main``, ``master``, ``agent`` or
+``user`` and keys should be valid puppet configuration options. The
 ``certname`` key supports string substitutions for ``%i`` and ``%f``,
 corresponding to the instance id and fqdn of the machine respectively.
-If ``ca_cert`` is present under ``conf``, it will not be written to
-``puppet.conf``, but instead will be used as the puppermaster certificate.
-It should be specified in pem format as a multi-line string (using the ``|``
-yaml notation).
+If ``ca_cert`` is present, it will not be written to ``puppet.conf``, but
+instead will be used as the puppermaster certificate. It should be specified
+in pem format as a multi-line string (using the ``|`` yaml notation).
 
 **Internal name:** ``cc_puppet``
 
@@ -43,12 +45,13 @@ yaml notation).
         install: <true/false>
         version: <version>
         conf:
-            server: "puppetmaster.example.org"
-            certname: "%i.%f"
-            ca_cert: |
-                -------BEGIN CERTIFICATE-------
-                <cert data>
-                -------END CERTIFICATE-------
+            agent:
+                server: "puppetmaster.example.org"
+                certname: "%i.%f"
+                ca_cert: |
+                    -------BEGIN CERTIFICATE-------
+                    <cert data>
+                    -------END CERTIFICATE-------
 """
 
 from six import StringIO
@@ -127,7 +130,7 @@ def handle(name, cfg, cloud, log, _args):
                 util.write_file(PUPPET_SSL_CERT_PATH, cfg)
                 util.chownbyname(PUPPET_SSL_CERT_PATH, 'puppet', 'root')
             else:
-                # Iterate throug the config items, we'll use ConfigParser.set
+                # Iterate through the config items, we'll use ConfigParser.set
                 # to overwrite or create new items as needed
                 for (o, v) in cfg.items():
                     if o == 'certname':
diff --git a/cloudinit/helpers.py b/cloudinit/helpers.py
index f01021a..bda40bb 100644
--- a/cloudinit/helpers.py
+++ b/cloudinit/helpers.py
@@ -13,7 +13,7 @@ from time import time
 import contextlib
 import os
 
-import six
+from six import StringIO
 from six.moves.configparser import (
     NoSectionError, NoOptionError, RawConfigParser)
 
@@ -441,12 +441,12 @@ class DefaultingConfigParser(RawConfigParser):
 
     def stringify(self, header=None):
         contents = ''
-        with six.StringIO() as outputstream:
-            self.write(outputstream)
-            outputstream.flush()
-            contents = outputstream.getvalue()
-            if header:
-                contents = "\n".join([header, contents])
+        outputstream = StringIO()
+        self.write(outputstream)
+        outputstream.flush()
+        contents = outputstream.getvalue()
+        if header:
+            contents = "\n".join([header, contents])
         return contents
 
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_handler/test_handler_landscape.py b/tests/unittests/test_handler/test_handler_landscape.py
new file mode 100644
index 0000000..d35ba60
--- /dev/null
+++ b/tests/unittests/test_handler/test_handler_landscape.py
@@ -0,0 +1,129 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+
+from cloudinit.config import cc_landscape
+from cloudinit.sources import DataSourceNone
+from cloudinit import (distros, helpers, cloud, util)
+from ..helpers import FilesystemMockingTestCase, mock
+
+from configobj import ConfigObj
+import logging
+import os
+
+
+LOG = logging.getLogger(__name__)
+
+
+class TestLandscape(FilesystemMockingTestCase):
+
+    with_logs = True
+
+    def setUp(self):
+        super(TestLandscape, self).setUp()
+        self.new_root = self.tmp_dir()
+        self.conf = os.path.join(self.new_root, 'client.conf')
+        self.default_file = os.path.join(self.new_root, 'default/landscape')
+
+    def _get_cloud(self, distro):
+        self.patchUtils(self.new_root)
+        paths = helpers.Paths({'templates_dir': self.new_root})
+        cls = distros.fetch(distro)
+        mydist = cls(distro, {}, paths)
+        myds = DataSourceNone.DataSourceNone({}, mydist, paths)
+        return cloud.Cloud(myds, paths, {}, mydist, None)
+
+    def test_handler_skips_empty_landscape_cloudconfig(self):
+        """Empty landscape cloud-config section does no work."""
+        mycloud = self._get_cloud('ubuntu')
+        mycloud.distro = mock.MagicMock()
+        cfg = {'landscape': {}}
+        cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertFalse(mycloud.distro.install_packages.called)
+
+    def test_handler_error_on_invalid_landscape_type(self):
+        """Raise an error when landscape configuraiton option is invalid."""
+        mycloud = self._get_cloud('ubuntu')
+        cfg = {'landscape': 'wrongtype'}
+        with self.assertRaises(RuntimeError) as context_manager:
+            cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertIn(
+            "'landscape' key existed in config, but not a dict",
+            str(context_manager.exception))
+
+    @mock.patch('cloudinit.config.cc_landscape.util')
+    def test_handler_restarts_landscape_client(self, m_util):
+        """handler restarts lansdscape-client after install."""
+        mycloud = self._get_cloud('ubuntu')
+        cfg = {'landscape': {'client': {}}}
+        with mock.patch('cloudinit.config.cc_landscape.LSC_CLIENT_CFG_FILE',
+                        self.conf):
+                cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertEqual(
+            [mock.call(['service', 'landscape-client', 'restart'])],
+            m_util.subp.call_args_list)
+
+    def test_handler_installs_client_and_creates_config_file(self):
+        """Write landscape client.conf and install landscape-client."""
+        self.assertFalse(os.path.exists(self.conf))
+        mycloud = self._get_cloud('ubuntu')
+        cfg = {'landscape': {'client': {}}}
+        expected = {'client': {
+            'log_level': 'info',
+            'url': 'https://landscape.canonical.com/message-system',
+            'ping_url': 'http://landscape.canonical.com/ping',
+            'data_path': '/var/lib/landscape/client'}}
+        mycloud.distro = mock.MagicMock()
+        with mock.patch('cloudinit.config.cc_landscape.LSC_CLIENT_CFG_FILE',
+                        self.conf):
+            with mock.patch('cloudinit.config.cc_landscape.LS_DEFAULT_FILE',
+                            self.default_file):
+                cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertEqual(
+            [mock.call('landscape-client')],
+            mycloud.distro.install_packages.call_args)
+        self.assertEqual(expected, dict(ConfigObj(self.conf)))
+        self.assertIn(
+            'Wrote landscape config file to {0}'.format(self.conf),
+            self.logs.getvalue())
+        with open(self.default_file) as stream:
+            default_content = stream.read()
+        self.assertEqual('RUN=1\n', default_content)
+
+    def test_handler_writes_merged_client_config_file_with_defaults(self):
+        """Merge and write options from LSC_CLIENT_CFG_FILE with defaults."""
+        # Write existing sparse client.conf file
+        util.write_file(self.conf, '[client]\ncomputer_title = My PC\n')
+        mycloud = self._get_cloud('ubuntu')
+        cfg = {'landscape': {'client': {}}}
+        expected = {'client': {
+            'log_level': 'info',
+            'url': 'https://landscape.canonical.com/message-system',
+            'ping_url': 'http://landscape.canonical.com/ping',
+            'data_path': '/var/lib/landscape/client',
+            'computer_title': 'My PC'}}
+        with mock.patch("cloudinit.config.cc_landscape.LSC_CLIENT_CFG_FILE",
+                        self.conf):
+            cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertEqual(expected, dict(ConfigObj(self.conf)))
+        self.assertIn(
+            'Wrote landscape config file to {0}'.format(self.conf),
+            self.logs.getvalue())
+
+    def test_handler_writes_merged_provided_cloudconfig_with_defaults(self):
+        """Merge and write options from cloud-config options with defaults."""
+        # Write empty sparse client.conf file
+        util.write_file(self.conf, '')
+        mycloud = self._get_cloud('ubuntu')
+        cfg = {'landscape': {'client': {'computer_title': 'My PC'}}}
+        expected = {'client': {
+            'log_level': 'info',
+            'url': 'https://landscape.canonical.com/message-system',
+            'ping_url': 'http://landscape.canonical.com/ping',
+            'data_path': '/var/lib/landscape/client',
+            'computer_title': 'My PC'}}
+        with mock.patch("cloudinit.config.cc_landscape.LSC_CLIENT_CFG_FILE",
+                        self.conf):
+            cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertEqual(expected, dict(ConfigObj(self.conf)))
+        self.assertIn(
+            'Wrote landscape config file to {0}'.format(self.conf),
+            self.logs.getvalue())
diff --git a/tests/unittests/test_handler/test_handler_puppet.py b/tests/unittests/test_handler/test_handler_puppet.py
new file mode 100644
index 0000000..805c76b
--- /dev/null
+++ b/tests/unittests/test_handler/test_handler_puppet.py
@@ -0,0 +1,142 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+
+from cloudinit.config import cc_puppet
+from cloudinit.sources import DataSourceNone
+from cloudinit import (distros, helpers, cloud, util)
+from ..helpers import CiTestCase, mock
+
+import logging
+
+
+LOG = logging.getLogger(__name__)
+
+
+@mock.patch('cloudinit.config.cc_puppet.util')
+@mock.patch('cloudinit.config.cc_puppet.os')
+class TestAutostartPuppet(CiTestCase):
+
+    with_logs = True
+
+    def test_wb_autostart_puppet_updates_puppet_default(self, m_os, m_util):
+        """Update /etc/default/puppet to autostart if it exists."""
+
+        def _fake_exists(path):
+            return path == '/etc/default/puppet'
+
+        m_os.path.exists.side_effect = _fake_exists
+        cc_puppet._autostart_puppet(LOG)
+        self.assertEqual(
+            [mock.call(['sed', '-i', '-e', 's/^START=.*/START=yes/',
+                        '/etc/default/puppet'], capture=False)],
+            m_util.subp.call_args_list)
+
+    def test_wb_autostart_pupppet_enables_puppet_systemctl(self, m_os, m_util):
+        """If systemctl is present, enable puppet via systemctl."""
+
+        def _fake_exists(path):
+            return path == '/bin/systemctl'
+
+        m_os.path.exists.side_effect = _fake_exists
+        cc_puppet._autostart_puppet(LOG)
+        expected_calls = [mock.call(
+            ['/bin/systemctl', 'enable', 'puppet.service'], capture=False)]
+        self.assertEqual(expected_calls, m_util.subp.call_args_list)
+
+    def test_wb_autostart_pupppet_enables_puppet_chkconfig(self, m_os, m_util):
+        """If chkconfig is present, enable puppet via checkcfg."""
+
+        def _fake_exists(path):
+            return path == '/sbin/chkconfig'
+
+        m_os.path.exists.side_effect = _fake_exists
+        cc_puppet._autostart_puppet(LOG)
+        expected_calls = [mock.call(
+            ['/sbin/chkconfig', 'puppet', 'on'], capture=False)]
+        self.assertEqual(expected_calls, m_util.subp.call_args_list)
+
+
+@mock.patch('cloudinit.config.cc_puppet._autostart_puppet')
+class TestPuppetHandle(CiTestCase):
+
+    with_logs = True
+
+    def setUp(self):
+        super(TestPuppetHandle, self).setUp()
+        self.new_root = self.tmp_dir()
+        self.conf = self.tmp_path('puppet.conf')
+
+    def _get_cloud(self, distro):
+        paths = helpers.Paths({'templates_dir': self.new_root})
+        cls = distros.fetch(distro)
+        mydist = cls(distro, {}, paths)
+        myds = DataSourceNone.DataSourceNone({}, mydist, paths)
+        return cloud.Cloud(myds, paths, {}, mydist, None)
+
+    def test_handler_skips_missing_puppet_key_in_cloudconfig(self, m_auto):
+        """Cloud-config containing no 'puppet' key is skipped."""
+        mycloud = self._get_cloud('ubuntu')
+        cfg = {}
+        cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertIn(
+            "no 'puppet' configuration found", self.logs.getvalue())
+        self.assertEqual(0, m_auto.call_count)
+
+    @mock.patch('cloudinit.config.cc_puppet.util.subp')
+    def test_handler_puppet_config_starts_puppet_service(self, m_subp, m_auto):
+        """Cloud-config 'puppet' configuration starts puppet."""
+        mycloud = self._get_cloud('ubuntu')
+        cfg = {'puppet': {'install': False}}
+        cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertEqual(1, m_auto.call_count)
+        self.assertEqual(
+            [mock.call(['service', 'puppet', 'start'], capture=False)],
+            m_subp.call_args_list)
+
+    @mock.patch('cloudinit.config.cc_puppet.util.subp')
+    def test_handler_empty_puppet_config_installs_puppet(self, m_subp, m_auto):
+        """Cloud-config empty 'puppet' configuration installs latest puppet."""
+        mycloud = self._get_cloud('ubuntu')
+        mycloud.distro = mock.MagicMock()
+        cfg = {'puppet': {}}
+        cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertEqual(
+            [mock.call(('puppet', None))],
+            mycloud.distro.install_packages.call_args_list)
+
+    @mock.patch('cloudinit.config.cc_puppet.util.subp')
+    def test_handler_puppet_config_installs_puppet_on_true(self, m_subp, _):
+        """Cloud-config with 'puppet' key installs when 'install' is True."""
+        mycloud = self._get_cloud('ubuntu')
+        mycloud.distro = mock.MagicMock()
+        cfg = {'puppet': {'install': True}}
+        cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertEqual(
+            [mock.call(('puppet', None))],
+            mycloud.distro.install_packages.call_args_list)
+
+    @mock.patch('cloudinit.config.cc_puppet.util.subp')
+    def test_handler_puppet_config_installs_puppet_version(self, m_subp, _):
+        """Cloud-config 'puppet' configuration can specify a version."""
+        mycloud = self._get_cloud('ubuntu')
+        mycloud.distro = mock.MagicMock()
+        cfg = {'puppet': {'version': '3.8'}}
+        cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
+        self.assertEqual(
+            [mock.call(('puppet', '3.8'))],
+            mycloud.distro.install_packages.call_args_list)
+
+    @mock.patch('cloudinit.config.cc_puppet.util.subp')
+    def test_handler_puppet_config_updates_puppet_conf(self, m_subp, m_auto):
+        """When 'conf' is provided update values in PUPPET_CONF_PATH."""
+        mycloud = self._get_cloud('ubuntu')
+        cfg = {
+            'puppet': {
+                'conf': {'agent': {'server': 'puppetmaster.example.org'}}}}
+        util.write_file(self.conf, '[agent]\nserver = origpuppet\nother = 3')
+        puppet_conf_path = 'cloudinit.config.cc_puppet.PUPPET_CONF_PATH'
+        mycloud.distro = mock.MagicMock()
+        with mock.patch(puppet_conf_path, self.conf):
+            cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
+        content = util.load_file(self.conf)
+        expected = '[agent]\nserver = puppetmaster.example.org\nother = 3\n\n'
+        self.assertEqual(expected, content)