Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add server support for application/grpc+proto over http #5505

Merged
merged 4 commits into from
Dec 11, 2023

Conversation

siggisim
Copy link
Member

This adds support for server-side streaming (and paves the way for client side streaming with websockets in the future if we want it).

If we end up using this for a bit and liking it, we could delete all of the gnarly reflection protolet.go and simplify that code a ton.

The concepts used here are losely based on https://github.com/improbable-eng/grpc-web/blob/master/go/grpcweb/wrapper.go - so they've been tested in a production environment, but have been greatly simplified.

Overview

Here's how it works:

  • If the client sets the application/grpc+proto content type, we forward requests to the grpc server's ServeHTTP method.
  • In order to support local development, we spoof HTTP2 requests by setting ProtoMajor and ProtoMinor. This is what the improbable library does too, so there's some precedent here: https://github.com/improbable-eng/grpc-web/blob/master/go/grpcweb/wrapper.go#L279
  • When returning the http response, we wrap the ResponseWriter so we can set an http status and the error message in the response body (grpc and grpc-web always return a 200 success status and then include trailers that contain the status and status message). This matches our current protolet behavior.

Binary format

The biggest difference between this and what we're currently doing is that the grpc over http protocol (you can read about this here: https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md) expects data to be sent in the Length-Prefixed-Message format. Here's what that looks like

  • Length-Prefixed-Message → Compressed-Flag Message-Length Message
    • Compressed-Flag → 0 / 1 # encoded as 1 byte unsigned integer
    • Message-Length → {length of Message} # encoded as 4 byte unsigned integer (big endian)
    • Message → *{binary octet}

Currently we only send the Message bit, so there are some places (like when parsing the request context) where we need to ignore those first 5 bytes. There's a separate PR that sets these 5 bytes on the client side when we're using this new content-type.

Misc

In order to support streaming responses, we need all of our ResponseWriters to implement http.Flusher - this PR also adds that (happy to break that out into a separate PR if that helps).

The final little quirk in this PR is that we GenerateHTTPHandlers after the grpc services are registered because we forward requests to the grpcServer.


"github.com/buildbuddy-io/buildbuddy/server/util/request_context"
requestcontext "github.com/buildbuddy-io/buildbuddy/server/util/request_context"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move down below unnamed ones

@siggisim siggisim merged commit a6f5254 into master Dec 11, 2023
1 check passed
@siggisim siggisim deleted the siggi-dev-branch-20231211-135100 branch December 11, 2023 23:20
Comment on lines +285 to +290
if i == 0 {
w.WriteHeader(200)
}

// Match our current behavior where we return 500 for all errors and return the message in the response body
w.WriteHeader(500)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there be a return after line 286?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I sent #5509

Not sure if this ever gets hit in practice, but there's definitely supposed to be a return there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants