-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add a filter to support HTTP/JSON<->gRPC transcoding #1079
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
Changes from all commits
6572487
a8f8f33
579d342
118d1e7
ca46f94
8034159
77f9c00
dd68af1
8d235bd
98c5ccf
547b92f
8f1f307
b88b0d5
9850c9b
53fcf47
6be5d0d
ae9094f
13b66db
5eabe6f
5446914
db48843
f92d527
b68400e
cc27cec
c99496d
c45226a
3acba06
65567f4
41416cd
79791ab
af5ac95
b2265b0
ce354b4
8b154c7
6894025
efba771
80de522
2a35000
d41f9d9
38d08fa
c82fd4a
cdeb87d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| .. _config_http_filters_grpc_json_transcoder: | ||
|
|
||
| gRPC-JSON transcoder filter | ||
| =========================== | ||
|
|
||
| gRPC :ref:`architecture overview <arch_overview_grpc>`. | ||
|
|
||
| This is a filter which allows a RESTful JSON API client to send requests to Envoy over HTTP | ||
| and get proxied to a gRPC service. The HTTP mapping for the gRPC service has to be defined by | ||
| `custom options <https://cloud.google.com/service-management/reference/rpc/google.api#http>`_. | ||
|
|
||
| Configure gRPC-JSON transcoder | ||
| ------------------------------ | ||
|
|
||
| The filter config for the filter requires the descriptor file as well as a list of the gRPC | ||
| services to be transcoded. | ||
|
|
||
| .. code-block:: json | ||
|
|
||
| { | ||
| "type": "both", | ||
| "name": "grpc_json_transcoder", | ||
| "config": { | ||
| "proto_descriptors": "proto.pb", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Not a full review, will do full code review tomorrow) Can we get a description of each of these config parameters? If I understand correctly, it's required that all descriptors get merged into a single pb file, right? What if there are multiple services from multiple protos? How do those get merged? Can you maybe provide an example? For "services" what happens if the .pb file contains more services than you specify? Those aren't transcoded? Can you clarify? Also, in general, I would love to talk about reflection possibilities. Obviously this is a follow up, but from Lyft perspective (and I think others), this filter is vastly less useful and more difficult to use if Envoy has to know ahead of time the .pb files and we need to keep them up to date. If we could do this with reflection I think that is the true absolutely killer feature. cc @twoism @rodaine @louiscryan
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Will write it in docs, in short, protoc accepts multiple input files, so just do them together.
Right, those aren't transcoded.
I totally agree with you that would provide better usability, I'm just saying that won't included in this PR as this is big enough :) Feel free to open an issue and assign to me once this get merged. |
||
| "services": ["grpc.service.Service"] | ||
| } | ||
| } | ||
|
|
||
| proto_descriptors | ||
| *(required, string)* Supplies the binary protobuf descriptor set for the gRPC services. | ||
| The descriptor set has to include all of the types that are used in the services. Make sure | ||
| to use the ``--include_import`` option for ``protoc``. | ||
|
|
||
| To generate a protobuf descriptor set for the gRPC service, you'll also need to clone the | ||
| googleapis repository from Github before running protoc, as you'll need annotations.proto | ||
| in your include path. | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| git clone https://github.com/googleapis/googleapis | ||
| GOOGLEAPIS_DIR=<your-local-googleapis-folder> | ||
|
|
||
| Then run protoc to generate the descriptor set from bookstore.proto: | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| protoc -I$(GOOGLEAPIS_DIR) -I. --include_imports --include_source_info \ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can make this more seamless via some
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed I don't think we should assume that the user of envoy has access to the envoy code or bazel. |
||
| --descriptor_set_out=proto.pb test/proto/bookstore.proto | ||
|
|
||
| If you have more than one proto source files, you can pass all of them in one command. | ||
|
|
||
| services | ||
| *(required, array)* A list of strings that supplies the service names that the | ||
| transcoder will translate. If the service name doesn't exist in ``proto_descriptors``, Envoy | ||
| will fail at startup. The ``proto_descriptors`` may contain more services than the service names | ||
| specified here, but they won't be translated. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the wrong if statement to have this under? I will defer to @htuch for review on all of the bazel changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since googleapis is loaded from envoy-api, so I added here, let me know if this should go somewhere else. @htuch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is OK, but in theory you could source http_api_protos independently, let's keep it like this for now.