feat: allows users to specify plugin execution priority #7273

tzssangglass · 2022-06-17T05:08:36Z

Description

Fixes #6580
Fixes #7248

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

docs/en/latest/terminology/plugin.md

apisix/plugin.lua

t/plugin/custom_sort_plugins.t

apisix/plugin.lua

nfrankel · 2022-06-17T11:39:20Z

docs/en/latest/terminology/plugin.md

+```json
+ {
+    "serverless-post-function": {
+        "_meta": {


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.

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

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

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.

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:

The parameter name, i.e., priority

The parent's name, i.e., meta

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.

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

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

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

nfrankel · 2022-06-17T11:43:38Z

docs/en/latest/terminology/plugin.md

+}
+```
+
+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.


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?

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

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.

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:

The prefix v1 to be removed

The request to be forwarded to the upstream

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.

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.

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

in this way, we can match the requirements

apisix/schema_def.lua

apisix/plugin.lua

tzssangglass added 4 commits June 17, 2022 12:30

feat: allows users to specify plugin execution priority

b3e59c1

add test cases and docs

46ca8b2

fix CI

6592f6b

fix format

92b945d

tzssangglass requested review from spacewander, bzp2010 and membphis June 17, 2022 08:13

spacewander reviewed Jun 17, 2022

View reviewed changes

apisix/plugin.lua Show resolved Hide resolved

nfrankel requested changes Jun 17, 2022

View reviewed changes

tzssangglass added 2 commits June 20, 2022 01:44

fix core review

600a8db

fix code lint

945466b

spacewander reviewed Jun 20, 2022

View reviewed changes

apisix/schema_def.lua Outdated Show resolved Hide resolved

apisix/plugin.lua Outdated Show resolved Hide resolved

tzssangglass added 2 commits June 20, 2022 10:04

fix code reviews

7fb05cc

fix CI

f417d87

tzssangglass requested a review from spacewander June 21, 2022 14:50

spacewander approved these changes Jun 23, 2022

View reviewed changes

bzp2010 approved these changes Jun 23, 2022

View reviewed changes

spacewander merged commit cbc29a7 into apache:master Jun 23, 2022

spacewander pushed a commit that referenced this pull request Jun 30, 2022

feat: allows users to specify plugin execution priority (#7273)

4c49771

tzssangglass deleted the IssueNo6580 branch October 10, 2022 10:42

Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022

feat: allows users to specify plugin execution priority (apache#7273)

80ee093

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allows users to specify plugin execution priority #7273

feat: allows users to specify plugin execution priority #7273

tzssangglass commented Jun 17, 2022 •

edited by spacewander

Loading

nfrankel Jun 17, 2022

tzssangglass Jun 19, 2022

spacewander Jun 20, 2022

tzssangglass Jun 20, 2022

nfrankel Jun 20, 2022

tzssangglass Jun 20, 2022

spacewander Jun 21, 2022

spacewander Jun 21, 2022

nfrankel Jun 17, 2022

nfrankel Jun 17, 2022

tzssangglass Jun 19, 2022

nfrankel Jun 20, 2022

tzssangglass Jun 20, 2022 •

edited

Loading

tzssangglass Jun 20, 2022

feat: allows users to specify plugin execution priority #7273

feat: allows users to specify plugin execution priority #7273

Conversation

tzssangglass commented Jun 17, 2022 • edited by spacewander Loading

Description

Checklist

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

tzssangglass Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzssangglass commented Jun 17, 2022 •

edited by spacewander

Loading

tzssangglass Jun 20, 2022 •

edited

Loading