Increase the robustness of the chef module

Add the following adjustments to the chef template and module

- Make it so that the chef directories can be provided (defaults
  to the existing directories)
- Make the params much more configurable, and if a parameter is
  provided in the chef configuration it will override existing template
  parameters.
- Make the template skip lines if the values are None in the configuration
  so that template lines can be removed if/when this is desirable.
- Allow the firstboot json path to be configurable (defaults to the
  existing location).
- Adds a basic set of tests to ensure that good things are happening.

4 files changed, 230 insertions, 138 deletions

diff --git a/cloudinit/config/cc_chef.py b/cloudinit/config/cc_chef.py
index 806deed9..691a51bc 100644
--- a/cloudinit/config/cc_chef.py
+++ b/cloudinit/config/cc_chef.py
@@ -18,6 +18,8 @@
 #    You should have received a copy of the GNU General Public License
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+from datetime import datetime
+
 import json
 import os
 
@@ -38,6 +40,61 @@ CHEF_DIRS = [
 
 OMNIBUS_URL = "https://www.opscode.com/chef/install.sh"
 
+CHEF_RB_TPL_DEFAULTS = {
+    # These are ruby symbols...
+    'ssl_verify_mode': ':verify_none',
+    'log_level': ':info',
+    # These are not symbols...
+    'log_location': '/var/log/chef/client.log',
+    'validation_key': "/etc/chef/validation.pem",
+    'client_key': "/etc/chef/client.pem",
+    'json_attribs': "/etc/chef/firstboot.json",
+    'file_cache_path': "/var/cache/chef",
+    'file_backup_path': "/var/backups/chef",
+    'pid_file': "/var/run/chef/client.pid",
+    'show_time': True,
+}
+CHEF_RB_TPL_BOOL_KEYS = frozenset(['show_time'])
+CHEF_RB_TPL_KEYS = list(CHEF_RB_TPL_DEFAULTS.keys())
+CHEF_RB_TPL_KEYS.extend(CHEF_RB_TPL_BOOL_KEYS)
+CHEF_RB_TPL_KEYS.extend([
+    'server_url',
+    'node_name',
+    'environment',
+    'validation_name',
+])
+CHEF_RB_TPL_KEYS = frozenset(CHEF_RB_TPL_KEYS)
+CHEF_RB_PATH = '/etc/chef/client.rb'
+CHEF_FB_PATH = '/etc/chef/firstboot.json'
+
+
+def get_template_params(iid, chef_cfg, log):
+    params = CHEF_RB_TPL_DEFAULTS.copy()
+    params.update({
+        'server_url': chef_cfg['server_url'],
+        'node_name': util.get_cfg_option_str(chef_cfg, 'node_name', iid),
+        'environment': util.get_cfg_option_str(chef_cfg, 'environment',
+                                               '_default'),
+        'validation_name': chef_cfg['validation_name'],
+    })
+    # Allow users to overwrite any of the keys they want (if they so choose),
+    # when a value is None, then the value will be set to None and no boolean
+    # or string version will be populated...
+    for (k, v) in chef_cfg.items():
+        if k not in CHEF_RB_TPL_KEYS:
+            log.debug("Skipping unknown chef template key '%s'", k)
+            continue
+        if v is None:
+            params[k] = None
+        else:
+            # This will make the value a boolean or string...
+            if k in CHEF_RB_TPL_BOOL_KEYS:
+                params[k] = util.get_cfg_option_bool(chef_cfg, k)
+            else:
+                params[k] = util.get_cfg_option_str(chef_cfg, k)
+    params['generated_on'] = datetime.now().isoformat()
+    return params
+
 
 def handle(name, cfg, cloud, log, _args):
 
@@ -49,7 +106,7 @@ def handle(name, cfg, cloud, log, _args):
     chef_cfg = cfg['chef']
 
     # Ensure the chef directories we use exist
-    for d in CHEF_DIRS:
+    for d in chef_cfg.get('directories', CHEF_DIRS):
         util.ensure_dir(d)
 
     # Set the validation key based on the presence of either 'validation_key'
@@ -64,26 +121,26 @@ def handle(name, cfg, cloud, log, _args):
     template_fn = cloud.get_template_filename('chef_client.rb')
     if template_fn:
         iid = str(cloud.datasource.get_instance_id())
-        params = {
-            'server_url': chef_cfg['server_url'],
-            'node_name': util.get_cfg_option_str(chef_cfg, 'node_name', iid),
-            'environment': util.get_cfg_option_str(chef_cfg, 'environment',
-                                                   '_default'),
-            'validation_name': chef_cfg['validation_name']
-        }
-        templater.render_to_file(template_fn, '/etc/chef/client.rb', params)
+        params = get_template_params(iid, chef_cfg, log)
+        templater.render_to_file(template_fn, CHEF_RB_PATH, params)
+    else:
+        log.warn("No template found, not rendering to %s",
+                 CHEF_RB_PATH)
+
+    # Set the firstboot json
+    fb_filename = util.get_cfg_option_str(chef_cfg, 'firstboot_path',
+                                          default=CHEF_FB_PATH)
+    if not fb_filename:
+        log.info("First boot path empty, not writing first boot json file")
     else:
-        log.warn("No template found, not rendering to /etc/chef/client.rb")
-
-    # set the firstboot json
-    initial_json = {}
-    if 'run_list' in chef_cfg:
-        initial_json['run_list'] = chef_cfg['run_list']
-    if 'initial_attributes' in chef_cfg:
-        initial_attributes = chef_cfg['initial_attributes']
-        for k in list(initial_attributes.keys()):
-            initial_json[k] = initial_attributes[k]
-    util.write_file('/etc/chef/firstboot.json', json.dumps(initial_json))
+        initial_json = {}
+        if 'run_list' in chef_cfg:
+            initial_json['run_list'] = chef_cfg['run_list']
+        if 'initial_attributes' in chef_cfg:
+            initial_attributes = chef_cfg['initial_attributes']
+            for k in list(initial_attributes.keys()):
+                initial_json[k] = initial_attributes[k]
+        util.write_file(fb_filename, json.dumps(initial_json))
 
     # If chef is not installed, we install chef based on 'install_type'
     if (not os.path.isfile('/usr/bin/chef-client') or
diff --git a/cloudinit/util.py b/cloudinit/util.py
index f236d0bf..bdb0f268 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -191,11 +191,11 @@ def ExtendedTemporaryFile(**kwargs):
     return fh
 
 
-def fork_cb(child_cb, *args, **kwargs):
+def fork_cb(child_cb, *args):
     fid = os.fork()
     if fid == 0:
         try:
-            child_cb(*args, **kwargs)
+            child_cb(*args)
             os._exit(0)
         except:
             logexc(LOG, "Failed forking and calling callback %s",
@@ -1297,7 +1297,7 @@ def unmounter(umount):
         yield umount
     finally:
         if umount:
-            umount_cmd = ["umount", umount]
+            umount_cmd = ["umount", '-l', umount]
             subp(umount_cmd)
 
 
@@ -1346,70 +1346,37 @@ def mount_cb(device, callback, data=None, rw=False, mtype=None, sync=True):
     Mount the device, call method 'callback' passing the directory
     in which it was mounted, then unmount.  Return whatever 'callback'
     returned.  If data != None, also pass data to callback.
-
-    mtype is a filesystem type.  it may be a list, string (a single fsname)
-    or a list of fsnames.
     """
-
-    if isinstance(mtype, str):
-        mtypes = [mtype]
-    elif isinstance(mtype, (list, tuple)):
-        mtypes = list(mtype)
-    elif mtype is None:
-        mtypes = None
-
-    # clean up 'mtype' input a bit based on platform.
-    platsys = platform.system().lower()
-    if platsys == "linux":
-        if mtypes is None:
-            mtypes = ["auto"]
-    elif platsys.endswith("bsd"):
-        if mtypes is None:
-            mtypes = ['ufs', 'cd9660', 'vfat']
-        for index, mtype in enumerate(mtypes):
-            if mtype == "iso9660":
-                mtypes[index] = "cd9660"
-    else:
-        # we cannot do a smart "auto", so just call 'mount' once with no -t
-        mtypes = ['']
-
     mounted = mounts()
     with tempdir() as tmpd:
         umount = False
         if device in mounted:
             mountpoint = mounted[device]['mountpoint']
         else:
-            for mtype in mtypes:
-                mountpoint = None
-                try:
-                    mountcmd = ['mount']
-                    mountopts = []
-                    if rw:
-                        mountopts.append('rw')
-                    else:
-                        mountopts.append('ro')
-                    if sync:
-                        # This seems like the safe approach to do
-                        # (ie where this is on by default)
-                        mountopts.append("sync")
-                    if mountopts:
-                        mountcmd.extend(["-o", ",".join(mountopts)])
-                    if mtype:
-                        mountcmd.extend(['-t', mtype])
-                    mountcmd.append(device)
-                    mountcmd.append(tmpd)
-                    subp(mountcmd)
-                    umount = tmpd  # This forces it to be unmounted (when set)
-                    mountpoint = tmpd
-                    break
-                except (IOError, OSError) as exc:
-                    LOG.debug("Failed mount of '%s' as '%s': %s",
-                              device, mtype, exc)
-                    pass
-            if not mountpoint:
-                raise MountFailedError("Failed mounting %s to %s due to: %s" %
+            try:
+                mountcmd = ['mount']
+                mountopts = []
+                if rw:
+                    mountopts.append('rw')
+                else:
+                    mountopts.append('ro')
+                if sync:
+                    # This seems like the safe approach to do
+                    # (ie where this is on by default)
+                    mountopts.append("sync")
+                if mountopts:
+                    mountcmd.extend(["-o", ",".join(mountopts)])
+                if mtype:
+                    mountcmd.extend(['-t', mtype])
+                mountcmd.append(device)
+                mountcmd.append(tmpd)
+                subp(mountcmd)
+                umount = tmpd  # This forces it to be unmounted (when set)
+                mountpoint = tmpd
+            except (IOError, OSError) as exc:
+                raise MountFailedError(("Failed mounting %s "
+                                        "to %s due to: %s") %
                                        (device, tmpd, exc))
-
         # Be nice and ensure it ends with a slash
         if not mountpoint.endswith("/"):
             mountpoint += "/"
@@ -1957,53 +1924,3 @@ def pathprefix2dict(base, required=None, optional=None, delim=os.path.sep):
         raise ValueError("Missing required files: %s", ','.join(missing))
 
     return ret
-
-
-def read_meminfo(meminfo="/proc/meminfo", raw=False):
-    # read a /proc/meminfo style file and return
-    # a dict with 'total', 'free', and 'available'
-    mpliers = {'kB': 2**10, 'mB': 2 ** 20, 'B': 1, 'gB': 2 ** 30}
-    kmap = {'MemTotal:': 'total', 'MemFree:': 'free',
-            'MemAvailable:': 'available'}
-    ret = {}
-    for line in load_file(meminfo).splitlines():
-        try:
-            key, value, unit = line.split()
-        except ValueError:
-            key, value = line.split()
-            unit = 'B'
-        if raw:
-            ret[key] = int(value) * mpliers[unit]
-        elif key in kmap:
-            ret[kmap[key]] = int(value) * mpliers[unit]
-
-    return ret
-
-
-def human2bytes(size):
-    """Convert human string or integer to size in bytes
-      10M => 10485760
-      .5G => 536870912
-    """
-    size_in = size
-    if size.endswith("B"):
-        size = size[:-1]
-
-    mpliers = {'B': 1, 'K': 2 ** 10, 'M': 2 ** 20, 'G': 2 ** 30, 'T': 2 ** 40}
-
-    num = size
-    mplier = 'B'
-    for m in mpliers:
-        if size.endswith(m):
-            mplier = m
-            num = size[0:-len(m)]
-
-    try:
-        num = float(num)
-    except ValueError:
-        raise ValueError("'%s' is not valid input." % size_in)
-
-    if num < 0:
-        raise ValueError("'%s': cannot be negative" % size_in)
-
-    return int(num * mpliers[mplier])
diff --git a/templates/chef_client.rb.tmpl b/templates/chef_client.rb.tmpl
index 538850ca..7b9e6298 100644
--- a/templates/chef_client.rb.tmpl
+++ b/templates/chef_client.rb.tmpl
@@ -9,17 +9,51 @@ you need to add the following to config:
     validation_name: XYZ
     server_url: XYZ
 -#}
-log_level              :info
-log_location           "/var/log/chef/client.log"
-ssl_verify_mode        :verify_none
+
+{#
+The reason these are not in quotes is because they are ruby
+symbols that will be placed inside here, and not actual strings...
+#}
+# This is a generated file, created on {{generated_on}}.
+{% if log_level %}
+log_level              {{log_level}}
+{% endif %}
+{% if ssl_verify_mode %}
+ssl_verify_mode        {{ssl_verify_mode}}
+{% endif %}
+{% if log_location %}
+log_location           "{{log_location}}"
+{% endif %}
+{% if validation_name %}
 validation_client_name "{{validation_name}}"
-validation_key         "/etc/chef/validation.pem"
-client_key             "/etc/chef/client.pem"
+{% endif %}
+{% if validation_key %}
+validation_key         "{{validation_key}}"
+{% endif %}
+{% if client_key %}
+client_key             "{{client_key}}"
+{% endif %}
+{% if server_url %}
 chef_server_url        "{{server_url}}"
+{% endif %}
+{% if environment %}
 environment            "{{environment}}"
+{% endif %}
+{% if node_name %}
 node_name              "{{node_name}}"
-json_attribs           "/etc/chef/firstboot.json"
-file_cache_path        "/var/cache/chef"
-file_backup_path       "/var/backups/chef"
-pid_file               "/var/run/chef/client.pid"
+{% endif %}
+{% if json_attribs %}
+json_attribs           "{{json_attribs}}"
+{% endif %}
+{% if file_cache_path %}
+file_cache_path        "{{file_cache_path}}"
+{% endif %}
+{% if file_backup_path %}
+file_backup_path       "{{file_backup_path}}"
+{% endif %}
+{% if pid_file %}
+pid_file               "{{pid_file}}"
+{% endif %}
+{% if show_time %}
 Chef::Log::Formatter.show_time = true
+{% endif %}
diff --git a/tests/unittests/test_handler/test_handler_chef.py b/tests/unittests/test_handler/test_handler_chef.py
new file mode 100644
index 00000000..5562d18a
--- /dev/null
+++ b/tests/unittests/test_handler/test_handler_chef.py
@@ -0,0 +1,84 @@
+import os
+import json
+
+from cloudinit.config import cc_chef
+
+from cloudinit import cloud
+from cloudinit import distros
+from cloudinit import helpers
+from cloudinit import util
+from cloudinit.sources import DataSourceNone
+
+from .. import helpers as t_help
+
+import logging
+
+LOG = logging.getLogger(__name__)
+
+
+class TestChef(t_help.FilesystemMockingTestCase):
+    def setUp(self):
+        super(TestChef, self).setUp()
+        self.tmp = self.makeDir(prefix="unittest_")
+
+    def fetch_cloud(self, distro_kind):
+        cls = distros.fetch(distro_kind)
+        paths = helpers.Paths({})
+        distro = cls(distro_kind, {}, paths)
+        ds = DataSourceNone.DataSourceNone({}, distro, paths, None)
+        return cloud.Cloud(ds, paths, {}, distro, None)
+
+    def test_no_config(self):
+        self.patchUtils(self.tmp)
+        self.patchOS(self.tmp)
+
+        cfg = {}
+        cc_chef.handle('chef', cfg, self.fetch_cloud('ubuntu'), LOG, [])
+        for d in cc_chef.CHEF_DIRS:
+            self.assertFalse(os.path.isdir(d))
+
+    def test_basic_config(self):
+        tpl_file = util.load_file('templates/chef_client.rb.tmpl')
+        self.patchUtils(self.tmp)
+        self.patchOS(self.tmp)
+
+        util.write_file('/etc/cloud/templates/chef_client.rb.tmpl', tpl_file)
+        cfg = {
+            'chef': {
+                'server_url': 'localhost',
+                'validation_name': 'bob',
+            },
+        }
+        cc_chef.handle('chef', cfg, self.fetch_cloud('ubuntu'), LOG, [])
+        for d in cc_chef.CHEF_DIRS:
+            self.assertTrue(os.path.isdir(d))
+        c = util.load_file(cc_chef.CHEF_RB_PATH)
+        for k, v in cfg['chef'].items():
+            self.assertIn(v, c)
+        for k, v in cc_chef.CHEF_RB_TPL_DEFAULTS.items():
+            if isinstance(v, basestring):
+                self.assertIn(v, c)
+        c = util.load_file(cc_chef.CHEF_FB_PATH)
+        self.assertEqual({}, json.loads(c))
+
+    def test_firstboot_json(self):
+        self.patchUtils(self.tmp)
+        self.patchOS(self.tmp)
+
+        cfg = {
+            'chef': {
+                'server_url': 'localhost',
+                'validation_name': 'bob',
+                'run_list': ['a', 'b', 'c'],
+                'initial_attributes': {
+                    'c': 'd',
+                }
+            },
+        }
+        cc_chef.handle('chef', cfg, self.fetch_cloud('ubuntu'), LOG, [])
+        c = util.load_file(cc_chef.CHEF_FB_PATH)
+        self.assertEqual(
+            {
+                'run_list': ['a', 'b', 'c'],
+                'c': 'd',
+            }, json.loads(c))