From b8d2ab8543a27b03bde534ef994027d9b44556c4 Mon Sep 17 00:00:00 2001 From: LIU Yulong Date: Mon, 8 Oct 2018 14:52:16 +0800 Subject: [PATCH] Prevent create port forwarding to port which has binding fip For dvr scenario, if port has a bound floating, and then create port forwarding to it, this port forwarding will not work, due to the traffic is redirected to dvr rules. This patch restricts such API request, if user try to create port forwarding to a port, check if it has bound floating IP first. This will be run for all type of routers, since neutron should not let user to waste public IP address on a port which already has a floating IP, it can take care all the procotol port numbers. Closes-Bug: #1799137 Change-Id: I4ba4b023d79185f8d478d60ce16417d3501bf785 --- .../portforwarding/common/exceptions.py | 7 ++++ neutron/services/portforwarding/pf_plugin.py | 16 ++++++++ .../portforwarding/test_port_forwarding.py | 40 ++++++++++++++++++ .../services/portforwarding/test_pf_plugin.py | 41 ++++++++++++++++++- 4 files changed, 102 insertions(+), 2 deletions(-) diff --git a/neutron/services/portforwarding/common/exceptions.py b/neutron/services/portforwarding/common/exceptions.py index f69c07d82f0..56af6e895e2 100644 --- a/neutron/services/portforwarding/common/exceptions.py +++ b/neutron/services/portforwarding/common/exceptions.py @@ -27,3 +27,10 @@ class PortForwardingNotSupportFilterField(n_exc.BadRequest): class FipInUseByPortForwarding(n_exc.InUse): message = _("Floating IP %(id)s in use by Port Forwarding resources.") + + +class PortHasBindingFloatingIP(n_exc.InUse): + message = _("Cannot create port forwarding to floating IP " + "%(floating_ip_address)s (%(fip_id)s) with port %(port_id)s " + "using fixed IP %(fixed_ip)s, as that port already " + "has a binding floating IP.") diff --git a/neutron/services/portforwarding/pf_plugin.py b/neutron/services/portforwarding/pf_plugin.py index 9616c3d0ada..d3ac1c3da13 100644 --- a/neutron/services/portforwarding/pf_plugin.py +++ b/neutron/services/portforwarding/pf_plugin.py @@ -277,12 +277,26 @@ class PortForwardingPlugin(fip_pf.PortForwardingPluginBase): raise lib_exc.BadRequest(resource=apidef.RESOURCE_NAME, msg=message) + def _check_port_has_binding_floating_ip(self, context, port_forwarding): + port_id = port_forwarding['internal_port_id'] + floatingip_objs = l3_obj.FloatingIP.get_objects( + context.elevated(), + fixed_port_id=port_id) + if floatingip_objs: + floating_ip_address = floatingip_objs[0].floating_ip_address + raise pf_exc.PortHasBindingFloatingIP( + floating_ip_address=floating_ip_address, + fip_id=floatingip_objs[0].id, + port_id=port_id, + fixed_ip=port_forwarding['internal_ip_address']) + @db_base_plugin_common.convert_result_to_dict def create_floatingip_port_forwarding(self, context, floatingip_id, port_forwarding): port_forwarding = port_forwarding.get(apidef.RESOURCE_NAME) port_forwarding['floatingip_id'] = floatingip_id + self._check_port_has_binding_floating_ip(context, port_forwarding) with db_api.CONTEXT_WRITER.using(context): fip_obj = self._get_fip_obj(context, floatingip_id) if fip_obj.fixed_port_id: @@ -329,6 +343,8 @@ class PortForwardingPlugin(fip_pf.PortForwardingPluginBase): new_internal_port_id = None if port_forwarding and port_forwarding.get('internal_port_id'): new_internal_port_id = port_forwarding.get('internal_port_id') + self._check_port_has_binding_floating_ip(context, port_forwarding) + try: with db_api.CONTEXT_WRITER.using(context): fip_obj = self._get_fip_obj(context, floatingip_id) diff --git a/neutron/tests/functional/services/portforwarding/test_port_forwarding.py b/neutron/tests/functional/services/portforwarding/test_port_forwarding.py index 340d134ba47..1c94f931344 100644 --- a/neutron/tests/functional/services/portforwarding/test_port_forwarding.py +++ b/neutron/tests/functional/services/portforwarding/test_port_forwarding.py @@ -161,6 +161,46 @@ class PortForwardingTestCase(PortForwardingTestCaseBase): self.pf_plugin.create_floatingip_port_forwarding, self.context, self.fip['id'], self.port_forwarding) + def test_create_port_forwarding_port_in_used_by_fip(self): + normal_fip = self._create_floatingip(self.ext_net['id']) + self._update_floatingip(normal_fip['id'], {'port_id': self.port['id']}) + self.assertRaises( + pf_exc.PortHasBindingFloatingIP, + self.pf_plugin.create_floatingip_port_forwarding, + self.context, self.fip['id'], self.port_forwarding) + + def test_update_port_forwarding_port_in_used_by_fip(self): + normal_fip = self._create_floatingip(self.ext_net['id']) + normal_port = self._create_port( + self.fmt, self.net['id']).json['port'] + self._update_floatingip( + normal_fip['id'], {'port_id': normal_port['id']}) + + res = self.pf_plugin.create_floatingip_port_forwarding( + self.context, self.fip['id'], self.port_forwarding) + expect = { + "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(expect, res) + + # Directly update port forwarding to a port which already has + # bound floating IP. + self.port_forwarding[apidef.RESOURCE_NAME].update( + {apidef.INTERNAL_PORT_ID: normal_port['id'], + apidef.INTERNAL_IP_ADDRESS: + normal_port['fixed_ips'][0]['ip_address']}) + self.assertRaises( + pf_exc.PortHasBindingFloatingIP, + self.pf_plugin.update_floatingip_port_forwarding, + self.context, res['id'], self.fip['id'], self.port_forwarding) + def test_update_floatingip_port_forwarding(self): # create a test port forwarding res = self.pf_plugin.create_floatingip_port_forwarding( diff --git a/neutron/tests/unit/services/portforwarding/test_pf_plugin.py b/neutron/tests/unit/services/portforwarding/test_pf_plugin.py index 8309ddf348d..06d94b8eef5 100644 --- a/neutron/tests/unit/services/portforwarding/test_pf_plugin.py +++ b/neutron/tests/unit/services/portforwarding/test_pf_plugin.py @@ -189,6 +189,8 @@ class TestPortForwardingPlugin(testlib_api.SqlTestCase): self.pf_plugin.update_floatingip_port_forwarding, self.ctxt, 'pf_id', **pf_input) + @mock.patch.object(pf_plugin.PortForwardingPlugin, + '_check_port_has_binding_floating_ip') @mock.patch.object(obj_base.NeutronDbObject, 'update_objects') @mock.patch.object(resources_rpc.ResourcesPushRpcApi, 'push') @mock.patch.object(pf_plugin.PortForwardingPlugin, '_check_router_match') @@ -199,7 +201,8 @@ class TestPortForwardingPlugin(testlib_api.SqlTestCase): @mock.patch('neutron.objects.port_forwarding.PortForwarding') def test_create_floatingip_port_forwarding( self, mock_port_forwarding, mock_fip_get_object, mock_find_router, - mock_check_router_match, mock_push_api, mock_update_objects): + mock_check_router_match, mock_push_api, mock_update_objects, + mock_check_bind_fip): # Update fip pf_input = { 'port_forwarding': @@ -240,6 +243,8 @@ class TestPortForwardingPlugin(testlib_api.SqlTestCase): mock_push_api.assert_called_once_with( self.ctxt, mock.ANY, rpc_events.CREATED) + @mock.patch.object(pf_plugin.PortForwardingPlugin, + '_check_port_has_binding_floating_ip') @mock.patch.object(pf_plugin.PortForwardingPlugin, '_find_existing_port_forwarding') @mock.patch.object(pf_plugin.PortForwardingPlugin, @@ -252,7 +257,8 @@ class TestPortForwardingPlugin(testlib_api.SqlTestCase): def test_negative_create_floatingip_port_forwarding( self, mock_port_forwarding, mock_fip_get_object, mock_find_router, - mock_check_router_match, mock_try_find_exist): + mock_check_router_match, mock_try_find_exist, + mock_check_bind_fip): pf_input = { 'port_forwarding': { 'internal_ip_address': '1.1.1.1', @@ -330,3 +336,34 @@ class TestPortForwardingPlugin(testlib_api.SqlTestCase): self.assertRaises(lib_exc.BadRequest, self.pf_plugin._check_router_match, self.ctxt, fip_obj, router_id, pf_dict) + + @mock.patch.object(router.FloatingIP, 'get_objects') + def test_create_floatingip_port_forwarding_port_in_use( + self, mock_fip_get_objects): + pf_input = { + 'port_forwarding': + {'port_forwarding': { + 'internal_ip_address': '1.1.1.1', + 'internal_port_id': 'internal_neutron_port', + 'floatingip_id': 'fip_id_1'}}, + 'floatingip_id': 'fip_id_1'} + fip_obj = mock.Mock(floating_ip_address="10.10.10.10") + mock_fip_get_objects.return_value = [fip_obj] + self.assertRaises(pf_exc.PortHasBindingFloatingIP, + self.pf_plugin.create_floatingip_port_forwarding, + self.ctxt, **pf_input) + + @mock.patch.object(router.FloatingIP, 'get_objects') + def test_update_floatingip_port_forwarding_port_in_use( + self, mock_fip_get_objects): + pf_input = { + 'port_forwarding': + {'port_forwarding': { + 'internal_ip_address': '1.1.1.1', + 'internal_port_id': 'internal_neutron_port', + 'floatingip_id': 'fip_id_2'}}} + fip_obj = mock.Mock(floating_ip_address="10.10.10.11") + mock_fip_get_objects.return_value = [fip_obj] + self.assertRaises(pf_exc.PortHasBindingFloatingIP, + self.pf_plugin.update_floatingip_port_forwarding, + self.ctxt, 'fake-pf-id', 'fip_id_2', **pf_input)