Skip to content

Fix deleting final ACL from a port#4815

Open
gizmoguy wants to merge 2 commits into
faucetsdn:mainfrom
gizmoguy:del-port-acl
Open

Fix deleting final ACL from a port#4815
gizmoguy wants to merge 2 commits into
faucetsdn:mainfrom
gizmoguy:del-port-acl

Conversation

@gizmoguy

@gizmoguy gizmoguy commented Jun 4, 2026

Copy link
Copy Markdown
Member

While reviewing #4733 a bug was discovered when deleting the final ACL from a port and warm reloading faucet. The bug caused the flow rules for the deleted ACL to remain on the datapath after the warm reload.

When the ACL configuration for a port changes, ValveAclManager.cold_start_port() will be called which sends a delete flowmod followed by the new ACL flowmods. In the case the final ACL is removed an implicit allow ACL rule is added, on the wire this looks like this:

OFPFlowMod(buffer_id=4294967295,command=3,cookie=1524372928,cookie_mask=0,flags=0,hard_timeout=0,idle_timeout=0,instructions=[],match=OFPMatch(oxm_fields={'in_port': 2}),out_group=4294967295,out_port=4294967295,priority=20480,table_id=0)
OFPFlowMod(buffer_id=4294967295,command=0,cookie=1524372928,cookie_mask=0,flags=0,hard_timeout=0,idle_timeout=0,instructions=(OFPInstructionGotoTable(len=8,table_id=1,type=1),),match=OFPMatch(oxm_fields={'in_port': 2}),out_group=0,out_port=0,priority=20480,table_id=0)

When flowmods are pushed to the dp, overlapping delete/adds that share match/cookie/priority/table_id (see: ece834d) are suppressed by remove_overlap_ofmsgs(). Because these two flowmods share the same value for all these fields the port cold start only partially occurs, as the implicit allow rule is added but the old flow rules are not deleted because the flowdel is suppressed and not actually sent to the dp.

To fix this issue, the priority is removed from the port ACL flowdel, so that flow rules will be correctly deleted and re-added.

I also found during testing that modifying an existing ACL worked fine and did not experience this bug because the key that was used to compare flowmods for overlap included an OFPMatch object (which lacks ___eq___ or ___hash___) so will only ever be equal with itself. Replacing this with a tuple of the flow match key/values makes the flow overlap suppression work in all cases.

@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.42%. Comparing base (076889d) to head (853dc3d).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4815   +/-   ##
=======================================
  Coverage   91.42%   91.42%           
=======================================
  Files          46       46           
  Lines        8923     8923           
=======================================
  Hits         8157     8157           
  Misses        766      766           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant