User ACLs logic implementation#1634
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:
📝 WalkthroughWalkthroughAdds a Rust ACL filter crate that builds tables from validated overlay ACLs, evaluates packet and flow-scoped rules, formats table state, tests behavior, and integrates configuration updates with dataplane packet processing. ChangesACL filter implementation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
53fbdae to
5526612
Compare
5526612 to
81463d2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
acl-filter/src/tests.rs (1)
644-654: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider testing prefix matching against pre-NAT addresses through the real NAT pipeline.
The e2e ACL rule uses empty src/dst patterns (match-all), so this test never validates that a prefix-restricted rule correctly matches the VPC-internal addresses present at ACL-evaluation time (as opposed to the translated/public addresses). Since the module doc comment specifically calls out this address-timing contract, a rule with concrete prefixes (e.g. matching
1.2.3.0/24/192.168.0.0/24) plus a negative case (a prefix that would only match the public/translated addresses) would give confidence the placement is correct rather than just documented.🤖 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-filter/src/tests.rs` around lines 644 - 654, Update the build_overlay test fixture to use concrete source and destination prefixes matching the pre-NAT VPC addresses, such as 1.2.3.0/24 and 192.168.0.0/24, instead of empty match-all patterns. Extend the real NAT-pipeline test with a negative case using prefixes that match only translated/public addresses, while preserving the existing UDP flow rule and ACL behavior.
🤖 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-filter/src/context.rs`:
- Around line 97-116: In insert_for_src_and_dst_prefix, correct the non-TCP/UDP
IPv6 match arm to append rules to self.other_v6 instead of self.other_v4, while
preserving the existing TCP/UDP routing and IPv4 behavior.
In `@acl-filter/src/tests.rs`:
- Around line 600-608: Update the end-to-end flow-scope test comment to describe
the ACL filter’s address state as pre-source-NAT and pre-destination-NAT,
replacing the incorrect post-destination-NAT wording while preserving the
surrounding NAT and flow-link explanation.
---
Nitpick comments:
In `@acl-filter/src/tests.rs`:
- Around line 644-654: Update the build_overlay test fixture to use concrete
source and destination prefixes matching the pre-NAT VPC addresses, such as
1.2.3.0/24 and 192.168.0.0/24, instead of empty match-all patterns. Extend the
real NAT-pipeline test with a negative case using prefixes that match only
translated/public addresses, while preserving the existing UDP flow rule and ACL
behavior.
🪄 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: ee2311e1-0af3-4640-bc97-5facff832f41
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.tomlacl-filter/Cargo.tomlacl-filter/src/access.rsacl-filter/src/context.rsacl-filter/src/display.rsacl-filter/src/lib.rsacl-filter/src/tests.rsacl/src/reference/table.rsconfig/src/external/overlay/acl.rsconfig/src/external/overlay/vpc.rsdataplane/Cargo.tomldataplane/src/packet_processor/mod.rsdataplane/src/runtime.rslpm/Cargo.tomllpm/src/prefix/ip.rslpm/src/prefix/with_ports.rsmgmt/Cargo.tomlmgmt/src/processor/proc.rsmgmt/src/tests/mgmt.rs
81463d2 to
7f5a882
Compare
There was a problem hiding this comment.
Pull request overview
Implements dataplane-side enforcement for the newly introduced user ACLs by adding an acl-filter stage to the packet-processing pipeline, wiring its hot-swappable context into the config processor, and adding the supporting model conversions and validation.
Changes:
- Add new
acl-filtercrate implementingAclFilternetwork function + context building, display, and comprehensive tests. - Integrate ACL context publication into mgmt config processing and inject
AclFilterstage after flow-filter in the dataplane pipeline. - Add match-action lowering helpers in
lpm(prefix/port specs) and tighten ACL validation (reject empty ACL rule lists).
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mgmt/src/tests/mgmt.rs | Adds AclFilterContextWriter to test processor setup. |
| mgmt/src/processor/proc.rs | Publishes AclFilterContext on reconfig and applies ACL config during processing. |
| mgmt/Cargo.toml | Adds workspace dependency on acl-filter. |
| lpm/src/prefix/with_ports.rs | Adds RangeSpec<u16> conversions + wildcard const for ACL lowering. |
| lpm/src/prefix/ip.rs | Adds conversions from IPv4/IPv6 prefix types into match_action::PrefixSpec. |
| lpm/Cargo.toml | Adds dependency on match-action. |
| dataplane/src/runtime.rs | Wires ACL context writer into ConfigProcessorParams. |
| dataplane/src/packet_processor/mod.rs | Installs AclFilter stage after flow-filter; creates reader/writer handles. |
| dataplane/Cargo.toml | Adds workspace dependency on acl-filter. |
| config/src/external/overlay/vpc.rs | Exposes validated peering ACL via ValidatedPeering::acl(). |
| config/src/external/overlay/acl.rs | Makes AclProtoMatch Copy, tweaks proto accessor, makes Acl::new public, rejects empty ACLs, adds tests. |
| Cargo.toml | Adds acl-filter workspace member + workspace metadata. |
| Cargo.lock | Records new dataplane-acl-filter package and dependency updates. |
| acl/src/reference/table.rs | Adds accessor to read back stored rules for display/debugging. |
| acl-filter/src/tests.rs | Adds unit + end-to-end tests for packet/flow scope behavior and NAT interactions. |
| acl-filter/src/lib.rs | Implements AclFilter network function and packet summary logic. |
| acl-filter/src/display.rs | Adds CLI/display decoding of lowered ACL rules and defaults. |
| acl-filter/src/context.rs | Builds lowered match-action tables and default-action map from validated overlay ACLs. |
| acl-filter/src/access.rs | Implements hot-swappable AclFilterContext reader/writer handles. |
| acl-filter/Cargo.toml | New crate manifest with deps and dev-deps for tests. |
82cb6ae to
f615f3a
Compare
f615f3a to
d254d56
Compare
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 `@acl-filter/src/tests.rs`:
- Around line 446-474: Update the UDP section of
protocol_any_rule_matches_both_tcp_and_udp to remove the stale
TCP-only/default-deny explanation and replace it with a comment stating that the
same AclProtoMatch::Any rule should also allow UDP traffic.
🪄 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: 6516df48-699d-43e3-88cf-523e75e2e7e0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlacl-filter/Cargo.tomlacl-filter/src/access.rsacl-filter/src/context.rsacl-filter/src/display.rsacl-filter/src/lib.rsacl-filter/src/tests.rsacl/src/reference/table.rsconfig/Cargo.tomlconfig/src/converters/k8s/config/acl.rsconfig/src/external/overlay/acl.rsconfig/src/external/overlay/vpc.rsdataplane/Cargo.tomldataplane/src/packet_processor/mod.rsdataplane/src/runtime.rslpm/src/prefix/with_ports.rsmgmt/Cargo.tomlmgmt/src/processor/proc.rsmgmt/src/tests/mgmt.rsnet/src/ip/mod.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- acl-filter/Cargo.toml
- dataplane/Cargo.toml
- config/src/external/overlay/vpc.rs
- dataplane/src/runtime.rs
- acl/src/reference/table.rs
- acl-filter/src/display.rs
- mgmt/Cargo.toml
- mgmt/src/tests/mgmt.rs
- mgmt/src/processor/proc.rs
- dataplane/src/packet_processor/mod.rs
- acl-filter/src/lib.rs
- Cargo.toml
- config/src/external/overlay/acl.rs
| manifest_left: &ValidatedManifest, | ||
| manifest_right: &ValidatedManifest, | ||
| ) -> Result<ValidatedAcl, ConfigError> { | ||
| if self.rules.is_empty() { |
There was a problem hiding this comment.
Didn't we say that in the API there would be a default ACL action that would be configurable?
How would this check work in such a case?
There was a problem hiding this comment.
The Acl object always has a default "rule" (a default action) if no rule in the list matches, but it's not in self.rules, it's self.default:
pub struct Acl {
default: AclAction, <-- default if no other rule matches
rules: Vec<AclRule>,
}So we reject the Acl object if there's no other rule than the default one. It's easier for the rest of the processing, and it doesn't really make sense to have an empty list anyway (just remove the ACL, or the peering if you want to block everything).
There was a problem hiding this comment.
Ok. Understood. I would not add that restriction, though. If there is a default, that means that there need not be rules. So a config with Acl = {default: whatever, rule = empty} should be a valid one, even if you would achieve the same behavior with other configuration objects (or the lack of them).
| match self.proto { | ||
| AclProtoMatch::Tcp | AclProtoMatch::Udp => true, | ||
| AclProtoMatch::Other(_) | AclProtoMatch::Icmp | AclProtoMatch::Any => { | ||
| AclProtoMatch::Other(_) | AclProtoMatch::Any => { |
There was a problem hiding this comment.
I think ICMP may actually deserve special treatment since users may want to filter out/allow certain types of errors? Also, there are icmp packets (e.g. errors) that we may want to allow regardless?
There was a problem hiding this comment.
No objection with adding something specific for ICMP at some point.
At the icmp protocol field is not supported in the CRD right now (although it's trivial to add), and even if we add it, we don't need a specific variant internally.
If/when we want additional match options for ICMP in the future, we need to define the API first, update the CRD, and then we can revisit this part in dataplane - but at the moment I see no reason why to keep this variant.
| } | ||
| } | ||
|
|
||
| fn packet_has_valid_flow<'a>( |
There was a problem hiding this comment.
Don't we have similar utils around that we could use for this?
There was a problem hiding this comment.
Do you know where? I can't remember seeing one, but I can look.
Note: You may have seen something similar in the PR for the new flow-filter implementation? If we keep both (if we don't already have something for this, I mean), I'll move them to a shared location.
There was a problem hiding this comment.
probably masquerade / port-forwarding or flow-filter NFs use it
There was a problem hiding this comment.
Flow-filter will after the re-implementation, I haven't been able to find one that fits here in masquerade or port-forwarding
|
@qmonnet I'll continue my review once Daniel's changes land. |
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>
|
I integrated the code changes from Daniel. The code is now the same as in #1639 (except for some |
|
All tests succeeded, but VLAB didn't run because I added the labels too late. Closing/reopening to trigger VLAB tests |
| return; | ||
| } | ||
| }; | ||
| let valid_flow = self.packet_has_valid_flow(packet.meta(), genid); |
There was a problem hiding this comment.
If the flow has a flow but it is not valid, we should probably drop the packet already?
There was a problem hiding this comment.
I don't think so? If the genid is too old, this means the flow was created from a previous config and is no longer valid; but this doesn't mean this packet is invalid. If it successfully goes through flow-filter without relying on the outdated flow, then gets marked for masquerade or port-forwarding, we can create a new updated flow for this packet?
| /// Load the current context for read-only access. | ||
| #[must_use] | ||
| pub fn load(&self) -> Arc<AclFilterContext> { | ||
| self.0.load_full() |
There was a problem hiding this comment.
Why load_full() and not load() ?
There was a problem hiding this comment.
Because I couldn't get it to compile with load() for all of the standard case, loom, and shuttle otherwise. I used load_full() like we use in masquerade for the allocator. I can look a bit more into it, do you know what the right solution would be instead?
There was a problem hiding this comment.
@Fredi-raspall The alternative I have consists in introducing dedicated features for the crate:
diff --git i/Cargo.lock w/Cargo.lock
index 66dc60c5e598..d49acb88cfc8 100644
--- i/Cargo.lock
+++ w/Cargo.lock
@@ -1241,6 +1241,7 @@ dependencies = [
name = "dataplane-acl-filter"
version = "0.22.0"
dependencies = [
+ "arc-swap",
"dataplane-acl",
"dataplane-common",
"dataplane-concurrency",
diff --git i/acl-filter/Cargo.toml w/acl-filter/Cargo.toml
index ce11065fc465..e60b6fb3c6eb 100644
--- i/acl-filter/Cargo.toml
+++ w/acl-filter/Cargo.toml
@@ -5,8 +5,14 @@ license.workspace = true
publish.workspace = true
version.workspace = true
+[features]
+loom = ["concurrency/loom"]
+shuttle = ["concurrency/shuttle"]
+shuttle_dfs = ["concurrency/shuttle_dfs", "shuttle"]
+
[dependencies]
acl = { workspace = true }
+arc-swap = { workspace = true }
common = { workspace = true }
concurrency = { workspace = true }
config = { workspace = true }
diff --git i/acl-filter/src/access.rs w/acl-filter/src/access.rs
index 0667dda7961a..5c1eb1fd954d 100644
--- i/acl-filter/src/access.rs
+++ w/acl-filter/src/access.rs
@@ -83,9 +83,17 @@ pub struct AclFilterContextReader(Arc<Slot<AclFilterContext>>);
impl AclFilterContextReader {
/// Load the current context for read-only access.
+ #[cfg(not(any(feature = "loom", feature = "shuttle")))]
+ #[must_use]
+ pub fn load(&self) -> arc_swap::Guard<Arc<AclFilterContext>> {
+ self.0.load()
+ }
+
+ /// Load the current context for read-only access (loom/shuttle).
+ #[cfg(any(feature = "loom", feature = "shuttle"))]
#[must_use]
pub fn load(&self) -> Arc<AclFilterContext> {
- self.0.load_full()
+ self.0.load()
}
}
If you think it's worth it, I can change.
@daniel-noland or do you know if we have a wrapper for this already? Introducing new crate features just to accommodate for the return type doesn't look optimal, maybe I'm missing something simpler?
There was a problem hiding this comment.
Discussed in meeting (without Fredi): Let's keep as it is for now, the perf delta isn't worth troubling ourselves with the new crate features. I added a FIXME: comment in the code to explain that, at some point, we should add a wrapper around the type in the concurrency crate, so we can use .load().
|
@qmonnet the PR looks good to me overall. I've pushed small enhancements / nits. |
6f5a544 to
6bf22e6
Compare
Add a new crate containing the implementation for the user ACLs: list of access control rules that allow or deny specific packets or flow. We recently introduced support for parsing and validating user ACL objects; The new pipeline stage builds up context tables from validated ACLs, and then apply the rules on coming packets. With the "scope: flow" option, reply traffic for authorized flows is automatically allowed as well (unless another explicit rule denies it). This mode should be extended to all NAT modes (including no-NAT) in the future, but is only supported with masquerade and port forwarding at this time. Internally, the new stage uses ACL structures (match-action) backed by DPDK (rte_acl). 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>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Add unit tests to validate the behaviour of the new ACL filter pipeline stage. 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> Co-authored-by: Fredi Raspall <fredi@githedgehog.com> Signed-off-by: Fredi Raspall <fredi@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
There are several reasons why packets may be dropped: routing, not allowed by peering, etc. Define new drop reason for the case where a packet is dropped due to an ACL denying it as this can help operations. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The ACL filter builds rte_acl classifiers when configuration is applied, which needs the EAL memory subsystem up. Initialize a minimal EAL (no hugepages / no PCI) early in dataplane::main(), before run_mgmt applies the first config; the guard is held for the process. The real DPDK datapath driver must eventually take over EAL ownership (there is one rte_eal_init per process). The mgmt config-processor test starts the EAL for the same reason. 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 a pipeline stage with the recently-added ACL filter to the main pipeline, so that users can enforce ACLs on the traffic. 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>
Add tracectl and linkme as dependencies and define a new tracing target "acl-filter". Otherwise the info!()'s used to log the verdict for a packet would be governed by the default loglevel, which may not be desirable. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
6bf22e6 to
7faebaa
Compare
Fredi, I replied to your comments above. If the reply regarding the packet drop does not address your concern, we can discuss again and address as a follow-up. Regarding the I'm toggling auto-merge. Thanks for the reviews! |
Based on #1618
It will probably require some changes in the way we use the ACLs (similar to the flow-filter reimplementation from #1611).