Handle ports assigned to routers without routerports13.0.5

In the case of having a port attached to a router but the routerport
register is missing (as seen in the bug reported), this patch retrieves
the full list of ports attached to a router, filtering by router ID
and network ID or port ID. In case of having a port attached to this
router with missing routerport register, a warning message is logged.

Closes-Bug: #1842937

Change-Id: I93f35eade6aa081160902d9d47278123815c04d1
(cherry picked from commit c952b5960001faf98186b630fde75deafe5a7b8f)
(cherry picked from commit e5650d19bf6ccb87dd7a4c67153a23cca70b3ab3)
(cherry picked from commit 626eca984f08b0c073d7a1be29f5075e0324dabe)

3 files changed, 240 insertions, 54 deletions

diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py
index 0fc3798874..13fa5ee2a6 100644
--- a/neutron/db/l3_db.py
+++ b/neutron/db/l3_db.py
@@ -998,21 +998,13 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
 
     def _remove_interface_by_port(self, context, router_id,
                                   port_id, subnet_id, owner):
-        obj = l3_obj.RouterPort.get_object(
-            context,
-            port_id=port_id,
-            router_id=router_id,
-            port_type=owner
-        )
-        if obj:
-            try:
-                port = self._core_plugin.get_port(context, obj.port_id)
-            except n_exc.PortNotFound:
-                raise l3_exc.RouterInterfaceNotFound(
-                    router_id=router_id, port_id=port_id)
-        else:
+        ports = port_obj.Port.get_ports_by_router_and_port(
+            context, router_id, owner, port_id)
+        if len(ports) < 1:
             raise l3_exc.RouterInterfaceNotFound(
                 router_id=router_id, port_id=port_id)
+
+        port = ports[0]
         port_subnet_ids = [fixed_ip['subnet_id']
                            for fixed_ip in port['fixed_ips']]
         if subnet_id and subnet_id not in port_subnet_ids:
@@ -1025,47 +1017,41 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
                     context, router_id, port_subnet_id)
         self._core_plugin.delete_port(context, port['id'],
                                       l3_port_check=False)
-        return (port, subnets)
+        return port, subnets
 
     def _remove_interface_by_subnet(self, context,
                                     router_id, subnet_id, owner):
         self._confirm_router_interface_not_in_use(
             context, router_id, subnet_id)
         subnet = self._core_plugin.get_subnet(context, subnet_id)
+        ports = port_obj.Port.get_ports_by_router_and_network(
+            context, router_id, owner, subnet['network_id'])
 
-        try:
-            ports = port_obj.Port.get_ports_by_router(
-                context, router_id, owner, subnet)
-
-            for p in ports:
-                try:
-                    p = self._core_plugin.get_port(context, p.id)
-                except n_exc.PortNotFound:
-                    continue
-                port_subnets = [fip['subnet_id'] for fip in p['fixed_ips']]
-                if subnet_id in port_subnets and len(port_subnets) > 1:
-                    # multiple prefix port - delete prefix from port
-                    fixed_ips = [dict(fip) for fip in p['fixed_ips']
-                                 if fip['subnet_id'] != subnet_id]
-                    self._core_plugin.update_port(context, p['id'],
-                            {'port':
-                                {'fixed_ips': fixed_ips}})
-                    return (p, [subnet])
-                elif subnet_id in port_subnets:
-                    # only one subnet on port - delete the port
-                    self._core_plugin.delete_port(context, p['id'],
-                                                  l3_port_check=False)
-                    return (p, [subnet])
-        except exc.NoResultFound:
-            pass
+        for p in ports:
+            try:
+                p = self._core_plugin.get_port(context, p.id)
+            except n_exc.PortNotFound:
+                continue
+            port_subnets = [fip['subnet_id'] for fip in p['fixed_ips']]
+            if subnet_id in port_subnets and len(port_subnets) > 1:
+                # multiple prefix port - delete prefix from port
+                fixed_ips = [dict(fip) for fip in p['fixed_ips']
+                             if fip['subnet_id'] != subnet_id]
+                self._core_plugin.update_port(
+                    context, p['id'], {'port': {'fixed_ips': fixed_ips}})
+                return (p, [subnet])
+            elif subnet_id in port_subnets:
+                # only one subnet on port - delete the port
+                self._core_plugin.delete_port(context, p['id'],
+                                              l3_port_check=False)
+                return (p, [subnet])
         raise l3_exc.RouterInterfaceNotFoundForSubnet(
             router_id=router_id, subnet_id=subnet_id)
 
     @db_api.retry_if_session_inactive()
     def remove_router_interface(self, context, router_id, interface_info):
