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/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/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/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/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 eafccebfa1..d8e98fabf5 100644 --- a/flow-filter/src/context/tables.rs +++ b/flow-filter/src/context/tables.rs @@ -46,7 +46,7 @@ use match_action::{ }; use net::ip::NextHeader; use net::packet::VpcDiscriminant; -#[cfg(test)] +use net::vxlan::Vni; use std::cmp::Reverse; use std::fmt; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; @@ -91,65 +91,55 @@ 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, + #[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, } @@ -157,14 +147,18 @@ pub(super) struct RemoteKey { #[derive(Debug, MatchKey, Clone, PartialEq, Eq)] pub(super) struct LocalKey { #[mask] - proto: u8, + proto: NextHeader, #[exact] - src_vpcd: u32, + #[cli(column_name = "src-vni")] + src_vni: Vni, #[exact] - dst_vpcd: u32, + #[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, } @@ -187,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, @@ -207,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); } @@ -235,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()) } @@ -280,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(); @@ -316,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, + }) } // ------------------------------------------------------------------------------------------------- @@ -355,12 +390,12 @@ 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>, - src_vpcd: u32, + v4: &mut Vec, Verdict>>, + v6: &mut Vec, Verdict>>, + src_vni: Vni, ip_range: Prefix, port_range: RangeSpec, - (proto_value, proto_mask): (u8, u8), + proto: MaskSpec, action: Verdict, ) { let priority = rule_priority( @@ -369,30 +404,30 @@ 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), + 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:: { - proto: MaskSpec::from((proto_value, proto_mask)), - src_vpcd: ExactSpec::new(src_vpcd), + 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, }); } @@ -402,13 +437,13 @@ 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>, - src_vpcd: u32, - dst_vpcd: u32, + v4: &mut Vec, NatMode>>, + v6: &mut Vec, NatMode>>, + 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 @@ -416,32 +451,32 @@ fn emit_local( let priority = rule_priority(ip_range, false); 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), + 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:: { - proto: MaskSpec::from((proto_value, proto_mask)), - src_vpcd: ExactSpec::new(src_vpcd), - dst_vpcd: ExactSpec::new(dst_vpcd), + 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, }); } @@ -450,20 +485,20 @@ 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 { 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 +522,7 @@ impl RuleSet { emit_remote( &mut rules.remote_v4, &mut rules.remote_v6, - src_vpcd, + src_vni, prefix.prefix(), prefix.into(), proto, @@ -499,10 +534,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 +559,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 +572,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, ); } @@ -564,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(), } } } @@ -610,8 +645,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 +654,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 +662,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 +676,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 +684,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 +719,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 +737,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 +773,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 +789,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 +820,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 +845,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/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 f310bc9a81..d11fe60f7d 100644 --- a/match-action-derive/src/lib.rs +++ b/match-action-derive/src/lib.rs @@ -54,7 +54,20 @@ impl Kind { } } -#[proc_macro_derive(MatchKey, attributes(prefix, mask, range, exact, phantom))] +/// 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, cli))] pub fn derive_match_key(input: TokenStream) -> TokenStream { let input = parse_macro_input!(input as DeriveInput); match expand(&input) { @@ -199,6 +212,10 @@ 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); + 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 @@ -227,6 +244,24 @@ 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, 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, #display_name, self.#name)?; + }); + 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! { #display_name }); + 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 @@ -279,6 +314,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 +329,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 +381,32 @@ 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(()) + } + } + 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) @@ -362,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/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 new file mode 100644 index 0000000000..e756965f78 --- /dev/null +++ b/match-action/src/display.rs @@ -0,0 +1,360 @@ +// 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}; + +/// 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 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. + /// + /// # 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) + } +} + +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 +/// `0x/0x`. +/// +/// 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); + 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 { + 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}")) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::Accepts; + 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(), "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()); + }); + } +} + +#[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/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..32aaa495f5 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; @@ -19,7 +20,8 @@ pub mod rule; #[cfg(feature = "bolero")] pub mod generator; -pub use field::FixedSize; +pub use display::{Field, RuleFields, write_grid}; +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, @@ -46,6 +48,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/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); diff --git a/match-action/tests/derive_roundtrip.rs b/match-action/tests/derive_roundtrip.rs index 96e341266e..e23f875ad1 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]) { @@ -390,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"]); + } +} 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` 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(