feat(core) exposing rewrite_by_lua handler to plugins #2354

subnetmarco · 2017-04-06T21:45:07Z

The rewrite_by_lua event can now be implemented by plugins.
Introduced no_api in the plugin's schema to determine if a plugin can be applied to a specific API.

subnetmarco · 2017-04-06T22:00:45Z

I have added a test that demonstrates rewriting a random URI to a URI that will later matched to an API.

Tieske · 2017-04-06T22:30:19Z

do we need this for some reason? if not urgent, maybe better to postpone until a new plugin api has been designed?

subnetmarco · 2017-04-06T22:36:33Z

do we need this for some reason?

Business requirements 😄.

Tieske · 2017-04-10T20:44:53Z

kong/kong.lua

+  core.rewrite.before()
+
+  for _, plugin in ipairs(singletons.loaded_plugins) do
+    plugin.handler:rewrite()


This currently runs the handler on every plugin loaded in the system. Say we now have some 25 plugins, so on every request we're now invoking 25 plugins. Not just configured ones, but all available ones.

As the rewrite phase does not have a consumer yet, and also doesn't have an api yet, the only plugins that should be selected here are the global ones.

Good point - I will update the PR to only chose plugins where api_id == nil and consumer_id == nil.

@Tieske done

Tieske · 2017-04-11T10:24:00Z

kong/kong.lua

+
+  core.rewrite.after()
+end
+


the above runs on loaded_plugins which means that it will run all plugins available, no matter what. So the user has no control on whether it runs or not, when it is in the system, it will execute.

It should fetch the plugins from the database, and execute only those that are neither attached to a consumer, nor to an api (eg. the ones that already run on every api).

btw, can't we bring the api detection forward, to run in the core.rewrite.before() handler? not sure what it takes, but then we can run plugins like in any phase, including "per api" configured plugins

the above runs on loaded_plugins which means that it will run all plugins available, no matter what.

I understand your point but it would be over-optimization IMHO. Currently every other handler also runs on every plugin regardless if they have an event handler associated or not (ie, the access() for every plugin is invoked regardless if the plugin implements that method).

btw, can't we bring the api detection forward, to run in the core.rewrite.before() handler? not sure what it takes, but then we can run plugins like in any phase, including "per api" configured plugins

My use-case is to implement URL rewriting before the APIs are matched, and rewrite the URI to be matched later by the router.

ie, the access() for every plugin is invoked regardless if the plugin implements that method

That's not what I mean. Because that access handler is only called if the user explcitly added that plugin through the management api.

In the code you have now, it will run the handler, even if the user did not configure it. Just a plugin "being in the system" will make it being executed.

hence if you run only the plugins that have been globally added by the user, neither attached to a consumer nor an api, it would work. And it would be up to the user to whether to add this plugin or not.

one more thing: does it currently honour the plugin priority setting?

no_api and no_consumer make sure that the plugin cannot be applied to a specific consumer or API, because in the rewrite phase we cannot possibly know who the consumer is and what API he is trying to consume.

With the last commit I am only executing the rewrite() check for those plugins that satisfy the requirement.

There are two ways to approach this problem

add the no_api flag: run only plugins that have no_consumer and no_api set. This causes a plugin to be configurable ONLY as a global plugin

execute the rewrite handler only for globally configured plugins, without adding the extra complexity of a no_api flag. This allows a plugin to be used either as a global, or api/consumer specific plugin. In the latter case the rewrite handler won't be used. But that is then up to the plugin author...

I find option 1 to be A) more complex due to the extra flag, 2) needlessly limiting flexibility

Option 3: use option 2 above (so select only global plugins when running the rewrite handler), but still add the no_api flag. This allows the author to specify the extra limitation only if his plugin needs it.

@Tieske no_api is important because it prevents, from the Admin API, to target a specific API when it doesn't make any sense to do so (like no_consumer).

@thefosk That's the option 3 above, which is nice. But that is a limitation that kicks in when configuring. But you should not be using that field to select the plugins to run, you should run all plugins that are configured as global, so where consumer_id and api_id are nil.

Running the global plugins is a Kong responsibility
Marking a plugin with no_api is a plugin developer responsibility.
Those are different things.

thibaultcha · 2017-04-11T19:40:39Z

The latest commit broke something, see the travis-ci build.

Tieske · 2017-04-25T12:42:23Z

@thefosk @p0pr0ck5 @thibaultcha updated this as discussed. Now in the rewrite phase only the global plugins will be executed. Hence the rewrite handler of any plugin will only be executed if the plugin is configured as a global one.

Note: do not merge yet, would prefer to merge #2455 first and update this PR before merging (see TODO comments)

Tieske · 2017-04-26T06:35:47Z

update: fixed the update after 2455 was merged, so TODO comments are gone now

thibaultcha

Starting to look good to me! However, a few things in there are lacking, or unneeded...

thibaultcha · 2017-04-26T20:58:13Z

kong/plugins/base_plugin.lua

 end

 function BasePlugin:log()
-  ngx.log(ngx.DEBUG, " executing plugin \""..self._name.."\": log")
+  ngx_log(DEBUG, "executing plugin \"", self._name, "\": log")


Sadly, these are some an unrelated change to this PR and should have been part of a different one...

thibaultcha · 2017-04-26T21:00:15Z

kong/core/handler.lua

+    end,
+    after = function ()
+      local ctx = ngx.ctx
+      ctx.KONG_REWRITE_TIME = get_now() - ctx.KONG_REWRITE_START -- time spent in Kong's rewrite_by_lua


