From 610fcaf3877f95fd2c4130ace9efa0c6eff4e413 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 3 Aug 2026 22:42:12 -0600 Subject: [PATCH 1/6] refactor(match-action): give masks their own constructors Add MaskSpec::exact and MaskSpec::wildcard so callers no longer mint fake domain values for the all-ones and zero masks. MaskBits limits them to types where every bit pattern is a valid value. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- fixed-size/src/lib.rs | 32 +++++++++++++++++++++++++++++++ match-action/src/field.rs | 2 +- match-action/src/lib.rs | 2 +- match-action/src/rule.rs | 40 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 73 insertions(+), 3 deletions(-) diff --git a/fixed-size/src/lib.rs b/fixed-size/src/lib.rs index 1a0617612b..af0cbf85ca 100644 --- a/fixed-size/src/lib.rs +++ b/fixed-size/src/lib.rs @@ -17,6 +17,38 @@ pub trait FixedSize: Copy { fn write_be(&self, out: &mut [u8]); } +/// The two bit patterns a bitmask match field is built from: every bit significant, or none. +/// +/// A mask is a bit pattern, not a value of the domain type it constrains, so without these a +/// caller has to mint a fake `Self` to say "match every bit" (`NextHeader::new(0xff)`). +/// +/// # Implementing +/// +/// Only for types where every bit pattern of the type's width is a valid inhabitant, since both +/// constants must be legal values. +/// That is also the condition for being sound to mask at all, so a type that cannot implement this +/// is one that should never be a masked field (`Vni` is non-zero and 24-bit: it has neither +/// constant). +pub trait MaskBits: FixedSize { + /// Every bit significant: the field matches only if it equals the value exactly. + const ALL_BITS: Self; + /// No bit significant: the field is a wildcard and matches anything. + const NO_BITS: Self; +} + +macro_rules! impl_mask_bits_for_uint { + ($($ty:ty),* $(,)?) => {$( + impl MaskBits for $ty { + const ALL_BITS: Self = <$ty>::MAX; + const NO_BITS: Self = 0; + } + )*}; +} + +// The unsigned integers are the natural bitmask carriers: every bit pattern is a valid value, and +// `MAX` / `0` are unambiguous. Domain newtypes implement `MaskBits` next to their `FixedSize` impl. +impl_mask_bits_for_uint!(u8, u16, u32, u64, u128); + impl FixedSize for u8 { const SIZE: usize = 1; fn write_be(&self, out: &mut [u8]) { diff --git a/match-action/src/field.rs b/match-action/src/field.rs index 2e31b16331..532be5469c 100644 --- a/match-action/src/field.rs +++ b/match-action/src/field.rs @@ -1,4 +1,4 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright Open Network Fabric Authors -pub use fixed_size::FixedSize; +pub use fixed_size::{FixedSize, MaskBits}; diff --git a/match-action/src/lib.rs b/match-action/src/lib.rs index 25f18f766c..20f9300103 100644 --- a/match-action/src/lib.rs +++ b/match-action/src/lib.rs @@ -19,7 +19,7 @@ pub mod rule; #[cfg(feature = "bolero")] pub mod generator; -pub use field::FixedSize; +pub use field::{FixedSize, MaskBits}; pub use predicate::{Erased, FieldBytes, FieldPredicate, MAX_FIELD_BYTES}; pub use rule::{ Accepts, Backend, ExactSpec, IntoBackendField, IsUniversal, MaskSpec, PrefixSpec, RangeSpec, diff --git a/match-action/src/rule.rs b/match-action/src/rule.rs index 117f5e0277..817b2c9d49 100644 --- a/match-action/src/rule.rs +++ b/match-action/src/rule.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright Open Network Fabric Authors -use crate::{FieldKind, FixedSize}; +use crate::{FieldKind, FixedSize, MaskBits}; pub trait RuleField { const KIND: FieldKind; type Value: FixedSize; @@ -84,6 +84,30 @@ impl MaskSpec { } } +impl MaskSpec { + /// Match `value` and nothing else: every bit significant. + /// + /// Prefer this to `new(value, all_ones)`, which makes the caller spell an all-ones bit pattern + /// as a `T` -- and `NextHeader::new(0xff)` is not a protocol. + #[must_use] + pub const fn exact(value: T) -> Self { + Self { + value, + mask: T::ALL_BITS, + } + } + + /// Match anything: no bit significant. Renders as `*` and lets one key field express "any" + /// without fanning rules across per-value tables. + #[must_use] + pub const fn wildcard() -> Self { + Self { + value: T::NO_BITS, + mask: T::NO_BITS, + } + } +} + impl RuleField for MaskSpec { const KIND: FieldKind = FieldKind::Mask; type Value = T; @@ -148,6 +172,20 @@ mod tests { use super::*; use core::net::{Ipv4Addr, Ipv6Addr}; + /// The constructors are defined by what they match, not by the bits they store. + /// Exhaustive over `u8`, so "nothing else" covers all 256 inputs rather than a sample. + #[test] + fn exact_and_wildcard_match_what_they_claim() { + let exact = MaskSpec::exact(6u8); + let wildcard = MaskSpec::::wildcard(); + for probe in 0..=u8::MAX { + assert_eq!(exact.accepts(&probe), probe == 6, "probe {probe}"); + assert!(wildcard.accepts(&probe), "probe {probe}"); + } + assert!(wildcard.is_universal()); + assert!(!exact.is_universal()); + } + #[test] fn prefix_spec_accepts_max_v4_length() { let _ = PrefixSpec::new(Ipv4Addr::UNSPECIFIED, 32); From f4132e34852c4a0e2b42b06af4ad39eb3d6e7461 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 3 Aug 2026 22:43:39 -0600 Subject: [PATCH 2/6] refactor(flow-filter): key the routing tables on Vni and NextHeader Use domain types instead of bare integers for the VNI and protocol key fields. The key encodings and classifier behavior remain byte-identical. Also implement FixedSize and MaskBits for NextHeader, and document why the classifier encoding of Vni occupies four bytes. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- flow-filter/src/context/tables.rs | 168 ++++++++++++++---------------- net/src/fixed_size.rs | 37 ++++++- net/src/ip/mod.rs | 7 +- 3 files changed, 120 insertions(+), 92 deletions(-) diff --git a/flow-filter/src/context/tables.rs b/flow-filter/src/context/tables.rs index eafccebfa1..490e3ad9f0 100644 --- a/flow-filter/src/context/tables.rs +++ b/flow-filter/src/context/tables.rs @@ -46,6 +46,7 @@ use match_action::{ }; use net::ip::NextHeader; use net::packet::VpcDiscriminant; +use net::vxlan::Vni; #[cfg(test)] use std::cmp::Reverse; use std::fmt; @@ -91,62 +92,49 @@ pub(super) struct Verdict { pub(super) dst_vpcd: VpcDiscriminant, } -/// A single IP-version's batched query (already lowered: proto byte + `u32` VNI + concrete addr). +/// A single IP-version's batched query (partitioned by IP version, so the address type is +/// concrete; every other field is carried verbatim from the [`LookupInput`]). struct Query { - src_vpcd: u32, - proto: u8, + src_vni: Vni, + proto: NextHeader, src_ip: I, dst_ip: I, src_port: u16, dst_port: u16, } -// IP protocol numbers. The flow-filter only ever emits rules for TCP, UDP, or "any" (see -// L4Protocol), so a non-TCP/UDP packet only ever needs to match an any-proto (mask 0x00) rule -- -// any sentinel byte other than TCP/UDP works for it. -const PROTO_TCP: u8 = NextHeader::TCP.as_u8(); -const PROTO_UDP: u8 = NextHeader::UDP.as_u8(); -const PROTO_OTHER: u8 = 0; - -/// Lower a config L4 protocol to a `(value, mask)` bitmask predicate: -/// a specific protocol matches exactly (`mask 0xff`); "any" wildcards (`mask 0x00`). -fn proto_mask(proto: L4Protocol) -> (u8, u8) { +/// Lower a config L4 protocol to a bitmask predicate: a specific protocol matches exactly (every +/// bit significant); "any" wildcards the field (no bit significant). +fn proto_mask(proto: L4Protocol) -> MaskSpec { match proto { - L4Protocol::Tcp => (PROTO_TCP, 0xff), - L4Protocol::Udp => (PROTO_UDP, 0xff), - L4Protocol::Any => (0, 0x00), - } -} - -/// Map a packet's next-header to the proto key byte (inverse of `proto_mask`). -fn proto_byte(next_header: NextHeader) -> u8 { - match next_header { - NextHeader::TCP => PROTO_TCP, - NextHeader::UDP => PROTO_UDP, - _ => PROTO_OTHER, + L4Protocol::Tcp => MaskSpec::exact(NextHeader::TCP), + L4Protocol::Udp => MaskSpec::exact(NextHeader::UDP), + L4Protocol::Any => MaskSpec::wildcard(), } } -/// A `VpcDiscriminant` as the `u32` carried in an exact key field. -fn vpcd_u32(vpcd: VpcDiscriminant) -> u32 { +/// The VNI that keys the tables. +fn key_vni(vpcd: VpcDiscriminant) -> Vni { match vpcd { - VpcDiscriminant::VNI(vni) => vni.as_u32(), + VpcDiscriminant::VNI(vni) => vni, } } // ------------------------------------------------------------------------------------------------- // Keys. // -// "proto" is first because rte_acl requires a one-byte first field; a #[mask] byte satisfies that -// (it lowers to the same Bitmask field type as #[exact]). VNIs are exact u32 fields. +// "proto" is first because rte_acl requires a one-byte first field; a #[mask] NextHeader satisfies +// that (one byte, and it lowers to the same Bitmask field type as #[exact]). VNIs are exact 4-byte +// fields -- see the FixedSize impl on Vni: the key encoding is padded to 4 bytes because classifier +// fields must be 1, 2, or 4 bytes wide, so it is NOT the 24-bit VXLAN wire encoding. /// Stage-1 key: "which peer does this destination belong to, for this source VPC?" #[derive(Debug, MatchKey, Clone, PartialEq, Eq)] pub(super) struct RemoteKey { #[mask] - proto: u8, + proto: NextHeader, #[exact] - src_vpcd: u32, + src_vni: Vni, #[prefix] dst_ip: I, #[range] @@ -157,11 +145,11 @@ pub(super) struct RemoteKey { #[derive(Debug, MatchKey, Clone, PartialEq, Eq)] pub(super) struct LocalKey { #[mask] - proto: u8, + proto: NextHeader, #[exact] - src_vpcd: u32, + src_vni: Vni, #[exact] - dst_vpcd: u32, + dst_vni: Vni, #[prefix] src_ip: I, #[range] @@ -357,10 +345,10 @@ fn rule_priority(ip_range: Prefix, port_forwarding: bool) -> u32 { fn emit_remote( v4: &mut Vec>, v6: &mut Vec>, - src_vpcd: u32, + src_vni: Vni, ip_range: Prefix, port_range: RangeSpec, - (proto_value, proto_mask): (u8, u8), + proto: MaskSpec, action: Verdict, ) { let priority = rule_priority( @@ -370,8 +358,8 @@ fn emit_remote( match ip_range { Prefix::IPV4(prefix) => { let fields = RemoteKeyRule:: { - proto: MaskSpec::from((proto_value, proto_mask)), - src_vpcd: ExactSpec::new(src_vpcd), + proto, + src_vni: ExactSpec::new(src_vni), dst_ip: PrefixSpec::from(prefix), dst_port: port_range, } @@ -384,8 +372,8 @@ fn emit_remote( } Prefix::IPV6(prefix) => { let fields = RemoteKeyRule:: { - proto: MaskSpec::from((proto_value, proto_mask)), - src_vpcd: ExactSpec::new(src_vpcd), + proto, + src_vni: ExactSpec::new(src_vni), dst_ip: PrefixSpec::from(prefix), dst_port: port_range, } @@ -404,11 +392,11 @@ fn emit_remote( fn emit_local( v4: &mut Vec>, v6: &mut Vec>, - src_vpcd: u32, - dst_vpcd: u32, + src_vni: Vni, + dst_vni: Vni, ip_range: Prefix, port_range: RangeSpec, - (proto_value, proto_mask): (u8, u8), + proto: MaskSpec, action: NatMode, ) { // Port-forwarding sources are never emitted into the local tables, so the tie-break bit is @@ -417,9 +405,9 @@ fn emit_local( match ip_range { Prefix::IPV4(prefix) => { let fields = LocalKeyRule:: { - proto: MaskSpec::from((proto_value, proto_mask)), - src_vpcd: ExactSpec::new(src_vpcd), - dst_vpcd: ExactSpec::new(dst_vpcd), + proto, + src_vni: ExactSpec::new(src_vni), + dst_vni: ExactSpec::new(dst_vni), src_ip: PrefixSpec::from(prefix), src_port: port_range, } @@ -432,9 +420,9 @@ fn emit_local( } Prefix::IPV6(prefix) => { let fields = LocalKeyRule:: { - proto: MaskSpec::from((proto_value, proto_mask)), - src_vpcd: ExactSpec::new(src_vpcd), - dst_vpcd: ExactSpec::new(dst_vpcd), + proto, + src_vni: ExactSpec::new(src_vni), + dst_vni: ExactSpec::new(dst_vni), src_ip: PrefixSpec::from(prefix), src_port: port_range, } @@ -460,10 +448,10 @@ impl RuleSet { fn from_overlay(overlay: &ValidatedOverlay) -> Self { let mut rules = Self::default(); for vpc in overlay.vpc_table().values() { - let src_vpcd = vpcd_u32(VpcDiscriminant::VNI(vpc.vni())); + let src_vni = vpc.vni(); for peering in vpc.peerings() { - let remote_vpcd = VpcDiscriminant::VNI(overlay.vpc_table().get_remote_vni(peering)); - let remote_u32 = vpcd_u32(remote_vpcd); + let remote_vni = overlay.vpc_table().get_remote_vni(peering); + let remote_vpcd = VpcDiscriminant::from_vni(remote_vni); let default_ip = || { if peering.is_v4() { Prefix::root_v4() @@ -487,7 +475,7 @@ impl RuleSet { emit_remote( &mut rules.remote_v4, &mut rules.remote_v6, - src_vpcd, + src_vni, prefix.prefix(), prefix.into(), proto, @@ -499,10 +487,10 @@ impl RuleSet { emit_remote( &mut rules.remote_v4, &mut rules.remote_v6, - src_vpcd, + src_vni, default_ip(), PORT_RANGE_WILDCARD, - (0, 0x00), + proto_mask(L4Protocol::Any), Verdict { nat_mode: None, dst_vpcd: remote_vpcd, @@ -524,8 +512,8 @@ impl RuleSet { emit_local( &mut rules.local_v4, &mut rules.local_v6, - src_vpcd, - remote_u32, + src_vni, + remote_vni, prefix.prefix(), prefix.into(), proto, @@ -537,11 +525,11 @@ impl RuleSet { emit_local( &mut rules.local_v4, &mut rules.local_v6, - src_vpcd, - remote_u32, + src_vni, + remote_vni, default_ip(), PORT_RANGE_WILDCARD, - (0, 0x00), + proto_mask(L4Protocol::Any), None, ); } @@ -610,8 +598,7 @@ impl FlowFilterContext { proto: NextHeader, ports: Option<(u16, u16)>, ) -> LookupResult { - let proto = proto_byte(proto); - let src = vpcd_u32(src_vpcd); + let src_vni = key_vni(src_vpcd); let (src_port, dst_port) = ports.unzip(); let src_port = src_port.unwrap_or(0); let dst_port = dst_port.unwrap_or(0); @@ -620,7 +607,7 @@ impl FlowFilterContext { (IpAddr::V4(src_ip), IpAddr::V4(dst_ip)) => { let Some(verdict) = self.remote_v4.lookup(&RemoteKey { proto, - src_vpcd: src, + src_vni, dst_ip, dst_port, }) else { @@ -628,8 +615,8 @@ impl FlowFilterContext { }; match self.local_v4.lookup(&LocalKey { proto, - src_vpcd: src, - dst_vpcd: vpcd_u32(verdict.dst_vpcd), + src_vni, + dst_vni: key_vni(verdict.dst_vpcd), src_ip, src_port, }) { @@ -642,7 +629,7 @@ impl FlowFilterContext { (IpAddr::V6(src_ip), IpAddr::V6(dst_ip)) => { let Some(verdict) = self.remote_v6.lookup(&RemoteKey { proto, - src_vpcd: src, + src_vni, dst_ip, dst_port, }) else { @@ -650,8 +637,8 @@ impl FlowFilterContext { }; match self.local_v6.lookup(&LocalKey { proto, - src_vpcd: src, - dst_vpcd: vpcd_u32(verdict.dst_vpcd), + src_vni, + dst_vni: key_vni(verdict.dst_vpcd), src_ip, src_port, }) { @@ -685,14 +672,14 @@ impl FlowFilterContext { for (i, input) in inputs.iter().enumerate() { out[i] = LookupResult::DestinationMiss; - let proto = proto_byte(input.proto); - let src_vpcd = vpcd_u32(input.src_vpcd); + let proto = input.proto; + let src_vni = key_vni(input.src_vpcd); let (src_port, dst_port) = input.ports.unwrap_or((0, 0)); match (input.src_ip, input.dst_ip) { (IpAddr::V4(src_ip), IpAddr::V4(dst_ip)) => { v4_idx.push(i); v4_q.push(Query { - src_vpcd, + src_vni, proto, src_ip, dst_ip, @@ -703,7 +690,7 @@ impl FlowFilterContext { (IpAddr::V6(src_ip), IpAddr::V6(dst_ip)) => { v6_idx.push(i); v6_q.push(Query { - src_vpcd, + src_vni, proto, src_ip, dst_ip, @@ -739,7 +726,7 @@ fn lookup_versioned( .iter() .map(|q| RemoteKey { proto: q.proto, - src_vpcd: q.src_vpcd, + src_vni: q.src_vni, dst_ip: q.dst_ip, dst_port: q.dst_port, }) @@ -755,8 +742,8 @@ fn lookup_versioned( let q = &q_chunk[pos]; local_keys.push(LocalKey { proto: q.proto, - src_vpcd: q.src_vpcd, - dst_vpcd: vpcd_u32(verdict.dst_vpcd), + src_vni: q.src_vni, + dst_vni: key_vni(verdict.dst_vpcd), src_ip: q.src_ip, src_port: q.src_port, }); @@ -786,14 +773,15 @@ fn lookup_versioned( mod unit_tests { use super::*; + /// Asserted at the bit level rather than against `MaskSpec::exact`/`wildcard`, which would + /// restate `proto_mask`'s own definition. rte_acl sees only these bytes. #[test] - fn proto_mask_and_byte_roundtrip() { - assert_eq!(proto_mask(L4Protocol::Tcp), (PROTO_TCP, 0xff)); - assert_eq!(proto_mask(L4Protocol::Udp), (PROTO_UDP, 0xff)); - assert_eq!(proto_mask(L4Protocol::Any), (0, 0x00)); - assert_eq!(proto_byte(NextHeader::TCP), PROTO_TCP); - assert_eq!(proto_byte(NextHeader::UDP), PROTO_UDP); - assert_eq!(proto_byte(NextHeader::ICMP), PROTO_OTHER); + fn proto_mask_makes_every_bit_significant_except_for_any() { + let tcp = proto_mask(L4Protocol::Tcp); + assert_eq!((tcp.value, tcp.mask.as_u8()), (NextHeader::TCP, 0xff)); + let udp = proto_mask(L4Protocol::Udp); + assert_eq!((udp.value, udp.mask.as_u8()), (NextHeader::UDP, 0xff)); + assert_eq!(proto_mask(L4Protocol::Any).mask.as_u8(), 0x00); } #[test] @@ -810,17 +798,19 @@ mod unit_tests { } /// The masked-byte lowering of the protocol constraint is equivalent to its direct - /// semantics for EVERY possible packet protocol and every rule protocol. In particular this - /// pins the `PROTO_OTHER = 0` sentinel: protocol 0 (IPv6 hop-by-hop) shares the sentinel - /// byte with every other non-TCP/UDP protocol and must match exactly the `Any` rules. + /// semantics for EVERY possible packet protocol and every rule protocol. + /// + /// Protocols other than TCP and UDP match only the zero-mask `Any` rules. #[test] fn proto_lowering_matches_direct_semantics() { + use match_action::Accepts; + bolero::check!().with_type::().for_each(|&raw| { let packet = NextHeader::new(raw); - let byte = proto_byte(packet); for rule in [L4Protocol::Tcp, L4Protocol::Udp, L4Protocol::Any] { - let (value, mask) = proto_mask(rule); - let lowered = (byte & mask) == (value & mask); + // Ask the spec rather than re-deriving the comparison: this is the same `Accepts` + // impl the reference backend matches through. + let lowered = proto_mask(rule).accepts(&packet); let direct = match rule { L4Protocol::Any => true, L4Protocol::Tcp => packet == NextHeader::TCP, diff --git a/net/src/fixed_size.rs b/net/src/fixed_size.rs index f618d1c625..01a31b4432 100644 --- a/net/src/fixed_size.rs +++ b/net/src/fixed_size.rs @@ -3,14 +3,32 @@ use std::net::{Ipv4Addr, Ipv6Addr}; -use fixed_size::FixedSize; +use fixed_size::{FixedSize, MaskBits}; +use crate::ip::NextHeader; use crate::ipv4::UnicastIpv4Addr; use crate::ipv6::UnicastIpv6Addr; use crate::tcp::TcpPort; use crate::udp::UdpPort; use crate::vxlan::Vni; +/// One byte, and exactly the wire byte: an IP protocol number is 8 bits wherever it appears. +/// (Contrast [`Vni`] below, whose key encoding is padded.) +impl FixedSize for NextHeader { + const SIZE: usize = 1; + fn write_be(&self, out: &mut [u8]) { + self.as_u8().write_be(out); + } +} + +/// Every 8-bit pattern is a valid protocol number, which is what makes `NextHeader` sound to mask. +/// Neither constant is a protocol: they let a rule say "every bit" or "no bit" without a caller +/// writing `NextHeader::new(0xff)` and implying protocol 255. +impl MaskBits for NextHeader { + const ALL_BITS: Self = NextHeader::new(u8::MAX); + const NO_BITS: Self = NextHeader::new(0); +} + impl FixedSize for TcpPort { const SIZE: usize = 2; fn write_be(&self, out: &mut [u8]) { @@ -40,6 +58,15 @@ impl FixedSize for UnicastIpv6Addr { } } +/// The VNI right-aligned in **4** big-endian bytes, leading byte always zero. +/// +/// # Note +/// +/// This is deliberately *not* the VXLAN wire encoding, which is 24 bits (3 bytes). `FixedSize` +/// exists to lay values out as classifier (match/action) key fields, and a classifier field must +/// be 1, 2, or 4 bytes wide -- `rte_acl` rejects anything else when the table is built -- so there +/// is no 3-byte option to pick. Do not reach for [`FixedSize::write_be`] to serialize a VXLAN +/// header: it will write a byte too many. impl FixedSize for Vni { const SIZE: usize = 4; fn write_be(&self, out: &mut [u8]) { @@ -74,6 +101,14 @@ mod tests { assert_eq!(buf, [10, 0, 1, 2]); } + #[test] + fn next_header_writes_one_wire_byte() { + assert_eq!(::SIZE, 1); + let mut buf = [0u8; 1]; + NextHeader::TCP.write_be(&mut buf); + assert_eq!(buf, [NextHeader::TCP.as_u8()]); + } + #[test] fn vni_writes_four_bytes_with_zero_high_byte() { assert_eq!(::SIZE, 4); diff --git a/net/src/ip/mod.rs b/net/src/ip/mod.rs index 0eb40b03b4..5442c5e000 100644 --- a/net/src/ip/mod.rs +++ b/net/src/ip/mod.rs @@ -61,9 +61,12 @@ impl NextHeader { pub const AUTH: NextHeader = NextHeader(IpNumber::AUTHENTICATION_HEADER); /// Generate a new [`NextHeader`] + /// + /// `const` so callers can build associated constants from it (see this type's `MaskBits` + /// impl). `IpNumber`'s `From` is the identity wrapper, so this is the same value. #[must_use] - pub fn new(inner: u8) -> Self { - Self(IpNumber::from(inner)) + pub const fn new(inner: u8) -> Self { + Self(IpNumber(inner)) } /// Return the [`NextHeader`] represented as a `u8` From 41efa5e6f26693e326e321ce245d67a0bf7f63b5 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 3 Aug 2026 22:44:40 -0600 Subject: [PATCH 3/6] feat(match-action): render rules from their typed form Add Display for the typed field specs and derive Display for the generated rule types, so CLI consumers render domain values without decoding erased predicates by width or position. Field types must now implement Display; add the missing port and test-newtype impls. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- acl/src/dpdk/rule.rs | 1 + acl/tests/eal_classify_via_projection.rs | 6 ++ acl/tests/eal_install_classify.rs | 6 ++ match-action-derive/src/lib.rs | 40 +++++++++++ match-action/src/display.rs | 90 ++++++++++++++++++++++++ match-action/src/lib.rs | 6 ++ match-action/src/predicate.rs | 2 +- match-action/tests/derive_roundtrip.rs | 6 ++ net/src/tcp/port.rs | 6 ++ net/src/udp/port.rs | 6 ++ 10 files changed, 168 insertions(+), 1 deletion(-) create mode 100644 match-action/src/display.rs diff --git a/acl/src/dpdk/rule.rs b/acl/src/dpdk/rule.rs index 21d5b70002..5767b298dc 100644 --- a/acl/src/dpdk/rule.rs +++ b/acl/src/dpdk/rule.rs @@ -350,6 +350,7 @@ mod tests { fn rejects_user_field_count_mismatch_in_new() { struct Five; impl MatchKey for Five { + type Rule = (); const N: usize = 5; const KEY_SIZE: usize = 13; fn field_specs() -> &'static [FieldSpec] { diff --git a/acl/tests/eal_classify_via_projection.rs b/acl/tests/eal_classify_via_projection.rs index 759f26e496..2480ea60d5 100644 --- a/acl/tests/eal_classify_via_projection.rs +++ b/acl/tests/eal_classify_via_projection.rs @@ -22,6 +22,12 @@ use net::tcp::{Tcp, TcpPort}; #[derive(Copy, Clone, Debug, PartialEq, Eq)] struct IpProto(u8); +impl core::fmt::Display for IpProto { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!(f, "{}", self.0) + } +} + impl FixedSize for IpProto { const SIZE: usize = 1; fn write_be(&self, out: &mut [u8]) { diff --git a/acl/tests/eal_install_classify.rs b/acl/tests/eal_install_classify.rs index 4ea882a524..2f5e3127b9 100644 --- a/acl/tests/eal_install_classify.rs +++ b/acl/tests/eal_install_classify.rs @@ -22,6 +22,12 @@ impl IpProto { const TCP: Self = IpProto(6); } +impl core::fmt::Display for IpProto { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!(f, "{}", self.0) + } +} + impl FixedSize for IpProto { const SIZE: usize = 1; fn write_be(&self, out: &mut [u8]) { diff --git a/match-action-derive/src/lib.rs b/match-action-derive/src/lib.rs index f310bc9a81..d409ec744b 100644 --- a/match-action-derive/src/lib.rs +++ b/match-action-derive/src/lib.rs @@ -54,6 +54,19 @@ impl Kind { } } +/// Derive [`MatchKey`] for a struct of match fields. +/// +/// Alongside the trait impl this emits a companion `Rule` struct -- the same fields, each +/// wrapped in the spec for its match flavor -- carrying `into_backend_fields`, `accepts`, +/// `is_universal`, and `Display`. +/// +/// # Field type requirements +/// +/// Every match field's type must implement `FixedSize` (its key layout) *and* `Display` (how it +/// is shown to a human). +/// `Display` is mandatory because a classifier an operator cannot inspect is one they cannot +/// debug; a field type missing it shows up as an unsatisfied `MaskSpec: Display` bound on the +/// generated rule struct. #[proc_macro_derive(MatchKey, attributes(prefix, mask, range, exact, phantom))] pub fn derive_match_key(input: TokenStream) -> TokenStream { let input = parse_macro_input!(input as DeriveInput); @@ -199,6 +212,8 @@ fn expand(input: &DeriveInput) -> syn::Result { let mut rule_field_universal: Vec = Vec::with_capacity(n); let mut rule_field_accept_bounds: Vec = Vec::with_capacity(n); let mut rule_field_universal_bounds: Vec = Vec::with_capacity(n); + let mut rule_field_displays: Vec = Vec::with_capacity(n); + let mut rule_field_display_bounds: Vec = Vec::with_capacity(n); for (i, field) in fields.iter().enumerate() { let name = field .ident @@ -227,6 +242,16 @@ fn expand(input: &DeriveInput) -> syn::Result { rule_field_universal_bounds.push(quote! { #crate_path::#spec<#ty>: #crate_path::IsUniversal }); + // Each field renders as `name=spec`, comma-separated, in key order. The name comes from + // the struct field itself, so the label can never drift from the value it labels. + let name_str = name.to_string(); + let separator = if i == 0 { "" } else { ", " }; + rule_field_displays.push(quote! { + ::core::write!(f, "{}{}={}", #separator, #name_str, self.#name)?; + }); + rule_field_display_bounds.push(quote! { + #crate_path::#spec<#ty>: ::core::fmt::Display + }); } // Phantom fields are carried through to the rule verbatim so that any generic // parameter used solely by a phantom field still appears in the rule struct @@ -279,6 +304,11 @@ fn expand(input: &DeriveInput) -> syn::Result { #(#existing_predicates,)* #(#rule_field_universal_bounds,)* }; + let merged_where_display = quote! { + where + #(#existing_predicates,)* + #(#rule_field_display_bounds,)* + }; let expanded = quote! { const _: () = { @@ -289,6 +319,8 @@ fn expand(input: &DeriveInput) -> syn::Result { } impl #impl_generics #crate_path::MatchKey for #key_ident #ty_generics #where_clause { + type Rule = #rule_ident #ty_generics; + const N: usize = #n_literal; const KEY_SIZE: usize = #key_size_expr; @@ -339,6 +371,14 @@ fn expand(input: &DeriveInput) -> syn::Result { #(#rule_field_universal) && * } } + impl #impl_generics ::core::fmt::Display for #rule_ident #ty_generics + #merged_where_display + { + fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result { + #(#rule_field_displays)* + ::core::result::Result::Ok(()) + } + } }; Ok(expanded) diff --git a/match-action/src/display.rs b/match-action/src/display.rs new file mode 100644 index 0000000000..1d645c490f --- /dev/null +++ b/match-action/src/display.rs @@ -0,0 +1,90 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! `Display` for the field specs, so a rule renders from its *typed* form. +//! +//! Typed specs let each field's domain type control its formatting, keeping values coupled to their +//! key fields. + +use core::fmt::{self, Display, Formatter}; + +use crate::IsUniversal; +use crate::field::FixedSize; +use crate::predicate::be_bytes; +use crate::rule::{ExactSpec, MaskSpec, PrefixSpec, RangeSpec}; + +impl Display for ExactSpec { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + Display::fmt(&self.value, f) + } +} + +impl Display for PrefixSpec { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{}/{}", self.value, self.len) + } +} + +impl Display for RangeSpec { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + if self.is_universal() { + f.write_str("*") + } else if self.min == self.max { + Display::fmt(&self.min, f) + } else { + write!(f, "{}..={}", self.min, self.max) + } + } +} + +/// A full mask prints the bare value (`TCP`), an empty mask a wildcard (`*`), and a partial mask +/// `value/0x`. +/// +/// The mask is rendered as the bit pattern it is rather than through `T`'s `Display`, which would +/// print a protocol keyword for a bitmask. +impl Display for MaskSpec { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + let mask = be_bytes(&self.mask); + if mask.iter().all(|byte| *byte == 0) { + f.write_str("*") + } else if mask.iter().all(|byte| *byte == u8::MAX) { + Display::fmt(&self.value, f) + } else { + write!(f, "{}/0x", self.value)?; + mask.iter().try_for_each(|byte| write!(f, "{byte:02x}")) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use core::net::Ipv4Addr; + + #[test] + fn exact_prints_the_value() { + assert_eq!(ExactSpec::new(42u16).to_string(), "42"); + } + + #[test] + fn prefix_prints_value_slash_length() { + assert_eq!( + PrefixSpec::new(Ipv4Addr::new(10, 0, 0, 0), 8).to_string(), + "10.0.0.0/8" + ); + } + + #[test] + fn range_collapses_wildcard_and_singleton() { + assert_eq!(RangeSpec::new(0u16, u16::MAX).to_string(), "*"); + assert_eq!(RangeSpec::exact(443u16).to_string(), "443"); + assert_eq!(RangeSpec::new(80u16, 8080u16).to_string(), "80..=8080"); + } + + #[test] + fn mask_collapses_full_and_empty() { + assert_eq!(MaskSpec::new(6u8, 0xffu8).to_string(), "6"); + assert_eq!(MaskSpec::new(0u8, 0x00u8).to_string(), "*"); + assert_eq!(MaskSpec::new(0xabu8, 0xf0u8).to_string(), "171/0xf0"); + } +} diff --git a/match-action/src/lib.rs b/match-action/src/lib.rs index 20f9300103..30c82e5817 100644 --- a/match-action/src/lib.rs +++ b/match-action/src/lib.rs @@ -12,6 +12,7 @@ #![allow(missing_docs)] #![allow(clippy::missing_errors_doc, clippy::missing_panics_doc)] +pub mod display; pub mod field; pub mod predicate; pub mod rule; @@ -46,6 +47,11 @@ pub struct FieldSpec { pub offset: usize, } pub trait MatchKey: Sized { + /// The rule (predicate) form of this key: one spec per match field, still typed. + /// A table can therefore retain the rules it was built from in a form that renders itself, + /// without decoding erased bytes back into domain types. + type Rule; + const N: usize; const KEY_SIZE: usize; fn field_specs() -> &'static [FieldSpec]; diff --git a/match-action/src/predicate.rs b/match-action/src/predicate.rs index bd1716264f..f8557d479c 100644 --- a/match-action/src/predicate.rs +++ b/match-action/src/predicate.rs @@ -167,7 +167,7 @@ impl FieldPredicate { } } } -fn be_bytes(value: &T) -> FieldBytes { +pub(crate) fn be_bytes(value: &T) -> FieldBytes { let mut buf = [0u8; MAX_FIELD_BYTES]; value.write_be(&mut buf); buf[..T::SIZE].iter().copied().collect() diff --git a/match-action/tests/derive_roundtrip.rs b/match-action/tests/derive_roundtrip.rs index 96e341266e..7d00349992 100644 --- a/match-action/tests/derive_roundtrip.rs +++ b/match-action/tests/derive_roundtrip.rs @@ -9,6 +9,12 @@ use dataplane_match_action::{ #[derive(Copy, Clone, Debug, PartialEq, Eq)] struct IpProto(u8); +impl core::fmt::Display for IpProto { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!(f, "{}", self.0) + } +} + impl FixedSize for IpProto { const SIZE: usize = 1; fn write_be(&self, out: &mut [u8]) { diff --git a/net/src/tcp/port.rs b/net/src/tcp/port.rs index ff5dab1e06..896ba6dad7 100644 --- a/net/src/tcp/port.rs +++ b/net/src/tcp/port.rs @@ -14,6 +14,12 @@ use std::num::NonZero; #[derive(Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Hash, Debug)] pub struct TcpPort(NonZero); +impl std::fmt::Display for TcpPort { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0.get()) + } +} + /// Errors which may occur in the creation or parsing of a [`TcpPort`]. #[repr(transparent)] #[derive( diff --git a/net/src/udp/port.rs b/net/src/udp/port.rs index 6cfe4a6611..9195492ad6 100644 --- a/net/src/udp/port.rs +++ b/net/src/udp/port.rs @@ -18,6 +18,12 @@ use std::num::NonZero; #[serde(try_from = "u16", into = "u16")] pub struct UdpPort(NonZero); +impl std::fmt::Display for UdpPort { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0.get()) + } +} + /// Errors which may occur in the creation or parsing of a [`UdpPort`]. #[repr(transparent)] #[derive( From 85cd563c48995a5a310332e6bd881f110699b2ee Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 3 Aug 2026 22:45:16 -0600 Subject: [PATCH 4/6] fix(match-action): render partial masks as the bits they match on Render a partial mask as 0x/0x. Clearing the insignificant value bits avoids formatting a partial bit pattern as a misleading domain value. Add a property test that equivalent normalized masks accept the same inputs and render identically. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- match-action/Cargo.toml | 6 +++++ match-action/src/display.rs | 52 +++++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/match-action/Cargo.toml b/match-action/Cargo.toml index 8cb35fb73f..73c8dd355e 100644 --- a/match-action/Cargo.toml +++ b/match-action/Cargo.toml @@ -11,6 +11,12 @@ bolero = { workspace = true, optional = true } fixed-size = { workspace = true, features = [] } match-action-derive = { workspace = true, optional = true } +[dev-dependencies] +# Property tests in the default test suite. The `bolero` *feature* above gates the public +# `generator` module and is a separate concern: this dev-dep only makes the harness available to +# `cfg(test)` builds, so property tests here run under a plain `cargo nextest run`. +bolero = { workspace = true, features = ["std"] } + [features] default = ["derive"] derive = ["dep:match-action-derive"] diff --git a/match-action/src/display.rs b/match-action/src/display.rs index 1d645c490f..f22fe4cf36 100644 --- a/match-action/src/display.rs +++ b/match-action/src/display.rs @@ -38,10 +38,14 @@ impl Display for RangeSpec { } /// A full mask prints the bare value (`TCP`), an empty mask a wildcard (`*`), and a partial mask -/// `value/0x`. +/// `0x/0x`. /// -/// The mask is rendered as the bit pattern it is rather than through `T`'s `Display`, which would -/// print a protocol keyword for a bitmask. +/// Neither operand of a partial mask is a meaningful `T`, so neither goes through `T`'s `Display`: +/// `TCP` under mask `0xf0` matches every protocol `0x00..=0x0f`, and printing `TCP/0xf0` would +/// name one member of that set as though it were the whole set. +/// +/// The value is masked first because that is what both backends match on -- `rte_acl` ANDs it into +/// the trie, and [`Accepts`](crate::Accepts) compares `(field & mask)` against `(value & mask)`. impl Display for MaskSpec { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { let mask = be_bytes(&self.mask); @@ -50,7 +54,13 @@ impl Display for MaskSpec { } else if mask.iter().all(|byte| *byte == u8::MAX) { Display::fmt(&self.value, f) } else { - write!(f, "{}/0x", self.value)?; + let value = be_bytes(&self.value); + f.write_str("0x")?; + value + .iter() + .zip(mask.iter()) + .try_for_each(|(value, mask)| write!(f, "{:02x}", value & mask))?; + f.write_str("/0x")?; mask.iter().try_for_each(|byte| write!(f, "{byte:02x}")) } } @@ -59,6 +69,7 @@ impl Display for MaskSpec { #[cfg(test)] mod tests { use super::*; + use crate::Accepts; use core::net::Ipv4Addr; #[test] @@ -85,6 +96,37 @@ mod tests { fn mask_collapses_full_and_empty() { assert_eq!(MaskSpec::new(6u8, 0xffu8).to_string(), "6"); assert_eq!(MaskSpec::new(0u8, 0x00u8).to_string(), "*"); - assert_eq!(MaskSpec::new(0xabu8, 0xf0u8).to_string(), "171/0xf0"); + assert_eq!(MaskSpec::new(0xabu8, 0xf0u8).to_string(), "0xa0/0xf0"); + } + + /// Both operands render as bit patterns over the field's full width, with the value's + /// don't-care bits cleared. + #[test] + fn partial_mask_renders_as_bit_patterns() { + assert_eq!( + MaskSpec::new(0xabcdu16, 0xff00u16).to_string(), + "0xab00/0xff00" + ); + } + + /// Two specs differing only outside the mask accept the same inputs -- asserted here rather + /// than assumed -- so they must render identically. + /// Otherwise the dump would show an operator a distinction the classifier does not make. + #[test] + fn render_ignores_value_bits_outside_the_mask() { + bolero::check!() + .with_type::<(u8, u8)>() + .for_each(|&(value, mask)| { + let raw = MaskSpec::new(value, mask); + let normalized = MaskSpec::new(value & mask, mask); + for probe in 0..=u8::MAX { + assert_eq!( + raw.accepts(&probe), + normalized.accepts(&probe), + "masked-off bits changed what {raw:?} accepts" + ); + } + assert_eq!(raw.to_string(), normalized.to_string()); + }); } } From fe2764a1be8293e6b9ac7695137d96953ebc8732 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 3 Aug 2026 22:47:01 -0600 Subject: [PATCH 5/6] feat(match-action): expose a rule's fields one at a time, and a grid 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) Signed-off-by: Daniel Noland --- match-action-derive/src/lib.rs | 27 ++++ match-action/src/display.rs | 226 +++++++++++++++++++++++++++++++++ match-action/src/lib.rs | 1 + 3 files changed, 254 insertions(+) diff --git a/match-action-derive/src/lib.rs b/match-action-derive/src/lib.rs index d409ec744b..d755d95d99 100644 --- a/match-action-derive/src/lib.rs +++ b/match-action-derive/src/lib.rs @@ -214,6 +214,8 @@ fn expand(input: &DeriveInput) -> syn::Result { let mut rule_field_universal_bounds: Vec = Vec::with_capacity(n); let mut rule_field_displays: Vec = Vec::with_capacity(n); let mut rule_field_display_bounds: Vec = Vec::with_capacity(n); + let mut rule_field_names: Vec = Vec::with_capacity(n); + let mut rule_field_fmt_arms: Vec = Vec::with_capacity(n); for (i, field) in fields.iter().enumerate() { let name = field .ident @@ -252,6 +254,13 @@ fn expand(input: &DeriveInput) -> syn::Result { rule_field_display_bounds.push(quote! { #crate_path::#spec<#ty>: ::core::fmt::Display }); + // The same fields again, reachable one at a time: a columnar caller needs each value's + // width before it can pad, and the whole-rule `Display` above hands back a finished + // string. See `match_action::RuleFields`. + rule_field_names.push(quote! { #name_str }); + rule_field_fmt_arms.push(quote! { + #i => ::core::fmt::Display::fmt(&self.#name, f), + }); } // Phantom fields are carried through to the rule verbatim so that any generic // parameter used solely by a phantom field still appears in the rule struct @@ -379,6 +388,24 @@ fn expand(input: &DeriveInput) -> syn::Result { ::core::result::Result::Ok(()) } } + impl #impl_generics #crate_path::RuleFields for #rule_ident #ty_generics + #merged_where_display + { + const FIELD_NAMES: &'static [&'static str] = &[#(#rule_field_names),*]; + + fn fmt_field( + &self, + index: usize, + f: &mut ::core::fmt::Formatter<'_>, + ) -> ::core::fmt::Result { + match index { + #(#rule_field_fmt_arms)* + // Out of range: `FIELD_NAMES` bounds every legitimate call, so report the + // caller bug as a formatting failure rather than panicking in a `Display`. + _ => ::core::result::Result::Err(::core::fmt::Error), + } + } + } }; Ok(expanded) diff --git a/match-action/src/display.rs b/match-action/src/display.rs index f22fe4cf36..19d7f47a64 100644 --- a/match-action/src/display.rs +++ b/match-action/src/display.rs @@ -13,6 +13,112 @@ use crate::field::FixedSize; use crate::predicate::be_bytes; use crate::rule::{ExactSpec, MaskSpec, PrefixSpec, RangeSpec}; +/// A rule's fields, reachable one at a time. +/// +/// Columnar renderers use +/// [`FIELD_NAMES`](RuleFields::FIELD_NAMES) in key order indexing +/// [`fmt_field`](RuleFields::fmt_field) to measure and align individual fields. +pub trait RuleFields { + /// The rule's field names, in key order. + const FIELD_NAMES: &'static [&'static str]; + + /// Render the field at `index`, or fail if there is no such field. + /// + /// # Errors + /// + /// Returns [`fmt::Error`] when `index` is out of range, or when the underlying field's own + /// `Display` fails. + fn fmt_field(&self, index: usize, f: &mut Formatter<'_>) -> fmt::Result; +} + +/// One field of one rule, as something printable. +/// +/// Honours width, fill and alignment (`{:<12}`), so a caller pads with the ordinary formatting +/// machinery rather than measuring by hand. +pub struct Field<'a, R: ?Sized> { + rule: &'a R, + index: usize, +} + +impl<'a, R: RuleFields + ?Sized> Field<'a, R> { + /// The field at `index` of `rule`. + #[must_use] + pub fn of(rule: &'a R, index: usize) -> Self { + Self { rule, index } + } +} + +impl Display for Field<'_, R> { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + struct Unpadded<'a, R: ?Sized>(&'a Field<'a, R>); + impl Display for Unpadded<'_, R> { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + self.0.rule.fmt_field(self.0.index, f) + } + } + // Buffered first so `pad` can apply the caller's width. `fmt::write` rather than + // `to_string`, which panics when a `Display` impl errors -- an out-of-range index is a + // caller bug and should not take down a CLI. + let mut rendered = String::new(); + core::fmt::write(&mut rendered, format_args!("{}", Unpadded(self)))?; + f.pad(&rendered) + } +} + +/// Render `rows` under `headings` as a fixed-width grid. +/// +/// Every column is padded to the widest cell in it, headings included, and columns are separated by +/// two spaces. The last column is not padded, so lines carry no trailing whitespace. +/// +/// # Errors +/// +/// Returns [`fmt::Error`] if the underlying writer fails, or if a row's cell count differs from +/// `headings` -- a miscounted row would otherwise silently drop or blank a column. +pub fn write_grid( + w: &mut W, + headings: &[&str], + rows: &[Vec], +) -> fmt::Result { + if rows.iter().any(|row| row.len() != headings.len()) { + return Err(fmt::Error); + } + + let width = |s: &str| s.chars().count(); + let widths: Vec = headings + .iter() + .enumerate() + .map(|(i, heading)| { + rows.iter() + .map(|row| width(&row[i])) + .chain(core::iter::once(width(heading))) + .max() + .unwrap_or(0) + }) + .collect(); + + let last = headings.len().saturating_sub(1); + let mut write_row = |cells: &dyn Fn(usize) -> String| -> fmt::Result { + let mut line = String::new(); + for (i, column) in widths.iter().enumerate() { + let text = cells(i); + line.push_str(&text); + if i != last { + line.extend(core::iter::repeat_n( + ' ', + column.saturating_sub(width(&text)) + 2, + )); + } + } + writeln!(w, "{}", line.trim_end()) + }; + + write_row(&|i| headings[i].to_string())?; + for row in rows { + write_row(&|i| row[i].clone())?; + } + Ok(()) +} + impl Display for ExactSpec { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { Display::fmt(&self.value, f) @@ -130,3 +236,123 @@ mod tests { }); } } + +#[cfg(test)] +mod field_and_grid_tests { + use super::*; + use crate::{MaskSpec, PrefixSpec, RangeSpec}; + use core::net::Ipv4Addr; + + /// A hand-written stand-in for a derived rule, so these tests exercise `RuleFields` without + /// depending on the derive macro (which lives in a different crate and cannot be used here). + struct Rule { + proto: MaskSpec, + dst_ip: PrefixSpec, + dst_port: RangeSpec, + } + + impl RuleFields for Rule { + const FIELD_NAMES: &'static [&'static str] = &["proto", "dst_ip", "dst_port"]; + + fn fmt_field(&self, index: usize, f: &mut Formatter<'_>) -> fmt::Result { + match index { + 0 => Display::fmt(&self.proto, f), + 1 => Display::fmt(&self.dst_ip, f), + 2 => Display::fmt(&self.dst_port, f), + _ => Err(fmt::Error), + } + } + } + + fn rule() -> Rule { + Rule { + proto: MaskSpec::exact(6u8), + dst_ip: PrefixSpec::new(Ipv4Addr::new(10, 0, 0, 0), 24), + dst_port: RangeSpec::exact(443u16), + } + } + + #[test] + fn fields_render_individually_and_in_key_order() { + let rule = rule(); + let rendered: Vec = (0..Rule::FIELD_NAMES.len()) + .map(|i| Field::of(&rule, i).to_string()) + .collect(); + assert_eq!(rendered, ["6", "10.0.0.0/24", "443"]); + } + + /// The point of the adapter. Rendering through `fmt_field` directly would ignore the width, + /// since padding is applied by the outermost `Display` and a field knows nothing of its column. + #[test] + fn a_field_honours_width_and_alignment() { + let rule = rule(); + assert_eq!(format!("{:<8}|", Field::of(&rule, 1)), "10.0.0.0/24|"); + assert_eq!(format!("{:<14}|", Field::of(&rule, 1)), "10.0.0.0/24 |"); + assert_eq!(format!("{:>6}|", Field::of(&rule, 2)), " 443|"); + } + + #[test] + fn an_out_of_range_field_fails_rather_than_panicking() { + use core::fmt::Write; + let mut sink = String::new(); + assert!(write!(sink, "{}", Field::of(&rule(), 3)).is_err()); + } + + #[test] + fn grid_pads_every_column_to_its_widest_cell() { + let mut out = String::new(); + write_grid( + &mut out, + &["rank", "destination", "NAT"], + &[ + vec!["[0]".into(), "10.0.0.0/24".into(), "-".into()], + vec!["[1]".into(), "1.2.3.4/32".into(), "masquerade".into()], + ], + ) + .unwrap(); + assert_eq!( + out, + "rank destination NAT\n\ + [0] 10.0.0.0/24 -\n\ + [1] 1.2.3.4/32 masquerade\n", + ); + } + + /// The last column is not padded, so no line carries trailing whitespace -- invisible diff + /// noise the moment anyone captures this output. + #[test] + fn grid_leaves_no_trailing_whitespace() { + let mut out = String::new(); + write_grid( + &mut out, + &["a", "bbbb"], + &[ + vec!["x".into(), "y".into()], + vec!["zz".into(), String::new()], + ], + ) + .unwrap(); + for line in out.lines() { + assert_eq!(line, line.trim_end(), "trailing whitespace in {line:?}"); + } + } + + /// A row that does not match the headings is a caller bug: rendering it would drop or blank a + /// column, which reads as a rule that has no action rather than as a mistake. + #[test] + fn grid_rejects_rows_with_the_wrong_number_of_cells() { + let mut out = String::new(); + assert!(write_grid(&mut out, &["a", "b"], &[vec!["x".into()]]).is_err()); + assert!(out.is_empty()); + + assert!( + write_grid( + &mut out, + &["a", "b"], + &[vec!["x".into(), "y".into(), "z".into()]], + ) + .is_err() + ); + assert!(out.is_empty()); + } +} diff --git a/match-action/src/lib.rs b/match-action/src/lib.rs index 30c82e5817..32aaa495f5 100644 --- a/match-action/src/lib.rs +++ b/match-action/src/lib.rs @@ -20,6 +20,7 @@ pub mod rule; #[cfg(feature = "bolero")] pub mod generator; +pub use display::{Field, RuleFields, write_grid}; pub use field::{FixedSize, MaskBits}; pub use predicate::{Erased, FieldBytes, FieldPredicate, MAX_FIELD_BYTES}; pub use rule::{ From 098fa413977d337ed6b61570de2309336db8c65a Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 3 Aug 2026 22:48:37 -0600 Subject: [PATCH 6/6] feat(flow-filter,acl-filter): dump installed rules from the CLI 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) Signed-off-by: Daniel Noland --- acl-filter/src/context.rs | 219 ++++++++++++++----------- acl-filter/src/display.rs | 210 +++++------------------- acl-filter/src/tests.rs | 79 +++++++++ config/src/external/overlay/acl.rs | 23 ++- flow-filter/src/context/display.rs | 179 ++++++++------------ flow-filter/src/context/tables.rs | 189 +++++++++++++-------- flow-filter/src/context/tests.rs | 140 +++++++++++++++- match-action-derive/src/lib.rs | 48 +++++- match-action/src/display.rs | 4 +- match-action/tests/derive_roundtrip.rs | 60 +++++++ 10 files changed, 678 insertions(+), 473 deletions(-) diff --git a/acl-filter/src/context.rs b/acl-filter/src/context.rs index ccb61b663a..ac3c2768fd 100644 --- a/acl-filter/src/context.rs +++ b/acl-filter/src/context.rs @@ -18,7 +18,8 @@ use config::external::overlay::acl::{AclAction, AclProtoMatch, AclScope, Validat use dpdk::acl::{CategoryMask, Priority}; use lookup::Lookup; use lpm::prefix::{Prefix, PrefixPortsSet, PrefixWithOptionalPorts}; -use match_action::{Erased, ExactSpec, FieldPredicate, FixedSize, MaskSpec, MatchKey, RangeSpec}; +use match_action::{Erased, ExactSpec, FieldPredicate, FixedSize, MatchKey, PrefixSpec, RangeSpec}; +use net::ip::NextHeader; use net::vxlan::Vni; use std::collections::HashMap; use std::fmt; @@ -167,25 +168,31 @@ impl From<&ValidatedOverlay> for PeeringAclRuleSet { #[derive(Debug, MatchKey, Clone, PartialEq, Eq)] pub(super) struct AclKey { #[mask] - proto: u8, + proto: NextHeader, #[exact] - src_vni: u32, + #[cli(column_name = "src-vni")] + src_vni: Vni, #[exact] - dst_vni: u32, + #[cli(column_name = "dst-vni")] + dst_vni: Vni, #[prefix] + #[cli(column_name = "source")] src_ip_range: I, #[prefix] + #[cli(column_name = "destination")] dst_ip_range: I, #[range] + #[cli(column_name = "src-port")] src_port_range: u16, #[range] + #[cli(column_name = "dst-port")] dst_port_range: u16, } impl AclKey { #[must_use] fn new( - proto: u8, + proto: NextHeader, src_vni: Vni, dst_vni: Vni, src_ip: I, @@ -195,8 +202,8 @@ impl AclKey { ) -> Self { Self { proto, - src_vni: src_vni.as_u32(), - dst_vni: dst_vni.as_u32(), + src_vni, + dst_vni, src_ip_range: src_ip, dst_ip_range: dst_ip, src_port_range: src_port.unwrap_or(0), @@ -208,7 +215,7 @@ impl AclKey { /// Lower a single rule to backend-neutral field predicates for the concrete IP version of its /// prefixes. Returns `None` if the source and destination prefixes disagree on IP version, which /// the config validation already rules out for a well-formed peering. -fn rule_predicates( +fn rule_predicates( proto: AclProtoMatch, src_vni: Vni, dst_vni: Vni, @@ -216,73 +223,75 @@ fn rule_predicates( dst_ip_range: Prefix, src_port_range: RangeSpec, dst_port_range: RangeSpec, -) -> Option> { - let proto: MaskSpec = proto.into(); - match (src_ip_range, dst_ip_range) { - (Prefix::IPV4(src_ip_range), Prefix::IPV4(dst_ip_range)) => Some( - AclKeyRule { - proto, - src_vni: ExactSpec::new(src_vni.as_u32()), - dst_vni: ExactSpec::new(dst_vni.as_u32()), - src_ip_range: src_ip_range.into(), - dst_ip_range: dst_ip_range.into(), - src_port_range, - dst_port_range, - } - .into_backend_fields::(), - ), - (Prefix::IPV6(src_ip_range), Prefix::IPV6(dst_ip_range)) => Some( - AclKeyRule { - proto, - src_vni: ExactSpec::new(src_vni.as_u32()), - dst_vni: ExactSpec::new(dst_vni.as_u32()), - src_ip_range: src_ip_range.into(), - dst_ip_range: dst_ip_range.into(), - src_port_range, - dst_port_range, - } - .into_backend_fields::(), - ), - _ => None, - } +) -> Option<(AclKeyRule, Vec)> { + let rule = AclKeyRule { + proto: proto.into(), + src_vni: ExactSpec::new(src_vni), + dst_vni: ExactSpec::new(dst_vni), + src_ip_range: T::prefix_spec(src_ip_range)?, + dst_ip_range: T::prefix_spec(dst_ip_range)?, + src_port_range, + dst_port_range, + }; + Some((rule, rule.into_backend_fields::())) } -pub(super) trait Wildcardable { +/// Per-IP-version behaviour for the address key fields: the wildcard prefix, and narrowing a +/// version-agnostic [`Prefix`] to this version. `prefix_spec` returns `None` for the other +/// version, which is what keeps each table to a single address width. +pub(super) trait IpVersion: FixedSize { fn wildcard() -> Prefix; + fn prefix_spec(prefix: Prefix) -> Option>; } -impl Wildcardable for Ipv4Addr { +impl IpVersion for Ipv4Addr { fn wildcard() -> Prefix { Prefix::root_v4() } + + fn prefix_spec(prefix: Prefix) -> Option> { + match prefix { + Prefix::IPV4(prefix) => Some(prefix.into()), + Prefix::IPV6(_) => None, + } + } } -impl Wildcardable for Ipv6Addr { +impl IpVersion for Ipv6Addr { fn wildcard() -> Prefix { Prefix::root_v6() } + + fn prefix_spec(prefix: Prefix) -> Option> { + match prefix { + Prefix::IPV6(prefix) => Some(prefix.into()), + Prefix::IPV4(_) => None, + } + } } /// Lower all rules for one IP version into `(predicates, action)` pairs, preserving order (which is /// the precedence). A missing prefix or port range becomes the wildcard for that field. -fn lower_rules( +fn lower_rules( rules: &[PeeringAclRule], -) -> Vec<(Vec, LookupResult)> { +) -> Vec<(AclKeyRule, Vec, LookupResult)> { rules .iter() .filter_map(|rule| { + let (key_rule, fields) = rule_predicates( + rule.proto, + rule.src_vni, + rule.dst_vni, + rule.src_ip_range.unwrap_or_else(T::wildcard), + rule.dst_ip_range.unwrap_or_else(T::wildcard), + rule.src_port_range + .unwrap_or(lpm::prefix::with_ports::PORT_RANGE_WILDCARD), + rule.dst_port_range + .unwrap_or(lpm::prefix::with_ports::PORT_RANGE_WILDCARD), + )?; Some(( - rule_predicates( - rule.proto, - rule.src_vni, - rule.dst_vni, - rule.src_ip_range.unwrap_or_else(T::wildcard), - rule.dst_ip_range.unwrap_or_else(T::wildcard), - rule.src_port_range - .unwrap_or(lpm::prefix::with_ports::PORT_RANGE_WILDCARD), - rule.dst_port_range - .unwrap_or(lpm::prefix::with_ports::PORT_RANGE_WILDCARD), - )?, + key_rule, + fields, LookupResult { action: rule.action, log: rule.log, @@ -308,13 +317,26 @@ pub(super) enum Backend { Reference, } -/// A built ACL table for one IP version. +/// A classifier and its rules for one IP version. +pub(super) struct AnyTable { + classifier: Classifier, + /// Typed rules in precedence order, retained because rte_acl cannot expose installed rules. + rules: Box<[RuleRow]>, +} + +/// A typed rule and its action, retained for display. +pub(super) struct RuleRow { + pub(super) rule: K::Rule, + pub(super) action: A, +} + +/// The classifier backing a table. /// /// `Empty` matches nothing -- used for the default context and any zero-rule table, so we never /// ask rte_acl to build an empty context. `Dpdk` is the production rte_acl classifier. `Reference` /// is the `cfg(test)` linear-scan oracle that drives the fast, EAL-free semantic suite. -#[allow(clippy::large_enum_variant)] // test only -pub(super) enum AnyTable { +#[allow(clippy::large_enum_variant)] // backend reprs differ in size; boxing would add an indirection +enum Classifier { Empty, Dpdk(DpdkAclLookup), #[cfg(test)] @@ -322,44 +344,42 @@ pub(super) enum AnyTable { } impl AnyTable { + /// A table that matches nothing. + fn empty() -> Self { + Self { + classifier: Classifier::Empty, + rules: Box::new([]), + } + } + /// Single-key lookup -- the per-packet production path (acl-filter classifies one packet at a /// time rather than in batches). fn lookup(&self, key: &K) -> Option<&A> { - match self { - AnyTable::Empty => None, - AnyTable::Dpdk(table) => table.lookup(key), + match &self.classifier { + Classifier::Empty => None, + Classifier::Dpdk(table) => table.lookup(key), #[cfg(test)] - AnyTable::Reference(table) => table.lookup(key), + Classifier::Reference(table) => table.lookup(key), } } pub(super) fn len(&self) -> usize { - match self { - AnyTable::Empty => 0, - AnyTable::Dpdk(table) => table.actions().len(), - #[cfg(test)] - AnyTable::Reference(table) => table.len(), - } + self.rules.len() } - /// The reference-backend rules, for the full CLI dump; `None` for the (opaque) rte_acl and - /// empty tables. - #[cfg(test)] - pub(super) fn reference_rules(&self) -> Option<&[RefRule]> { - match self { - AnyTable::Reference(table) => Some(table.rules()), - _ => None, - } + /// The rules this table was built from, in precedence (first-match) order. + pub(super) fn rules(&self) -> &[RuleRow] { + &self.rules } } impl fmt::Debug for AnyTable { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let kind = match self { - AnyTable::Empty => "empty", - AnyTable::Dpdk(_) => "dpdk", + let kind = match self.classifier { + Classifier::Empty => "empty", + Classifier::Dpdk(_) => "dpdk", #[cfg(test)] - AnyTable::Reference(_) => "reference", + Classifier::Reference(_) => "reference", }; write!(f, "AnyTable::{kind}({} rules)", self.len()) } @@ -378,24 +398,35 @@ fn table_name(base: &str) -> String { } /// Build one table for the selected backend from rules in precedence (insertion) order. -fn build_table( +fn build_table( backend: Backend, base_name: &str, - rules: Vec<(Vec, A)>, -) -> Result, String> { - match backend { + rules: Vec<(K::Rule, Vec, A)>, +) -> Result, String> +where + K::Rule: Copy, +{ + let rows: Box<[RuleRow]> = rules + .iter() + .map(|(rule, _, action)| RuleRow { + rule: *rule, + action: action.clone(), + }) + .collect(); + + let classifier = match backend { Backend::Dpdk => { // A zero-rule table matches nothing; represent it as `Empty` rather than asking // rte_acl to build an empty context. if rules.is_empty() { - return Ok(AnyTable::Empty); + return Ok(AnyTable::empty()); } let n = rules.len(); let max = NonZero::new(u32::try_from(n).unwrap_or(u32::MAX)) .ok_or_else(|| "zero-rule table reached the dpdk builder".to_string())?; let specs = K::field_specs(); let mut rule_specs = Vec::with_capacity(n); - for (i, (fields, action)) in rules.into_iter().enumerate() { + for (i, (_, fields, action)) in rules.into_iter().enumerate() { // Positional first-match: the first matching rule wins. rte_acl returns the // highest-priority match, so priority descends with insertion index (rule 0 gets // the highest priority). Distinct priorities keep the outcome deterministic. @@ -416,8 +447,8 @@ fn build_table( rule_specs.push(spec); } install_table::(&table_name(base_name), max, rule_specs) - .map(AnyTable::Dpdk) - .map_err(|e| e.to_string()) + .map(Classifier::Dpdk) + .map_err(|e| e.to_string())? } #[cfg(test)] Backend::Reference => { @@ -425,11 +456,15 @@ fn build_table( // precedence we want -- no priority sort needed. let rules = rules .into_iter() - .map(|(fields, action)| RefRule::new(fields, action)) + .map(|(_, fields, action)| RefRule::new(fields, action)) .collect(); - Ok(AnyTable::Reference(ReferenceTable::new(rules))) + Classifier::Reference(ReferenceTable::new(rules)) } - } + }; + Ok(AnyTable { + classifier, + rules: rows, + }) } #[derive(Debug, Clone)] @@ -449,8 +484,8 @@ pub(super) struct AclTables { impl Default for AclTables { fn default() -> Self { Self { - v4: AnyTable::Empty, - v6: AnyTable::Empty, + v4: AnyTable::empty(), + v6: AnyTable::empty(), default_actions: HashMap::new(), } } @@ -479,7 +514,7 @@ impl AclTables { impl AclTables { #[must_use] pub(super) fn lookup(&self, p: &PacketSummary) -> Option<&LookupResult> { - let proto = p.proto.as_u8(); + let proto = p.proto; let (src_ports, dst_ports) = p.ports.unzip(); match (p.src_ip, p.dst_ip) { (IpAddr::V4(src_ip), IpAddr::V4(dst_ip)) => { diff --git a/acl-filter/src/display.rs b/acl-filter/src/display.rs index b4b20b3e70..b2da8e680f 100644 --- a/acl-filter/src/display.rs +++ b/acl-filter/src/display.rs @@ -3,12 +3,8 @@ //! Display implementations for the ACL filter context. //! -//! In production (rte_acl backend) the rules are baked into an opaque classifier, so only a rule -//! count is shown per table. In test / `reference` builds the reference backend keeps the rules, so -//! each rule's field predicates and action are rendered in full by decoding the positional field -//! layout of the `AclKey` match key defined in `context.rs`: -//! -//! - `AclKey`: proto, src_vni, dst_vni, src_ip, dst_ip, src_port, dst_port +//! Tables retain typed rules for backend-independent display. Each field's type controls its +//! formatting, keeping values coupled to their key fields. use common::cliprovider::{CliSource, Heading}; @@ -16,7 +12,8 @@ use std::fmt::{self, Display, Write}; use crate::AclFilterContext; use crate::PacketSummary; -use crate::context::AclTables; +use crate::context::{AclTables, AnyTable, RuleRow}; +use match_action::{Field, MatchKey, RuleFields, write_grid}; impl CliSource for AclFilterContext {} @@ -27,8 +24,7 @@ impl Display for AclFilterContext { } } -/// Dump the peering default actions, sorted for a deterministic rendering. Shared by both the -/// production (count-only) and test (full) table renderings. +/// Dump the peering default actions in deterministic order. fn fmt_default_actions(w: &mut W, tables: &AclTables) -> fmt::Result { writeln!(w, "default actions:")?; let mut w = indenter::indented(w).with_str(" "); @@ -44,64 +40,55 @@ fn fmt_default_actions(w: &mut W, tables: &AclTables) -> fmt::Result { } // ------------------------------------------------------------------------------------------------- -// Production (rte_acl / opaque): a rule count per table. +// Rendering: one section per IP version, each rule on a line, in precedence (first-match) order. -#[cfg(not(test))] -impl Display for AclTables { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - writeln!(f, "IPv4: {} rules", self.v4.len())?; - writeln!(f, "IPv6: {} rules", self.v6.len())?; - fmt_default_actions(f, self) - } -} - -// ------------------------------------------------------------------------------------------------- -// Test / `reference` builds: full per-rule rendering from the reference backend. - -#[cfg(test)] impl Display for AclTables { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { use indenter::indented; writeln!(f, "IPv4:")?; - fmt_ref_table(&mut indented(f).with_str(" "), &self.v4)?; + fmt_table(&mut indented(f).with_str(" "), &self.v4)?; writeln!(f, "IPv6:")?; - fmt_ref_table(&mut indented(f).with_str(" "), &self.v6)?; + fmt_table(&mut indented(f).with_str(" "), &self.v6)?; fmt_default_actions(f, self) } } -/// Format a single table as a numbered list of rules, decoding each rule's predicates by position -/// (`AclKey` layout: proto, src_vni, dst_vni, src_ip, dst_ip, src_port, dst_port). -#[cfg(test)] -fn fmt_ref_table( +/// Format a single table as a grid of rules, in precedence (first-match) order. +/// +/// The rank column is the rule's position: the first match wins, so `[0]` is consulted first. Key +/// columns come from the rule's own fields, with a `|` before the action columns so it is obvious +/// which half is matched on and which half is the result. +fn fmt_table( w: &mut W, - table: &crate::context::AnyTable, -) -> fmt::Result { - let Some(rules) = table.reference_rules() else { - // rte_acl / empty tables are opaque; fall back to a count. - return writeln!(w, "({} rules)", table.len()); - }; - if rules.is_empty() { + table: &AnyTable, +) -> fmt::Result +where + K::Rule: RuleFields, +{ + if table.len() == 0 { return writeln!(w, "(none)"); } - for (idx, rule) in rules.iter().enumerate() { - let fields = rule.fields(); - let result = rule.action(); - let proto = decode_proto(fields.first()); - let src_vni = decode_vni(fields.get(1)); - let dst_vni = decode_vni(fields.get(2)); - let src_ip = decode_prefix(fields.get(3)); - let dst_ip = decode_prefix(fields.get(4)); - let src_ports = decode_ports(fields.get(5)); - let dst_ports = decode_ports(fields.get(6)); - let log = if result.log { ", log" } else { "" }; - writeln!( - w, - "[{idx}] VPC {src_vni} -> VPC {dst_vni} | proto {proto} | src {src_ip}:{src_ports} | dst {dst_ip}:{dst_ports} | {:?} ({:?}{log})", - result.action, result.scope - )?; + + let key_fields = ::FIELD_NAMES; + let mut headings: Vec<&str> = Vec::with_capacity(key_fields.len() + 5); + headings.push("rank"); + headings.extend_from_slice(key_fields); + headings.extend_from_slice(&["|", "action", "scope", "log"]); + + let mut rows: Vec> = Vec::with_capacity(table.len()); + for (rank, RuleRow { rule, action }) in table.rules().iter().enumerate() { + let mut row = Vec::with_capacity(headings.len()); + row.push(format!("[{rank}]")); + for index in 0..key_fields.len() { + row.push(Field::of(rule, index).to_string()); + } + row.push("|".to_string()); + row.push(format!("{:?}", action.action)); + row.push(format!("{:?}", action.scope)); + row.push(if action.log { "log" } else { "-" }.to_string()); + rows.push(row); } - Ok(()) + write_grid(w, &headings, &rows) } impl Display for PacketSummary { @@ -121,120 +108,3 @@ impl Display for PacketSummary { } } } - -/// Decode a VNI stored as a 4-byte big-endian exact-match predicate. -#[cfg(test)] -fn decode_vni(predicate: Option<&match_action::FieldPredicate>) -> String { - match predicate.and_then(match_action::FieldPredicate::as_exact) { - Some([a, b, c, d]) => u32::from_be_bytes([*a, *b, *c, *d]).to_string(), - _ => "?".to_string(), - } -} - -/// Decode an IP prefix stored as a prefix-match predicate (4 bytes for IPv4, 16 bytes for IPv6, -/// plus a prefix length). -#[cfg(test)] -fn decode_prefix(predicate: Option<&match_action::FieldPredicate>) -> String { - use std::net::{Ipv4Addr, Ipv6Addr}; - match predicate.and_then(match_action::FieldPredicate::as_prefix) { - Some(([a, b, c, d], len)) => format!("{}/{len}", Ipv4Addr::new(*a, *b, *c, *d)), - Some((bytes, len)) if bytes.len() == 16 => { - let mut octets = [0u8; 16]; - octets.copy_from_slice(bytes); - format!("{}/{len}", Ipv6Addr::from(octets)) - } - _ => "?".to_string(), - } -} - -/// Decode an IP protocol stored as a 1-byte bitmask predicate. A zero mask (wildcard) renders as -/// `any`, a full mask as the single protocol number. -#[cfg(test)] -fn decode_proto(predicate: Option<&match_action::FieldPredicate>) -> String { - match predicate.and_then(match_action::FieldPredicate::as_mask) { - Some(([value], [mask])) => { - if *mask == 0 { - "any".to_string() - } else if *mask == u8::MAX { - value.to_string() - } else { - format!("{value}&{mask:#04x}") - } - } - _ => "?".to_string(), - } -} - -/// Decode a port range stored as a pair of 2-byte big-endian range bounds. A full range renders as -/// `*`, an exact match as the single port. -#[cfg(test)] -fn decode_ports(predicate: Option<&match_action::FieldPredicate>) -> String { - match predicate.and_then(match_action::FieldPredicate::as_range) { - Some(([lo_hi, lo_lo], [hi_hi, hi_lo])) => { - let lo = u16::from_be_bytes([*lo_hi, *lo_lo]); - let hi = u16::from_be_bytes([*hi_hi, *hi_lo]); - if lo == 0 && hi == u16::MAX { - "*".to_string() - } else if lo == hi { - lo.to_string() - } else { - format!("{lo}-{hi}") - } - } - _ => "?".to_string(), - } -} - -#[cfg(test)] -mod tests { - use super::{decode_ports, decode_prefix, decode_proto, decode_vni}; - use match_action::{Erased, ExactSpec, IntoBackendField, MaskSpec, PrefixSpec, RangeSpec}; - use std::net::{Ipv4Addr, Ipv6Addr}; - - #[test] - fn decode_vni_reads_u32() { - let p = IntoBackendField::::into_backend_field(ExactSpec::new(4242u32)); - assert_eq!(decode_vni(Some(&p)), "4242"); - assert_eq!(decode_vni(None), "?"); - } - - #[test] - fn decode_prefix_reads_v4_and_v6() { - let v4 = IntoBackendField::::into_backend_field(PrefixSpec::new( - Ipv4Addr::new(10, 0, 0, 0), - 8, - )); - assert_eq!(decode_prefix(Some(&v4)), "10.0.0.0/8"); - - let v6 = IntoBackendField::::into_backend_field(PrefixSpec::new( - Ipv6Addr::new(0x2001, 0xdb8, 0, 0, 0, 0, 0, 0), - 32, - )); - assert_eq!(decode_prefix(Some(&v6)), "2001:db8::/32"); - } - - #[test] - fn decode_ports_handles_wildcard_exact_and_range() { - let wildcard = - IntoBackendField::::into_backend_field(RangeSpec::new(0u16, u16::MAX)); - assert_eq!(decode_ports(Some(&wildcard)), "*"); - - let exact = IntoBackendField::::into_backend_field(RangeSpec::new(80u16, 80u16)); - assert_eq!(decode_ports(Some(&exact)), "80"); - - let range = IntoBackendField::::into_backend_field(RangeSpec::new(80u16, 8080u16)); - assert_eq!(decode_ports(Some(&range)), "80-8080"); - } - - #[test] - fn decode_proto_handles_any_and_exact() { - let any = IntoBackendField::::into_backend_field(MaskSpec::new(0u8, 0u8)); - assert_eq!(decode_proto(Some(&any)), "any"); - - let tcp = IntoBackendField::::into_backend_field(MaskSpec::new(6u8, u8::MAX)); - assert_eq!(decode_proto(Some(&tcp)), "6"); - - let udp = IntoBackendField::::into_backend_field(MaskSpec::new(17u8, u8::MAX)); - assert_eq!(decode_proto(Some(&udp)), "17"); - } -} diff --git a/acl-filter/src/tests.rs b/acl-filter/src/tests.rs index 6e5401ab57..c7a29f71dd 100644 --- a/acl-filter/src/tests.rs +++ b/acl-filter/src/tests.rs @@ -1053,6 +1053,85 @@ mod dpdk_backend { ); } + /// Retained typed rules must render identically across backends. + #[test] + #[dpdk::with_eal] + fn display_is_identical_across_backends() { + let acl = Acl::new( + AclAction::Deny, + vec![rule( + "allow-tcp", + AclAction::Allow, + AclScope::Packet, + pattern(&[V1_IPS], &[V2_IPS], AclProtoMatch::Tcp), + )], + ); + let overlay = overlay( + &[("vpc1", VNI1), ("vpc2", VNI2)], + vec![peering( + "vpc1-to-vpc2", + ("vpc1", vec![expose(V1_IPS)]), + ("vpc2", vec![expose(V2_IPS)]), + Some(acl), + )], + ); + + let reference = AclFilterContext::for_test(&overlay); + let dpdk = AclFilterContext::for_test_dpdk(&overlay).expect("rte_acl backend build"); + assert_eq!(reference.to_string(), dpdk.to_string()); + + // Assert cell values without pinning widths, which depend on every value in the table. + let dump = dpdk.to_string(); + let rule_row = dump + .lines() + .find(|line| line.trim_start().starts_with("[0]")) + .unwrap_or_else(|| panic!("no rule row in:\n{dump}")); + let cells: Vec<&str> = rule_row.split_whitespace().collect(); + assert_eq!( + cells, + [ + "[0]", + "TCP", + &VNI1.to_string(), + &VNI2.to_string(), + V1_IPS, + V2_IPS, + "*", + "*", + "|", + "Allow", + "Packet", + "log", + ], + "unexpected rule rendering:\n{dump}" + ); + + // The heading row names the same columns, in the same order, so a reader can tell which + // value is which without counting fields against the key definition. + let headings = dump + .lines() + .find(|line| line.trim_start().starts_with("rank")) + .unwrap_or_else(|| panic!("no heading row in:\n{dump}")); + assert_eq!( + headings.split_whitespace().collect::>(), + [ + "rank", + "proto", + "src-vni", + "dst-vni", + "source", + "destination", + "src-port", + "dst-port", + "|", + "action", + "scope", + "log", + ], + "unexpected heading row:\n{dump}" + ); + } + #[test] #[dpdk::with_eal] fn dpdk_agrees_with_reference() { diff --git a/config/src/external/overlay/acl.rs b/config/src/external/overlay/acl.rs index add75d6fb6..342a06fc33 100644 --- a/config/src/external/overlay/acl.rs +++ b/config/src/external/overlay/acl.rs @@ -27,21 +27,18 @@ pub enum AclProtoMatch { Any, } -const TCP: u8 = NextHeader::TCP.as_u8(); -const UDP: u8 = NextHeader::UDP.as_u8(); - -/// Lower a protocol match to a `(value, mask)` bitmask predicate on the 1-byte protocol key field. -/// A specific protocol matches exactly (`mask 0xff`); `Any` wildcards the field (`mask 0x00`), so a -/// single key field expresses "any protocol" without fanning rules across per-protocol tables. This -/// is also what lets the protocol byte be the rte_acl-mandated 1-byte first field (a `#[mask]` byte -/// lowers to the same `Bitmask` field type as `#[exact]`). -impl From for MaskSpec { +/// Lower a protocol match to a bitmask predicate on the 1-byte protocol key field. A specific +/// protocol matches exactly (every bit significant); `Any` wildcards the field (no bit +/// significant), so a single key field expresses "any protocol" without fanning rules across +/// per-protocol tables. This is also what lets the protocol byte be the rte_acl-mandated 1-byte +/// first field (a `#[mask]` byte lowers to the same `Bitmask` field type as `#[exact]`). +impl From for MaskSpec { fn from(proto: AclProtoMatch) -> Self { match proto { - AclProtoMatch::Tcp => MaskSpec::new(TCP, u8::MAX), - AclProtoMatch::Udp => MaskSpec::new(UDP, u8::MAX), - AclProtoMatch::Other(p) => MaskSpec::new(p, u8::MAX), - AclProtoMatch::Any => MaskSpec::new(0, 0), + AclProtoMatch::Tcp => MaskSpec::exact(NextHeader::TCP), + AclProtoMatch::Udp => MaskSpec::exact(NextHeader::UDP), + AclProtoMatch::Other(p) => MaskSpec::exact(NextHeader::new(p)), + AclProtoMatch::Any => MaskSpec::wildcard(), } } } diff --git a/flow-filter/src/context/display.rs b/flow-filter/src/context/display.rs index 63cda56daa..b9561c9bc6 100644 --- a/flow-filter/src/context/display.rs +++ b/flow-filter/src/context/display.rs @@ -3,9 +3,8 @@ //! Display implementations for the routing context tables. //! -//! In production (rte_acl backend) the rules are baked into an opaque classifier, so only a rule -//! count is shown per table. In test / `reference`-feature builds the reference backend keeps the -//! rules, so the field predicates + action are rendered in full. +//! Tables retain typed rules for backend-independent display. Each field's type controls its +//! formatting, keeping values coupled to their key fields. use super::tables::FlowFilterContext; @@ -26,151 +25,105 @@ impl std::fmt::Display for crate::NatRequirement { } // ------------------------------------------------------------------------------------------------- -// Production (rte_acl / opaque): a one-line summary per table. +// Rendering: one section per table, each rule on a line, in match order. -#[cfg(not(test))] mod render { - use super::FlowFilterContext; - use common::cliprovider::{CliSource, Heading}; - use std::fmt::{self, Display, Formatter}; - - impl CliSource for FlowFilterContext {} - - impl Display for FlowFilterContext { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - Heading("Routing context (flow filter)").fmt(f)?; - writeln!(f, "remote v4: {} rules", self.remote_v4.len())?; - writeln!(f, "local v4: {} rules", self.local_v4.len())?; - writeln!(f, "remote v6: {} rules", self.remote_v6.len())?; - writeln!(f, "local v6: {} rules", self.local_v6.len()) - } - } -} - -// ------------------------------------------------------------------------------------------------- -// Test / `reference` builds: full per-rule rendering when the reference backend holds the rules. - -#[cfg(test)] -mod render { - use super::super::tables::{AnyTable, Verdict}; + use super::super::tables::{AnyTable, RuleRow, Verdict}; use super::FlowFilterContext; use crate::NatRequirement; - use common::cliprovider::Heading; + use common::cliprovider::{CliSource, Heading}; use indenter::indented; - use match_action::{FieldPredicate, MatchKey}; + use match_action::{Field, MatchKey, RuleFields, write_grid}; use std::fmt::{self, Display, Formatter, Write}; - use std::net::{Ipv4Addr, Ipv6Addr}; + + impl CliSource for FlowFilterContext {} impl Display for FlowFilterContext { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { Heading("Routing context (flow filter)").fmt(f)?; - writeln!(f, "remote v4 (destination -> dst VPC + dst NAT):")?; + writeln!(f, "Remote v4 (destination -> dst VPC + dst NAT):")?; write!(indented(f).with_str(" "), "{}", Table(&self.remote_v4))?; - writeln!(f, "local v4 (source -> src NAT):")?; + writeln!(f, "Local v4 (source -> src NAT):")?; write!(indented(f).with_str(" "), "{}", Table(&self.local_v4))?; - writeln!(f, "remote v6 (destination -> dst VPC + dst NAT):")?; + writeln!(f, "Remote v6 (destination -> dst VPC + dst NAT):")?; write!(indented(f).with_str(" "), "{}", Table(&self.remote_v6))?; - writeln!(f, "local v6 (source -> src NAT):")?; + writeln!(f, "Local v6 (source -> src NAT):")?; write!(indented(f).with_str(" "), "{}", Table(&self.local_v6)) } } struct Table<'a, K: MatchKey, A>(&'a AnyTable); - impl Display for Table<'_, K, A> { + /// Rules as a table in match order: `[0]` is consulted first. + /// + /// The rank column is the rule's position, not its internal priority value. A priority is a + /// computed encoding of (prefix length, port-forwarding bit) that means nothing outside the + /// table builder and is not stable across releases -- printing it invites an operator to read + /// precedence out of an opaque number, or to compare two numbers whose scale may have changed + /// underneath them. The rank answers the question they actually have: which rule wins. + /// + /// Key columns come from the rule's own fields, action columns from [`ActionColumns`], with a + /// `|` between the two so it is obvious which half is matched on and which half is the result. + impl Display for Table<'_, K, A> + where + K::Rule: RuleFields, + { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - let Some(rules) = self.0.reference_rules() else { - return writeln!(f, "({} rules)", self.0.len()); - }; - if rules.is_empty() { + if self.0.len() == 0 { return writeln!(f, "(no rules)"); } - for rule in rules { - for (i, pred) in rule.fields().iter().enumerate() { - if i > 0 { - write!(f, ", ")?; - } - fmt_predicate(f, pred)?; - } - write!(f, " -> ")?; - rule.action().fmt_action(f)?; - writeln!(f)?; - } - Ok(()) - } - } - fn fmt_predicate(f: &mut Formatter<'_>, pred: &FieldPredicate) -> fmt::Result { - if let Some((bytes, len)) = pred.as_prefix() { - match bytes.len() { - 4 => write!( - f, - "{}/{len}", - Ipv4Addr::from(<[u8; 4]>::try_from(bytes).unwrap()) - ), - 16 => write!( - f, - "{}/{len}", - Ipv6Addr::from(<[u8; 16]>::try_from(bytes).unwrap()) - ), - _ => write!(f, "{bytes:02x?}/{len}"), - } - } else if let Some((min, max)) = pred.as_range() { - let (Some(lo), Some(hi)) = (read_u16(min), read_u16(max)) else { - return write!(f, "range {min:02x?}..={max:02x?}"); - }; - if lo == 0 && hi == u16::MAX { - write!(f, "ports *") - } else if lo == hi { - write!(f, "port {lo}") - } else { - write!(f, "ports {lo}..={hi}") - } - } else if let Some(bytes) = pred.as_exact() { - write!(f, "{bytes:02x?}") - } else if let Some((value, mask)) = pred.as_mask() { - if mask.iter().all(|&b| b == 0) { - write!(f, "*") - } else { - write!(f, "{value:02x?}/{mask:02x?}") + let key_fields = ::FIELD_NAMES; + let mut headings: Vec<&str> = + Vec::with_capacity(key_fields.len() + A::HEADINGS.len() + 2); + headings.push("rank"); + headings.extend_from_slice(key_fields); + headings.push("|"); + headings.extend_from_slice(A::HEADINGS); + + let mut rows: Vec> = Vec::with_capacity(self.0.len()); + for (rank, RuleRow { rule, action }) in self.0.rules().iter().enumerate() { + let mut row = Vec::with_capacity(headings.len()); + row.push(format!("[{rank}]")); + for index in 0..key_fields.len() { + row.push(Field::of(rule, index).to_string()); + } + row.push("|".to_string()); + action.columns(&mut row); + rows.push(row); } - } else { - Ok(()) + write_grid(f, &headings, &rows) } } - fn read_u16(bytes: &[u8]) -> Option { - if let [hi, lo] = bytes { - Some(u16::from_be_bytes([*hi, *lo])) - } else { - None - } + /// An action, as the columns it occupies. + /// + /// A trait rather than `Display` because one action type is the alias + /// `Option`, which this crate cannot implement `Display` for -- and because a + /// columnar layout needs the cells separately anyway. + trait ActionColumns { + const HEADINGS: &'static [&'static str]; + fn columns(&self, row: &mut Vec); } - // Dedicated trait (rather than `Display`) because one action type is the alias - // `Option`, for which we cannot implement `Display`. - trait ActionDisplay { - fn fmt_action(&self, f: &mut Formatter<'_>) -> fmt::Result; - } + impl ActionColumns for Verdict { + const HEADINGS: &'static [&'static str] = &["to", "NAT"]; - impl ActionDisplay for Verdict { - fn fmt_action(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "{}, NAT: ", self.dst_vpcd)?; - fmt_nat_mode(f, &self.nat_mode) + fn columns(&self, row: &mut Vec) { + row.push(self.dst_vpcd.to_string()); + row.push(nat_mode(&self.nat_mode)); } } - impl ActionDisplay for Option { - fn fmt_action(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "NAT: ")?; - fmt_nat_mode(f, self) + impl ActionColumns for Option { + const HEADINGS: &'static [&'static str] = &["NAT"]; + + fn columns(&self, row: &mut Vec) { + row.push(nat_mode(self)); } } - fn fmt_nat_mode(f: &mut Formatter<'_>, nat: &Option) -> fmt::Result { - match nat { - Some(nat) => write!(f, "{nat}"), - None => write!(f, "-"), - } + fn nat_mode(nat: &Option) -> String { + nat.map_or_else(|| "-".to_string(), |nat| nat.to_string()) } } diff --git a/flow-filter/src/context/tables.rs b/flow-filter/src/context/tables.rs index 490e3ad9f0..d8e98fabf5 100644 --- a/flow-filter/src/context/tables.rs +++ b/flow-filter/src/context/tables.rs @@ -47,7 +47,6 @@ use match_action::{ use net::ip::NextHeader; use net::packet::VpcDiscriminant; use net::vxlan::Vni; -#[cfg(test)] use std::cmp::Reverse; use std::fmt; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; @@ -134,10 +133,13 @@ pub(super) struct RemoteKey { #[mask] proto: NextHeader, #[exact] + #[cli(column_name = "src-vni")] src_vni: Vni, #[prefix] + #[cli(column_name = "destination")] dst_ip: I, #[range] + #[cli(column_name = "dst-port")] dst_port: u16, } @@ -147,12 +149,16 @@ pub(super) struct LocalKey { #[mask] proto: NextHeader, #[exact] + #[cli(column_name = "src-vni")] src_vni: Vni, #[exact] + #[cli(column_name = "dst-vni")] dst_vni: Vni, #[prefix] + #[cli(column_name = "source")] src_ip: I, #[range] + #[cli(column_name = "src-port")] src_port: u16, } @@ -175,17 +181,40 @@ pub(super) enum Backend { Reference, } -/// One backend-neutral, already-lowered rule: priority, field predicates, action. -struct NeutralRule { +/// One backend-neutral, already-lowered rule. +/// +/// `fields` is the erased form the backends consume; `rule` is the same rule with its field types +/// intact, which is what the table retains for the CLI (see [`RuleRow`]). +struct NeutralRule { priority: u32, fields: Vec, + rule: K::Rule, action: A, } -/// A built table. The `Dpdk` variant is production (and exposes `lookup_batch` for the batched +/// A typed rule and its action, retained for display in match order. +/// +/// The rule's priority is deliberately not carried here. It is an internal encoding of +/// (prefix length, port-forwarding bit) -- see [`rule_priority`] -- with no meaning outside this +/// module and no stability across releases. Position in [`AnyTable::rules`] already expresses the +/// only thing the priority says that an operator can act on: which rule is consulted first. +pub(super) struct RuleRow { + pub(super) rule: K::Rule, + pub(super) action: A, +} + +/// A classifier and its rules. +/// +/// Typed rules are retained because rte_acl cannot expose installed rules. +pub(super) struct AnyTable { + classifier: Classifier, + rules: Box<[RuleRow]>, +} + +/// The classifier backing a table. `Dpdk` is production (and exposes `lookup_batch` for the batched /// fast path); `Reference` is the test/opt-in linear-scan oracle; `Empty` matches nothing. #[allow(clippy::large_enum_variant)] // backend reprs differ in size; boxing would add a hot-path indirection -pub(super) enum AnyTable { +enum Classifier { /// No rules: every lookup misses. Used for the default context and zero-rule tables (avoids /// asking rte_acl to build an empty context). Empty, @@ -195,26 +224,34 @@ pub(super) enum AnyTable { } impl AnyTable { + /// A table that matches nothing. + pub(super) fn empty() -> Self { + Self { + classifier: Classifier::Empty, + rules: Box::new([]), + } + } + // Single-key lookup: only the test oracle uses it (production runs lookup_batch()). #[cfg(test)] fn lookup(&self, key: &K) -> Option<&A> { - match self { - AnyTable::Empty => None, - AnyTable::Dpdk(table) => table.lookup(key), - AnyTable::Reference(table) => table.lookup(key), + match &self.classifier { + Classifier::Empty => None, + Classifier::Dpdk(table) => table.lookup(key), + Classifier::Reference(table) => table.lookup(key), } } /// Classify a batch of keys (`keys.len() <= MAX_BATCH`, `out.len() == keys.len()`), writing one /// result per key. The `Dpdk` backend does this in a single rte_acl call; the others loop. fn lookup_batch<'a>(&'a self, keys: &[K], out: &mut [Option<&'a A>]) { - match self { - AnyTable::Empty => out.iter_mut().for_each(|slot| *slot = None), - AnyTable::Dpdk(table) => table + match &self.classifier { + Classifier::Empty => out.iter_mut().for_each(|slot| *slot = None), + Classifier::Dpdk(table) => table .lookup_batch(keys, out) .expect("caller chunks to MAX_BATCH with a matching output length"), #[cfg(test)] - AnyTable::Reference(table) => { + Classifier::Reference(table) => { for (key, slot) in keys.iter().zip(out.iter_mut()) { *slot = table.lookup(key); } @@ -223,31 +260,22 @@ impl AnyTable { } pub(super) fn len(&self) -> usize { - match self { - AnyTable::Empty => 0, - AnyTable::Dpdk(table) => table.actions().len(), - #[cfg(test)] - AnyTable::Reference(table) => table.len(), - } + self.rules.len() } - /// The reference-backend rules, for display; `None` for the (opaque) rte_acl / empty tables. - #[cfg(test)] - pub(super) fn reference_rules(&self) -> Option<&[RefRule]> { - match self { - AnyTable::Reference(table) => Some(table.rules()), - _ => None, - } + /// The rules this table was built from, in match order: index 0 is consulted first. + pub(super) fn rules(&self) -> &[RuleRow] { + &self.rules } } impl fmt::Debug for AnyTable { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let kind = match self { - AnyTable::Empty => "empty", - AnyTable::Dpdk(_) => "dpdk", + let kind = match self.classifier { + Classifier::Empty => "empty", + Classifier::Dpdk(_) => "dpdk", #[cfg(test)] - AnyTable::Reference(_) => "reference", + Classifier::Reference(_) => "reference", }; write!(f, "AnyTable::{kind}({} rules)", self.len()) } @@ -268,17 +296,36 @@ fn table_name(base: &str) -> String { } /// Build one table from backend-neutral rules using the selected backend. -fn build_table( +fn build_table( backend: Backend, base_name: &str, - rules: Vec>, -) -> Result, String> { - match backend { + mut rules: Vec>, +) -> Result, String> +where + K::Rule: Copy, +{ + // Descending priority is match order. The reference backend needs it (it is first-match, so + // this is what reproduces rte_acl's highest-priority-wins), and the CLI dump reads top-down. + // rte_acl takes each priority explicitly, so the order is immaterial to it. + // + // The sort is stable, so equal-priority rules keep the order the overlay walk emitted them in. + // That only decides the *display* order among them: rules that can both match a packet never + // share a priority (see rule_priority), so ties are rules that partition the key space. + rules.sort_by_key(|rule| Reverse(rule.priority)); + let rows: Box<[RuleRow]> = rules + .iter() + .map(|rule| RuleRow { + rule: rule.rule, + action: rule.action, + }) + .collect(); + + let classifier = match backend { Backend::Dpdk => { // A zero-rule table matches nothing; represent it as Empty rather than asking rte_acl // to build an empty context if rules.is_empty() { - return Ok(AnyTable::Empty); + return Ok(AnyTable::empty()); } let specs = K::field_specs(); let max = NonZero::new(u32::try_from(rules.len()).unwrap_or(u32::MAX)).unwrap(); @@ -304,22 +351,22 @@ fn build_table( ); } install_table::(&table_name(base_name), max, specs_out) - .map(AnyTable::Dpdk) - .map_err(|e| e.to_string()) + .map(Classifier::Dpdk) + .map_err(|e| e.to_string())? } #[cfg(test)] Backend::Reference => { - // The reference backend is first-match on insertion order; sort descending by priority - // so first-match reproduces rte_acl's highest-priority-wins (longest-prefix-match) - let mut rules = rules; - rules.sort_by_key(|r| Reverse(r.priority)); let rules = rules .into_iter() .map(|r| RefRule::new(r.fields, r.action)) .collect(); - Ok(AnyTable::Reference(ReferenceTable::new(rules))) + Classifier::Reference(ReferenceTable::new(rules)) } - } + }; + Ok(AnyTable { + classifier, + rules: rows, + }) } // ------------------------------------------------------------------------------------------------- @@ -343,8 +390,8 @@ fn rule_priority(ip_range: Prefix, port_forwarding: bool) -> u32 { /// Lower a stage-1 (remote) rule into the v4 or v6 bucket according to its prefix. #[allow(clippy::too_many_arguments)] // internal builder; grouping the fields would not aid clarity fn emit_remote( - v4: &mut Vec>, - v6: &mut Vec>, + v4: &mut Vec, Verdict>>, + v6: &mut Vec, Verdict>>, src_vni: Vni, ip_range: Prefix, port_range: RangeSpec, @@ -357,30 +404,30 @@ fn emit_remote( ); match ip_range { Prefix::IPV4(prefix) => { - let fields = RemoteKeyRule:: { + let rule = RemoteKeyRule:: { proto, src_vni: ExactSpec::new(src_vni), dst_ip: PrefixSpec::from(prefix), dst_port: port_range, - } - .into_backend_fields::(); + }; v4.push(NeutralRule { priority, - fields, + fields: rule.into_backend_fields::(), + rule, action, }); } Prefix::IPV6(prefix) => { - let fields = RemoteKeyRule:: { + let rule = RemoteKeyRule:: { proto, src_vni: ExactSpec::new(src_vni), dst_ip: PrefixSpec::from(prefix), dst_port: port_range, - } - .into_backend_fields::(); + }; v6.push(NeutralRule { priority, - fields, + fields: rule.into_backend_fields::(), + rule, action, }); } @@ -390,8 +437,8 @@ fn emit_remote( /// Lower a stage-2 (local) rule into the v4 or v6 bucket according to its prefix. #[allow(clippy::too_many_arguments)] // internal builder; grouping the fields would not aid clarity fn emit_local( - v4: &mut Vec>, - v6: &mut Vec>, + v4: &mut Vec, NatMode>>, + v6: &mut Vec, NatMode>>, src_vni: Vni, dst_vni: Vni, ip_range: Prefix, @@ -404,32 +451,32 @@ fn emit_local( let priority = rule_priority(ip_range, false); match ip_range { Prefix::IPV4(prefix) => { - let fields = LocalKeyRule:: { + let rule = LocalKeyRule:: { proto, src_vni: ExactSpec::new(src_vni), dst_vni: ExactSpec::new(dst_vni), src_ip: PrefixSpec::from(prefix), src_port: port_range, - } - .into_backend_fields::(); + }; v4.push(NeutralRule { priority, - fields, + fields: rule.into_backend_fields::(), + rule, action, }); } Prefix::IPV6(prefix) => { - let fields = LocalKeyRule:: { + let rule = LocalKeyRule:: { proto, src_vni: ExactSpec::new(src_vni), dst_vni: ExactSpec::new(dst_vni), src_ip: PrefixSpec::from(prefix), src_port: port_range, - } - .into_backend_fields::(); + }; v6.push(NeutralRule { priority, - fields, + fields: rule.into_backend_fields::(), + rule, action, }); } @@ -438,10 +485,10 @@ fn emit_local( #[derive(Default)] struct RuleSet { - remote_v4: Vec>, - remote_v6: Vec>, - local_v4: Vec>, - local_v6: Vec>, + remote_v4: Vec, Verdict>>, + remote_v6: Vec, Verdict>>, + local_v4: Vec, NatMode>>, + local_v6: Vec, NatMode>>, } impl RuleSet { @@ -552,10 +599,10 @@ pub struct FlowFilterContext { impl Default for FlowFilterContext { fn default() -> Self { Self { - remote_v4: AnyTable::Empty, - local_v4: AnyTable::Empty, - remote_v6: AnyTable::Empty, - local_v6: AnyTable::Empty, + remote_v4: AnyTable::empty(), + local_v4: AnyTable::empty(), + remote_v6: AnyTable::empty(), + local_v6: AnyTable::empty(), } } } diff --git a/flow-filter/src/context/tests.rs b/flow-filter/src/context/tests.rs index 7ad6d71d79..c94dcd49bb 100644 --- a/flow-filter/src/context/tests.rs +++ b/flow-filter/src/context/tests.rs @@ -6,6 +6,7 @@ #![cfg(test)] use super::LookupResult; +use super::tables::RuleRow; use crate::test_utils::*; use crate::{FlowFilterContext, NatMode, NatRequirement}; use lpm::prefix::L4Protocol; @@ -565,16 +566,12 @@ fn discrepancy_overlapping_contiguous_prefixes() { assert_eq!(r.dst_nat, None); // Check there are no /32 prefixes in the remote-side rules - let rules = ctx.remote_v4.reference_rules().unwrap(); - for rule in rules { - let dst_prefix = rule.fields()[2].as_prefix().unwrap(); - assert_ne!(dst_prefix.1, 32); + for RuleRow { rule, .. } in ctx.remote_v4.rules() { + assert_ne!(rule.dst_ip.len, 32); } // Check there are no /32 prefixes in the local-side rules - let rules = ctx.local_v4.reference_rules().unwrap(); - for rule in rules { - let src_prefix = rule.fields()[3].as_prefix().unwrap(); - assert_ne!(src_prefix.1, 32); + for RuleRow { rule, .. } in ctx.local_v4.rules() { + assert_ne!(rule.src_ip.len, 32); } } @@ -757,3 +754,130 @@ fn reference_and_dpdk_backends_agree() { assert_eq!(ref_out[i], single, "batched != single at index {i}"); } } + +/// Retained typed rules must render identically across backends. +#[test] +#[dpdk::with_eal] +fn display_is_identical_across_backends() { + use super::tables::{Backend, FlowFilterContext}; + + let ov = overlay( + &[("vpc1", 100), ("vpc2", 200)], + vec![peering( + "vpc1-to-vpc2", + ("vpc1", vec![expose("10.0.0.0/24")]), + ( + "vpc2", + vec![ + expose("90.0.0.0/24"), + expose_masquerade("192.168.70.0/24", "70.0.0.0/24"), + expose_port_forwarding( + "192.168.80.5/32", + (22, 22), + "80.0.0.5/32", + (2222, 2222), + Some(L4Protocol::Tcp), + ), + ], + ), + )], + ); + + let reference = FlowFilterContext::build(&ov, Backend::Reference).expect("reference build"); + let dpdk = FlowFilterContext::build(&ov, Backend::Dpdk).expect("dpdk build"); + assert_eq!(reference.to_string(), dpdk.to_string()); + + // Assert cell values without pinning widths, which depend on every value in the table. + let dump = dpdk.to_string(); + + // Every assertion below is scoped to one section. The four tables share column names and some + // of their values, so a search over the whole dump answers with the first table's rows whatever + // it was asked about, and the later tables go unchecked. + // + // A section heading is written at the left margin with its table indented under it, so a + // section ends at the first line that is not indented. Finding the end that way means a test + // that reads one section does not have to name the section that follows it. + let section = |heading: &str| -> String { + let mut lines = dump.lines().skip_while(|line| !line.starts_with(heading)); + let head = lines + .next() + .unwrap_or_else(|| panic!("no {heading:?} section in:\n{dump}")); + std::iter::once(head) + .chain(lines.take_while(|line| line.starts_with(" "))) + .collect::>() + .join("\n") + }; + let cells = |section: &str, prefix: &str| -> Vec { + section + .lines() + .find(|line| line.trim_start().starts_with(prefix)) + .unwrap_or_else(|| panic!("no row starting {prefix:?} in:\n{section}")) + .split_whitespace() + .map(str::to_string) + .collect() + }; + let remote_v4 = section("Remote v4"); + let local_v4 = section("Local v4"); + + assert_eq!( + cells(&remote_v4, "rank"), + [ + "rank", + "proto", + "src-vni", + "destination", + "dst-port", + "|", + "to", + "NAT" + ], + "unexpected heading row:\n{remote_v4}" + ); + assert_eq!( + cells(&remote_v4, "[0]"), + [ + "[0]", + "TCP", + "100", + "80.0.0.5/32", + "2222", + "|", + "VNI(200)", + "port-forwarding" + ], + "unexpected rule rendering:\n{remote_v4}" + ); + + // The local table too: its key carries a second VNI and its action is a bare NAT mode, so it + // exercises a different `ActionColumns` impl. + assert_eq!( + cells(&local_v4, "rank"), + [ + "rank", "proto", "src-vni", "dst-vni", "source", "src-port", "|", "NAT" + ], + "unexpected local heading row:\n{local_v4}" + ); + + // The heading assertions above would still pass if a section ran past its own table, since they + // read its first heading row and stop. The ordering assertion below would not: it compares + // positions, so a section that swallowed the table after it could order two rules that are not + // even in the same table and call the result precedence. Pin the boundary directly -- `dst-vni` + // is a local-table column, and the local table is the one that follows. + assert!( + !remote_v4.contains("dst-vni"), + "the remote v4 section ran past its own table:\n{remote_v4}" + ); + + // The index is the operator-facing precedence claim -- `[0]` is consulted first -- so the dump + // must read in match order. Within one table that is longest-prefix-first: the port-forwarding + // /32 outranks the /24s it is nested among. + let rank = |needle: &str| { + remote_v4 + .find(needle) + .unwrap_or_else(|| panic!("{needle} missing from the remote v4 table:\n{remote_v4}")) + }; + assert!( + rank("80.0.0.5/32") < rank("70.0.0.0/24") && rank("80.0.0.5/32") < rank("90.0.0.0/24"), + "rules are not rendered in precedence order:\n{remote_v4}" + ); +} diff --git a/match-action-derive/src/lib.rs b/match-action-derive/src/lib.rs index d755d95d99..d11fe60f7d 100644 --- a/match-action-derive/src/lib.rs +++ b/match-action-derive/src/lib.rs @@ -67,7 +67,7 @@ impl Kind { /// `Display` is mandatory because a classifier an operator cannot inspect is one they cannot /// debug; a field type missing it shows up as an unsatisfied `MaskSpec: Display` bound on the /// generated rule struct. -#[proc_macro_derive(MatchKey, attributes(prefix, mask, range, exact, phantom))] +#[proc_macro_derive(MatchKey, attributes(prefix, mask, range, exact, phantom, cli))] pub fn derive_match_key(input: TokenStream) -> TokenStream { let input = parse_macro_input!(input as DeriveInput); match expand(&input) { @@ -245,11 +245,12 @@ fn expand(input: &DeriveInput) -> syn::Result { #crate_path::#spec<#ty>: #crate_path::IsUniversal }); // Each field renders as `name=spec`, comma-separated, in key order. The name comes from - // the struct field itself, so the label can never drift from the value it labels. - let name_str = name.to_string(); + // the struct field itself, or its `#[cli(column_name = "...")]`, so the label can never + // drift from the value it labels. + let display_name = parse_field_column_name(field)?.unwrap_or_else(|| name.to_string()); let separator = if i == 0 { "" } else { ", " }; rule_field_displays.push(quote! { - ::core::write!(f, "{}{}={}", #separator, #name_str, self.#name)?; + ::core::write!(f, "{}{}={}", #separator, #display_name, self.#name)?; }); rule_field_display_bounds.push(quote! { #crate_path::#spec<#ty>: ::core::fmt::Display @@ -257,7 +258,7 @@ fn expand(input: &DeriveInput) -> syn::Result { // The same fields again, reachable one at a time: a columnar caller needs each value's // width before it can pad, and the whole-rule `Display` above hands back a finished // string. See `match_action::RuleFields`. - rule_field_names.push(quote! { #name_str }); + rule_field_names.push(quote! { #display_name }); rule_field_fmt_arms.push(quote! { #i => ::core::fmt::Display::fmt(&self.#name, f), }); @@ -429,6 +430,43 @@ fn is_phantom_data_type(ty: &Type) -> bool { false } +/// The heading a field is shown under, from `#[cli(column_name = "...")]`, or `None` for the +/// field's own name. +/// +/// Only display is affected: the field keeps its identifier everywhere else, including in +/// `FieldSpec`, which describes the key's structure to the layout planner. +/// +/// This keeps a heading next to the field it heads rather than in a lookup table in whichever +/// crate happens to be rendering. +fn parse_field_column_name(field: &Field) -> syn::Result> { + let mut found: Option = None; + for attr in &field.attrs { + if !attr.path().is_ident("cli") { + continue; + } + attr.parse_nested_meta(|meta| { + if !meta.path.is_ident("column_name") { + return Err(meta.error( + r#"unrecognised #[cli(..)] option; the only one is column_name = "...""#, + )); + } + if found.is_some() { + return Err(meta.error(r#"duplicate #[cli(column_name = "...")] on one field"#)); + } + let value: syn::LitStr = meta.value()?.parse()?; + let name = value.value(); + if name.is_empty() { + return Err( + meta.error("column_name cannot be empty; omit it to use the field's own name") + ); + } + found = Some(name); + Ok(()) + })?; + } + Ok(found) +} + fn parse_field_role(field: &Field) -> syn::Result { let mut has_phantom_attr = false; let mut found: Option<(Kind, &Attribute)> = None; diff --git a/match-action/src/display.rs b/match-action/src/display.rs index 19d7f47a64..e756965f78 100644 --- a/match-action/src/display.rs +++ b/match-action/src/display.rs @@ -19,7 +19,9 @@ use crate::rule::{ExactSpec, MaskSpec, PrefixSpec, RangeSpec}; /// [`FIELD_NAMES`](RuleFields::FIELD_NAMES) in key order indexing /// [`fmt_field`](RuleFields::fmt_field) to measure and align individual fields. pub trait RuleFields { - /// The rule's field names, in key order. + /// The name each field is displayed under, in key order. + /// + /// The field's own identifier, unless it carries `#[cli(column_name = "...")]`. const FIELD_NAMES: &'static [&'static str]; /// Render the field at `index`, or fail if there is no such field. diff --git a/match-action/tests/derive_roundtrip.rs b/match-action/tests/derive_roundtrip.rs index 7d00349992..e23f875ad1 100644 --- a/match-action/tests/derive_roundtrip.rs +++ b/match-action/tests/derive_roundtrip.rs @@ -396,3 +396,63 @@ fn wrapper_field_bounds_the_field_type_not_its_parameter() { tagged: ExactSpec::new(Tag(0xDEAD_BEEF, PhantomData)), }; } + +/// `#[cli(column_name = "...")]` changes only how a field is *displayed*. +/// +/// The point is that a column heading lives next to the field it heads, rather than in a lookup +/// table in whichever crate happens to be rendering. Without it, a consumer has to map field names +/// to headings by string, which duplicates the mapping in every consumer and silently falls back to +/// a raw identifier when a field is renamed. +mod column_name { + use core::net::Ipv4Addr; + use dataplane_match_action::{ExactSpec, Field, MatchKey, PrefixSpec, RuleFields}; + + #[derive(Debug, MatchKey, Clone, PartialEq, Eq)] + struct Key { + #[exact] + proto: u8, + #[exact] + #[cli(column_name = "src-vni")] + src_vni: u32, + #[prefix] + #[cli(column_name = "destination")] + dst_ip: Ipv4Addr, + } + + fn rule() -> KeyRule { + KeyRule { + proto: ExactSpec::new(6u8), + src_vni: ExactSpec::new(100u32), + dst_ip: PrefixSpec::new(Ipv4Addr::new(10, 0, 0, 0), 24), + } + } + + #[test] + fn named_fields_report_their_heading_and_the_rest_their_identifier() { + assert_eq!(KeyRule::FIELD_NAMES, ["proto", "src-vni", "destination"]); + } + + /// The whole-rule `Display` uses the same labels, so the two renderings cannot disagree about + /// what a field is called. + #[test] + fn the_whole_rule_display_uses_the_same_labels() { + assert_eq!( + rule().to_string(), + "proto=6, src-vni=100, destination=10.0.0.0/24" + ); + } + + /// Renaming is display-only: field order, values, and the key encoding are untouched. + #[test] + fn naming_a_column_does_not_disturb_the_key() { + let values: Vec = (0..KeyRule::FIELD_NAMES.len()) + .map(|i| Field::of(&rule(), i).to_string()) + .collect(); + assert_eq!(values, ["6", "100", "10.0.0.0/24"]); + + // The runtime field specs keep the *identifiers*: they describe the key's structure, which + // a display label has no business changing. + let spec_names: Vec<&str> = Key::field_specs().iter().map(|spec| spec.name).collect(); + assert_eq!(spec_names, ["proto", "src_vni", "dst_ip"]); + } +}