Generate a Client method for Dropshot websocket channels #183

lifning · 2022-08-31T08:49:19Z

(rel: oxidecomputer/dropshot#403 )
Generated methods return ResponseValue<reqwest::Upgrade, which may be passed to a websocket protocol implementation such as tokio_tungstenite::WebSocketStream::from_raw_stream(rv.into_inner(), ...) for the purpose of implementing against the raw websocket connection, but may later be extended as a generic to allow higher-level channel message definitions. (rel: oxidecomputer/dropshot#429)

Per the changelog, consumers will need to depend on reqwest from git until such time upstream cuts a new crates.io release:

reqwest = {
    git = "https://github.com/seanmonstar/reqwest",
    rev = "6ceb23958c8b6d7f1d8ee093f0ad73184d133d40",
    features = ["json", "stream"]
}

progenitor-client/Cargo.toml

ahl · 2022-09-06T18:05:35Z

It would be great to see a release that includes seanmonstar/reqwest#1376 to move forward with this

ahl

very cool; it's a bummer that we'd need that git dependency, but I guess what can you do. If we do move forward with that, let's pause with merging this PR: I'd like to make a public release first to crates.io

Also it looks like some output needs updating to pass the tests.

progenitor-impl/src/method.rs

ahl · 2022-09-08T21:09:22Z

progenitor-impl/src/method.rs

+                let typ = if dropshot_websocket {
+                    OperationResponseType::Upgrade


Is the Content-type meaningless for the response from one of these endpoints?
https://stackoverflow.com/a/39105228/1371209

I assume it will never be application/json (is that true?)

Could we do

let typ = if let Some(mt) = response.content.get("application/json") { // ... } else if !dropshot_websocket { OperationResponseType::Raw } else { OperationResponseType::Upgrade }

My preference (when it doesn't suck!) is to have the typical case first. And can we please update the comment above to cover the new case as well?

correct re: it not being application/json - the schema emitted from the API generated by dropshot looks like

"/instance/serial": { "get": { "operationId": "instance_serial", "responses": { "default": { "description": "", "content": { "*/*": { "schema": {} } } } }, "x-dropshot-websocket": {} } }

so */* is what we end up calling it. i think that ends up making the check for is_some in this if-else chain preempt our check for websocket, so i'm inclined to do it in this order:

let typ = if let Some(mt) = response.content.get("application/json") { // ... OperationResponseType::Type(typ) } else if dropshot_websocket { OperationResponseType::Upgrade } else if response.content.first().is_some() { OperationResponseType::Raw } else { OperationResponseType::None }

ahl · 2022-09-08T21:14:08Z

progenitor-client/Cargo.toml

+base64 = "0.13"
+rand = "0.8"


I think these would need to be added here as well: https://github.com/oxidecomputer/progenitor/blob/main/progenitor-impl/src/lib.rs#L426

Is there a reasonable way to avoid these dependencies? "No" is a fine answer...

i guess the way to avoid it would be re-implementing them ourselves, which feels pretty far out of scope.
(for what it's worth, we're pulling these in to avoid pulling in all of tungstenite)
this (and potential future such cases of adding new deps) would also be less of an issue if we were to move to emitting code that uses crate re-exports from progenitor-client

ahl · 2022-09-08T21:18:35Z

progenitor-impl/src/method.rs

+                    OperationResponseType::Upgrade => {
+                        quote! {
+                            Err(Error::ErrorResponse(
+                                ResponseValue::upgrade(response).await?
+                            ))
+                        }
+                    }


In what situation would we hit this?

ah! my mistake, we wouldn't, as per below we don't yet describe any error responses in the generated OpenAPI schema.

ahl · 2022-09-08T21:19:55Z

sample_openapi/propolis-server.json

+          "default": {
+            "description": "",
+            "content": {
+              "*/*": {
+                "schema": {}
+              }
+            }
+          }


In this case, what would the proper handling of a 4xx or 5xx response be? I realize that this is more germane for the dropshot code, but I'm thinking through how clients (direct or through progenitor) would handle it.

presently Dropshot just returns 400 Bad Request for all errors that can occur in its method for performing the websocket upgrade (and technically could also return a 500 Internal Error if a gamma ray flips the right bit, but that code is ordinarily unreachable), and doesn't explicitly specify any of that in the schema as is evident here.

I think the appropriate thing to do would be to update Dropshot to use more specific/appropriate error codes (e.g. 426 Upgrade Required for version mismatch) and model them accordingly in the generated schema for all websocket channel endpoints, at which point progenitor-client's ResponseValue would have something a little more clear to show for what went wrong.

progenitor/tests/build_propolis.rs

lifning · 2022-09-09T16:52:09Z

very cool; it's a bummer that we'd need that git dependency, but I guess what can you do. If we do move forward with that, let's pause with merging this PR: I'd like to make a public release first to crates.io

thinking about it more, would it perhaps make sense for us to just re-export everything the generated code needs from our crate, and have the output import them that way so consumers don't have to garden their dependencies for updates like this in general? (like, git or not)

ahl · 2022-09-09T18:02:36Z

thinking about it more, would it perhaps make sense for us to just re-export everything the generated code needs from our crate, and have the output import them that way so consumers don't have to garden their dependencies for updates like this in general? (like, git or not)

I've considered this in the past, but for reqwest in particular I was concerned with consumers who had their own uses outside of the generated client i.e. often uses seems to be broader than a single client. What do you think?

lifning · 2022-09-13T06:24:43Z

thinking about it more, would it perhaps make sense for us to just re-export everything the generated code needs from our crate, and have the output import them that way so consumers don't have to garden their dependencies for updates like this in general? (like, git or not)

I've considered this in the past, but for reqwest in particular I was concerned with consumers who had their own uses outside of the generated client i.e. often uses seems to be broader than a single client. What do you think?

if they're forced to version-match with us either way, i think it's better to model it within the cargo metadata rather than leaking the dependency graph. as it sits it feels more awkward than a re-export, especially when it comes to adding new features like this which are now a breaking change with much larger footprint than they'd have otherwise (making them find out which of our dependencies to version-bump in their Cargo.toml when our macro output stops working due to an update) vs. just having the transitive dependency solved in the golden path where people don't have their own independent dependency declared on reqwest (in addition to the comment above about base64 and rand as well).

essentially it would avoid having to explain (and keep re-explaining any time it changes) what to add to Cargo.toml when the general expectation people have when they think "i want to depend on progenitor-client" is to type progenitor-client = "X.Y.Z".

and re-exporting crates is a pretty common pattern in my personal experience - rocket re-exports tokio, for example, and in projects where i've needed to use parts of tokio directly in conjunction, i'm generally happier to put something like use rocket::tokio; in my code rather than something like tokio = "X.Y.Z" # NOTE: must match the version rocket depends on! in my Cargo.toml (which i've had to do with other crates)

ahl · 2022-09-13T15:39:34Z

@lifning ok, let's try it.

ahl · 2022-09-15T16:46:59Z

And by "it" I mean re-exporting reqwest.

lifning · 2022-09-21T07:24:22Z

updated without re-exports thanks to the newly-tagged reqwest release! https://github.com/seanmonstar/reqwest/releases/tag/v0.11.12

progenitor-impl/src/lib.rs

luqmana

One small nit re: the unused import but otherwise LGTM. Thanks for driving this forward lif!

I think it'd be worth figuring out a different approach than requiring every consumer to manually add the needed dependencies but I think that's outside the scope of this PR.

Generated methods return `ResponseValue<reqwest::Upgrade`, which may be passed to a websocket protocol implementation such as `tokio_tungstenite::WebSocketStream::from_raw_stream(rv.into_inner(), ...)` for the purpose of implementing against the raw websocket connection, but may later be extended as a generic to allow higher-level channel message definitions. Per the changelog, consumers will need to depend on reqwest 0.11.12 or newer for HTTP Upgrade support, as well as base64 and rand if any endpoints are websocket channels: ``` [dependencies] reqwest = { version = "0.11.12" features = ["json", "stream"] } base64 = "0.13" rand = "0.8" ```

lifning force-pushed the dropsock branch from 0e2ef51 to be93e43 Compare August 31, 2022 09:24

lifning mentioned this pull request Sep 1, 2022

Dropshot-based websocket endpoint & Add Progenitor-based client oxidecomputer/propolis#206

Merged

luqmana reviewed Sep 2, 2022

View reviewed changes

progenitor-client/Cargo.toml Outdated Show resolved Hide resolved

lifning force-pushed the dropsock branch from be93e43 to ef7db01 Compare September 8, 2022 05:47

ahl reviewed Sep 8, 2022

View reviewed changes

lifning force-pushed the dropsock branch from ef7db01 to 183ef35 Compare September 13, 2022 06:56

lifning force-pushed the dropsock branch from 28f397c to 26abdce Compare September 15, 2022 08:47

lifning force-pushed the dropsock branch 4 times, most recently from eeaa9c8 to d229de5 Compare September 21, 2022 07:23

luqmana reviewed Sep 22, 2022

View reviewed changes

progenitor-impl/src/lib.rs Outdated Show resolved Hide resolved

luqmana approved these changes Sep 22, 2022

View reviewed changes

lifning force-pushed the dropsock branch from d229de5 to 38ccd19 Compare September 22, 2022 00:40

lifning force-pushed the dropsock branch from 38ccd19 to 7d0df33 Compare September 22, 2022 00:44

lifning merged commit 4e2dcc5 into oxidecomputer:main Sep 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate a Client method for Dropshot websocket channels #183

Generate a Client method for Dropshot websocket channels #183

lifning commented Aug 31, 2022 •

edited

Loading

ahl commented Sep 6, 2022

ahl left a comment

ahl Sep 8, 2022

lifning Sep 13, 2022 •

edited

Loading

ahl Sep 8, 2022

lifning Sep 13, 2022

ahl Sep 8, 2022

lifning Sep 17, 2022

ahl Sep 8, 2022

lifning Sep 17, 2022

lifning commented Sep 9, 2022

ahl commented Sep 9, 2022

lifning commented Sep 13, 2022

ahl commented Sep 13, 2022

ahl commented Sep 15, 2022

lifning commented Sep 21, 2022

luqmana left a comment

		let typ = if dropshot_websocket {
		OperationResponseType::Upgrade

Generate a Client method for Dropshot websocket channels #183

Generate a Client method for Dropshot websocket channels #183

Conversation

lifning commented Aug 31, 2022 • edited Loading

ahl commented Sep 6, 2022

ahl left a comment

Choose a reason for hiding this comment

ahl Sep 8, 2022

Choose a reason for hiding this comment

lifning Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

ahl Sep 8, 2022

Choose a reason for hiding this comment

lifning Sep 13, 2022

Choose a reason for hiding this comment

ahl Sep 8, 2022

Choose a reason for hiding this comment

lifning Sep 17, 2022

Choose a reason for hiding this comment

ahl Sep 8, 2022

Choose a reason for hiding this comment

lifning Sep 17, 2022

Choose a reason for hiding this comment

lifning commented Sep 9, 2022

ahl commented Sep 9, 2022

lifning commented Sep 13, 2022

ahl commented Sep 13, 2022

ahl commented Sep 15, 2022

lifning commented Sep 21, 2022

luqmana left a comment

Choose a reason for hiding this comment

lifning commented Aug 31, 2022 •

edited

Loading

lifning Sep 13, 2022 •

edited

Loading