lua: Add per route config by dio · Pull Request #9279 · envoyproxy/envoy

dio · 2019-12-09T15:56:04Z

Description: This patch adds Lua HTTP filter per route config.
Risk Level: Low
Testing: unit test, integration test
Docs Changes: Added
Release Notes: Added
Fixes #3124

Signed-off-by: Dhi Aurrahman dio@tetrate.io

repokitteh-read-only · 2019-12-09T15:56:10Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9279 was opened by dio.

see: more, trace.

This patch adds Lua HTTP filter per route config. Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2019-12-11T07:07:27Z

@mattklein123 I'm not sure if this is the right approach to implement this. WDYT?

mattklein123

Nice, this is cool. A few API comments to get started.

/wait

mattklein123 · 2019-12-11T17:40:57Z

api/envoy/config/filter/http/lua/v2/lua.proto

  // strings so complex scripts can be easily expressed inline in the configuration.
  string inline_code = 1 [(validate.rules).string = {min_bytes: 1}];
+
+  // The Lua source codes that Envoy will execute based on name. Key is Lua source code name, e.g.


nit: s/source codes/source code

mattklein123 · 2019-12-11T17:41:10Z

api/envoy/config/filter/http/lua/v2/lua.proto

+  // The Lua source codes that Envoy will execute based on name. Key is Lua source code name, e.g.
+  // ``foo.lua``, this name can be referenced by
+  // :ref:`LuaPerRoute <envoy_api_msg_config.filter.http.lua.v2.LuaPerRoute>`.
+  map<string, api.v2.core.DataSource> source_codes = 2;


nit: s/source_codes/source_code

mattklein123 · 2019-12-11T17:44:28Z

api/envoy/config/filter/http/lua/v2/lua.proto

@@ -18,4 +20,23 @@ message Lua {
  // be properly escaped. YAML configuration may be easier to read since YAML supports multi-line
  // strings so complex scripts can be easily expressed inline in the configuration.
  string inline_code = 1 [(validate.rules).string = {min_bytes: 1}];


From an API standpoint, it's kind of strange that we still require inline code if we now support named code. Should we now potentially relax requiring inline_code and deprecate it, and optionally allow a root/global code by name?

mattklein123 · 2019-12-11T17:47:33Z

api/envoy/config/filter/http/lua/v2/lua.proto

+
+    // The name of a Lua source code defined in
+    // :ref:`Lua.source_codes <envoy_api_field_config.filter.http.lua.v2.Lua.source_codes>`.
+    string name = 2 [(validate.rules).string = {min_bytes: 1}];


I get that you might want to lookup the code by name for performance/de-dup reasons, but wouldn't you also potentially want to allow the code to be delivered via RDS? I think this would block that? Should this be a oneof that allows either lookup by name or direct inline code?

To clarify, "direct inline code" here is to open the possibility to "compile" code on the fly (when the route is being called)?

Yeah that's what I meant, or at least have some type of facility to see if we already have a thread local Lua context ready for a per-route code? I'm not sure if we would then need some type of callback to clean this up. I would need to look into all of the ownership stuff in more detail to advise. But I think this is optimally how it would work and we should optimally make sure the API works for that? WDYT?

SGTM. I'll update the PR to accommodate that first then.

erikbos · 2019-12-15T13:12:12Z

@dio Thanks for implementing this! I just gave docker.io/dio123/envoy-lua a try with the following configuration:

          - match:
              prefix: "/route1"
            route:
              cluster: test
              prefix_rewrite: "/people"
            per_filter_config:
              envoy.lua:
                name: route1.lua
          - match:
              prefix: "/route2"
            route:
              cluster: test
              prefix_rewrite: "/people"
            per_filter_config:
              envoy.lua:
                name: route2.lua

      - name: envoy.lua
        typed_config:
          "@type": type.googleapis.com/envoy.config.filter.http.lua.v2.Lua
          inline_code: |
            function envoy_on_response(request_handle)
              request_handle:headers():add("Global catchall","Delivery catch all")
            end

          source_codes:
            route1.lua:
              inline_string: |
                function envoy_on_request(request_handle)
                  request_handle:headers():add("Requested","route 1")
                end
                function envoy_on_response(response_handle)
                  response_handle:headers():add("Handled","route 1")
                end

            route3.lua:
              inline_string: |
                function envoy_on_request(request_handle)
                  request_handle:headers():add("Requested","route 2")
                end
                function envoy_on_response(response_handle)
                  response_handle:headers():add("Handled","route 2")
                end

A few observations:

Requesting /route1 works fine: upstream receives additional header, but I don't see the downstream header "Handled: route1". Is my configuration correct?
Requesting /route2 (referring "route3.lua" which does not exist) results in Envoy logging 200 status code logged, with RESPONSE_FLAGS = "-", client gets disconnected and no upstream cluster was contacted. Looks wrong? Shouldn't this result in a clean http 503 response with a corresponding response flag?
The global envoy_on_response() does not get called next to the route specific lua.. (perhaps related to 1) Is that functionally actually still supposed to work? (Would be nice :-)
Envoy does not complain about route2.lua not existing, I assume because of it's eventual consistency model? (I'm an envoy beginner)

