From b3647782e4dae56529bbf59918a51326df47b32e Mon Sep 17 00:00:00 2001 From: Flavio Fernandes Date: Thu, 25 Jun 2020 23:34:32 -0400 Subject: [PATCH] port_forwarding: validate args before invoking db update Add validator to update_floatingip_port_forwarding so codepath does not attempt performing invalid database operation. With that, operation fails right away, with a hint on the offending argument(s). This is a backport that combines changes from 2 changes that go together: https://review.opendev.org/#/c/738145/ https://review.opendev.org/#/c/744993/ Note: pep8 failed with following error: ./neutron/tests/unit/services/portforwarding/test_pf_plugin.py:237:9: ./neutron/tests/unit/services/portforwarding/test_pf_plugin.py:261:9: N322 Possible use of no-op mock method. please use assert_called_once_with. mock_pf_get_objects.assert_called_once() ^ So additional changes were needed for backport. Change-Id: I8284b22c5d691bfd9eadeb8590c3d4b27d261b04 Closes-Bug: #1878299 (cherry picked from commit f3797403486ba43100ff9ec8917d38c8723e12b2) (cherry picked from commit 838399f0a489e898cfefe49dbe026553bc117381) --- neutron/services/portforwarding/pf_plugin.py | 43 +++++++++ .../services/portforwarding/test_pf_plugin.py | 89 ++++++++++++++++++- 2 files changed, 131 insertions(+), 1 deletion(-) diff --git a/neutron/services/portforwarding/pf_plugin.py b/neutron/services/portforwarding/pf_plugin.py index 86db3404d01..0d82b12c73e 100644 --- a/neutron/services/portforwarding/pf_plugin.py +++ b/neutron/services/portforwarding/pf_plugin.py @@ -400,6 +400,7 @@ class PortForwardingPlugin(fip_pf.PortForwardingPluginBase): pf_obj.internal_port }) pf_obj.update_fields(port_forwarding, reset_changes=True) + self._check_port_forwarding_update(context, pf_obj) pf_obj.update() except obj_exc.NeutronDbObjectDuplicateEntry: (__, conflict_params) = self._find_existing_port_forwarding( @@ -437,6 +438,48 @@ class PortForwardingPlugin(fip_pf.PortForwardingPluginBase): raise lib_exc.BadRequest(resource=apidef.RESOURCE_NAME, msg=message) + def _check_port_forwarding_update(self, context, pf_obj): + def _raise_port_forwarding_update_failed(conflict): + message = _("Another port forwarding entry with the same " + "attributes already exists, conflicting " + "values are %s") % conflict + raise lib_exc.BadRequest(resource=apidef.RESOURCE_NAME, + msg=message) + + db_port = self.core_plugin.get_port(context, pf_obj.internal_port_id) + for fixed_ip in db_port['fixed_ips']: + if str(pf_obj.internal_ip_address) == fixed_ip['ip_address']: + break + else: + # Reached end of internal_port iteration w/out finding fixed_ip + message = _("The internal IP does not correspond to an " + "address on the internal port, which has " + "fixed_ips %s") % db_port['fixed_ips'] + raise lib_exc.BadRequest(resource=apidef.RESOURCE_NAME, + msg=message) + objs = pf.PortForwarding.get_objects( + context, + floatingip_id=pf_obj.floatingip_id, + protocol=pf_obj.protocol) + for obj in objs: + if obj.id == pf_obj.id: + continue + # Ensure there are no conflicts on the outside + if (obj.floating_ip_address == pf_obj.floating_ip_address and + obj.external_port == pf_obj.external_port): + _raise_port_forwarding_update_failed( + {'floating_ip_address': str(obj.floating_ip_address), + 'external_port': obj.external_port}) + # Ensure there are no conflicts in the inside + # socket: internal_ip_address + internal_port + if (obj.internal_port_id == pf_obj.internal_port_id and + obj.internal_ip_address == pf_obj.internal_ip_address and + obj.internal_port == pf_obj.internal_port): + _raise_port_forwarding_update_failed( + {'internal_port_id': obj.internal_port_id, + 'internal_ip_address': str(obj.internal_ip_address), + 'internal_port': obj.internal_port}) + def _find_existing_port_forwarding(self, context, floatingip_id, port_forwarding, specify_params=None): # Because the session had been flushed by NeutronDbObjectDuplicateEntry diff --git a/neutron/tests/unit/services/portforwarding/test_pf_plugin.py b/neutron/tests/unit/services/portforwarding/test_pf_plugin.py index 06d94b8eef5..8a8a1c8f157 100644 --- a/neutron/tests/unit/services/portforwarding/test_pf_plugin.py +++ b/neutron/tests/unit/services/portforwarding/test_pf_plugin.py @@ -15,6 +15,7 @@ import mock +from neutron_lib.callbacks import registry from neutron_lib import context from neutron_lib import exceptions as lib_exc from neutron_lib.exceptions import l3 as lib_l3_exc @@ -154,10 +155,14 @@ class TestPortForwardingPlugin(testlib_api.SqlTestCase): self.pf_plugin.delete_floatingip_port_forwarding, self.ctxt, 'pf_id', floatingip_id='fip_id') + @mock.patch.object(port_forwarding.PortForwarding, 'get_objects') + @mock.patch.object(db_base_plugin_v2.NeutronDbPluginV2, 'get_port') + @mock.patch.object(registry, 'notify') @mock.patch.object(resources_rpc.ResourcesPushRpcApi, 'push') @mock.patch.object(port_forwarding.PortForwarding, 'get_object') def test_update_floatingip_port_forwarding( - self, mock_pf_get_object, mock_rpc_push): + self, mock_pf_get_object, mock_rpc_push, mock_registry_notify, + mock_get_port, mock_pf_get_objects): pf_input = { 'port_forwarding': {'port_forwarding': { @@ -165,7 +170,12 @@ class TestPortForwardingPlugin(testlib_api.SqlTestCase): 'floatingip_id': 'fip_id'}}, 'floatingip_id': 'fip_id'} pf_obj = mock.Mock() + pf_obj.internal_ip_address = "10.0.0.1" mock_pf_get_object.return_value = pf_obj + port_dict = {'id': 'ID', 'fixed_ips': [{"subnet_id": "test-subnet-id", + "ip_address": "10.0.0.1"}]} + mock_get_port.return_value = port_dict + mock_pf_get_objects.return_value = [] self.pf_plugin.update_floatingip_port_forwarding( self.ctxt, 'pf_id', **pf_input) mock_pf_get_object.assert_called_once_with(self.ctxt, id='pf_id') @@ -174,6 +184,83 @@ class TestPortForwardingPlugin(testlib_api.SqlTestCase): mock_rpc_push.assert_called_once_with( self.ctxt, mock.ANY, rpc_events.UPDATED) + @mock.patch.object(port_forwarding.PortForwarding, 'get_objects') + @mock.patch.object(db_base_plugin_v2.NeutronDbPluginV2, 'get_port') + def test_check_port_forwarding_update(self, mock_get_port, + mock_pf_get_objects): + port_dict = {'fixed_ips': [{"ip_address": "10.0.0.1"}]} + mock_get_port.return_value = port_dict + other_pf_obj = mock.Mock(id='cmp_obj_id') + pf_obj = mock.Mock(id='pf_obj_id', internal_port_id='int_port_id', + internal_ip_address='10.0.0.1') + mock_pf_get_objects.return_value = [pf_obj, other_pf_obj] + self.pf_plugin._check_port_forwarding_update(self.ctxt, pf_obj) + mock_get_port.assert_called_once_with(self.ctxt, 'int_port_id') + mock_pf_get_objects.assert_called_once_with(self.ctxt, + floatingip_id=pf_obj.floatingip_id, protocol=pf_obj.protocol) + + @mock.patch.object(port_forwarding.PortForwarding, 'get_objects') + @mock.patch.object(db_base_plugin_v2.NeutronDbPluginV2, 'get_port') + def test_check_port_forwarding_update_invalid_address( + self, mock_get_port, mock_pf_get_objects): + port_dict = {'fixed_ips': [{"ip_address": "10.0.0.1"}]} + mock_get_port.return_value = port_dict + pf_obj = mock.Mock(id='pf_obj_id', internal_port_id='int_port_id', + internal_ip_address="99.99.99.99") + self.assertRaisesRegex( + lib_exc.BadRequest, + "not correspond to an address on the internal port", + self.pf_plugin._check_port_forwarding_update, + self.ctxt, pf_obj) + mock_get_port.assert_called_once_with(self.ctxt, 'int_port_id') + mock_pf_get_objects.assert_not_called() + + @mock.patch.object(port_forwarding.PortForwarding, 'get_objects') + @mock.patch.object(db_base_plugin_v2.NeutronDbPluginV2, 'get_port') + def test_check_port_forwarding_update_invalid_external( + self, mock_get_port, mock_pf_get_objects): + port_dict = {'fixed_ips': [{"ip_address": "10.0.0.1"}]} + mock_get_port.return_value = port_dict + pf_obj_dict = {'id': 'pf_obj_one', + 'floating_ip_address': 'same_fip_addr', + 'external_port': 'same_ext_port'} + other_pf_obj = mock.Mock(**pf_obj_dict) + pf_obj_dict.update(id='pf_obj_two', internal_ip_address='10.0.0.1') + pf_obj = mock.Mock(**pf_obj_dict) + mock_pf_get_objects.return_value = [pf_obj, other_pf_obj] + self.assertRaisesRegex( + lib_exc.BadRequest, + "already exist.*same_fip_addr", + self.pf_plugin._check_port_forwarding_update, + self.ctxt, pf_obj) + mock_get_port.assert_called_once_with(self.ctxt, mock.ANY) + mock_pf_get_objects.assert_called_with( + self.ctxt, floatingip_id=mock.ANY, protocol=mock.ANY) + + @mock.patch.object(port_forwarding.PortForwarding, 'get_objects') + @mock.patch.object(db_base_plugin_v2.NeutronDbPluginV2, 'get_port') + def test_check_port_forwarding_update_invalid_internal( + self, mock_get_port, mock_pf_get_objects): + same_internal_ip = "10.0.0.10" + port_dict = {'fixed_ips': [{"ip_address": same_internal_ip}]} + mock_get_port.return_value = port_dict + pf_obj_dict = {'id': 'pf_obj_one', + 'internal_port_id': 'same_int_port_id', + 'internal_ip_address': same_internal_ip, + 'internal_port': 'same_int_port'} + other_pf_obj = mock.Mock(**pf_obj_dict) + pf_obj_dict.update(id='pf_obj_two') + pf_obj = mock.Mock(**pf_obj_dict) + mock_pf_get_objects.return_value = [pf_obj, other_pf_obj] + self.assertRaisesRegex( + lib_exc.BadRequest, + "already exist.*{}".format(same_internal_ip), + self.pf_plugin._check_port_forwarding_update, + self.ctxt, pf_obj) + mock_get_port.assert_called_once_with(self.ctxt, 'same_int_port_id') + mock_pf_get_objects.assert_called_with( + self.ctxt, floatingip_id=mock.ANY, protocol=mock.ANY) + @mock.patch.object(port_forwarding.PortForwarding, 'get_object') def test_negative_update_floatingip_port_forwarding( self, mock_pf_get_object):