Merge "Enforce router admin state before distributed"

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

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 = {}

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

neutron/services/l3_router/l3_router_plugin.py

@@ -48,6 +48,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
@@ -100,7 +101,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

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"

neutron/tests/unit/db/test_l3_dvr_db.py

@@ -81,7 +81,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,
@@ -91,7 +91,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,
@@ -102,10 +119,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)
@@ -557,7 +651,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)
@@ -589,6 +683,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}
@@ -851,7 +958,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:
@@ -861,6 +968,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}

releasenotes/notes/bug-1811166-314d4b89de1cc0f1.yaml

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