Prevent bind fip to port has port forwarding

If one port has port forwarding and the port is under
a dvr router, then binding floating IP to this port
will not be allowed.

Change-Id: Ia014e18264b43cf751a5bc0e82bc55d106582620
Closes-Bug: #1799138

neutron/db/l3_db.py

@@ -1336,6 +1336,14 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
 
     def _create_floatingip(self, context, floatingip,
             initial_status=constants.FLOATINGIP_STATUS_ACTIVE):
+        try:
+            registry.publish(resources.FLOATING_IP, events.BEFORE_CREATE,
+                             self, payload=events.DBEventPayload(
+                                 context, request_body=floatingip))
+        except exceptions.CallbackFailure as e:
+            # raise the underlying exception
+            raise e.errors[0].error
+
         fip = floatingip['floatingip']
         fip_id = uuidutils.generate_uuid()
 
@@ -1441,6 +1449,15 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
         return self._create_floatingip(context, floatingip, initial_status)
 
     def _update_floatingip(self, context, id, floatingip):
+        try:
+            registry.publish(resources.FLOATING_IP, events.BEFORE_UPDATE,
+                             self, payload=events.DBEventPayload(
+                                 context, request_body=floatingip,
+                                 resource_id=id))
+        except exceptions.CallbackFailure as e:
+            # raise the underlying exception
+            raise e.errors[0].error
+
         fip = floatingip['floatingip']
         with context.session.begin(subtransactions=True):
             floatingip_obj = self._get_floatingip(context, id)

neutron/services/portforwarding/common/exceptions.py

@@ -25,6 +25,12 @@ class PortForwardingNotSupportFilterField(n_exc.BadRequest):
     message = _("Port Forwarding filter %(filter)s is not supported.")
 
 
+class PortHasPortForwarding(n_exc.BadRequest):
+    message = _("Cannot associate floating IP to port "
+                "%(port_id)s because it already has a "
+                "Port Forwarding binding.")
+
+
 class FipInUseByPortForwarding(n_exc.InUse):
     message = _("Floating IP %(id)s in use by Port Forwarding resources.")
 

neutron/services/portforwarding/pf_plugin.py

@@ -38,6 +38,7 @@ from neutron.api.rpc.handlers import resources_rpc
 from neutron.common import utils
 from neutron.db import _resource_extend as resource_extend
 from neutron.db import db_base_plugin_common
+from neutron.db import l3_dvr_db
 from neutron.extensions import floating_ip_port_forwarding as fip_pf
 from neutron.objects import base as base_obj
 from neutron.objects import port_forwarding as pf
@@ -103,6 +104,27 @@ class PortForwardingPlugin(fip_pf.PortForwardingPluginBase):
             result_dict[apidef.COLLECTION_NAME] = port_forwarding_result
         return result_dict
 
