openstack/neutron
Code Issues Proposed changes

Enforce router admin state before distributed

Browse Source 
Enforce that a user updates the admin state of a router before modifying
the distributed state. The API currently allows setting admin state to
false concurrently with changing the distributed state.
This is fine for a transition of centralized->distributed, but the
distributed->centralized transition could leave other nodes configured
as distributed until an audit is performed.

Commit adds shim api extension which should be replaced by neutron-lib
shim extension once https://review.openstack.org/#/c/634509/ is merged.
New method 'is_admin_state_down_necessary' checks that shim extension
is loaded.

Set extension as standard by adding to _supported_extension_aliases in
neutron/services/l3_router/l3_router_plugin.py

Closes-Bug: #1811166
Co-Authored-By: Allain Legacy <allain.legacy@windriver.com>
Co-Authored-By: Enyinna Ochulor <enyinna.ochulor@intel.com>
Change-Id: Ie624aeb3f3aeb4db176d2ca0b22020208d4b408a
Signed-off-by: Matt Welch <matt.welch@intel.com>
This commit is contained in:
Matt Welch 2018-12-13 15:52:34 -08:00
parent 05d93684fb
commit 00b6460df2
7 changed files with 221 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
neutron/db/l3_dvr_db.py
View File

@ -37,12 +37,14 @@ from oslo_utils import excutils
import six
from neutron._i18n import _
from neutron.api import extensions
from neutron.common import utils as n_utils
from neutron.conf.db import l3_dvr_db
from neutron.db import l3_attrs_db
from neutron.db import l3_db
from neutron.db.models import allowed_address_pair as aap_models
from neutron.db import models_v2
from neutron.extensions import _admin_state_down_before_update_lib
from neutron.ipam import utils as ipam_utils
from neutron.objects import agent as ag_obj
from neutron.objects import base as base_obj
@ -52,6 +54,16 @@ from neutron.objects import router as l3_obj
LOG = logging.getLogger(__name__)
l3_dvr_db.register_db_l3_dvr_opts()
_IS_ADMIN_STATE_DOWN_NECESSARY = None
def is_admin_state_down_necessary():
global _IS_ADMIN_STATE_DOWN_NECESSARY
if _IS_ADMIN_STATE_DOWN_NECESSARY is None:
_IS_ADMIN_STATE_DOWN_NECESSARY = \
_admin_state_down_before_update_lib.ALIAS in (extensions.
PluginAwareExtensionManager.get_instance().extensions)
return _IS_ADMIN_STATE_DOWN_NECESSARY
@registry.has_registry_receivers
@ -81,9 +93,18 @@ class DVRResourceOperationHandler(object):
self.l3plugin.set_extra_attr_value(context, router_db, 'distributed',
dist)
def _validate_router_migration(self, context, router_db, router_res):
def _validate_router_migration(self, context, router_db, router_res,
old_router=None):
"""Allow transition only when admin_state_up=False"""
original_distributed_state = router_db.extra_attributes.distributed
admin_state_down_extension_loaded = is_admin_state_down_necessary()
# to preserve extant API behavior, only check the distributed attribute
# of old_router when the "router-admin-state-down-before-update" shim
# API extension is loaded. Don't bother checking if old_router is
# "None"
if old_router and admin_state_down_extension_loaded:
original_distributed_state = old_router.get('distributed')
else:
original_distributed_state = router_db.extra_attributes.distributed
requested_distributed_state = router_res.get('distributed', None)
distributed_changed = (
@ -91,7 +112,18 @@ class DVRResourceOperationHandler(object):
requested_distributed_state != original_distributed_state)
if not distributed_changed:
return False
if router_db.admin_state_up:
# to preserve old API behavior, only check old_router if shim API
# extension has been loaded
if admin_state_down_extension_loaded and old_router:
# if one OR both routers is still up, the *collective*
# "admin_state_up" should be True and we should throw the
# BadRequest exception below.
admin_state_up = (old_router.get('admin_state_up') or
router_db.get('admin_state_up'))
else:
admin_state_up = router_db.get('admin_state_up')
if admin_state_up:
msg = _("Cannot change the 'distributed' attribute of active "
"routers. Please set router admin_state_up to False "
"prior to upgrade")
@ -117,7 +149,7 @@ class DVRResourceOperationHandler(object):
"""Event handler for router update migration to distributed."""
if not self._validate_router_migration(
payload.context, payload.desired_state,
payload.request_body):
payload.request_body, payload.states[0]):
return
migrating_to_distributed = (

32
neutron/extensions/_admin_state_down_before_update_lib.py Normal file
View File

@ -0,0 +1,32 @@
# 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.
"""
TODO(mattw4): This module should be deleted once neutron-lib containing
https://review.openstack.org/#/c/634509/ change is released.
"""
from neutron_lib.api.definitions import l3 as l3_apidef
ALIAS = 'router-admin-state-down-before-update'
IS_SHIM_EXTENSION = True
IS_STANDARD_ATTR_EXTENSION = False
NAME = "Enforce Router's Admin State Down Before Update Extension"
DESCRIPTION = ('Ensure that the admin state of a router is down '
'(admin_state_up=False) before updating the distributed '
'attribute')
UPDATED_TIMESTAMP = '2019-04-08 13:30:00'
RESOURCE_ATTRIBUTE_MAP = {}
SUB_RESOURCE_ATTRIBUTE_MAP = {}
ACTION_MAP = {}
REQUIRED_EXTENSIONS = [l3_apidef.ALIAS]
OPTIONAL_EXTENSIONS = []
ACTION_STATUS = {}

18
neutron/extensions/admin_state_down_before_update.py Normal file
View File

@ -0,0 +1,18 @@
# 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 neutron.extensions import _admin_state_down_before_update_lib as apidef
from neutron_lib.api import extensions
class Admin_state_down_before_update(extensions.APIExtensionDescriptor):
api_definition = apidef

4
neutron/services/l3_router/l3_router_plugin.py
View File

@ -49,6 +49,7 @@ from neutron.db import l3_gateway_ip_qos
from neutron.db import l3_hamode_db
from neutron.db import l3_hascheduler_db
from neutron.db.models import l3 as l3_models
from neutron.extensions import _admin_state_down_before_update_lib
from neutron.quota import resource_registry
from neutron import service
from neutron.services.l3_router.service_providers import driver_controller
@ -102,7 +103,8 @@ class L3RouterPlugin(service_base.ServicePluginBase,
fip_port_details.ALIAS,
floatingip_pools.ALIAS,
qos_gateway_ip.ALIAS,
l3_port_ip_change_not_allowed.ALIAS]
l3_port_ip_change_not_allowed.ALIAS,
_admin_state_down_before_update_lib.ALIAS]
__native_pagination_support = True
__native_sorting_support = True