-        remove_by_port, remove_by_subnet = (
-            self._validate_interface_info(interface_info, for_removal=True)
-        )
+        remove_by_port, _ = self._validate_interface_info(interface_info,
+                                                          for_removal=True)
         port_id = interface_info.get('port_id')
         subnet_id = interface_info.get('subnet_id')
         device_owner = self._get_device_owner(context, router_id)
@@ -1073,9 +1059,6 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
             port, subnets = self._remove_interface_by_port(context, router_id,
                                                            port_id, subnet_id,
                                                            device_owner)
-        # remove_by_subnet is not used here, because the validation logic of
-        # _validate_interface_info ensures that at least one of remote_by_*
-        # is True.
         else:
             port, subnets = self._remove_interface_by_subnet(
                     context, router_id, subnet_id, device_owner)
diff --git a/neutron/objects/ports.py b/neutron/objects/ports.py
index f41e399964..5830ba8e09 100644
--- a/neutron/objects/ports.py
+++ b/neutron/objects/ports.py
@@ -14,6 +14,7 @@
 
 import netaddr
 from neutron_lib import constants
+from oslo_log import log as logging
 from oslo_utils import versionutils
 from oslo_versionedobjects import fields as obj_fields
 
@@ -28,6 +29,8 @@ from neutron.objects.db import api as obj_db_api
 from neutron.objects.qos import binding
 from neutron.plugins.ml2 import models as ml2_models
 
+LOG = logging.getLogger(__name__)
+
 
 class PortBindingBase(base.NeutronDbObject):
 
@@ -489,15 +492,65 @@ class Port(base.NeutronDbObject):
                         break
 
     @classmethod
-    def get_ports_by_router(cls, context, router_id, owner, subnet):
-        rport_qry = context.session.query(models_v2.Port).join(
-            l3.RouterPort)
-        ports = rport_qry.filter(
-            l3.RouterPort.router_id == router_id,
-            l3.RouterPort.port_type == owner,
-            models_v2.Port.network_id == subnet['network_id']
-        )
-        return [cls._load_object(context, db_obj) for db_obj in ports.all()]
+    def get_ports_by_router_and_network(cls, context, router_id, owner,
+                                        network_id):
+        """Returns port objects filtering by router ID, owner and network ID"""
+        rports_filter = (models_v2.Port.network_id == network_id, )
+        router_filter = (models_v2.Port.network_id == network_id, )
+        return cls._get_ports_by_router(context, router_id, owner,
+                                        rports_filter, router_filter)
+
+    @classmethod
+    def get_ports_by_router_and_port(cls, context, router_id, owner, port_id):
+        """Returns port objects filtering by router ID, owner and port ID"""
+        rports_filter = (l3.RouterPort.port_id == port_id, )
+        router_filter = (models_v2.Port.id == port_id, )
+        return cls._get_ports_by_router(context, router_id, owner,
+                                        rports_filter, router_filter)
+
+    @classmethod
+    def _get_ports_by_router(cls, context, router_id, owner, rports_filter,
+                             router_filter):
+        """Returns port objects filtering by router id and owner
+
+        The method will receive extra filters depending of the caller (filter
+        by network or filter by port).
+
+        The ports are retrieved using:
+        - The RouterPort registers. Each time a port is assigned to a router,
+          a new RouterPort register is added to the DB.
+        - The port owner and device_id information.
+
+        Both searches should return the same result. If not, a warning message
+        is logged and the port list to be returned is completed with the
+        missing ones.
+        """
+        rports_filter += (l3.RouterPort.router_id == router_id,
+                          l3.RouterPort.port_type == owner)
+        router_filter += (models_v2.Port.device_id == router_id,
+                          models_v2.Port.device_owner == owner)
+
+        ports = context.session.query(models_v2.Port).join(
+            l3.RouterPort).filter(*rports_filter)
+        ports_rports = [cls._load_object(context, db_obj)
+                        for db_obj in ports.all()]
+
+        ports = context.session.query(models_v2.Port).filter(*router_filter)
+        ports_router = [cls._load_object(context, db_obj)
+                        for db_obj in ports.all()]
+
+        ports_rports_ids = {p.id for p in ports_rports}
+        ports_router_ids = {p.id for p in ports_router}
+        missing_port_ids = ports_router_ids - ports_rports_ids
+        if missing_port_ids:
+            LOG.warning('The following ports, assigned to router '
+                        '%(router_id)s, do not have a "routerport" register: '
+                        '%(port_ids)s', {'router_id': router_id,
+                                         'port_ids': missing_port_ids})
+            port_objs = [p for p in ports_router if p.id in missing_port_ids]
+            ports_rports += port_objs
+
+        return ports_rports
 
     @classmethod
     def get_ports_ids_by_security_groups(cls, context, security_group_ids):
diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py
index 8e0969f809..9abcc0c6b6 100644
--- a/neutron/tests/unit/db/test_l3_db.py
+++ b/neutron/tests/unit/db/test_l3_db.py
@@ -22,14 +22,19 @@ from neutron_lib import constants as n_const
 from neutron_lib import context
 from neutron_lib import exceptions as n_exc
 from neutron_lib.exceptions import l3 as l3_exc
+from neutron_lib.plugins import constants as plugin_constants
 from neutron_lib.plugins import directory
 from oslo_utils import uuidutils
 import testtools
 
 from neutron.db import l3_db
 from neutron.db.models import l3 as l3_models
+from neutron.objects import network as network_obj
+from neutron.objects import ports as port_obj
 from neutron.objects import router as l3_obj
+from neutron.objects import subnet as subnet_obj
 from neutron.tests import base
+from neutron.tests.unit.db import test_db_base_plugin_v2
 
 
 class TestL3_NAT_dbonly_mixin(base.BaseTestCase):
@@ -332,3 +337,148 @@ class L3_NAT_db_mixin(base.BaseTestCase):
             self.assertRaises(
                 n_exc.BadRequest,
                 self.db.add_router_interface, mock.Mock(), router_db.id)
+
+
+class FakeL3Plugin(l3_db.L3_NAT_dbonly_mixin):
+    pass
+
+
+class L3TestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
+
+    GET_PORTS_BY_ROUTER_MSG = (
+        'The following ports, assigned to router %(router_id)s, do not have a '
+        '"routerport" register: %(port_ids)s')
+
+    def setUp(self, *args, **kwargs):
+        super(L3TestCase, self).setUp(plugin='ml2')
+        self.core_plugin = directory.get_plugin()
+        self.ctx = context.get_admin_context()
+        self.mixin = FakeL3Plugin()
+        directory.add_plugin(plugin_constants.L3, self.mixin)
+        self.network = self.create_network()
+        self.subnets = []
+        self.subnets.append(self.create_subnet(self.network, '1.1.1.1',
+                                               '1.1.1.0/24'))
+        self.subnets.append(self.create_subnet(self.network, '1.1.2.1',
+                                               '1.1.2.0/24'))
+        router = {'router': {'name': 'foo_router', 'admin_state_up': True,
+                             'tenant_id': 'foo_tenant'}}
+        self.router = self.create_router(router)
+        self.ports = []
+        for subnet in self.subnets:
+            ipa = str(netaddr.IPNetwork(subnet['subnet']['cidr']).ip + 10)
+            fixed_ips = [{'subnet_id': subnet['subnet']['id'],
+                          'ip_address': ipa}]
+            self.ports.append(self.create_port(
+                self.network['network']['id'], {'fixed_ips': fixed_ips}))
+        self.addCleanup(self._clean_objs)
+
+    def _clean_objs(self):
+        port_obj.Port.delete_objects(
+            self.ctx, network_id=self.network['network']['id'])
+        subnet_obj.Subnet.delete_objects(
+            self.ctx, network_id=self.network['network']['id'])
+        network_obj.Network.get_object(
+            self.ctx, id=self.network['network']['id']).delete()
+        l3_obj.Router.get_object(self.ctx, id=self.router['id']).delete()
+
+    def create_router(self, router):
+        with self.ctx.session.begin(subtransactions=True):
+            return self.mixin.create_router(self.ctx, router)
+
+    def create_port(self, net_id, port_info):
+        with self.ctx.session.begin(subtransactions=True):
+            return self._make_port(self.fmt, net_id, **port_info)
+
+    def create_network(self, name=None, **kwargs):
+        name = name or 'network1'
+        with self.ctx.session.begin(subtransactions=True):
+            return self._make_network(self.fmt, name, True, **kwargs)
+
+    def create_subnet(self, network, gateway, cidr, **kwargs):
+        with self.ctx.session.begin(subtransactions=True):
+            return self._make_subnet(self.fmt, network, gateway, cidr,
+                                     **kwargs)
+
+    def _add_router_interfaces(self):
+        return [self.mixin.add_router_interface(
+            self.ctx, self.router['id'],
+            interface_info={'port_id': port['port']['id']})
+            for port in self.ports]
+
+    def _check_routerports(self, ri_statuses):
+        port_ids = []
+        for idx, ri_status in enumerate(ri_statuses):
+            rp_obj = l3_obj.RouterPort.get_object(
+                self.ctx, port_id=self.ports[idx]['port']['id'],
+                router_id=self.router['id'])
+            if ri_status:
+                self.assertEqual(self.ports[idx]['port']['id'], rp_obj.port_id)
+                port_ids.append(rp_obj.port_id)
+            else:
+                self.assertIsNone(rp_obj)
+
+        _router_obj = l3_obj.Router.get_object(self.ctx, id=self.router['id'])
+        router_port_ids = [rp.port_id for rp in
+                           _router_obj.db_obj.attached_ports]
+        self.assertEqual(sorted(port_ids), sorted(router_port_ids))
+
+    @mock.patch.object(port_obj, 'LOG')
+    def test_remove_router_interface_by_port(self, mock_log):
+        self._add_router_interfaces()
+        self._check_routerports((True, True))
+
+        interface_info = {'port_id': self.ports[0]['port']['id']}
+        self.mixin.remove_router_interface(self.ctx, self.router['id'],
+                                           interface_info)
+        mock_log.warning.assert_not_called()
+        self._check_routerports((False, True))
+
+    @mock.patch.object(port_obj, 'LOG')
+    def test_remove_router_interface_by_port_removed_rport(self, mock_log):
+        self._add_router_interfaces()
+        self._check_routerports((True, True))
+
+        rp_obj = l3_obj.RouterPort.get_object(
+            self.ctx, router_id=self.router['id'],
+            port_id=self.ports[0]['port']['id'])
+        rp_obj.delete()
+
+        interface_info = {'port_id': self.ports[0]['port']['id']}
+        self.mixin.remove_router_interface(self.ctx, self.router['id'],
+                                           interface_info)
+        msg_vars = {'router_id': self.router['id'],
+                    'port_ids': {self.ports[0]['port']['id']}}
+        mock_log.warning.assert_called_once_with(self.GET_PORTS_BY_ROUTER_MSG,
+                                                 msg_vars)
+        self._check_routerports((False, True))
+
+    @mock.patch.object(port_obj, 'LOG')
+    def test_remove_router_interface_by_subnet(self, mock_log):
+        self._add_router_interfaces()
+        self._check_routerports((True, True))
+
+        interface_info = {'subnet_id': self.subnets[1]['subnet']['id']}
+        self.mixin.remove_router_interface(self.ctx, self.router['id'],
+                                           interface_info)
+        mock_log.warning.not_called_once()
+        self._check_routerports((True, False))
+
+    @mock.patch.object(port_obj, 'LOG')
+    def test_remove_router_interface_by_subnet_removed_rport(self, mock_log):
+        self._add_router_interfaces()
+        self._check_routerports((True, True))
+
+        rp_obj = l3_obj.RouterPort.get_object(
+            self.ctx, router_id=self.router['id'],
+            port_id=self.ports[0]['port']['id'])
+        rp_obj.delete()
+
+        interface_info = {'subnet_id': self.subnets[0]['subnet']['id']}
+        self.mixin.remove_router_interface(self.ctx, self.router['id'],
+                                           interface_info)
+        msg_vars = {'router_id': self.router['id'],
+                    'port_ids': {self.ports[0]['port']['id']}}
+        mock_log.warning.assert_called_once_with(self.GET_PORTS_BY_ROUTER_MSG,
+                                                 msg_vars)
+        self._check_routerports((False, True))