Skip to content
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.

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

feat: allows users to specify plugin execution priority #7273

Merged
merged 8 commits into from
Jun 23, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
5 changes: 3 additions & 2 deletions apisix/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -450,9 +450,10 @@ function _M.http_access_phase()
if changed then
api_ctx.matched_route = route
core.table.clear(api_ctx.plugins)
api_ctx.plugins = plugin.filter(api_ctx, route, api_ctx.plugins)
local phase = "rewrite_in_consumer"
api_ctx.plugins = plugin.filter(api_ctx, route, api_ctx.plugins, nil, phase)
-- rerun rewrite phase for newly added plugins in consumer
plugin.run_plugin("rewrite_in_consumer", api_ctx.plugins, api_ctx)
plugin.run_plugin(phase, api_ctx.plugins, api_ctx)
end
end
plugin.run_plugin("access", plugins, api_ctx)
Expand Down
60 changes: 59 additions & 1 deletion apisix/plugin.lua
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ local function sort_plugin(l, r)
return l.priority > r.priority
end

local function custom_sort_plugin(l, r)
return l._meta.priority > r._meta.priority
end


local PLUGIN_TYPE_HTTP = 1
local PLUGIN_TYPE_STREAM = 2
Expand Down Expand Up @@ -365,7 +369,7 @@ local function trace_plugins_info_for_debug(ctx, plugins)
end


function _M.filter(ctx, conf, plugins, route_conf)
function _M.filter(ctx, conf, plugins, route_conf, phase)
local user_plugin_conf = conf.value.plugins
if user_plugin_conf == nil or
core.table.nkeys(user_plugin_conf) == 0 then
Expand All @@ -375,6 +379,7 @@ function _M.filter(ctx, conf, plugins, route_conf)
return plugins or core.tablepool.fetch("plugins", 0, 0)
end

local custom_sort = false
local route_plugin_conf = route_conf and route_conf.value.plugins
plugins = plugins or core.tablepool.fetch("plugins", 32, 0)
for _, plugin_obj in ipairs(local_plugins) do
Expand All @@ -389,6 +394,9 @@ function _M.filter(ctx, conf, plugins, route_conf)
end
end

if plugin_conf._meta and plugin_conf._meta.priority then
custom_sort = true
end
core.table.insert(plugins, plugin_obj)
core.table.insert(plugins, plugin_conf)

Expand All @@ -398,6 +406,49 @@ function _M.filter(ctx, conf, plugins, route_conf)

trace_plugins_info_for_debug(ctx, plugins)


