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

@@ -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

neqo-bin/src/client/http09.rs

@@ -25,22 +25,34 @@ 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<StreamId, Option<BufWriter<File>>>,
     url_queue: VecDeque<Url>,
     all_paths: Vec<PathBuf>,
     args: &'a Args,
     token: Option<ResumptionToken>,
-    key_update: KeyUpdateState,
+    needs_key_update: bool,
 }
 
 impl<'a> super::Handler for Handler<'a> {
     type Client = Connection;
 
     fn handle(&mut self, client: &mut Self::Client) -> Res<bool> {
         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<ResumptionToken> {
         self.token.take()
     }
@@ -169,14 +172,14 @@ impl super::Client for Connection {
 }
 
 impl<'b> Handler<'b> {
-    pub fn new(url_queue: VecDeque<Url>, args: &'b Args, key_update: KeyUpdateState) -> Self {
+    pub fn new(url_queue: VecDeque<Url>, 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;
         }

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<ResumptionToken>,
     output_read_data: bool,
 }
 
 impl<'a> Handler<'a> {
-    pub(crate) fn new(
-        url_queue: VecDeque<Url>,
-        args: &'a Args,
-        key_update: KeyUpdateState,
-    ) -> Self {
+    pub(crate) fn new(url_queue: VecDeque<Url>, 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<ResumptionToken> {
         self.token.take()
     }

neqo-bin/src/client/mod.rs

@@ -100,39 +100,6 @@ impl std::error::Error for Error {}
 
 type Res<T> = Result<T, Error>;
 
-/// Track whether a key update is needed.
-#[derive(Debug, PartialEq, Eq)]
-struct KeyUpdateState(bool);
-
-impl KeyUpdateState {
-    pub fn maybe_update<F, E>(&mut self, update_fn: F) -> Res<()>
-    where
-        F: FnOnce() -> Result<(), E>,
-        E: Into<Error>,
-    {
-        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<bool>;
-    fn maybe_key_update(&mut self, c: &mut Self::Client) -> Res<()>;
     fn take_token(&mut self) -> Option<ResumptionToken>;
     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,