Feature/ws over h2 #220

drymus · 2023-04-25T01:39:54Z

Description

Add h2 CONNECT specs
Update h2 binding to support CONNECT

Fixes # (issue)

...ttp/src/main/java/io/aklivity/zilla/runtime/binding/http/internal/codec/Http2SettingsFW.java

jfallows · 2023-04-26T05:09:43Z

.../scripts/io/aklivity/zilla/specs/binding/http/streams/application/rfc7540/connect/client.rpt

+                           .header("date", "Wed, 01 Feb 2017 19:12:46 GMT")
+                           .build()}
+
+write "Hello, world"


Please add a newline at end of file.

...t/java/io/aklivity/zilla/runtime/binding/http/internal/streams/rfc7540/client/ConnectIT.java

jfallows · 2023-04-26T05:16:20Z

.../scripts/io/aklivity/zilla/specs/binding/http/streams/application/rfc7540/connect/client.rpt

These scripts should move to rfc8441 directory.

...main/scripts/io/aklivity/zilla/specs/binding/http/streams/network/rfc7540/connect/server.rpt

jfallows · 2023-04-26T05:21:33Z

.../src/main/java/io/aklivity/zilla/runtime/binding/http/internal/stream/HttpServerFactory.java

+                            break;
+                        } else {
+                            streamError = Http2ErrorCode.PROTOCOL_ERROR;
+                        }


Here we are just tracking the presence and count of the pseudo-headers, so they can be checked in decodeHeaders. So we likely need to add a count for protocol and include in the check.

Note we are validating the pseudo-headers themselves, not the values.

I think I misread this part of the spec in rfc8441:

The :protocol pseudo-header field MUST be included in the CONNECT request, and it MUST have a value of websocket to initiate a WebSocket connection on an HTTP/2 stream

I now think it means that if we have a CONNECT request, before we create a websocket connection we must ensure that the request also contains the :protocol pseudo-header and it must have the value websocket.

Maybe we don't need to do any validation in the http2 binding in relation to a connect request.

Until we find a better place for it, I have added a check to allow a :protocol pseudo-header in a CONNECT request which is otherwise not allowed by rfc7540. This seems to make sense to be in the http2 binding because the binding is always advertising the ENABLE_CONNECT_PROTOCOL setting.

Agreed, just make sure you are doing the simple count here, and the enforcement in decodeHeaders, aligned with approach for other checks.

.../scripts/io/aklivity/zilla/specs/binding/http/streams/application/rfc7540/connect/client.rpt

jfallows · 2023-04-26T05:24:01Z

.../scripts/io/aklivity/zilla/specs/binding/http/streams/application/rfc7540/connect/client.rpt

+                           .header(":status", "200")
+                           .header("server", "CERN/3.0 libwww/2.17")
+                           .header("date", "Wed, 01 Feb 2017 19:12:46 GMT")
+                           .build()}


Use http:matchBeginEx when reading, and only include the headers that matter per spec, such as :status above.

jfallows · 2023-04-26T05:25:06Z

.../scripts/io/aklivity/zilla/specs/binding/http/streams/application/rfc7540/connect/client.rpt

+                           .header("date", "Wed, 01 Feb 2017 19:12:46 GMT")
+                           .build()}
+
+write "Hello, world"


If we are going with websocket handshake, then payload should either be omitted or follow websocket framing instead of Hello, world.

drymus · 2023-05-07T11:23:12Z

...g-ws/src/scripts/io/aklivity/zilla/specs/binding/ws/streams/network/rfc8441/data.request.rpt

+write ${client125}
+write option mask [0x00 0x00 0x00 0x00]
+
+read [0x82 0x7d] ${client125}


I expect the first octet to be 0x81 for text but the server is returning 0x82 (binary). I couldn't find any tests that demonstrate receiving plain text from the engine.

Given that text is a subset of binary, we default to binary to ensure correctness.

However, if WsDataEx on DATA frame has flags 0x81 then we send a text frame instead.

You can see this in action via ws.echo example. Sending a text frame causes a text frame to be echoed back, because the WsDataEx for the decoded text frame, is sent back from echo binding, and then ws binding encodes DATA as a text frame.

Btw, missing newline at EOF.

jfallows

Btw, when you are done making the changes, please also test end-to-end using a local build with ws.echo example and changing values file to set the image tag to develop-SNAPSHOT, similar to mqtt.kafka.reflect example.

Looks like Chrome first shipped support for WebSocket over HTTP/2 in v91.
https://chromestatus.com/feature/6251293127475200

jfallows · 2023-04-28T21:43:30Z

...main/scripts/io/aklivity/zilla/specs/binding/http/streams/network/rfc8441/connect/server.rpt

+      [0x01]                                                # HEADERS frame
+      [0x04]                                                # END_HEADERS
+      [0x00 0x00 0x00 0x01]                                 # stream_id=1
+      [0x88]                                                # HTTP2 status 200


Missing newline at end of file.

jfallows · 2023-05-19T22:56:52Z

...g-ws/src/scripts/io/aklivity/zilla/specs/binding/ws/streams/network/rfc8441/data.request.rpt

+write ${client125}
+write option mask [0x00 0x00 0x00 0x00]
+
+read [0x82 0x7d] ${client125}


Given that text is a subset of binary, we default to binary to ensure correctness.

However, if WsDataEx on DATA frame has flags 0x81 then we send a text frame instead.

You can see this in action via ws.echo example. Sending a text frame causes a text frame to be echoed back, because the WsDataEx for the decoded text frame, is sent back from echo binding, and then ws binding encodes DATA as a text frame.

jfallows · 2023-05-19T22:59:33Z

.../src/main/java/io/aklivity/zilla/runtime/binding/http/internal/stream/HttpServerFactory.java

