Add RL response headers in correct phase #144
Conversation
Signed-off-by: Adam Cattermole <[email protected]>
|
Apologies to @didierofrivia here I incorrectly suggested to break this #95 (comment) |
adam-cattermole
changed the title
src/filter/http_context.rs
Outdated
| if GrpcService::process_grpc_response( | ||
| operation, | ||
| resp_size, | ||
| &mut self.response_headers_to_add, |
There was a problem hiding this comment.
This is a code smell... this seems to invert the indirection between the state machine possibly progressing and the result of such a progress... Not a huge deal, I don't think this is even a first. "Just" mentioning it...
The way I think about it: something happens, this has a "result" (i.e. some output) and that output is dealt with. Which in this particular case would be adding the headers to this Vec, but the action knows nothing about that Vec, instead it merely returns the entries to add to a caller that then knows what to do with them.
There was a problem hiding this comment.
Right, so you think it'd be preferable to have some kind of GrpcResult containing the response headers to let the filter deal with that
There was a problem hiding this comment.
Never mind for now... we probably would want to go to the drawing board at some point and design this a little more thoroughly
There was a problem hiding this comment.
I missed this.. I realise we would likely change a lot of things here, but how about this alternative in latest commit?
Signed-off-by: Adam Cattermole <[email protected]>
adam-cattermole
force-pushed
the
fix-rl-response-headers
branch
from
67c6751 to
9f280cc
Compare
There was a problem hiding this comment.
Apologies to @didierofrivia here I incorrectly suggested to break this #95 (comment)
No worries, at the time it made sense to me too!
👍🏼
This passes the
response_headersback up to the filter layer so that we can add the headers only at the last phaseAdding these headers before we send to the upstream service is not allowed, and if you set
rateLimitHeaders: DRAFT_VERSION_03for limitador the shim will panic without this change.