Encoding some ext_proc filter configuration in the ProcessingRequest message #38451
Conversation
…message Signed-off-by: Yanjun Xiang <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Yanjun Xiang <[email protected]>
|
/assign @yanavlasov @tyxia @adisuissa |
| @@ -124,6 +138,9 @@ message ProcessingRequest { | |||
| // are needed. | |||
| // | |||
| bool observability_mode = 10; | |||
|
|
|||
| // The filter configuration to be sent to the server. | |||
| FilterConfigToSend filter_config = 11; | |||
There was a problem hiding this comment.
Consider renaming FilterConfigToSend to FilterConfigContext, as not the entire config is sent, but some filter config context.
There was a problem hiding this comment.
ProtocolConfiguration would be even better.
I think also the ext_proc server only needs to know if body streaming is full or half duplex. It does not need all other options (buffered, etc). I think we should just limit to two cases, full duplex or half duplex. The default value in this case would be half-duplex, if it is old ext_proc filter that does not set this option.
There was a problem hiding this comment.
Done. Also encoded send_body_without_waiting_for_header_response flag
| @@ -124,6 +138,9 @@ message ProcessingRequest { | |||
| // are needed. | |||
| // | |||
| bool observability_mode = 10; | |||
|
|
|||
| // The filter configuration to be sent to the server. | |||
| FilterConfigToSend filter_config = 11; | |||
There was a problem hiding this comment.
ProtocolConfiguration would be even better.
I think also the ext_proc server only needs to know if body streaming is full or half duplex. It does not need all other options (buffered, etc). I think we should just limit to two cases, full duplex or half duplex. The default value in this case would be half-duplex, if it is old ext_proc filter that does not set this option.
yanjunxiang-google
changed the title
Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
|
Kind Ping! |
| @@ -124,6 +145,10 @@ message ProcessingRequest { | |||
| // are needed. | |||
| // | |||
| bool observability_mode = 10; | |||
|
|
|||
| // [#not-implemented-hide:] | |||
| // The filter protocol configurations to be sent to the server. | |||
There was a problem hiding this comment.
Please add under which conditions this will be sent (e.g., first-request, all-requests, some-requests), and what is the expected behavior from the server (specifically, could the server decide to NACK this, and if not, what is the expected behavior on the client and server, if the server doesn't support the protocol config)?
There was a problem hiding this comment.
These are part of the filter configurations. They will be encoded in all the ProcessingRequest sent to the ext_proc server. Customer control both the server software and Envoy configuration. It's a miss-config if server does not support the Envoy configuration. There is no NACK message in ext_proc protocol. However, the ext_proc server can send immediate response to shut down the connection and alert customer to fix the configuration.
Do we need to mention this in the API?
BTW, this ProtocolConfiguration parameters are for server to know what mode Envoy is operating. It should be expected that server support both old and new mode. For what's the server behavior if Envoy is operating in STREAMED or FULL_DUPLEX_STREAMED mode, that's specified here:
.There was a problem hiding this comment.
These are part of the filter configurations. They will be encoded in all the ProcessingRequest sent to the ext_proc server. Customer control both the server software and Envoy configuration. It's a miss-config if server does not support the Envoy configuration. There is no NACK message in ext_proc protocol. However, the ext_proc server can send immediate response to shut down the connection and alert customer to fix the configuration.
Will it be possible to change the values between a ProcessingRequest that is sent with headers to one that is sent with body or trailers? If so, how should the the server behave in such case?
Do we need to mention this in the API?
Every protocol should define what is the expected behavior of the parties in the protocol. If this can be enforced at protocol level (specific ACK/NACK mechanism) that would be great. If not, it should still be clarified by describing the behavior.
BTW, this ProtocolConfiguration parameters are for server to know what mode Envoy is operating. It should be expected that server support both old and new mode. For what's the server behavior if Envoy is operating in STREAMED or FULL_DUPLEX_STREAMED mode, that's specified here:
I think what's missing is at what level the new features are expected to take effect.
Should they be sent once when the connection between Envoy and the ext-proc server is created, and will be true for all the streams that are created?
Should the be sent on each new stream? I'm actually not an ext-proc expert, and don't fully understand whether a single ext-proc stream maps to a single downstream-request-upstream-response-stream or whether each ext-proc stream maps to a single-frame that is being sent (e.g., frame). I'm assuming it's the former, so I'm guessing that the new information needs to be sent only when the ext-proc stream is created.
.
This is the API part change of encoding some ext_proc filter configuration in the ProcessingRequest message.
Encoding such filter configurations when sending ProcessingRequest message to the ext_proc server helps the server to take right action with it, like send response immediately, or buffer the request.