Improve volume validation

We need to support both a source filesystem location and container
volume for the volume mounts. This change adds the ability to validate
that the provided source container is a container volume. Additionally
aligns the validation between the docker/podman methods so they operate
in the same fashion.

Change-Id: I9a55698b04dc2c5f01d776c6bdc2f26d47baf803
Closes-Bug: #1843734

paunch/builder/base.py

@@ -352,6 +352,28 @@ class BaseBuilder(object):
             n += float(m.group(10)) / 1000000.0
         return n
 
+    def validate_volumes(self, volumes):
+        """Validate volume sources
+
+        Validates that the source volume either exists on the filesystem
+        or is a valid container volume.  Since podman will error if the
+        source volume filesystem path doesn't exist, we want to catch the
+        error before podman.
+
+        :param: volumes: list of volume mounts in the format of "src:path"
+        """
+        valid = True
+        for volume in volumes:
+            if not volume:
+                # ignore when volume is ''
+                continue
+            src_path = volume.split(':', 1)[0]
+            check = self.runner.validate_volume_source(src_path)
+            if not check:
+                self.log.error("%s is not a valid volume source" % src_path)
+                valid = False
+        return valid
+
 
 class PullException(Exception):
 

paunch/builder/compose1.py

@@ -11,8 +11,6 @@
 #   under the License.
 #
 
-import os
-
 from paunch.builder import base
 
 
@@ -119,18 +117,4 @@ class ComposeV1Builder(base.BaseBuilder):
         cmd.append(cconfig.get('image', ''))
         cmd.extend(self.command_argument(cconfig.get('command')))
 
-        # NOTE(xek): If the file to be mounted doesn't exist, Docker version
+        return self.validate_volumes(cconfig.get('volumes', []))
-        # 1.13.1 creates a directory in it's place. This behavior is different
-        # then Podman, which throws an error. We want to replicate the Podman
-        # behavior. Creating directories in place of files creates errors which
-        # are very difficult to diagnose.
-        for volume in cconfig.get('volumes', []):
-            host_path = volume.split(':', 1)[0]
-            if host_path and not os.path.exists(host_path):
-                self.log.error("Error running %s.\n" % cmd)
-                self.log.error('stderr: Error: error checking path "%s":'
-                               ' stat %s: no such file or directory\n' % (
-                                   host_path, host_path))
-                return False
-
-        return True

paunch/builder/podman.py

@@ -105,4 +105,4 @@ class PodmanBuilder(base.BaseBuilder):
         cmd.append(cconfig.get('image', ''))
         cmd.extend(self.command_argument(cconfig.get('command')))
 
-        return True
+        return self.validate_volumes(cconfig.get('volumes', []))

paunch/runner.py

@@ -14,6 +14,7 @@
 import collections
 import jmespath
 import json
+import os
 import random
 import string
 import subprocess
@@ -283,6 +284,35 @@ class BaseRunner(object):
                 self.rename_container(current, desired)
                 current_containers.append(desired)
 
+    def validate_volume_source(self, volume):
+        """Validate that the provided volume
+
+        This checks that the provided volume either exists on the filesystem
+        or is a container volume.
+
+        :param: volume: string containing either a filesystme path or container
+                        volume name
+        """
+        if os.path.exists(volume):
+            return True
+
+        if os.path.sep in volume:
+            # if we get here and have a path seperator, let's skip the
+            # container lookup because container volumes won't have / in them.
+            self.log.debug('Path seperator found in volume (%s), but did not '
+                           'exist on the file system' % volume)
+            return False
+
+        self.log.debug('Running volume lookup for "%s"' % volume)
+        filter_opt = '--filter=name={}'.format(volume)
+        cmd = [self.cont_cmd, 'volume', 'ls', '-q', filter_opt]
+        cmd_stdout, cmd_stderr, returncode = self.execute(cmd)
+        if returncode != 0:
+            self.log.error('Error during volume verification')
+            self.log.error(cmd_stderr)
+            return False
+        return (volume in set(cmd_stdout.split()))
+
 
 class DockerRunner(BaseRunner):
 

paunch/tests/test_builder_base.py

@@ -18,6 +18,7 @@ import json
 import mock
 import tenacity
 
+from paunch.builder import base as basebuilder
 from paunch.builder import compose1
 from paunch import runner
 from paunch.tests import base