(To comment on Matt's remark: yes I'd love to deliver all of this via xDS)

erikbos · 2019-12-15T16:03:07Z

Played with it a bit more:

Requesting /route1 works fine: upstream receives additional header, but I don't see the downstream header "Handled: route1". Is my configuration correct?

Adding an empty non-route specific envoy_on_request() ensures route1.lua's envoy_on_response() does run.. (hm.. how is possible that these two influence each other?)

Requesting /route2 (referring "route3.lua" which does not exist) results in Envoy logging 200 status code logged, with RESPONSE_FLAGS = "-", client gets disconnected and no upstream cluster was contacted. Looks wrong? Shouldn't this result in a clean http 503 response with a corresponding response flag?

Found the reason: this was caused by setting a non-valid header "Global catchall" with a space in it, my bad.

The global envoy_on_response() does not get called next to the route specific lua.. (perhaps related to 1) Is that functionally actually still supposed to work? (Would be nice :-)

With 2 fixed it appears it falls back to running the non-route specific lua in case the per-route function does not exist.

And I just noticed a per-route lua function followed by a for-all-routes lua function can be defined by having multiple "envoy.lua" filters in sequence. Nice 👍

dio · 2019-12-15T20:39:58Z

@erikbos Thanks for testing the sketch.

(To comment on Matt's remark: yes I'd love to deliver all of this via xDS)

I'll try to add this one.

dio · 2019-12-15T20:43:06Z

Re your issue on 1, I'll try to check it out and do some tests.

stale · 2019-12-27T22:28:56Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-01-03T22:38:55Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

wbpcode · 2020-05-11T02:32:50Z

Is there any progress in this work?

dio · 2020-05-11T02:57:14Z

@wbpcode yes, I've been working on something else. I'll try to update this week.

wbpcode · 2020-05-11T07:54:14Z

source/extensions/filters/common/lua/lua.h

-  ThreadLocal::SlotPtr tls_slot_;
-  uint64_t current_global_slot_{};
+  absl::flat_hash_map<std::string, ThreadLocal::SlotPtr> tls_slot_map_;
+  absl::flat_hash_map<std::string, uint64_t> current_global_slot_map_;


Why not keep ThreadLocalState unchanged, but maintain a map in FilterConfig to manage different Lua scripts?

wbpcode · 2020-05-11T07:56:52Z

Thanks. I am very much looking forward to the completion of this function, because I just need this function recently.

wbpcode · 2020-05-14T12:10:49Z

@dio I implemented a new version based on your commit and it seems to work fine. If you are too busy recently, maybe I can continue to complete this function based on your work.

erikbos · 2020-05-14T12:31:22Z

I'd like to see this functionality included as well :-) I am available for testing if you need a review.

repokitteh-read-only bot added the api label Dec 9, 2019

dio added 5 commits December 9, 2019 21:29

lua: Add per route config

8ed4111

This patch adds Lua HTTP filter per route config. Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Merge remote-tracking branch 'upstream/master'

d5253bf

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Add docs

9fdca66

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Fix

1766e42

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Add unit tests and release notes.

dfb2460

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio marked this pull request as ready for review December 10, 2019 21:56

dio changed the title ~~WIP: lua: Add per route config~~ lua: Add per route config Dec 10, 2019

Merge remote-tracking branch 'upstream/master' into per-route-lua-filter

39e1fab

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio mentioned this pull request Dec 10, 2019

Support per-route config for Lua #3124

Closed

mattklein123 self-assigned this Dec 11, 2019

mattklein123 requested changes Dec 11, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 11, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 27, 2019

stale bot closed this Jan 3, 2020

wbpcode reviewed May 11, 2020

View reviewed changes

wbpcode mentioned this pull request May 18, 2020

lua: Add per filter config for Lua filter #11235

Merged

wbpcode mentioned this pull request Aug 4, 2020

Dispatch inline Lua code in per filter config of Lua filter by RDS #12460

Closed

Conversation

dio commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Dec 9, 2019

Uh oh!

dio commented Dec 11, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

dio Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

dio Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

erikbos commented Dec 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erikbos commented Dec 15, 2019

Uh oh!

dio commented Dec 15, 2019

Uh oh!

dio commented Dec 15, 2019

Uh oh!

stale bot commented Dec 27, 2019

Uh oh!

stale bot commented Jan 3, 2020

Uh oh!

wbpcode commented May 11, 2020

Uh oh!

dio commented May 11, 2020

Uh oh!

wbpcode May 11, 2020

Choose a reason for hiding this comment

Uh oh!

wbpcode commented May 11, 2020

Uh oh!

wbpcode commented May 14, 2020

Uh oh!

erikbos commented May 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dio commented Dec 9, 2019 •

edited

Loading

dio Dec 14, 2019 •

edited

Loading

erikbos commented Dec 15, 2019 •

edited

Loading