Merge ~powersj/cloud-init:cii-strings into cloud-init:master

[Joshua Powers]

tests: execute: support command as string

If a string is passed to execute, then invoke 'bash', '-c',
'string'. That allows the less verbose execution of simple
commands:
  image.execute("ls /run")
compared to the more explicit but longer winded:
  image.execute(["ls", "/run"])

If 'env' was ever modified in execute or a method that it called,
then the next invocation's default value would be changed. Instead
use None and then set to a new empty dict in the method.

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

PASSED: Continuous integration, rev:2f2717cc364a7ab2d7ff129f5b862643ab24af3f
https://jenkins.ubuntu.com/server/job/cloud-init-ci/283/
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/283/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I'd like 2 things changed
a.) do not use /bin/bash or require it by (Instance/Image).execute.
   That is not python's default shell, unless /bin/sh == /bin/bash on the system.
b.) lets only fix strings where we were already using sh (via sh -c).

throwing stuff to sh just allows for errors and unexpected behavior.

I realize this is a contrived example, but its the kind of unexpected results you can get when you hand things off to shell interpretation and are not careful about quoting

## pretend tmp=$(mktemp -d) happens to return the literal name '/tmp/foo?'
## simulate this behavior by the 'tmp=; mkdir -p'
$ touch /tmp/foo1 /tmp/foo2
$ tmp=/tmp/foo?; mkdir -p "/tmp/foo?"
# ls /tmp/
foo1 foo2 foo?
# rm -Rvf $tmp
removed `/tmp/foo1'
removed `/tmp/foo2'
removed directory: `/tmp/foo?'

## YIKES!

I'll give you a diff or a MP with the changes i'd like.

[Scott Moser (smoser)]

Note above, 'mktemp -d' is probably not going to give you 'foo?'. However, nothing that i'm aware of promises that it's returned filenames will not have any characters that may be "special" to /bin/sh (or /usr/bin/perl or /usr/bin/python or gcc).

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

PASSED: Continuous integration, rev:2be9c0f1418b76b2a65198f35588e3f24d7b5f89
https://jenkins.ubuntu.com/server/job/cloud-init-ci/287/
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/287/rebuild

review: Approve (continuous-integration)

diff --git a/tests/cloud_tests/bddeb.py b/tests/cloud_tests/bddeb.py
index fe80535..fba8a0c 100644
--- a/tests/cloud_tests/bddeb.py
+++ b/tests/cloud_tests/bddeb.py
@@ -28,8 +28,7 @@ def build_deb(args, instance):
     # update remote system package list and install build deps
     LOG.debug('installing pre-reqs')
     pkgs = ' '.join(pre_reqs)
-    cmd = 'apt-get update && apt-get install --yes {}'.format(pkgs)
-    instance.execute(['/bin/sh', '-c', cmd])
+    instance.execute('apt-get update && apt-get install --yes {}'.format(pkgs))
 
     # local tmpfile that must be deleted
     local_tarball = tempfile.NamedTemporaryFile().name
diff --git a/tests/cloud_tests/instances/base.py b/tests/cloud_tests/instances/base.py
index 959e9cc..58f45b1 100644
--- a/tests/cloud_tests/instances/base.py
+++ b/tests/cloud_tests/instances/base.py
@@ -23,7 +23,7 @@ class Instance(object):
         self.config = config
         self.features = features
 
-    def execute(self, command, stdout=None, stderr=None, env={},
+    def execute(self, command, stdout=None, stderr=None, env=None,
                 rcs=None, description=None):
         """Execute command in instance, recording output, error and exit code.
 
@@ -31,6 +31,8 @@ class Instance(object):
         target filesystem being available at /.
 
         @param command: the command to execute as root inside the image
+            if command is a string, then it will be executed as:
+            ['sh', '-c', command]
         @param stdout, stderr: file handles to write output and error to
         @param env: environment variables
         @param rcs: allowed return codes from command
@@ -137,9 +139,9 @@ class Instance(object):
             tests.append(self.config['cloud_init_ready_script'])
 
         formatted_tests = ' && '.join(clean_test(t) for t in tests)
-        test_cmd = ('for ((i=0;i<{time};i++)); do {test} && exit 0; sleep 1; '
-                    'done; exit 1;').format(time=time, test=formatted_tests)
-        cmd = ['/bin/bash', '-c', test_cmd]
+        cmd = ('i=0; while [ $i -lt {time} ] && i=$(($i+1)); do {test} && '
+               'exit 0; sleep 1; done; exit 1').format(time=time,
+                                                       test=formatted_tests)
 
         if self.execute(cmd, rcs=(0, 1))[-1] != 0:
             raise OSError('timeout: after {}s system not started'.format(time))
diff --git a/tests/cloud_tests/instances/lxd.py b/tests/cloud_tests/instances/lxd.py
index b9c2cc6..a43918c 100644
--- a/tests/cloud_tests/instances/lxd.py
+++ b/tests/cloud_tests/instances/lxd.py
@@ -31,7 +31,7 @@ class LXDInstance(base.Instance):
         self._pylxd_container.sync()
         return self._pylxd_container
 
-    def execute(self, command, stdout=None, stderr=None, env={},
+    def execute(self, command, stdout=None, stderr=None, env=None,
                 rcs=None, description=None):
         """Execute command in instance, recording output, error and exit code.
 
@@ -39,6 +39,8 @@ class LXDInstance(base.Instance):
         target filesystem being available at /.
 
         @param command: the command to execute as root inside the image
+            if command is a string, then it will be executed as:
+            ['sh', '-c', command]
         @param stdout: file handler to write output
         @param stderr: file handler to write error
         @param env: environment variables
@@ -46,6 +48,12 @@ class LXDInstance(base.Instance):
         @param description: purpose of command
         @return_value: tuple containing stdout data, stderr data, exit code
         """
+        if env is None:
+            env = {}
+
+        if isinstance(command, str):
+            command = ['sh', '-c', command]
+
         # ensure instance is running and execute the command
         self.start()
         res = self.pylxd_container.execute(command, environment=env)
diff --git a/tests/cloud_tests/setup_image.py b/tests/cloud_tests/setup_image.py
index 8053a09..3c0fff6 100644
--- a/tests/cloud_tests/setup_image.py
+++ b/tests/cloud_tests/setup_image.py
@@ -49,8 +49,8 @@ def install_deb(args, image):
     LOG.debug(msg)
     remote_path = os.path.join('/tmp', os.path.basename(args.deb))
     image.push_file(args.deb, remote_path)
-    cmd = 'dpkg -i {} || apt-get install --yes -f'.format(remote_path)
-    image.execute(['/bin/sh', '-c', cmd], description=msg)
+    cmd = 'dpkg -i {}; apt-get install --yes -f'.format(remote_path)
+    image.execute(cmd, description=msg)
 
     # check installed deb version matches package
     fmt = ['-W', "--showformat='${Version}'"]
@@ -113,7 +113,7 @@ def upgrade(args, image):
 
     msg = 'upgrading cloud-init'
     LOG.debug(msg)
-    image.execute(['/bin/sh', '-c', cmd], description=msg)
+    image.execute(cmd, description=msg)
 
 
 def upgrade_full(args, image):
@@ -134,7 +134,7 @@ def upgrade_full(args, image):
 
     msg = 'full system upgrade'
     LOG.debug(msg)
-    image.execute(['/bin/sh', '-c', cmd], description=msg)
+    image.execute(cmd, description=msg)
 
 
 def run_script(args, image):
@@ -165,7 +165,7 @@ def enable_ppa(args, image):
     msg = 'enable ppa: "{}" in target'.format(ppa)
     LOG.debug(msg)
     cmd = 'add-apt-repository --yes {} && apt-get update'.format(ppa)
-    image.execute(['/bin/sh', '-c', cmd], description=msg)
+    image.execute(cmd, description=msg)
 
 
 def enable_repo(args, image):
@@ -188,7 +188,7 @@ def enable_repo(args, image):
 
     msg = 'enable repo: "{}" in target'.format(args.repo)
     LOG.debug(msg)
-    image.execute(['/bin/sh', '-c', cmd], description=msg)
+    image.execute(cmd, description=msg)
 
 
 def setup_image(args, image):