From 546d03e7ac35a765860c32ae58ed53459f06cf9e Mon Sep 17 00:00:00 2001 From: Lingxian Kong Date: Thu, 12 Apr 2018 20:10:03 +1200 Subject: [PATCH] Include md5 in function package path This is the preliminary patch for function versioning support. In order to create a new function version, we need to check if the function package data has been changed compared with the latest version. The package md5 is stored as part of the package file path. Change-Id: Icf7c3999033989762b31bb629571bab6ad06b05e Story: #2001829 Task: #12586 --- qinling/api/controllers/v1/function.py | 33 ++++++-- qinling/storage/base.py | 22 +++++- qinling/storage/file_system.py | 34 +++++--- .../unit/api/controllers/v1/test_function.py | 35 ++++++++- qinling/tests/unit/base.py | 2 +- .../tests/unit/storage/test_file_system.py | 78 +++++++++++-------- 6 files changed, 146 insertions(+), 58 deletions(-) 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()