diff --git a/qinling/api/controllers/v1/function.py b/qinling/api/controllers/v1/function.py index 445925b0..2e5cf975 100644 --- a/qinling/api/controllers/v1/function.py +++ b/qinling/api/controllers/v1/function.py @@ -107,7 +107,8 @@ class FunctionsController(rest.RestController): source = func_db.code['source'] if source == constants.PACKAGE_FUNCTION: - f = self.storage_provider.retrieve(ctx.projectid, id) + f = self.storage_provider.retrieve(ctx.projectid, id, + func_db.code['md5sum']) elif source == constants.SWIFT_FUNCTION: container = func_db.code['swift']['container'] obj = func_db.code['swift']['object'] @@ -187,14 +188,19 @@ class FunctionsController(rest.RestController): 'Trust creation failed for function.' ) + # Create function and store the package data inside a db transaction so + # that the function won't be created if any error happened during + # package store. with db_api.transaction(): func_db = db_api.create_function(values) - if store: try: ctx = context.get_ctx() - self.storage_provider.store(ctx.projectid, func_db.id, - data, md5sum=md5sum) + actual_md5 = self.storage_provider.store( + ctx.projectid, func_db.id, data, md5sum=md5sum + ) + values['code'].update({"md5sum": actual_md5}) + func_db = db_api.update_function(func_db.id, values) except Exception as e: LOG.exception("Failed to store function package.") keystone_util.delete_trust(values['trust_id']) @@ -254,7 +260,8 @@ class FunctionsController(rest.RestController): source = func_db.code['source'] if source == constants.PACKAGE_FUNCTION: - self.storage_provider.delete(func_db.project_id, id) + self.storage_provider.delete(func_db.project_id, id, + func_db.code['md5sum']) # Delete all resources created by orchestrator asynchronously. self.engine_client.delete_function(id) @@ -282,6 +289,8 @@ class FunctionsController(rest.RestController): values = {} for key in UPDATE_ALLOWED: if kwargs.get(key) is not None: + if key == "code": + kwargs[key] = json.loads(kwargs[key]) values.update({key: kwargs[key]}) LOG.info('Update function %s, params: %s', id, values) @@ -291,6 +300,7 @@ class FunctionsController(rest.RestController): func_db = db_api.update_function(id, values) else: source = values.get('code', {}).get('source') + md5sum = values.get('code', {}).get('md5sum') with db_api.transaction(): pre_func = db_api.get_function(id) @@ -300,10 +310,15 @@ class FunctionsController(rest.RestController): ) pre_source = pre_func.code['source'] + pre_md5sum = pre_func.code['md5sum'] if source and source != pre_source: raise exc.InputException( "The function code type can not be changed." ) + if md5sum and md5sum == pre_md5sum: + raise exc.InputException( + "The function code checksum is not changed." + ) if source == constants.IMAGE_FUNCTION: raise exc.InputException( "The image type function code can not be changed." @@ -312,10 +327,14 @@ class FunctionsController(rest.RestController): values.get('package') is not None): # Update the package data. data = values['package'].file.read() - self.storage_provider.store( + md5sum = self.storage_provider.store( ctx.projectid, id, - data + data, + md5sum=md5sum + ) + values.setdefault('code', {}).update( + {"md5sum": md5sum, "source": pre_source} ) values.pop('package') if pre_source == constants.SWIFT_FUNCTION: diff --git a/qinling/storage/base.py b/qinling/storage/base.py index ffe3dcbf..dbeb3c58 100644 --- a/qinling/storage/base.py +++ b/qinling/storage/base.py @@ -27,15 +27,31 @@ class PackageStorage(object): """PackageStorage interface.""" @abc.abstractmethod - def store(self, project_id, funtion, data, **kwargs): + def store(self, project_id, function, data, **kwargs): + """Store the function package data. + + :param project_id: Project ID. + :param function: Function ID. + :param data: Package file content. + :param kwargs: A dict may including + - md5sum: The MD5 provided by the user. + :return: MD5 value of the package. + """ raise NotImplementedError @abc.abstractmethod - def retrieve(self, project_id, function): + def retrieve(self, project_id, function, md5sum): + """Get function package data. + + :param project_id: Project ID. + :param function: Function ID. + :param md5sum: The function MD5. + :return: File descriptor that needs to close outside. + """ raise NotImplementedError @abc.abstractmethod - def delete(self, project_id, function): + def delete(self, project_id, function, md5sum): raise NotImplementedError diff --git a/qinling/storage/file_system.py b/qinling/storage/file_system.py index 20e5d890..0e19b523 100644 --- a/qinling/storage/file_system.py +++ b/qinling/storage/file_system.py @@ -23,6 +23,9 @@ from qinling.storage import base from qinling.utils import common LOG = logging.getLogger(__name__) +PACKAGE_NAME_TEMPLATE = "%s_%s.zip" +# Package path name including project ID +PACKAGE_PATH_TEMPLATE = "%s/%s_%s.zip" class FileSystemStorage(base.PackageStorage): @@ -37,6 +40,8 @@ class FileSystemStorage(base.PackageStorage): :param project_id: Project ID. :param function: Function ID. :param data: Package file content. + :param md5sum: The MD5 provided by the user. + :return: MD5 value of the package. """ LOG.debug( 'Store package, function: %s, project: %s', function, project_id @@ -45,14 +50,16 @@ class FileSystemStorage(base.PackageStorage): project_path = os.path.join(self.base_path, project_id) fileutils.ensure_tree(project_path) - new_func_zip = os.path.join(project_path, '%s.zip.new' % function) - func_zip = os.path.join(project_path, '%s.zip' % function) - # Check md5 md5_actual = common.md5(content=data) if md5sum and md5_actual != md5sum: raise exc.InputException("Package md5 mismatch.") + # The md5 is contained in the package path. + new_func_zip = os.path.join(project_path, '%s.zip.new' % function) + func_zip = os.path.join(project_path, + PACKAGE_NAME_TEMPLATE % (function, md5_actual)) + # Store package with open(new_func_zip, 'wb') as fd: fd.write(data) @@ -62,20 +69,24 @@ class FileSystemStorage(base.PackageStorage): raise exc.InputException("Package is not a valid ZIP package.") os.rename(new_func_zip, func_zip) + return md5_actual - def retrieve(self, project_id, function): + def retrieve(self, project_id, function, md5sum): """Get function package data. :param project_id: Project ID. :param function: Function ID. + :param md5sum: The function MD5. :return: File descriptor that needs to close outside. """ LOG.debug( - 'Get package data, function: %s, project: %s', function, project_id + 'Getting package data, function: %s, md5sum: %s, project: %s', + function, md5sum, project_id ) func_zip = os.path.join( - self.base_path, '%s/%s.zip' % (project_id, function) + self.base_path, + PACKAGE_PATH_TEMPLATE % (project_id, function, md5sum) ) if not os.path.exists(func_zip): @@ -85,18 +96,19 @@ class FileSystemStorage(base.PackageStorage): ) f = open(func_zip, 'rb') - LOG.debug('Found package data') + LOG.debug('Found package data for function %s', function) return f - def delete(self, project_id, function): + def delete(self, project_id, function, md5sum): LOG.debug( - 'Delete package data, function: %s, project: %s', function, - project_id + 'Deleting package data, function: %s, md5sum: %s, project: %s', + function, md5sum, project_id ) func_zip = os.path.join( - self.base_path, '%s/%s.zip' % (project_id, function) + self.base_path, + PACKAGE_PATH_TEMPLATE % (project_id, function, md5sum) ) if os.path.exists(func_zip): diff --git a/qinling/tests/unit/api/controllers/v1/test_function.py b/qinling/tests/unit/api/controllers/v1/test_function.py index 7557f601..b984793e 100644 --- a/qinling/tests/unit/api/controllers/v1/test_function.py +++ b/qinling/tests/unit/api/controllers/v1/test_function.py @@ -37,6 +37,8 @@ class TestFunctionController(base.APITest): @mock.patch('qinling.storage.file_system.FileSystemStorage.store') def test_post(self, mock_store): + mock_store.return_value = 'fake_md5' + with tempfile.NamedTemporaryFile() as f: body = { 'name': self.rand_name('function', prefix=TEST_CASE_NAME), @@ -52,7 +54,12 @@ class TestFunctionController(base.APITest): self.assertEqual(201, resp.status_int) self.assertEqual(1, mock_store.call_count) - body.update({'entry': 'main.main', 'code': {"source": "package"}}) + body.update( + { + 'entry': 'main.main', + 'code': {"source": "package", "md5sum": "fake_md5"} + } + ) self._assertDictContainsSubset(resp.json, body) @mock.patch('qinling.utils.openstack.keystone.get_swiftclient') @@ -90,7 +97,7 @@ class TestFunctionController(base.APITest): ) expected = { 'id': db_func.id, - "code": {"source": "package"}, + "code": {"source": "package", "md5sum": "fake_md5"}, "name": db_func.name, 'entry': 'main.main', "project_id": unit_base.DEFAULT_PROJECT_ID, @@ -107,7 +114,6 @@ class TestFunctionController(base.APITest): ) expected = { 'id': db_func.id, - "code": json.dumps({"source": "package"}), "name": db_func.name, 'entry': 'main.main', "project_id": unit_base.DEFAULT_PROJECT_ID, @@ -140,6 +146,7 @@ class TestFunctionController(base.APITest): db_func = self.create_function( runtime_id=self.runtime_id, prefix=TEST_CASE_NAME ) + mock_store.return_value = "fake_md5_changed" with tempfile.NamedTemporaryFile() as f: resp = self.app.put( @@ -150,9 +157,29 @@ class TestFunctionController(base.APITest): self.assertEqual(200, resp.status_int) self.assertEqual(1, mock_store.call_count) + self.assertEqual('fake_md5_changed', resp.json['code'].get('md5sum')) mock_delete_func.assert_called_once_with(db_func.id) mock_etcd_del.assert_called_once_with(db_func.id) + def test_put_package_same_md5(self): + db_func = self.create_function( + runtime_id=self.runtime_id, prefix=TEST_CASE_NAME + ) + + with tempfile.NamedTemporaryFile() as f: + resp = self.app.put( + '/v1/functions/%s' % db_func.id, + params={ + "code": json.dumps( + {"md5sum": "fake_md5", "source": "package"} + ) + }, + upload_files=[('package', f.name, f.read())], + expect_errors=True + ) + + self.assertEqual(400, resp.status_int) + @mock.patch('qinling.utils.etcd_util.delete_function') @mock.patch('qinling.rpc.EngineClient.delete_function') @mock.patch('qinling.storage.file_system.FileSystemStorage.delete') @@ -164,7 +191,7 @@ class TestFunctionController(base.APITest): self.assertEqual(204, resp.status_int) mock_delete.assert_called_once_with( - unit_base.DEFAULT_PROJECT_ID, db_func.id + unit_base.DEFAULT_PROJECT_ID, db_func.id, "fake_md5" ) mock_delete_func.assert_called_once_with(db_func.id) mock_etcd_delete.assert_called_once_with(db_func.id) diff --git a/qinling/tests/unit/base.py b/qinling/tests/unit/base.py index 59e23e2e..1f3a91cd 100644 --- a/qinling/tests/unit/base.py +++ b/qinling/tests/unit/base.py @@ -186,7 +186,7 @@ class DbTestCase(BaseTest): { 'name': self.rand_name('function', prefix=prefix), 'runtime_id': runtime_id, - 'code': {"source": "package"}, + 'code': {"source": "package", "md5sum": "fake_md5"}, 'entry': 'main.main', # 'auth_enable' is disabled by default, we create runtime for # default tenant. diff --git a/qinling/tests/unit/storage/test_file_system.py b/qinling/tests/unit/storage/test_file_system.py index 2e23e904..c89732c9 100644 --- a/qinling/tests/unit/storage/test_file_system.py +++ b/qinling/tests/unit/storage/test_file_system.py @@ -21,6 +21,7 @@ from qinling import config from qinling import exceptions as exc from qinling.storage import file_system from qinling.tests.unit import base +from qinling.utils import common CONF = cfg.CONF FAKE_STORAGE_PATH = 'TMP_DIR' @@ -46,20 +47,23 @@ class TestFileSystemStorage(base.BaseTest): function = self.rand_name('function', prefix='TestFileSystemStorage') # For python3, data should be encoded into bytes before hashing. function_data = "Some data".encode('utf8') + md5 = common.md5(content=function_data) self.storage.store(self.project_id, function, function_data) + temp_package_path = os.path.join(FAKE_STORAGE_PATH, self.project_id, + '%s.zip.new' % function) + package_path = os.path.join( + FAKE_STORAGE_PATH, + file_system.PACKAGE_PATH_TEMPLATE % (self.project_id, function, + md5) + ) ensure_tree_mock.assert_called_once_with( - os.path.join(FAKE_STORAGE_PATH, self.project_id)) + os.path.join(FAKE_STORAGE_PATH, self.project_id) + ) fake_fd.write.assert_called_once_with(function_data) - is_zipfile_mock.assert_called_once_with( - os.path.join(FAKE_STORAGE_PATH, self.project_id, - '%s.zip.new' % function)) - rename_mock.assert_called_once_with( - os.path.join(FAKE_STORAGE_PATH, self.project_id, - '%s.zip.new' % function), - os.path.join(FAKE_STORAGE_PATH, self.project_id, - '%s.zip' % function)) + is_zipfile_mock.assert_called_once_with(temp_package_path) + rename_mock.assert_called_once_with(temp_package_path, package_path) @mock.patch('oslo_utils.fileutils.ensure_tree') def test_store_md5_mismatch(self, ensure_tree_mock): @@ -113,15 +117,15 @@ class TestFileSystemStorage(base.BaseTest): open_mock.return_value = fake_fd function = self.rand_name('function', prefix='TestFileSystemStorage') - ret = self.storage.retrieve(self.project_id, function) + ret = self.storage.retrieve(self.project_id, function, "fake_md5") - exists_mock.assert_called_once_with( - os.path.join(FAKE_STORAGE_PATH, self.project_id, - '%s.zip' % function)) - open_mock.assert_called_once_with( - os.path.join(FAKE_STORAGE_PATH, self.project_id, - '%s.zip' % function), - 'rb') + package_path = os.path.join( + FAKE_STORAGE_PATH, + file_system.PACKAGE_PATH_TEMPLATE % (self.project_id, function, + "fake_md5") + ) + exists_mock.assert_called_once_with(package_path) + open_mock.assert_called_once_with(package_path, 'rb') self.assertEqual(fake_fd, ret) @mock.patch('os.path.exists') @@ -134,11 +138,17 @@ class TestFileSystemStorage(base.BaseTest): "^Package of function %s for project %s not found\.$" % ( function, self.project_id), self.storage.retrieve, - self.project_id, function) + self.project_id, + function, + "fake_md5" + ) - exists_mock.assert_called_once_with( - os.path.join(FAKE_STORAGE_PATH, self.project_id, - '%s.zip' % function)) + package_path = os.path.join( + FAKE_STORAGE_PATH, + file_system.PACKAGE_PATH_TEMPLATE % (self.project_id, function, + "fake_md5") + ) + exists_mock.assert_called_once_with(package_path) @mock.patch('os.path.exists') @mock.patch('os.remove') @@ -146,14 +156,15 @@ class TestFileSystemStorage(base.BaseTest): exists_mock.return_value = True function = self.rand_name('function', prefix='TestFileSystemStorage') - self.storage.delete(self.project_id, function) + self.storage.delete(self.project_id, function, "fake_md5") - exists_mock.assert_called_once_with( - os.path.join(FAKE_STORAGE_PATH, self.project_id, - '%s.zip' % function)) - remove_mock.assert_called_once_with( - os.path.join(FAKE_STORAGE_PATH, self.project_id, - '%s.zip' % function)) + package_path = os.path.join( + FAKE_STORAGE_PATH, + file_system.PACKAGE_PATH_TEMPLATE % (self.project_id, function, + "fake_md5") + ) + exists_mock.assert_called_once_with(package_path) + remove_mock.assert_called_once_with(package_path) @mock.patch('os.path.exists') @mock.patch('os.remove') @@ -161,9 +172,12 @@ class TestFileSystemStorage(base.BaseTest): exists_mock.return_value = False function = self.rand_name('function', prefix='TestFileSystemStorage') - self.storage.delete(self.project_id, function) + self.storage.delete(self.project_id, function, "fake_md5") - exists_mock.assert_called_once_with( - os.path.join(FAKE_STORAGE_PATH, self.project_id, - '%s.zip' % function)) + package_path = os.path.join( + FAKE_STORAGE_PATH, + file_system.PACKAGE_PATH_TEMPLATE % (self.project_id, function, + "fake_md5") + ) + exists_mock.assert_called_once_with(package_path) remove_mock.assert_not_called()