[vpp][acl] add vpp acl IN_PORT support HLD - #2506
Merged
Merged
Conversation
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
There was a problem hiding this comment.
Pull request overview
Adds an HLD for enforcing per-entry VPP ACL ingress-port scoping.
Changes:
- Defines port-signature ACL partitioning and rebinding.
- Specifies failure handling, counters, and scaling.
- Documents proposed implementation and testing.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Placeholder default ACL is not literally unmatchable: it holds a deny of destination 0.0.0.0/32, and the reason it must deny rather than permit is now stated. - Failure handling: a port whose unbind failed keeps its old assignment, so the retry unbinds what VPP actually holds instead of stranding the stale ACL bound and undeletable. - Counters: REQ-7 holds in the steady state only; a partial failure under-reports until the next successful reprogram. - Scaling is driven by distinct signatures, not distinct IN_PORTS sets, and the default ACL is always additional. - Testing: add REQ-4 and REQ-7 cases; describe the GCU drop-removal test as partial coverage of REQ-4, which is what it is. Signed-off-by: Longxiang Lyu <lolv@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
A port whose unbind failed keeps its old swindex, but only a scoped ACL is carried under the empty signature. The default ACL is owned by m_acl_swindex_map for the table's lifetime; tracking it as scoped would expose it to the reap and leave that map holding a swindex VPP had released. Signed-off-by: Longxiang Lyu <lolv@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
Merged
5 tasks
The document described scoping as a binding-layer problem, on the premise that a VPP ACL rule has no interface field and so a per-entry qualifier has nowhere to live. That premise no longer holds: the implementation adds the field to the rule instead. The ingress interface was already in the per-packet 5-tuple, carried in lsb_of_sw_if_index for session handling and simply masked off during matching. Unmasking it is close to free at packet rate, which makes the binding-layer partitioning the previous revision specified, with its per-signature ACLs, whole-port rebinds, unbound window and dirty-port tracking, a large structural cost paid to avoid a small one. Rework the design around the match instead. saivpp fans a scoped entry out into one rule per named interface, the table keeps its single VPP ACL, and bindings are never touched, so the rebinding and reconciliation machinery goes away entirely. Keep the rejected alternative in the document as Option A, since the cardinality mismatch it addresses is still what motivates either approach. Also record what the implementation actually does: the three match paths that must agree, the 16-bit limit on the interface index and its config-time rejection, the fan-out failure cases, the tuplemerge folding caveat, the CRC coupling between VPP and sairedis, and the testbed results obtained so far. Signed-off-by: Longxiang Lyu <lolv@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
|
/azp run |
Contributor
Author
|
Pushed v0.2, which reworks the design rather than refining it. Flagging that here since anyone who read v0.1 will find the middle of the document unrecognisable. What changed and why. v0.1 argued that ingress scoping had to live in the binding layer, on the premise that a VPP ACL rule has no interface field and so a per-entry qualifier has nowhere to go. While implementing it I found the premise was wrong. The ingress interface is already in the per-packet 5-tuple, carried in That reversed the cost comparison. The binding-layer design was paying a large structural price, per-signature ACLs, whole-port unbind/rebind, a window where the port enforces nothing, and dirty-port tracking to recover from a partially applied rebind, to avoid what turned out to be a handful of lines of match logic. So the scope is now a match on the rule, saivpp fans a scoped entry out into one rule per named interface, and the table keeps its single VPP ACL with bindings untouched. All the rebinding and reconciliation machinery is gone. I kept the old approach in the document as Option A under Two Ways to Express the Scope, with the reasoning for choosing Option B. The cardinality mismatch it addresses is still what motivates either approach, and the reasons not to take it are worth having on record. Also now documented from the implementation: the three match paths that must agree (the fragment/linear path and the tuplemerge collision re-check are both load-bearing), the 16-bit limit on the interface index and its config-time rejection, the fan-out failure cases, the tuplemerge folding caveat for large fan-outs, and the CRC coupling that requires VPP and sairedis to merge together. Testing now separates what was verified on the testbed (rule counts tracking the standby set on two ToRs, bindings unchanged) from what is still outstanding (forwarding under mux toggles). Implementation: sonic-net/sonic-platform-vpp#278 and sonic-net/sonic-sairedis#2064. |
|
No pipelines are associated with this pull request. |
10 tasks
yue-fred-gao
reviewed
Sep 1, 2026
Egress stage (yue-fred-gao, twice). The 5-tuple slot this design matches on is filled from sw_if_index[VLIB_TX] when an ACL is bound outbound, so an IN_PORTS-scoped entry in an egress table would have matched the egress port while still reporting the ingress port the operator named. SAI does not restrict the qualifier to ingress and saivpp binds egress groups for real, so the combination is expressible. Document the mechanism in a new "Why This Is Ingress-Only" section and reject the combination at programming time, consistent with how an out-of-range index is handled. Replace the vague "egress scoping is not addressed" restriction with two precise ones, and add a test case. sw_if_index recycling. Explain that sw_if_index is a pool index, freed on delete and reused LIFO from the free list, so the 16-bit ceiling is bounded by concurrent interface count rather than cumulative churn. Also state plainly that a rule's index is resolved once and never invalidated, and that this is safe only because IN_PORTS can name nothing but front-panel ports. Terminology. "named" was being used for the new per-rule match while "bound" meant the unchanged VPP attachment, and the Cases table read as if IN_PORTS drove binding. Define both terms and reword the affected rows. Also refresh the patch filename to 0019 after the policy-drop split, and record that whoever merges second must hand-bump VPP_VERSION, since git auto-resolves an identical version edit without flagging it. Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
Failing the entry is not survivable in saivpp: createAclEntry() registers it in m_objectHash and m_acl_tbl_rules_map before AclAddRemoveCheck() runs and never rolls back, orchagent does not remove a rule whose create failed, and on its retry create_internal() returns ITEM_ALREADY_EXISTS, which AclRule::create() treats as success. The table would be left permanently unprogrammable while orchagent reported the rule ACTIVE - and since IN_PORTS was previously ignored outright, that is a regression for a configuration that programs today. Document emitting no rule for the entry instead, matching how an enabled-but-empty IN_PORTS list is already handled, and record that the combination is reachable by configuration alone rather than needing a code change. Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
lolyu
added a commit
to sonic-net/sonic-sairedis
that referenced
this pull request
Sep 10, 2026
) Approach What is the motivation for this PR? Fixes: sonic-net/sonic-buildimage#29077 IN_PORTS is how orchagent scopes an ACL entry to a subset of the ports its table is bound to. aclorch restricts it to physical interfaces, and the dual ToR standby DENY entries rely on it. Silently widening those entries to the whole table is worse than not supporting the field at all, because nothing in the logs distinguishes a rule that was scoped from one that was not. Work item tracking Microsoft ADO (number only): 39466368 How did you do it? A VPP ACL rule matches one interface, so a SAI entry naming N ports becomes N VPP rules: acl_entry_in_ports_get() finds the attribute and resolves it. The cached attribute only carries the list count, its list pointer is still null, so the entry is read again with a buffer of the right size. Each port OID is then resolved to a VPP interface name via vpp_get_hwif_name(). fill_acl_rules() pops the rules it just generated for the entry and re-adds one copy per resolved interface. This is the same shape as the existing fan-out where a port based entry with no protocol becomes a UDP rule and a TCP rule. vpp_acl_rule_t gains in_hwif_name. The SAI layer keeps working in interface names, as it already does for binding, and vpp_acl_add_replace() resolves the name to a sw_if_index when it builds the API message. Empty means any. SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS becomes an explicit no-op in acl_rule_field_update(), since it is not a per rule field. The fanned out rules stay contiguous from ace.vpp_rule_base_index, so num_rules still describes the entry and the counter aggregation in getAclEntryStats() sums over them unchanged. Failure handling is deliberate, since three different things can leave the interface set empty and they do not mean the same thing: Case Behaviour Field absent or not enabled Not scoped, one rule as before Enabled but names no port Scoped to nothing, no rule emitted Port list unreadable, or no port resolves Error, ACL programming fails The last row matters. On failure the scope of the entry is simply unknown, and installing it unscoped would re-introduce exactly the blackhole this PR is fixing, so fill_acl_rules() fails and CHECK_STATUS_ACLTBLCONFIG leaves the previous ACL in place. The empty list case is left as "matches nothing" on purpose: no ingress interface can be a member of an empty list, so emitting no rule is the faithful translation. How did you verify/test it? Built into docker-syncd-vpp and deployed to a vms-kvm-dual-vpp-t0-1 dual ToR testbed, then checked the programmed rules against the mux state: vlab-vpp-03: 18 standby ports, 18 matching ingress scoped deny rules. vlab-vpp-04: 6 standby ports, 6 matching ingress scoped deny rules. ACL 3 stayed bound to all 32 interfaces in both cases, confirming the scoping comes from the rules and not from the binding. Rule counts tracking the standby set on two ToRs with different mux distributions is the behaviour this PR is after, and is what regressed before. Still outstanding: live downstream forwarding under mux toggles (standby → active → standby) has not been run yet, so the dataplane effect is verified by rule inspection rather than by packets. Any platform specific information? VPP only. This depends on sonic-net/sonic-platform-vpp#278 and the two have to merge together. Adding the field changes the acl_add_replace message, and therefore its CRC, so a syncd built against one VPP version cannot program ACLs on the other. That PR also bumps VPP_VERSION to 2606-0.6. A scale note for reviewers: an entry naming N ports costs N rules, so a table bound to many mux ports grows proportionally. VPP's tuplemerge can additionally fold the fanned out copies into one less specific mask type, and split_partition() has no interface dimension to split them back apart on, so they can end up on a single collision chain. Correctness is unaffected, single_rule_match_5tuple() in the platform PR re-checks the interface on the collision path, but it is worth knowing before scaling the port count up. Documentation Design is written up in sonic-net/SONiC#2506.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add VPP ACL
IN_PORTsupport.Signed-off-by: Longxiang Lyu lolv@microsoft.com