diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index 6d2c64f8564..54200a10a7e 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -775,6 +775,14 @@ def _protocol_name_map(self): if not self._iptables_protocol_name_map: tmp_map = constants.IPTABLES_PROTOCOL_NAME_MAP.copy() tmp_map.update(self._local_protocol_name_map()) + # iptables-save uses different strings for 'ipip' (protocol 4) + # depending on the distro, which corresponds to the entry for + # '4' in /etc/protocols. For example: + # - 'ipencap' in Ubuntu + # - 'ipv4' in CentOS/Fedora + # For this reason, we need to map the string for 'ipip' to the + # system-dependent string for '4', see bug #2054324. + tmp_map[constants.PROTO_NAME_IPIP] = tmp_map['4'] self._iptables_protocol_name_map = tmp_map return self._iptables_protocol_name_map diff --git a/neutron/objects/router.py b/neutron/objects/router.py index 60cf54e2972..0539f806761 100644 --- a/neutron/objects/router.py +++ b/neutron/objects/router.py @@ -10,8 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import itertools - import netaddr from neutron_lib.api.definitions import availability_zone as az_def @@ -409,20 +407,11 @@ def get_scoped_floating_ips(cls, context, router_ids): # Filter out on router_ids query = query.filter(l3.FloatingIP.router_id.in_(router_ids)) - return cls._unique_floatingip_iterator(context, query) - @classmethod - def _unique_floatingip_iterator(cls, context, query): - """Iterates over only one row per floating ip. Ignores others.""" - # Group rows by fip id. They must be sorted by same. - q = query.order_by(l3.FloatingIP.id) - keyfunc = lambda row: row[0]['id'] - group_iterator = itertools.groupby(q, keyfunc) - - # Just hit the first row of each group - for key, value in group_iterator: - # pylint: disable=stop-iteration-return - row = list(next(value)) + # Remove duplicate rows based on FIP IDs + query = query.group_by(l3.FloatingIP.id) + + for row in query: yield (cls._load_object(context, row[0]), row[1]) @classmethod diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py index 89434db9660..35b1e9ba2f8 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -489,6 +489,48 @@ def test_filter_ipv4_ingress_protocol_encap_by_num(self): egress = None self._test_prepare_port_filter(rule, ingress, egress) + def test_filter_ipv4_ingress_protocol_ipip(self): + # We want to use what the system-dependent string here is for 'ipip', + # as it could be 'ipencap' or 'ipv4' depending on the distro. + # See bug #2054324. + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': 'ipip'} + expected_proto_name = self.firewall._iptables_protocol_name('ipip') + ingress = mock.call.add_rule('ifake_dev', + '-p %s -j RETURN' % expected_proto_name, + top=False, comment=None) + egress = None + self._test_prepare_port_filter(rule, ingress, egress) + + def test_filter_ipv4_ingress_protocol_4(self): + # We want to use what the system-dependent string here is for '4', + # as it could be 'ipencap' or 'ipv4' depending on the distro. + # See bug #2054324. + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': '4'} + expected_proto_name = self.firewall._iptables_protocol_name('4') + ingress = mock.call.add_rule('ifake_dev', + '-p %s -j RETURN' % expected_proto_name, + top=False, comment=None) + egress = None + self._test_prepare_port_filter(rule, ingress, egress) + + def test_filter_ipv4_ingress_protocol_94(self): + # We want to use what the system-dependent string here is for '94', + # as it could be 'ipip' or something else depending on the distro. + # See bug #2054324. + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': '94'} + expected_proto_name = self.firewall._iptables_protocol_name('94') + ingress = mock.call.add_rule('ifake_dev', + '-p %s -j RETURN' % expected_proto_name, + top=False, comment=None) + egress = None + self._test_prepare_port_filter(rule, ingress, egress) + def test_filter_ipv4_ingress_protocol_999_local(self): # There is no protocol 999, so let's return a mapping # that says there is and make sure the rule is created diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py index 652ad8ae95c..4579fefd34f 100644 --- a/neutron/tests/unit/db/test_l3_db.py +++ b/neutron/tests/unit/db/test_l3_db.py @@ -252,27 +252,6 @@ def test__make_floatingip_dict_with_scope(self, make_fip_dict): 'fixed_ip_address_scope': mock.sentinel.address_scope_id, 'id': mock.sentinel.fip_ip}, result) - def test__unique_floatingip_iterator(self): - context = mock.MagicMock() - query = mock.MagicMock() - query.order_by().__iter__.return_value = [ - ({'id': 'id1'}, 'scope1'), - ({'id': 'id1'}, 'scope1'), - ({'id': 'id2'}, 'scope2'), - ({'id': 'id2'}, 'scope2'), - ({'id': 'id2'}, 'scope2'), - ({'id': 'id3'}, 'scope3')] - query.reset_mock() - with mock.patch.object( - l3_obj.FloatingIP, '_load_object', - side_effect=({'id': 'id1'}, {'id': 'id2'}, {'id': 'id3'})): - result = list( - l3_obj.FloatingIP._unique_floatingip_iterator(context, query)) - query.order_by.assert_called_once_with(l3_models.FloatingIP.id) - self.assertEqual([({'id': 'id1'}, 'scope1'), - ({'id': 'id2'}, 'scope2'), - ({'id': 'id3'}, 'scope3')], result) - @mock.patch.object(directory, 'get_plugin') def test_prevent_l3_port_deletion_port_not_found(self, gp): # port not found doesn't prevent diff --git a/neutron/tests/unit/objects/test_router.py b/neutron/tests/unit/objects/test_router.py index 88714e70113..5c86aec2fcb 100644 --- a/neutron/tests/unit/objects/test_router.py +++ b/neutron/tests/unit/objects/test_router.py @@ -12,8 +12,11 @@ # License for the specific language governing permissions and limitations # under the License. +from itertools import chain from unittest import mock +import netaddr + from neutron_lib.db import api as db_api from oslo_utils import uuidutils @@ -293,6 +296,51 @@ def test_v1_2_to_v1_1_drops_qos_network_policy_id(self): self.assertNotIn('qos_network_policy_id', obj_v1_1['versioned_object.data']) + def test_get_scoped_floating_ips(self): + def compare_results(router_ids, original_fips): + self.assertCountEqual( + original_fips, + [ + fip[0].id + for fip in router.FloatingIP.get_scoped_floating_ips( + self.context, router_ids) + ] + ) + + # Setup three routers, networks and external networks + routers = {} + for i in range(3): + router_id = self._create_test_router_id(name=f'router-{i}') + routers[router_id] = [] + net_id = self._create_test_network_id() + fip_net_id = self._create_external_network_id() + + # Create three subnets and three FIPs using the + # aforementioned networks and routers + for j in range(3): + self._create_test_subnet_id(net_id) + fip = router.FloatingIP( + self.context, + floating_ip_address=netaddr.IPAddress(f'10.{i}.{j}.3'), + floating_network_id=fip_net_id, + floating_port_id=self._create_test_port_id( + network_id=fip_net_id), + fixed_port_id=self._create_test_port_id( + network_id=net_id), + router_id=router_id, + ) + fip.create() + routers[router_id].append(fip.id) + + # For each router we created, fetch the fips and ensure the + # results match what we originally created + for router_id, original_fips in routers.items(): + compare_results([router_id], original_fips) + + # Now try to fetch all the fips for all the routers at once + original_fips = list(chain.from_iterable(routers.values())) + compare_results(routers.keys(), original_fips) + class DvrFipGatewayPortAgentBindingTestCase( obj_test_base.BaseObjectIfaceTestCase):