Conversation
Adds a new body send mode for gRPC traffic. Also adds a safe way for the ext_proc server to return OK status without losing data in FULL_DUPLEX_STREAMED and GRPC modes. See grpc/proposal#484 for context. Risk Level: Low Testing: N/A Docs Changes: Included in PR Release Notes: N/A Platform Specific Features: N/A --------- Signed-off-by: Mark D. Roth <roth@google.com> Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Adds a new body send mode for gRPC traffic. Also adds a safe way for the ext_proc server to return OK status without losing data in FULL_DUPLEX_STREAMED and GRPC modes. See grpc/proposal#484 for context. Risk Level: Low Testing: N/A Docs Changes: Included in PR Release Notes: N/A Platform Specific Features: N/A --------- Signed-off-by: Mark D. Roth <roth@google.com> Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Mirrored from https://github.com/envoyproxy/envoy @ 7b3a632333b587c784aff65e72ff618ff034f331
| - [request_headers](https://github.com/envoyproxy/envoy/blob/cdd19052348f7f6d85910605d957ba4fe0538aec/api/envoy/service/ext_proc/v3/external_processor.proto#L76) | ||
| and | ||
| [response_headers](https://github.com/envoyproxy/envoy/blob/cdd19052348f7f6d85910605d957ba4fe0538aec/api/envoy/service/ext_proc/v3/external_processor.proto#L81). | ||
| Populated when sending client headers or server headers, respectively. |
There was a problem hiding this comment.
I know we talked about this, but what's the story around what headers should be included vs. not? Are we going to define which headers specifically? It would be good to call out exactly whether method/scheme/path/te/user-agent/message-type/etc, etc are supposed to be included or not. In Go many of these things are added by the transport on the way out or are removed by the transport on the way in. It would be great if we can specify: "only the things set by the application [plus X, Y, and Z, which some libraries may need to manually synthesize before sending to the ext_proc server]"
There was a problem hiding this comment.
Yeah, we need to add this for both ext_authz and ext_proc. I think @easwars was compiling a list of what headers we should document.
| If true, indicates that a half-close should be sent after the | ||
| message. Honored only on client-to-server messages. |
There was a problem hiding this comment.
This puts the stream into an interesting state where the client application may continue sending messages but the filter needs to ignore them. In Go, on the client, it can simply ignore calls to SendMsg(), but on the server (in all languages with a blocking API, so maybe just Go/Rust?), it means we need to spawn a job to read from the client send stream and discard the results so the client doesn't block on flow control.
Actually in Rust we may implement it a bit differently, at least with my current design -- we would be giving the handler an owned RecvStream to poll and the gRPC library will already have to handle the situation where the application drops it early before returning from the handler -- in that case, we probably want to do this behavior of discarding any received messages vs. the alternative of letting the client application back up on flow control.
(I assume it's better to drain it, otherwise it could lead to deadlocks.)
There was a problem hiding this comment.
Yes, I think we need to drain it for flow control reasons.
This seems like mostly a comment about the implementation, not the gRFC, so let's discuss further offline if needed.
There was a problem hiding this comment.
This is going to apply to all implementations, though, so I think it's important to mention it here -- half-close is normally initiated by the client, but in this case the server is simulating it with no way to signal it is doing so to the client. So servers need to make sure they continue to read from the request stream and discard anything received.
|
|
||
| #### Server-Side Metrics | ||
|
|
||
| The following server-side metrics will be exported: |
There was a problem hiding this comment.
Are there no labels that could be used on the server?
There was a problem hiding this comment.
Not that I can think of. If you have any suggestions of labels that you think might be useful here, I'm open to them.
There was a problem hiding this comment.
Aren't these actually per-call/attempt metrics, so should they have all the same labels as the default per-call metrics?
|
|
||
| ### Metrics | ||
|
|
||
| The ext_authz filter will export metrics using the non-per-call metrics |
| | grpc.target | required | The target of the gRPC channel in which ext_authz is used, as the defined in [A66]. | | ||
| | grpc.lb.backend_service | optional | The backend service to which the traffic is being sent, as defined in [A89]. This will be populated from the xDS cluster name, which will be passed to the ext_authz filter as described in [A60]. | |
| The ext_authz filter will export metrics using the non-per-call metrics | ||
| architecture defined in [A79]. There will be a separate set of metrics |
There was a problem hiding this comment.
Oh maybe the implication is that it will get all of those labels already? If so we should call that out, probably?
| difference to the ext_proc server, we have added a new field called | ||
| `grpc_message_compressed` in both the ext_proc request and response | ||
| message (see https://github.com/envoyproxy/envoy/pull/38753). Envoy | ||
| will set this bit in the ext_authz request based on the corresponding |
| - message_timeout and max_message_timeout: Message timeouts do not make | ||
| sense in GRPC body send mode. | ||
| - http_service: It doesn't make sense for gRPC to support non-gRPC | ||
| mechanisms for contacting the ext_authz server. |
| and | ||
| [response_body](https://github.com/envoyproxy/envoy/blob/cdd19052348f7f6d85910605d957ba4fe0538aec/api/envoy/service/ext_proc/v3/external_processor.proto#L89). | ||
| Populated when sending a client message or server message, respectively. | ||
| Inside of them: |
There was a problem hiding this comment.
Have we talked about the performance implications here? If Go uses its current interceptor API, then we'll have only deserialized messages available, meaning we'll have to re-serialize when sending to the ext_proc server. So in theory we're burning extra CPU than if we could just take the payload straight from the bytes and set them in the body field.
No description provided.