Skip to content

feat(lua): allow setting response body when the upstream response body is empty #14486

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.

Sign up for GitHub

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

Merged
mattklein123 merged 13 commits into from
Jan 6, 2021
Merged

feat(lua): allow setting response body when the upstream response body is empty #14486

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c1144f3
feat(lua): allow setting response body when the upstream response bod…
spacewander Dec 20, 2020
9ad27b2
Merge branch 'master' into body
spacewander Dec 22, 2020
b209dcf
Add runtime flag
spacewander Dec 22, 2020
240ff41
format
spacewander Dec 23, 2020
d387335
release note
spacewander Dec 27, 2020
229d2de
sorted
spacewander Dec 27, 2020
ade3534
Merge branch 'master' into body
spacewander Dec 27, 2020
40fd113
Merge branch 'master' into body
spacewander Dec 29, 2020
09b05bb
Merge branch 'master' into body
spacewander Dec 30, 2020
0101c9c
new way
spacewander Jan 4, 2021
63d1354
doc lint
spacewander Jan 4, 2021
d59c8e1
tweak for review
spacewander Jan 5, 2021
c8cf954
typo
spacewander Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Minor Behavior Changes
* jwt_authn filter: added support of Jwt time constraint verification with a clock skew (default to 60 seconds) and added a filter config field :ref:`clock_skew_seconds <envoy_v3_api_field_extensions.filters.http.jwt_authn.v3.JwtProvider.clock_skew_seconds>` to configure it.
* kill_request: enable a way to configure kill header name in KillRequest proto.
* listener: injection of the :ref:`TLS inspector <config_listener_filters_tls_inspector>` has been disabled by default. This feature is controlled by the runtime guard `envoy.reloadable_features.disable_tls_inspector_injection`.
* lua: always return a :ref:`buffer object <config_http_filters_lua_buffer_wrapper>` even the body is empty. This feature is controlled by the runtime guard `envoy.reloadable_features.lua_always_wrap_body`.

@mattklein123 mattklein123 Jan 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* lua: always return a :ref:`buffer object <config_http_filters_lua_buffer_wrapper>` even the body is empty. This feature is controlled by the runtime guard `envoy.reloadable_features.lua_always_wrap_body`.
* lua: always return a :ref:`buffer object <config_http_filters_lua_buffer_wrapper>` even if the body is empty. This feature is controlled by the runtime guard `envoy.reloadable_features.lua_always_wrap_body` and defaults to enabled.

