diff --git a/.pylintrc b/.pylintrc index b3500b4..a05f2fa 100644 --- a/.pylintrc +++ b/.pylintrc @@ -333,7 +333,7 @@ max-locals=15 max-returns=6 # Maximum number of branch for function / method body -max-branches=12 +max-branches=13 # Maximum number of statements in function / method body max-statements=50 diff --git a/skipper/runner.py b/skipper/runner.py index e490aec..bc9958f 100644 --- a/skipper/runner.py +++ b/skipper/runner.py @@ -165,17 +165,21 @@ def handle_volumes_bind_mount(docker_cmd, homedir, volumes, workspace): volume = '/private' + volume # if part of host directory is empty, skipping this mount - if not volume.split(":")[0]: + host_path = volume.split(":")[0] + if not host_path: continue - create_vol_localpath_if_needed(volume) - docker_cmd += ['-v', volume] + if not create_vol_localpath_if_needed(host_path.strip()) and utils.get_runtime_command() == utils.PODMAN: + logging.warning("Mount source %s doesn't exist and it couldn't be created by skipper, " + "this will cause Podman to fail, ignoring volume mount %s to prevent " + "podman failure", host_path, volume) + else: + docker_cmd += ['-v', volume] return docker_cmd -def create_vol_localpath_if_needed(volume): - host_path = volume.split(":")[0].strip() +def create_vol_localpath_if_needed(host_path): # We have couple of special case mounts # 1. gitconfig file - it is required by skipper but may not exists, we don't want # to create folder if it doesn't exist @@ -196,7 +200,9 @@ def create_vol_localpath_if_needed(volume): except OSError: # If we have no permissions to create the directory, we'll just let # docker create it with root-privileges - pass + return False + + return True @contextmanager diff --git a/tests/test_runner_podman.py b/tests/test_runner_podman.py index 942d118..a9ad1a1 100644 --- a/tests/test_runner_podman.py +++ b/tests/test_runner_podman.py @@ -22,7 +22,18 @@ ENV = ["KEY1=VAL1", "KEY2=VAL2"] -@mock.patch('os.path.exists', mock.MagicMock(autospec=True, return_value=True)) +def mock_makedirs(param): + if param == '/usr/bin/nonexistentbinary': + raise OSError('Permission denied') + + return True + + +def mock_path_exists(param): + return param != '/usr/bin/nonexistentbinary' + + +@mock.patch('os.path.exists', mock.MagicMock(autospec=True, side_effect=mock_path_exists)) class TestRunnerPodman(unittest.TestCase): def setUp(self): @@ -169,6 +180,51 @@ def test_run_complex_command_nested(self, resource_filename_mock, check_output_m assert not check_output_mock.called popen_mock.assert_called_once_with(expected_nested_command) + @mock.patch('getpass.getuser', mock.MagicMock(autospec=True, return_value='testuser')) + @mock.patch('os.getcwd', mock.MagicMock(autospec=True, return_value=PROJECT_DIR)) + @mock.patch('os.path.expanduser', mock.MagicMock(autospec=True, return_value=HOME_DIR)) + @mock.patch('os.getuid', autospec=True) + @mock.patch('subprocess.Popen', autospec=False) + @mock.patch('subprocess.check_output', autospec=False) + @mock.patch('pkg_resources.resource_filename', autospec=False) + @mock.patch('os.makedirs', mock.MagicMock(autospec=True, side_effect=mock_makedirs)) + def test_run_non_existent_unauthorized_volume(self, resource_filename_mock, + check_output_mock, popen_mock, os_getuid_mock): + resource_filename_mock.return_value = "entrypoint.sh" + popen_mock.return_value.stdout.readline.side_effect = ['aaa', 'bbb', 'ccc', ''] + popen_mock.return_value.poll.return_value = -1 + os_getuid_mock.return_value = USER_ID + command = ['ls', '-l'] + + runner.run(command, FQDN_IMAGE, volumes=["/usr/bin/nonexistentbinary:/usr/bin/nonexistentbinary"]) + + expected_nested_command = [ + self.runtime, 'run', + '-t', + '-e', 'KEEP_CONTAINERS=True', + '--ulimit', 'nofile=65536:65536', + '--privileged', + '--net', get_default_net(), + '-e', 'SKIPPER_USERNAME=testuser', + '-e', 'SKIPPER_UID=%(user_uid)s' % dict(user_uid=USER_ID), + '-e', 'HOME=%(homedir)s' % dict(homedir=HOME_DIR), + '-e', 'CONTAINER_RUNTIME_COMMAND=%(runtime_command)s' % dict(runtime_command=utils.get_runtime_command()), + '-v', get_volume_mapping('%(homedir)s/.netrc:%(homedir)s/.netrc:ro' % dict(homedir=HOME_DIR)), + '-v', get_volume_mapping('%(homedir)s/.gitconfig:%(homedir)s/.gitconfig:ro' % dict(homedir=HOME_DIR)), + '-v', get_volume_mapping('%(homedir)s/.docker/config.json:%(homedir)s/.docker/config.json:ro' % dict(homedir=HOME_DIR)), + '-v', get_volume_mapping('/etc/docker:/etc/docker:ro'), + '-v', get_volume_mapping('%(workdir)s:%(workdir)s:rw,shared' % dict(workdir=WORKDIR)), + '-v', get_volume_mapping('entrypoint.sh:/opt/skipper/skipper-entrypoint.sh:rw'), + '-v', get_volume_mapping('/var/run/docker.sock:/var/run/docker.sock:rw'), + '-v', get_volume_mapping('/var/lib/osmosis:/var/lib/osmosis:rw'), + '-w', PROJECT_DIR, + '--entrypoint', '/opt/skipper/skipper-entrypoint.sh', + FQDN_IMAGE, + ' '.join(command) + ] + assert not check_output_mock.called + popen_mock.assert_called_once_with(expected_nested_command) + @mock.patch('getpass.getuser', mock.MagicMock(autospec=True, return_value='testuser')) @mock.patch('os.getcwd', mock.MagicMock(autospec=True, return_value=PROJECT_DIR)) @mock.patch('os.path.expanduser', mock.MagicMock(autospec=True, return_value=HOME_DIR))