+    @registry.receives(resources.FLOATING_IP, [events.BEFORE_CREATE,
+                                               events.BEFORE_UPDATE])
+    def _check_port_has_port_forwarding(self, resource, event,
+                                        trigger, payload=None):
+        port_id = payload.request_body['floatingip'].get('port_id')
+        if not port_id:
+            return
+
+        pf_objs = pf.PortForwarding.get_objects(
+            payload.context, internal_port_id=port_id)
+        if not pf_objs:
+            return
+        # Port may not bind to host yet, or port may migrate from one
+        # dvr_no_external host to one dvr host. So we just do not allow
+        # all dvr router's floating IP to be binded to a port which
+        # already has port forwarding.
+        router = self.l3_plugin.get_router(payload.context.elevated(),
+                                           pf_objs[0].router_id)
+        if l3_dvr_db.is_distributed_router(router):
+            raise pf_exc.PortHasPortForwarding(port_id=port_id)
+
     @registry.receives(resources.FLOATING_IP, [events.PRECOMMIT_UPDATE,
                                                events.PRECOMMIT_DELETE])
     def _check_floatingip_request(self, resource, event, trigger, context,

neutron/tests/functional/services/portforwarding/test_port_forwarding.py

@@ -86,7 +86,7 @@ class PortForwardingTestCase(PortForwardingTestCaseBase):
         self._prepare_env()
 
     def _prepare_env(self):
-        self.router = self._create_router()
+        self.router = self._create_router(distributed=True)
         self.ext_net = self._create_network(
             self.fmt, 'ext-net', True, arg_list=("router:external",),
             **{"router:external": True}).json['network']
@@ -461,3 +461,24 @@ class PortForwardingTestCase(PortForwardingTestCaseBase):
         self._simulate_concurrent_requests_process_and_raise(funcs, args_list)
         self.assertEqual([], self.pf_plugin.get_floatingip_port_forwardings(
             self.context, floatingip_id=self.fip['id']))
+
+    def test_create_floatingip_port_forwarding_port_in_use(self):
+        res = self.pf_plugin.create_floatingip_port_forwarding(
+            self.context, self.fip['id'], self.port_forwarding)
+        expected = {
+            "external_port": 2225,
+            "internal_port": 25,
+            "internal_port_id": self.port['id'],
+            "protocol": "tcp",
+            "internal_ip_address": self.port['fixed_ips'][0]['ip_address'],
+            'id': mock.ANY,
+            'router_id': self.router['id'],
+            'floating_ip_address': self.fip['floating_ip_address'],
+            'floatingip_id': self.fip['id']}
+        self.assertEqual(expected, res)
+
+        fip_2 = self._create_floatingip(self.ext_net['id'])
+        self.assertRaises(
+            pf_exc.PortHasPortForwarding,
+            self._update_floatingip,
+            fip_2['id'], {'port_id': self.port['id']})

neutron/tests/unit/extensions/test_expose_port_forwarding_in_fip.py

@@ -16,18 +16,22 @@ from neutron_lib.api.definitions import external_net as extnet_apidef
 from neutron_lib.api.definitions import floating_ip_port_forwarding as apidef
 from neutron_lib import constants
 from neutron_lib import context
+from neutron_lib.plugins import constants as plugin_constants
+from neutron_lib.plugins import directory
 from oslo_utils import uuidutils
+from webob import exc
 
+from neutron.db import l3_dvr_db
 from neutron.db import l3_fip_qos
 from neutron.extensions import floating_ip_port_forwarding as pf_ext
 from neutron.extensions import l3
 from neutron.objects.qos import policy
-from neutron.services.portforwarding import pf_plugin
 from neutron.tests.unit.db import test_db_base_plugin_v2
 from neutron.tests.unit.extensions import test_l3
 
 
-PLUGIN_NAME = 'port_forwarding'
+PF_PLUGIN_NAME = ('neutron.services.portforwarding.'
+                  'pf_plugin.PortForwardingPlugin')
 L3_PLUGIN = ('neutron.tests.unit.extensions.'
              'test_expose_port_forwarding_in_fip.'
              'TestL3PorForwardingServicePlugin')
@@ -70,14 +74,13 @@ class TestExtendFipPortForwardingExtension(
     def setUp(self):
         mock.patch('neutron.api.rpc.handlers.resources_rpc.'
                    'ResourcesPushRpcApi').start()
-        svc_plugins = {'l3_plugin_name': L3_PLUGIN,
+        svc_plugins = (L3_PLUGIN, PF_PLUGIN_NAME,
-                       'port_forwarding_plugin_name': PLUGIN_NAME,
+                       'neutron.services.qos.qos_plugin.QoSPlugin')
-                       'qos': 'neutron.services.qos.qos_plugin.QoSPlugin'}
         ext_mgr = ExtendFipPortForwardingExtensionManager()
         super(TestExtendFipPortForwardingExtension, self).setUp(
             plugin=CORE_PLUGIN, ext_mgr=ext_mgr, service_plugins=svc_plugins)
-        self.pf_plugin = pf_plugin.PortForwardingPlugin()
+        self.l3_plugin = directory.get_plugin(plugin_constants.L3)
-        self.l3_plugin = TestL3PorForwardingServicePlugin()
+        self.pf_plugin = directory.get_plugin(plugin_constants.PORTFORWARDING)
 
         ctx = context.get_admin_context()
         self.policy_1 = policy.QosPolicy(ctx,
@@ -291,3 +294,56 @@ class TestExtendFipPortForwardingExtension(
                     self._router_interface_action(
                         'remove', router['router']['id'],
                         insub3['subnet']['id'], None)
+
+    @mock.patch.object(l3_dvr_db.L3_NAT_with_dvr_db_mixin,
+                       '_notify_floating_ip_change')
+    @mock.patch.object(l3_dvr_db.DVRResourceOperationHandler,
+                       '_create_dvr_floating_gw_port')
+    def test_port_in_used_by_port_forwarding(self, mock_gw_port, mock_notify):
+        port_forwarding = {
+            apidef.RESOURCE_NAME:
+                {apidef.EXTERNAL_PORT: 2225,
+                 apidef.INTERNAL_PORT: 25,
+                 apidef.INTERNAL_PORT_ID: None,
+                 apidef.PROTOCOL: "tcp",
+                 apidef.INTERNAL_IP_ADDRESS: None}}
+        ctx = context.get_admin_context()
+        kwargs = {'arg_list': (extnet_apidef.EXTERNAL,),
+                  extnet_apidef.EXTERNAL: True}
+        with self.network(**kwargs) as extnet, self.network() as innet:
+            with self.subnet(network=extnet, cidr='200.0.0.0/22'),\
+                    self.subnet(network=innet, cidr='10.0.0.0/24') as insub,\
+                    self.router(distributed=True) as router:
+                fip = self._make_floatingip(self.fmt, extnet['network']['id'])
+                # check the floatingip response contains port_forwarding field
+                self.assertIn(apidef.COLLECTION_NAME, fip['floatingip'])
+                self._add_external_gateway_to_router(router['router']['id'],
+                                                     extnet['network']['id'])
+                self._router_interface_action('add', router['router']['id'],
+                                              insub['subnet']['id'], None)
+
+                with self.port(subnet=insub) as port1:
+                    update_dict1 = {
+                        apidef.INTERNAL_PORT_ID: port1['port']['id'],
+                        apidef.INTERNAL_IP_ADDRESS:
+                            port1['port']['fixed_ips'][0]['ip_address']}
+                    port_forwarding[apidef.RESOURCE_NAME].update(update_dict1)
+                    self.pf_plugin.create_floatingip_port_forwarding(
+                        ctx, fip['floatingip']['id'], port_forwarding)
+
+                    body = self._show('floatingips', fip['floatingip']['id'])
+                    self.assertEqual(
+                        1, len(body['floatingip'][apidef.COLLECTION_NAME]))
+
+                    self._make_floatingip(
+                        self.fmt,
+                        extnet['network']['id'],
+                        port_id=port1['port']['id'],
+                        http_status=exc.HTTPBadRequest.code)
+                    fip_2 = self._make_floatingip(self.fmt,
+                                                  extnet['network']['id'])
+                    self._update(
+                        'floatingips',
+                        fip_2['floatingip']['id'],
+                        {'floatingip': {'port_id': port1['port']['id']}},
+                        expected_code=exc.HTTPBadRequest.code)

neutron/tests/unit/extensions/test_l3.py

@@ -388,6 +388,8 @@ class L3NatTestCaseMixin(object):
             # Arg must be present and not empty
             if arg in kwargs:
                 data['router'][arg] = kwargs[arg]
+        if 'distributed' in kwargs:
+            data['router']['distributed'] = bool(kwargs['distributed'])
         router_req = self.new_create_request('routers', data, fmt)
         if set_context and tenant_id:
             # create a specific auth context for this request

releasenotes/notes/fip-binding-limitation-1d2509950847b085.yaml

@@ -0,0 +1,7 @@
+---
+other:
+  - |
+    If an instance port is under a dvr router, and the port already has
+    binding port forwarding(s). Neutron will no longer allow binding a
+    floating IP to that port again, because dvr floating IP traffic rules
+    will break the existing port forwarding functionality.