if custom_sort then
local tmp_plugin_objs = core.tablepool.fetch("tmp_plugin_objs", 0, #plugins / 2)
local tmp_plugin_confs = core.tablepool.fetch("tmp_plugin_confs", #plugins / 2, 0)

for i = 1, #plugins, 2 do
local plugin_obj = plugins[i]
local plugin_conf = plugins[i + 1]

-- in the rewrite phase, the plugin executes in the following order:
-- 1. execute the rewrite phase of the plugins on route(including the auth plugins)
-- 2. merge plugins from consumer and route
-- 3. execute the rewrite phase of the plugins on consumer(phase: rewrite_in_consumer)
-- in this case, we need to skip the plugins that was already executed(step 1)
if phase and phase == "rewrite_in_consumer" and not plugin_conf._from_consumer then
tzssangglass marked this conversation as resolved.
Show resolved Hide resolved
plugin_conf._skip_rewrite_in_consumer = true
tzssangglass marked this conversation as resolved.
Show resolved Hide resolved
end

tmp_plugin_objs[tostring(plugin_conf)] = plugin_obj
tzssangglass marked this conversation as resolved.
Show resolved Hide resolved
core.table.insert(tmp_plugin_confs, plugin_conf)

if not plugin_conf._meta or not plugin_conf._meta.priority then
plugin_conf._meta = core.table.new(0, 1)
tzssangglass marked this conversation as resolved.
Show resolved Hide resolved
plugin_conf._meta.priority = plugin_obj.priority
end

end

sort_tab(tmp_plugin_confs, custom_sort_plugin)

core.table.clear(plugins)
tzssangglass marked this conversation as resolved.
Show resolved Hide resolved

for i = 1, #tmp_plugin_confs do
local plugin_conf = tmp_plugin_confs[i]
local plugin_obj = tmp_plugin_objs[tostring(plugin_conf)]
tzssangglass marked this conversation as resolved.
Show resolved Hide resolved
core.table.insert(plugins, plugin_obj)
core.table.insert(plugins, plugin_conf)
end

core.tablepool.release("tmp_plugin_objs", tmp_plugin_objs)
core.tablepool.release("tmp_plugin_confs", tmp_plugin_confs)
end

return plugins
end

Expand Down Expand Up @@ -754,6 +805,11 @@ function _M.run_plugin(phase, plugins, api_ctx)
phase = "rewrite"
end
local phase_func = plugins[i][phase]

if phase == "rewrite" and plugins[i + 1]._skip_rewrite_in_consumer then
goto CONTINUE
end

if phase_func then
plugin_run = true
local conf = plugins[i + 1]
Expand Down Expand Up @@ -781,6 +837,8 @@ function _M.run_plugin(phase, plugins, api_ctx)
end
end
end

::CONTINUE::
end
return api_ctx, plugin_run
end
Expand Down
5 changes: 5 additions & 0 deletions apisix/schema_def.lua
Original file line number Diff line number Diff line change
Expand Up @@ -952,6 +952,11 @@ _M.plugin_injected_schema = {
{ type = "object" },
}
},
priority = {
description = "priority of plugins by customized order",
type = "integer",
default = 0,
tzssangglass marked this conversation as resolved.
Show resolved Hide resolved
},
}
}
}
Expand Down
37 changes: 37 additions & 0 deletions docs/en/latest/terminology/plugin.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,43 @@ the configuration above means customizing the error response from the jwt-auth p
| Name | Type | Description |
|--------------|------|-------------|
| error_response | string/object | Custom error response |
| priority | integer | Custom plugin priority |

### Custom Plugin Priority

All plugins have a default priority, but it is possible to customize the plugin priority to change the plugin's execution order.

```json
{
"serverless-post-function": {
"_meta": {
Copy link
Contributor

Choose a reason for hiding this comment

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

As I already mentioned, this is poor Developer Experience. It makes the use of the configuration more complex for no reason except that it's easier to implement.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also think priority should not be in _meta, it doesn't fit the definition of _meta, it only works for plugin bound objects. How about a custom priority directly as a base property of the plugin? ref: #6580 (comment)

cc @spacewander @membphis

Copy link
Member

Choose a reason for hiding this comment

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

The plugin property will affect every plugin configuration, not just what we configure for.

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussing with @spacewander , I changed my mind.

_meta is based on the concept of abstraction of all plugin configurations, which are available to all plugins. So to distinguish it, it should have a separate configuration field, i.e. _meta.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, but again, you speak from an engineering perspective. I'm talking from a user perspective. As a user, it's not fundamental whether the configuration parameter can be used across multiple plugins.

Here, I need to know and remember:

  1. The parameter name, i.e., priority
  2. The parent's name, i.e., meta
  3. The prefix, i.e., _

Only #1 is mandatory. You make #2 and #3 necessary and impair the ease of use for engineering reasons. While I understand there's a tension between the ease of implementation and ease of use, I don't see any good reason to force those additional requirements onto users at this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is most directly to make priority one of the properties of the plugin. just like disable, what your option? @spacewander

Copy link
Member

Choose a reason for hiding this comment

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

No. Setting the new field directly on the configuration may override people's current configuration. We should avoid break change

Copy link
Member

Choose a reason for hiding this comment

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

I have submitted a PR to explain it in the comment: #7290

"priority": 10000
},
"phase": "rewrite",
"functions" : ["return function(conf, ctx)
ngx.say(\"serverless-post-function\");
end"]
},
"serverless-pre-function": {
"_meta": {
"priority": -2000
},
"phase": "rewrite",
"functions": ["return function(conf, ctx)
ngx.say(\"serverless-pre-function\");
end"]
}
}
```

