Kludge: Copy the serial console client impl from propolis-cli #268
Conversation
lifning
force-pushed
the
serial-stream
branch
from
c3358e9 to
9c88966
Compare
|
looks like 'make cross' dies because this uses some unix-specifics. as this is a stopgap solution i may just |
karencfv
left a comment
karencfv
left a comment
There was a problem hiding this comment.
Thanks for the PR @lifning!
I'm assuming this PR depends on oxidecomputer/omicron#1766? If so, can we convert this PR to draft so it doesn't get accidentally merged?
src/cmd_instance.rs
Outdated
| } | ||
|
|
||
| impl CmdInstanceSerial { | ||
| async fn websock_stream_tty(&self, client: oxide_api::Client) -> Result<()> { |
There was a problem hiding this comment.
Can you add some tests for this?
src/cmd_instance.rs
Outdated
| async fn stdin_to_websockets_task( | ||
| mut stdinrx: tokio::sync::mpsc::Receiver<Vec<u8>>, | ||
| wstx: tokio::sync::mpsc::Sender<Vec<u8>>, | ||
| ) { | ||
| // next_raw must live outside loop, because Ctrl-A should work across | ||
| // multiple inbuf reads. | ||
| let mut next_raw = false; | ||
|
|
||
| loop { | ||
| let inbuf = if let Some(inbuf) = stdinrx.recv().await { | ||
| inbuf | ||
| } else { | ||
| continue; | ||
| }; | ||
|
|
||
| // Put bytes from inbuf to outbuf, but don't send Ctrl-A unless | ||
| // next_raw is true. | ||
| let mut outbuf = Vec::with_capacity(inbuf.len()); | ||
|
|
||
| let mut exit = false; | ||
| for c in inbuf { | ||
| match c { | ||
| // Ctrl-A means send next one raw | ||
| b'\x01' => { | ||
| if next_raw { | ||
| // Ctrl-A Ctrl-A should be sent as Ctrl-A | ||
| outbuf.push(c); | ||
| next_raw = false; | ||
| } else { | ||
| next_raw = true; | ||
| } | ||
| } | ||
| b'\x03' => { | ||
| if !next_raw { | ||
| // Exit on non-raw Ctrl-C | ||
| exit = true; | ||
| break; | ||
| } else { | ||
| // Otherwise send Ctrl-C | ||
| outbuf.push(c); | ||
| next_raw = false; | ||
| } | ||
| } | ||
| _ => { | ||
| outbuf.push(c); | ||
| next_raw = false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Send what we have, even if there's a Ctrl-C at the end. | ||
| if !outbuf.is_empty() { | ||
| wstx.send(outbuf).await.unwrap(); | ||
| } | ||
|
|
||
| if exit { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl CmdInstanceSerial { | ||
| async fn websock_stream_tty(&self, client: oxide_api::Client) -> Result<()> { | ||
| let uri = format!( | ||
| "/organizations/{}/projects/{}/instances/{}/serial-console/stream", | ||
| self.organization, | ||
| self.project, | ||
| self.instance, | ||
| ); | ||
| let reqw = client.request_raw(http::Method::GET, &uri, None) | ||
| .await? | ||
| .build()?; | ||
|
|
||
| let mut url = reqw.url().to_owned(); | ||
| url.set_scheme(&url.scheme().replace("http", "ws")) | ||
| .or_else(|()| anyhow::bail!("couldn't change protocol scheme"))?; | ||
| let mut req = url.into_client_request()?; | ||
| req.headers_mut().insert( | ||
| http::header::AUTHORIZATION, | ||
| reqw.headers().get(http::header::AUTHORIZATION).unwrap().to_owned(), | ||
| ); | ||
| let (mut ws, _) = tokio_tungstenite::connect_async(req).await?; | ||
|
|
||
| let _raw_guard = RawTermiosGuard::stdio_guard() | ||
| .expect("failed to set raw mode"); | ||
|
|
||
| let mut stdout = tokio::io::stdout(); | ||
|
|
||
| // https://docs.rs/tokio/latest/tokio/io/trait.AsyncReadExt.html#method.read_exact | ||
| // is not cancel safe! Meaning reads from tokio::io::stdin are not cancel | ||
| // safe. Spawn a separate task to read and put bytes onto this channel. | ||
| let (stdintx, stdinrx) = tokio::sync::mpsc::channel(16); | ||
| let (wstx, mut wsrx) = tokio::sync::mpsc::channel(16); | ||
|
|
||
| tokio::spawn(async move { | ||
| let mut stdin = tokio::io::stdin(); | ||
| let mut inbuf = [0u8; 1024]; | ||
|
|
||
| loop { | ||
| let n = match stdin.read(&mut inbuf).await { | ||
| Err(_) | Ok(0) => break, | ||
| Ok(n) => n, | ||
| }; | ||
|
|
||
| stdintx.send(inbuf[0..n].to_vec()).await.unwrap(); | ||
| } | ||
| }); | ||
|
|
||
| tokio::spawn(async move { stdin_to_websockets_task(stdinrx, wstx).await }); | ||
|
|
||
| loop { | ||
| tokio::select! { | ||
| c = wsrx.recv() => { | ||
| match c { | ||
| None => { | ||
| // channel is closed | ||
| break; | ||
| } | ||
| Some(c) => { | ||
| ws.send(Message::Binary(c)).await?; | ||
| }, | ||
| } | ||
| } | ||
| msg = ws.next() => { | ||
| match msg { | ||
| Some(Ok(Message::Binary(input))) => { | ||
| stdout.write_all(&input).await?; | ||
| stdout.flush().await?; | ||
| } | ||
| Some(Ok(Message::Close(..))) | None => break, | ||
| _ => continue, | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
So, I've been thinking about this functionality for a bit. Are there any reasons this can't be part of the API? I can see our customers who'll want to integrate this to their own software. It'd be pretty neat to have all of this available to our API and language client users; not just the ones using the CLI. What do you think?
Additionally, I'm a bit reluctant to hardcode endpoint paths and requests (lines 657-674). This could bring us some problems down the line 🤔
There was a problem hiding this comment.
I actually merged support for the websocket endpoint into progenitor already, so in theory when the omicron PR for this feature lands it'll be supported in the progenitor-made API straight away; this was just me bolting it onto this program to make the feature accessible at all, under the assumption that https://github.com/oxidecomputer/oxide-sdk-rust will soon replace it more cleanly. (it seems oxide.rs's codegen isn't too happy with being fed the current nexus OpenAPI, and i wasn't sure if it was worth the time fixing all that and adding websockets support to it if it's slated for replacement -- especially considering the business about tag-swap.sh -- but if i'm wrong about that do let me know!)
There was a problem hiding this comment.
Ah gotcha! yeah that makes perfect sense. You're right to not to spend too much time (or any at all 😅 ) fixing the current generator.
We're still working out a few things to be able to switch the current
Rust client+CLI to the new one. Will keep every one updated and sorry for this mess in the mean time.
There was a problem hiding this comment.
The more I think about this, the more RFD 28's 'extension' section tells me I should perhaps just make a one-off binary that uses the progenitor-based client for this, and shell out to it for this subcommand instead - it's kind of a special case compared to the rest of the subcommands, especially when we get to generating implementations for the CRUD-ish ones.
lifning
force-pushed
the
serial-stream
branch
2 times, most recently
from
707e42f to
7765430
Compare
This implements a basic interactive websocket client for Nexus's instance/serial-console/stream channel, using the a progenitor-based client for this one endpoint specifically.
lifning
force-pushed
the
serial-stream
branch
from
7765430 to
6e428a6
Compare
|
(fwiw, tests are passing locally with a local nexus deployment and |
2 participants
This implements a basic interactive websocket client for Nexus's instance/serial-console/stream channel (PR: oxidecomputer/omicron#1766)