Skip to content

Commit

Permalink
fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
toidiu committed Jun 14, 2024
1 parent 2f2e433 commit f8d9971
Show file tree
Hide file tree
Showing 11 changed files with 57 additions and 56 deletions.
39 changes: 16 additions & 23 deletions quic/s2n-quic-core/src/path/mtu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ macro_rules! impl_mtu {

fn try_from(value: u16) -> Result<Self, Self::Error> {
if value < MINIMUM_MTU {
return Err(MtuError::Validation);
return Err(MtuError);
}

Ok($name(value.try_into().expect(
Expand Down Expand Up @@ -216,23 +216,16 @@ impl_mtu!(MaxMtu, DEFAULT_MAX_MTU);
impl_mtu!(InitialMtu, DEFAULT_INITIAL_MTU);
impl_mtu!(BaseMtu, DEFAULT_BASE_MTU);

#[non_exhaustive]
#[derive(Debug, Eq, PartialEq)]
pub enum MtuError {
Validation,
}
pub struct MtuError;

impl Display for MtuError {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
match self {
MtuError::Validation => {
write!(
f,
"MTU must have {} <= base_mtu (default: {}) <= initial_mtu (default: {}) <= max_mtu (default: {})",
MINIMUM_MTU, DEFAULT_BASE_MTU.0, DEFAULT_INITIAL_MTU.0, DEFAULT_MAX_MTU.0
)
}
}
write!(
f,
"MTU must have {} <= base_mtu (default: {}) <= initial_mtu (default: {}) <= max_mtu (default: {})",
MINIMUM_MTU, DEFAULT_BASE_MTU.0, DEFAULT_INITIAL_MTU.0, DEFAULT_MAX_MTU.0
)
}
}

Expand Down Expand Up @@ -326,10 +319,10 @@ pub struct CheckedConfig {
impl CheckedConfig {
// Check that the Application provided MTU values are valid for this endpoint.
pub fn new(config: Config, info: &mtu::PathInfo) -> Result<CheckedConfig, MtuError> {
ensure!(config.is_valid(), Err(MtuError::Validation));
ensure!(config.is_valid(), Err(MtuError));
ensure!(
u16::from(config.max_mtu) <= u16::from(info.endpoint_config.max_mtu),
Err(MtuError::Validation)
Err(MtuError)
);

Ok(CheckedConfig {
Expand Down Expand Up @@ -369,11 +362,11 @@ impl Builder {
/// [with_initial_mtu]: https://docs.rs/s2n-quic/latest/s2n_quic/provider/io/tokio/struct.Builder.html#method.with_initial_mtu
pub fn with_initial_mtu(mut self, initial_mtu: u16) -> Result<Self, MtuError> {
if let Some(base_mtu) = self.base_mtu {
ensure!(initial_mtu >= base_mtu.0.get(), Err(MtuError::Validation));
ensure!(initial_mtu >= base_mtu.0.get(), Err(MtuError));
}

if let Some(max_mtu) = self.max_mtu {
ensure!(initial_mtu <= max_mtu.0.get(), Err(MtuError::Validation));
ensure!(initial_mtu <= max_mtu.0.get(), Err(MtuError));
}

self.initial_mtu = Some(initial_mtu.try_into()?);
Expand All @@ -387,11 +380,11 @@ impl Builder {
/// [with_base_mtu]: https://docs.rs/s2n-quic/latest/s2n_quic/provider/io/tokio/struct.Builder.html#method.with_base_mtu
pub fn with_base_mtu(mut self, base_mtu: u16) -> Result<Self, MtuError> {
if let Some(initial_mtu) = self.initial_mtu {
ensure!(initial_mtu.0.get() >= base_mtu, Err(MtuError::Validation));
ensure!(initial_mtu.0.get() >= base_mtu, Err(MtuError));
}

if let Some(max_mtu) = self.max_mtu {
ensure!(base_mtu <= max_mtu.0.get(), Err(MtuError::Validation));
ensure!(base_mtu <= max_mtu.0.get(), Err(MtuError));
}

self.base_mtu = Some(base_mtu.try_into()?);
Expand All @@ -406,11 +399,11 @@ impl Builder {
/// [with_max_mtu]: https://docs.rs/s2n-quic/latest/s2n_quic/provider/io/tokio/struct.Builder.html#method.with_max_mtu
pub fn with_max_mtu(mut self, max_mtu: u16) -> Result<Self, MtuError> {
if let Some(initial_mtu) = self.initial_mtu {
ensure!(initial_mtu.0.get() <= max_mtu, Err(MtuError::Validation));
ensure!(initial_mtu.0.get() <= max_mtu, Err(MtuError));
}

if let Some(base_mtu) = self.base_mtu {
ensure!(base_mtu.0.get() <= max_mtu, Err(MtuError::Validation));
ensure!(base_mtu.0.get() <= max_mtu, Err(MtuError));
}

self.max_mtu = Some(max_mtu.try_into()?);
Expand Down Expand Up @@ -439,7 +432,7 @@ impl Builder {
base_mtu,
};

ensure!(config.is_valid(), Err(MtuError::Validation));
ensure!(config.is_valid(), Err(MtuError));
Ok(config)
}
}
Expand Down
20 changes: 10 additions & 10 deletions quic/s2n-quic-core/src/path/mtu/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,45 +80,45 @@ fn mtu_config_builder() {
.with_base_mtu(DEFAULT_MAX_MTU.0.get() + 1_u16)
.unwrap()
.build();
assert_eq!(Some(MtuError::Validation), result.err());
assert_eq!(Some(MtuError), result.err());

// Setting the initial MTU higher than the default max MTU results in an error
let builder = mtu::Config::builder();
let result = builder
.with_initial_mtu(DEFAULT_MAX_MTU.0.get() + 1_u16)
.unwrap()
.build();
assert_eq!(Some(MtuError::Validation), result.err());
assert_eq!(Some(MtuError), result.err());

// Setting the base MTU higher than the configured initial MTU results in an error
let builder = mtu::Config::builder();
let result = builder.with_initial_mtu(1300).unwrap().with_base_mtu(1301);
assert_eq!(Some(MtuError::Validation), result.err());
assert_eq!(Some(MtuError), result.err());

// Setting the max MTU lower than the configured initial MTU results in an error
let builder = mtu::Config::builder();
let result = builder.with_initial_mtu(1300).unwrap().with_max_mtu(1299);
assert_eq!(Some(MtuError::Validation), result.err());
assert_eq!(Some(MtuError), result.err());

// Setting the initial MTU lower than the configured base MTU results in an error
let builder = mtu::Config::builder();
let result = builder.with_base_mtu(1300).unwrap().with_initial_mtu(1299);
assert_eq!(Some(MtuError::Validation), result.err());
assert_eq!(Some(MtuError), result.err());

// Setting the initial MTU higher than the configured max MTU results in an error
let builder = mtu::Config::builder();
let result = builder.with_max_mtu(1300).unwrap().with_initial_mtu(1301);
assert_eq!(Some(MtuError::Validation), result.err());
assert_eq!(Some(MtuError), result.err());

// Setting the base MTU higher than the configured max MTU results in an error
let builder = mtu::Config::builder();
let result = builder.with_max_mtu(1300).unwrap().with_base_mtu(1301);
assert_eq!(Some(MtuError::Validation), result.err());
assert_eq!(Some(MtuError), result.err());

// Setting the max MTU lower than the configured base MTU results in an error
let builder = mtu::Config::builder();
let result = builder.with_base_mtu(1300).unwrap().with_max_mtu(1299);
assert_eq!(Some(MtuError::Validation), result.err());
assert_eq!(Some(MtuError), result.err());
}

#[test]
Expand Down Expand Up @@ -151,7 +151,7 @@ fn checked_mtu_config() {
assert!(!conn_config.is_valid());
assert_eq!(
CheckedConfig::new(conn_config, &info).unwrap_err(),
MtuError::Validation
MtuError
);

// invalid: conn_config.max_mtu > endpoint_config.max_mtu
Expand All @@ -163,7 +163,7 @@ fn checked_mtu_config() {
assert!(conn_config.is_valid());
assert_eq!(
CheckedConfig::new(conn_config, &info).unwrap_err(),
MtuError::Validation
MtuError
);
}

Expand Down
7 changes: 5 additions & 2 deletions quic/s2n-quic-transport/src/connection/connection_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -600,8 +600,11 @@ impl<Config: endpoint::Config> connection::Trait for ConnectionImpl<Config> {
parameters.limits.anti_amplification_multiplier(),
);

let max_mtu = parameters.mtu_config.endpoint.max_mtu();
let path_manager = path::Manager::new(initial_path, parameters.peer_id_registry, max_mtu);
let path_manager = path::Manager::new(
initial_path,
parameters.peer_id_registry,
parameters.mtu_config,
);

let mut publisher =
event_context.publisher(parameters.timestamp, parameters.event_subscriber);
Expand Down
4 changes: 2 additions & 2 deletions quic/s2n-quic-transport/src/endpoint/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pub trait Config: 'static + Send + Sized + core::fmt::Debug {
type EndpointLimits: endpoint::Limiter;
/// The connection limits
type ConnectionLimits: connection::limits::Limiter;
/// The connection specific mtu config
/// The path specific mtu config
type Mtu: s2n_quic_core::path::mtu::Endpoint;
/// The type of stream
type StreamManager: stream::Manager;
Expand Down Expand Up @@ -82,7 +82,7 @@ pub struct Context<'a, Cfg: Config> {
/// The connection limits
pub connection_limits: &'a mut Cfg::ConnectionLimits,

/// The connection specific mtu config
/// The path specific mtu config
pub mtu: &'a mut Cfg::Mtu,

pub connection_close_formatter: &'a mut Cfg::ConnectionCloseFormatter,
Expand Down
8 changes: 4 additions & 4 deletions quic/s2n-quic-transport/src/path/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,22 +70,22 @@ pub struct Manager<Config: endpoint::Config> {
/// The `paths` data structure will need to be enhanced to include garbage collection
/// of old paths to overcome this limitation.
pending_packet_authentication: Option<u8>,
endpoint_max_mtu: MaxMtu,
mtu_config: CheckedConfig,
}

impl<Config: endpoint::Config> Manager<Config> {
pub fn new(
initial_path: Path<Config>,
peer_id_registry: PeerIdRegistry,
max_mtu: MaxMtu,
mtu_config: CheckedConfig,
) -> Self {
let mut manager = Manager {
paths: SmallVec::from_elem(initial_path, 1),
peer_id_registry,
active: 0,
last_known_active_validated_path: None,
pending_packet_authentication: None,
endpoint_max_mtu: max_mtu,
mtu_config,
};
manager.paths[0].activated = true;
manager.paths[0].is_active = true;
Expand Down Expand Up @@ -862,7 +862,7 @@ impl<Config: endpoint::Config> Manager<Config> {
/// Returns the maximum size the UDP payload can reach for any probe packet.
#[inline]
pub fn max_mtu(&self) -> MaxMtu {
self.endpoint_max_mtu
self.mtu_config.endpoint.max_mtu()
}

#[cfg(test)]
Expand Down
4 changes: 2 additions & 2 deletions quic/s2n-quic-transport/src/path/manager/fuzz_target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ impl Model {
true,
);

Manager::new(zero_path, peer_id_registry)
Manager::new(zero_path, peer_id_registry, mtu::CheckedConfig::default())
};

let clock = Clock::default();
Expand Down Expand Up @@ -177,7 +177,7 @@ impl Model {
&mut Default::default(),
&mut migration_validator,
mtu::Config::default().into(),
&mut Default::default(),
&mut mtu::Config::default(),
&Limits::default(),
&mut publisher,
) {
Expand Down
13 changes: 7 additions & 6 deletions quic/s2n-quic-transport/src/path/manager/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,29 +38,30 @@ fn manager_server(first_path: ServerPath) -> ServerManager {
first_path.peer_connection_id,
true,
);
ServerManager::new(first_path, peer_id_registry)
ServerManager::new(first_path, peer_id_registry, mtu::CheckedConfig::default())
}

// Helper function to easily create a PathManager as a Client
fn manager_client(first_path: ClientPath) -> ClientManager {
let mut random_generator = random::testing::Generator(123);
let peer_id_registry = ConnectionIdMapper::new(&mut random_generator, endpoint::Type::Client)
.create_client_peer_id_registry(InternalConnectionIdGenerator::new().generate_id(), true);
ClientManager::new(first_path, peer_id_registry)
ClientManager::new(first_path, peer_id_registry, mtu::CheckedConfig::default())
}

#[test]
fn get_path_by_address_test() {
let first_conn_id = connection::PeerId::try_from_bytes(&[0, 1, 2, 3, 4, 5]).unwrap();
let first_local_conn_id = connection::LocalId::TEST_ID;
let mtu_config = mtu::Config::default().into();
let first_path = ServerPath::new(
Default::default(),
first_conn_id,
first_local_conn_id,
RttEstimator::default(),
Default::default(),
false,
mtu::Config::default().into(),
mtu_config,
ANTI_AMPLIFICATION_MULTIPLIER,
);

Expand All @@ -72,7 +73,7 @@ fn get_path_by_address_test() {
RttEstimator::default(),
Default::default(),
false,
mtu::Config::default().into(),
mtu_config,
ANTI_AMPLIFICATION_MULTIPLIER,
);

Expand Down Expand Up @@ -1735,7 +1736,7 @@ fn last_known_validated_path_should_update_on_path_response() {
.on_new_connection_id(&second_conn_id, 2, 0, &TEST_TOKEN_2)
.is_ok());

let mut manager = Manager::new(zero_path, peer_id_registry);
let mut manager = Manager::new(zero_path, peer_id_registry, mtu::CheckedConfig::default());

assert!(!manager[zero_path_id].is_challenge_pending());
assert!(manager[zero_path_id].is_validated());
Expand Down Expand Up @@ -1904,7 +1905,7 @@ pub fn helper_manager_with_paths_base(
.is_ok());
}

let mut manager = ServerManager::new(zero_path, peer_id_registry);
let mut manager = ServerManager::new(zero_path, peer_id_registry, CheckedConfig::default());
assert!(manager.peer_id_registry.is_active(&first_conn_id));
manager.paths.push(first_path);
manager.paths.push(second_path);
Expand Down
12 changes: 8 additions & 4 deletions quic/s2n-quic-transport/src/recovery/manager/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3426,18 +3426,20 @@ fn helper_generate_path_manager_with_first_addr(
);
let mut rtt_estimator = RttEstimator::default();
rtt_estimator.on_max_ack_delay(max_ack_delay.try_into().unwrap());

let mtu_config = mtu::Config::default().into();
let path = Path::new(
first_addr,
connection::PeerId::TEST_ID,
connection::LocalId::TEST_ID,
rtt_estimator,
MockCongestionController::new(first_addr),
true,
mtu::Config::default().into(),
mtu_config,
ANTI_AMPLIFICATION_MULTIPLIER,
);

path::Manager::new(path, registry)
path::Manager::new(path, registry, mtu_config)
}

fn helper_generate_client_path_manager(
Expand All @@ -3450,18 +3452,20 @@ fn helper_generate_client_path_manager(
.create_client_peer_id_registry(InternalConnectionIdGenerator::new().generate_id(), true);
let mut rtt_estimator = RttEstimator::default();
rtt_estimator.on_max_ack_delay(max_ack_delay.try_into().unwrap());

let mtu_config = mtu::Config::default().into();
let path = super::Path::new(
first_addr,
connection::PeerId::TEST_ID,
connection::LocalId::TEST_ID,
rtt_estimator,
MockCongestionController::new(first_addr),
false,
mtu::Config::default().into(),
mtu_config,
ANTI_AMPLIFICATION_MULTIPLIER,
);

path::Manager::new(path, registry)
path::Manager::new(path, registry, mtu_config)
}

struct MockContext<'a, Config: endpoint::Config> {
Expand Down
2 changes: 1 addition & 1 deletion quic/s2n-quic/src/client/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ impl<Providers: ClientProviders> Builder<Providers> {
);

impl_provider_method!(
/// Sets the connection specific mtu config provider for the [`Client`]
/// Sets the path specific mtu config provider for the [`Client`]
///
/// # Examples
///
Expand Down
2 changes: 1 addition & 1 deletion quic/s2n-quic/src/server/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ impl<Providers: ServerProviders> Builder<Providers> {
);

impl_provider_method!(
/// Sets the connection specific mtu config provider for the [`Server`]
/// Sets the path specific mtu config provider for the [`Server`]
///
/// # Examples
///
Expand Down
2 changes: 1 addition & 1 deletion quic/s2n-quic/src/tests/mtu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use s2n_quic_core::{
struct CustomMtu(mtu::Config);

impl mtu::Endpoint for CustomMtu {
fn on_connection(&mut self, _info: &mtu::PathInfo) -> mtu::Config {
fn on_path(&mut self, _info: &mtu::PathInfo) -> mtu::Config {
self.0
}
}
Expand Down

0 comments on commit f8d9971

Please sign in to comment.