lua: Add per filter config for Lua filter by wbpcode · Pull Request #11235 · envoyproxy/envoy

wbpcode · 2020-05-18T01:37:40Z

Commit Message: Add per filter config support for Lua filter.

This allows Lua filter to support per-route configuration. This patch enables the configured Lua filter to have multiple registered codes that can be referenced from each per-route config. Disabling running the global Lua filter for a route is also supported.

Risk Level: Low
Testing: Added.
Docs Changes: Added.
Release Notes: Added.
Fixes: #9279 #3124

repokitteh-read-only · 2020-05-18T01:37:46Z

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

🐱

Caused by: #11235 was opened by wbpcode.

see: more, trace.

htuch · 2020-05-18T14:41:21Z

api/envoy/extensions/filters/http/lua/v3/lua.proto

Should name and code be in a oneof?

The name is used to refer to a Lua script in the global configuration, and the code is used to configure a Lua script directly in the Route. They should not be configured repeatedly.

I kinda agree with @htuch, what is the expected behavior when we set both?

We cannot set the name and code at the same time. If you need to use the same script in different routes, you can send the script through LDS, and then set the name field in the route configuration. When the script is large, a part of redundant configuration can be reduced. The code is for better flexibility, to avoid all scripts must be issued using LDS.

I suggest a oneof for this.

OK, it seems like it is. One of disabled, name, or code.

Looks good, but nit, rename s/code/source_code/ for consistency with source_codes.

@wbpcode could you rename this to source_code as per @htuch's comment?

wbpcode · 2020-05-19T12:26:25Z

This PR is based on @dio 's work.
Lua per route config allows users to execute different Lua scripts in different routes. It also allows users to directly reference a Lua script in the global configuration. The following is a typical configuration.

          http_filters:
          - name: envoy.lua
            config:
              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
                route2.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

              routes:
              - match:
                  prefix: "/route1"
                route:
                  prefix_rewrite: /anything
                  cluster: httpbin
                per_filter_config:
                  envoy.lua:
                    name: route1.lua        # Ref 'route1.lua' in global configuration.
              - match:
                  prefix: "/route2"         # No config and GLOBAL Lua script will be loaded.
                route:
                  prefix_rewrite: /anything
                  cluster: httpbin
              - match:
                  prefix: "/route4"
                route:
                  prefix_rewrite: /anything
                  cluster: httpbin
                per_filter_config:
                  envoy.lua:
                    disabled: true           # Lua filter is disabled
              - match:
                  prefix: "/route5"
                route:
                  prefix_rewrite: /anything
                  cluster: httpbin
                per_filter_config:
                  envoy.lua:
                    code:                     # Lua script embedded in the route
                      inline_string: |
                        function envoy_on_request(request_handle)
                          request_handle:headers():add("Requested","route in route")
                        end
                        function envoy_on_response(response_handle)
                          response_handle:headers():add("Handled","route in route")
                        end

jmarantz · 2020-05-19T13:10:36Z

@wbpcode please do not force-push in Envoy PRs. It makes reviewing your PR impossible.

Also, can you fix format errors? That will unblock the rest of CI.

Thank you!

Signed-off-by: wbpcode <comems@msn.com>

wbpcode · 2020-05-19T13:26:03Z

@wbpcode please do not force-push in Envoy PRs. It makes reviewing your PR impossible.

Also, can you fix format errors? That will unblock the rest of CI.

Thank you!

Okay.

Signed-off-by: wbpcode <comems@msn.com>

stale · 2020-05-30T08:22:37Z

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-06-06T09:10:34Z

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!

erikbos · 2020-06-10T06:50:47Z

Any luck of this one getting reviewed and merged in?

dio

Thanks for picking this up. Sorry for the super late response. Seems like this one is fell off my radar. Sorry.

Functionally it looks good. However, we need to add docs as well regarding this newly introduced features.

dio · 2020-06-10T07:44:21Z

api/envoy/extensions/filters/http/lua/v3/lua.proto

I kinda agree with @htuch, what is the expected behavior when we set both?

docs/root/version_history/current.rst

api/envoy/extensions/filters/http/lua/v3/lua.proto

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

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

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

dio

Requesting changes for this. 🙂

dio · 2020-06-10T08:17:27Z

@wbpcode I also appreciate it if you could elaborate on the changes introduced in this PR into a proper commit message. Thanks!

wbpcode · 2020-06-10T08:32:43Z

Thanks for your detailed review and I will try to update this PR ASAP. @dio

wbpcode · 2020-06-10T08:34:10Z

@wbpcode I also appreciate it if you could elaborate on the changes introduced in this PR into a proper commit message. Thanks!

I will.

dio · 2020-06-10T08:40:09Z

I think you should do "merge master" as well. Thank you!

Signed-off-by: wbpcode <comems@msn.com>

wbpcode · 2020-06-30T10:09:29Z

A new commit that reduced redundant code and updated integration test.

wbpcode · 2020-06-30T12:24:16Z

/retest

repokitteh-read-only · 2020-06-30T12:24:19Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #11235 (comment) was created by @wbpcode.

