diff --git a/murano/api/v1/catalog.py b/murano/api/v1/catalog.py index 74d9e0dd..ffe5e511 100644 --- a/murano/api/v1/catalog.py +++ b/murano/api/v1/catalog.py @@ -149,7 +149,6 @@ class Controller(object): policy.check('publicize_package', req.context) break package = db_api.package_update(package_id, body, req.context) - return package.to_dict() def get(self, req, package_id): diff --git a/murano/common/engine.py b/murano/common/engine.py index e99d0974..ed8a8adf 100755 --- a/murano/common/engine.py +++ b/murano/common/engine.py @@ -139,7 +139,8 @@ class TaskExecutor(object): murano_client_factory = lambda: \ self._environment.clients.get_murano_client(self._environment) with package_loader.ApiPackageLoader( - murano_client_factory) as pkg_loader: + murano_client_factory, + self._environment.tenant_id) as pkg_loader: return self._execute(pkg_loader) finally: if self._model['Objects'] is None: diff --git a/murano/db/catalog/api.py b/murano/db/catalog/api.py index a2455ed4..100f81b3 100644 --- a/murano/db/catalog/api.py +++ b/murano/db/catalog/api.py @@ -235,11 +235,22 @@ def package_update(pkg_id_or_name, changes, context): session = db_session.get_session() with session.begin(): pkg = _package_get(pkg_id_or_name, session) + was_private = not pkg.is_public if not context.is_admin: _authorize_package(pkg, context) for change in changes: pkg = operation_methods[change['op']](pkg, change) + became_public = pkg.is_public + class_names = [clazz.name for clazz in pkg.class_definitions] + if was_private and became_public: + with db_session.get_lock("public_packages", session): + _check_for_existing_classes(session, class_names, None, + check_public=True, + ignore_package_with_id=pkg.id) + _check_for_public_packages_with_fqn(session, + pkg.fully_qualified_name, + pkg.id) session.add(pkg) return pkg @@ -359,16 +370,37 @@ def package_upload(values, tenant_id): composite_attr_to_func = {'categories': _get_categories, 'tags': _get_tags, 'class_definitions': _get_class_definitions} - with session.begin(): + is_public = values.get('is_public', False) + + if is_public: + public_lock = db_session.get_lock("public_packages", session) + else: + public_lock = None + tenant_lock = db_session.get_lock("classes_of_" + tenant_id, session) + try: + _check_for_existing_classes(session, values.get('class_definitions'), + tenant_id, check_public=is_public) + if is_public: + _check_for_public_packages_with_fqn( + session, + values.get('fully_qualified_name')) for attr, func in composite_attr_to_func.iteritems(): if values.get(attr): result = func(values[attr], session) setattr(package, attr, result) del values[attr] - package.update(values) package.owner_id = tenant_id package.save(session) + tenant_lock.commit() + if public_lock is not None: + public_lock.commit() + except Exception: + tenant_lock.rollback() + if public_lock is not None: + public_lock.rollback() + raise + return package @@ -442,3 +474,49 @@ def category_delete(category_id): LOG.error(msg) raise exc.HTTPNotFound(msg) session.delete(category) + + +def _check_for_existing_classes(session, class_names, tenant_id, + check_public=False, + ignore_package_with_id=None): + if not class_names: + return + q = session.query(models.Class.name).filter( + models.Class.name.in_(class_names)) + private_filter = None + public_filter = None + predicate = None + if tenant_id is not None: + private_filter = models.Class.package.has( + models.Package.owner_id == tenant_id) + if check_public: + public_filter = models.Class.package.has( + models.Package.is_public) + if private_filter is not None and public_filter is not None: + predicate = sa.or_(private_filter, public_filter) + elif private_filter is not None: + predicate = private_filter + elif public_filter is not None: + predicate = public_filter + if predicate is not None: + q = q.filter(predicate) + if ignore_package_with_id is not None: + q = q.filter(models.Class.package_id != ignore_package_with_id) + if q.first() is not None: + msg = _('Class with the same full name is already ' + 'registered in the visibility scope') + LOG.error(msg) + raise exc.HTTPConflict(msg) + + +def _check_for_public_packages_with_fqn(session, fqn, + ignore_package_with_id=None): + q = session.query(models.Package.id).\ + filter(models.Package.is_public).\ + filter(models.Package.fully_qualified_name == fqn) + if ignore_package_with_id is not None: + q = q.filter(models.Package.id != ignore_package_with_id) + if q.first() is not None: + msg = _('Package with the same Name is already made public') + LOG.error(msg) + raise exc.HTTPConflict(msg) diff --git a/murano/db/migration/alembic_migrations/versions/008_fix_unique_constraints.py b/murano/db/migration/alembic_migrations/versions/008_fix_unique_constraints.py new file mode 100644 index 00000000..1a090e44 --- /dev/null +++ b/murano/db/migration/alembic_migrations/versions/008_fix_unique_constraints.py @@ -0,0 +1,199 @@ +# Copyright 2015 OpenStack Foundation. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +from alembic import op +import sqlalchemy as sa + +import murano.db.migration.helpers as helpers +from murano.db.sqla import types as st + +"""fix_unique_constraints + +Revision ID: 008 +Revises: 007 +Create Date: 2015-04-08 14:01:06.458512 + +""" + +# revision identifiers, used by Alembic. +revision = '008' +down_revision = '007' + +MYSQL_ENGINE = 'InnoDB' +MYSQL_CHARSET = 'utf8' + + +def upgrade(): + engine = op.get_bind() + if engine.dialect.dialect_description == 'mysql+mysqldb': + engine.execute('SET FOREIGN_KEY_CHECKS=0') + if engine.dialect.dialect_description == 'postgresql+psycopg2': + op.drop_constraint('package_to_tag_package_id_fkey', + 'package_to_tag', 'foreignkey') + op.drop_constraint('package_to_tag_tag_id_fkey', + 'package_to_tag', 'foreignkey') + op.drop_constraint('package_to_category_package_id_fkey', + 'package_to_category', 'foreignkey') + op.drop_constraint('class_definition_package_id_fkey', + 'class_definition', 'foreignkey') + helpers.transform_table( + 'package', {}, {}, + sa.Column('created', sa.DateTime(), nullable=False), + sa.Column('updated', sa.DateTime(), nullable=False), + sa.Column('id', sa.String(length=36), nullable=False), + sa.Column('archive', st.LargeBinary(), nullable=True), + sa.Column('fully_qualified_name', + sa.String(length=128), + nullable=False), + sa.Column('type', sa.String(length=20), nullable=False), + sa.Column('author', sa.String(length=80), nullable=True), + sa.Column('name', sa.String(length=80), nullable=False), + sa.Column('enabled', sa.Boolean(), nullable=True), + sa.Column('description', sa.Text(), nullable=False), + sa.Column('is_public', sa.Boolean(), nullable=True), + sa.Column('logo', st.LargeBinary(), nullable=True), + sa.Column('owner_id', sa.String(length=36), nullable=False), + sa.Column('ui_definition', sa.Text(), nullable=True), + sa.Column('supplier_logo', sa.types.LargeBinary), + sa.Column('supplier', sa.types.Text()), + sa.PrimaryKeyConstraint('id'), + mysql_engine=MYSQL_ENGINE, + mysql_charset=MYSQL_CHARSET + ) + + helpers.transform_table( + 'class_definition', {}, {}, + sa.Column('created', sa.DateTime(), nullable=False), + sa.Column('updated', sa.DateTime(), nullable=False), + sa.Column('id', sa.String(length=36), nullable=False), + sa.Column('name', + sa.String(length=128), + nullable=False), + sa.Column('package_id', sa.String(length=36), nullable=True), + sa.ForeignKeyConstraint(['package_id'], ['package.id'], ), + sa.PrimaryKeyConstraint('id'), + mysql_engine=MYSQL_ENGINE, + mysql_charset=MYSQL_CHARSET + ) + + if engine.dialect.dialect_description == 'mysql+mysqldb': + engine.execute('SET FOREIGN_KEY_CHECKS=1') + + if engine.dialect.dialect_description == 'postgresql+psycopg2': + op.create_foreign_key('package_to_tag_package_id_fkey', + 'package_to_tag', 'package', + ['package_id'], ['id']) + + op.create_foreign_key('package_to_tag_tag_id_fkey', + 'package_to_tag', 'tag', + ['tag_id'], ['id']) + + op.create_foreign_key('package_to_category_package_id_fkey', + 'package_to_category', 'package', + ['package_id'], ['id']) + + op.create_foreign_key('class_definition_package_id_fkey', + 'class_definition', 'package', + ['package_id'], ['id']) + + op.create_index('ix_package_fqn_and_owner', table_name='package', + columns=['fully_qualified_name', 'owner_id'], unique=True) + op.create_index('ix_class_definition_name', + 'class_definition', + ['name']) + + +def downgrade(): + op.drop_index('ix_package_fqn_and_owner', table_name='package') + op.drop_index('ix_class_definition_name', table_name='class_definition') + + engine = op.get_bind() + if engine.dialect.dialect_description == 'mysql+mysqldb': + engine.execute('SET FOREIGN_KEY_CHECKS=0') + if engine.dialect.dialect_description == 'postgresql+psycopg2': + op.drop_constraint('package_to_tag_package_id_fkey', + 'package_to_tag', 'foreignkey') + op.drop_constraint('package_to_tag_tag_id_fkey', + 'package_to_tag', 'foreignkey') + op.drop_constraint('package_to_category_package_id_fkey', + 'package_to_category', 'foreignkey') + op.drop_constraint('class_definition_package_id_fkey', + 'class_definition', 'foreignkey') + + helpers.transform_table( + 'class_definition', {}, {}, + sa.Column('created', sa.DateTime(), nullable=False), + sa.Column('updated', sa.DateTime(), nullable=False), + sa.Column('id', sa.String(length=36), nullable=False), + sa.Column('name', + sa.String(length=128), + nullable=False, + unique=True), + sa.Column('package_id', sa.String(length=36), nullable=True), + sa.PrimaryKeyConstraint('id'), + mysql_engine=MYSQL_ENGINE, + mysql_charset=MYSQL_CHARSET + ) + + helpers.transform_table( + 'package', {}, {}, + sa.Column('created', sa.DateTime(), nullable=False), + sa.Column('updated', sa.DateTime(), nullable=False), + sa.Column('id', sa.String(length=36), nullable=False), + sa.Column('archive', st.LargeBinary(), nullable=True), + sa.Column('fully_qualified_name', + sa.String(length=128), + nullable=False, + unique=True), + sa.Column('type', sa.String(length=20), nullable=False), + sa.Column('author', sa.String(length=80), nullable=True), + sa.Column('name', sa.String(length=80), nullable=False), + sa.Column('enabled', sa.Boolean(), nullable=True), + sa.Column('description', sa.Text(), nullable=False), + sa.Column('is_public', sa.Boolean(), nullable=True), + sa.Column('logo', st.LargeBinary(), nullable=True), + sa.Column('owner_id', sa.String(length=36), nullable=False), + sa.Column('ui_definition', sa.Text(), nullable=True), + sa.Column('supplier_logo', sa.types.LargeBinary), + sa.Column('supplier', sa.types.Text()), + sa.PrimaryKeyConstraint('id'), + mysql_engine=MYSQL_ENGINE, + mysql_charset=MYSQL_CHARSET + ) + + if engine.dialect.dialect_description == 'mysql+mysqldb': + engine.execute('SET FOREIGN_KEY_CHECKS=1') + + if engine.dialect.dialect_description == 'postgresql+psycopg2': + op.create_foreign_key('package_to_tag_package_id_fkey', + 'package_to_tag', 'package', + ['package_id'], ['id']) + + op.create_foreign_key('package_to_tag_tag_id_fkey', + 'package_to_tag', 'tag', + ['tag_id'], ['id']) + + op.create_foreign_key('package_to_category_package_id_fkey', + 'package_to_category', 'package', + ['package_id'], ['id']) + + op.create_foreign_key('class_definition_package_id_fkey', + 'class_definition', 'package', + ['package_id'], ['id']) + + op.create_index('ix_class_definition_name', + 'class_definition', + ['name']) diff --git a/murano/db/models.py b/murano/db/models.py index 134ac6c8..a52e36ae 100644 --- a/murano/db/models.py +++ b/murano/db/models.py @@ -220,15 +220,15 @@ class Instance(Base): class Package(Base, TimestampMixin): """Represents a meta information about application package.""" __tablename__ = 'package' - + __table_args__ = (sa.Index('ix_package_fqn_and_owner', + 'fully_qualified_name', + 'owner_id', unique=True),) id = sa.Column(sa.String(36), primary_key=True, default=uuidutils.generate_uuid) archive = sa.Column(st.LargeBinary()) fully_qualified_name = sa.Column(sa.String(128), - nullable=False, - index=True, - unique=True) + nullable=False) type = sa.Column(sa.String(20), nullable=False, default='class') author = sa.Column(sa.String(80), default='Openstack') supplier = sa.Column(st.JsonBlob(), nullable=True, default={}) @@ -251,7 +251,8 @@ class Package(Base, TimestampMixin): cascade='save-update, merge', lazy='joined') class_definitions = sa_orm.relationship( - "Class", cascade='save-update, merge, delete', lazy='joined') + "Class", cascade='save-update, merge, delete', lazy='joined', + backref='package') def to_dict(self): d = self.__dict__.copy() diff --git a/murano/engine/package_loader.py b/murano/engine/package_loader.py index 8a9297f6..2a869b87 100644 --- a/murano/engine/package_loader.py +++ b/murano/engine/package_loader.py @@ -45,12 +45,13 @@ class PackageLoader(six.with_metaclass(abc.ABCMeta)): class ApiPackageLoader(PackageLoader): - def __init__(self, murano_client_factory): + def __init__(self, murano_client_factory, tenant_id): self._cache_directory = self._get_cache_directory() self._murano_client_factory = murano_client_factory + self.tenant_id = tenant_id def get_package_by_class(self, name): - filter_opts = {'class_name': name, 'limit': 1} + filter_opts = {'class_name': name} try: package_definition = self._get_definition(filter_opts) except LookupError: @@ -59,7 +60,7 @@ class ApiPackageLoader(PackageLoader): return self._get_package_by_definition(package_definition) def get_package(self, name): - filter_opts = {'fqn': name, 'limit': 1} + filter_opts = {'fqn': name} try: package_definition = self._get_definition(filter_opts) except LookupError: @@ -82,15 +83,19 @@ class ApiPackageLoader(PackageLoader): def _get_definition(self, filter_opts): try: - packages = self._murano_client_factory().packages.filter( - **filter_opts) - try: - return packages.next() - except StopIteration: + packages = list(self._murano_client_factory().packages.filter( + **filter_opts)) + if len(packages) > 1: + LOG.debug('Ambiguous package resolution: ' + 'more then 1 package found for query "{0}", ' + 'will resolve based on the ownership'. + format(filter_opts)) + return get_best_package_match(packages, self.tenant_id) + elif len(packages) == 1: + return packages[0] + else: LOG.debug('There are no packages matching filter ' '{0}'.format(filter_opts)) - # TODO(smelikyan): This exception should be replaced with one - # defined in python-muranoclient raise LookupError() except muranoclient_exc.HTTPException: LOG.debug('Failed to get package definition from repository') @@ -187,3 +192,19 @@ class DirectoryPackageLoader(PackageLoader): self._packages_by_name[package.full_name] = package self._processed_entries.add(entry) + + +def get_best_package_match(packages, tenant_id): + public = None + other = [] + for package in packages: + if package.owner_id == tenant_id: + return package + elif package.is_public: + public = package + else: + other.append(package) + if public is not None: + return public + elif other: + return other[0] diff --git a/murano/tests/unit/db/test_catalog.py b/murano/tests/unit/db/test_catalog.py index 03451ab6..da8e3dba 100644 --- a/murano/tests/unit/db/test_catalog.py +++ b/murano/tests/unit/db/test_catalog.py @@ -26,14 +26,16 @@ class CatalogDBTestCase(base.MuranoWithDBTestCase): def setUp(self): super(CatalogDBTestCase, self).setUp() self.tenant_id = str(uuid.uuid4()) + self.tenant_id_2 = str(uuid.uuid4()) self.context = utils.dummy_context(tenant_id=self.tenant_id) + self.context_2 = utils.dummy_context(tenant_id=self.tenant_id_2) def _create_categories(self): api.category_add('cat1') api.category_add('cat2') - def _stub_package(self): - return { + def _stub_package(self, **kwargs): + base = { 'archive': "archive blob here", 'fully_qualified_name': 'com.example.package', 'type': 'class', @@ -46,6 +48,15 @@ class CatalogDBTestCase(base.MuranoWithDBTestCase): 'logo': "logo blob here", 'ui_definition': '{}', } + base.update(**kwargs) + return base + + def get_change(self, op, path, value): + return { + 'op': op, + 'path': path, + 'value': value + } def test_list_empty_categories(self): res = api.category_get_names() @@ -87,3 +98,97 @@ class CatalogDBTestCase(base.MuranoWithDBTestCase): self.assertRaises(exc.HTTPNotFound, api.package_get, package.id, self.context) + + def test_package_upload_to_different_tenants_with_same_fqn(self): + values = self._stub_package() + + api.package_upload(values, self.tenant_id) + api.package_upload(values, self.tenant_id_2) + + def test_package_upload_public_public_fqn_violation(self): + values = self._stub_package(is_public=True) + api.package_upload(values, self.tenant_id) + values = self._stub_package(is_public=True) + self.assertRaises(exc.HTTPConflict, api.package_upload, + values, self.tenant_id_2) + + def test_package_upload_public_private_no_fqn_violation(self): + values = self._stub_package(is_public=True) + api.package_upload(values, self.tenant_id) + values = self._stub_package(is_public=False) + api.package_upload(values, self.tenant_id_2) + + def test_package_upload_private_public_no_fqn_violation(self): + values = self._stub_package() + api.package_upload(values, self.tenant_id) + values = self._stub_package(is_public=True) + api.package_upload(values, self.tenant_id_2) + + def test_class_name_is_unique(self): + value = self._stub_package(class_definitions=('foo', 'bar')) + api.package_upload(value, self.tenant_id) + value = self._stub_package(class_definitions=('bar', 'baz'), + fully_qualified_name='com.example.package2') + self.assertRaises(exc.HTTPConflict, api.package_upload, value, + self.tenant_id) + + def test_class_name_uniqueness_not_enforced_in_different_tenants(self): + value = self._stub_package(class_definitions=('foo', 'bar')) + api.package_upload(value, self.tenant_id) + value = self._stub_package(class_definitions=('foo', 'bar'), + fully_qualified_name='com.example.package2') + api.package_upload(value, self.tenant_id_2) + + def test_class_name_public_public_violation(self): + value = self._stub_package(class_definitions=('foo', 'bar'), + is_public=True) + api.package_upload(value, self.tenant_id) + value = self._stub_package(class_definitions=('foo', 'bar'), + is_public=True, + fully_qualified_name='com.example.package2') + self.assertRaises(exc.HTTPConflict, api.package_upload, + value, self.tenant_id_2) + + def test_class_name_public_private_no_violation(self): + value = self._stub_package(class_definitions=('foo', 'bar'), + is_public=True) + api.package_upload(value, self.tenant_id) + value = self._stub_package(class_definitions=('foo', 'bar'), + is_public=False, + fully_qualified_name='com.example.package2') + api.package_upload(value, self.tenant_id_2) + + def test_class_name_private_public_no_violation(self): + value = self._stub_package(class_definitions=('foo', 'bar'), + is_public=False) + api.package_upload(value, self.tenant_id) + value = self._stub_package(class_definitions=('foo', 'bar'), + is_public=True, + fully_qualified_name='com.example.package2') + api.package_upload(value, self.tenant_id_2) + + def test_package_make_public(self): + id = api.package_upload(self._stub_package(), self.tenant_id).id + patch = self.get_change('replace', ['is_public'], True) + api.package_update(id, [patch], self.context) + package = api.package_get(id, self.context) + self.assertEqual(True, package.is_public) + + def test_package_update_public_public_fqn_violation(self): + id1 = api.package_upload(self._stub_package(), self.tenant_id).id + id2 = api.package_upload(self._stub_package(), self.tenant_id_2).id + patch = self.get_change('replace', ['is_public'], True) + api.package_update(id1, [patch], self.context) + self.assertRaises(exc.HTTPConflict, api.package_update, + id2, [patch], self.context_2) + + def test_package_update_public_public_class_name_violation(self): + id1 = api.package_upload(self._stub_package( + class_definitions=('foo', 'bar')), self.tenant_id).id + id2 = api.package_upload(self._stub_package( + class_definitions=('foo', 'bar'), + fully_qualified_name='com.example.package2'), self.tenant_id_2).id + patch = self.get_change('replace', ['is_public'], True) + api.package_update(id1, [patch], self.context) + self.assertRaises(exc.HTTPConflict, api.package_update, + id2, [patch], self.context_2)