@@ -418,7 +419,8 @@ three-12345678 three''', '', 0),
              '--label', 'config_data=null'],
             cmd)
 
-    def test_durations(self):
+    @mock.patch('paunch.runner.DockerRunner', autospec=True)
+    def test_durations(self, runner):
         config = {
             'a': {'stop_grace_period': 123},
             'b': {'stop_grace_period': 123.5},
@@ -430,7 +432,7 @@ three-12345678 three''', '', 0),
             'h': {'stop_grace_period': '5h34m56s'},
             'i': {'stop_grace_period': '1h1m1s1ms1us'},
         }
-        builder = compose1.ComposeV1Builder('foo', config, None)
+        builder = compose1.ComposeV1Builder('foo', config, runner)
 
         result = {
             'a': '--stop-timeout=123',
@@ -449,9 +451,8 @@ three-12345678 three''', '', 0),
             builder.container_run_args(cmd, container)
             self.assertIn(arg, cmd)
 
-    @mock.patch('os.path.exists')
+    @mock.patch('paunch.runner.DockerRunner', autospec=True)
-    def test_container_run_args_lists(self, path_exists):
+    def test_container_run_args_lists(self, runner):
-        path_exists.return_value = True
         config = {
             'one': {
                 'image': 'centos:7',
@@ -470,7 +471,7 @@ three-12345678 three''', '', 0),
                 'cap_drop': ['NET_RAW']
             }
         }
-        builder = compose1.ComposeV1Builder('foo', config, None)
+        builder = compose1.ComposeV1Builder('foo', config, runner)
 
         cmd = ['docker', 'run', '--name', 'one']
         builder.container_run_args(cmd, 'one')
