New ACL-based flow-filter implementation#1611
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (16)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughAdds flofi routing context, packet processing, tests, and dataplane/mgmt wiring, with supporting overlay validation, ACL, and prefix-range conversions. ChangesOverlay, workspace, and lookup prerequisites
flofi crate: tables, display, processing, and tests
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new ACL-based routing/flow-filter implementation (“Flofi”) that looks up peerings/exposes using match-action-style match keys (prefix + optional port range), and threads NAT requirements (static/masquerade/port-forwarding) into packet metadata and flow invalidation logic.
Changes:
- Add a new
floficrate implementing the flow-filter NF plus end-to-end/context-table tests. - Add
match-actionconversion helpers inlpm(PrefixSpec/RangeSpec) to feed ACL predicates. - Extend overlay validation/utilities to support the new routing model (e.g., protocol handling, default expose queries, IP-version checks).
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lpm/src/prefix/with_ports.rs |
Adds conversions from port/prefix types to match_action::RangeSpec<u16> for ACL use. |
lpm/src/prefix/ip.rs |
Adds conversions from Ipv4Prefix/Ipv6Prefix to match_action::rule::PrefixSpec. |
lpm/Cargo.toml |
Adds match-action dependency to support new conversions. |
flofi/src/lib.rs |
New Flofi NF: route lookup + NAT tagging + flow bypass/invalidation logic. |
flofi/src/context/mod.rs |
New Flofi context wrapper around routing tables. |
flofi/src/context/tables.rs |
Builds per-VPC, per-protocol ACL tables for route/NAT lookup. |
flofi/src/context/display.rs |
Adds CLI display formatting for the new routing tables. |
flofi/src/context/tests.rs |
Adds routing-context unit tests covering defaults, NAT modes, protocols, IPv6. |
flofi/src/tests.rs |
Adds end-to-end NF tests including flow bypass/invalidation behaviors. |
flofi/src/test_utils.rs |
Adds helpers to build overlays/peerings/exposes and packets for tests. |
flofi/Cargo.toml |
Declares the new crate and dependencies/features for tests. |
config/src/utils/overlap.rs |
Adjusts L4-protocol intersection call to match updated signature usage. |
config/src/external/overlay/vpcpeering.rs |
Adds helper APIs (can_init_connection, can_receive_connection, has_default_expose, is_v4/is_v6) and changes nat_proto() to return by-value. |
config/src/external/overlay/vpc.rs |
Adds an IP-version consistency check for peerings. |
acl/src/reference/table.rs |
Exposes rules() accessor for table display/debugging. |
Cargo.toml |
Adds flofi to workspace members and workspace dependencies. |
Cargo.lock |
Adds lock entries for the new crate and dependencies. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@Cargo.toml`:
- Line 83: The workspace dependency alias for flofi is pointing at the wrong
package name, so update the Cargo.toml entry for flofi to use the crate name
declared by the flofi package instead of dataplane-flowfi. Locate the existing
workspace dependency declaration for flofi and correct the package field so
downstream crates using workspace = true can resolve it properly.
In `@config/src/external/overlay/vpcpeering.rs`:
- Around line 741-749: The current family detection in
`ValidatedPeering::is_v4()` and `ValidatedPeering::is_v6()` is incorrect for
default exposes and mixed-family manifests, so update the `ValidatedPeering`
classification logic to be default-aware and mutually exclusive before
`Peering::validate()` uses it. Replace the two separate boolean checks with a
single family classifier (or equivalent shared helper) that treats default
exposes correctly and never returns both families for the same manifest, then
use that result for routing/validation instead of the current
`self.valexp.iter().any(...)` behavior.
In `@flofi/src/context/display.rs`:
- Around line 109-148: fmt_predicate currently hides malformed or unknown ACL
predicates by returning Ok(()) and by letting read_u16 coerce any non-2-byte
range bound to 0, which can produce misleading route-table output. Update
fmt_predicate to explicitly surface unsupported predicate kinds instead of
rendering a valid-looking rule, and make read_u16 validate its input so invalid
bounds are reported or skipped with an obvious placeholder. Keep the change
localized to fmt_predicate and read_u16 in the display formatter.
In `@flofi/src/context/tables.rs`:
- Around line 41-46: The default-expose handling in PeeringEndsContext should
preserve the full rule rather than collapsing it into has_default/default-vpc
behavior. Update the PeeringEndsContext<V> lookup path and the code around the
table-building and fallback logic so default exposes keep their protocol, port
range, and NAT mode instead of returning a blanket Some(None) for any miss. In
particular, adjust the logic in the table lookup methods and the default-rule
storage so a TCP/443 default expose only applies to matching traffic, and avoid
hardcoding nat_mode: None when creating the default entry.
In `@flofi/src/lib.rs`:
- Around line 86-97: The `flow_key` attachment logic in `lib.rs` is too
restrictive because it only runs when `attached_flow` is already present, which
skips new packets that still need NAT state. Update the condition around the
`FlowKey::try_from(&*packet)` block so packets requiring `{port forwarding,
masquerading} + static NAT` can attach `packet.meta_mut().flow_key` even when no
valid `attached_flow` exists, while preserving the existing checks for
`dst_vpcd_from_valid_flow` where they are actually needed.
- Around line 41-47: The valid-flow bypass in FlowSummary handling is not
short-circuiting, so packets tagged by Self::tag_for_bypass can still continue
into ACL/route lookup and be rejected later. Update the control flow in lib.rs
around FlowSummary::from_meta, dst_vpcd_from_valid_flow, and
Self::tag_for_bypass so that once a valid current flow is detected and bypass
tagging is applied, the function returns immediately and skips the remaining
route lookup path.
In `@flofi/src/test_utils.rs`:
- Around line 121-125: The vpc_table helper is generating VpcId values that stop
being 5 characters once the index reaches 100, which breaks Vpc::new in the
loop. Update vpc_table to keep producing fixed-width IDs in the VpcId formatting
logic (the format! call inside the enumerate loop), either by enforcing a
99-entry limit or by using a wider zero-padded scheme that still matches the
exact-length requirement.
🪄 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: faa4ab98-31e3-42bb-9369-df161097f2f6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlacl/src/reference/table.rsconfig/src/external/overlay/vpc.rsconfig/src/external/overlay/vpcpeering.rsconfig/src/utils/overlap.rsflofi/Cargo.tomlflofi/src/context/display.rsflofi/src/context/mod.rsflofi/src/context/tables.rsflofi/src/context/tests.rsflofi/src/lib.rsflofi/src/test_utils.rsflofi/src/tests.rslpm/Cargo.tomllpm/src/prefix/ip.rslpm/src/prefix/with_ports.rs
Add helpers to tell whether a validated manifest and peering uses IPv4 or IPv6 prefixes. We just need to check the first expose of the manifest, and (respectively) one of the two manifests, because these objects have been validated so we know they all use the same IP version. Signed-off-by: Quentin Monnet <qmo@qmon.net>
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. Signed-off-by: Quentin Monnet <qmo@qmon.net>
mvachhar
left a comment
mvachhar
left a comment
There was a problem hiding this comment.
Please circle back with @Fredi-raspall and @daniel-noland to improve this PR. The big thing for me is that we should be using the match-action library more aggressively for chaining, or just use one table depending on what the common H/W limitations are.
There are some other minor comments here as well. After we address the big issue, I may have smaller comments on the tests/etc. but for now they are fine.
| /// Returns whether a packet with a source connection from this expose block can initiate a | ||
| /// stateful connection | ||
| #[must_use] | ||
| pub fn can_init_connection(&self) -> bool { |
There was a problem hiding this comment.
I think these are a bit misleading, an expose with no NAT can also initiate connections, same with static nat.
There was a problem hiding this comment.
Good catch, I'll fix the comment.
| if let Some(flow_summary) = attached_flow.as_ref() { | ||
| // Bypass flow-filter if packet has up-to-date active flow-info | ||
| if let Some(dst_vpcd) = self.dst_vpcd_from_valid_flow(flow_summary, genid) { | ||
| Self::tag_for_bypass(packet.meta_mut(), dst_vpcd, flow_summary); |
There was a problem hiding this comment.
Doesn't this just retag the packet meta data with data that we just got from the packet meta data a few lines above? Why do we do this? Can't we just drop this function and this single call site?
There was a problem hiding this comment.
Doesn't this just retag the packet meta data with data that we just got from the packet meta data a few lines above?
Sort of, but this is not a no-op. Packet metadata has flags for the different NAT modes; it also has an optional flow info attached (when there's an existing flow table entry for the packet, and we found it at the flow table lookup stage). What this function does is report the NAT modes from the flow info entry to the metadata NAT flags, that the NAT stages will later look at, regardless of the flow info being present. Dropping this would mean looking for NAT requirements at two different locations in every NAT stage.
|
|
||
| struct FlofiContextWrapper(context::FlofiContext); | ||
|
|
||
| impl FlofiContextWrapper { |
There was a problem hiding this comment.
It's going away, it was a temporary wrapper until I add the proper ArcSwap protection around the context. I'll push this soon and get rid of this.
| { | ||
| fn lookup(&self, proto: NextHeader, tuple: &T) -> Option<&V> { | ||
| match proto { | ||
| NextHeader::TCP => self.tcp.lookup(tuple), |
There was a problem hiding this comment.
We should be able to have the match action table just match the protocol here, I don't know that we need to manually have 2 tables.
| dst_vpcd, | ||
| }) | ||
| })?; | ||
| let local_table = self.local_ends.get(&remote_end_verdict.dst_vpcd)?; |
There was a problem hiding this comment.
Here, you effectively have 3 tables, one for the dst_ip/port, one for the proto, and then one for the source IP to know the expose and policy to apply. I would suggest that this can be just one table (per src_vni). The proto should certainly be collapsed into the first table. You should also ask @daniel-noland if there is a clever way to handle v6 vs. v4 using an outer table.
Having said this, if we want to chain tables, we should not be doing it my hand. I think the match-action library has a way to do chained lookups and we should use it. If we use the match-action library's chaining, then we can optimize it and hardware offload it.
@Fredi-raspall and I disagree as to whether there should be one table or two. My take is that in software, one table is fine, and modern H/W can handle large tables, but I am open to chained tables. I would ask @daniel-noland for advice on which approach is best based on his H/W knowledge.
There was a problem hiding this comment.
After discussing this further with @mvachhar , we believe that the 2 table lookup is fine to avoid the quadratic explosion of rules, but the proto and port could be collapsed in the first table.
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>
We'll use this helper when reworking the flow-filter code. Signed-off-by: Quentin Monnet <qmo@qmon.net>
In preparation for using port ranges with ACL tables, implement trait From to convert types with ports into RangeSpec<u16> elements. Signed-off-by: Quentin Monnet <qmo@qmon.net>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dataplane/src/packet_processor/mod.rs (1)
116-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm
_flow_filteris intentionally unused during the flofi transition.
FlowFilter::new(...)is constructed but never added as a pipeline stage (underscore-prefixed to silence the unused-var lint). The old flow-filter table plumbing (flowfiltertablesw,apply_flow_filtering_configinmgmt/src/processor/proc.rs) is still fully wired and computed on every config update even though nothing consumes it in the packet path now. Given the PR is marked WIP, please confirm this dual-maintenance is intentional for the transition and track removing the old flow-filter stage/wiring once flofi fully replaces it.🤖 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 `@dataplane/src/packet_processor/mod.rs` around lines 116 - 118, Confirm that FlowFilter::new and the underscore-prefixed _flow_filter in packet_processor::mod are intentionally left unused during the flofi transition, and if so, keep the temporary dual wiring explicit in the code/comments. Also ensure the legacy flow-filter plumbing in proc::apply_flow_filtering_config and the flowfiltertablesw path is tracked for removal once flofi fully replaces it, so the packet path and config update path stay aligned.
🤖 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 `@flofi/src/lib.rs`:
- Around line 101-110: Remove the redundant flow-validity re-check in the packet
flow-key attachment block: the attached_flow branch already guarantees
dst_vpcd_from_valid_flow was handled earlier, so the is_none_or(|flow_summary|
self.dst_vpcd_from_valid_flow(...).is_none()) predicate is always true here.
Simplify the condition around the packet meta checks and
FlowKey::try_from(&*packet) by dropping that extra call, and keep
dst_vpcd_from_valid_flow confined to the earlier bypass logic to avoid
re-triggering its invalidate_pair side effect and duplicate logging.
---
Nitpick comments:
In `@dataplane/src/packet_processor/mod.rs`:
- Around line 116-118: Confirm that FlowFilter::new and the underscore-prefixed
_flow_filter in packet_processor::mod are intentionally left unused during the
flofi transition, and if so, keep the temporary dual wiring explicit in the
code/comments. Also ensure the legacy flow-filter plumbing in
proc::apply_flow_filtering_config and the flowfiltertablesw path is tracked for
removal once flofi fully replaces it, so the packet path and config update path
stay aligned.
🪄 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: 153b5656-687a-4c8e-8008-3e051cb88d8d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlacl/src/reference/table.rsconfig/src/external/overlay/vpcpeering.rsconfig/src/utils/overlap.rsdataplane/Cargo.tomldataplane/src/packet_processor/mod.rsdataplane/src/runtime.rsflofi/Cargo.tomlflofi/src/context/display.rsflofi/src/context/mod.rsflofi/src/context/tables.rsflofi/src/context/tests.rsflofi/src/lib.rsflofi/src/test_utils.rsflofi/src/tests.rslpm/Cargo.tomllpm/src/prefix/ip.rslpm/src/prefix/with_ports.rsmgmt/Cargo.tomlmgmt/src/processor/proc.rsmgmt/src/tests/mgmt.rs
✅ Files skipped from review due to trivial changes (4)
- flofi/Cargo.toml
- lpm/Cargo.toml
- mgmt/Cargo.toml
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (11)
- lpm/src/prefix/ip.rs
- config/src/utils/overlap.rs
- flofi/src/context/mod.rs
- acl/src/reference/table.rs
- lpm/src/prefix/with_ports.rs
- flofi/src/tests.rs
- flofi/src/test_utils.rs
- config/src/external/overlay/vpcpeering.rs
- flofi/src/context/display.rs
- flofi/src/context/tests.rs
- flofi/src/context/tables.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@flofi/src/lib.rs`:
- Around line 183-196: The flow invalidation logic in meta-based state handling
only checks whether both NAT modes are gone, so a flow can incorrectly keep an
orphaned port-forwarding or masquerade state when the config drops just one of
them. Update the invalidation path in the flow-state check around
meta.requires_port_forwarding(), meta.requires_masquerade(), and flow_summary so
that a mismatch on either required state alone invalidates the flow, not just
when both are absent. Keep the existing debug logging but make sure the
condition covers each removed NAT mode independently.
🪄 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: 7437bd27-01b8-43f7-9e08-70d679f5cc87
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlacl/src/reference/table.rsdataplane/Cargo.tomldataplane/src/packet_processor/mod.rsdataplane/src/runtime.rsflofi/Cargo.tomlflofi/src/context/display.rsflofi/src/context/mod.rsflofi/src/context/tables.rsflofi/src/context/tests.rsflofi/src/lib.rsflofi/src/test_utils.rsflofi/src/tests.rsmgmt/Cargo.tomlmgmt/src/processor/proc.rsmgmt/src/tests/mgmt.rs
✅ Files skipped from review due to trivial changes (2)
- acl/src/reference/table.rs
- dataplane/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (12)
- dataplane/src/runtime.rs
- mgmt/Cargo.toml
- flofi/Cargo.toml
- Cargo.toml
- dataplane/src/packet_processor/mod.rs
- mgmt/src/tests/mgmt.rs
- flofi/src/context/display.rs
- mgmt/src/processor/proc.rs
- flofi/src/test_utils.rs
- flofi/src/context/tests.rs
- flofi/src/context/tables.rs
- flofi/src/tests.rs
This is a new, simpler version of the flow-filter, the main changes being that the logic has been simplified, because we can get rid of the logic regarding the "multiple matches" used by the legacy implementation, and the use of ACL tables for the lookups. The new stage is not plugged into the datapath pipeline, yet. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Signed-off-by: Quentin Monnet <qmo@qmon.net>
Signed-off-by: Quentin Monnet <qmo@qmon.net>
Swap the flow-filter stage for its new, ACL-based implementation, but do not delete the legacy implementation (and setup) just yet. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Some parts are still under work (see commit description for the commit adding the new pipeline stage), but this is ready for some early reviews.
In particular, I'm interested in comments on the structure and use of the ACLs in this Pull Request.