some cleanups and changes

 * use util.subp from inside parse_shell_config,
   and adjust exception handling accordingly.
 * add 'switch_user_cmd' as a callback function to pass
   to parse_shell_config, which allows us to mock this to avoid
   'sudo' when running test cases.
   Basically the test cases just return '[]' here.
 * fix some pylint
 * handle empty 'content' in parse_shell_config and remove
   the protection that was present.

2 files changed, 40 insertions, 25 deletions

diff --git a/cloudinit/sources/DataSourceOpenNebula.py b/cloudinit/sources/DataSourceOpenNebula.py
index 141bd454..07dc25ff 100644
--- a/cloudinit/sources/DataSourceOpenNebula.py
+++ b/cloudinit/sources/DataSourceOpenNebula.py
@@ -23,9 +23,9 @@
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+import pwd
 import re
-import string
-import subprocess
+import string  # pylint: disable=W0402
 
 from cloudinit import log as logging
 from cloudinit import sources
@@ -58,7 +58,7 @@ class DataSourceOpenNebula(sources.DataSource):
 
         # decide parseuser for context.sh shell reader
         parseuser = DEFAULT_PARSEUSER
-        if self.ds_cfg.get('parseuser'):
+        if 'parseuser' in self.ds_cfg:
             parseuser = self.ds_cfg.get('parseuser')
 
         candidates = [self.seed_dir]
@@ -260,13 +260,21 @@ def find_candidate_devs():
     return combined
 
 
-def parse_shell_config(content, keylist=None, bash=None, asuser=None):
+def switch_user_cmd(user):
+    return ['sudo', '-u', user]
+
+
+def parse_shell_config(content, keylist=None, bash=None, asuser=None,
+                       switch_user_cb=None):
 
     if isinstance(bash, str):
         bash = [bash]
     elif bash is None:
         bash = ['bash', '-e']
 
+    if switch_user_cb is None:
+        switch_user_cb = switch_user_cmd
+
     # allvars expands to all existing variables by using '${!x*}' notation
     # where x is lower or upper case letters or '_'
     allvars = ["${!%s*}" % x for x in string.letters + "_"]
@@ -302,23 +310,17 @@ def parse_shell_config(content, keylist=None, bash=None, asuser=None):
     bcmd = ('unset IFS\n' +
             setup +
             varprinter(allvars) +
-            '{\n%s\n\n} > /dev/null\n' % content +
+            '{\n%s\n\n:\n} > /dev/null\n' % content +
             'unset IFS\n' +
             varprinter(keylist) + "\n")
 
     cmd = []
     if asuser is not None:
-        cmd = ['sudo', '-u', asuser]
+        cmd = switch_user_cb(asuser)
 
     cmd.extend(bash)
 
-    sp = subprocess.Popen(cmd, stdin=subprocess.PIPE,
-                          stdout=subprocess.PIPE,
-                          stderr=subprocess.PIPE)
-    (output, error) = sp.communicate(input=bcmd)
-
-    if sp.returncode != 0:
-        raise Exception("Process returned %d" % sp.returncode)
+    (output, _error) = util.subp(cmd, data=bcmd)
 
     # exclude vars in bash that change on their own or that we used
     excluded = ("RANDOM", "LINENO", "_", "__v")
@@ -329,7 +331,7 @@ def parse_shell_config(content, keylist=None, bash=None, asuser=None):
 
     # go through output.  First _start_ is for 'preset', second for 'target'.
     # Add to target only things were changed and not in volitile
-    for line in output.split("\0"):
+    for line in output.split("\x00"):
         try:
             (key, val) = line.split("=", 1)
             if target is preset:
@@ -367,21 +369,21 @@ def read_context_disk_dir(source_dir, asuser=None):
     results = {'userdata': None, 'metadata': {}}
 
     if "context.sh" in found:
+        if asuser is not None:
+            try:
+                pwd.getpwnam(asuser)
+            except KeyError as e:
+                raise BrokenContextDiskDir("configured user '%s' "
+                                           "does not exist", asuser)
         try:
             with open(os.path.join(source_dir, 'context.sh'), 'r') as f:
                 content = f.read().strip()
-                f.close()
-
-                # don't pass empty context script
-                # to shell parser
-                non_empty = re.match(r'.*?^\s*([^# ]+)', content,
-                                     re.MULTILINE | re.DOTALL)
-                if non_empty:
-                    context = parse_shell_config(content, asuser=asuser)
+
+            context = parse_shell_config(content, asuser=asuser)
+        except util.ProcessExecutionError as e:
+            raise BrokenContextDiskDir("Error processing context.sh: %s" % (e))
         except IOError as e:
             raise NonContextDiskDir("Error reading context.sh: %s" % (e))
-        except Exception as e:
-            raise BrokenContextDiskDir("Error processing context.sh: %s" % (e))
     else:
         raise NonContextDiskDir("Missing context.sh")
 
diff --git a/tests/unittests/test_datasource/test_opennebula.py b/tests/unittests/test_datasource/test_opennebula.py
index f2457657..9c7a644a 100644
--- a/tests/unittests/test_datasource/test_opennebula.py
+++ b/tests/unittests/test_datasource/test_opennebula.py
@@ -37,6 +37,7 @@ CMD_IP_OUT = '''\
 
 
 class TestOpenNebulaDataSource(MockerTestCase):
+    parsed_user = None
 
     def setUp(self):
         super(TestOpenNebulaDataSource, self).setUp()
@@ -48,6 +49,18 @@ class TestOpenNebulaDataSource(MockerTestCase):
         self.seed_dir = os.path.join(self.paths.seed_dir, "opennebula")
         self.sys_cfg = {'datasource': {'OpenNebula': {'dsmode': 'local'}}}
 
+        # we don't want 'sudo' called in tests. so we patch switch_user_cmd
+        def my_switch_user_cmd(user):
+            self.parsed_user = user
+            return []
+
+        self.switch_user_cmd_real = ds.switch_user_cmd
+        ds.switch_user_cmd = my_switch_user_cmd
+
+    def tearDown(self):
+        ds.switch_user_cmd = self.switch_user_cmd_real
+        super(TestOpenNebulaDataSource, self).tearDown()
+
     def test_get_data_non_contextdisk(self):
         try:
             # dont' try to lookup for CDs
@@ -96,9 +109,9 @@ class TestOpenNebulaDataSource(MockerTestCase):
             util.find_devs_with = orig_find_devs_with
 
     def test_get_data(self):
+        orig_find_devs_with = util.find_devs_with
         try:
             # dont' try to lookup for CDs
-            orig_find_devs_with = util.find_devs_with
             util.find_devs_with = lambda n: []
             populate_context_dir(self.seed_dir, {'KEY1': 'val1'})
             dsrc = self.ds(sys_cfg=self.sys_cfg, distro=None, paths=self.paths)