This variable needs to be added to the basic log serializer when computing the latencies.kong property.

thibaultcha · 2017-04-26T21:04:00Z

kong/core/reports.lua

@@ -180,6 +180,9 @@ return {

    create_timer(PING_INTERVAL, ping_handler)
  end,
+  rewrite = function()
+    -- Do nothing
+  end,


I don't think we need this? We manually call reports.init_worker() and reports.log(), but I see nowhere where reports.rewrite() is called. There is no reports.access() either, for example and for that matters.

thibaultcha · 2017-04-26T21:14:41Z

spec/02-integration/05-proxy/10-handler_spec.lua

+
+describe("OpenResty handlers", function()
+
+  describe("rewrite on global plugin", function()


Would have been slightly better to have the sentences be slightly more specific when tests run with --o=gtest flag:

describe("OpenResty phases", function() describe("rewrite_by_lua", function() describe("enabled on all APIs", function() it("runs") end) describe("enabled on a specific APIs", function() it("doesn't run") end) end) end)

reverted api-only config option. Rewrite handler will only be executed on the globally configured plugins

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com> This handler exposes the Nginx rewrite phase. This handler is called for all loaded plugins (since the rewrite phase is executed prior to API matching). From #2354

thibaultcha · 2017-04-28T08:12:50Z

Merged to release/0.10.2 for imminent release.

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com> This handler exposes the Nginx rewrite phase. This handler is called for all loaded plugins (since the rewrite phase is executed prior to API matching). From #2354

subnetmarco force-pushed the feat/rewrite-by-lua branch from 4b25115 to 37508c2 Compare April 6, 2017 22:00

subnetmarco force-pushed the feat/rewrite-by-lua branch from 37508c2 to 96dfc2b Compare April 6, 2017 22:01

subnetmarco mentioned this pull request Apr 10, 2017

docs(plugin-dev) added docs for rewrite_by_lua_block Kong/docs.konghq.com#382

Merged

thibaultcha added the pr/status/needs review label Apr 10, 2017

Tieske reviewed Apr 10, 2017

View reviewed changes

Tieske reviewed Apr 11, 2017

View reviewed changes

subnetmarco force-pushed the feat/rewrite-by-lua branch from 12fb88a to 125f97f Compare April 11, 2017 20:55

p0pr0ck5 assigned Tieske Apr 20, 2017

thibaultcha mentioned this pull request Apr 21, 2017

Can I inject some actions before API handling? #2053

Closed

Tieske force-pushed the feat/rewrite-by-lua branch 2 times, most recently from d76ef57 to efef49e Compare April 25, 2017 12:32

Tieske added this to the 0.10.2 milestone Apr 25, 2017

subnetmarco added 3 commits April 26, 2017 02:38

feat(core) exposing rewrite_by_lua handler to plugins

548ac00

feat(schema) allowing "no_api" in schema

e151d4a

perf(init) preload global plugins

e92276d

Tieske force-pushed the feat/rewrite-by-lua branch from efef49e to 3a5f370 Compare April 26, 2017 06:30

thibaultcha requested changes Apr 26, 2017

View reviewed changes

feat(core) expose rewrite handler on global plugins

09659cb

reverted api-only config option. Rewrite handler will only be executed on the globally configured plugins

Tieske force-pushed the feat/rewrite-by-lua branch from 3a5f370 to 09659cb Compare April 27, 2017 22:32

subnetmarco added 2 commits April 27, 2017 20:58

renaming tests

3f2a8b4

changelog

508c87f

thibaultcha closed this Apr 28, 2017

thibaultcha deleted the feat/rewrite-by-lua branch April 28, 2017 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core) exposing rewrite_by_lua handler to plugins #2354

feat(core) exposing rewrite_by_lua handler to plugins #2354

subnetmarco commented Apr 6, 2017 •

edited

Loading

subnetmarco commented Apr 6, 2017

Tieske commented Apr 6, 2017

subnetmarco commented Apr 6, 2017

Tieske Apr 10, 2017

subnetmarco Apr 10, 2017

subnetmarco Apr 10, 2017

Tieske Apr 11, 2017

subnetmarco Apr 11, 2017

Tieske Apr 11, 2017

Tieske Apr 11, 2017

Tieske Apr 11, 2017

subnetmarco Apr 11, 2017

Tieske Apr 12, 2017

Tieske Apr 12, 2017

subnetmarco Apr 12, 2017

Tieske Apr 13, 2017

thibaultcha commented Apr 11, 2017

Tieske commented Apr 25, 2017

Tieske commented Apr 26, 2017

thibaultcha left a comment

thibaultcha Apr 26, 2017

thibaultcha Apr 26, 2017

Tieske Apr 27, 2017

thibaultcha Apr 26, 2017

thibaultcha Apr 26, 2017

thibaultcha commented Apr 28, 2017


		describe("OpenResty handlers", function()

		describe("rewrite on global plugin", function()

feat(core) exposing rewrite_by_lua handler to plugins #2354

feat(core) exposing rewrite_by_lua handler to plugins #2354

Conversation

subnetmarco commented Apr 6, 2017 • edited Loading

subnetmarco commented Apr 6, 2017

Tieske commented Apr 6, 2017

subnetmarco commented Apr 6, 2017

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

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

Choose a reason for hiding this comment

thibaultcha commented Apr 11, 2017

Tieske commented Apr 25, 2017

Tieske commented Apr 26, 2017

thibaultcha 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

thibaultcha commented Apr 28, 2017

subnetmarco commented Apr 6, 2017 •

edited

Loading