The default priority of serverless-pre-function is 10000, and the default priority of serverless-post-function is -2000. By default, the serverless-pre-function plugin will be executed first, and serverless-post-function plugin will be executed next.
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point, why do we keep two different functions? We can keep a single one as call it whenever we required it. Can we change the sample?

Copy link
Contributor

Choose a reason for hiding this comment

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

A good use-case would be the combination of proxy-mirror plugin and the proxy-rewrite one.

Copy link
Member Author

Choose a reason for hiding this comment

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

A good use-case would be the combination of proxy-mirror plugin and the proxy-rewrite one.

I tried it and the two plugins don't show well to work together by customizing the priority.

Maybe you're talking about changing the request path via the proxy-rewrite plugin, and then proxy-mirror using that new path.

But proxy-rewrite plugin update uri on the var ctx.var.upstream_uri, but proxy-mirror get the uri by the var ctx.var.uri, this is the difference in implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried it and the two plugins don't show well to work together by customizing the priority.

That's actually the origin of my initial requirement.

Maybe you're talking about changing the request path via the proxy-rewrite plugin, and then proxy-mirror using that new path.

Indeed!

Imagine that I receive a request for v1/hello. Now, my requirements are:

  1. The prefix v1 to be removed
  2. The request to be forwarded to the upstream
  3. The request to be duplicated to the mirror

But proxy-rewrite plugin update uri on the var ctx.var.upstream_uri, but proxy-mirror get the uri by the var ctx.var.uri, this is the difference in implementation.

Again, how are users supposed to know this? It's an implementation detail.

The requirements make sense from a functional point of view.

Copy link
Member Author

@tzssangglass tzssangglass Jun 20, 2022

Choose a reason for hiding this comment

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

Since proxy-mirror support rewrite path, so we can implement the above requirements, but we just need to configure them separately on proxy-mirror and proxy-rewrite.

Of course, we can also modify the proxy-mirror plugin to read the ctx.var.upstream_uri variable. But it won't be done in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

we can also modify the proxy-mirror plugin to read the ctx.var.upstream_uri variable

in this way, we can match the requirements


The above configuration means setting the priority of the serverless-pre-function plugin to -2000 and the priority of the serverless-post-function plugin to 10000. The serverless-post-function plugin will be executed first, and serverless-pre-function plugin will be executed next.

Note:

- Custom plugin priority only affects the current object(route, service ...) of the plugin instance binding, not all instances of that plugin. For example, if the above plugin configuration belongs to Route A, the order of execution of the plugins serverless-post-function and serverless-post-function on Route B will not be affected and the default priority will be used.
- Custom plugin priority does not apply to the rewrite phase of some plugins configured on the consumer. The rewrite phase of plugins configured on the route will executed first, and then the rewrite phase of plugins(exclude auth plugins) on the consumer will be executed.
tzssangglass marked this conversation as resolved.
Show resolved Hide resolved

## Hot Reload

