From 3bca3b9a518e3d592a6f5bd1cc79952e5c9e54ad Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum <55108558+WesleyRosenblum@users.noreply.github.com> Date: Tue, 7 Jan 2025 13:04:39 -0800 Subject: [PATCH] events: adding detail to DatagramDropped event (#2438) * events: adding detail to DatagramDropped event * formatting * add test * remove unread fields --- quic/s2n-quic-core/events/connection.rs | 6 +- quic/s2n-quic-core/src/event/generated.rs | 48 ++++++++++--- .../src/endpoint/initial.rs | 7 ++ quic/s2n-quic-transport/src/endpoint/mod.rs | 9 +++ .../src/tests/connection_migration.rs | 70 ++++++++++++++++++- quic/s2n-quic/src/tests/recorder.rs | 25 +++++++ 6 files changed, 154 insertions(+), 11 deletions(-) diff --git a/quic/s2n-quic-core/events/connection.rs b/quic/s2n-quic-core/events/connection.rs index cb77e33e4c..2001a1c74f 100644 --- a/quic/s2n-quic-core/events/connection.rs +++ b/quic/s2n-quic-core/events/connection.rs @@ -272,7 +272,11 @@ struct DatagramReceived { #[event("transport:datagram_dropped")] //= https://tools.ietf.org/id/draft-marx-qlog-event-definitions-quic-h3-02#5.3.12 /// Datagram dropped by a connection -struct DatagramDropped { +struct DatagramDropped<'a> { + local_addr: SocketAddress<'a>, + remote_addr: SocketAddress<'a>, + destination_cid: ConnectionId<'a>, + source_cid: Option>, #[measure("bytes", Bytes)] #[counter("bytes.total", Bytes)] len: u16, diff --git a/quic/s2n-quic-core/src/event/generated.rs b/quic/s2n-quic-core/src/event/generated.rs index bc0d15f37c..f3e27d4d1d 100644 --- a/quic/s2n-quic-core/src/event/generated.rs +++ b/quic/s2n-quic-core/src/event/generated.rs @@ -2289,20 +2289,28 @@ pub mod api { #[derive(Clone, Debug)] #[non_exhaustive] #[doc = " Datagram dropped by a connection"] - pub struct DatagramDropped { + pub struct DatagramDropped<'a> { + pub local_addr: SocketAddress<'a>, + pub remote_addr: SocketAddress<'a>, + pub destination_cid: ConnectionId<'a>, + pub source_cid: Option>, pub len: u16, pub reason: DatagramDropReason, } #[cfg(any(test, feature = "testing"))] - impl crate::event::snapshot::Fmt for DatagramDropped { + impl<'a> crate::event::snapshot::Fmt for DatagramDropped<'a> { fn fmt(&self, fmt: &mut core::fmt::Formatter) -> core::fmt::Result { let mut fmt = fmt.debug_struct("DatagramDropped"); + fmt.field("local_addr", &self.local_addr); + fmt.field("remote_addr", &self.remote_addr); + fmt.field("destination_cid", &self.destination_cid); + fmt.field("source_cid", &self.source_cid); fmt.field("len", &self.len); fmt.field("reason", &self.reason); fmt.finish() } } - impl Event for DatagramDropped { + impl<'a> Event for DatagramDropped<'a> { const NAME: &'static str = "transport:datagram_dropped"; } #[derive(Clone, Debug)] @@ -3902,8 +3910,15 @@ pub mod tracing { event: &api::DatagramDropped, ) { let id = context.id(); - let api::DatagramDropped { len, reason } = event; - tracing :: event ! (target : "datagram_dropped" , parent : id , tracing :: Level :: DEBUG , { len = tracing :: field :: debug (len) , reason = tracing :: field :: debug (reason) }); + let api::DatagramDropped { + local_addr, + remote_addr, + destination_cid, + source_cid, + len, + reason, + } = event; + tracing :: event ! (target : "datagram_dropped" , parent : id , tracing :: Level :: DEBUG , { local_addr = tracing :: field :: debug (local_addr) , remote_addr = tracing :: field :: debug (remote_addr) , destination_cid = tracing :: field :: debug (destination_cid) , source_cid = tracing :: field :: debug (source_cid) , len = tracing :: field :: debug (len) , reason = tracing :: field :: debug (reason) }); } #[inline] fn on_connection_id_updated( @@ -5931,15 +5946,30 @@ pub mod builder { } #[derive(Clone, Debug)] #[doc = " Datagram dropped by a connection"] - pub struct DatagramDropped { + pub struct DatagramDropped<'a> { + pub local_addr: SocketAddress<'a>, + pub remote_addr: SocketAddress<'a>, + pub destination_cid: ConnectionId<'a>, + pub source_cid: Option>, pub len: u16, pub reason: DatagramDropReason, } - impl IntoEvent for DatagramDropped { + impl<'a> IntoEvent> for DatagramDropped<'a> { #[inline] - fn into_event(self) -> api::DatagramDropped { - let DatagramDropped { len, reason } = self; + fn into_event(self) -> api::DatagramDropped<'a> { + let DatagramDropped { + local_addr, + remote_addr, + destination_cid, + source_cid, + len, + reason, + } = self; api::DatagramDropped { + local_addr: local_addr.into_event(), + remote_addr: remote_addr.into_event(), + destination_cid: destination_cid.into_event(), + source_cid: source_cid.into_event(), len: len.into_event(), reason: reason.into_event(), } diff --git a/quic/s2n-quic-transport/src/endpoint/initial.rs b/quic/s2n-quic-transport/src/endpoint/initial.rs index 45572f60b3..f3959fdc3c 100644 --- a/quic/s2n-quic-transport/src/endpoint/initial.rs +++ b/quic/s2n-quic-transport/src/endpoint/initial.rs @@ -338,6 +338,13 @@ impl endpoint::Endpoint { endpoint_context.event_subscriber, |publisher, _path| { publisher.on_datagram_dropped(event::builder::DatagramDropped { + local_addr: header.path.local_address().into_event(), + remote_addr: header.path.remote_address().into_event(), + destination_cid: datagram.destination_connection_id.into_event(), + source_cid: datagram + .source_connection_id + .as_ref() + .map(|cid| cid.into_event()), len: datagram.payload_len as u16, reason: err, }); diff --git a/quic/s2n-quic-transport/src/endpoint/mod.rs b/quic/s2n-quic-transport/src/endpoint/mod.rs index 2219e13e5e..d4314991d4 100644 --- a/quic/s2n-quic-transport/src/endpoint/mod.rs +++ b/quic/s2n-quic-transport/src/endpoint/mod.rs @@ -575,6 +575,15 @@ impl Endpoint { endpoint_context.event_subscriber, |publisher, _path| { publisher.on_datagram_dropped(event::builder::DatagramDropped { + local_addr: header.path.local_address().into_event(), + remote_addr: header.path.remote_address().into_event(), + destination_cid: datagram + .destination_connection_id + .into_event(), + source_cid: datagram + .source_connection_id + .as_ref() + .map(|cid| cid.into_event()), len: datagram.payload_len as u16, reason: datagram_drop_reason, }); diff --git a/quic/s2n-quic/src/tests/connection_migration.rs b/quic/s2n-quic/src/tests/connection_migration.rs index 862ab8d2a7..fb7e6d7321 100644 --- a/quic/s2n-quic/src/tests/connection_migration.rs +++ b/quic/s2n-quic/src/tests/connection_migration.rs @@ -4,7 +4,7 @@ use super::*; use s2n_codec::DecoderBufferMut; use s2n_quic_core::{ - event::api::Subject, + event::api::{DatagramDropReason, Subject}, packet::interceptor::{Datagram, Interceptor}, }; @@ -218,3 +218,71 @@ fn port_rebind_test() { fn ip_and_port_rebind_test() { run_test(|addr| rebind_ip(rebind_port(addr))); } + +// Changes the port of the second datagram received +#[derive(Default)] +struct RebindPortBeforeHandshakeConfirmed { + datagram_count: usize, + changed_port: bool, +} + +const REBIND_PORT: u16 = 55555; +impl Interceptor for RebindPortBeforeHandshakeConfirmed { + fn intercept_rx_remote_port(&mut self, _subject: &Subject, port: &mut u16) { + if self.datagram_count == 1 && !self.changed_port { + *port = REBIND_PORT; + self.changed_port = true; + } + } + + fn intercept_rx_datagram<'a>( + &mut self, + _subject: &Subject, + _datagram: &Datagram, + payload: DecoderBufferMut<'a>, + ) -> DecoderBufferMut<'a> { + self.datagram_count += 1; + payload + } +} + +/// Ensures that a datagram received from a client that changes +/// its port before the handshake is confirmed is dropped. +#[test] +fn rebind_before_handshake_confirmed() { + let model = Model::default(); + let subscriber = recorder::DatagramDropped::new(); + let datagram_dropped_events = subscriber.events(); + + test(model, move |handle| { + let server = Server::builder() + .with_io(handle.builder().build()?)? + .with_tls(SERVER_CERTS)? + .with_event((tracing_events(), subscriber))? + .with_random(Random::with_seed(456))? + .with_packet_interceptor(RebindPortBeforeHandshakeConfirmed::default())? + .start()?; + + let client = Client::builder() + .with_io(handle.builder().build()?)? + .with_tls(certificates::CERT_PEM)? + .with_event(tracing_events())? + .with_random(Random::with_seed(456))? + .start()?; + + let addr = start_server(server)?; + start_client(client, addr, Data::new(1000))?; + Ok(addr) + }) + .unwrap(); + + let datagram_dropped_events = datagram_dropped_events.lock().unwrap(); + + assert_eq!(1, datagram_dropped_events.len()); + let event = datagram_dropped_events.first().unwrap(); + assert!(matches!( + event.reason, + DatagramDropReason::ConnectionMigrationDuringHandshake { .. }, + )); + assert_eq!(REBIND_PORT, event.remote_addr.port()); +} diff --git a/quic/s2n-quic/src/tests/recorder.rs b/quic/s2n-quic/src/tests/recorder.rs index a8bc332fc5..ec5e15fce6 100644 --- a/quic/s2n-quic/src/tests/recorder.rs +++ b/quic/s2n-quic/src/tests/recorder.rs @@ -165,3 +165,28 @@ event_recorder!( storage.push(addr); } ); + +use s2n_quic_core::event::api::DatagramDropReason; +pub struct DatagramDroppedEvent { + pub remote_addr: SocketAddr, + pub reason: DatagramDropReason, +} + +impl<'a> From<&events::DatagramDropped<'a>> for DatagramDroppedEvent { + fn from(value: &events::DatagramDropped<'a>) -> Self { + DatagramDroppedEvent { + remote_addr: value.remote_addr.to_string().parse().unwrap(), + reason: value.reason.clone(), + } + } +} + +event_recorder!( + DatagramDropped, + DatagramDropped, + on_datagram_dropped, + DatagramDroppedEvent, + |event: &events::DatagramDropped, storage: &mut Vec| { + storage.push(event.into()); + } +);