From 0fac0d515a0cf5696a37dcde4fb2fdff88a8b537 Mon Sep 17 00:00:00 2001 From: Elena Ezhova Date: Mon, 21 Aug 2017 01:08:13 +0400 Subject: [PATCH] Fix router update on L3 agent restart Currently on L3 agent restart FWaaS L3 agent extension _process_router_update iterates over all router ports and trigger firewall group update if a port belong to it. In case when a firewall group contains several ports iptables rules get re-written each time and in the result only the chains for the last port in a loop remain. With this change each firewall group would be updated with a full list of a router ports that belong to it. Additionaly, refactor of the _process_router_update method reduced its complexity and made it more readable. If a router would appear to have ports associated with several firewall groups a warning would be emitted. Added a unit test. Closes-Bug: #1712075 Change-Id: I251f4f50578cd10da904a56e1622c18f2adf2d18 --- .../l3reference/firewall_l3_agent_v2.py | 62 +++++++++++++------ .../l3reference/test_firewall_l3_agent_v2.py | 58 +++++++++++++++++ 2 files changed, 102 insertions(+), 18 deletions(-) diff --git a/neutron_fwaas/services/firewall/agents/l3reference/firewall_l3_agent_v2.py b/neutron_fwaas/services/firewall/agents/l3reference/firewall_l3_agent_v2.py index 3205e9f3c..6b08180d9 100644 --- a/neutron_fwaas/services/firewall/agents/l3reference/firewall_l3_agent_v2.py +++ b/neutron_fwaas/services/firewall/agents/l3reference/firewall_l3_agent_v2.py @@ -166,8 +166,12 @@ class FWaaSL3AgentExtension(l3_extension.L3AgentExtension): return self._get_in_ns_ports(fwg_port_ids) def _get_in_ns_ports(self, port_ids): - """Returns port objects in the local namespace, along with their - router_info. + """Get ports in namespace by their IDs. + + Returns port objects in the local namespace, along with their + router_info. + + :param port_ids: IDs of router ports (set, list or tuple) """ in_ns_ports = {} # This will be converted to a list later. if port_ids and self.agent_api: @@ -181,13 +185,21 @@ class FWaaSL3AgentExtension(l3_extension.L3AgentExtension): in_ns_ports[router_info] = [port_id] return list(in_ns_ports.items()) - def _invoke_driver_for_sync_from_plugin(self, ctx, port, firewall_group): - """Calls the FWaaS driver's delete_firewall_group method if firewall - group has status of PENDING_DELETE; calls driver's - update_firewall_group method for all other statuses. Both of these - methods are idempotent. + def _invoke_driver_for_sync_from_plugin(self, ctx, ports, firewall_group): + """Call driver to sync firewall group. + + Calls the FWaaS driver's delete_firewall_group method if firewall + group has status of PENDING_DELETE; calls driver's + update_firewall_group method for all other statuses. Both of these + methods are idempotent. + + :param ctx: RPC context + :param ports: IDs of ports associated with a firewall group + (set, list or tuple) + :param firewall_group: Dictionary describing the firewall group object + """ - port_list = self._get_in_ns_ports([port['id']]) + port_list = self._get_in_ns_ports(ports) if firewall_group['status'] == nl_constants.PENDING_DELETE: try: self.fwaas_driver.delete_firewall_group( @@ -245,17 +257,31 @@ class FWaaSL3AgentExtension(l3_extension.L3AgentExtension): ctx = context.Context('', updated_router['tenant_id']) fwg_list = self.fwplugin_rpc.get_firewall_groups_for_project(ctx) + if nl_constants.INTERFACE_KEY not in updated_router: + return + # Apply a firewall group, as requested, to ports on the new router. - if nl_constants.INTERFACE_KEY in updated_router: - for port in updated_router[nl_constants.INTERFACE_KEY]: - for firewall_group in fwg_list: - if (self._has_port_insertion_fields(firewall_group) and - (port['id'] in firewall_group['add-port-ids'] or - port['id'] in firewall_group['del-port-ids'])): - self._invoke_driver_for_sync_from_plugin(ctx, port, - firewall_group) - # A port can have at most one firewall group. - break + all_router_ports = set( + p['id'] for p in updated_router[nl_constants.INTERFACE_KEY] + ) + processed_ports = set() + for firewall_group in fwg_list: + if not self._has_port_insertion_fields(firewall_group): + continue + + ports_to_process = (set(firewall_group['add-port-ids'] + + firewall_group['del-port-ids']) & + all_router_ports) + # A port can have at most one firewall group. + port_ids_to_exclude = ports_to_process & processed_ports + if port_ids_to_exclude: + LOG.warning("Port(s) %s is associated with " + "more than one firewall group(s).", + port_ids_to_exclude) + ports_to_process -= port_ids_to_exclude + self._invoke_driver_for_sync_from_plugin( + ctx, ports_to_process, firewall_group) + processed_ports |= ports_to_process def add_router(self, context, new_router): """Handles agent restart and router add. Fetches firewall groups from diff --git a/neutron_fwaas/tests/unit/services/firewall/agents/l3reference/test_firewall_l3_agent_v2.py b/neutron_fwaas/tests/unit/services/firewall/agents/l3reference/test_firewall_l3_agent_v2.py index 2550fa00c..bf3cbab8a 100644 --- a/neutron_fwaas/tests/unit/services/firewall/agents/l3reference/test_firewall_l3_agent_v2.py +++ b/neutron_fwaas/tests/unit/services/firewall/agents/l3reference/test_firewall_l3_agent_v2.py @@ -383,4 +383,62 @@ class TestFWaaSL3AgentExtension(base.BaseTestCase): mock_get_ipt_mgrs_with_if_prefix.assert_any_call( agent.conf.agent_mode, mock.ANY) + @mock.patch('oslo_utils.importutils.import_object') + def test_add_router_with_several_ports(self, mock_import_object): + fw_agent = _setup_test_agent_class([fwaas_constants.FIREWALL]) + cfg.CONF.set_override('enabled', True, 'fwaas') + updated_router = { + '_interfaces': [ + {'device_owner': 'network: router_interface', + 'id': '1', + 'tenant_id': 'demo_tenant_id'}, + {'device_owner': 'network: router_interface', + 'id': '2', + 'tenant_id': 'demo_tenant_id'}, + {'device_owner': 'network: router_interface', + 'id': '3', + 'tenant_id': 'demo_tenant_id'}], + 'tenant_id': 'demo_tenant_id', + 'id': '0b109a4e-d228-479d-ad43-08bf3245adbb', + 'name': 'demo_router' + } + fwg1 = { + 'status': 'ACTIVE', + 'admin_state_up': True, + 'tenant_id': 'demo_tenant_id', + 'del-port-ids': [], + 'add-port-ids': ['1', '3'], + 'id': '2932b3d9-3a7b-48a1-a16c-bf9f7b2751a5' + } + fwg2 = { + 'status': 'ACTIVE', + 'admin_state_up': True, + 'tenant_id': 'demo_tenant_id', + 'del-port-ids': [], + 'add-port-ids': ['2', '3'], + 'id': '2932b3d9-3a7b-48a1-a16c-bf9f7b2751a5' + } + agent = fw_agent(cfg.CONF) + agent.agent_api = mock.Mock() + agent.fwplugin_rpc = mock.Mock() + agent.conf.agent_mode = mock.Mock() + agent.fwaas_driver = iptables_fwaas_v2.IptablesFwaasDriver() + + patch_project = mock.patch.object( + agent.fwplugin_rpc, 'get_firewall_groups_for_project') + patch_invoke = mock.patch.object( + agent, '_invoke_driver_for_sync_from_plugin') + + with patch_project as mock_get_firewall_groups, \ + patch_invoke as mock_invoke_driver: + mock_get_firewall_groups.return_value = [fwg1, fwg2] + agent.add_router(self.context, updated_router) + + # Check that mock_invoke_driver was called exactly twice with + # correct arguments. + self.assertEqual([ + mock.call(mock.ANY, {'1', '3'}, fwg1), + mock.call(mock.ANY, {'2'}, fwg2), + ], mock_invoke_driver.call_args_list) + #TODO(Margaret) Add test for add_router method.