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

spacewander · 2020-12-20T06:59:36Z

Commit Message: allow setting response body when the upstream response body is empty
Additional Description:
Risk Level: Medium
Testing: unit and integration tests
Docs Changes: TBA
Release Notes: Lua body() will no longer return nil when the response body is empty. Can be changed via runtime flag.
Platform Specific Features: N/A
Fixes #14226

Signed-off-by: spacewander spacewanderlzx@gmail.com

…y is empty Close envoyproxy#14226. Signed-off-by: spacewander <spacewanderlzx@gmail.com>

dio · 2020-12-22T03:19:57Z

Can you merge master? Thanks!

dio

The approach seems OK. A nit and a request for refactoring the test helper.

source/extensions/filters/http/lua/lua_filter.cc

dio · 2020-12-22T03:28:45Z

test/extensions/filters/http/lua/lua_integration_test.cc

+    end
+)EOF";
+
+  testRewriteResponseWithoutUpstreamBody(FILTER_AND_CODE);


While you're here, can we refactor this a bit to have e.g. expectResponseBodyRewrite(), parameterized with the body from upstream (this can be empty or filled (buffered)) and the expected rewritten body?

So we can reuse this to test both cases.

Thank you!

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

test/extensions/filters/http/lua/lua_filter_test.cc

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

docs/root/version_history/current.rst

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

dio · 2020-12-27T07:04:59Z

Thanks, @spacewander, could you merge main? Probably it will help to remedy:

2020-12-27T03:11:40.3528486Z Code coverage for source/common/network is lower than limit of 95.3 (95.2)

/wait

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

dio · 2020-12-28T23:21:51Z

Interesting since "master" has that 95.3% coverage for source/common/network. https://storage.googleapis.com/envoy-postsubmit/master/coverage/index.html. Need to add more coverage on that to make it pass :).

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander · 2020-12-29T04:25:29Z

@dio
I looked into source/common/network and can't find anywhere I can add test. It is quite weird since I don't touch this part of code.
Now I rerun the test and it failed on source/extensions/common/wasm: https://dev.azure.com/cncf/envoy/_build/results?buildId=62373&view=logs&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&t=e00c5a13-c6dc-5e9a-6104-69976170e881
I don't touch source/extensions/common/wasm too...
Maybe we can relax this limitation since it causes flaky report?

dio · 2020-12-29T22:50:59Z

Since this https://github.com/envoyproxy/envoy/pull/14528/files is merged. I think we can do another main branch merging. Sorry for the hassle.

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander · 2020-12-30T04:04:14Z

@dio
Thanks! Finally it pass all the checks.

dio

Thanks!

mattklein123

Thanks for working on this. A question on overall design to get started. Thank you!

/wait

mattklein123 · 2021-01-03T00:03:54Z

docs/root/version_history/current.rst