* memory: enable new tcmalloc with restartable sequences for aarch64 builds.
* mongo proxy metrics: swapped network connection remote and local closed counters previously set reversed (`cx_destroy_local_with_active_rq` and `cx_destroy_remote_with_active_rq`).
* outlier detection: added :ref:`max_ejection_time <envoy_v3_api_field_config.cluster.v3.OutlierDetection.max_ejection_time>` to limit ejection time growth when a node stays unhealthy for extended period of time. By default :ref:`max_ejection_time <envoy_v3_api_field_config.cluster.v3.OutlierDetection.max_ejection_time>` limits ejection time to 5 minutes. Additionally, when the node stays healthy, ejection time decreases. See :ref:`ejection algorithm<arch_overview_outlier_detection_algorithm>` for more info. Previously, ejection time could grow without limit and never decreased.
Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ constexpr const char* runtime_features[] = {
"envoy.reloadable_features.http_transport_failure_reason_in_body",
"envoy.reloadable_features.http2_skip_encoding_empty_trailers",
"envoy.reloadable_features.listener_in_place_filterchain_update",
"envoy.reloadable_features.lua_always_wrap_body",
"envoy.reloadable_features.overload_manager_disable_keepalive_drain_http2",
"envoy.reloadable_features.prefer_quic_kernel_bpf_packet_routing",
"envoy.reloadable_features.preserve_query_string_in_path_redirects",
Expand Down
15 changes: 12 additions & 3 deletions source/extensions/filters/http/lua/lua_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "common/config/datasource.h"
#include "common/crypto/utility.h"
#include "common/http/message_impl.h"
#include "common/runtime/runtime_features.h"

#include "absl/strings/escaping.h"

Expand Down Expand Up @@ -421,13 +422,21 @@ int StreamHandleWrapper::luaBody(lua_State* state) {
if (end_stream_) {
if (!buffered_body_ && saw_body_) {
return luaL_error(state, "cannot call body() after body has been streamed");
} else if (callbacks_.bufferedBody() == nullptr) {
ENVOY_LOG(debug, "end stream. no body");
return 0;
} else {
if (body_wrapper_.get() != nullptr) {
body_wrapper_.pushStack();
} else {
if (callbacks_.bufferedBody() == nullptr) {
ENVOY_LOG(debug, "end stream. no body");

if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.lua_always_wrap_body")) {
return 0;
}

Buffer::OwnedImpl body(EMPTY_STRING);
callbacks_.addData(body);
}

body_wrapper_.reset(Filters::Common::Lua::BufferWrapper::create(
state, const_cast<Buffer::Instance&>(*callbacks_.bufferedBody())),
true);
Expand Down
4 changes: 2 additions & 2 deletions test/extensions/filters/http/lua/lua_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ class LuaHttpFilterTest : public testing::Test {
function envoy_on_request(request_handle)
request_handle:logTrace(request_handle:headers():get(":path"))

if request_handle:body() ~= nil then
if request_handle:body():length() ~= 0 then

@mattklein123 mattklein123 Jan 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried about this change breaking scripts, but I'm not sure how likely that is. I guess with a runtime guard we can see how it goes? The alternative would be to allow body() to take an optional boolean which enables the behavior? This could default to the existing behavior? I think I'm probably leaning towards that now that I think about it? @dio WDYT?

@dio dio Jan 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this PR introduces a new runtime feature flag (and tested in integration test).

But yeah, thinking it again, to have a cleaner approach: seems like bool (or enum (Envoy.ALWAYS_WRAP_BODY)? for syntactic sugar, similar to this PR) or a new API (e.g. wrappedBody(), this new API always has a non-nil "buffer").

-- with bool.
response_handle:body(true):setBytes("ok")

-- with "enum".
response_handle:body(Envoy.ALWAYS_WRAP_BODY):setBytes("ok")

-- with new API.
response_handle:wrappedBody():setBytes("ok")

@spacewander spacewander Jan 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will vote for the first one, because:

  1. argument is enough to distinguish them, not need to add an extra method
  2. there is only "always wrap body" and "not always wrap body", so a boolean is enough.

@dio dio Jan 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG. The reason for offering new API is for readability in the Lua code.

request_handle:logTrace(request_handle:body():length())
else
request_handle:logTrace("no body")
Expand All @@ -193,7 +193,7 @@ class LuaHttpFilterTest : public testing::Test {
function envoy_on_request(request_handle)
request_handle:logTrace(request_handle:headers():get(":path"))

if request_handle:body() ~= nil then
if request_handle:body():length() ~= 0 then
Comment thread
dio marked this conversation as resolved.
Outdated
request_handle:logTrace(request_handle:body():length())
else
request_handle:logTrace("no body")
Expand Down
84 changes: 73 additions & 11 deletions test/extensions/filters/http/lua/lua_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,11 @@ class LuaIntegrationTest : public testing::TestWithParam<Network::Address::IpVer
registerTestServerPorts({"http"});
}

void testRewriteResponse(const std::string& code) {
void expectResponseBodyRewrite(const std::string& code, bool empty_body, bool enable_wrap_body) {
if (!enable_wrap_body) {
config_helper_.addRuntimeOverride("envoy.reloadable_features.lua_always_wrap_body", "false");
}

initializeFilter(code);
codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http")));
Http::TestRequestHeaderMapImpl request_headers{{":method", "POST"},
Expand All @@ -164,22 +168,40 @@ class LuaIntegrationTest : public testing::TestWithParam<Network::Address::IpVer
waitForNextUpstreamRequest();

Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, {"foo", "bar"}};
upstream_request_->encodeHeaders(response_headers, false);
Buffer::OwnedImpl response_data1("good");
upstream_request_->encodeData(response_data1, false);
Buffer::OwnedImpl response_data2("bye");
upstream_request_->encodeData(response_data2, true);

if (empty_body) {
upstream_request_->encodeHeaders(response_headers, true);
} else {
upstream_request_->encodeHeaders(response_headers, false);
Buffer::OwnedImpl response_data1("good");
upstream_request_->encodeData(response_data1, false);
Buffer::OwnedImpl response_data2("bye");
upstream_request_->encodeData(response_data2, true);
}

response->waitForEndStream();

EXPECT_EQ("2", response->headers()
.get(Http::LowerCaseString("content-length"))[0]
->value()
.getStringView());
EXPECT_EQ("ok", response->body());
if (enable_wrap_body) {
EXPECT_EQ("2", response->headers()
.get(Http::LowerCaseString("content-length"))[0]
->value()
.getStringView());
EXPECT_EQ("ok", response->body());
} else {
EXPECT_EQ("", response->body());
}

cleanup();
}

void testRewriteResponse(const std::string& code) {
expectResponseBodyRewrite(code, false, true);
}

void testRewriteResponseWithoutUpstreamBody(const std::string& code, bool enable_wrap_body) {
expectResponseBodyRewrite(code, true, enable_wrap_body);
}

void cleanup() {
codec_client_->close();
if (fake_lua_connection_ != nullptr) {
Expand Down Expand Up @@ -974,6 +996,46 @@ name: lua
testRewriteResponse(FILTER_AND_CODE);
}

// Rewrite response buffer, without original upstream response body.
TEST_P(LuaIntegrationTest, RewriteResponseBufferWithoutUpstreamBody) {
const std::string FILTER_AND_CODE =
R"EOF(
name: lua
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua
inline_code: |
function envoy_on_response(response_handle)
local content_length = response_handle:body():setBytes("ok")
response_handle:logTrace(content_length)

response_handle:headers():replace("content-length", content_length)
end
)EOF";

testRewriteResponseWithoutUpstreamBody(FILTER_AND_CODE, true);
}

// Rewrite response buffer, without original upstream response body
// and disable lua_always_wrap_body feature.
TEST_P(LuaIntegrationTest, RewriteResponseBufferWithoutUpstreamBodyAndDisableWrapBody) {
const std::string FILTER_AND_CODE =
R"EOF(
name: lua
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua
inline_code: |
function envoy_on_response(response_handle)
if response_handle:body() then
local content_length = response_handle:body():setBytes("ok")

@mattklein123 mattklein123 Jan 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. Shouldn't this end up returning nil and the script should crash? Shouldn't we be verifying nil here instead?

@spacewander spacewander Jan 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response_handle:body(false) always returns nil so this branch is skipped. It is just a mirror of the previous test but with the false given.

@mattklein123 mattklein123 Jan 6, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I missed that. OK LGTM modulo the typo.

response_handle:logTrace(content_length)
response_handle:headers():replace("content-length", content_length)
end
end
)EOF";

testRewriteResponseWithoutUpstreamBody(FILTER_AND_CODE, false);
}

// Rewrite chunked response body.
TEST_P(LuaIntegrationTest, RewriteChunkedBody) {
const std::string FILTER_AND_CODE =
Expand Down