@@ -533,3 +534,24 @@ three-12345678 three''', '', 0),
             ['ls', '-l', '"/foo', 'bar"'],
             b.command_argument('ls -l "/foo bar"')
         )
+
+
+class TestVolumeChecks(base.TestCase):
+    @mock.patch('paunch.runner.PodmanRunner', autospec=True)
+    def test_validate_volumes(self, runner):
+        runner.validate_volume_source.return_value = True
+        builder = basebuilder.BaseBuilder('test', {}, runner, {})
+        volumes = ['', '/var:/var', 'test:/bar']
+        self.assertTrue(builder.validate_volumes(volumes))
+
+    def test_validate_volumes_empty(self):
+        builder = basebuilder.BaseBuilder('test', {}, runner, {})
+        volumes = []
+        self.assertTrue(builder.validate_volumes(volumes))
+
+    @mock.patch('paunch.runner.PodmanRunner', autospec=True)
+    def test_validate_volumes_fail(self, runner):
+        runner.validate_volume_source.return_value = False
+        builder = basebuilder.BaseBuilder('test', {}, runner, {})
+        volumes = ['/var:/var']
+        self.assertFalse(builder.validate_volumes(volumes))

paunch/tests/test_builder_compose1.py

@@ -19,7 +19,8 @@ from paunch.tests import test_builder_base as tbb
 
 
 class TestComposeV1Builder(tbb.TestBaseBuilder):
-    def test_cont_run_args(self):
+    @mock.patch('paunch.runner.DockerRunner', autospec=True)
+    def test_cont_run_args(self, runner):
         config = {
             'one': {
                 'image': 'centos:7',
@@ -53,7 +54,8 @@ class TestComposeV1Builder(tbb.TestBaseBuilder):
 
             }
         }
-        builder = compose1.ComposeV1Builder('foo', config, None)
+        runner.validate_volume_source.return_value = True
+        builder = compose1.ComposeV1Builder('foo', config, runner)
 
         cmd = ['docker', 'run', '--name', 'one']
         builder.container_run_args(cmd, 'one')
@@ -79,16 +81,16 @@ class TestComposeV1Builder(tbb.TestBaseBuilder):
             cmd
         )
 
-    @mock.patch('os.path.exists')
+    @mock.patch('paunch.runner.DockerRunner', autospec=True)
-    def test_cont_run_args_validation_true(self, path_exists):
+    def test_cont_run_args_validation_true(self, runner):
-        path_exists.return_value = True
         config = {
             'one': {
                 'image': 'foo',
                 'volumes': ['/foo:/foo:rw', '/bar:/bar:ro'],
             }
         }
-        builder = compose1.ComposeV1Builder('foo', config, None)
+        runner.validate_volume_source.return_value = True
+        builder = compose1.ComposeV1Builder('foo', config, runner)
 
         cmd = ['docker']
         self.assertTrue(builder.container_run_args(cmd, 'one'))
@@ -98,16 +100,16 @@ class TestComposeV1Builder(tbb.TestBaseBuilder):
             cmd
         )
 
-    @mock.patch('os.path.exists')
+    @mock.patch('paunch.runner.DockerRunner', autospec=True)
-    def test_cont_run_args_validation_false(self, path_exists):
+    def test_cont_run_args_validation_false(self, runner):
-        path_exists.return_value = False
         config = {
             'one': {
                 'image': 'foo',
                 'volumes': ['/foo:/foo:rw', '/bar:/bar:ro'],
             }
         }
-        builder = compose1.ComposeV1Builder('foo', config, None)
+        runner.validate_volume_source.return_value = False
+        builder = compose1.ComposeV1Builder('foo', config, runner)
 
         cmd = ['docker']
         self.assertFalse(builder.container_run_args(cmd, 'one'))

paunch/tests/test_builder_podman.py

@@ -12,6 +12,8 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import mock
+
 from paunch.builder import podman
 from paunch.tests import test_builder_base as base
 
@@ -67,3 +69,41 @@ class TestPodmanBuilder(base.TestBaseBuilder):
              'centos:7'],
             cmd
         )
+
+    @mock.patch('paunch.runner.PodmanRunner', autospec=True)
+    def test_cont_run_args_validation_true(self, runner):
+        config = {
+            'one': {
+                'image': 'foo',
+                'volumes': ['/foo:/foo:rw', '/bar:/bar:ro'],
+            }
+        }
+        runner.validate_volume_source.return_value = True
+        builder = podman.PodmanBuilder('foo', config, runner)
+
+        cmd = ['podman']
+        self.assertTrue(builder.container_run_args(cmd, 'one'))
+        self.assertEqual(
+            ['podman', '--conmon-pidfile=/var/run/one.pid', '--detach=true',
+             '--volume=/foo:/foo:rw', '--volume=/bar:/bar:ro', 'foo'],
+            cmd
+        )
+
+    @mock.patch('paunch.runner.PodmanRunner', autospec=True)
+    def test_cont_run_args_validation_false(self, runner):
+        config = {
+            'one': {
+                'image': 'foo',
+                'volumes': ['/foo:/foo:rw', '/bar:/bar:ro'],
+            }
+        }
+        runner.validate_volume_source.return_value = False
+        builder = podman.PodmanBuilder('foo', config, runner)
+
+        cmd = ['podman']
+        self.assertFalse(builder.container_run_args(cmd, 'one'))
+        self.assertEqual(
+            ['podman', '--conmon-pidfile=/var/run/one.pid', '--detach=true',
+             '--volume=/foo:/foo:rw', '--volume=/bar:/bar:ro', 'foo'],
+            cmd
+        )

paunch/tests/test_runner.py

@@ -384,6 +384,30 @@ two-12345678 two
             ['two-12345678', 'two']
         ], names)
 
+    @mock.patch('os.path.exists', return_value=True)
+    def test_validate_volume_source_file(self, exists_mock):
+        self.assertTrue(self.podman_runner.validate_volume_source('/tmp'))
+
+    @mock.patch('os.path.exists', return_value=False)
+    def test_validate_volume_source_file_fail(self, exists_mock):
+        self.assertFalse(self.podman_runner.validate_volume_source('/nope'))
+
+    @mock.patch('os.path.exists', return_value=False)
+    @mock.patch('subprocess.Popen')
+    def test_validate_volume_source_container(self, popen, exists_mock):
+        ps_result = '''foo
+foobar
+'''
+        self.mock_execute(popen, ps_result, '', 0)
+        self.assertTrue(self.podman_runner.validate_volume_source('foo'))
+
+    @mock.patch('os.path.exists', return_value=False)
+    @mock.patch('subprocess.Popen')
+    def test_validate_volume_source_container_fail(self, popen, exists_mock):
+        ps_result = ''
+        self.mock_execute(popen, ps_result, '', 0)
+        self.assertFalse(self.podman_runner.validate_volume_source('foo'))
+
 
 class TestDockerRunner(TestBaseRunner):