openstack/octavia
Code Issues Proposed changes

Merge "Stop allowing the deletion of an in-use flavor"

Browse Source
This commit is contained in:
Zuul 2019-11-19 18:56:06 +00:00 committed by Gerrit Code Review
commit 4a5c24ef6f
9 changed files with 253 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
octavia/api/v2/controllers/flavor_profiles.py
View File

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

23
octavia/api/v2/controllers/flavors.py
View File

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

3
octavia/common/constants.py
View File

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

78
octavia/db/migration/alembic_migrations/versions/e37941b010db_add_lb_flavor_constraint.py Normal file
View File

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

54
octavia/db/repositories.py
View File

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

26
octavia/tests/functional/api/v2/test_flavor_profiles.py
View File

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

46
octavia/tests/functional/api/v2/test_flavors.py
View File

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

12
octavia/tests/functional/db/base.py
View File

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

7
octavia/tests/functional/db/test_repositories.py
View File

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