see: more, trace.

mattklein123

Thanks, nice work!

mattklein123 · 2020-06-30T23:07:26Z

Please check CI and merge master.

/wait

dio · 2020-06-30T23:15:11Z

api/envoy/extensions/filters/http/lua/v3/lua.proto

+
+    // A name of a Lua source code stored in
+    // :ref:`Lua.source_codes <envoy_v3_api_field_extensions.filters.http.lua.v3.Lua.source_codes>`.
+    string name = 2 [(validate.rules).string.min_len = 1];


string name = 2 [(validate.rules).string = {min_len: 1}];

Signed-off-by: wbpcode <comems@msn.com>

wbpcode · 2020-07-01T09:22:54Z

/retest

repokitteh-read-only · 2020-07-01T09:22:57Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #11235 (comment) was created by @wbpcode.

see: more, trace.

wbpcode · 2020-07-01T09:34:24Z

hmmm, look some wrong with this windows ci.

mattklein123

Thanks!

repokitteh-read-only bot added the api label May 18, 2020

htuch reviewed May 18, 2020

View reviewed changes

wbpcode force-pushed the lua branch 2 times, most recently from 4a85e30 to 480f466 Compare May 19, 2020 12:12

jmarantz assigned dio May 19, 2020

Add per filter config for lua filter

4c43ab6

Signed-off-by: wbpcode <comems@msn.com>

wbpcode force-pushed the lua branch from 480f466 to 4c43ab6 Compare May 19, 2020 13:15

Fix: fix spelling error in lua filter

de28cee

Signed-off-by: wbpcode <comems@msn.com>

wbpcode changed the title ~~[WIP] Add per filter config for lua filter~~ Add per filter config for lua filter May 20, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 30, 2020

stale bot closed this Jun 6, 2020

dio reopened this Jun 10, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 10, 2020

dio reviewed Jun 10, 2020

View reviewed changes

dio suggested changes Jun 10, 2020

View reviewed changes

dio changed the title ~~Add per filter config for lua filter~~ lua: Add per filter config for Lua filter Jun 10, 2020

Merge branch 'master' of github.com:envoyproxy/envoy into lua

7a4eea0

wbpcode changed the title ~~lua: Add per filter config for Lua filter~~ [WIP] lua: Add per filter config for Lua filter Jun 11, 2020

update function/variable name and wrapped a function to get per lua code

6e18740

Signed-off-by: wbpcode <comems@msn.com>

wbpcode added 2 commits June 30, 2020 15:31

Lua: simplified some redundant code

e089801

Signed-off-by: wbpcode <comems@msn.com>

Lua: remove xds stuff of integration test temporarily

a755849

Signed-off-by: wbpcode <comems@msn.com>

wbpcode dismissed dio’s stale review via a755849 June 30, 2020 09:33

repokitteh-read-only bot removed the waiting label Jun 30, 2020

mattklein123 previously approved these changes Jun 30, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jun 30, 2020

repokitteh-read-only bot added the waiting label Jun 30, 2020

dio reviewed Jun 30, 2020

View reviewed changes

wbpcode added 2 commits July 1, 2020 10:32

Lua: correct format error of lua.proto

3d0eb40

Signed-off-by: wbpcode <comems@msn.com>

Merge branch 'master' of github.com:envoyproxy/envoy into lua

f6064fa

wbpcode dismissed mattklein123’s stale review via f6064fa July 1, 2020 03:54

repokitteh-read-only bot added api and removed waiting labels Jul 1, 2020

mattklein123 approved these changes Jul 1, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jul 1, 2020

mattklein123 merged commit 75930e3 into envoyproxy:master Jul 1, 2020

wbpcode mentioned this pull request Jul 7, 2020

Segmentation faults in Lua filter (LuaThreadLocal) #10241

Closed

This was referenced Aug 4, 2020

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

Closed

support inline Lua code in per filter config #12516

Merged

wbpcode mentioned this pull request Jul 12, 2022

lua: deprecate inline_code and use default_source_code #22125

Merged

wbpcode deleted the lua branch April 8, 2024 09:42

Conversation

wbpcode commented May 18, 2020 • edited by dio Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented May 19, 2020

Uh oh!

wbpcode commented May 19, 2020

Uh oh!

stale bot commented May 30, 2020

Uh oh!

stale bot commented Jun 6, 2020

Uh oh!

erikbos commented Jun 10, 2020

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

dio commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Jun 10, 2020

Uh oh!

wbpcode commented Jun 10, 2020

Uh oh!

dio commented Jun 10, 2020

Uh oh!

wbpcode commented Jun 30, 2020

Uh oh!

wbpcode commented Jun 30, 2020

Uh oh!

repokitteh-read-only bot commented Jun 30, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jun 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Jul 1, 2020

Uh oh!

repokitteh-read-only bot commented Jul 1, 2020

Uh oh!

wbpcode commented Jul 1, 2020

Uh oh!

wbpcode commented May 18, 2020 •

edited by dio

Loading

wbpcode commented May 19, 2020 •

edited

Loading

dio commented Jun 10, 2020 •

edited

Loading