-
Notifications
You must be signed in to change notification settings - Fork 5.3k
xDS: ext_proc: add GRPC body send mode #38753
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 7 commits
9cd388d
c30e220
ed81c37
b30a64b
2800967
42bd600
532f526
5166f35
ace4e47
8c307bc
bc9a0ad
975e849
e8afc4b
532c31d
cd3b71f
4b7f6f5
e7cbd4d
aa8d3cc
6932eb6
0015fbd
708120f
8b95c52
9186b12
51224c4
d2334a8
ced8f28
28c5bd6
af6aa49
ec538a3
4fa776a
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 |
|---|---|---|
|
|
@@ -65,8 +65,7 @@ message ProcessingMode { | |
| // Do not send the body at all. This is the default. | ||
| NONE = 0; | ||
|
|
||
| // Stream the body to the server in pieces as they arrive at the | ||
| // proxy. | ||
| // Stream the body to the server in pieces as they are seen. | ||
| STREAMED = 1; | ||
|
|
||
| // Buffer the message body in memory and send the entire body at once. | ||
|
|
@@ -79,11 +78,11 @@ message ProcessingMode { | |
| // up to the buffer limit will be sent. | ||
| BUFFERED_PARTIAL = 3; | ||
|
|
||
| // Envoy streams the body to the server in pieces as they arrive. | ||
| // The ext_proc client streams the body to the server in pieces as they arrive. | ||
| // | ||
| // 1) The server may choose to buffer any number chunks of data before processing them. | ||
| // After it finishes buffering, the server processes the buffered data. Then it splits the processed | ||
| // data into any number of chunks, and streams them back to Envoy one by one. | ||
| // data into any number of chunks, and streams them back to the client one by one. | ||
| // The server may continuously do so until the complete body is processed. | ||
| // The individual response chunk size is recommended to be no greater than 64K bytes, or | ||
| // :ref:`max_receive_message_length <envoy_v3_api_field_config.core.v3.GrpcService.EnvoyGrpc.max_receive_message_length>` | ||
|
|
@@ -98,17 +97,29 @@ message ProcessingMode { | |
| // | ||
| // In this body mode: | ||
| // * The corresponding trailer mode has to be set to ``SEND``. | ||
| // * Envoy will send body and trailers (if present) to the server as they arrive. | ||
| // * The client will send body and trailers (if present) to the server as they arrive. | ||
| // Sending the trailers (if present) is to inform the server the complete body arrives. | ||
| // In case there are no trailers, then Envoy will set | ||
| // In case there are no trailers, then the client will set | ||
| // :ref:`end_of_stream <envoy_v3_api_field_service.ext_proc.v3.HttpBody.end_of_stream>` | ||
| // to true as part of the last body chunk request to notify the server that no other data is to be sent. | ||
| // * The server needs to send | ||
| // :ref:`StreamedBodyResponse <envoy_v3_api_msg_service.ext_proc.v3.StreamedBodyResponse>` | ||
| // to Envoy in the body response. | ||
| // * Envoy will stream the body chunks in the responses from the server to the upstream/downstream as they arrive. | ||
| // to the client in the body response. | ||
| // * The client will stream the body chunks in the responses from the server to the upstream/downstream as they arrive. | ||
|
|
||
| FULL_DUPLEX_STREAMED = 4; | ||
|
|
||
| // [#not-implemented-hide:] | ||
| // gRPC traffic. In this mode, the ext_proc client will de-frame the | ||
| // individual gRPC messages inside the HTTP/2 DATA frames, and as each | ||
| // message is de-framed, it will be sent to the ext_proc server as a | ||
| // :ref:`request_body | ||
| // <envoy_v3_api_field_service.ext_proc.v3.ProcessingRequest.request_body>` | ||
| // or :ref:`response_body | ||
| // <envoy_v3_api_field_service.ext_proc.v3.ProcessingRequest.response_body>`. | ||
| // If the ext_proc server modifies the body, that modified body will | ||
| // be used to replace the gRPC message in the stream. | ||
| GRPC = 5; | ||
|
Contributor
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. This protocol mode does not seem to define a new behavior but rather a semantic interpretation of content of the body bytes. An ext_proc client that sends a In the end I think this change complicates the already complicated and confusing protocol even more without offering a tangible benefit. More over server implementations still have to support gRPC messages split over multiple ext_proc messages since this is an existing behavior.
Contributor
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.
That's not actually true -- the contents sent to the ext_proc server are actually different under this mode, so the ext_proc server can absolutely tell the difference. For gRPC traffic, the content of the HTTP/2 DATA frames is a sequence of framed gRPC messages. However, there is no guarantee that each DATA frame contains exactly one framed gRPC message; it's entirely possible to have a single framed gRPC message span multiple DATA frames, and it's also entirely possible to have multiple framed gRPC messages within a single DATA frame. The goal of the new In an existing body send mode like In addition, note that gRPC cannot implement any other mode here, because our filters do not have access to the raw HTTP/2 DATA frames. In gRPC, we handle deframing the gRPC messages in our transport layer (i.e., the same place where we handle the HTTP/2 protocol), and our filters see only the deframed gRPC messages. The only way we could implement the current So basically, there are two arguments for this new mode:
I think there's a clear path to removing the need for that in existing servers:
Contributor
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. The
Contributor
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. The state machine is essentially the same as FULL_DUPLEX_STREAMED, just with the addition of the buffering and deframing of gRPC messages. In other words, the data plane will buffer only until it sees a complete gRPC message, at which point it will deframe the message and stream it to the ext_proc server. And the body responses sent back from the ext_proc server will each be an individual gRPC message as well. I don't think it makes sense to use BUFFERED mode here. Note that the gRPC wire protocol is designed to handle streaming as a first-class citizen, and unary RPCs are just a special case: a unary RPC is a bidi stream that just happens to have a single request followed by a single result. Inside the HTTP/2 DATA frames, the request and response messages are still encoded with the same gRPC framing as in streaming cases. So using BUFFERED mode would still require the data plane to send the raw contents of the HTTP/2 DATA frames, and it would require the ext_proc server to handle the deframing, which is what we're trying to avoid here. (I don't think we want to add a BUFFERED_GRPC mode to make it handle the framing, and even if we did, the behavior would wind up being the same as the GRPC mode proposed here, so I think that would add complexity without actually providing any benefit.)
Contributor
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. @markdroth can gRPC client just re-use FULL_DUPLEX_STREAMED mode then. The gRPC client implementation can do the buffering and deframing before sending the message to the gRPC ext_proc server, which is the gRPC client implementation detail.
Contributor
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.
No, my expectation is that Envoy would also implement GRPC mode. Users that want to use ext_proc for gRPC traffic would want to migrate to this mode if they need to use proxyless gRPC.
If the goal is to implement compression on a per-message basis, which is the way compression is normally done for gRPC, then it should be totally possible in If the goal is to implement compression for the entire HTTP/2 stream, even compressing the gRPC framing within the HTTP/2 DATA frames, then that's not something gRPC will be able to support in the first place, regardless of how we structure the configuration. In gRPC, we deal with the gRPC framing in our transport layer, and the xDS HTTP filters are above that, so the ext_proc filter will see only the deframed gRPC messages. It's not possible for the filter to replace the raw HTTP/2 DATA frames, since it simply doesn't operate at that layer. Given gRPC's architecture, I don't think there's actually anything that the ext_proc server could do that we could actually support that it won't be able to do with
Contributor
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. BTW, I will note that this is yet another reason why it makes sense for gRPC to not support
Contributor
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. After thinking more and looking at other ext_proc implementations I think it would be preferable to use a separate field to indicate content and leave body sending mode as is. Proxyless gRPC would then use content_type: GRPC and ...body_mode: FULL_DUPLEX_STREAMED. Proxyless gRPC does need to implement other modes like BUFFERED or STREAMED. The reason is we want to add support for other content types such as JSON-RPC and we want to be able to send them in all modes, most importantly in BUFFERED mode, since some ext_proc servers only support this mode and it will delay adoption if we have to force implementations to use FULL_DUPLEX_STREAMED to be able to process other content types. If we put the GRPC content as the send mode then we have to add content-type for JSON-RPC anyway and then have to explain that it works in all cases except GRPC mode, which is not great. Using content-type; GRPC and ..._body_mode: FULL_DUPLEX_STREAMED will work exactly the same way as GRPC body send mode without limiting implementation from using other send modes either for GRPC or for other protocols we are adding.
Contributor
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 understand the desire to support other content-aware modes like JSON-RPC, and I agree that that should be possible. So let's talk about the best way to do that. It's not clear to me that introducing a separate content type setting actually provides a better story for doing that, because it's not actually an independent dimension from the body send mode. The content type would actually directly affect the behavior of the body send mode -- e.g., the exact buffering behavior of FULL_DUPLEX_STREAMED will be different depending on whether the content type is GRPC -- which means that both code and humans will have to look at both settings in order to understand the behavior. Specifically:
Given that these two things are really just a single dimension, I think it would make things much easier for the configuration to reflect that. I would suggest instead defining new body send modes for each actual body send behavior, including those that are content-aware. In other words:
I realize that this approach will wind up with a larger number of body send modes, but I think it will ultimately make it easier to understand and implement. To be clear, I think we are going to ultimately support the same set of modes either way; the question here is only about which configuration structure winds up being easier to understand and which adds more complexity. If there's a strong argument for doing this as a separate content type knob, we can make that work, but it seems worse to me in terms of complexity and understandability. Thoughts...? As an aside, it's still not clear to me that it makes sense to support BUFFERED mode for gRPC traffic, for the following reasons:
That having been said, we don't need to decide this right now, as long as we leave room for the possibility in the future. I think that if we rename the GRPC mode to FULL_DUPLEX_STREAMED_GRPC, it will leave an easy way to add support for a BUFFERED_GRPC mode in the future if we decide to do that.
Contributor
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.
Why can it be independent? The bytes that ext_proc filter sends to the server is opaque to it, there is no specific meaning attached to them today. Today it is implied to be a portion (not even corresponding to H/2 DATA or H/1 chunks) of HTTP body because no other other protocols are used with ext_proc. This can be made explicit with additional content-type field. We'd like to make ext_proc and other filters work generically with the protocols encapsulated within HTTP. I think we can make this work by adding ambient frame information to the request that ext_proc can use to generically support sending and receiving of encapsulated protocols. We have at least 2 in the pipeline: GRPC, JSON-RPC/MCP. WebSocket is also being asked for, but this is less definitive, so we want to give developers a flexible way to add support for encapsulated protocols without having to also modify ext_proc protocol. |
||
| } | ||
|
|
||
| // How to handle the request header. Default is "SEND". | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.