@@ -6258,8 +6274,16 @@ private void validatePseudoHeaders(
                        scheme++;
                        break;
                    default:
-                        streamError = Http2ErrorCode.PROTOCOL_ERROR;
-                        return;
+                        if (HEADER_NAME_PROTOCOL.equals(name.getStringWithoutLengthUtf8(0, name.capacity())))


Instead of performing a UTF-8 decode to String then comparing to String constant, please create a DirectBuffer constant for HEADER_NAME_PROTOCOL and compare without the need for a decode.

Note: one easy way to create the right DirectBuffer constant is new String8FW(":protocol").value().

jfallows · 2023-05-19T23:01:06Z

.../src/main/java/io/aklivity/zilla/runtime/binding/http/internal/stream/HttpServerFactory.java

+                            break;
+                        } else {
+                            streamError = Http2ErrorCode.PROTOCOL_ERROR;
+                        }


Agreed, just make sure you are doing the simple count here, and the enforcement in decodeHeaders, aligned with approach for other checks.

jfallows · 2023-05-19T23:04:34Z

...g-ws/src/main/java/io/aklivity/zilla/runtime/binding/ws/internal/stream/WsServerFactory.java

+                            scheme,
+                            authority,
+                            path)::onNetMessage;
+                    }


Given the vastly different approaches to the handshake, suggest you avoid all the if/else logic and instead have separate Ws11Server and Ws2Server, both extending generic abstract WsServer if there is enough overlap in implementation beyond the handshake.

jfallows · 2023-05-19T23:10:43Z

...s/src/scripts/io/aklivity/zilla/specs/binding/ws/streams/network/rfc8441/opening.request.rpt

+
+connected
+
+# connection established


Missing newline at EOF.

jfallows · 2023-05-19T23:11:21Z

...src/main/scripts/io/aklivity/zilla/specs/binding/ws/streams/rfc8441/network/data.request.rpt

+write ${client125}
+write option mask [0x00 0x00 0x00 0x00]
+
+read [0x82 0x7d] ${client125}


Missing newline at EOF.

jfallows · 2023-05-19T23:11:30Z

...rc/main/scripts/io/aklivity/zilla/specs/binding/ws/streams/rfc8441/network/data.response.rpt

+read [0x81 0xfd]
+read [0..125]
+
+write [0x82 0x7d] ${client125}


Missing newline at EOF.

jfallows · 2023-05-19T23:11:38Z

.../main/scripts/io/aklivity/zilla/specs/binding/ws/streams/rfc8441/network/opening.request.rpt

+
+connected
+
+# connection established


Missing newline at EOF.

jfallows · 2023-05-19T23:11:45Z

...main/scripts/io/aklivity/zilla/specs/binding/ws/streams/rfc8441/network/opening.response.rpt

+
+connected
+
+# connection established


Missing newline at EOF.

squash

jfallows requested changes Apr 26, 2023

View reviewed changes

drymus force-pushed the feature/ws-over-h2 branch 5 times, most recently from 931ca9f to a971c15 Compare April 30, 2023 10:46

drymus force-pushed the feature/ws-over-h2 branch from a971c15 to 8fef015 Compare May 7, 2023 11:16

drymus commented May 7, 2023

View reviewed changes

jfallows requested changes May 19, 2023

View reviewed changes

drymus changed the base branch from develop to main May 21, 2023 09:28

drymus changed the base branch from main to develop May 21, 2023 09:29

drymus force-pushed the feature/ws-over-h2 branch from e59a18c to 1671da6 Compare May 21, 2023 10:00

drymus force-pushed the feature/ws-over-h2 branch 2 times, most recently from 967218e to 5821774 Compare June 1, 2023 08:34

drymus force-pushed the feature/ws-over-h2 branch 2 times, most recently from 46b0f5b to 52782f6 Compare July 5, 2023 12:56

drymus requested a review from jfallows July 5, 2023 13:45

drymus force-pushed the feature/ws-over-h2 branch from 52782f6 to 47abdac Compare July 22, 2023 09:20

drymus added 12 commits January 10, 2024 19:37

Update http2 binding to support CONNECT

054175a

Add http2 support to ws server binding

b07d7e3

Update http2 binding to support CONNECT

9c38a02

Add http2 support to ws server binding

eb4c7a9

Update license headers

d1965a2

Add missing new lines.

3d67e7b

Use DirectBuffer to count protocol header

d60189c

Rename websocketProtocol to subprotocol

3e34b9b

Add SETTINGS_ENABLE_CONNECT_PROTOCOL to specs

8af6a12

Refactor WsServer.

d026b3c

squash

Rename spec directories.

a4a6b7d

Remove unused engine configuration

cab5a94

drymus force-pushed the feature/ws-over-h2 branch from 47abdac to cab5a94 Compare January 10, 2024 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ws over h2 #220

Feature/ws over h2 #220

drymus commented Apr 25, 2023

jfallows Apr 26, 2023

jfallows Apr 26, 2023

jfallows Apr 26, 2023

drymus Apr 26, 2023

drymus Apr 26, 2023

jfallows May 19, 2023

jfallows Apr 26, 2023

jfallows Apr 26, 2023

drymus May 7, 2023 •

edited

jfallows May 19, 2023

jfallows May 19, 2023

jfallows left a comment

jfallows Apr 28, 2023

jfallows May 19, 2023

jfallows May 19, 2023

jfallows May 19, 2023

jfallows May 19, 2023

jfallows May 19, 2023

jfallows May 19, 2023

jfallows May 19, 2023

jfallows May 19, 2023

jfallows May 19, 2023

Feature/ws over h2 #220

Are you sure you want to change the base?

Feature/ws over h2 #220

Conversation

drymus commented Apr 25, 2023

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drymus May 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfallows left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drymus May 7, 2023 •

edited