From 00b6460df2748cbf0bfa8fccdaad6a7356c15388 Mon Sep 17 00:00:00 2001 From: Matt Welch Date: Thu, 13 Dec 2018 15:52:34 -0800 Subject: [PATCH] Enforce router admin state before distributed 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 Co-Authored-By: Enyinna Ochulor Change-Id: Ie624aeb3f3aeb4db176d2ca0b22020208d4b408a Signed-off-by: Matt Welch --- neutron/db/l3_dvr_db.py | 40 +++++- .../_admin_state_down_before_update_lib.py | 32 +++++ .../admin_state_down_before_update.py | 18 +++ .../services/l3_router/l3_router_plugin.py | 4 +- .../tests/contrib/hooks/api_all_extensions | 1 + neutron/tests/unit/db/test_l3_dvr_db.py | 127 +++++++++++++++++- .../notes/bug-1811166-314d4b89de1cc0f1.yaml | 10 ++ 7 files changed, 221 insertions(+), 11 deletions(-) create mode 100644 neutron/extensions/_admin_state_down_before_update_lib.py create mode 100644 neutron/extensions/admin_state_down_before_update.py create mode 100644 releasenotes/notes/bug-1811166-314d4b89de1cc0f1.yaml diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index 52eb8dfc7e8..21ebff9cb24 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -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 = ( diff --git a/neutron/extensions/_admin_state_down_before_update_lib.py b/neutron/extensions/_admin_state_down_before_update_lib.py new file mode 100644 index 00000000000..65e22c091f5 --- /dev/null +++ b/neutron/extensions/_admin_state_down_before_update_lib.py @@ -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 = {} diff --git a/neutron/extensions/admin_state_down_before_update.py b/neutron/extensions/admin_state_down_before_update.py new file mode 100644 index 00000000000..900dbdc2de1 --- /dev/null +++ b/neutron/extensions/admin_state_down_before_update.py @@ -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 diff --git a/neutron/services/l3_router/l3_router_plugin.py b/neutron/services/l3_router/l3_router_plugin.py index 051277ffa55..de77df4e173 100644 --- a/neutron/services/l3_router/l3_router_plugin.py +++ b/neutron/services/l3_router/l3_router_plugin.py @@ -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 diff --git a/neutron/tests/contrib/hooks/api_all_extensions b/neutron/tests/contrib/hooks/api_all_extensions index 18085ddf57a..af81db49cbb 100644 --- a/neutron/tests/contrib/hooks/api_all_extensions +++ b/neutron/tests/contrib/hooks/api_all_extensions @@ -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" diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index 32c5f582df7..77c41de2775 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -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} diff --git a/releasenotes/notes/bug-1811166-314d4b89de1cc0f1.yaml b/releasenotes/notes/bug-1811166-314d4b89de1cc0f1.yaml new file mode 100644 index 00000000000..476da6ac54a --- /dev/null +++ b/releasenotes/notes/bug-1811166-314d4b89de1cc0f1.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + [`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. +