From 7c7b020abe15fb7086dcca9095cddb964f03eab6 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Thu, 22 Feb 2024 18:33:14 -0500 Subject: [PATCH 1/3] Fix iptables mapping of 'ipip' protocol Map 'ipip' to use the string 'ipencap' so the IptablesFirewallDriver class in neutron works correctly. Once neutron-lib is bumped this can be removed. Add tests for IP protocol 'ipip', '4' and '94' to make sure the IptablesFirewallDriver class in neutron treats them correctly. Long description below. This is one of those confusing edge cases and I think Linux is conspiring against us. Let me explain. 1) neutron-lib does correctly define the protocol name 'ipip' as 4. 2) The linux kernel uses the same in in.h: IPPROTO_IPIP = 4 IPPROTO_BEETPH = 94 (?) 3) iptables maps 'ipip' to 94 and 'ipencap' to 4. # for num in {0..255}; do iptables -A INPUT -p $num; done # iptables-save | grep -E 'ipip|ipencap' -A INPUT -p ipencap -A INPUT -p ipip 4) /etc/protocols does the same as iptables: grep -E 'ipencap|ipip' /etc/protocols ipencap 4 IP-ENCAP # IP encapsulated in IP (officially ``IP'') ipip 94 IPIP # IP-within-IP Encapsulation Protocol 5) getprotoby{name|number} does what /etc/protocols does: $ getprotobyname ipip struct protoent: (0x7fbbbcca9c60) p_name ipip p_aliases IPIP p_proto 94 $ getprotobynumber 4 struct protoent: (0x7fc51ad86be0) p_name ipencap p_aliases IP-ENCAP p_proto 4 Neutron actually builds a mapping based on the getprotoby* calls, so in the iptables case it winds-up doing the wrong thing. Partial-bug: #2054324 Change-Id: Icc84b54be07d39059723d6c233c03aa130102423 (cherry picked from commit 793dfb04d087c96470dc4913eb4f17aeff777534) --- neutron/agent/linux/iptables_firewall.py | 4 +++ .../agent/linux/test_iptables_firewall.py | 36 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index 28180c838bc..7c98b080e10 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -769,6 +769,10 @@ 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()) + # TODO(haleyb): remove once neutron-lib with fix is available + # - 'ipip' uses 'ipencap' to match IPPROTO_IPIP from in.h, + # which is IP-ENCAP/'4' in /etc/protocols (see bug #2054324) + tmp_map[constants.PROTO_NAME_IPIP] = 'ipencap' self._iptables_protocol_name_map = tmp_map return self._iptables_protocol_name_map diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py index b177c7ee06c..a2bf18c3652 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -489,6 +489,42 @@ 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): + # 'ipip' via the API uses 'ipencap' to match what iptables-save + # uses, which is IP-ENCAP/'4' from /etc/protocols (see bug #2054324) + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': 'ipip'} + ingress = mock.call.add_rule('ifake_dev', + '-p ipencap -j RETURN', + top=False, comment=None) + egress = None + self._test_prepare_port_filter(rule, ingress, egress) + + def test_filter_ipv4_ingress_protocol_ipip_by_num(self): + # '4' via the API uses 'ipencap' to match what iptables-save + # uses, which is IP-ENCAP/'4' from /etc/protocols (see bug #2054324) + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': '4'} + ingress = mock.call.add_rule('ifake_dev', + '-p ipencap -j RETURN', + top=False, comment=None) + egress = None + self._test_prepare_port_filter(rule, ingress, egress) + + def test_filter_ipv4_ingress_protocol_ipencap_by_num(self): + # '94' via the API uses 'ipip' to match what iptables-save + # uses, which is IPIP/'94' from /etc/protocols (see bug #2054324) + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': '94'} + ingress = mock.call.add_rule('ifake_dev', + '-p ipip -j RETURN', + 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 From 29d28152ac70e8897de3841a32262379f2ef1578 Mon Sep 17 00:00:00 2001 From: Adam Oswick Date: Thu, 2 Mar 2023 13:26:50 +0000 Subject: [PATCH 2/3] Remove duplicate rows in MySQL query output Previously we would expect duplicates rows from MySQL and then filter them with _unique_floatingip_iterator. This means that we were converting rows to ORM-mapped objects unnecessarily. This conversion is very CPU intensive. Instead, we can remove the duplicates as a part of the query which means that only unique rows are returned and the number of conversions from rows to ORM-mapped objects is reduced significantly. It also means that the _unique_floatingip_iterator function is no longer needed. Change-Id: I05136302f7b8abc0a985b91c993008595234ad6b Signed-off-by: Adam Oswick (cherry picked from commit 8946684fb2a8cc800b5decac95acc3106eb723d0) (cherry picked from commit f96691b2088ac2a4a165d2d5281a8a891734acfd) --- neutron/objects/router.py | 19 ++------- neutron/tests/unit/db/test_l3_db.py | 21 ---------- neutron/tests/unit/objects/test_router.py | 48 +++++++++++++++++++++++ 3 files changed, 52 insertions(+), 36 deletions(-) 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/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): From 386ebde547419a3cd2ba9e2228da000cf43bfa17 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Tue, 5 Mar 2024 11:59:05 -0500 Subject: [PATCH 3/3] Use the system-dependent string for IP protocol 4 iptables-save uses a system-dependent value, usually that found in /etc/protocols, when 'ipip' is given as the security group protocol. The intent is to always use the string value for IP protocol '4', as iptables-save has no '-n' flag to print values numerically. This updates a previous change (793dfb04d) that hard-coded that string to 'ipencap', which broke CentOS/Fedora, which uses 'ipv4'. For this reason we cannot hard-code anything in neutron-lib, this needs to be added dynamically, so this one-line change needs to stay here, and effectively closes the bug. Closes-bug: #2054324 Change-Id: Ic40b539c9ef5cfa4cbbd6575e19e653342e8342b (cherry picked from commit cd1d191e335dca707ac9324fd81e642cb453a6cf) --- neutron/agent/linux/iptables_firewall.py | 12 +++++--- .../agent/linux/test_iptables_firewall.py | 28 +++++++++++-------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index 7c98b080e10..7d97e6e0878 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -769,10 +769,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()) - # TODO(haleyb): remove once neutron-lib with fix is available - # - 'ipip' uses 'ipencap' to match IPPROTO_IPIP from in.h, - # which is IP-ENCAP/'4' in /etc/protocols (see bug #2054324) - tmp_map[constants.PROTO_NAME_IPIP] = 'ipencap' + # 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/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py index a2bf18c3652..fec4e9c1eb9 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -490,37 +490,43 @@ def test_filter_ipv4_ingress_protocol_encap_by_num(self): self._test_prepare_port_filter(rule, ingress, egress) def test_filter_ipv4_ingress_protocol_ipip(self): - # 'ipip' via the API uses 'ipencap' to match what iptables-save - # uses, which is IP-ENCAP/'4' from /etc/protocols (see bug #2054324) + # 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 ipencap -j RETURN', + '-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_ipip_by_num(self): - # '4' via the API uses 'ipencap' to match what iptables-save - # uses, which is IP-ENCAP/'4' from /etc/protocols (see bug #2054324) + 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 ipencap -j RETURN', + '-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_ipencap_by_num(self): - # '94' via the API uses 'ipip' to match what iptables-save - # uses, which is IPIP/'94' from /etc/protocols (see bug #2054324) + 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 ipip -j RETURN', + '-p %s -j RETURN' % expected_proto_name, top=False, comment=None) egress = None self._test_prepare_port_filter(rule, ingress, egress)