1
neutron/tests/contrib/hooks/api_all_extensions
View File

@ -46,6 +46,7 @@ NETWORK_API_EXTENSIONS+=",quota_details"
NETWORK_API_EXTENSIONS+=",rbac-policies"
NETWORK_API_EXTENSIONS+=",rbac-security-groups""
NETWORK_API_EXTENSIONS+=",router"
NETWORK_API_EXTENSIONS+=",router-admin-state-down-before-update"
NETWORK_API_EXTENSIONS+=",router_availability_zone"
NETWORK_API_EXTENSIONS+=",security-group"
NETWORK_API_EXTENSIONS+=",port-mac-address-regenerate"

127
neutron/tests/unit/db/test_l3_dvr_db.py
View File

@ -83,7 +83,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
def test_create_router_db_distributed(self):
self._test__create_router_db(expected=True, distributed=True)
def test__validate_router_migration_on_router_update(self):
def _test__validate_router_migration_on_router_update(self, mock_arg):
router = {
'name': 'foo_router',
'admin_state_up': True,
@ -93,7 +93,24 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
self.assertFalse(self.mixin._validate_router_migration(
self.ctx, router_db, {'name': 'foo_router_2'}))
def test__validate_router_migration_raise_error(self):
# mock the check function to indicate that the variable
# _admin_state_down_necessary set to True
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=True)
def test__validate_router_migration_on_router_update_mock(self,
mock_arg):
# call test with admin_state_down_before_update ENABLED
self._test__validate_router_migration_on_router_update(mock_arg)
# mock the check function to indicate that the variable
# _admin_state_down_necessary set to False
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=False)
def test__validate_router_migration_on_router_update(self, mock_arg):
# call test with admin_state_down_before_update DISABLED
self._test__validate_router_migration_on_router_update(mock_arg)
def _test__validate_router_migration_raise_error(self):
router = {
'name': 'foo_router',
'admin_state_up': True,
@ -104,10 +121,87 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
self.mixin._validate_router_migration,
self.ctx, router_db, {'distributed': False})
def test_upgrade_active_router_to_distributed_validation_failure(self):
router = {'name': 'foo_router', 'admin_state_up': True}
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=True)
def test__validate_router_migration_raise_error_mocked(self, mock_arg):
# call test with admin_state_down_before_update ENABLED
self._test__validate_router_migration_raise_error()
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=False)
def test__validate_router_migration_raise_error(self, mock_arg):
# call test with admin_state_down_before_update DISABLED
self._test__validate_router_migration_raise_error()
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=True)
def test__validate_router_migration_old_router_up_raise_error(self,
mock_arg):
# call test with admin_state_down_before_update ENABLED
old_router = {
'name': 'bar_router',
'admin_state_up': True,
'distributed': True
}
new_router = {
'name': 'foo_router',
'admin_state_up': False,
'distributed': False
}
update = {'distributed': False}
router_db = self._create_router(new_router)
self.assertRaises(exceptions.BadRequest,
self.mixin._validate_router_migration,
self.ctx, router_db, update,
old_router)
def _test_upgrade_inactive_router_to_distributed_validation_success(self):
router = {'name': 'foo_router', 'admin_state_up': False,
'distributed': False}
router_db = self._create_router(router)
update = {'distributed': True}
self.assertTrue(self.mixin._validate_router_migration(
self.ctx, router_db, update))
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=True)
def test_upgrade_inactive_router_to_distributed_validation_success_mocked(
self, mock_arg):
# call test with admin_state_down_before_update ENABLED
self._test_upgrade_inactive_router_to_distributed_validation_success()
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=False)
def test_upgrade_inactive_router_to_distributed_validation_success(self,
mock_arg):
# call test with admin_state_down_before_update DISABLED
self._test_upgrade_inactive_router_to_distributed_validation_success()
def _test_upgrade_active_router_to_distributed_validation_failure(self):
router = {'name': 'foo_router', 'admin_state_up': True,
'distributed': False}
router_db = self._create_router(router)
update = {'distributed': True}
self.assertRaises(exceptions.BadRequest,
self.mixin._validate_router_migration,
self.ctx, router_db, update)
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=True)
def test_upgrade_active_router_to_distributed_validation_failure(self,
mock_arg):
# call test with admin_state_down_before_update ENABLED
self._test_upgrade_active_router_to_distributed_validation_failure()
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=True)
def test_downgrade_active_router_to_centralized_validation_failure(self,
mock_arg):
# call test with admin_state_down_before_update ENABLED
router = {'name': 'foo_router', 'admin_state_up': True,
'distributed': True}
router_db = self._create_router(router)
update = {'distributed': False}
self.assertRaises(exceptions.BadRequest,
self.mixin._validate_router_migration,
self.ctx, router_db, update)
@ -528,7 +622,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
fip, floatingip, router))
self.assertFalse(create_fip.called)
def test_update_router_gw_info_external_network_change(self):
def _test_update_router_gw_info_external_network_change(self):
router_dict = {'name': 'test_router', 'admin_state_up': True,
'distributed': True}
router = self._create_router(router_dict)
@ -560,6 +654,19 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
self.ctx, filters=csnat_filters)
self.assertEqual(1, len(csnat_ports))
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=True)
def test_update_router_gw_info_external_network_change_mocked(self,
mock_arg):
# call test with admin_state_down_before_update ENABLED
self._test_update_router_gw_info_external_network_change()
@mock.patch('neutron.db.l3_dvr_db.is_admin_state_down_necessary',
return_value=False)
def test_update_router_gw_info_external_network_change(self, mock_arg):
# call test with admin_state_down_before_update DISABLED
self._test_update_router_gw_info_external_network_change()
def _test_csnat_ports_removal(self, ha=False):
router_dict = {'name': 'test_router', 'admin_state_up': True,
'distributed': True}
@ -822,7 +929,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
self.ctx, filters=dvr_filters)
self.assertEqual(1, len(dvr_ports))
def test__validate_router_migration_notify_advanced_services(self):
def _test__validate_router_migration_notify_advanced_services(self):
router = {'name': 'foo_router', 'admin_state_up': False}
router_db = self._create_router(router)
with mock.patch.object(l3_dvr_db.registry, 'notify') as mock_notify:
@ -832,6 +939,14 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
mock_notify.assert_called_once_with(
'router', 'before_update', self.mixin, **kwargs)
def test__validate_router_migration_notify_advanced_services_mocked(self):
# call test with admin_state_down_before_update ENABLED
self._test__validate_router_migration_notify_advanced_services()
def test__validate_router_migration_notify_advanced_services(self):
# call test with admin_state_down_before_update DISABLED
self._test__validate_router_migration_notify_advanced_services()
def test_validate_add_router_interface_by_subnet_notify_advanced_services(
self):
router = {'name': 'foo_router', 'admin_state_up': False}

10
releasenotes/notes/bug-1811166-314d4b89de1cc0f1.yaml Normal file
View File

@ -0,0 +1,10 @@
---
fixes:
- |
[`bug 1811166 <https://bugs.launchpad.net/neutron/+bug/1811166>`_]
Changes the API behavior to enforce that a router's administrative state
must be down (``router.admin_state_up==False`` ) before modifying its
distributed attribute. If the router ``admin_state_up==True`` when trying
to change the ``distributed`` attribute, a BadRequest exception will be
thrown.