@@ -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`.


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.

mattklein123 · 2021-01-03T00:06:41Z

test/extensions/filters/http/lua/lua_filter_test.cc

@@ -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


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?

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")

I will vote for the first one, because:

argument is enough to distinguish them, not need to add an extra method

there is only "always wrap body" and "not always wrap body", so a boolean is enough.

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

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

mattklein123

Thanks this looks good. Can you check CI also?

/wait

docs/root/configuration/http/http_filters/lua_filter.rst

mattklein123 · 2021-01-04T18:43:37Z

source/extensions/filters/http/lua/lua_filter.cc

@@ -418,16 +418,31 @@ int StreamHandleWrapper::luaHeaders(lua_State* state) {
 int StreamHandleWrapper::luaBody(lua_State* state) {
  ASSERT(state_ == State::Running);

+  int always_wrap_body = 0;


nit: use bool here

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander · 2021-01-05T03:07:49Z

@mattklein123
All tests pass.

mattklein123

Thanks LGTM at a high level with some test comments.

/wait

mattklein123 · 2021-01-05T17:37:28Z

test/extensions/filters/http/lua/lua_integration_test.cc

@@ -974,6 +992,47 @@ name: lua
  testRewriteResponse(FILTER_AND_CODE);
 }

+// Rewrite response buffer, without original upstream response body
+// and always warp body.


Suggested change

// and always warp body.

// and always wrap body.

@mattklein123
Typo fixed.

mattklein123 · 2021-01-05T17:38:47Z

test/extensions/filters/http/lua/lua_integration_test.cc

+  inline_code: |
+    function envoy_on_response(response_handle)
+      if response_handle:body(false) then
+        local content_length = response_handle:body():setBytes("ok")


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

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.

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

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

mattklein123

Thanks!

* master: (48 commits) Resolve 14506, avoid libidn2 for our curl dependency (envoyproxy#14601) fix new/free mismatch in Mainthread utility (envoyproxy#14596) opencensus: deprecate Zipkin configuration. (envoyproxy#14576) upstream: clean up code location (envoyproxy#14580) configuration impl: add cast for ios compilation (envoyproxy#14590) buffer impl: add cast for android compilation (envoyproxy#14589) ratelimit: add dynamic metadata to ratelimit response (envoyproxy#14508) tcp_proxy: wait for CONNECT response before start streaming data (envoyproxy#14317) stream info: cleanup address handling (envoyproxy#14432) [deps] update upb to latest commit (envoyproxy#14582) Add utility to check whether the execution is in main thread. (envoyproxy#14457) listener: undeprecate bind_to_port (envoyproxy#14480) Fix data race in overload integration test (envoyproxy#14586) deps: update PGV (envoyproxy#14571) dependencies: update cve_scan.py for some libcurl 7.74.0 false positives. (envoyproxy#14572) Network::Connection: Add L4 crash dumping support (envoyproxy#14509) ssl: remember stat names for configured ciphers. (envoyproxy#14534) formatter: add custom date formatting to downstream cert start and end dates (envoyproxy#14502) feat(lua): allow setting response body when the upstream response body is empty (envoyproxy#14486) Generalize the gRPC access logger base classes (envoyproxy#14469) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

spacewander force-pushed the body branch from efce2e9 to 96d7c46 Compare December 20, 2020 07:31

feat(lua): allow setting response body when the upstream response bod…

c1144f3

…y is empty Close envoyproxy#14226. Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander force-pushed the body branch from 96d7c46 to c1144f3 Compare December 20, 2020 12:04

spacewander marked this pull request as ready for review December 20, 2020 13:21

mattklein123 assigned dio Dec 21, 2020

dio suggested changes Dec 22, 2020

View reviewed changes

Merge branch 'master' into body

9ad27b2

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

dio reviewed Dec 22, 2020

View reviewed changes

test/extensions/filters/http/lua/lua_filter_test.cc Outdated Show resolved Hide resolved

spacewander added 3 commits December 22, 2020 19:20

Add runtime flag

b209dcf

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

format

240ff41

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

release note

d387335

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

dio reviewed Dec 27, 2020

View reviewed changes

docs/root/version_history/current.rst Outdated Show resolved Hide resolved

sorted

229d2de

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

repokitteh-read-only bot added the waiting label Dec 27, 2020

Merge branch 'master' into body

ade3534

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

repokitteh-read-only bot removed the waiting label Dec 27, 2020

Merge branch 'master' into body

40fd113

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

Merge branch 'master' into body

09b05bb

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

dio previously approved these changes Dec 30, 2020

View reviewed changes

mattklein123 requested changes Jan 3, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 3, 2021

mattklein123 self-assigned this Jan 3, 2021

new way

0101c9c

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander dismissed dio’s stale review via 0101c9c January 4, 2021 11:45

repokitteh-read-only bot removed the waiting label Jan 4, 2021

doc lint

63d1354

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

mattklein123 requested changes Jan 4, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 4, 2021

tweak for review

d59c8e1

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

repokitteh-read-only bot removed the waiting label Jan 5, 2021

repokitteh-read-only bot added the waiting label Jan 5, 2021

mattklein123 requested changes Jan 5, 2021

View reviewed changes

typo

c8cf954

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

repokitteh-read-only bot removed the waiting label Jan 6, 2021

mattklein123 approved these changes Jan 6, 2021

View reviewed changes

mattklein123 merged commit d9f5edd into envoyproxy:master Jan 6, 2021

spacewander mentioned this pull request Jan 28, 2021

chore: correct grammar in upstream/connection_pooling.rst #14846

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

spacewander commented Dec 20, 2020 •

edited

Loading

dio commented Dec 22, 2020

dio left a comment

dio Dec 22, 2020

dio commented Dec 27, 2020

dio commented Dec 28, 2020

spacewander commented Dec 29, 2020

dio commented Dec 29, 2020

spacewander commented Dec 30, 2020

dio left a comment

mattklein123 left a comment

mattklein123 Jan 3, 2021

mattklein123 Jan 3, 2021

dio Jan 3, 2021

spacewander Jan 4, 2021

dio Jan 4, 2021

mattklein123 left a comment

mattklein123 Jan 4, 2021

spacewander commented Jan 5, 2021

mattklein123 left a comment

mattklein123 Jan 5, 2021

spacewander Jan 6, 2021

mattklein123 Jan 5, 2021

spacewander Jan 6, 2021

mattklein123 Jan 6, 2021

mattklein123 left a comment

	* 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.

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

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

Conversation

spacewander commented Dec 20, 2020 • edited Loading

dio commented Dec 22, 2020

dio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Dec 27, 2020

dio commented Dec 28, 2020

spacewander commented Dec 29, 2020

dio commented Dec 29, 2020

spacewander commented Dec 30, 2020

dio left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacewander commented Jan 5, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

spacewander commented Dec 20, 2020 •

edited

Loading