flow filter printer - #1679
Conversation
📝 WalkthroughWalkthroughThe change adds typed protocol and VNI values, retains typed rules beside backend predicates, introduces shared rule-display utilities, and renders ACL and flow tables consistently across reference and DPDK backends. ChangesTyped match-action contracts
ACL and flow table integration
Validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the MatchKey ecosystem to retain and render typed rules (rather than decoding erased predicate bytes), enabling consistent human-readable CLI output across both the reference and DPDK (opaque rte_acl) backends.
Changes:
- Add
MatchKey::Ruleand enhance the derive macro to generate a companion*Ruletype that implementsDisplay, enabling typed rule retention and rendering. - Update flow-filter and acl-filter table builders to retain typed rules and render them consistently across backends; add cross-backend display equivalence tests.
- Add/extend
DisplayandFixedSizeimpls for protocol/port types used in match keys.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| net/src/udp/port.rs | Implement Display for UdpPort so it can participate in typed rule rendering. |
| net/src/tcp/port.rs | Implement Display for TcpPort so it can participate in typed rule rendering. |
| net/src/fixed_size.rs | Implement FixedSize for NextHeader and add tests for 1-byte encoding. |
| match-action/tests/derive_roundtrip.rs | Add Display for test key field types to satisfy new derive bounds. |
| match-action/src/predicate.rs | Expose be_bytes to the crate for typed-spec formatting helpers. |
| match-action/src/lib.rs | Add MatchKey::Rule associated type and export the new display module. |
| match-action/src/display.rs | New: Display impls for ExactSpec/PrefixSpec/RangeSpec/MaskSpec to format typed rules. |
| match-action-derive/src/lib.rs | Emit a companion *Rule struct and Display impl; wire it into MatchKey::Rule. |
| flow-filter/src/context/tests.rs | Add test asserting identical display across reference and DPDK backends. |
| flow-filter/src/context/tables.rs | Switch key fields to typed NextHeader/Vni; retain typed rules alongside classifiers. |
| flow-filter/src/context/display.rs | Render full rule dumps from retained typed rules in all builds/backends. |
| config/src/external/overlay/acl.rs | Lower ACL proto matches into MaskSpec<NextHeader> (typed protocol field). |
| acl/tests/eal_install_classify.rs | Add Display for test protocol type to satisfy new derive/display requirements. |
| acl/tests/eal_classify_via_projection.rs | Add Display for test protocol type to satisfy new derive/display requirements. |
| acl/src/dpdk/rule.rs | Update manual MatchKey impl in test to include type Rule. |
| acl-filter/src/tests.rs | Add test asserting identical display across reference and DPDK backends. |
| acl-filter/src/display.rs | Render full rule dumps from retained typed rules (no positional decoding). |
| acl-filter/src/context.rs | Switch key fields to typed NextHeader/Vni; retain typed rules alongside classifiers. |
| let kind = match self { | ||
| AnyTable::Empty => "empty", | ||
| AnyTable::Dpdk(_) => "dpdk", | ||
| let kind = match self.classifier { |
daniel-noland
changed the title
mvachhar
left a comment
mvachhar
left a comment
There was a problem hiding this comment.
Please fix up some of these comments, plus could you post in the comments some sample output from the Display formatter so we can see what these look like. Also printing in priority order would be very helpful.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (3)
match-action/src/lib.rs:59
confidence: 8
tags: [other]
`MatchKey` is a public trait in a published crate (`publish.workspace = true`), and adding the required associated type `Rule` is a semver-breaking change for any downstream manual `impl MatchKey`. If this crate is consumed outside the workspace, this should be paired with an appropriate major version bump / release note (or an explicit statement that the API is not yet stable).
pub trait MatchKey: Sized {
/// The rule (predicate) form of this key: one spec per match field, still carrying each
/// field's type. A table can therefore retain the rules it was built from in a form that
/// renders itself, without having to decode erased bytes back into domain types.
type Rule;
const N: usize;
const KEY_SIZE: usize;
fn field_specs() -> &'static [FieldSpec];
fn as_key_into(&self, out: &mut [u8]);
}
**flow-filter/src/context/display.rs:32**
* ```yaml
confidence: 7
tags: [style]
This private render module isn’t referenced anywhere, and because it only contains impls (no referenced items), it’s likely to trip the dead_code/“module is never used” lint in warning-as-error builds. Consider inlining the contents at the file scope, or explicitly allowing the lint on the module.
mod render {
flow-filter/src/context/display.rs:39
confidence: 9
tags: [style]
`Write` is imported here but never used (the `write!`/`writeln!` macros don’t require the trait in scope), which will trigger an `unused_imports` warning.
use std::fmt::{self, Display, Formatter, Write};
</details>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (2)
acl-filter/src/tests.rs:1184
confidence: 9
tags: [style]
`printer_sample` is documented as a manual "printer" (not a real assertion test) but it is still annotated with `#[test]`, so it will run in normal CI by default. Mark it `#[ignore]` (or feature-gate it) so it only runs when explicitly requested.
#[test]
fn printer_sample() {
**flow-filter/src/context/tests.rs:754**
* ```yaml
confidence: 9
tags: [style]
printer_sample is labeled as THROWAWAY: not a test, but it is still annotated with #[test], so it will run in the default test suite (and spam output / add runtime) unless explicitly filtered. Mark it ignored (or gate behind a feature) so it only runs when intentionally requested.
#[test]
fn printer_sample() {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
flow-filter/src/context/tests.rs:754
confidence: 9
tags: [style]
`printer_sample` is described as “not a test, a printer” but it’s still a normal `#[test]`, so it will run in CI/`cargo nextest run` by default and produce noise/time cost. Mark it `#[ignore]` (or gate it behind a feature) so it only runs when explicitly requested.
#[test]
fn printer_sample() {
**acl-filter/src/tests.rs:1184**
* ```yaml
confidence: 9
tags: [style]
printer_sample is explicitly “not a test, a printer” but it will still execute as part of the default unit test suite. Mark it #[ignore] so it doesn’t run in CI unless explicitly selected.
#[test]
fn printer_sample() {
| pub trait MatchKey: Sized { | ||
| /// The rule (predicate) form of this key: one spec per match field, still carrying each | ||
| /// field's type. A table can therefore retain the rules it was built from in a form that | ||
| /// renders itself, without having to decode erased bytes back into domain types. | ||
| type Rule; | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (2)
flow-filter/src/context/tests.rs:783
confidence: 8
tags: [style]
`printer_sample` is described as “THROWAWAY: not a test, a printer”, but it is currently a normal `#[test]` and will run in the default CI suite. Mark it `#[ignore]` (and run it explicitly when needed) to avoid adding runtime to routine test runs.
#[test]
fn printer_sample() {
**acl-filter/src/tests.rs:1184**
* ```yaml
confidence: 8
tags: [style]
printer_sample is documented as a “THROWAWAY” printer, but it is currently a normal #[test] and will run in the default CI suite. Mark it #[ignore] (and run it explicitly when needed) to keep routine test runs focused.
#[test]
fn printer_sample() {
Fredi-raspall
left a comment
Fredi-raspall
left a comment
There was a problem hiding this comment.
@daniel-noland thanks for writing this.
However, it goes beyond the goal of being able to expose the contents of the flow filter tables. Can you please clarify the need for the changes related to masquerading?
| const TCP: Self = IpProto(6); | ||
| } | ||
|
|
||
| impl core::fmt::Display for IpProto { |
There was a problem hiding this comment.
Why is IpProto re defined also in eal_classify_via_projection.rs, as well as its Display and FixedSize implementations?
There was a problem hiding this comment.
Three reasons:
- I was avoiding test dependency on net (can create cycles)
- because these tests are intended for illustration as much as testing
- DPDK requires you to have at least one single byte field (which may be an ignored wildcard but still needs to exist)
The idea was to show how to use the library here and IpProto is a really simple example.
| /// An input to the lookup, not an output: it is the candidate stage 3 verifies when stage 1 | ||
| /// reports a masquerade destination. Carried here rather than consulted in the NF so the | ||
| /// verification stays on the batched path with the other two stages. | ||
| pub(crate) flow_dst_vpcd: Option<VpcDiscriminant>, |
There was a problem hiding this comment.
What is the purpose of this?
There was a problem hiding this comment.
I think you were reviewing scratch code from before I split and refactored all of that. This isn't part of this PR.
| // the address alone (two peers may masquerade behind one range), so they verified the | ||
| // candidate the flow supplied; `None` means no candidate, or one the configuration | ||
| // does not agree with. Either way nothing vouches for the packet. | ||
| LookupResult::MasqueradeDestination(verified) => { |
There was a problem hiding this comment.
I don't understand the need for MasqueradeDestination and the problem it addresses.
If a packet has a flow and the flow is active, it will bypass the filter and never get here.
If the flow is not anymore active, the packet should be just dropped; the flow entry won't live much (if it does at all). If the flow was outdated (a config was changed), the masquerading module will take care of checking if it has to survive or be cancelled. Am I missing something here?
There was a problem hiding this comment.
+1, I'm also struggling to understand these changes
There was a problem hiding this comment.
I think you were reviewing scratch code from before I split and refactored all of that. This isn't part of this PR
| @@ -9,6 +9,12 @@ use dataplane_match_action::{ | |||
| #[derive(Copy, Clone, Debug, PartialEq, Eq)] | |||
| struct IpProto(u8); | |||
There was a problem hiding this comment.
I think this is the 3rd definition I see of IpProto ? (and the impl of FixedSize and Display)
|
The current version shows as: I would opt for something more compact and human-friendly like
|
|
I haven't really reviewed the PR, but I saw this commit: A |
|
The follow-up commit relies on a similar assumption: But unless we have a bug, we ensure that validated manifests only contain one IP version so the difference between |
Review feedback (Fredi-raspall on #1679): the per-rule `name=value, ...` lines are hard to scan, and a compact columnar layout with a heading row would read better. Both dumps now render as a grid: a rank column, one column per key field, a `|`, then the action columns. Before and after, same table: [0] proto=TCP, src_vni=100, dst_ip=80.0.0.5/32, dst_port=2222 -> VNI(200), NAT: port-forwarding [1] proto=*, src_vni=100, dst_ip=90.0.0.0/24, dst_port=* -> VNI(200), NAT: - rank proto srcVpc destination dst-port | to NAT [0] TCP 100 80.0.0.5/32 2222 | VNI(200) port-forwarding [1] * 100 90.0.0.0/24 * | VNI(200) - Column headings are looked up by the field name the derive reports, not by position, so reordering a key's fields cannot silently mislabel a column -- the same property that rendering from the typed form was introduced to give. An unrecognised field falls back to its own name, so a newly added field appears with a slightly raw heading rather than being dropped or, worse, taking its neighbour's label. The `|` separates what is matched on from what results, which the old `-> ` did positionally and less visibly. The rank column keeps its previous meaning: position in match order, not the internal priority value. `ActionDisplay` becomes `ActionColumns`, since an action now contributes cells rather than a rendered tail. It stays a bespoke trait rather than `Display` for the same reason as before -- one action type is the alias `Option<NatRequirement>`, which this crate cannot implement `Display` for. The two `display_is_identical_across_backends` tests still assert that every field renders through its own type (a VNI as a VNI, a protocol by keyword), but now check the cells of a row and the heading row rather than an exact substring. Column widths shift whenever a wider value appears anywhere in the same table, so asserting on spacing would make these tests fail on unrelated fixture edits. Not addressed here, from the same review comment: whether the local table needs its `proto` field at all (it is always `*` -- the local table is built only from exposes that pass `can_init_connection()`, and only port forwarding can carry a protocol -- but it is also the 1-byte first field rte_acl requires), and unifying how a VNI and a `VpcDiscriminant` render (`100` vs `VNI(200)`). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
054501d to
2911063
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (2)
acl-filter/src/context.rs:378
confidence: 10
tags: [logic]
`fmt::Debug` currently matches on `self.classifier` by value, which attempts to move the `classifier` field out of `&self` and will fail to compile. Match on a reference instead.
let kind = match self.classifier {
**flow-filter/src/context/tables.rs:274**
* ```yaml
confidence: 10
tags: [logic]
fmt::Debug currently matches on self.classifier by value, which attempts to move the classifier field out of &self and will fail to compile. Match on a reference instead.
let kind = match self.classifier {
…renderer Add RuleFields and Field so consumers can render and align typed rule fields individually. Add write_grid for fixed-width columnar output without trailing whitespace. An out-of-range field index and a row whose cell count disagrees with the headings both surface as formatting errors rather than panicking or silently dropping a column. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Retain each table's typed rules and render them as a grid: rank, key columns from the rule's own fields, a `|`, then the action columns. Rendering from the typed form means a VNI prints as a VNI and a protocol as `TCP`, with no decoding of erased bytes by width or position. The rank column is the rule's match position, not its internal priority, which is a computed encoding with no meaning outside the table builder and no stability across releases. MatchKey fields may carry `#[cli(column_name = "...")]` so a heading is declared next to the field it heads while the identifier still drives classifier layout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
16b1d80 to
098fa41
Compare
| const TCP: Self = IpProto(6); | ||
| } | ||
|
|
||
| impl core::fmt::Display for IpProto { |
There was a problem hiding this comment.
Three reasons:
- I was avoiding test dependency on net (can create cycles)
- because these tests are intended for illustration as much as testing
- DPDK requires you to have at least one single byte field (which may be an ignored wildcard but still needs to exist)
The idea was to show how to use the library here and IpProto is a really simple example.
| /// An input to the lookup, not an output: it is the candidate stage 3 verifies when stage 1 | ||
| /// reports a masquerade destination. Carried here rather than consulted in the NF so the | ||
| /// verification stays on the batched path with the other two stages. | ||
| pub(crate) flow_dst_vpcd: Option<VpcDiscriminant>, |
There was a problem hiding this comment.
I think you were reviewing scratch code from before I split and refactored all of that. This isn't part of this PR.
| // the address alone (two peers may masquerade behind one range), so they verified the | ||
| // candidate the flow supplied; `None` means no candidate, or one the configuration | ||
| // does not agree with. Either way nothing vouches for the packet. | ||
| LookupResult::MasqueradeDestination(verified) => { |
There was a problem hiding this comment.
I think you were reviewing scratch code from before I split and refactored all of that. This isn't part of this PR
|
Example flow filter CLI print Note I didn't use the exact column names suggested by @Fredi-raspall because I think conflating vpc with vni is quite confusing |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (2)
flow-filter/src/context/tables.rs:276
confidence: 9
tags: [logic]
`AnyTable::fmt` matches on `self.classifier` by value, which attempts to move the `classifier` field out of `&self`. This won’t compile when the `Reference` variant is present (tests), because `ReferenceTable` is not `Copy` (`acl/src/reference/table.rs:37-41`). Match on a reference instead.
impl<K: MatchKey, A> fmt::Debug for AnyTable<K, A> {
fn fmt(&self, f: &mut fmt::Formatter<'>) -> fmt::Result {
let kind = match self.classifier {
Classifier::Empty => "empty",
Classifier::Dpdk() => "dpdk",
**acl-filter/src/context.rs:383**
* ```yaml
confidence: 9
tags: [logic]
AnyTable::fmt matches on self.classifier by value, which attempts to move the classifier field out of &self. This won’t compile under cfg(test) because the Reference variant holds ReferenceTable, which is not Copy (acl/src/reference/table.rs:37-41). Match on a reference instead.
impl<K: MatchKey, A> fmt::Debug for AnyTable<K, A> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let kind = match self.classifier {
Classifier::Empty => "empty",
Classifier::Dpdk(_) => "dpdk",
#[cfg(test)]
Classifier::Reference(_) => "reference",
};
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
acl-filter/src/display.rs (1)
61-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing the grid assembly with the flow-filter renderer.
fmt_tableduplicates the row and heading assembly inflow-filter/src/context/display.rs(Lines 71-96): rank column, key-field loop overField::of,|separator, then action columns. Only the action columns differ. A shared helper inmatch-actionthat takes the heading list and an action-column callback would keep both dumps aligned as fields are added.This is optional. The two copies are small today.
🤖 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/display.rs` around lines 61 - 92, Optionally reduce duplication by extracting the shared heading and row assembly from fmt_table and the flow-filter renderer into a match-action helper. The helper should accept the key headings and an action-column callback, preserve the rank, Field::of, and separator columns, and let each renderer supply its differing action columns; update both renderers to use it while preserving their current output.
🤖 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.
Nitpick comments:
In `@acl-filter/src/display.rs`:
- Around line 61-92: Optionally reduce duplication by extracting the shared
heading and row assembly from fmt_table and the flow-filter renderer into a
match-action helper. The helper should accept the key headings and an
action-column callback, preserve the rank, Field::of, and separator columns, and
let each renderer supply its differing action columns; update both renderers to
use it while preserving their current output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d508e02d-8f47-43d5-9da1-a738046283a5
📒 Files selected for processing (23)
acl-filter/src/context.rsacl-filter/src/display.rsacl-filter/src/tests.rsacl/src/dpdk/rule.rsacl/tests/eal_classify_via_projection.rsacl/tests/eal_install_classify.rsconfig/src/external/overlay/acl.rsfixed-size/src/lib.rsflow-filter/src/context/display.rsflow-filter/src/context/tables.rsflow-filter/src/context/tests.rsmatch-action-derive/src/lib.rsmatch-action/Cargo.tomlmatch-action/src/display.rsmatch-action/src/field.rsmatch-action/src/lib.rsmatch-action/src/predicate.rsmatch-action/src/rule.rsmatch-action/tests/derive_roundtrip.rsnet/src/fixed_size.rsnet/src/ip/mod.rsnet/src/tcp/port.rsnet/src/udp/port.rs
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
The last commit is large and would be easier to review as smaller chunks (although it's probably not worth splitting, now).
Looks good to me, thank you! It will be super helpful to be able to dump these tables.
Manish specifically requested that this be approved out of band
`VpcExpose::validate` enforces a single IP version *within* an expose, via `have_consistent_ip_version` over that expose's own prefix sets. Nothing enforced it *across* the exposes of a manifest, so a manifest could hold `10.0.0.0/24` and `2001:db8::/32` at once and validate cleanly. That is not a supported configuration, and both filters already rely on it not being one: each reads a single IP version off the peering and files its rules by it. The flow-filter picks the root prefix for a catch-all expose from `ValidatedPeering::is_v4`; the ACL filter picks which per-version table every rule of a peering goes into. The gap was invisible from those call sites because `ValidatedManifest::is_v4` is `any`, not `all`. A mixed manifest answers `true` to both `is_v4` and `is_v6`, so `ValidatedPeering::validate_ip_version` -- which compares only `is_v4` -- saw two mixed manifests as agreeing on version and accepted the peering. The consequences were real. A v6 ACL rule on such a peering was filed into the v4 table, where narrowing the key discarded it and the peering's default action then admitted the traffic the rule denied. A catch-all expose covered only IPv4, leaving the peering's IPv6 traffic with nothing to match. Enforcing the invariant here makes it true in one place, rather than defended against in each lowering that assumes it -- which was the first approach tried, and abandoned on review (qmonnet, #1679) because one check in config beats defensive handling in two crates. A default expose names no address, so it belongs to no version and is skipped -- consistent with the carve-out `validate_ip_version` already makes for default-only manifests. Tests cover both orderings (so the check cannot depend on which version is seen first), a default expose alongside either version, and single-version manifests in both versions. Nothing asserted this invariant before, which is how it came to drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`lower_rules` used `filter_map` to drop a rule whose prefixes did not narrow to the table's IP version, and then built the table without it. With the preceding commit that is unreachable -- a manifest's exposes all share one version, so every rule of a peering matches the table it is filed under -- but silently dropping a rule is the wrong shape whether or not the invariant holds, and it was the shape that turned the missing invariant into a fail-open rather than a loud failure. A dropped rule is indistinguishable from a rule that never matched. An operator's `Deny` simply stops being enforced, and under a permissive peering default that is a fail-open: the traffic the rule names is admitted. Nothing logs, nothing fails, and the CLI dump shows a table that is missing a rule the configuration asked for. Returning an error instead refuses the whole configuration, which leaves the previously applied (and correct) tables in place and makes any future drift loud rather than silent. This is defence in depth behind the config check, not a substitute for it: enforcing the invariant once in config is what review settled on (qmonnet, #1679), and this commit assumes it holds. The `expect` messages on the test-only reference builder are updated to say what is actually being asserted: the reference backend cannot fail, but rule lowering now can, and it runs before the backend is chosen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`VpcExpose::validate` enforces a single IP version *within* an expose, via `have_consistent_ip_version` over that expose's own prefix sets. Nothing enforced it *across* the exposes of a manifest, so a manifest could hold `10.0.0.0/24` and `2001:db8::/32` at once and validate cleanly. That is not a supported configuration, and both filters already rely on it not being one: each reads a single IP version off the peering and files its rules by it. The flow-filter picks the root prefix for a catch-all expose from `ValidatedPeering::is_v4`; the ACL filter picks which per-version table every rule of a peering goes into. The gap was invisible from those call sites because `ValidatedManifest::is_v4` is `any`, not `all`. A mixed manifest answers `true` to both `is_v4` and `is_v6`, so `ValidatedPeering::validate_ip_version` -- which compares only `is_v4` -- saw two mixed manifests as agreeing on version and accepted the peering. The consequences were real. A v6 ACL rule on such a peering was filed into the v4 table, where narrowing the key discarded it and the peering's default action then admitted the traffic the rule denied. A catch-all expose covered only IPv4, leaving the peering's IPv6 traffic with nothing to match. Enforcing the invariant here makes it true in one place, rather than defended against in each lowering that assumes it -- which was the first approach tried, and abandoned on review (qmonnet, #1679) because one check in config beats defensive handling in two crates. A default expose names no address, so it belongs to no version and is skipped -- consistent with the carve-out `validate_ip_version` already makes for default-only manifests. Tests cover both orderings (so the check cannot depend on which version is seen first), a default expose alongside either version, and single-version manifests in both versions. Nothing asserted this invariant before, which is how it came to drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`lower_rules` used `filter_map` to drop a rule whose prefixes did not narrow to the table's IP version, and then built the table without it. With the preceding commit that is unreachable -- a manifest's exposes all share one version, so every rule of a peering matches the table it is filed under -- but silently dropping a rule is the wrong shape whether or not the invariant holds, and it was the shape that turned the missing invariant into a fail-open rather than a loud failure. A dropped rule is indistinguishable from a rule that never matched. An operator's `Deny` simply stops being enforced, and under a permissive peering default that is a fail-open: the traffic the rule names is admitted. Nothing logs, nothing fails, and the CLI dump shows a table that is missing a rule the configuration asked for. Returning an error instead refuses the whole configuration, which leaves the previously applied (and correct) tables in place and makes any future drift loud rather than silent. This is defence in depth behind the config check, not a substitute for it: enforcing the invariant once in config is what review settled on (qmonnet, #1679), and this commit assumes it holds. The `expect` messages on the test-only reference builder are updated to say what is actually being asserted: the reference backend cannot fail, but rule lowering now can, and it runs before the backend is chosen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Make the production CLI show the rules installed in the flow-filter and ACL-filter tables, rather than only reporting their counts.
DPDK’s
rte_aclcontexts are opaque after construction, so each table now retains the typed rules from which its classifier was built. Rendering from those typed rules lets domain values format themselves—for example, VNIs appear as VNIs and protocols asTCP—without decoding erased predicate bytes by width or position.Rules are displayed in match order as a fixed-width grid containing:
The displayed rank is the rule’s match position, not its internal priority encoding.
The classifier layout and matching semantics are intended to remain unchanged. The principal runtime tradeoff is that tables retain a typed copy of their rules and actions for operator inspection.
The final commit contains the user-visible behavior. The preceding commits establish the typed representation and formatting infrastructure it depends on.