(11) feat(acl): crate scaffolding + software reference backend#1575
Merged
Conversation
This was referenced May 31, 2026
daniel-noland
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new dataplane-acl workspace crate that defines the ACL public API surface and implements a pure-software “reference/oracle” backend (typed and dynamic) behind the lookup::Lookup interface.
Changes:
- Add
aclas a workspace member and workspace dependency, plus package metadata for miri/wasm selection. - Introduce
dataplane-aclcrate scaffolding with strict linting and areferencemodule. - Implement
ReferenceTable(typedMatchKey) andDynReferenceTable(runtimeFieldSpec) linear-scan classifiers with unit tests.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Adds acl to workspace + workspace deps; introduces miri/wasm metadata entry. |
Cargo.lock |
Locks the new dataplane-acl crate and its dependencies. |
acl/Cargo.toml |
Defines the new dataplane-acl package and dependencies. |
acl/src/lib.rs |
Crate-level lint policy and top-level documentation; exports reference module. |
acl/src/reference/mod.rs |
Wires up reference backend modules + re-exports. |
acl/src/reference/table.rs |
Implements typed ReferenceTable/RefRule and Lookup<K, A> integration. |
acl/src/reference/dyn_table.rs |
Implements runtime-shaped DynReferenceTable with validation and byte-slice lookup. |
Comment on lines
+29
to
+35
| pub(crate) fn matches_packed(&self, specs: &[FieldSpec], buf: &[u8]) -> bool { | ||
| debug_assert_eq!(self.fields.len(), specs.len()); | ||
| self.fields | ||
| .iter() | ||
| .zip(specs) | ||
| .all(|(pred, spec)| pred.matches(&buf[spec.offset..spec.offset + spec.size])) | ||
| } |
Comment on lines
+66
to
+73
| fn pack(key: &K) -> Option<[u8; MAX_KEY_BYTES]> { | ||
| if K::KEY_SIZE > MAX_KEY_BYTES { | ||
| return None; | ||
| } | ||
| let mut buf = [0u8; MAX_KEY_BYTES]; | ||
| key.as_key_into(&mut buf[..K::KEY_SIZE]); | ||
| Some(buf) | ||
| } |
Comment on lines
+88
to
+103
| #[must_use] | ||
| pub fn lookup_bytes(&self, key: &[u8]) -> Option<&A> { | ||
| assert_eq!(key.len(), self.key_size, "key length must equal key_size"); | ||
| self.rules | ||
| .iter() | ||
| .find(|rule| rule.matches_packed(&self.specs, key)) | ||
| .map(RefRule::action) | ||
| } | ||
| #[must_use] | ||
| pub fn matches_bytes(&self, key: &[u8]) -> Vec<&RefRule<A>> { | ||
| assert_eq!(key.len(), self.key_size, "key length must equal key_size"); | ||
| self.rules | ||
| .iter() | ||
| .filter(|rule| rule.matches_packed(&self.specs, key)) | ||
| .collect() | ||
| } |
Comment on lines
+265
to
+272
| [workspace.metadata.package.acl] | ||
| package = "dataplane-acl" | ||
| # Default features enable the DPDK `rte_acl` backend, which pulls in | ||
| # `dpdk-sys` (bindgen against the system DPDK headers). miri can't | ||
| # build that path on the cross target, and the reference backend's | ||
| # unit tests run fine outside the miri profile. | ||
| miri = false # hopeless + pointless | ||
| wasm = false # hopeless + pointless |
Comment on lines
+17
to
+18
| //! - [`reference`](mod@reference): linear-scan software classifier; | ||
| //! differential oracle and a mutable cascade front. Always built. |
daniel-noland
force-pushed
the
pr/daniel-noland/match-action
branch
from
c071ee9 to
7d5e7ad
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/acl-reference
branch
from
3220e66 to
710b08b
Compare
Base automatically changed from
pr/daniel-noland/match-action
to
pr/daniel-noland/dpdk-test-eal
June 5, 2026 06:21
daniel-noland
force-pushed
the
pr/daniel-noland/dpdk-test-eal
branch
4 times, most recently
from
df2ad93 to
3741795
Compare
mvachhar
approved these changes
Jun 10, 2026
mvachhar
left a comment
mvachhar
left a comment
Contributor
There was a problem hiding this comment.
I feel like I have reviewed a bunch of this before, perhaps a prior branch got merged onto this one? In any case, the stuff I don't remember looks good to go. I'm a bit torn on the dataplane-acl name since there is a user facing feature for that, which may ore may not end up in this crate (probably not). But we can always rename it if we want.
daniel-noland
force-pushed
the
pr/daniel-noland/acl-reference
branch
from
710b08b to
598ba60
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughA new Changesdataplane-acl crate introduction
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@acl/src/reference/dyn_table.rs`:
- Around line 89-99: The lookup_bytes and matches_bytes methods use assert_eq!
to validate key length at runtime, which will panic if the key length doesn't
match key_size. For public APIs accepting runtime byte inputs, panicking is not
appropriate. Replace the assert_eq! statements in both methods with graceful
error handling: in lookup_bytes, return None when the key length doesn't match
the expected key_size; in matches_bytes, return an empty Vec when the key length
doesn't match. This ensures the code fails closed without crashing on invalid
input.
In `@acl/src/reference/table.rs`:
- Around line 66-73: The pack function in table.rs silently returns None when
K::KEY_SIZE exceeds MAX_KEY_BYTES, causing every lookup to fail
deterministically without distinguishing between a legitimate "no match" and an
invalid key size. Replace this silent failure with a compile-time assertion or
validation mechanism that ensures KEY_SIZE is validated upfront (such as using
const assertions within the trait implementation or by panicking if the
condition is violated at runtime), so that oversized keys are caught early as a
logic error rather than causing cryptic lookup failures.
- Around line 29-35: The matches_packed function uses debug_assert_eq to check
that fields and specs have matching lengths, but this assertion is compiled away
in release builds. When zip silently truncates in release mode due to a shape
mismatch, missing predicates implicitly become wildcards, unexpectedly widening
ACL matches. Replace the debug_assert_eq on line 30 with an explicit runtime
check that returns false immediately if self.fields.len() does not equal
specs.len(), ensuring shape mismatches are properly rejected in both debug and
release builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7bd1fbb-5744-48d6-aed3-aeca13a25166
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlacl/Cargo.tomlacl/src/lib.rsacl/src/reference/dyn_table.rsacl/src/reference/mod.rsacl/src/reference/table.rs
Comment on lines
+89
to
+99
| pub fn lookup_bytes(&self, key: &[u8]) -> Option<&A> { | ||
| assert_eq!(key.len(), self.key_size, "key length must equal key_size"); | ||
| self.rules | ||
| .iter() | ||
| .find(|rule| rule.matches_packed(&self.specs, key)) | ||
| .map(RefRule::action) | ||
| } | ||
| #[must_use] | ||
| pub fn matches_bytes(&self, key: &[u8]) -> Vec<&RefRule<A>> { | ||
| assert_eq!(key.len(), self.key_size, "key length must equal key_size"); | ||
| self.rules |
There was a problem hiding this comment.
Do not panic on invalid key length in public byte lookup APIs.
Line 90 and Line 98 panic on length mismatch. For runtime byte inputs, this should fail closed without crashing.
As per coding guidelines, “Find logic errors in the code under review. If confident that code is incorrect, suggest a fix.”
Proposed fix
pub fn lookup_bytes(&self, key: &[u8]) -> Option<&A> {
- assert_eq!(key.len(), self.key_size, "key length must equal key_size");
+ if key.len() != self.key_size {
+ return None;
+ }
self.rules
.iter()
.find(|rule| rule.matches_packed(&self.specs, key))
.map(RefRule::action)
}
@@
pub fn matches_bytes(&self, key: &[u8]) -> Vec<&RefRule<A>> {
- assert_eq!(key.len(), self.key_size, "key length must equal key_size");
+ if key.len() != self.key_size {
+ return Vec::new();
+ }
self.rules
.iter()
.filter(|rule| rule.matches_packed(&self.specs, key))
.collect()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn lookup_bytes(&self, key: &[u8]) -> Option<&A> { | |
| assert_eq!(key.len(), self.key_size, "key length must equal key_size"); | |
| self.rules | |
| .iter() | |
| .find(|rule| rule.matches_packed(&self.specs, key)) | |
| .map(RefRule::action) | |
| } | |
| #[must_use] | |
| pub fn matches_bytes(&self, key: &[u8]) -> Vec<&RefRule<A>> { | |
| assert_eq!(key.len(), self.key_size, "key length must equal key_size"); | |
| self.rules | |
| pub fn lookup_bytes(&self, key: &[u8]) -> Option<&A> { | |
| if key.len() != self.key_size { | |
| return None; | |
| } | |
| self.rules | |
| .iter() | |
| .find(|rule| rule.matches_packed(&self.specs, key)) | |
| .map(RefRule::action) | |
| } | |
| #[must_use] | |
| pub fn matches_bytes(&self, key: &[u8]) -> Vec<&RefRule<A>> { | |
| if key.len() != self.key_size { | |
| return Vec::new(); | |
| } | |
| self.rules |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@acl/src/reference/dyn_table.rs` around lines 89 - 99, The lookup_bytes and
matches_bytes methods use assert_eq! to validate key length at runtime, which
will panic if the key length doesn't match key_size. For public APIs accepting
runtime byte inputs, panicking is not appropriate. Replace the assert_eq!
statements in both methods with graceful error handling: in lookup_bytes, return
None when the key length doesn't match the expected key_size; in matches_bytes,
return an empty Vec when the key length doesn't match. This ensures the code
fails closed without crashing on invalid input.
Source: Coding guidelines
Comment on lines
+29
to
+35
| pub(crate) fn matches_packed(&self, specs: &[FieldSpec], buf: &[u8]) -> bool { | ||
| debug_assert_eq!(self.fields.len(), specs.len()); | ||
| self.fields | ||
| .iter() | ||
| .zip(specs) | ||
| .all(|(pred, spec)| pred.matches(&buf[spec.offset..spec.offset + spec.size])) | ||
| } |
There was a problem hiding this comment.
Reject shape-mismatched rules during matching.
Line 30 only checks rule/spec arity in debug builds. In release, Line 33 truncates with zip, so missing predicates become implicit wildcards and can widen ACL matches unexpectedly.
As per coding guidelines, “Find logic errors in the code under review. If confident that code is incorrect, suggest a fix.”
Proposed fix
pub(crate) fn matches_packed(&self, specs: &[FieldSpec], buf: &[u8]) -> bool {
- debug_assert_eq!(self.fields.len(), specs.len());
+ if self.fields.len() != specs.len() {
+ return false;
+ }
self.fields
.iter()
.zip(specs)
- .all(|(pred, spec)| pred.matches(&buf[spec.offset..spec.offset + spec.size]))
+ .all(|(pred, spec)| {
+ pred.width() == spec.size
+ && pred.matches(&buf[spec.offset..spec.offset + spec.size])
+ })
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@acl/src/reference/table.rs` around lines 29 - 35, The matches_packed function
uses debug_assert_eq to check that fields and specs have matching lengths, but
this assertion is compiled away in release builds. When zip silently truncates
in release mode due to a shape mismatch, missing predicates implicitly become
wildcards, unexpectedly widening ACL matches. Replace the debug_assert_eq on
line 30 with an explicit runtime check that returns false immediately if
self.fields.len() does not equal specs.len(), ensuring shape mismatches are
properly rejected in both debug and release builds.
Source: Coding guidelines
Comment on lines
+66
to
+73
| fn pack(key: &K) -> Option<[u8; MAX_KEY_BYTES]> { | ||
| if K::KEY_SIZE > MAX_KEY_BYTES { | ||
| return None; | ||
| } | ||
| let mut buf = [0u8; MAX_KEY_BYTES]; | ||
| key.as_key_into(&mut buf[..K::KEY_SIZE]); | ||
| Some(buf) | ||
| } |
There was a problem hiding this comment.
Avoid silent lookup failure for large MatchKey::KEY_SIZE.
Lines 67-68 make every lookup miss when K::KEY_SIZE > 256, which is a deterministic wrong result rather than “no match”.
As per coding guidelines, “Find logic errors in the code under review. If confident that code is incorrect, suggest a fix.”
Proposed fix
-const MAX_KEY_BYTES: usize = 256;
@@
- fn pack(key: &K) -> Option<[u8; MAX_KEY_BYTES]> {
- if K::KEY_SIZE > MAX_KEY_BYTES {
- return None;
- }
- let mut buf = [0u8; MAX_KEY_BYTES];
- key.as_key_into(&mut buf[..K::KEY_SIZE]);
- Some(buf)
+ fn pack(key: &K) -> Vec<u8> {
+ let mut buf = vec![0u8; K::KEY_SIZE];
+ key.as_key_into(&mut buf);
+ buf
}
@@
- let Some(buf) = Self::pack(key) else {
- return Vec::new();
- };
+ let buf = Self::pack(key);
@@
- let buf = Self::pack(key)?;
+ let buf = Self::pack(key);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@acl/src/reference/table.rs` around lines 66 - 73, The pack function in
table.rs silently returns None when K::KEY_SIZE exceeds MAX_KEY_BYTES, causing
every lookup to fail deterministically without distinguishing between a
legitimate "no match" and an invalid key size. Replace this silent failure with
a compile-time assertion or validation mechanism that ensures KEY_SIZE is
validated upfront (such as using const assertions within the trait
implementation or by panicking if the condition is violated at runtime), so that
oversized keys are caught early as a logic error rather than causing cryptic
lookup failures.
Source: Coding guidelines
mvachhar
approved these changes
Jun 15, 2026
daniel-noland
force-pushed
the
pr/daniel-noland/acl-reference
branch
from
598ba60 to
08b4c60
Compare
Introduces the dataplane-acl crate with the software reference classifier. The DPDK rte_acl backend lands behind a feature gate in a follow-up PR. The reference backend is a linear-scan software classifier built on the canonical FieldPredicate form from match-action (rule.into_backend_fields::<Erased>()), so it speaks the same four predicate kinds (Prefix / Mask / Range / Exact) as every other backend. Two roles: 1. Differential-testing oracle against rte_acl (a future PR's differential property tests pit both backends against the same random rule + packet draws). 2. Non-lossy substrate for a small-delta cascade front over a slow tail backend. Layout: - src/lib.rs declares the crate-level docs and re-exports the reference module. The dpdk feature gate and dpdk_table_alias! macro land alongside the rte_acl backend itself in the next PR. - src/reference/table.rs is the typed surface: ReferenceTable<K, A> parameterised by a MatchKey and an action; RefRule wraps the lowered Erased predicates plus an action. Inline unit tests cover positional precedence (first match wins) and the four predicate kinds. - src/reference/dyn_table.rs is the runtime-shape twin: DynReferenceTable carries its FieldSpec layout at runtime so property tests can fuzz the schema itself. Returns DynShapeError on shape mismatch. just fmt; cargo check --workspace --all-targets passes. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
daniel-noland
force-pushed
the
pr/daniel-noland/acl-reference
branch
from
08b4c60 to
7a7d109
Compare
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.
Stack (11). Base:
pr/daniel-noland/match-action.The ACL crate scaffolding and the software reference backend -- the readable
oracle that defines ACL semantics and the public API surface.
feat(acl): crate scaffolding + software reference backend.The DPDK backend (proven equivalent to this oracle via a differential test)
lands in the next PR.
Review stack (merge bottom -> top):