Skip to content

Commit 58da74b

Browse files
qmonnetclaude
andcommitted
refactor(net): Drop the Uni<T> wrapper around packets
Uni<T> existed to tag a packet for unidirectional FlowKey extraction back when bidirectional keys were also supported. Bidirectional keys are gone, so Uni is not longer necessary, it simply adds dead weight that every call site has to thread through, and makes the code less clear. Convert the TryFrom impl to accept &Packet<Buf> directly, delete the Uni struct, and drop the wrapper at the call sites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
1 parent 7f868d7 commit 58da74b

7 files changed

Lines changed: 44 additions & 51 deletions

File tree

flow-entry/src/flow_table/nf_lookup.rs

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ use pipeline::NetworkFunction;
1212

1313
use crate::flow_table::FlowTable;
1414
use net::FlowKey;
15-
use net::flow_key;
1615

1716
use tracectl::trace_target;
1817
trace_target!("flow-lookup", LevelFilter::INFO, &["pipeline"]);
@@ -40,7 +39,7 @@ impl<Buf: PacketBufferMut> NetworkFunction<Buf> for FlowLookup {
4039
input.filter_map(move |mut packet| {
4140
let nfi = &self.name;
4241
if !packet.is_done() && packet.meta().is_overlay() && packet.meta().dst_vpcd.is_none() {
43-
if let Ok(flow_key) = FlowKey::try_from(flow_key::Uni(&packet)) {
42+
if let Ok(flow_key) = FlowKey::try_from(&packet) {
4443
if let Some(flow_info) = self.flow_table.lookup(&flow_key) {
4544
debug!("{nfi}: Tagging packet with flow info for flow key {flow_key}",);
4645
packet.meta_mut().flow_info = Some(flow_info);
@@ -101,7 +100,7 @@ mod test {
101100
packet.meta_mut().set_overlay(true);
102101

103102
// Insert matching flow entry
104-
let flow_key = FlowKey::try_from(net::flow_key::Uni(&packet)).unwrap();
103+
let flow_key = FlowKey::try_from(&packet).unwrap();
105104
let flow_info = FlowInfo::new(flow_key, Instant::now() + Duration::from_secs(10));
106105
flow_table.insert(flow_info).unwrap();
107106

@@ -130,7 +129,7 @@ mod test {
130129
input: Input,
131130
) -> impl Iterator<Item = Packet<Buf>> + 'a {
132131
input.filter_map(move |packet| {
133-
let flow_key = FlowKey::try_from(net::flow_key::Uni(&packet)).unwrap();
132+
let flow_key = FlowKey::try_from(&packet).unwrap();
134133
let flow_info = FlowInfo::new(flow_key, Instant::now() + self.timeout);
135134
self.flow_table
136135
.insert(flow_info)
@@ -193,8 +192,8 @@ mod test {
193192
packet_2.meta_mut().set_overlay(true);
194193

195194
// build keys for the packets
196-
let key_1 = FlowKey::try_from(net::flow_key::Uni(&packet_1)).unwrap();
197-
let key_2 = FlowKey::try_from(net::flow_key::Uni(&packet_2)).unwrap();
195+
let key_1 = FlowKey::try_from(&packet_1).unwrap();
196+
let key_2 = FlowKey::try_from(&packet_2).unwrap();
198197

199198
// create a pair of related flow entries; flow_2 will get a longer timeout
200199
let expires_at = tokio::time::Instant::now().into_std() + Duration::from_secs(2);
@@ -251,8 +250,8 @@ mod test {
251250
let mut packet_2 = build_test_udp_ipv4_packet("192.168.1.1", "20.0.0.1", 500, 80);
252251
packet_1.meta_mut().set_overlay(true);
253252
packet_2.meta_mut().set_overlay(true);
254-
let key_1 = FlowKey::try_from(net::flow_key::Uni(&packet_1)).unwrap();
255-
let key_2 = FlowKey::try_from(net::flow_key::Uni(&packet_2)).unwrap();
253+
let key_1 = FlowKey::try_from(&packet_1).unwrap();
254+
let key_2 = FlowKey::try_from(&packet_2).unwrap();
256255
let input = vec![packet_1, packet_2];
257256
let out: Vec<_> = pipeline.process(input.into_iter()).collect();
258257
let packet_1 = &out[0];

flow-filter/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,7 @@ impl FlowFilter {
369369
return;
370370
}
371371

372-
let Ok(flow_key) = FlowKey::try_from(net::flow_key::Uni(&*packet)) else {
372+
let Ok(flow_key) = FlowKey::try_from(&*packet) else {
373373
return;
374374
};
375375

flow-filter/src/tests.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ use config::external::overlay::vpcpeering::{VpcExpose, VpcManifest, VpcPeering,
1212
use lpm::prefix::{L4Protocol, PortRange, Prefix, PrefixWithOptionalPorts};
1313
use net::FlowKey;
1414
use net::buffer::{PacketBufferMut, TestBuffer};
15-
use net::flow_key::Uni;
1615
use net::flows::{FlowInfo, FlowStatus};
1716
use net::headers::{Net, TryHeadersMut, TryIpMut};
1817
use net::ip::NextHeader;
@@ -191,7 +190,7 @@ fn fake_flow_session<Buf: PacketBufferMut>(
191190
set_port_fw_state: bool,
192191
) {
193192
// build flow key
194-
let flow_key = FlowKey::try_from(Uni(&*packet)).unwrap();
193+
let flow_key = FlowKey::try_from(&*packet).unwrap();
195194

196195
// Create flow_info with dst_vpcd and NAT info and attach it to the packet
197196
let flow_info = FlowInfo::new(flow_key, Instant::now() + Duration::from_secs(60));

nat/src/portfw/flow_state.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
#![allow(clippy::single_match_else)]
77

88
use net::buffer::PacketBufferMut;
9-
use net::flow_key::Uni;
109
use net::flows::{ExtractRef, FlowStatus};
1110
use net::ip::UnicastIpAddr;
1211
use net::packet::{Packet, VpcDiscriminant};
@@ -107,7 +106,7 @@ pub(crate) fn build_portfw_flow_keys<Buf: PacketBufferMut>(
107106
dst_vpcd: VpcDiscriminant, // destination VPC to forward to
108107
) -> Result<(FlowKey, FlowKey), ()> {
109108
// Extract flow key for the current packet
110-
let current_flow_key = FlowKey::try_from(Uni(&*packet)).map_err(|_| ())?;
109+
let current_flow_key = FlowKey::try_from(&*packet).map_err(|_| ())?;
111110

112111
// Retrieve initial flow key for the current packet (before any other NAT translation); if
113112
// we don't have the information, we didn't populate it because we don't need it and fall

nat/src/stateful/nf.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use crate::stateful::state::MasqueradeState;
1616
use concurrency::sync::{Arc, Weak};
1717
use flow_entry::flow_table::table::{FlowTable, FlowTableError};
1818
use net::buffer::PacketBufferMut;
19-
use net::flow_key::{IcmpProtoKey, Uni};
19+
use net::flow_key::IcmpProtoKey;
2020
use net::flows::{ExtractRef, FlowInfo};
2121
use net::headers::{TryIp, TryTcp};
2222
use net::ip::UnicastIpAddr;
@@ -375,7 +375,7 @@ impl StatefulNat {
375375

376376
// Extract flow key for the current packet
377377
let current_flow_key =
378-
FlowKey::try_from(Uni(&*packet)).map_err(|_| StatefulNatError::FlowKeyError)?;
378+
FlowKey::try_from(&*packet).map_err(|_| StatefulNatError::FlowKeyError)?;
379379

380380
// Retrieve initial flow key for the current packet (before any other NAT translation); if
381381
// we don't have the information, we didn't populate it because we don't need it and fall

nat/src/stateful/test.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ use flow_entry::flow_table::{FlowLookup, FlowTable};
1818
use flow_filter::{FlowFilter, FlowFilterTable, FlowFilterTableWriter};
1919
use net::buffer::{PacketBufferMut, TestBuffer};
2020
use net::eth::mac::Mac;
21-
use net::flow_key::Uni;
2221
use net::flows::FlowStatus;
2322
use net::flows::flow_info_item::ExtractRef;
2423
use net::headers::TryTcpMut;
@@ -366,7 +365,7 @@ fn check_packet(
366365
}
367366

368367
fn flow_lookup<Buf: PacketBufferMut>(flow_table: &FlowTable, packet: &mut Packet<Buf>) {
369-
let flow_key = FlowKey::try_from(Uni(&*packet)).unwrap();
368+
let flow_key = FlowKey::try_from(&*packet).unwrap();
370369
if let Some(flow_info) = flow_table.lookup(&flow_key) {
371370
packet.meta_mut().flow_info = Some(flow_info);
372371
}

net/src/flows/flow_key.rs

Lines changed: 31 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -585,40 +585,37 @@ impl Hash for FlowKey {
585585
}
586586
}
587587

588-
/// Wrapper to specify unidirectional `FlowKey` creation
589-
#[repr(transparent)]
590-
#[derive(Debug)]
591-
pub struct Uni<T>(pub T);
592-
593-
fn flow_key_from_packet<Buf: PacketBufferMut>(packet: &Packet<Buf>) -> Option<FlowKey> {
594-
let ip = packet.headers().try_ip()?;
595-
let src_ip = ip.src_addr();
596-
let dst_ip = ip.dst_addr();
597-
598-
let transport = packet.headers().try_transport()?;
599-
let ip_proto_key = match transport {
600-
Transport::Tcp(tcp) => IpProtoKey::Tcp(TcpProtoKey {
601-
src_port: tcp.source(),
602-
dst_port: tcp.destination(),
603-
}),
604-
Transport::Udp(udp) => IpProtoKey::Udp(UdpProtoKey {
605-
src_port: udp.source(),
606-
dst_port: udp.destination(),
607-
}),
608-
Transport::Icmp4(icmp) => IpProtoKey::Icmp(IcmpProtoKey::new_icmp_v4(packet, icmp)),
609-
Transport::Icmp6(icmp) => IpProtoKey::Icmp(IcmpProtoKey::new_icmp_v6(packet, icmp)),
610-
#[allow(unreachable_patterns)]
611-
_ => return None,
612-
};
588+
impl<Buf: PacketBufferMut> TryFrom<&Packet<Buf>> for FlowKey {
589+
type Error = FlowKeyError;
590+
fn try_from(packet: &Packet<Buf>) -> Result<Self, Self::Error> {
591+
let ip = packet
592+
.headers()
593+
.try_ip()
594+
.ok_or(FlowKeyError::NoFlowKeyData)?;
595+
let src_ip = ip.src_addr();
596+
let dst_ip = ip.dst_addr();
613597

614-
let src_vpcd = packet.meta().src_vpcd;
615-
Some(FlowKey::new(src_vpcd, src_ip, dst_ip, ip_proto_key))
616-
}
598+
let transport = packet
599+
.headers()
600+
.try_transport()
601+
.ok_or(FlowKeyError::NoFlowKeyData)?;
602+
let ip_proto_key = match transport {
603+
Transport::Tcp(tcp) => IpProtoKey::Tcp(TcpProtoKey {
604+
src_port: tcp.source(),
605+
dst_port: tcp.destination(),
606+
}),
607+
Transport::Udp(udp) => IpProtoKey::Udp(UdpProtoKey {
608+
src_port: udp.source(),
609+
dst_port: udp.destination(),
610+
}),
611+
Transport::Icmp4(icmp) => IpProtoKey::Icmp(IcmpProtoKey::new_icmp_v4(packet, icmp)),
612+
Transport::Icmp6(icmp) => IpProtoKey::Icmp(IcmpProtoKey::new_icmp_v6(packet, icmp)),
613+
#[allow(unreachable_patterns)]
614+
_ => return Err(FlowKeyError::NoFlowKeyData),
615+
};
617616

618-
impl<Buf: PacketBufferMut> TryFrom<Uni<&Packet<Buf>>> for FlowKey {
619-
type Error = FlowKeyError;
620-
fn try_from(packet: Uni<&Packet<Buf>>) -> Result<Self, Self::Error> {
621-
flow_key_from_packet(packet.0).ok_or(FlowKeyError::NoFlowKeyData)
617+
let src_vpcd = packet.meta().src_vpcd;
618+
Ok(FlowKey::new(src_vpcd, src_ip, dst_ip, ip_proto_key))
622619
}
623620
}
624621

@@ -1078,7 +1075,7 @@ mod tests {
10781075
.with_generator(FlowKeyAndPacket)
10791076
.for_each(|(flow_key, packet)| match flow_key {
10801077
Some(_) => {
1081-
let gen_flow_key = FlowKey::try_from(Uni(packet)).unwrap();
1078+
let gen_flow_key = FlowKey::try_from(packet).unwrap();
10821079
assert_eq!(
10831080
gen_flow_key,
10841081
flow_key.unwrap(),
@@ -1087,7 +1084,7 @@ mod tests {
10871084
);
10881085
}
10891086
None => {
1090-
assert!(FlowKey::try_from(Uni(packet)).is_err());
1087+
assert!(FlowKey::try_from(packet).is_err());
10911088
}
10921089
});
10931090
}

0 commit comments

Comments
 (0)