From d040a38f499d7d3d9dea9192a98583c1cc081ecd Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Fri, 26 Apr 2024 22:48:09 -0400 Subject: [PATCH] Rename create_flows_for_ip_address to reflect it accepts (ip, mac) The semantics changed since I2e3aa7c400d7bb17cc117b65faaa160b41013dde but the name was not updated. This patch also revealed a number of issues in unit tests where mock flow_states were using the old key format (bare ip) instead of the new (ip, mac) tuple. These are also now fixed. Change-Id: Ic96b5ce6576c31075602e0f033fd74acbae333ee --- .../linux/openvswitch_firewall/firewall.py | 31 +++++++------- .../agent/linux/openvswitch_firewall/rules.py | 9 ++--- .../openvswitch_firewall/test_firewall.py | 40 +++++++++++++------ .../linux/openvswitch_firewall/test_rules.py | 4 +- 4 files changed, 49 insertions(+), 35 deletions(-) diff --git a/neutron/agent/linux/openvswitch_firewall/firewall.py b/neutron/agent/linux/openvswitch_firewall/firewall.py index 27797191a1d..c287aadfb5a 100644 --- a/neutron/agent/linux/openvswitch_firewall/firewall.py +++ b/neutron/agent/linux/openvswitch_firewall/firewall.py @@ -415,11 +415,11 @@ class ConjIPFlowManager(object): flow_state, addr_to_conj, direction, ethertype, vlan_tag) for conj_id_set in conj_id_to_remove: # Remove any remaining flow with remote SG/AG ID conj_id_to_remove - for current_ip, conj_ids in flow_state.items(): + for (current_ip, current_mac), conj_ids in flow_state.items(): conj_ids_to_remove = conj_id_set & set(conj_ids) self.driver.delete_flow_for_ip( - current_ip, direction, ethertype, vlan_tag, - conj_ids_to_remove) + current_ip, current_mac, direction, ethertype, + vlan_tag, conj_ids_to_remove) # NOTE(hangyang): Handle add/delete overlapped IPs among # remote security groups and remote address groups @@ -451,8 +451,8 @@ class ConjIPFlowManager(object): # match flow priority or actions for different conjunction # ids, therefore we need to recreate the affected flows. continue - for flow in rules.create_flows_for_ip_address( - (addr, mac), direction, ethertype, vlan_tag, conj_ids): + for flow in rules.create_flows_for_ip_address_and_mac( + addr, mac, direction, ethertype, vlan_tag, conj_ids): self.driver._add_flow(flow_group_id=ofport, **flow) def update_flows_for_vlan(self, vlan_tag, ofport, conj_id_to_remove=None): @@ -1621,23 +1621,24 @@ class OVSFirewallDriver(firewall.FirewallDriver): self, flow_state, addr_to_conj, direction, ethertype, vlan_tag): # Remove rules for deleted IPs and action=conjunction(conj_id, 1/2) removed_ips = set(flow_state.keys()) - set(addr_to_conj.keys()) - for removed_ip in removed_ips: - conj_ids = flow_state[removed_ip] - self.delete_flow_for_ip(removed_ip, direction, ethertype, vlan_tag, - conj_ids) + for removed_ip, removed_mac in removed_ips: + conj_ids = flow_state[(removed_ip, removed_mac)] + self.delete_flow_for_ip( + removed_ip, removed_mac, direction, + ethertype, vlan_tag, conj_ids) if not cfg.CONF.AGENT.explicitly_egress_direct: return - for ip_addr in removed_ips: + for ip, mac in removed_ips: # Generate deletion template with bogus conj_id. - self.delete_flow_for_ip(ip_addr, direction, ethertype, vlan_tag, - [0]) + self.delete_flow_for_ip( + ip, mac, direction, ethertype, vlan_tag, [0]) - def delete_flow_for_ip(self, ip_address, direction, ethertype, + def delete_flow_for_ip(self, ip, mac, direction, ethertype, vlan_tag, conj_ids): - for flow in rules.create_flows_for_ip_address( - ip_address, direction, ethertype, vlan_tag, conj_ids): + for flow in rules.create_flows_for_ip_address_and_mac( + ip, mac, direction, ethertype, vlan_tag, conj_ids): # The following del statements are partly for # complying the OpenFlow spec. It forbids the use of # these field in non-strict delete flow messages, and diff --git a/neutron/agent/linux/openvswitch_firewall/rules.py b/neutron/agent/linux/openvswitch_firewall/rules.py index a195832ac26..7134aee34ad 100644 --- a/neutron/agent/linux/openvswitch_firewall/rules.py +++ b/neutron/agent/linux/openvswitch_firewall/rules.py @@ -294,12 +294,11 @@ def _flow_priority_offset_from_conj_id(conj_id): return conj_id % 8 // 2 -def create_flows_for_ip_address(ip_address, direction, ethertype, - vlan_tag, conj_ids): - """Create flows from a rule and an ip_address derived from - remote_group_id or remote_address_group_id +def create_flows_for_ip_address_and_mac(ip_address, mac_address, direction, + ethertype, vlan_tag, conj_ids): + """Create flows from a rule, ip, and mac addresses derived from + remote_group_id or remote_address_group_id. """ - ip_address, mac_address = ip_address net = netaddr.IPNetwork(str(ip_address)) any_src_ip = net.prefixlen == 0 diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py index 354ed82b800..ea0e6141798 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py @@ -506,7 +506,7 @@ class TestConjIPFlowManager(base.BaseTestCase): # "conj_id_to_remove" is populated with the remote_sg conj_id assigned, # "_update_flows_for_vlan_subr" will call "delete_flow_for_ip". self.driver.delete_flow_for_ip.assert_called_once_with( - ('10.22.3.4', 'ff:ee:dd:cc:bb:aa'), 'ingress', 'IPv4', 100, + '10.22.3.4', 'ff:ee:dd:cc:bb:aa', 'ingress', 'IPv4', 100, {self.conj_id}) @@ -1206,40 +1206,54 @@ class TestOVSFirewallDriver(base.BaseTestCase): vlan_tag = 'taaag' with mock.patch.object(self.firewall, 'delete_flow_for_ip') as \ mock_delete_flow_for_ip: - flow_state = {'addr1': {8, 16, 24}, 'addr2': {32, 40}} + flow_state = { + ('addr1', 'mac1'): {8, 16, 24}, + ('addr2', 'mac2'): {32, 40}, + } cfg.CONF.set_override('explicitly_egress_direct', explicitly_egress_direct, 'AGENT') self.firewall.delete_flows_for_flow_state( flow_state, addr_to_conj, direction, ethertype, vlan_tag) calls = [] - for removed_ip in flow_state.keys() - addr_to_conj.keys(): - calls.append(mock.call(removed_ip, direction, ethertype, vlan_tag, - flow_state[removed_ip])) + for removed_ip, removed_mac in flow_state.keys() - addr_to_conj.keys(): + calls.append(mock.call(removed_ip, removed_mac, direction, + ethertype, vlan_tag, + flow_state[(removed_ip, removed_mac)])) if explicitly_egress_direct: - calls.append(mock.call(removed_ip, direction, ethertype, - vlan_tag, [0])) - mock_delete_flow_for_ip.assert_has_calls(calls) + calls.append(mock.call(removed_ip, removed_mac, direction, + ethertype, vlan_tag, [0])) + mock_delete_flow_for_ip.assert_has_calls(calls, any_order=True) def test_delete_flows_for_flow_state_no_removed_ips_exp_egress(self): - addr_to_conj = {'addr1': {8, 16, 24}, 'addr2': {32, 40}} + addr_to_conj = { + ('addr1', 'mac1'): {8, 16, 24}, + ('addr2', 'mac2'): {32, 40}, + } self._test_delete_flows_for_flow_state(addr_to_conj) def test_delete_flows_for_flow_state_no_removed_ips_no_exp_egress(self): - addr_to_conj = {'addr1': {8, 16, 24}, 'addr2': {32, 40}} + addr_to_conj = { + ('addr1', 'mac1'): {8, 16, 24}, + ('addr2', 'mac2'): {32, 40}, + } self._test_delete_flows_for_flow_state(addr_to_conj, False) def test_delete_flows_for_flow_state_removed_ips_exp_egress(self): - addr_to_conj = {'addr2': {32, 40}} + addr_to_conj = { + ('mac2', 'addr2'): {32, 40}, + } self._test_delete_flows_for_flow_state(addr_to_conj) def test_delete_flows_for_flow_state_removed_ips_no_exp_egress(self): - addr_to_conj = {'addr1': {8, 16, 24}} + addr_to_conj = { + ('mac1', 'addr1'): {8, 16, 24}, + } self._test_delete_flows_for_flow_state(addr_to_conj, False) def test_delete_flow_for_ip_using_cookie_any(self): with mock.patch.object(self.firewall, '_delete_flows') as \ mock_delete_flows: - self.firewall.delete_flow_for_ip(('10.1.2.3', None), + self.firewall.delete_flow_for_ip('10.1.2.3', None, constants.INGRESS_DIRECTION, constants.IPv4, 100, [0]) _, kwargs = mock_delete_flows.call_args diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py index 88b64544a4d..a89a42c4161 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py @@ -348,8 +348,8 @@ class TestCreateFlowsForIpAddress(base.BaseTestCase): } conj_ids = [12, 20] - flows = rules.create_flows_for_ip_address( - ('192.168.0.1', 'fa:16:3e:aa:bb:cc'), + flows = rules.create_flows_for_ip_address_and_mac( + '192.168.0.1', 'fa:16:3e:aa:bb:cc', constants.EGRESS_DIRECTION, constants.IPv4, 0x123, conj_ids)