diff --git a/paunch/builder/base.py b/paunch/builder/base.py index 523355a..c0f30fb 100644 --- a/paunch/builder/base.py +++ b/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): diff --git a/paunch/builder/compose1.py b/paunch/builder/compose1.py index b5fcc80..a47d19e 100644 --- a/paunch/builder/compose1.py +++ b/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 - # 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 + return self.validate_volumes(cconfig.get('volumes', [])) diff --git a/paunch/builder/podman.py b/paunch/builder/podman.py index d2e7648..4f734c3 100644 --- a/paunch/builder/podman.py +++ b/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', [])) diff --git a/paunch/runner.py b/paunch/runner.py index 58a7eb7..d735c58 100644 --- a/paunch/runner.py +++ b/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): diff --git a/paunch/tests/test_builder_base.py b/paunch/tests/test_builder_base.py index 618bd82..f316731 100644 --- a/paunch/tests/test_builder_base.py +++ b/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') - def test_container_run_args_lists(self, path_exists): - path_exists.return_value = True + @mock.patch('paunch.runner.DockerRunner', autospec=True) + def test_container_run_args_lists(self, runner): 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)) diff --git a/paunch/tests/test_builder_compose1.py b/paunch/tests/test_builder_compose1.py index 2ec9788..53eb46e 100644 --- a/paunch/tests/test_builder_compose1.py +++ b/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') - def test_cont_run_args_validation_true(self, path_exists): - path_exists.return_value = True + @mock.patch('paunch.runner.DockerRunner', autospec=True) + def test_cont_run_args_validation_true(self, runner): 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') - def test_cont_run_args_validation_false(self, path_exists): - path_exists.return_value = False + @mock.patch('paunch.runner.DockerRunner', autospec=True) + def test_cont_run_args_validation_false(self, runner): 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')) diff --git a/paunch/tests/test_builder_podman.py b/paunch/tests/test_builder_podman.py index 0c9580f..1208832 100644 --- a/paunch/tests/test_builder_podman.py +++ b/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 + ) diff --git a/paunch/tests/test_runner.py b/paunch/tests/test_runner.py index fa9776d..e0862d4 100644 --- a/paunch/tests/test_runner.py +++ b/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):