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

@@ -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)

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()

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'

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'])

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
 
 

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"}')

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'))

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:

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])