From b202eee7ce0134b32a894a3c2a2004105c329fe9 Mon Sep 17 00:00:00 2001 From: Nick Rogers Date: Tue, 19 May 2026 13:21:18 -0400 Subject: [PATCH] Allow warm reload for port and VLAN ACL configuration changes Adopts the per-ACL design @gizmoguy proposed in his 2026-05-01 review of #4733: for each port/VLAN whose only change is acls_in, iterate the old and new ACL lists and call del/add per-ACL with a cold-start priority decrement. remove_overlap_ofmsgs in valve_flowreorder cancels unchanged del+add pairs at flow-emit, so unchanged ACL rules don't churn on the wire. Inserting an ACL mid-list shifts subsequent priorities -- those re-emit (acceptable per gizmoguy's fallback in the linked review). dp.py - _get_vlan_config_changes returns a new changed_acl_vlans set; ACL-only VLAN changes are no longer folded into changed_vlans so they warm-reload instead of cold valve.py _apply_config_changes - Snapshot the old acls_in lists before dp_init swaps self.dp - Per-ACL del+add loop replaces the prior cold_start_port handling for changed_acl_ports, and adds the equivalent for changed_acl_vlans - Empty<->non-empty port transitions need the wildcard rule flipped, so fall back to cold_start_port valve_acl.py - add_port_acl / del_port_acl get a priority kwarg defaulting to self.auth_priority so dot1x callers are unchanged - build_acl_port_of_msgs derives allow/force_port_vlan instructions from the pipeline (matching cold-start build_acl_ofmsgs), so rules with force_port_vlan: 1 land in the right table; dot1x ACLs never carry force_port_vlan: 1 so its behavior is preserved - New add_vlan_acl / del_vlan_acl symmetric to the port helpers (the methods gizmoguy noted were missing for per-ACL VLAN reload) - del_port's flowdel is now priority-less so its flowmodkey differs from the acl_priority wildcard that add_port emits for a port with no acls_in; without this, remove_overlap_ofmsgs cancels the delete and old ACL flows stay on the switch (the @hieunt79 fix from #4733) Tests - FaucetConfigReloadVlanAclChangeTest and FaucetConfigReloadPortAclRemoveTest come from @hieunt79's #4733 (the latter exercises the bug gizmoguy reproduced reviewing #4733: removing one ACL from a port that has multiple ACLs must leave the others installed) - FaucetConfigReloadPortAclRemoveAllTest covers removing the last ACL from a port; without the del_port fix above this hits the remove_overlap_ofmsgs cancellation and the old flow stays stale. Both ports share the same ACL so removing it from one port doesn't change the overall pipeline match set (which would otherwise force cold-start for table reconfiguration) - ValveRemoveAllPortACLsTestCase covers the same scenario at unit level - test_vlan_acl_update now expects warm reload (was cold) - ValveChangeVLANACLTestCase.test_change_vlan_acl now expects warm Co-Authored-By: hieunt79 --- faucet/dp.py | 20 ++- faucet/valve.py | 54 ++++++- faucet/valve_acl.py | 54 +++++-- tests/integration/mininet_tests.py | 198 ++++++++++++++++++++++++- tests/unit/faucet/test_valve_config.py | 68 ++++++++- 5 files changed, 369 insertions(+), 25 deletions(-) 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):