Refactoring the processing of a service grpc response #95
Conversation
didierofrivia
force-pushed
the
refactoring-grpc-response
branch
from
56794f9 to
65f2b60
Compare
didierofrivia
changed the title
didierofrivia
changed the title
src/filter/http_context.rs
Outdated
| GrpcService::process_grpc_response( | ||
| operation, | ||
| resp_size, | ||
| &mut self.response_headers_to_add, |
There was a problem hiding this comment.
I realise this is how this was before, but I'm now wondering why we're doing it like this - why is the response headers stored locally in the filter instead of using the envoy hostcalls like in auth? Should auth do it this way, or should ratelimit do the same as auth?
There was a problem hiding this comment.
Makes total sense to have them both the same. I'll review the option of matching external auth 👍🏼
There was a problem hiding this comment.
Just for clarification now that I've learned - the reason we do it like this is to ensure the response headers are added at the correct phase - just before we return a response to the downstream client. In the case here we're adding them before the request is made to the upstream. I've fixed this in #144
* Treated in their respective Service modules * Calling directly `hostcalls` instead of relying on Filter instance Signed-off-by: dd di cesare <[email protected]>
* Instead of passing the Filter instance of `response_headers_to_add` * Matching the Auth Service Signed-off-by: dd di cesare <[email protected]>
didierofrivia
force-pushed
the
refactoring-grpc-response
branch
from
026d497 to
725d043
Compare
There was a problem hiding this comment.
Nice job, lgtm!
I will quickly add just so we have it documented - perhaps we in the future want to refactor this again so that we can pass information between the extensions without performing hostcalls; just for the worst case where we write the response headers but then end up denying the request at a future extension anyway.
hostcallsinstead of relying on Filter instance