diff --git a/glance/async/flows/base_import.py b/glance/async/flows/base_import.py index 3416ce468b..26a09e58c6 100644 --- a/glance/async/flows/base_import.py +++ b/glance/async/flows/base_import.py @@ -13,13 +13,16 @@ # License for the specific language governing permissions and limitations # under the License. +import json import logging import os import glance_store as store_api from glance_store import backend +from oslo_concurrency import processutils as putils from oslo_config import cfg from oslo_utils import encodeutils +from oslo_utils import excutils import six from stevedore import named from taskflow.patterns import linear_flow as lf @@ -147,6 +150,29 @@ class _ImportToFS(task.Task): data = script_utils.get_image_data_iter(self.uri) path = self.store.add(image_id, data, 0, context=None)[0] + + try: + # NOTE(flaper87): Consider moving this code to a common + # place that other tasks can consume as well. + stdout, stderr = putils.trycmd('qemu-img', 'info', + '--output=json', path, + log_errors=putils.LOG_ALL_ERRORS) + except OSError as exc: + with excutils.save_and_reraise_exception(): + msg = (_LE('Failed to execute security checks on the image ' + '%(task_id)s: %(exc)s') % + {'task_id': self.task_id, 'exc': exc.message}) + LOG.error(msg) + + metadata = json.loads(stdout) + + backing_file = metadata.get('backing-filename') + if backing_file is not None: + msg = _("File %(path)s has invalid backing file " + "%(bfile)s, aborting.") % {'path': path, + 'bfile': backing_file} + raise RuntimeError(msg) + return path def revert(self, image_id, result, **kwargs): diff --git a/glance/tests/unit/async/flows/test_import.py b/glance/tests/unit/async/flows/test_import.py index 03d37c503d..7221e853d2 100644 --- a/glance/tests/unit/async/flows/test_import.py +++ b/glance/tests/unit/async/flows/test_import.py @@ -13,10 +13,12 @@ # License for the specific language governing permissions and limitations # under the License. +import json import mock import os import glance_store +from oslo_concurrency import processutils as putils from oslo_config import cfg from six.moves import cStringIO from six.moves import urllib @@ -107,16 +109,23 @@ class TestImportTask(test_utils.BaseTestCase): with mock.patch.object(script_utils, 'get_image_data_iter') as dmock: dmock.return_value = cStringIO("TEST_IMAGE") - executor.begin_processing(self.task.task_id) - image_path = os.path.join(self.test_dir, self.image.image_id) - tmp_image_path = os.path.join(self.work_dir, - "%s.tasks_import" % image_path) - self.assertFalse(os.path.exists(tmp_image_path)) - self.assertTrue(os.path.exists(image_path)) - self.assertEqual(1, len(list(self.image.locations))) - self.assertEqual("file://%s/%s" % (self.test_dir, - self.image.image_id), - self.image.locations[0]['url']) + + with mock.patch.object(putils, 'trycmd') as tmock: + tmock.return_value = (json.dumps({ + 'format': 'qcow2', + }), None) + + executor.begin_processing(self.task.task_id) + image_path = os.path.join(self.test_dir, self.image.image_id) + tmp_image_path = os.path.join(self.work_dir, + "%s.tasks_import" % image_path) + + self.assertFalse(os.path.exists(tmp_image_path)) + self.assertTrue(os.path.exists(image_path)) + self.assertEqual(1, len(list(self.image.locations))) + self.assertEqual("file://%s/%s" % (self.test_dir, + self.image.image_id), + self.image.locations[0]['url']) def test_import_flow_missing_work_dir(self): self.config(engine_mode='serial', group='taskflow_executor') @@ -190,6 +199,54 @@ class TestImportTask(test_utils.BaseTestCase): # the store as the flow failed before ImportToStore Task. self.assertFalse(os.path.exists(image_path)) + def test_import_flow_backed_file_import_to_fs(self): + self.config(engine_mode='serial', group='taskflow_executor') + + img_factory = mock.MagicMock() + + executor = taskflow_executor.TaskExecutor( + self.context, + self.task_repo, + self.img_repo, + img_factory) + + self.task_repo.get.return_value = self.task + + def create_image(*args, **kwargs): + kwargs['image_id'] = UUID1 + return self.img_factory.new_image(*args, **kwargs) + + self.img_repo.get.return_value = self.image + img_factory.new_image.side_effect = create_image + + with mock.patch.object(script_utils, 'get_image_data_iter') as dmock: + dmock.return_value = cStringIO("TEST_IMAGE") + + with mock.patch.object(putils, 'trycmd') as tmock: + tmock.return_value = (json.dumps({ + 'backing-filename': '/etc/password' + }), None) + + with mock.patch.object(import_flow._ImportToFS, + 'revert') as rmock: + self.assertRaises(RuntimeError, + executor.begin_processing, + self.task.task_id) + self.assertTrue(rmock.called) + self.assertIsInstance(rmock.call_args[1]['result'], + failure.Failure) + + image_path = os.path.join(self.test_dir, + self.image.image_id) + + fname = "%s.tasks_import" % image_path + tmp_image_path = os.path.join(self.work_dir, fname) + + self.assertFalse(os.path.exists(tmp_image_path)) + # Note(sabari): The image should not have been uploaded to + # the store as the flow failed before ImportToStore Task. + self.assertFalse(os.path.exists(image_path)) + def test_import_flow_revert(self): self.config(engine_mode='serial', group='taskflow_executor') @@ -214,20 +271,31 @@ class TestImportTask(test_utils.BaseTestCase): with mock.patch.object(script_utils, 'get_image_data_iter') as dmock: dmock.return_value = cStringIO("TEST_IMAGE") - with mock.patch.object(import_flow, "_get_import_flows") as imock: - imock.return_value = (x for x in [_ErrorTask()]) - self.assertRaises(RuntimeError, - executor.begin_processing, self.task.task_id) - image_path = os.path.join(self.test_dir, self.image.image_id) - tmp_image_path = os.path.join(self.work_dir, - "%s.tasks_import" % image_path) - self.assertFalse(os.path.exists(tmp_image_path)) + with mock.patch.object(putils, 'trycmd') as tmock: + tmock.return_value = (json.dumps({ + 'format': 'qcow2', + }), None) - # NOTE(flaper87): Eventually, we want this to be assertTrue. - # The current issue is there's no way to tell taskflow to - # continue on failures. That is, revert the subflow but keep - # executing the parent flow. Under discussion/development. - self.assertFalse(os.path.exists(image_path)) + with mock.patch.object(import_flow, + "_get_import_flows") as imock: + imock.return_value = (x for x in [_ErrorTask()]) + self.assertRaises(RuntimeError, + executor.begin_processing, + self.task.task_id) + + image_path = os.path.join(self.test_dir, + self.image.image_id) + tmp_image_path = os.path.join(self.work_dir, + ("%s.tasks_import" % + image_path)) + self.assertFalse(os.path.exists(tmp_image_path)) + + # NOTE(flaper87): Eventually, we want this to be assertTrue + # The current issue is there's no way to tell taskflow to + # continue on failures. That is, revert the subflow but + # keep executing the parent flow. Under + # discussion/development. + self.assertFalse(os.path.exists(image_path)) def test_import_flow_no_import_flows(self): self.config(engine_mode='serial', @@ -310,15 +378,20 @@ class TestImportTask(test_utils.BaseTestCase): with mock.patch.object(script_utils, 'get_image_data_iter') as dmock: dmock.return_value = "test" - image_id = UUID1 - path = import_fs.execute(image_id) - reader, size = glance_store.get_from_backend(path) - self.assertEqual(4, size) - self.assertEqual(dmock.return_value, "".join(reader)) + with mock.patch.object(putils, 'trycmd') as tmock: + tmock.return_value = (json.dumps({ + 'format': 'qcow2', + }), None) - image_path = os.path.join(self.work_dir, image_id) - tmp_image_path = os.path.join(self.work_dir, image_path) - self.assertTrue(os.path.exists(tmp_image_path)) + image_id = UUID1 + path = import_fs.execute(image_id) + reader, size = glance_store.get_from_backend(path) + self.assertEqual(4, size) + self.assertEqual(dmock.return_value, "".join(reader)) + + image_path = os.path.join(self.work_dir, image_id) + tmp_image_path = os.path.join(self.work_dir, image_path) + self.assertTrue(os.path.exists(tmp_image_path)) def test_delete_from_fs(self): delete_fs = import_flow._DeleteFromFS(self.task.task_id,