Discuss Michal's feedback on the grpc-web protocol spec

[wenbozhu]

a) Message framing of trailer
"8th (MSB) bit of the 1st gRPC frame byte"
I assume you mean the Compressed-Flag of the Delimited-Message in the wire spec?
So for an uncompressed trailer the Compressed-Flag byte would be 10000000b, and for a compressed one it would be 10000001b, right?

b) Trailers encoding
"Response status encoded as part of the response body; Key-value pairs encoded as a HTTP/1 headers block (without the terminating newline)".

Basically what you mean is that the contents of the "trailer frame" (last frame) is just a plaintext HTTP/1.1 header block according to the RFC2616 sec 4 (with CRLF as a separator, I assume?

c) Content-Type and lack of Accept
Assuming an empty request (google.Empty), is the Client expected to always set the Content-Type? Why not Accept and deal with a bi-directional negotiation? Shouldn't it set both Accept and Content-Type?
This is relevant regarding the discussion in grpc-go#803.

d) User agent and Server headers
Can we drop them out of the spec? It may seem like they're needed for the implementation to be compliant, and some may be tempted to make this part of the negotiation. The negotiation should only depend on Content-Type gRPC Web or gRPC is to be used should be over

e) Text Encoding
How does one indicate the marshaller of messages (similar to application/grpc-web+json) for what the text-encoding is "encoding" from raw bytes to base64? Or do we assume that it only works for protos?
Also, you mentioned we could have the text encoding specified as optional. Could we have this marked as such?

[wenbozhu]

a) Precisely, for an uncompressed message (inc. trailer) the Compressed-Flag byte would be 10000000b, and for a compressed one it would be 10000001b.

I'll update the spec with the above text, to clarify.

b) Yes. I'll add an example; and also clarify that the empty newline is not needed.

BTW, we started with the http/2 headers block but found it is unnecessarily complicated (for JS). Besides, almost all the known headers are ascii headers.

c) We'd like to keep the web spec in sync with the grpc spec. If the latter is updated to also support Accept, yes, we should add the same support in grpc-web.

d) This is a good feedback. Internally we use C-T too for negotiation (e.g. when we use protobuf to encode trailers, similar to your current implementation). Will discuss more and then patch the spec.

e) grpc-web-text is similar to "Content-Transfer-Encoding: base64" .. which encodes the entire body (or stream) not individual messages. In other words, grpc-web-text introduces a new framing. I'll clarify the spec ... and add examples: e.g. "grpc-web-text+thrift" or "grpc-web-text+json" ....

[mwitkow]

Cool. That clarifies a lot.
I've rebased the current Golang wrapper implementation onto the gRPC frame MSB and the HTTP encoding:
https://github.com/mwitkow/grpc-web/blob/master/go/grpcweb/grpc_web_response.go#L94

The implementation now also has a Go HTTP1.1/HTTP2 client-based integration test:
https://github.com/mwitkow/grpc-web/blob/master/go/grpcweb/wrapper_test.go#L113

The test verifies:

• HTTP1.1 header encoding of the trailers.
• MSB encoding of the last "data frame"
• joining of Trailers and Headers in case there is no payload (wasn't easy ;) )

A couple of further clartification points:

f) CORS, we currently allow all using a separate library to handle CORS-preflight.
https://github.com/mwitkow/grpc-web/blob/master/go/demoserver/demoserver.go#L40

I can extend the Golang implementation to only respond to CORS headers to methods registered on the server. Additionally Wanted to clarify that the following holds (see spec):

• users should be able to list supported origin domains (or * for all)
• we should set Access-Control-Allow-Credentials so that if userspace passes an authorization header, stuff works
• we should only do Access-Control-Allow-Methods to POST and OPTIONS (for preflight)
• we should set Access-Control-Allow-Headers to whatever the CORS preflight OPTIONS request carries (so we support user specified headers as well).

g) Base64 encoding and padding:
which one of the base64 encoders should we use (https://golang.org/pkg/encoding/base64/#RawStdEncoding)? In relation to padding, I read " client library should not assume base64 padding always happens at the boundary of message frames" as:
the implementation just sends a bunch of bytes encoded as base64 "stream", with potential padding inside the "stream" whenever a server needs to flash a byte buffer. is that correct?

[wenbozhu]

g) base64 encoding

Yes, correct. I'll clarify the spec.

For the actual base64 encoder, please consult with go developers.

Also, could you involve @fengli79 in code reviews? Feng is adding nginx support for the new spec.

[wenbozhu]

CORS ... to methods registered on the server

HTTP methods or grpc methods?

[mwitkow]

Grpc methods, all grpc methods are Http post, IIRC

…

On Mon, 27 Feb 2017, 22:11 Wenbo Zhu, ***@***.***> wrote: CORS ... to methods registered on the server HTTP methods or grpc methods? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#57 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJNWo8zgxJfml1aSkYAN2gPa-heOH2OAks5rg0oTgaJpZM4MIA8C> .

[wenbozhu]

Grpc methods, all grpc methods are Http post, IIRC

Then let's not enforce this at the CORS layer (IMO)>

[mwitkow]

Ok, just to clarify: The reason why adding an explicit white list of URL resource strings (based on the list of registered grpc methods) that would pass CORS is to allow Golang gRPC server to coexist with other http.Handler implementations. Allowing a blanket pass through may not be what users want, in case they want to CORS protect other handlers.

…

On Mon, 27 Feb 2017, 22:30 Wenbo Zhu, ***@***.***> wrote: Grpc methods, all grpc methods are Http post, IIRC Then let's not enforce this at the CORS layer (IMO)> — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#57 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJNWo8xfzFBSZOAlJcFmZqcpI2qo8clsks5rg06hgaJpZM4MIA8C> .

[wenbozhu]

Makes sense. Once my PR to the spec is submitted, could you propose a clause in the grpc-web spec to cover the use case behind URL whitelist?

Thanks.

[mwitkow]

Sure :)

[wenbozhu]

Could you review & close the PR?

grpc/grpc#9897

Thanks.