-
Notifications
You must be signed in to change notification settings - Fork 5.5k
lua: clear route cache if headers are modified in decode path #2093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -261,8 +261,22 @@ int StreamHandleWrapper::luaHeaders(lua_State* state) { | |
| if (headers_wrapper_.get() != nullptr) { | ||
| headers_wrapper_.pushStack(); | ||
| } else { | ||
| headers_wrapper_.reset( | ||
| 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 | ||
| // operation, blow away the route cache. We | ||
| // could be a little more intelligent about | ||
| // when we do this so the performance would be | ||
| // higher, but this is simple and will get the | ||
| // job done for now. This is a NOP on the | ||
| // encoder path. | ||
| if (!headers_continued_) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| callbacks_.onHeadersModified(); | ||
| } | ||
|
|
||
| return !headers_continued_; | ||
| }), | ||
| true); | ||
| } | ||
| return 1; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -609,6 +609,7 @@ TEST_F(LuaHttpFilterTest, RequestAndResponse) { | |
| const std::string SCRIPT{R"EOF( | ||
| function envoy_on_request(request_handle) | ||
| request_handle:logTrace(request_handle:headers():get(":path")) | ||
| request_handle:headers():add("foo", "bar") | ||
|
|
||
| for chunk in request_handle:bodyChunks() do | ||
| request_handle:logTrace(chunk:length()) | ||
|
|
@@ -619,6 +620,7 @@ TEST_F(LuaHttpFilterTest, RequestAndResponse) { | |
|
|
||
| function envoy_on_response(response_handle) | ||
| response_handle:logTrace(response_handle:headers():get(":status")) | ||
| response_handle:headers():add("foo", "bar") | ||
|
|
||
| for chunk in response_handle:bodyChunks() do | ||
| response_handle:logTrace(chunk:length()) | ||
|
|
@@ -633,6 +635,7 @@ TEST_F(LuaHttpFilterTest, RequestAndResponse) { | |
|
|
||
| TestHeaderMapImpl request_headers{{":path", "/"}}; | ||
| EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("/"))); | ||
| EXPECT_CALL(decoder_callbacks_, clearRouteCache()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would a test to check that the reroute actually happens also be useful, or is it overkill?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only way to do that would be to add a much more complex integration test. I could do it, but probably overkill IMO.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, no worries, fine as is. |
||
| EXPECT_EQ(FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); | ||
|
|
||
| Buffer::OwnedImpl data("hello"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: How come clang-format makes so much indent, so much wow, here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't know. It bothered me also but not sure what I can do about it. It seems like clang-format bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, no worries.