From 033c1e02ffda7d7cddc03e70fa4bf1b8e2843a11 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Thu, 31 Oct 2019 11:47:42 -0700 Subject: [PATCH] Stop allowing the deletion of an in-use flavor Currently the API allows an operator to delete a flavor even when it is in use by a load balancer. This patch corrects this by blocking the deletion of a flavor that is currently in use by a load balancer. It also correctly handles load balancers in the "DELETED" provisioning_status. Change-Id: Ie6d4f74e36c2fb7cee4e0ff1e198602c5d8394cc Story: 2006782 Task: 37307 --- octavia/api/v2/controllers/flavor_profiles.py | 11 ++- octavia/api/v2/controllers/flavors.py | 23 +++++- octavia/common/constants.py | 3 + .../e37941b010db_add_lb_flavor_constraint.py | 78 +++++++++++++++++++ octavia/db/repositories.py | 54 ++++++++++++- .../functional/api/v2/test_flavor_profiles.py | 26 +++++++ .../tests/functional/api/v2/test_flavors.py | 46 +++++++++++ octavia/tests/functional/db/base.py | 12 +++ .../tests/functional/db/test_repositories.py | 7 +- 9 files changed, 253 insertions(+), 7 deletions(-) create mode 100644 octavia/db/migration/alembic_migrations/versions/e37941b010db_add_lb_flavor_constraint.py diff --git a/octavia/api/v2/controllers/flavor_profiles.py b/octavia/api/v2/controllers/flavor_profiles.py index 0de743283d..fb37959a5f 100644 --- a/octavia/api/v2/controllers/flavor_profiles.py +++ b/octavia/api/v2/controllers/flavor_profiles.py @@ -47,6 +47,9 @@ class FlavorProfileController(base.BaseController): context = pecan.request.context.get('octavia_context') self._auth_validate_action(context, context.project_id, constants.RBAC_GET_ONE) + if id == constants.NIL_UUID: + raise exceptions.NotFound(resource='Flavor profile', + id=constants.NIL_UUID) db_flavor_profile = self._get_db_flavor_profile(context.session, id) result = self._convert_db_to_type(db_flavor_profile, profile_types.FlavorProfileResponse) @@ -140,6 +143,9 @@ class FlavorProfileController(base.BaseController): constants.RBAC_PUT) self._validate_update_fp(context, id, flavorprofile) + if id == constants.NIL_UUID: + raise exceptions.NotFound(resource='Flavor profile', + id=constants.NIL_UUID) if not isinstance(flavorprofile.flavor_data, wtypes.UnsetType): # Do a basic JSON validation on the metadata @@ -189,12 +195,15 @@ class FlavorProfileController(base.BaseController): self._auth_validate_action(context, context.project_id, constants.RBAC_DELETE) + if flavor_profile_id == constants.NIL_UUID: + raise exceptions.NotFound(resource='Flavor profile', + id=constants.NIL_UUID) + # Don't allow it to be deleted if it is in use by a flavor if self.repositories.flavor.count( context.session, flavor_profile_id=flavor_profile_id) > 0: raise exceptions.ObjectInUse(object='Flavor profile', id=flavor_profile_id) - try: self.repositories.flavor_profile.delete(context.session, id=flavor_profile_id) diff --git a/octavia/api/v2/controllers/flavors.py b/octavia/api/v2/controllers/flavors.py index 5aab894590..367657a0df 100644 --- a/octavia/api/v2/controllers/flavors.py +++ b/octavia/api/v2/controllers/flavors.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_db import api as oslo_db_api from oslo_db import exception as odb_exceptions from oslo_log import log as logging from oslo_utils import excutils @@ -44,7 +45,8 @@ class FlavorsController(base.BaseController): context = pecan.request.context.get('octavia_context') self._auth_validate_action(context, context.project_id, constants.RBAC_GET_ONE) - + if id == constants.NIL_UUID: + raise exceptions.NotFound(resource='Flavor', id=constants.NIL_UUID) db_flavor = self._get_db_flavor(context.session, id) result = self._convert_db_to_type(db_flavor, flavor_types.FlavorResponse) @@ -107,6 +109,8 @@ class FlavorsController(base.BaseController): context = pecan.request.context.get('octavia_context') self._auth_validate_action(context, context.project_id, constants.RBAC_PUT) + if id == constants.NIL_UUID: + raise exceptions.NotFound(resource='Flavor', id=constants.NIL_UUID) lock_session = db_api.get_session(autocommit=False) try: flavor_dict = flavor.to_dict(render_unsets=False) @@ -126,6 +130,7 @@ class FlavorsController(base.BaseController): flavor_types.FlavorResponse) return flavor_types.FlavorRootResponse(flavor=result) + @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True) @wsme_pecan.wsexpose(None, wtypes.text, status_code=204) def delete(self, flavor_id): """Deletes a Flavor""" @@ -133,10 +138,24 @@ class FlavorsController(base.BaseController): self._auth_validate_action(context, context.project_id, constants.RBAC_DELETE) + if flavor_id == constants.NIL_UUID: + raise exceptions.NotFound(resource='Flavor', id=constants.NIL_UUID) + serial_session = db_api.get_session(autocommit=False) + serial_session.connection( + execution_options={'isolation_level': 'SERIALIZABLE'}) try: - self.repositories.flavor.delete(context.session, id=flavor_id) + self.repositories.flavor.delete(serial_session, id=flavor_id) + serial_session.commit() # Handle when load balancers still reference this flavor except odb_exceptions.DBReferenceError: + serial_session.rollback() raise exceptions.ObjectInUse(object='Flavor', id=flavor_id) except sa_exception.NoResultFound: + serial_session.rollback() raise exceptions.NotFound(resource='Flavor', id=flavor_id) + except Exception as e: + with excutils.save_and_reraise_exception(): + LOG.error('Unknown flavor delete exception: %s', str(e)) + serial_session.rollback() + finally: + serial_session.close() diff --git a/octavia/common/constants.py b/octavia/common/constants.py index ee02d57af1..2230c78e8a 100644 --- a/octavia/common/constants.py +++ b/octavia/common/constants.py @@ -716,3 +716,6 @@ SUPPORTED_VOLUME_DRIVERS = [VOLUME_NOOP_DRIVER, CINDER_STATUS_AVAILABLE = 'available' CINDER_STATUS_ERROR = 'error' CINDER_ACTION_CREATE_VOLUME = 'create volume' + +# The nil UUID (used in octavia for deleted references) - RFC 4122 +NIL_UUID = '00000000-0000-0000-0000-000000000000' diff --git a/octavia/db/migration/alembic_migrations/versions/e37941b010db_add_lb_flavor_constraint.py b/octavia/db/migration/alembic_migrations/versions/e37941b010db_add_lb_flavor_constraint.py new file mode 100644 index 0000000000..436254e166 --- /dev/null +++ b/octavia/db/migration/alembic_migrations/versions/e37941b010db_add_lb_flavor_constraint.py @@ -0,0 +1,78 @@ +# +# 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. + +"""Add lb flavor ID constraint + +Revision ID: e37941b010db +Revises: dcf88e59aae4 +Create Date: 2019-10-31 10:09:37.869653 + +""" + +from alembic import op +import sqlalchemy as sa +from sqlalchemy import sql + +from octavia.common import constants + +# revision identifiers, used by Alembic. +revision = 'e37941b010db' +down_revision = 'dcf88e59aae4' + + +def upgrade(): + insert_table = sql.table( + u'flavor_profile', + sa.Column(u'id', sa.String(36), nullable=False), + sa.Column(u'name', sa.String(255), nullable=False), + sa.Column(u'provider_name', sa.String(255), nullable=False), + sa.Column(u'flavor_data', sa.String(4096), nullable=False), + ) + + op.bulk_insert( + insert_table, + [ + {'id': constants.NIL_UUID, 'name': 'DELETED-PLACEHOLDER', + 'provider_name': 'DELETED', 'flavor_data': '{}'}, + ] + ) + + insert_table = sql.table( + u'flavor', + sa.Column(u'id', sa.String(36), nullable=False), + sa.Column(u'name', sa.String(255), nullable=False), + sa.Column(u'description', sa.String(255), nullable=True), + sa.Column(u'enabled', sa.Boolean(), nullable=False), + sa.Column(u'flavor_profile_id', sa.String(36), nullable=False), + ) + + op.bulk_insert( + insert_table, + [ + {'id': constants.NIL_UUID, 'name': 'DELETED-PLACEHOLDER', + 'description': 'Placeholder for DELETED LBs with DELETED flavors', + 'enabled': False, 'flavor_profile_id': constants.NIL_UUID} + ] + ) + + # Make sure any existing load balancers with invalid flavor_id + # map to a valid flavor. + # Note: constant is not used here to not trigger security tool errors. + op.execute("UPDATE load_balancer LEFT JOIN flavor ON " + "load_balancer.flavor_id = flavor.id SET " + "load_balancer.flavor_id = " + "'00000000-0000-0000-0000-000000000000' WHERE " + "flavor.id IS NULL and load_balancer.flavor_id IS NOT NULL") + + op.create_foreign_key('fk_loadbalancer_flavor_id', 'load_balancer', + 'flavor', ['flavor_id'], ['id']) diff --git a/octavia/db/repositories.py b/octavia/db/repositories.py index 9e94de88f0..a03d910c89 100644 --- a/octavia/db/repositories.py +++ b/octavia/db/repositories.py @@ -1809,7 +1809,39 @@ class QuotasRepository(BaseRepository): session.flush() -class FlavorRepository(BaseRepository): +class _GetALLExceptDELETEDIdMixin(object): + + def get_all(self, session, pagination_helper=None, + query_options=None, **filters): + + """Retrieves a list of entities from the database. + + This filters the "DELETED" placeholder from the list. + + :param session: A Sql Alchemy database session. + :param pagination_helper: Helper to apply pagination and sorting. + :param query_options: Optional query options to apply. + :param filters: Filters to decide which entities should be retrieved. + :returns: [octavia.common.data_model] + """ + query = session.query(self.model_class).filter_by(**filters) + if query_options: + query = query.options(query_options) + + query = query.filter(self.model_class.id != consts.NIL_UUID) + + if pagination_helper: + model_list, links = pagination_helper.apply( + query, self.model_class) + else: + links = None + model_list = query.all() + + data_model_list = [model.to_data_model() for model in model_list] + return data_model_list, links + + +class FlavorRepository(_GetALLExceptDELETEDIdMixin, BaseRepository): model_class = models.Flavor def get_flavor_metadata_dict(self, session, flavor_id): @@ -1831,8 +1863,26 @@ class FlavorRepository(BaseRepository): .filter(models.Flavor.flavor_profile_id == models.FlavorProfile.id).one()[0]) + def delete(self, serial_session, **filters): + """Sets DELETED LBs flavor_id to NIL_UUID, then removes the flavor -class FlavorProfileRepository(BaseRepository): + :param serial_session: A Sql Alchemy database transaction session. + :param filters: Filters to decide which entity should be deleted. + :returns: None + :raises: odb_exceptions.DBReferenceError + :raises: sqlalchemy.orm.exc.NoResultFound + """ + (serial_session.query(models.LoadBalancer). + filter(models.LoadBalancer.flavor_id == filters['id']). + filter(models.LoadBalancer.provisioning_status == consts.DELETED). + update({models.LoadBalancer.flavor_id: consts.NIL_UUID}, + synchronize_session=False)) + flavor = (serial_session.query(self.model_class). + filter_by(**filters).one()) + serial_session.delete(flavor) + + +class FlavorProfileRepository(_GetALLExceptDELETEDIdMixin, BaseRepository): model_class = models.FlavorProfile diff --git a/octavia/tests/functional/api/v2/test_flavor_profiles.py b/octavia/tests/functional/api/v2/test_flavor_profiles.py index 869d39ed9a..0c0f38ae02 100644 --- a/octavia/tests/functional/api/v2/test_flavor_profiles.py +++ b/octavia/tests/functional/api/v2/test_flavor_profiles.py @@ -171,6 +171,12 @@ class TestFlavorProfiles(base.BaseAPITest): self.assertEqual('name', response.get('name')) self.assertEqual(fp.get('id'), response.get('id')) + def test_get_one_deleted_id(self): + response = self.get(self.FP_PATH.format(fp_id=constants.NIL_UUID), + status=404) + self.assertEqual('Flavor profile {} not found.'.format( + constants.NIL_UUID), response.json.get('faultstring')) + def test_get_one_fields_filter(self): fp = self.create_flavor_profile('name', 'noop_driver', '{"x": "y"}') @@ -328,6 +334,14 @@ class TestFlavorProfiles(base.BaseAPITest): self.assertEqual('{"hello": "world"}', response.get(constants.FLAVOR_DATA)) + def test_update_deleted_id(self): + update_data = {'name': 'fake_profile'} + body = self._build_body(update_data) + response = self.put(self.FP_PATH.format(fp_id=constants.NIL_UUID), + body, status=404) + self.assertEqual('Flavor profile {} not found.'.format( + constants.NIL_UUID), response.json.get('faultstring')) + def test_update_nothing(self): fp = self.create_flavor_profile('test_profile', 'noop_driver', '{"x": "y"}') @@ -485,6 +499,18 @@ class TestFlavorProfiles(base.BaseAPITest): err_msg = "Flavor Profile %s not found." % fp.get('id') self.assertEqual(err_msg, response.json.get('faultstring')) + def test_delete_deleted_id(self): + response = self.delete(self.FP_PATH.format(fp_id=constants.NIL_UUID), + status=404) + self.assertEqual('Flavor profile {} not found.'.format( + constants.NIL_UUID), response.json.get('faultstring')) + + def test_delete_nonexistent_id(self): + response = self.delete(self.FP_PATH.format(fp_id='bogus_id'), + status=404) + self.assertEqual('Flavor profile bogus_id not found.', + response.json.get('faultstring')) + def test_delete_authorized(self): fp = self.create_flavor_profile('test1', 'noop_driver', '{"image": "ubuntu"}') diff --git a/octavia/tests/functional/api/v2/test_flavors.py b/octavia/tests/functional/api/v2/test_flavors.py index ed4e3457e2..00ad005e6b 100644 --- a/octavia/tests/functional/api/v2/test_flavors.py +++ b/octavia/tests/functional/api/v2/test_flavors.py @@ -21,6 +21,7 @@ from oslo_config import fixture as oslo_fixture from octavia.common import constants import octavia.common.context +from octavia.common import exceptions from octavia.tests.functional.api.v2 import base @@ -188,6 +189,12 @@ class TestFlavors(base.BaseAPITest): self.assertNotIn(u'description', response) self.assertNotIn(u'enabled', response) + def test_get_one_deleted_id(self): + response = self.get( + self.FLAVOR_PATH.format(flavor_id=constants.NIL_UUID), status=404) + self.assertEqual('Flavor {} not found.'.format(constants.NIL_UUID), + response.json.get('faultstring')) + def test_get_authorized(self): flavor = self.create_flavor('name', 'description', self.fp.get('id'), True) @@ -357,6 +364,15 @@ class TestFlavors(base.BaseAPITest): updated_flavor.get('flavor_profile_id')) self.assertFalse(updated_flavor.get('enabled')) + def test_update_deleted_id(self): + update_json = {'name': 'fake_name'} + body = self._build_body(update_json) + response = self.put( + self.FLAVOR_PATH.format(flavor_id=constants.NIL_UUID), body, + status=404) + self.assertEqual('Flavor {} not found.'.format(constants.NIL_UUID), + response.json.get('faultstring')) + def test_update_none(self): flavor_json = {'name': 'Fancy_Flavor', 'description': 'A great flavor. Pick me!', @@ -478,6 +494,16 @@ class TestFlavors(base.BaseAPITest): updated_flavor.get('flavor_profile_id')) self.assertTrue(updated_flavor.get('enabled')) + @mock.patch('octavia.db.repositories.FlavorRepository.update') + def test_update_exception(self, mock_update): + mock_update.side_effect = [exceptions.OctaviaException()] + update_json = {'name': 'A_Flavor'} + body = self._build_body(update_json) + response = self.put(self.FLAVOR_PATH.format(flavor_id='bogus'), body, + status=500) + self.assertEqual('An unknown exception occurred.', + response.json.get('faultstring')) + def test_delete(self): flavor = self.create_flavor('name1', 'description', self.fp.get('id'), True) @@ -488,6 +514,18 @@ class TestFlavors(base.BaseAPITest): err_msg = "Flavor %s not found." % flavor.get('id') self.assertEqual(err_msg, response.json.get('faultstring')) + def test_delete_nonexistent_id(self): + response = self.delete( + self.FLAVOR_PATH.format(flavor_id='bogus_id'), status=404) + self.assertEqual('Flavor bogus_id not found.', + response.json.get('faultstring')) + + def test_delete_deleted_id(self): + response = self.delete( + self.FLAVOR_PATH.format(flavor_id=constants.NIL_UUID), status=404) + self.assertEqual('Flavor {} not found.'.format(constants.NIL_UUID), + response.json.get('faultstring')) + def test_delete_authorized(self): flavor = self.create_flavor('name1', 'description', self.fp.get('id'), True) @@ -556,3 +594,11 @@ class TestFlavors(base.BaseAPITest): response = self.get(self.FLAVOR_PATH.format( flavor_id=flavor.get('id'))).json.get(self.root_tag) self.assertEqual('name1', response.get('name')) + + @mock.patch('octavia.db.repositories.FlavorRepository.delete') + def test_delete_exception(self, mock_delete): + mock_delete.side_effect = [exceptions.OctaviaException()] + response = self.delete(self.FLAVOR_PATH.format(flavor_id='bogus'), + status=500) + self.assertEqual('An unknown exception occurred.', + response.json.get('faultstring')) diff --git a/octavia/tests/functional/db/base.py b/octavia/tests/functional/db/base.py index 13c16129c6..d687bcc14a 100644 --- a/octavia/tests/functional/db/base.py +++ b/octavia/tests/functional/db/base.py @@ -96,6 +96,18 @@ class OctaviaDBTestBase(test_base.DbTestCase): models.L7PolicyAction) self._seed_lookup_table(session, constants.SUPPORTED_CLIENT_AUTH_MODES, models.ClientAuthenticationMode) + # Add in the id='DELETED' placeholders + deleted_flavor_profile = models.FlavorProfile( + id=constants.NIL_UUID, name='DELETED-PLACEHOLDER', + provider_name=constants.DELETED, flavor_data='{}') + session.add(deleted_flavor_profile) + session.flush() + deleted_flavor = models.Flavor( + id=constants.NIL_UUID, flavor_profile_id=constants.NIL_UUID, + name='DELETED-PLACEHOLDER', enabled=False, + description='Placeholder for DELETED LBs with DELETED flavors') + session.add(deleted_flavor) + session.flush() def _seed_lookup_table(self, session, name_list, model_cls): for name in name_list: diff --git a/octavia/tests/functional/db/test_repositories.py b/octavia/tests/functional/db/test_repositories.py index 5d6d3d5749..5ea1a0fbd4 100644 --- a/octavia/tests/functional/db/test_repositories.py +++ b/octavia/tests/functional/db/test_repositories.py @@ -21,6 +21,7 @@ from oslo_config import cfg from oslo_config import fixture as oslo_fixture from oslo_db import exception as db_exception from oslo_utils import uuidutils +from sqlalchemy.orm import defer from sqlalchemy.orm import exc as sa_exception from octavia.common import constants @@ -4348,7 +4349,8 @@ class FlavorProfileRepositoryTest(BaseRepositoryTest): def test_get_all(self): fp1 = self.create_flavor_profile(fp_id=self.FAKE_UUID_1) fp2 = self.create_flavor_profile(fp_id=self.FAKE_UUID_2) - fp_list, _ = self.flavor_profile_repo.get_all(self.session) + fp_list, _ = self.flavor_profile_repo.get_all( + self.session, query_options=defer('name')) self.assertIsInstance(fp_list, list) self.assertEqual(2, len(fp_list)) self.assertEqual(fp1, fp_list[0]) @@ -4395,7 +4397,8 @@ class FlavorRepositoryTest(BaseRepositoryTest): def test_get_all(self): fl1 = self.create_flavor(flavor_id=self.FAKE_UUID_2, name='flavor1') fl2 = self.create_flavor(flavor_id=self.FAKE_UUID_3, name='flavor2') - fl_list, _ = self.flavor_repo.get_all(self.session) + fl_list, _ = self.flavor_repo.get_all(self.session, + query_options=defer('enabled')) self.assertIsInstance(fl_list, list) self.assertEqual(2, len(fl_list)) self.assertEqual(fl1, fl_list[0])