From c44e53647d922359ef859d18d86dd9eb7fe4891c Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 10 Apr 2024 07:46:42 +0200 Subject: [PATCH] refactor(client): simplify keyupdate testcase implementation (#1808) The QUIC Interop Runner `keyupdate` testcase has a client establish a connection to the server and then trigger a key update. https://github.com/quic-interop/quic-interop-runner/blob/2a2534a1284d50d99ff92884d4f1ecf98fb41e4c/testcases.py#L889 This testcase always uses the `http09` client and server implementation. This commit simplifies the testcase implementation: - Given that it is only used with `http09`, move it to `http09.rs`. - Reduce the `KeyUpdateState` `struct` to a single `bool`. - Mark the `--key-update` command line argument as hidden, given that it is only set indirectly through the `-t keyupdate` flag. - Try to run `client.initiate_key_update` on events only, not on ever new received datagram. In addition it enables the `keyupdate` test on the Neqo `qns.yml` CI workflow. --- .github/workflows/qns.yml | 2 +- neqo-bin/src/client/http09.rs | 31 ++++++++++++---------- neqo-bin/src/client/http3.rs | 16 ++---------- neqo-bin/src/client/mod.rs | 48 ++++++----------------------------- 4 files changed, 28 insertions(+), 69 deletions(-) diff --git a/.github/workflows/qns.yml b/.github/workflows/qns.yml index caadb022df..17cd584a26 100644 --- a/.github/workflows/qns.yml +++ b/.github/workflows/qns.yml @@ -71,6 +71,6 @@ jobs: name: 'neqo-latest' image: ${{ steps.docker_build_and_push.outputs.imageID }} url: https://github.com/mozilla/neqo - test: handshake + test: handshake,keyupdate client: neqo-latest,quic-go,ngtcp2,neqo,msquic server: neqo-latest,quic-go,ngtcp2,neqo,msquic diff --git a/neqo-bin/src/client/http09.rs b/neqo-bin/src/client/http09.rs index e0b254f67b..a9ed12b157 100644 --- a/neqo-bin/src/client/http09.rs +++ b/neqo-bin/src/client/http09.rs @@ -25,7 +25,7 @@ use neqo_transport::{ }; use url::Url; -use super::{get_output_file, qlog_new, Args, KeyUpdateState, Res}; +use super::{get_output_file, qlog_new, Args, Res}; pub struct Handler<'a> { streams: HashMap>>, @@ -33,7 +33,7 @@ pub struct Handler<'a> { all_paths: Vec, args: &'a Args, token: Option, - key_update: KeyUpdateState, + needs_key_update: bool, } impl<'a> super::Handler for Handler<'a> { @@ -41,6 +41,18 @@ impl<'a> super::Handler for Handler<'a> { fn handle(&mut self, client: &mut Self::Client) -> Res { while let Some(event) = client.next_event() { + if self.needs_key_update { + match client.initiate_key_update() { + Ok(()) => { + qdebug!("Keys updated"); + self.needs_key_update = false; + self.download_urls(client); + } + Err(neqo_transport::Error::KeyUpdateBlocked) => (), + Err(e) => return Err(e.into()), + } + } + match event { ConnectionEvent::AuthenticationNeeded => { client.authenticated(AuthenticationStatus::Ok, Instant::now()); @@ -66,9 +78,6 @@ impl<'a> super::Handler for Handler<'a> { qdebug!("{event:?}"); self.download_urls(client); } - ConnectionEvent::StateChange(State::Confirmed) => { - self.maybe_key_update(client)?; - } ConnectionEvent::ResumptionToken(token) => { self.token = Some(token); } @@ -86,12 +95,6 @@ impl<'a> super::Handler for Handler<'a> { Ok(false) } - fn maybe_key_update(&mut self, c: &mut Self::Client) -> Res<()> { - self.key_update.maybe_update(|| c.initiate_key_update())?; - self.download_urls(c); - Ok(()) - } - fn take_token(&mut self) -> Option { self.token.take() } @@ -169,14 +172,14 @@ impl super::Client for Connection { } impl<'b> Handler<'b> { - pub fn new(url_queue: VecDeque, args: &'b Args, key_update: KeyUpdateState) -> Self { + pub fn new(url_queue: VecDeque, args: &'b Args) -> Self { Self { streams: HashMap::new(), url_queue, all_paths: Vec::new(), args, token: None, - key_update, + needs_key_update: args.key_update, } } @@ -195,7 +198,7 @@ impl<'b> Handler<'b> { } fn download_next(&mut self, client: &mut Connection) -> bool { - if self.key_update.needed() { + if self.needs_key_update { qdebug!("Deferring requests until after first key update"); return false; } diff --git a/neqo-bin/src/client/http3.rs b/neqo-bin/src/client/http3.rs index 09a30461bf..b3f577127e 100644 --- a/neqo-bin/src/client/http3.rs +++ b/neqo-bin/src/client/http3.rs @@ -27,7 +27,7 @@ use neqo_transport::{ }; use url::Url; -use super::{get_output_file, qlog_new, Args, KeyUpdateState, Res}; +use super::{get_output_file, qlog_new, Args, Res}; pub(crate) struct Handler<'a> { #[allow( @@ -36,17 +36,12 @@ pub(crate) struct Handler<'a> { clippy::redundant_field_names )] url_handler: UrlHandler<'a>, - key_update: KeyUpdateState, token: Option, output_read_data: bool, } impl<'a> Handler<'a> { - pub(crate) fn new( - url_queue: VecDeque, - args: &'a Args, - key_update: KeyUpdateState, - ) -> Self { + pub(crate) fn new(url_queue: VecDeque, args: &'a Args) -> Self { let url_handler = UrlHandler { url_queue, stream_handlers: HashMap::new(), @@ -61,7 +56,6 @@ impl<'a> Handler<'a> { Self { url_handler, - key_update, token: None, output_read_data: args.output_read_data, } @@ -225,12 +219,6 @@ impl<'a> super::Handler for Handler<'a> { Ok(self.url_handler.done()) } - fn maybe_key_update(&mut self, c: &mut Http3Client) -> Res<()> { - self.key_update.maybe_update(|| c.initiate_key_update())?; - self.url_handler.process_urls(c); - Ok(()) - } - fn take_token(&mut self) -> Option { self.token.take() } diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index 791e2a6366..61e43c00d1 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -100,39 +100,6 @@ impl std::error::Error for Error {} type Res = Result; -/// Track whether a key update is needed. -#[derive(Debug, PartialEq, Eq)] -struct KeyUpdateState(bool); - -impl KeyUpdateState { - pub fn maybe_update(&mut self, update_fn: F) -> Res<()> - where - F: FnOnce() -> Result<(), E>, - E: Into, - { - if self.0 { - if let Err(e) = update_fn() { - let e = e.into(); - match e { - Error::TransportError(TransportError::KeyUpdateBlocked) - | Error::Http3Error(neqo_http3::Error::TransportError( - TransportError::KeyUpdateBlocked, - )) => (), - _ => return Err(e), - } - } else { - qerror!("Keys updated"); - self.0 = false; - } - } - Ok(()) - } - - fn needed(&self) -> bool { - self.0 - } -} - #[derive(Debug, Parser)] #[command(author, version, about, long_about = None)] #[allow(clippy::struct_excessive_bools)] // Not a good use of that lint. @@ -176,7 +143,7 @@ pub struct Args { /// Use this for 0-RTT: the stack always attempts 0-RTT on resumption. resume: bool, - #[arg(name = "key-update", long)] + #[arg(name = "key-update", long, hide = true)] /// Attempt to initiate a key update immediately after confirming the connection. key_update: bool, @@ -255,6 +222,11 @@ impl Args { return; }; + if self.key_update { + qerror!("internal option key_update set by user"); + exit(127) + } + // Only use v1 for most QNS tests. self.shared.quic_parameters.quic_version = vec![Version::Version1]; match testcase.as_str() { @@ -370,7 +342,6 @@ trait Handler { type Client: Client; fn handle(&mut self, client: &mut Self::Client) -> Res; - fn maybe_key_update(&mut self, c: &mut Self::Client) -> Res<()>; fn take_token(&mut self) -> Option; fn has_token(&self) -> bool; } @@ -474,7 +445,6 @@ impl<'a, H: Handler> Runner<'a, H> { self.client .process_multiple_input(dgrams.iter(), Instant::now()); self.process_output().await?; - self.handler.maybe_key_update(&mut self.client)?; } Ok(()) @@ -578,14 +548,12 @@ pub async fn client(mut args: Args) -> Res<()> { first = false; - let key_update = KeyUpdateState(args.key_update); - token = if args.shared.use_old_http { let client = http09::create_client(&args, real_local, remote_addr, &hostname, token) .expect("failed to create client"); - let handler = http09::Handler::new(to_request, &args, key_update); + let handler = http09::Handler::new(to_request, &args); Runner { args: &args, @@ -601,7 +569,7 @@ pub async fn client(mut args: Args) -> Res<()> { let client = http3::create_client(&args, real_local, remote_addr, &hostname, token) .expect("failed to create client"); - let handler = http3::Handler::new(to_request, &args, key_update); + let handler = http3::Handler::new(to_request, &args); Runner { args: &args,