openstack/paunch
Code Issues Proposed changes

Merge "Improve volume validation"

Browse Source
This commit is contained in:
Zuul 2019-09-20 06:11:52 +00:00 committed by Gerrit Code Review
commit e7a12e883c
8 changed files with 158 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
paunch/builder/base.py
View File

@ -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):

18
paunch/builder/compose1.py
View File

@ -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', []))

2
paunch/builder/podman.py
View File

@ -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', []))

30
paunch/runner.py
View File

@ -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):

34
paunch/tests/test_builder_base.py
View File

@ -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))

22
paunch/tests/test_builder_compose1.py
View File

@ -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'))

40
paunch/tests/test_builder_podman.py
View File

@ -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
)

24
paunch/tests/test_runner.py
View File

@ -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):