diff --git a/faucet/dp.py b/faucet/dp.py index 697a579583..1b7ef71c5c 100644 --- a/faucet/dp.py +++ b/faucet/dp.py @@ -1485,6 +1485,7 @@ def _get_vlan_config_changes(self, logger, new_dp, changed_acls): changes (tuple) of: deleted_vlans (set): deleted VLAN IDs. changed_vlans (set): changed/added VLAN IDs. + changed_acl_vlans (dict): {vid: (old_acls, new_acls)}. """ ( _, @@ -1502,15 +1503,17 @@ def _get_vlan_config_changes(self, logger, new_dp, changed_acls): ignore_keys=frozenset(["acls_in"]), ) changed_vlans = added_vlans.union(changed_vlans) - # TODO: optimize for warm start. + changed_acl_vlans = set() for vlan_id in same_vlans: old_vlan = self.vlans[vlan_id] new_vlan = new_dp.vlans[vlan_id] if self._acl_ref_changes( "VLAN %u" % vlan_id, old_vlan, new_vlan, changed_acls, logger ): - changed_vlans.add(vlan_id) - return (deleted_vlans, changed_vlans) + changed_acl_vlans.add(vlan_id) + if changed_acl_vlans: + logger.info("VLANs where ACL only changed: %s" % changed_acl_vlans) + return (deleted_vlans, changed_vlans, changed_acl_vlans) def _acl_ref_changes(self, conf_desc, old_conf, new_conf, changed_acls, logger): changed = False @@ -1719,6 +1722,7 @@ def get_config_changes(self, logger, new_dp): deleted_meters (set): deleted meter numbers added_meters (set): Added meter numbers changed_meters (set): changed/added meter numbers + changed_acl_vlans (set): changed ACL only VLAN IDs. """ if ( new_dp.stack @@ -1736,9 +1740,11 @@ def get_config_changes(self, logger, new_dp): ) else: changed_acls = self._get_acl_config_changes(logger, new_dp) - deleted_vlans, changed_vlans = self._get_vlan_config_changes( - logger, new_dp, changed_acls - ) + ( + deleted_vlans, + changed_vlans, + changed_acl_vlans, + ) = self._get_vlan_config_changes(logger, new_dp, changed_acls) ( all_meters_changed, deleted_meters, @@ -1767,6 +1773,7 @@ def get_config_changes(self, logger, new_dp): deleted_meters, added_meters, changed_meters, + changed_acl_vlans, ) # default cold start return ( @@ -1781,6 +1788,7 @@ def get_config_changes(self, logger, new_dp): set(), set(), set(), + set(), ) def get_tables(self): diff --git a/faucet/valve.py b/faucet/valve.py index bf0d8833cd..614b805ce2 100644 --- a/faucet/valve.py +++ b/faucet/valve.py @@ -1586,6 +1586,7 @@ def _apply_config_changes(self, new_dp, changes, valves=None): deleted_meters: (set): deleted meter numbers. changed_meters: (set): changed meter numbers. added_meters: (set): added meter numbers. + changed_acl_vlans (set): changed ACL only VLAN IDs. valves (list): List of other running valves Returns: tuple: @@ -1604,6 +1605,7 @@ def _apply_config_changes(self, new_dp, changes, valves=None): deleted_meters, added_meters, changed_meters, + changed_acl_vlans, ) = changes restart_type = "cold" ofmsgs = [] @@ -1629,6 +1631,14 @@ def _apply_config_changes(self, new_dp, changes, valves=None): self.dp_init(new_dp) return restart_type, ofmsgs + # Snapshot OLD acls_in lists before dp_init swaps self.dp. + old_port_acls = { + p: list(self.dp.ports[p].acls_in or []) for p in changed_acl_ports + } + old_vlan_acls = { + v: list(self.dp.vlans[v].acls_in or []) for v in changed_acl_vlans + } + if deleted_ports: ofmsgs.extend(self.ports_delete(deleted_ports)) if changed_ports: @@ -1665,10 +1675,48 @@ def _apply_config_changes(self, new_dp, changes, valves=None): port for port in changed_ports if port in self.dp.dyn_up_port_nos ] ofmsgs.extend(self.ports_add(all_up_port_nos)) - if self.acl_manager and changed_acl_ports: + # Each port/VLAN: del each old ACL + add each new ACL at the same + # priorities cold-start install would use, so remove_overlap_ofmsgs + # cancels the del+add pairs for unchanged ACL rules. Ports gaining + # or losing all ACLs flip the wildcard "in_port -> goto vlan" rule + # add_port installs, so they take cold_start_port instead. + if self.acl_manager: for port_num in changed_acl_ports: - port = self.dp.ports[port_num] - ofmsgs.extend(self.acl_manager.cold_start_port(port)) + old = old_port_acls[port_num] + new = list(self.dp.ports[port_num].acls_in or []) + if not old or not new: + ofmsgs.extend( + self.acl_manager.cold_start_port(self.dp.ports[port_num]) + ) + continue + priority = self.acl_manager.acl_priority + for acl in old: + ofmsgs.extend( + self.acl_manager.del_port_acl(acl, port_num, priority=priority) + ) + priority -= len(acl.rules) + priority = self.acl_manager.acl_priority + for acl in new: + ofmsgs.extend( + self.acl_manager.add_port_acl(acl, port_num, priority=priority) + ) + priority -= len(acl.rules) + for vid in changed_acl_vlans: + vlan = self.dp.vlans[vid] + old = old_vlan_acls[vid] + new = list(vlan.acls_in or []) + priority = self.acl_manager.acl_priority + for acl in old: + ofmsgs.extend( + self.acl_manager.del_vlan_acl(acl, vlan, priority=priority) + ) + priority -= len(acl.rules) + priority = self.acl_manager.acl_priority + for acl in new: + ofmsgs.extend( + self.acl_manager.add_vlan_acl(acl, vlan, priority=priority) + ) + priority -= len(acl.rules) if changed_vids: changed_vlans = [self.dp.vlans[vid] for vid in changed_vids] # TODO: handle change versus add separately so can avoid delete first. diff --git a/faucet/valve_acl.py b/faucet/valve_acl.py index df25e19bf1..c9c35c0bf0 100644 --- a/faucet/valve_acl.py +++ b/faucet/valve_acl.py @@ -402,15 +402,15 @@ def build_acl_ofmsgs( return ofmsgs -def build_acl_port_of_msgs(acl, vid, port_num, acl_table, goto_table, priority): +def build_acl_port_of_msgs(acl, vid, port_num, acl_table, pipeline, priority): """A Helper function for building Openflow Mod Messages for Port ACLs""" ofmsgs = None if acl.rules: ofmsgs = build_acl_ofmsgs( [acl], acl_table, - [valve_of.goto_table(goto_table)], - [valve_of.goto_table(goto_table)], + pipeline.accept_to_vlan(), + pipeline.accept_to_l2_forwarding(), priority, acl.meter, acl.exact_match, @@ -511,7 +511,10 @@ def del_port(self, port): ofmsgs = [] if self._port_acls_allowed(port): in_port_match = self.port_acl_table.match(in_port=port.number) - ofmsgs.append(self.port_acl_table.flowdel(in_port_match, self.acl_priority)) + # Priority-less so the flowmodkey differs from the acl_priority + # wildcard add_port emits when acls_in is empty -- otherwise + # remove_overlap_ofmsgs would cancel this delete. + ofmsgs.append(self.port_acl_table.flowdel(in_port_match)) return ofmsgs def cold_start_port(self, port): @@ -599,7 +602,7 @@ def del_authed_mac(self, port_num, mac=None, strict=True): ) ] - def del_port_acl(self, acl, port_num, mac=None): + def del_port_acl(self, acl, port_num, mac=None, priority=None): """Delete ACL rules for Port""" def convert_to_flow_del(ofp_flowmods): @@ -613,30 +616,32 @@ def convert_to_flow_del(ofp_flowmods): return flowdels - pipeline_vlan_table = self.pipeline.vlan_table + if priority is None: + priority = self.auth_priority flowmods = build_acl_port_of_msgs( acl, None, port_num, self.port_acl_table, - pipeline_vlan_table, - self.auth_priority, + self.pipeline, + priority, ) for flow in flowmods: flow.match = add_mac_address_to_match(flow.match, mac) return convert_to_flow_del(flowmods) - def add_port_acl(self, acl, port_num, mac=None): + def add_port_acl(self, acl, port_num, mac=None, priority=None): """Create ACL openflow rules for Port""" - pipeline_vlan_table = self.pipeline.vlan_table + if priority is None: + priority = self.auth_priority flowmods = build_acl_port_of_msgs( acl, None, port_num, self.port_acl_table, - pipeline_vlan_table, - self.auth_priority, + self.pipeline, + priority, ) for flow in flowmods: @@ -644,6 +649,31 @@ def add_port_acl(self, acl, port_num, mac=None): return flowmods + def add_vlan_acl(self, acl, vlan, priority=None): + """Create ACL openflow rules for a single VLAN ACL.""" + if not acl.rules: + return [] + if priority is None: + priority = self.acl_priority + return build_acl_ofmsgs( + [acl], + self.vlan_acl_table, + self.pipeline.accept_to_classification(), + self.pipeline.accept_to_l2_forwarding(), + priority, + acl.meter, + acl.exact_match, + vlan_vid=vlan.vid, + ) + + def del_vlan_acl(self, acl, vlan, priority=None): + """Delete ACL rules for a single VLAN ACL.""" + flowmods = self.add_vlan_acl(acl, vlan, priority=priority) + return [ + self.vlan_acl_table.flowdel(match=fm.match, priority=fm.priority) + for fm in flowmods + ] + def create_dot1x_flow_pair(self, port_num, nfv_sw_port_num, mac): """Create dot1x flow pair""" ofmsgs = [ diff --git a/tests/integration/mininet_tests.py b/tests/integration/mininet_tests.py index 63f917f504..7af244db3e 100644 --- a/tests/integration/mininet_tests.py +++ b/tests/integration/mininet_tests.py @@ -3297,7 +3297,7 @@ def test_vlan_acl_update(self): new_yaml_acl_conf, self.acl_config_file, # pytype: disable=attribute-error restart=True, - cold_start=True, + cold_start=False, ) self.wait_until_matching_flow({"dl_type": 0x800}, table_id=self._VLAN_ACL_TABLE) self.wait_until_matching_flow({"dl_type": 0x806}, table_id=self._VLAN_ACL_TABLE) @@ -3307,7 +3307,7 @@ def test_vlan_acl_update(self): orig_yaml_acl_conf, self.acl_config_file, # pytype: disable=attribute-error restart=True, - cold_start=True, + cold_start=False, ) self.wait_until_matching_flow({"dl_type": 0x800}, table_id=self._VLAN_ACL_TABLE) self.wait_until_no_matching_flow( @@ -3864,6 +3864,200 @@ def test_port_change_vlan(self): self.assertLess(len(self.scrape_prometheus(var="learned_l2_port")), 4) +class FaucetConfigReloadVlanAclChangeTest(FaucetConfigReloadTestBase): + """Warm-reload of a VLAN ACL: changing one rule inside an ACL + referenced by a VLAN should keep the unchanged rule's flow and + install the new rule's flow.""" + + CONFIG_GLOBAL = """ +vlans: + 100: + description: "office network" + acls_in: [vlan-block-icmp] +""" + ACL = """ +acls: + vlan-block-icmp: + - rule: + dl_type: 0x800 + ip_proto: 1 + actions: + allow: 0 + - rule: + actions: + allow: 1 +""" + CONFIG = """ + interfaces: + %(port_1)d: + native_vlan: 100 + %(port_2)d: + native_vlan: 100 + %(port_3)d: + native_vlan: 100 + %(port_4)d: + native_vlan: 100 +""" + + NEW_ACL = """ +acls: + vlan-block-icmp: + - rule: + dl_type: 0x800 + ip_proto: 1 + actions: + allow: 0 + - rule: + dl_type: 0x806 + actions: + allow: 0 + - rule: + actions: + allow: 1 +""" + + def test_vlan_acl_warm_reload(self): + self.wait_until_matching_flow( + {"dl_type": 0x800, "ip_proto": 1}, + table_id=self._VLAN_ACL_TABLE, + ) + + new_yaml_acl_conf = yaml_load(self.NEW_ACL) + self.reload_conf( + new_yaml_acl_conf, + self.acl_config_file, # pytype: disable=attribute-error + restart=True, + cold_start=False, + ) + + self.wait_until_matching_flow( + {"dl_type": 0x800, "ip_proto": 1}, + table_id=self._VLAN_ACL_TABLE, + ) + self.wait_until_matching_flow( + {"dl_type": 0x806}, + table_id=self._VLAN_ACL_TABLE, + ) + + +class FaucetConfigReloadPortAclRemoveTest(FaucetConfigReloadTestBase): + CONFIG_GLOBAL = """ +vlans: + 100: + description: "office network" +""" + ACL = """ +acls: + block-ssl: + - rule: + dl_type: 0x800 + ip_proto: 6 + tcp_dst: 443 + actions: + allow: 0 + block-http: + - rule: + dl_type: 0x800 + ip_proto: 6 + tcp_dst: 80 + actions: + allow: 0 + block-ping: + - rule: + dl_type: 0x800 + ip_proto: 1 + actions: + allow: 0 +""" + CONFIG = """ + interfaces: + %(port_1)d: + native_vlan: 100 + acls_in: [block-ping] + %(port_2)d: + native_vlan: 100 + acls_in: [block-ping, block-http, block-ssl] + %(port_3)d: + native_vlan: 100 +""" + + def test_remove_port_acl(self): + hup = not self.STAT_RELOAD + in_port_2_match = { + "in_port": int(self.port_map["port_2"]), + "eth_type": 0x800, + "ip_proto": 1, + } + self.wait_until_matching_flow(in_port_2_match, table_id=self._PORT_ACL_TABLE) + + self.change_port_config( + self.port_map["port_2"], + "acls_in", + ["block-http", "block-ssl"], + restart=True, + cold_start=False, + hup=hup, + ) + + self.wait_until_no_matching_flow( + in_port_2_match, table_id=self._PORT_ACL_TABLE, timeout=30 + ) + + +class FaucetConfigReloadPortAclRemoveAllTest(FaucetConfigReloadTestBase): + """Removing the last ACL from a port must wipe its flows, not leave + them stale behind the wildcard rule that add_port installs in their + place.""" + + CONFIG_GLOBAL = """ +vlans: + 100: + description: "office network" +""" + ACL = """ +acls: + block-ping: + - rule: + dl_type: 0x800 + ip_proto: 1 + actions: + allow: 0 +""" + CONFIG = """ + interfaces: + %(port_1)d: + native_vlan: 100 + acls_in: [block-ping] + %(port_2)d: + native_vlan: 100 + acls_in: [block-ping] + %(port_3)d: + native_vlan: 100 +""" + + def test_remove_all_port_acls(self): + hup = not self.STAT_RELOAD + in_port_1_match = { + "in_port": int(self.port_map["port_1"]), + "eth_type": 0x800, + "ip_proto": 1, + } + self.wait_until_matching_flow(in_port_1_match, table_id=self._PORT_ACL_TABLE) + + self.change_port_config( + self.port_map["port_1"], + "acls_in", + [], + restart=True, + cold_start=False, + hup=hup, + ) + + self.wait_until_no_matching_flow( + in_port_1_match, table_id=self._PORT_ACL_TABLE, timeout=30 + ) + + class FaucetConfigReloadEmptyAclTest(FaucetConfigReloadTestBase): CONFIG = """ interfaces: diff --git a/tests/unit/faucet/test_valve_config.py b/tests/unit/faucet/test_valve_config.py index 81ff0ad86f..808c37f3e8 100755 --- a/tests/unit/faucet/test_valve_config.py +++ b/tests/unit/faucet/test_valve_config.py @@ -185,8 +185,72 @@ def setUp(self): self.setup_valves(self.CONFIG) def test_change_vlan_acl(self): - """Test vlan ACL change is detected.""" - self.update_and_revert_config(self.CONFIG, self.MORE_CONFIG, "cold") + """Test vlan ACL change warm-reloads (per-ACL del+add).""" + self.update_and_revert_config(self.CONFIG, self.MORE_CONFIG, "warm") + + +class ValveRemoveAllPortACLsTestCase(ValveTestBases.ValveTestNetwork): + """Removing the last ACL from a port that shares its ACL with + another port must warm-reload, not strand the old ACL flows.""" + + CONFIG = ( + """ +acls: + block-ping: + - rule: + eth_type: 0x0800 + ip_proto: 1 + actions: + allow: 0 +vlans: + vlan1: + vid: 10 +dps: + s1: +%s + interfaces: + 1: + native_vlan: vlan1 + acls_in: [block-ping] + 2: + native_vlan: vlan1 + acls_in: [block-ping] +""" + % DP1_CONFIG + ) + + NEW_CONFIG = ( + """ +acls: + block-ping: + - rule: + eth_type: 0x0800 + ip_proto: 1 + actions: + allow: 0 +vlans: + vlan1: + vid: 10 +dps: + s1: +%s + interfaces: + 1: + native_vlan: vlan1 + 2: + native_vlan: vlan1 + acls_in: [block-ping] +""" + % DP1_CONFIG + ) + + def setUp(self): + """Setup basic port and vlan config""" + self.setup_valves(self.CONFIG) + + def test_remove_all_port_acls(self): + """Test removing the last ACL from a port warm-reloads.""" + self.update_and_revert_config(self.CONFIG, self.NEW_CONFIG, "warm") class ValveChangePortTestCase(ValveTestBases.ValveTestNetwork):