Merge ~chad.smith/cloud-init:cc-landscape-py3-config-fix into cloud-init:master

[Chad Smith]

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.

LP: #1699282
LP: #1710932

[Ryan Harper (raharper)]

Nice work. Couple of questions inline.

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

PASSED: Continuous integration, rev:cffa37df647e0822f0ecfae08efb442cc9ed6278
https://jenkins.ubuntu.com/server/job/cloud-init-ci/150/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: CentOS 6 & 7: Build & Test
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/150/rebuild

review: Approve (continuous-integration)

8492ee9... by Chad Smith on 2017-08-16

drop unneeded path.exists assert and reformat default_landscape test conf file

[Chad Smith (chad.smith)]

comments addressed

[Scott Moser (smoser)]

Under
 https://code.launchpad.net/~mb-methot/cloud-init/+git/cloud-init/+merge/326017
I pointed to http://paste.ubuntu.com/25069502/
which showed why the fix there was insufficient.
if you've *not* covered what those unit tests cover, please add them.

over all, looks good, some comments inline.

[Scott Moser (smoser)]

i dont want to hold getting this in for a test re-work..
if you can show an example of wrap_and_call busted, just open a bug with that, and then lets fix this here.

[Scott Moser (smoser)]

s/fix this here/get this in as it is/

2870d72... by Chad Smith on 2017-08-22

add newline to end of stringify, use tmp_path in unit tests instead of os.path.join

a6ac3e4... by Chad Smith on 2017-08-22

drop nested mock.patch context managers in favor of wrap_and_call

f32d374... by Chad Smith on 2017-08-22

flakes8

[Chad Smith (chad.smith)]

Thanks for the patience on this. I figured out the issue I had with my calls to helpers.wrap_and_call resulted from relying on mock.patch(return_value) instead of mock.patch(new)

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

PASSED: Continuous integration, rev:f32d3748874bf947ba688d52ccda7095d5929515
https://jenkins.ubuntu.com/server/job/cloud-init-ci/185/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/185/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

thanks!

review: Approve

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..1979cd9 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..7c247fa
--- /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, wrap_and_call
+
+from configobj import ConfigObj
+import logging
+
+
+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 = self.tmp_path('client.conf', self.new_root)
+        self.default_file = self.tmp_path('default_landscape', self.new_root)
+
+    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': {}}}
+        wrap_and_call(
+            'cloudinit.config.cc_landscape',
+            {'LSC_CLIENT_CFG_FILE': {'new': 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."""
+        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()
+        wrap_and_call(
+            'cloudinit.config.cc_landscape',
+            {'LSC_CLIENT_CFG_FILE': {'new': self.conf},
+             'LS_DEFAULT_FILE': {'new': 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())
+        default_content = util.load_file(self.default_file)
+        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'}}
+        wrap_and_call(
+            'cloudinit.config.cc_landscape',
+            {'LSC_CLIENT_CFG_FILE': {'new': 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'}}
+        wrap_and_call(
+            'cloudinit.config.cc_landscape',
+            {'LSC_CLIENT_CFG_FILE': {'new': 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__)
+
+
[e-mail]('cloudinit.config.cc_puppet.util')
[e-mail]('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)
+
+
[e-mail]('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)