From e13226d4cf279071a07d7196c3e631d004031a1b Mon Sep 17 00:00:00 2001 From: Emilien Macchi Date: Thu, 23 Jul 2020 17:04:14 -0400 Subject: [PATCH] [queens-only] Improve volume validation (unclean backport) Manually backport I9a55698b04dc2c5f01d776c6bdc2f26d47baf803. 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: Id42eb9d550865682078781a04f961c13bfbcde78 Closes-Bug: #1843734 --- paunch/builder/compose1.py | 24 +++++++++++++ paunch/runner.py | 30 ++++++++++++++++ paunch/tests/test_builder_compose1.py | 52 ++++++++++++++++++++++++--- paunch/tests/test_runner.py | 24 +++++++++++++ 4 files changed, 126 insertions(+), 4 deletions(-) diff --git a/paunch/builder/compose1.py b/paunch/builder/compose1.py index 001828a..fd65460 100644 --- a/paunch/builder/compose1.py +++ b/paunch/builder/compose1.py @@ -235,6 +235,8 @@ class ComposeV1Builder(object): cmd.append(cconfig.get('image', '')) cmd.extend(self.command_argument(cconfig.get('command'))) + return self.validate_volumes(cconfig.get('volumes', [])) + def docker_exec_args(self, cmd, container): cconfig = self.config[container] if 'privileged' in cconfig: @@ -311,6 +313,28 @@ class ComposeV1Builder(object): return command.split() return command + 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 docker will error if the + source volume filesystem path doesn't exist, we want to catch the + error before docker. + + :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/runner.py b/paunch/runner.py index 8eb612c..2953be4 100644 --- a/paunch/runner.py +++ b/paunch/runner.py @@ -13,6 +13,7 @@ import collections import json +import os import random import string import subprocess @@ -225,3 +226,32 @@ class DockerRunner(object): for container in self.containers_in_config(conf_id): configs[conf_id].append(self.inspect(container)) return configs + + 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.docker_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())) diff --git a/paunch/tests/test_builder_compose1.py b/paunch/tests/test_builder_compose1.py index 5716a51..6a31884 100644 --- a/paunch/tests/test_builder_compose1.py +++ b/paunch/tests/test_builder_compose1.py @@ -477,7 +477,8 @@ three-12345678 three''', '', 0), ) @mock.patch("psutil.Process.cpu_affinity", return_value=[0, 1, 2, 3]) - def test_docker_run_args_lists(self, mock_cpu): + @mock.patch('paunch.runner.DockerRunner', autospec=True) + def test_docker_run_args_lists(self, mock_runner, mock_cpu): config = { 'one': { 'image': 'centos:7', @@ -493,7 +494,8 @@ three-12345678 three''', '', 0), 'volumes_from': ['two', 'three'] } } - builder = compose1.ComposeV1Builder('foo', config, None) + mock_runner.validate_volume_source.return_value = True + builder = compose1.ComposeV1Builder('foo', config, mock_runner) cmd = ['docker', 'run', '--name', 'one'] builder.docker_run_args(cmd, 'one') @@ -511,7 +513,8 @@ three-12345678 three''', '', 0), ) @mock.patch("psutil.Process.cpu_affinity", return_value=[0, 1, 2, 3]) - def test_docker_run_args_lists_with_cpu(self, mock_cpu): + @mock.patch('paunch.runner.DockerRunner', autospec=True) + def test_docker_run_args_lists_with_cpu(self, mock_runner, mock_cpu): config = { 'one': { 'image': 'centos:7', @@ -528,7 +531,8 @@ three-12345678 three''', '', 0), 'cpuset_cpus': '0-2', } } - builder = compose1.ComposeV1Builder('foo', config, None) + mock_runner.validate_volume_source.return_value = True + builder = compose1.ComposeV1Builder('foo', config, mock_runner) cmd = ['docker', 'run', '--name', 'one'] builder.docker_run_args(cmd, 'one') @@ -592,3 +596,43 @@ three-12345678 three''', '', 0), ['ls', '-l', '"/foo', 'bar"'], b.command_argument('ls -l "/foo bar"') ) + + @mock.patch('paunch.runner.DockerRunner', autospec=True) + @mock.patch("psutil.Process.cpu_affinity", return_value=[0, 1, 2, 3]) + def test_docker_run_args_validation_true(self, mock_cpu, mock_runner): + config = { + 'one': { + 'image': 'foo', + 'volumes': ['/foo:/foo:rw', '/bar:/bar:ro'], + } + } + mock_runner.validate_volume_source.return_value = True + builder = compose1.ComposeV1Builder('foo', config, mock_runner) + + cmd = ['docker'] + self.assertTrue(builder.docker_run_args(cmd, 'one')) + self.assertEqual( + ['docker', '--detach=true', '--volume=/foo:/foo:rw', + '--volume=/bar:/bar:ro', '--cpuset-cpus=0,1,2,3', 'foo'], + cmd + ) + + @mock.patch('paunch.runner.DockerRunner', autospec=True) + @mock.patch("psutil.Process.cpu_affinity", return_value=[0, 1, 2, 3]) + def test_docker_run_args_validation_false(self, mock_cpu, mock_runner): + config = { + 'one': { + 'image': 'foo', + 'volumes': ['/foo:/foo:rw', '/bar:/bar:ro'], + } + } + mock_runner.validate_volume_source.return_value = False + builder = compose1.ComposeV1Builder('foo', config, mock_runner) + + cmd = ['docker'] + self.assertFalse(builder.docker_run_args(cmd, 'one')) + self.assertEqual( + ['docker', '--detach=true', '--volume=/foo:/foo:rw', + '--volume=/bar:/bar:ro', '--cpuset-cpus=0,1,2,3', 'foo'], + cmd + ) diff --git a/paunch/tests/test_runner.py b/paunch/tests/test_runner.py index d4d9edd..6f95768 100644 --- a/paunch/tests/test_runner.py +++ b/paunch/tests/test_runner.py @@ -284,3 +284,27 @@ four-12345678 four 'two': [{'e': 'f'}, {'e': 'f'}, {'e': 'f'}], 'three': [{'e': 'f'}, {'e': 'f'}, {'e': 'f'}] }, result) + + @mock.patch('os.path.exists', return_value=True) + def test_validate_volume_source_file(self, exists_mock): + self.assertTrue(self.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.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.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.runner.validate_volume_source('foo'))