lua: clear route cache if headers are modified in decode path#2093
lua: clear route cache if headers are modified in decode path#2093
Conversation
Signed-off-by: Matt Klein <mklein@lyft.com>
| HeaderMapWrapper::create(state, headers_, [this]() { return !headers_continued_; }), true); | ||
| headers_wrapper_.reset(HeaderMapWrapper::create(state, headers_, | ||
| [this] { | ||
| // If we are about to do a modifiable header |
There was a problem hiding this comment.
Nit: How come clang-format makes so much indent, so much wow, here?
There was a problem hiding this comment.
Yeah I don't know. It bothered me also but not sure what I can do about it. It seems like clang-format bug.
|
|
||
| TestHeaderMapImpl request_headers{{":path", "/"}}; | ||
| EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("/"))); | ||
| EXPECT_CALL(decoder_callbacks_, clearRouteCache()); |
There was a problem hiding this comment.
Would a test to check that the reroute actually happens also be useful, or is it overkill?
There was a problem hiding this comment.
The only way to do that would be to add a much more complex integration test. I could do it, but probably overkill IMO.
| // higher, but this is simple and will get the | ||
| // job done for now. This is a NOP on the | ||
| // encoder path. | ||
| if (!headers_continued_) { |
There was a problem hiding this comment.
Should this also be done in regular filters? I.e. is there a reason to make Lua behave non-uniform here, such as keeping is simple?
There was a problem hiding this comment.
The reason is keeping it simple. It would be a more complex and performance sensitive change to do it in all filters. I can open an issue to track this idea if you like.
There was a problem hiding this comment.
Do we have a doc for filter authors that provides advice on how to do things like a rerouting filter? I think just capturing the idioms for Lua vs. C++ would be useful somewhere, issues is fine.
|
Do you think there could be a change to the filter API that could be done (probably in a followup PR or issue) to make it easier to learn if there has been route change? |
|
@htuch turns out it was pretty easy to add an integration test so I added one. |
| ->mutable_match() | ||
| ->set_prefix("/test/long/url"); | ||
|
|
||
| auto new_route = hcm.mutable_route_config()->mutable_virtual_hosts(0)->add_routes(); |
What do you mean exactly? Like a callback if another filter changes the route? |
|
More like, instead of returning continue, stop+buffer, etc., you could pass additional metadata back to HCM. It would be a bit more abstract/declarative, vs. the current situation where you have to map from (as filter author) "I made a route change, do something with it" to "I made a route change, aha, I need to clear the route cache.". |
|
Right. OK. I opened #2097. Over the holidays I'm going to explore adding the ability to write filters on a Coroutine style in C++. I will think about this during that work. |
|
@htuch updated again |
* add mixer error details into metadata Signed-off-by: Lizan Zhou <lizan@tetrate.io> * format Signed-off-by: Lizan Zhou <lizan@tetrate.io> * review Signed-off-by: Lizan Zhou <lizan@tetrate.io> * comment Signed-off-by: Lizan Zhou <lizan@tetrate.io>
It was never safe to do a case-sensitive lookup, but this broke in envoyproxy/envoy-mobile#2088 (comment). Signed-off-by: JP Simard <jp@jpsim.com>
It was never safe to do a case-sensitive lookup, but this broke in envoyproxy/envoy-mobile#2088 (comment). Signed-off-by: JP Simard <jp@jpsim.com>
This allows filters to modify headers such that the router will lookup
the route again.
Risk Level: Low
Testing: UT