Allow comments in runcmd and report failed commands correctly (#1049)

Allow comments in runcmd and report failed commands correctly

A `runcmd` script may fail to parse properly, but does not mark
`runcmd` as failed when that occurs. Additionally `shellify()` fails
to correctly parse scripts that contain a comment line.

Rectify both issues and add unit tests to verify correct behavior.

LP: #1853146

5 files changed, 34 insertions, 9 deletions

diff --git a/cloudinit/config/cc_runcmd.py b/cloudinit/config/cc_runcmd.py
index 1f75d6c5..15960c7d 100644
--- a/cloudinit/config/cc_runcmd.py
+++ b/cloudinit/config/cc_runcmd.py
@@ -65,7 +65,8 @@ schema = {
             'items': {
                 'oneOf': [
                     {'type': 'array', 'items': {'type': 'string'}},
-                    {'type': 'string'}]
+                    {'type': 'string'},
+                    {'type': 'null'}]
             },
             'additionalItems': False,  # Reject items of non-string non-list
             'additionalProperties': False,
@@ -90,7 +91,7 @@ def handle(name, cfg, cloud, log, _args):
     try:
         content = util.shellify(cmd)
         util.write_file(out_fn, content, 0o700)
-    except Exception:
-        util.logexc(log, "Failed to shellify %s into file %s", cmd, out_fn)
+    except Exception as e:
+        raise type(e)('Failed to shellify {} into file {}'.format(cmd, out_fn))
 
 # vi: ts=4 expandtab
diff --git a/cloudinit/tests/test_util.py b/cloudinit/tests/test_util.py
index 977ad8e0..ab5eb35c 100644
--- a/cloudinit/tests/test_util.py
+++ b/cloudinit/tests/test_util.py
@@ -349,6 +349,11 @@ class TestShellify(CiTestCase):
             util.shellify(["echo hi mom", ["echo", "hi dad"],
                            ('echo', 'hi', 'sis')]))
 
+    def test_supports_comments(self):
+        self.assertEqual(
+            '\n'.join(["#!/bin/sh", "echo start", "echo end", ""]),
+            util.shellify(["echo start", None, "echo end"]))
+
 
 class TestGetHostnameFqdn(CiTestCase):
 
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 22d8917e..1b4384e1 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -2031,6 +2031,9 @@ def shellify(cmdlist, add_header=True):
         elif isinstance(args, str):
             content = "%s%s\n" % (content, args)
             cmds_made += 1
+        # Yaml parsing of a comment results in None
+        elif args is None:
+            pass
         else:
             raise TypeError(
                 "Unable to shellify type '%s'. Expected list, string, tuple. "
diff --git a/tests/unittests/test_handler/test_handler_runcmd.py b/tests/unittests/test_handler/test_handler_runcmd.py
index 73237d68..c03efa67 100644
--- a/tests/unittests/test_handler/test_handler_runcmd.py
+++ b/tests/unittests/test_handler/test_handler_runcmd.py
@@ -7,6 +7,7 @@ from cloudinit.tests.helpers import (
     CiTestCase, FilesystemMockingTestCase, SchemaTestCaseMixin,
     skipUnlessJsonSchema)
 
+from unittest.mock import patch
 import logging
 import os
 import stat
@@ -41,15 +42,27 @@ class TestRuncmd(FilesystemMockingTestCase):
             "Skipping module named notimportant, no 'runcmd' key",
             self.logs.getvalue())
 
+    @patch('cloudinit.util.shellify')
+    def test_runcmd_shellify_fails(self, cls):
+        """When shellify fails throw exception"""
+        cls.side_effect = TypeError("patched shellify")
+        valid_config = {'runcmd': ['echo 42']}
+        cc = self._get_cloud('ubuntu')
+        with self.assertRaises(TypeError) as cm:
+            with self.allow_subp(['/bin/sh']):
+                handle('cc_runcmd', valid_config, cc, LOG, None)
+        self.assertIn("Failed to shellify", str(cm.exception))
+
     def test_handler_invalid_command_set(self):
         """Commands which can't be converted to shell will raise errors."""
         invalid_config = {'runcmd': 1}
         cc = self._get_cloud('ubuntu')
-        handle('cc_runcmd', invalid_config, cc, LOG, [])
+        with self.assertRaises(TypeError) as cm:
+            handle('cc_runcmd', invalid_config, cc, LOG, [])
         self.assertIn(
             'Failed to shellify 1 into file'
             ' /var/lib/cloud/instances/iid-datasource-none/scripts/runcmd',
-            self.logs.getvalue())
+            str(cm.exception))
 
     @skipUnlessJsonSchema()
     def test_handler_schema_validation_warns_non_array_type(self):
@@ -60,11 +73,12 @@ class TestRuncmd(FilesystemMockingTestCase):
         """
         invalid_config = {'runcmd': 1}
         cc = self._get_cloud('ubuntu')
-        handle('cc_runcmd', invalid_config, cc, LOG, [])
+        with self.assertRaises(TypeError) as cm:
+            handle('cc_runcmd', invalid_config, cc, LOG, [])
         self.assertIn(
             'Invalid config:\nruncmd: 1 is not of type \'array\'',
             self.logs.getvalue())
-        self.assertIn('Failed to shellify', self.logs.getvalue())
+        self.assertIn('Failed to shellify', str(cm.exception))
 
     @skipUnlessJsonSchema()
     def test_handler_schema_validation_warns_non_array_item_type(self):
@@ -76,7 +90,8 @@ class TestRuncmd(FilesystemMockingTestCase):
         invalid_config = {
             'runcmd': ['ls /', 20, ['wget', 'http://stuff/blah'], {'a': 'n'}]}
         cc = self._get_cloud('ubuntu')
-        handle('cc_runcmd', invalid_config, cc, LOG, [])
+        with self.assertRaises(TypeError) as cm:
+            handle('cc_runcmd', invalid_config, cc, LOG, [])
         expected_warnings = [
             'runcmd.1: 20 is not valid under any of the given schemas',
             'runcmd.3: {\'a\': \'n\'} is not valid under any of the given'
@@ -85,7 +100,7 @@ class TestRuncmd(FilesystemMockingTestCase):
         logs = self.logs.getvalue()
         for warning in expected_warnings:
             self.assertIn(warning, logs)
-        self.assertIn('Failed to shellify', logs)
+        self.assertIn('Failed to shellify', str(cm.exception))
 
     def test_handler_write_valid_runcmd_schema_to_file(self):
         """Valid runcmd schema is written to a runcmd shell script."""
diff --git a/tools/.github-cla-signers b/tools/.github-cla-signers
index 3e16ddf3..0aa168d6 100644
--- a/tools/.github-cla-signers
+++ b/tools/.github-cla-signers
@@ -27,6 +27,7 @@ esposem
 GabrielNagy
 giggsoff
 hamalq
+holmanb
 impl
 irishgordo
 izzyleung