Expand Down
37 changes: 37 additions & 0 deletions docs/zh/latest/terminology/plugin.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,43 @@ local _M = {
| 名称 | 类型 | 描述 |
|--------------|------|----------------|
| error_response | string/object | 自定义错误响应 |
| priority | integer | 自定义插件优先级 |

### 自定义插件优先级

所有插件都有默认优先级,但是可以自定义插件优先级来改变插件执行顺序。

```json
{
"serverless-post-function": {
"_meta": {
"priority": 10000
},
"phase": "rewrite",
"functions" : ["return function(conf, ctx)
ngx.say(\"serverless-post-function\");
end"]
},
"serverless-pre-function": {
"_meta": {
"priority": -2000
},
"phase": "rewrite",
"functions": ["return function(conf, ctx)
ngx.say(\"serverless-pre-function\");
end"]
}
}
```

serverless-pre-function 的默认优先级是 10000,serverless-post-function 的默认优先级是 -2000。默认情况下会先执行 serverless-pre-function 插件,再执行 serverless-post-function 插件。

上面的配置意味着将 serverless-pre-function 插件的优先级设置为 -2000,serverless-post-function 插件的优先级设置为 10000。serverless-post-function 插件会先执行,再执行 serverless-pre-function 插件。

注意:

- 自定义插件优先级只会影响插件实例绑定的主体,不会影响该插件的所有实例。比如上面的插件配置属于路由 A ,路由 B 上的插件 serverless-post-function 和 serverless-post-function 插件执行顺序不会受到影响,会使用默认优先级。
- 自定义插件优先级不适用于 consumer 上配置的插件的 rewrite 阶段。路由上配置的插件的 rewrite 阶段将会优先运行,然后才会运行 consumer 上除 auth 插件之外的其他插件的 rewrite 阶段。

## 热加载

Expand Down
4 changes: 2 additions & 2 deletions t/admin/plugins.t
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ plugins:
}
}
--- response_body eval
qr/\{"metadata_schema":\{"properties":\{"ikey":\{"minimum":0,"type":"number"\},"skey":\{"type":"string"\}\},"required":\["ikey","skey"\],"type":"object"\},"priority":0,"schema":\{"\$comment":"this is a mark for our injected plugin schema","properties":\{"_meta":\{"properties":\{"error_response":\{"oneOf":\[\{"type":"string"\},\{"type":"object"\}\]\}\},"type":"object"\},"disable":\{"type":"boolean"\},"i":\{"minimum":0,"type":"number"\},"ip":\{"type":"string"\},"port":\{"type":"integer"\},"s":\{"type":"string"\},"t":\{"minItems":1,"type":"array"\}\},"required":\["i"\],"type":"object"\},"version":0.1\}/
qr/\{"metadata_schema":\{"properties":\{"ikey":\{"minimum":0,"type":"number"\},"skey":\{"type":"string"\}\},"required":\["ikey","skey"\],"type":"object"\},"priority":0,"schema":\{"\$comment":"this is a mark for our injected plugin schema","properties":\{"_meta":\{"properties":\{"error_response":\{"oneOf":\[\{"type":"string"\},\{"type":"object"\}\]\},"priority":\{"default":0,"description":"priority of plugins by customized order","type":"integer"\}\},"type":"object"\},"disable":\{"type":"boolean"\},"i":\{"minimum":0,"type":"number"\},"ip":\{"type":"string"\},"port":\{"type":"integer"\},"s":\{"type":"string"\},"t":\{"minItems":1,"type":"array"\}\},"required":\["i"\],"type":"object"\},"version":0.1\}/



Expand Down Expand Up @@ -366,7 +366,7 @@ qr/\{"properties":\{"password":\{"type":"string"\},"username":\{"type":"string"\
}
}
--- response_body
{"priority":1003,"schema":{"$comment":"this is a mark for our injected plugin schema","properties":{"_meta":{"properties":{"error_response":{"oneOf":[{"type":"string"},{"type":"object"}]}},"type":"object"},"burst":{"minimum":0,"type":"integer"},"conn":{"exclusiveMinimum":0,"type":"integer"},"default_conn_delay":{"exclusiveMinimum":0,"type":"number"},"disable":{"type":"boolean"},"key":{"type":"string"},"key_type":{"default":"var","enum":["var","var_combination"],"type":"string"},"only_use_default_delay":{"default":false,"type":"boolean"}},"required":["conn","burst","default_conn_delay","key"],"type":"object"},"version":0.1}
{"priority":1003,"schema":{"$comment":"this is a mark for our injected plugin schema","properties":{"_meta":{"properties":{"error_response":{"oneOf":[{"type":"string"},{"type":"object"}]},"priority":{"default":0,"description":"priority of plugins by customized order","type":"integer"}},"type":"object"},"burst":{"minimum":0,"type":"integer"},"conn":{"exclusiveMinimum":0,"type":"integer"},"default_conn_delay":{"exclusiveMinimum":0,"type":"number"},"disable":{"type":"boolean"},"key":{"type":"string"},"key_type":{"default":"var","enum":["var","var_combination"],"type":"string"},"only_use_default_delay":{"default":false,"type":"boolean"}},"required":["conn","burst","default_conn_delay","key"],"type":"object"},"version":0.1}



Expand Down
Loading