Implement websocket support in Dropshot as an Extractor and #[channel] macro by lifning · Pull Request #403 · oxidecomputer/dropshot

lifning · 2022-07-28T00:06:36Z

This allows endpoints meant for handling continuous websocket streams to be easily constructed by providing an async handler to an extractor parameter in the endpoint function definition.

This change also refactors the previous paginated bool to instead be a dedicated type describing whether an endpoint uses pagination or websockets Dropshot-specific extensions, aspirationally leaving room for describing any more extension modes we may add atop the basic OpenAPI.

david-crespo · 2022-07-28T02:29:09Z

            description: None,
            tags: vec![],
-            paginated: func_parameters.paginated,
+            extension_mode: func_parameters.extension_mode,


So this means a given endpoint can only be in one mode at a time, right? That makes sense for pagination and websockets, since they're mutually exclusive. I have no idea what other kinds of extensions we might dream up, but is it possible we could want to use more than one at a time?

yeah - i suppose if such a thing comes to pass, we could change to be something more sophisticated than a bare enum, but i didn't want to try to anticipate unknown unknowns yet, just represent that pagination and websockets are mutually exclusive extensions

david-crespo · 2022-07-28T02:29:36Z

+}]
+async fn example_api_websocket_counter(
+    _rqctx: Arc<RequestContext<()>>,
+    ws_upg: WebsocketUpgrade,


amazing API for this

luqmana

I left a couple small comments but otherwise looks good, thanks @lifning!

davepacheco

Cool! I had a few questions.

Would you mind updating the changelog to describe the new feature?

davepacheco · 2022-08-02T16:59:10Z

+                extension_mode = match (extension_mode, metadata.extension_mode) {
+                    (ExtensionMode::None, x) | (x, ExtensionMode::None) => x,
+                    (x, y) if x != y => {
+                        panic!("incompatible extension modes in tuple: {:?} != {:?}", x, y);


What does this condition mean?

it means the two tuple elements were indicators of different non-None extension modes (i.e. Paginated and Websocket at this point), in either order, which we currently take to be mutually exclusive things

davepacheco · 2022-08-02T19:05:43Z

+ * The consumer of this *must* call [WebsocketUpgrade::handle] (or if they do
+ * not wish to upgrade a given connection, [WebsocketUpgrade::do_not_upgrade])
+ * on the owned instance in the endpoint's fn, or else WebsocketUpgrade's
+ * implementation of [Drop::drop] will panic.


Why's that? (why not implicitly call do_not_upgrade() on drop, say?)

It looks like it's a non-fatal error to handle the upgrade more than once (we produce an error) but it's fatal to not handle it at all. I don't have a strong idea here but I just wonder if there's a way to make it harder to accidentally panic in production. (One thought I had was that WebsocketUpgrade could be parametrized by an impl of some trait that would allow us to immediately and automatically invoke it exactly once for you...but then I'm not sure what the body of the function would do. Also, I'm not sure if it's desirable to be able to do stuff or look at the request before the upgrade -- presumably it is (e.g., authentication). Then I thought this could be a different kind of endpoint...but then I worry I'm overthinking it.)

for the interface to be more discoverable (because .handle() has to be called for any of this to work; this way nobody's wondering why their connection isn't getting upgraded, even if they weren't originally clear on the docs) and less prone to incorrect use (e.g. accidental inclusion of a WebsocketUpgrade in a param list due to copy/paste errors, causing us to eventually generate an confusing/incorrect client from the resulting schema)

if we're very concerned about panics making it into production, we can make it be an error log in Drop instead, but that's less likely to be caught at development time -- maybe panic if debug assertions are on, else log?

davepacheco · 2022-08-02T19:06:21Z

+#[derive(Debug)]
+pub struct WebsocketUpgrade(Option<WebsocketUpgradeInner>);
+
+/**


Did you mean to put some docs here and on WebsocketUpgradeResponse?

🤦‍♀️ sure did! thanks

ahl

Are other extractors permitted alongside WebsocketUpgrade? Or must it be the lone extractor?

What does this look like in the OpenAPI output? The OpenAPI spec folks have pretty much declared Websockets as "not HTTP APIs" and therefore out of scope.

I vaguely had intended to use AsyncAPI to describe websocket interfaces, both the channels and the messages. AsyncAPI and OpenAPI are at least sort of compatible in that I think we could produce a single JSON file with both descriptions in there.

Does it make sense to use the same endpoint macro with the constraints that the method must be a GET, the WebsocketUpgrade must be present, and the return type is Result<Response<Body>, HttpError>? Or might we want some other interface such as #[channel { path = ... } ] or #[endpoint { method = WEBSOCKET, ...?

ahl · 2022-08-03T00:46:29Z

 }

+/**
+ * Dropshot/Progenitor features used by endpoints which are not a part of the base OpenAPI spec.


what's the plan here?

per our discussion on matrix, i'll file some issues after this merges about turning this AsyncAPI support (and perhaps coming up with a general.. philosophy? on how to approach other extensions, like pagination or anything we come up with in the future that's outside of the scope of AsyncAPI in particular)

ahl · 2022-08-03T00:47:06Z

+ */
+#[derive(Copy, Clone, Debug, Default, Eq, PartialEq)]
+pub enum ExtensionMode {
+    #[default]


Do we have a MSRV for dropshot? This is quite recent, no?

i actually changed to this from a proper bare impl Default after an earlier PR comment pointed out that the rust-toolchain.toml for this project declares rust 1.62 - simple enough to revert to what i originally had if it's a concern at all

I should have clarified. I don't know if we have an MSRV! Yes, I know we specify the toolchain; that's at least in part to ensure repeatable builds as we have several tests for the endpoint macros that are a bit.. finicky with regard to version dependence.

I love #[default]; I don't know if using it would be annoying for our internal consumers (or maybe for external consumers; 👋 I don't know if you exist, but if you do, we love you!)

I did mention the derive Default but we don't have an MSRV I could find so depends on how conservative we wanna be here I guess.

In terms of our own internal consumers, of the ones I know of:

propolis lists an MSRV of 1.62

omicron is pinned on an old nightly (due to unstable features): nightly-2022-04-27

crucible is also pinned on an even older nightly: nightly-2021-11-24

buildomat doesn't have a rust-toolchain or note about version

cio is pinned on an old nightly: nightly-2022-03-13

Do the old nightly's support #[default]? If so I say we go ahead and keep it. If not... I guess maybe roll them forward first?

i think i'm not quite as inclined to have us drag everything forward for something as trivial as some very small syntax sugar (vs. say, implementing an actual feature in such a way that otherwise wouldn't have been possible), but i wouldn't say no if other folks felt strongly otherwise

Yea, I don't think that that should hold this up.

ahl · 2022-08-03T00:49:29Z

+      async fn my_ws_endpoint(
+          rqctx: std::sync::Arc<dropshot::RequestContext<()>>,
+          websock: dropshot::WebsocketUpgrade,
+      ) -> Result<http::Response<hyper::Body>, dropshot::HttpError> {


Is it always the case that the Result::Ok response type must be Response<Body>? If so, do we check that?

we don't explicitly check for it in proc-macro-land, but in the latest patch i did introduce some friendlier type aliases for what's going on - and the user need not concern themselves with the Result<Response<Body>, HttpError> one any more; that's all abstracted away by #[channel] and all their socket-handling function has to return is a very generic Result<(), Box<dyn Error+Send+Sync+'static> (chosen so we can log their errors, if any, and let them use the ergonomic ?)

ahl · 2022-08-03T00:55:10Z

+    * [`tokio::io::AsyncWrite`] type (e.g. `tokio_tungstenite`).
+    *
+    * ```
+      #[dropshot::endpoint { method = GET, path = "/my/ws/endpoint" }]


it seems like this always needs to be a GET request. Do we check that somewhere?

i think this is resolved by the new #[channel...] approach, as we don't let method be specified at all in that macro (it's always GET for protocol=WEBSOCKETS)

ahl · 2022-08-03T01:02:08Z

 pub use server::ServerContext;
 pub use server::{HttpServer, HttpServerStarter};
+pub use websocket::HyperUpgraded;
+pub use websocket::SchemaWebsocketParams;


Why is this pub?

lifning · 2022-08-05T17:31:53Z

Are other extractors permitted alongside WebsocketUpgrade? Or must it be the lone extractor?

They shouldn't be mutually exclusive (or else we wouldn't be able to do much useful with it, like say addressing a specific org/proj/inst in nexus by Path<> params)

What does this look like in the OpenAPI output? The OpenAPI spec folks have pretty much declared Websockets as "not HTTP APIs" and therefore out of scope.

It looks like a GET endpoint with a "x-dropshot-websocket": true stashed in it, just like the "x-dropshot-pagination": true we already have. Here's an example (with the unrelated formatting condensed a little for brevity)

{
  "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/serial-console": {
    "get": {
      "tags": ["instances"],
      "summary": "Connect to an instance's serial console stream",
      "operationId": "instance_serial_console",
      "parameters": [
        { "in": "path", "name": "instance_name", "required": true,
          "schema": { "$ref": "#/components/schemas/Name" }, "style": "simple" },
        { "in": "path", "name": "organization_name", "required": true,
          "schema": { "$ref": "#/components/schemas/Name" }, "style": "simple" },
        { "in": "path", "name": "project_name", "required": true,
          "schema": { "$ref": "#/components/schemas/Name" }, "style": "simple" }
      ],
      "responses": { "default": { "description": "", "content": { "*/*": {"schema": {}}}}},

      "x-dropshot-websocket": true
    }
  }
}

I vaguely had intended to use AsyncAPI to describe websocket interfaces, both the channels and the messages. AsyncAPI and OpenAPI are at least sort of compatible in that I think we could produce a single JSON file with both descriptions in there.

Would it be reasonable (in the interest of providing a websockets interface in the nearer term for serial & VNC API work) to punt on the full extent of that for the moment with the "x-dropshot-websocket" sentinel in an OpenAPI path's "get" method, and then follow up by implementing the proper AsyncAPI version?

For the use cases I'm thinking of, AsyncAPI's specifications are a bit heavier weight than we need, but that might be a blessing in this regard - I think we often want to forward other protocols through raw binary messages, so most of the work that I think this would end up unblocking would have a fairly trivial expression as far as just annotating everywhere it's used with an effective "yep, it's binary" in such a way that our macro could accept before having a "real" AsyncAPI implementation backing it.

Does it make sense to use the same endpoint macro with the constraints that the method must be a GET, the WebsocketUpgrade must be present, and the return type is Result<Response<Body>, HttpError>? Or might we want some other interface such as #[channel { path = ... } ] or #[endpoint { method = WEBSOCKET, ...?

I did have the thought of perhaps making endpoint { method = WEBSOCKET, ... decorating an async fn in which you're required to put an upgraded connection parameter (much like requiring path variables), and not have an intermediate function at all... but I shyed away from the former part of that approach at first because I didn't want to use a non-HTTP-verb there, and now the latter part too because would mean we wouldn't have a good way to expose the JoinHandle of what will likely be a longer-running future than most other kinds of endpoint.

I also had the use case of "what if we want to leave a path open for hybrid GET endpoints requests that may or may not be upgraded depending on how it's accessed," but I am increasingly unsure this use case would be worth the additional complexity.

#[channel... would be a nice way to both get a little closer to AsyncAPI terminology and possibly isolate some of the additional websocket/AsyncAPI-specific complexity from the endpoint decorator's implementation when it comes to things like describing message formats. My current inclination in light of this is to perhaps make such a thing as a trivial macro for now, that expands to the currently-equivalent #[endpoint = GET decorator.

ahl · 2022-08-08T17:52:59Z

Are other extractors permitted alongside WebsocketUpgrade? Or must it be the lone extractor?

They shouldn't be mutually exclusive (or else we wouldn't be able to do much useful with it, like say addressing a specific org/proj/inst in nexus by Path<> params)

Is it compatible with Query or Body extractors?

What does this look like in the OpenAPI output? The OpenAPI spec folks have pretty much declared Websockets as "not HTTP APIs" and therefore out of scope.

It looks like a GET endpoint with a "x-dropshot-websocket": true stashed in it, just like the "x-dropshot-pagination": true we already have. Here's an example (with the unrelated formatting condensed a little for brevity)

Cool!

Would it be reasonable (in the interest of providing a websockets interface in the nearer term for serial & VNC API work) to punt on the full extent of that for the moment with the "x-dropshot-websocket" sentinel in an OpenAPI path's "get" method, and then follow up by implementing the proper AsyncAPI version?

Absolutely, and I apologize if I seemed to be implying otherwise. I don't want to stand in the way of urgent progress; I also want to have line of sight to how we want this to work in the future.

For the use cases I'm thinking of, AsyncAPI's specifications are a bit heavier weight than we need, but that might be a blessing in this regard - I think we often want to forward other protocols through raw binary messages, so most of the work that I think this would end up unblocking would have a fairly trivial expression as far as just annotating everywhere it's used with an effective "yep, it's binary" in such a way that our macro could accept before having a "real" AsyncAPI implementation backing it.

Neat. I haven't used AsyncAPI. I inferred that it lets you specify the protocol such that e.g. we could generate a client that had the messages as Rust types... but that's just imagineering.

Does it make sense to use the same endpoint macro with the constraints that the method must be a GET, the WebsocketUpgrade must be present, and the return type is Result<Response<Body>, HttpError>? Or might we want some other interface such as #[channel { path = ... } ] or #[endpoint { method = WEBSOCKET, ...?

I did have the thought of perhaps making endpoint { method = WEBSOCKET, ... decorating an async fn in which you're required to put an upgraded connection parameter (much like requiring path variables), and not have an intermediate function at all... but I shyed away from the former part of that approach at first because I didn't want to use a non-HTTP-verb there, and now the latter part too because would mean we wouldn't have a good way to expose the JoinHandle of what will likely be a longer-running future than most other kinds of endpoint.

I also had the use case of "what if we want to leave a path open for hybrid GET endpoints requests that may or may not be upgraded depending on how it's accessed," but I am increasingly unsure this use case would be worth the additional complexity.

Should we validate at least that folks are using a GET rather than some other method? I would be happy to help or to do this as a follow-on PR.

#[channel... would be a nice way to both get a little closer to AsyncAPI terminology and possibly isolate some of the additional websocket/AsyncAPI-specific complexity from the endpoint decorator's implementation when it comes to things like describing message formats. My current inclination in light of this is to perhaps make such a thing as a trivial macro for now, that expands to the currently-equivalent #[endpoint = GET decorator.

That's a neat idea for a follow-on.

Thanks for the thorough discussion!

lifning · 2022-08-10T08:54:14Z

so the little "panic in Drop if it's not handled" scheme is kinda invalidated when any of the other extractors fail to parse from the request - it never reaches the user's code for them to call handle, which would probably be pretty inconvenient even when the panic is downgraded to a warn! when built in release mode. i'm leaning toward just trying to writing the wrapper for it in the proc macro, and if the user needs a JoinHandle there's theoretically nothing stopping them from tokio::spawning inside the handler, though it would theoretically be the slightest one-time bit of perf hit as we end up spawning a task to spawn a task.

This allows endpoints meant for handling continuous websocket streams to be easily constructed by providing an async handler that accepts the upgraded websocket connection as an argument. This change also refactors the previous `paginated` bool to instead be a dedicated type describing any 'extension' modes we may add atop the basic OpenAPI.

ahl · 2022-08-22T05:41:04Z

 */
 pub struct ExtractorMetadata {
-    pub paginated: bool,
+    pub extension_mode: ExtensionMode,


@lifning it looks like this was a breaking change:

error[E0560]: struct `dropshot::ExtractorMetadata` has no field named `paginated` --> nexus/src/authn/external/cookies.rs:51:29 | 51 | ExtractorMetadata { paginated: false, parameters: vec![] } | ^^^^^^^^^ `dropshot::ExtractorMetadata` does not have this field | = note: available fields are: `extension_mode`, `parameters`

oxidecomputer/omicron#1626

Could you please update the changelog?

rel: oxidecomputer/dropshot#403 Generated methods return a `WebsocketReactants` type that at present can only be unwrapped into its inner `http::Request` and `tokio::net::TcpStream` 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) The returned `Request` is an HTTP request containing the client's part of the handshake for establishing a Websocket, and the `TcpStream` is a raw TCP (non-Web-) socket. The consumer of the raw `into_request_and_tcp_stream` interface is expected to send the HTTP request over the TCP socket, i.e. by providing them to a websocket implementation such as `tokio_tungstenite::client_async(Request, TcpStream)`.

This moves the websocket-related boilerplate into the code generated by Dropshot's `#[channel]` annotation macro. (rel: oxidecomputer/dropshot#403 )

lifning requested review from jclulow and luqmana July 28, 2022 00:06

lifning mentioned this pull request Jul 28, 2022

initial websocket implmentation #389

Closed

1 task

david-crespo reviewed Jul 28, 2022

View reviewed changes

luqmana reviewed Jul 29, 2022

View reviewed changes

lifning force-pushed the websock branch 2 times, most recently from 20def1d to 7d64f54 Compare August 1, 2022 03:54

luqmana reviewed Aug 1, 2022

View reviewed changes

Comment thread dropshot/src/websocket.rs Outdated

lifning force-pushed the websock branch from 7d64f54 to 2d115d0 Compare August 2, 2022 00:04

luqmana approved these changes Aug 2, 2022

View reviewed changes

lifning force-pushed the websock branch 3 times, most recently from 6678f55 to d438904 Compare August 2, 2022 08:00

davepacheco reviewed Aug 2, 2022

View reviewed changes

ahl reviewed Aug 3, 2022

View reviewed changes

Comment thread dropshot/src/websocket.rs Outdated

ahl reviewed Aug 3, 2022

View reviewed changes

Comment thread dropshot/src/websocket.rs

ahl reviewed Aug 3, 2022

View reviewed changes

Comment thread dropshot/src/websocket.rs Outdated

lifning force-pushed the websock branch 3 times, most recently from 845269d to 5ef073a Compare August 10, 2022 19:50

lifning changed the title ~~Implement websocket support in Dropshot as an Extractor~~ Implement websocket support in Dropshot as an Extractor and #[channel] macro Aug 10, 2022

lifning removed the request for review from jclulow August 16, 2022 00:30

lifning force-pushed the websock branch from 5ef073a to 011e83b Compare August 18, 2022 19:45

lifning merged commit fad4e98 into oxidecomputer:main Aug 18, 2022

ahl reviewed Aug 22, 2022

View reviewed changes

lifning mentioned this pull request Aug 22, 2022

Update changelog with breaking change info for ExtractorMetadata refactor done for WebSockets #428

Merged

lifning mentioned this pull request Aug 31, 2022

Generate a Client method for Dropshot websocket channels oxidecomputer/progenitor#183

Merged

lifning mentioned this pull request Sep 1, 2022

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

Merged

Conversation

lifning commented Jul 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luqmana left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lifning commented Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahl commented Aug 8, 2022

Uh oh!

lifning commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

lifning commented Aug 5, 2022 •

edited

Loading

lifning commented Aug 10, 2022 •

edited

Loading