New ACL-based flow-filter implementation (with DPDK)#1638
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy flow-filter lookup structure with a new ACL-based implementation backed by DPDK rte_acl, introducing a new FlowFilterContext that is built on config application and used by the dataplane in a burst-batched lookup path.
Changes:
- Reworked
flow-filterinto an ACL-table-backedFlowFilterContext(DPDKrte_acl) with burst batching and a new writer/reader API. - Added supporting match-action derive features (
#[phantom]) and conversion utilities (prefix/port specs, FixedSize impls) to express flow-filter ACL keys/rules. - Wired the new context through mgmt/dataplane and introduced EAL initialization requirements for code paths that build
rte_aclclassifiers.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| net/src/tcp/port.rs | Adds TcpPort::{MIN,MAX} constants. |
| net/src/ip/mod.rs | Makes NextHeader::as_u8 a const fn. |
| net/src/icmp4/mod.rs | Adds #[repr(u8)] to Icmp4Type. |
| net/src/fixed_size.rs | Implements FixedSize for unicast IP wrapper types via std IP types. |
| nat/src/test.rs | Updates NAT tests to use FlowFilterContext writer/reader. |
| nat/src/static_nat/setup/mod.rs | Adjusts static-NAT test data (IPv4 ranges). |
| nat/src/masquerade/test.rs | Updates masquerade tests to use FlowFilterContext. |
| nat/Cargo.toml | Updates dev-deps after config/testing changes and flow-filter refactor. |
| mgmt/src/tests/mgmt.rs | Uses FlowFilterContextWriter and initializes EAL in tests that build rte_acl tables. |
| mgmt/src/processor/proc.rs | Switches config application to build/store FlowFilterContext. |
| mgmt/Cargo.toml | Adds dpdk test dependency for EAL in tests. |
| match-action/tests/derive_roundtrip.rs | Adds tests for phantom-field handling in MatchKey derive. |
| match-action/src/rule.rs | Adds From conversions for ExactSpec/MaskSpec. |
| match-action-derive/src/lib.rs | Adds #[phantom] support and narrows FixedSize bounds to match fields only. |
| lpm/src/prefix/with_ports.rs | Adds port-range conversions and helpers to build RangeSpec<u16> for ACLs. |
| lpm/src/prefix/ip.rs | Adds conversions from IP prefixes to PrefixSpec. |
| lpm/Cargo.toml | Adds dependency on match-action. |
| flow-filter/src/test_utils.rs | Adds shared test utilities for building overlays and headers. |
| flow-filter/src/tables.rs | Removes legacy nested-table flow-filter implementation. |
| flow-filter/src/setup.rs | Removes legacy table build logic. |
| flow-filter/src/lib.rs | Replaces stage implementation with batched ACL lookups and FlowFilterContext usage. |
| flow-filter/src/filter_rw.rs | Removes left-right table RW in favor of slot-based context swap. |
| flow-filter/src/display.rs | Removes legacy table display logic. |
| flow-filter/src/context/tests.rs | Adds semantic + differential tests (reference vs dpdk backend). |
| flow-filter/src/context/tables.rs | Implements ACL key/rule lowering and batched two-stage lookup. |
| flow-filter/src/context/mod.rs | Defines FlowFilterContext and slot-based writer/reader handles. |
| flow-filter/src/context/display.rs | Adds production/test rendering for context tables. |
| flow-filter/README.md | Adds documentation for the new flow-filter design and pipeline role. |
| flow-filter/Cargo.toml | Updates dependencies for ACL/DPDK backend and test-only reference backend. |
| dataplane/src/runtime.rs | Initializes a minimal EAL early so config application can build rte_acl contexts. |
| dataplane/src/packet_processor/mod.rs | Wires FlowFilterContextWriter into router setup and CLI sources. |
| dataplane/Cargo.toml | Adds dpdk dependency for EAL init. |
| config/src/utils/overlap.rs | Fixes L4 protocol intersection call with updated return type. |
| config/src/external/overlay/vpcpeering.rs | Adds can_init_connection / can_receive_connection; changes nat_proto() to return by value. |
| config/src/external/overlay/vpc.rs | Adds validation forbidding peerings with mismatched IP-version manifests; removes unsafe test-only fakes. |
| config/src/external/overlay/mod.rs | Removes unsafe test-only fake validation helpers. |
| config/src/external/mod.rs | Removes unsafe test-only fake validation helpers. |
| config/Cargo.toml | Removes testing feature. |
| Cargo.lock | Updates lockfile for new/changed dependencies. |
| acl/src/reference/table.rs | Exposes rules() accessor for reference backend display/testing. |
| acl/src/reference/mod.rs | Gates reference backend behind a feature (now impacts tests). |
| acl/src/lib.rs | Gates reference backend behind a feature (now impacts tests/docs). |
| acl/src/dpdk/lookup.rs | Makes DpdkAclLookup clonable (Arc classifier) and fixes batch result initialization. |
| acl/src/dpdk/dyn_table.rs | Switches dyn classifier ownership to Arc. |
| acl/Cargo.toml | Enables dpdk by default; introduces reference feature. |
d2d1a5b to
bbbfde3
Compare
bbbfde3 to
81e311d
Compare
This comment was marked as low quality.
This comment was marked as low quality.
81e311d to
b184b20
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
acl/src/dpdk/dyn_table.rs (1)
773-877: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover batched lookup for every key shape.
lookup_batchis only exercised onRemoteKey<Ipv4Addr>. Add batch assertions forLocalKey<Ipv4Addr>and both IPv6 tables so their distinct field counts and strides are covered.🤖 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/dpdk/dyn_table.rs` around lines 773 - 877, Extend the existing lookup_batch test after the local_v4, remote_v6, and local_v6 table setup to cover each remaining key shape. For LocalKey<Ipv4Addr>, RemoteKey<Ipv6Addr>, and LocalKey<Ipv6Addr>, build representative key arrays including matching and non-matching entries, call lookup_batch, and assert the outputs match the corresponding single-lookup expectations.flow-filter/README.md (1)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRewrite this README for the new wide-table, batched architecture.
These sections still describe per-source-VPC reference tables,
VpcContext, protocol buckets,process_packet,lookup_route, and per-packet snapshot loads. The implementation now uses four global ACL tables, key fields for VPC/protocol,lookup_route_batch, and one snapshot per burst.Also applies to: 33-34, 53-180
🤖 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 `@flow-filter/README.md` around lines 21 - 23, Rewrite the flow-filter README to document the wide-table, batched architecture: four global ACL tables keyed by VPC and protocol, batch lookups through lookup_route_batch, and a single snapshot load per packet burst. Remove or replace descriptions of per-source-VPC tables, VpcContext, protocol buckets, process_packet, lookup_route, and per-packet snapshot loads while preserving the control-plane/data-plane and ArcSwap context where applicable.
🤖 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 `@flow-filter/src/context/mod.rs`:
- Around line 91-94: Update the reconfiguration flow around
FlowFilterContext::store and pipeline_data.set_genid so the context publication
and generation update occur atomically or under a single synchronized operation.
Ensure packets cannot observe the new ACL tables with the old generation during
the update gap, while preserving the existing context and generation values.
In `@flow-filter/src/lib.rs`:
- Around line 262-273: Update the stale-flow validation logic near the existing
requires_masquerade and requires_port_forwarding comparisons to also compare
flow_summary.requires_static_nat_src and flow_summary.requires_static_nat_dst
against the corresponding meta requirements, invalidating on either direction
change. Add a regression test covering transition from stateful plus static-NAT
routing to stateful-only routing and verify the old flow is invalidated.
In `@match-action-derive/src/lib.rs`:
- Around line 126-143: The generic-bound logic currently scans match-field
tokens and incorrectly constrains inner type parameters such as T in Wrapper<T>.
Replace the collect_idents-based updates in the generics setup with a FixedSize
predicate for each complete match field type, using the field type symbol from
the match-field generation path, and remove the token-scan approach. Preserve
author-defined bounds and ensure wrapper types are accepted based on the
wrapper’s FixedSize implementation.
---
Nitpick comments:
In `@acl/src/dpdk/dyn_table.rs`:
- Around line 773-877: Extend the existing lookup_batch test after the local_v4,
remote_v6, and local_v6 table setup to cover each remaining key shape. For
LocalKey<Ipv4Addr>, RemoteKey<Ipv6Addr>, and LocalKey<Ipv6Addr>, build
representative key arrays including matching and non-matching entries, call
lookup_batch, and assert the outputs match the corresponding single-lookup
expectations.
In `@flow-filter/README.md`:
- Around line 21-23: Rewrite the flow-filter README to document the wide-table,
batched architecture: four global ACL tables keyed by VPC and protocol, batch
lookups through lookup_route_batch, and a single snapshot load per packet burst.
Remove or replace descriptions of per-source-VPC tables, VpcContext, protocol
buckets, process_packet, lookup_route, and per-packet snapshot loads while
preserving the control-plane/data-plane and ArcSwap context where applicable.
🪄 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
Run ID: 3a4dec45-530e-475a-9346-3eaa4f86c8b4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
Cargo.tomlacl/Cargo.tomlacl/benches/table_build.rsacl/src/dpdk/dyn_table.rsacl/src/dpdk/lookup.rsacl/src/lib.rsacl/src/reference/mod.rsacl/src/reference/table.rsacl/tests/property_dyn_shape.rsacl/tests/property_predicate.rsconfig/Cargo.tomlconfig/src/external/mod.rsconfig/src/external/overlay/mod.rsconfig/src/external/overlay/vpc.rsconfig/src/external/overlay/vpcpeering.rsconfig/src/utils/overlap.rsdataplane/Cargo.tomldataplane/src/packet_processor/mod.rsdataplane/src/runtime.rsflow-filter/Cargo.tomlflow-filter/README.mdflow-filter/src/context/display.rsflow-filter/src/context/mod.rsflow-filter/src/context/tables.rsflow-filter/src/context/tests.rsflow-filter/src/display.rsflow-filter/src/filter_rw.rsflow-filter/src/lib.rsflow-filter/src/setup.rsflow-filter/src/tables.rsflow-filter/src/test_utils.rsflow-filter/src/tests.rslpm/Cargo.tomllpm/src/prefix/ip.rslpm/src/prefix/with_ports.rsmatch-action-derive/src/lib.rsmatch-action/src/rule.rsmatch-action/tests/derive_roundtrip.rsmgmt/Cargo.tomlmgmt/src/processor/proc.rsmgmt/src/tests/mgmt.rsnat/Cargo.tomlnat/src/masquerade/test.rsnat/src/static_nat/setup/mod.rsnat/src/test.rsnet/src/fixed_size.rsnet/src/icmp4/mod.rsnet/src/ip/mod.rsnet/src/tcp/port.rs
💤 Files with no reviewable changes (7)
- flow-filter/src/setup.rs
- flow-filter/src/filter_rw.rs
- config/Cargo.toml
- flow-filter/src/display.rs
- config/src/external/mod.rs
- config/src/external/overlay/mod.rs
- flow-filter/src/tables.rs
b184b20 to
7f26ea9
Compare
7f26ea9 to
b9ef7b0
Compare
b9ef7b0 to
58b3876
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
flow-filter/README.md:83
confidence: 8
tags: [docs]
This section describes protocol bucketing into separate `tcp`/`udp`/`other` tables, but the new implementation encodes protocol as a (masked) key field in the ACL tables instead. This is misleading for readers trying to map the docs to `context/tables.rs`.
Inside each end, rules are bucketed by L4 protocol so lookups are protocol-aware:
tcp/udptables: keyed on prefix + port range (TwoTuple).othertable: keyed on prefix only (Singleton) -- used for ICMP etc.- An expose with no protocol restriction (
L4Protocol::Any) is fanned out into all three.
**flow-filter/README.md:136**
* ```yaml
confidence: 9
tags: [docs]
The code no longer has a FlowFilter::process_packet method (the data-path is now burst-oriented via process_burst). The heading should be updated so the symbol reference is accurate.
## 4. The per-packet path (`FlowFilter::process_packet`)
We don't support peerings with two manifests using different IP versions for the prefixes in their expose blocks. We should reject it at validation time. We already validate that all expose blocks in a given manifest use the same IP version for their prefixes, so there's no additional verification required on that side. Update a test in nat crate where we use both IP versions in a peering. Also remove helpers is_v4_or_default() and is_v6_or_default(): they are unused, we kept them for a previous version of this commit but we don't use them, in the end. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Add helpers to see if the NAT mode for an expose block allows it to initiate and/or be at the receiving end of a connection. We'll use this later to rework the flow-filter implementation. Signed-off-by: Quentin Monnet <qmo@qmon.net>
This will help us make the code simpler later when reusing this method. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Add a bitmask field kind (value + mask) to the MatchKey derive and its companion *Rule type, alongside exact/prefix/range. A field matches when input & mask == value & mask, so a full mask is an exact match and a zero mask is a wildcard - letting a single key field express "any" without fanning rules across separate tables. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
Add some trait implementations and some constants so that header field types can be packed in to ACL key bytes. This includes: - for net: #[repr(u8)] on Icmp4Type, TcpPort::MIN/MAX, and FixedSize - for lpm: From prefix/port for RangeSpec<u16> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
We recently added a freature for the reference backend in the "acl" crate. We had the commit doing that in two different branches, one for user ACLs (which got merged first) and one for the new flow-filter implementation. The "less clean" version, regarding the comments, got merged; in particular, one comment in acl/src/dpdk/dyn_table.rs is not where it should be (not strictly above the import directive it comments on). Let's address it. We also have some comments mentioning "flofi", which was a working name for the new flow-filter implementation, but the comments made it through cherry-picking into the user ACLs branch. Let's clean them up, too. Fixes: 43e5930 ("feat(acl): Add feature for reference backend, Arc-share DpdkAclLookup") Fixes: 9415a2b ("feat(dataplane,mgmt): Init EAL early for the ACL filter rte_acl backend") Signed-off-by: Quentin Monnet <qmo@qmon.net>
Add two tests for the dyn_table module. Test flow_filter_two_table_key_shapes_build_and_classify() proves an rte_acl key can carry a masked proto byte, one or two exact u32 VNI fields, and v4/v6 prefix + port range, and that single and batched lookups classify correctly. Test flow_filter_build_capacity_probe() (skipped by default) characterizes build cost for disjoint vs overlapping rule sets, to inform a config-layer rule-count cap. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
In preparation for a new flow-filter implementation, make room for the new version of the crate by renaming the existing flow-filter crate as flow-filter-legacy. No functional change. Signed-off-by: Quentin Monnet <qmo@qmon.net>
This is a new, simpler version of the flow-filter. One of the main changes is that the logic has been simplified, because we can get rid of the logic regarding the "multiple matches" used by the legacy implementation. Another essential change is the use of ACL tables for the lookups, relying on DPDK as a backend, with support for batch processing. Routing is a two-stage lookup (destination -> peer + dst NAT, then source -> src NAT); a default expose lowers to a lowest-priority /0 rule so longest-prefix-match handles it. Function process() collects each rx burst and runs the lookups in batched rte_acl calls, partitioning by IP version without reordering the output. The rte_acl backend is used in production, while a linear-scan reference backend is used for tests. The new stage is not plugged into the datapath pipeline, yet. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
Under the loom backend, concurrency::sync::atomic::{AtomicU32,
AtomicU64}::new is not const: each instance registers with the loom
executor, so it cannot initialize a "static". This makes "cargo check
--features loom --workspace --tests --benches" fail with E0015 ("cannot
call non-const associated function ... in statics").
This is a consequence of a recent commit where we flipped acl's default
features from [] to ["dpdk"]. Every atomic static involved lives behind
'feature = "dpdk"' -- the integration tests and bench module, and the
lib's #[cfg(all(test, feature = "dpdk"))] modules. With dpdk off by
default they were compiled out and never seen by the loom backend;
turning dpdk on by default brings them into every build, including the
loom one.
Wrap each in LazyLock<Atomic..> so the initializer runs on first use
instead of at compile time. The value is still the backend atomic, so
"fetch_add" stays loom-instrumented; only the const requirement on
construction is removed. These are process-unique name/sequence counters
on cold paths, so the one-time lazy init is negligible, and on every
non-loom backend LazyLock merely wraps an otherwise-const atomic.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Quentin Monnet <qmo@qmon.net>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
Swap the legacy flow-filter stage for its new, ACL-based implementation in dataplane, in the mgmt crate, and when used in nat tests. Remove one NAT (masquerade) test that now fails: it relied on a non-valid configuration anyway, and is no longer relevant with the new flow-filter implementation. We do not delete the legacy implementation just yet. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Now that we've changed the flow-filter implementation and that it relies on DPDK (rte_acl), pulling dpdk-sys in the build, we can no longer run the NAT tests relying on it with Miri. Where a simpler mock version is used instead (TestFlowFilter), we can keep using Miri. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
Remove the now deprecated legacy flow-filter implementation. Along with it, we get rid of the "testing" feature for the config crate, and of the related "fake_validated...()" helpers from the same crate. Signed-off-by: Quentin Monnet <qmo@qmon.net>
58b3876 to
8bc10eb
Compare
The MatchKey derive scanned the tokens of each match field's type and added a "FixedSize" bound to every generic type parameter whose name appeared in them. But the generated code never requires the parameter to be "FixedSize": it requires the field's type to be, since that is what it calls "SIZE" and "write_be" on. The two only coincide when the field is spelled as a bare parameter. A field of type "Wrapper<T>", with "Wrapper<T>: FixedSize" holding for every T, gets rejected: the derive demands "T: FixedSize", a constraint the generated code has no use for, so the key cannot be instantiated with a tag type that is not itself "FixedSize". This is the usual "perfect derive" pitfall, the same reason "#[derive(Clone)]" over-bounds its parameters. Bound each match field's type directly, with one where-predicate per field, and drop the token scan. This is exactly what the expansion needs, so it accepts wrappers while staying no more permissive than the generated code demands. It also keeps phantom-only parameters unbounded by construction rather than by name matching, since a parameter that no match field mentions appears in no predicate. The predicates reach the rule struct as well, whose spec fields bound their own parameter. Raised by CodeRabbit. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
This builds up from #1611 and #1636, with the following differences:
mainflofihas been renamed asflow-filter, legacy flow-filter is temporarily renamed then removed