You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/03/03 10:47:07 UTC
[GitHub] [apisix] tzssangglass opened a new pull request #6502: fix: rerun rewrite phase for newly added plugins in consumer
tzssangglass opened a new pull request #6502:
URL: https://github.com/apache/apisix/pull/6502
Closes #6405
Signed-off-by: tzssangglass <tz...@gmail.com>
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
<!--
Please follow the PR manners:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. If you need to resolve merge conflicts after the PR is reviewed, please merge master but do not rebase
6. Use "request review" to notify the reviewer once you have resolved the review
7. Only reviewer can click "Resolve conversation" to mark the reviewer's review resolved
-->
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r819386150
##########
File path: apisix/init.lua
##########
@@ -456,6 +456,8 @@ function _M.http_access_phase()
api_ctx.matched_route = route
core.table.clear(api_ctx.plugins)
api_ctx.plugins = plugin.filter(api_ctx, route, api_ctx.plugins)
+ -- rerun rewrite phase for newly added plugins in consumer
+ plugin.rerun_plugins_of_consumer(api_ctx.plugins, api_ctx)
Review comment:
Currently, plugins in APISIX can only be run once, but after introducing this method, some plugins (in rewrite phase) can be run twice.
This inconsistence will confuse the users. I think either we only allow running each plugin for just one time or we need to redesign the plugin system so that plugins can be run multiple times.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r820249132
##########
File path: apisix/plugin.lua
##########
@@ -805,4 +810,32 @@ function _M.run_global_rules(api_ctx, global_rules, phase_name)
end
+function _M.rerun_plugins_of_consumer(plugins, api_ctx)
+ for i = 1, #plugins, 2 do
+ -- no need to rerun the auth plugins
+ if plugins[i + 1]["_from_consumer"] and plugins[i].type ~= "auth" then
Review comment:
I had thought about it, but `if plugins[i + 1]["_from_consumer"] and plugins[i].type ~= "auth" then` is difficult to port. Unless we add a flag or something like that. This makes run_plugin look even more complicated.
Another way is as follows:
```
for i = 1, #plugins, 2 do
-- no need to rerun the non-auth plugins
if plugins[i + 1]["_from_consumer"] and plugins[i].type ~= "auth" then
local new_plugins = core.table.new(2, 0)
core.table.insert(new_plugins, plugins[i])
core.table.insert(new_plugins, plugins[i + 1])
_M.run_plugin("rewrite", new_plugins, api_ctx)
end
end
return api_ctx
```
this adds some table overhead.
##########
File path: apisix/plugin.lua
##########
@@ -805,4 +810,32 @@ function _M.run_global_rules(api_ctx, global_rules, phase_name)
end
+function _M.rerun_plugins_of_consumer(plugins, api_ctx)
+ for i = 1, #plugins, 2 do
+ -- no need to rerun the auth plugins
+ if plugins[i + 1]["_from_consumer"] and plugins[i].type ~= "auth" then
Review comment:
I had thought about it, but `if plugins[i + 1]["_from_consumer"] and plugins[i].type ~= "auth" then` is difficult to port. Unless we add a flag or something like that. This makes run_plugin look even more complicated.
Another way is as follows:
```lua
for i = 1, #plugins, 2 do
-- no need to rerun the non-auth plugins
if plugins[i + 1]["_from_consumer"] and plugins[i].type ~= "auth" then
local new_plugins = core.table.new(2, 0)
core.table.insert(new_plugins, plugins[i])
core.table.insert(new_plugins, plugins[i + 1])
_M.run_plugin("rewrite", new_plugins, api_ctx)
end
end
return api_ctx
```
this adds some table overhead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r820248173
##########
File path: apisix/plugin.lua
##########
@@ -503,6 +503,11 @@ local function merge_consumer_route(route_conf, consumer_conf)
end
Review comment:
tried, Test 2 will fail. We need to merge the `new_route_conf.value.plugins["proxy-rewrite"], it will not be nil.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] shuaijinchao commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
shuaijinchao commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r820107618
##########
File path: apisix/plugin.lua
##########
@@ -365,17 +365,7 @@ function _M.filter(ctx, conf, plugins, route_conf, runned_plugins)
return plugins or core.tablepool.fetch("plugins", 0, 0)
end
- local unrunn_plugins
- local runned_plugins_names
- if runned_plugins and type(runned_plugins) == "table" then
- runned_plugins_names = core.table.new(#runned_plugins / 2, 0)
- for i = 1, #runned_plugins, 2 do
- core.table.insert(runned_plugins_names, runned_plugins[i].name)
- end
- unrunn_plugins = core.table.new(4, 0)
- end
-
- local route_plugin_conf = route_conf and route_conf.value and route_conf.value.plugins
+ local route_plugin_conf = route_conf and route_conf.value.plugins
Review comment:
```suggestion
local route_plugin_conf = route_conf and route_conf.value and route_conf.value.plugins
```
Keeping the old way should be more rigorous.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r820223821
##########
File path: apisix/init.lua
##########
@@ -456,6 +456,8 @@ function _M.http_access_phase()
api_ctx.matched_route = route
core.table.clear(api_ctx.plugins)
api_ctx.plugins = plugin.filter(api_ctx, route, api_ctx.plugins)
+ -- rerun rewrite phase for newly added plugins in consumer
+ plugin.rerun_plugins_of_consumer(api_ctx.plugins, api_ctx)
Review comment:
Thanks for the explanation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r820397805
##########
File path: apisix/plugin.lua
##########
@@ -805,4 +810,32 @@ function _M.run_global_rules(api_ctx, global_rules, phase_name)
end
+function _M.rerun_plugins_of_consumer(plugins, api_ctx)
+ for i = 1, #plugins, 2 do
+ -- no need to rerun the auth plugins
+ if plugins[i + 1]["_from_consumer"] and plugins[i].type ~= "auth" then
Review comment:
Maybe we can add another fake phase (rewrite_in_consumer) in `run_plugin` so we don't need to duplicate the conditions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r820174811
##########
File path: apisix/plugin.lua
##########
@@ -365,17 +365,7 @@ function _M.filter(ctx, conf, plugins, route_conf, runned_plugins)
return plugins or core.tablepool.fetch("plugins", 0, 0)
end
- local unrunn_plugins
- local runned_plugins_names
- if runned_plugins and type(runned_plugins) == "table" then
- runned_plugins_names = core.table.new(#runned_plugins / 2, 0)
- for i = 1, #runned_plugins, 2 do
- core.table.insert(runned_plugins_names, runned_plugins[i].name)
- end
- unrunn_plugins = core.table.new(4, 0)
- end
-
- local route_plugin_conf = route_conf and route_conf.value and route_conf.value.plugins
+ local route_plugin_conf = route_conf and route_conf.value.plugins
Review comment:
hi @shuaijinchao what you reviewed is already the previous version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #6502:
URL: https://github.com/apache/apisix/pull/6502
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r820472828
##########
File path: apisix/plugin.lua
##########
@@ -805,4 +810,32 @@ function _M.run_global_rules(api_ctx, global_rules, phase_name)
end
+function _M.rerun_plugins_of_consumer(plugins, api_ctx)
+ for i = 1, #plugins, 2 do
+ -- no need to rerun the auth plugins
+ if plugins[i + 1]["_from_consumer"] and plugins[i].type ~= "auth" then
Review comment:
update
##########
File path: apisix/plugin.lua
##########
@@ -503,6 +503,11 @@ local function merge_consumer_route(route_conf, consumer_conf)
end
Review comment:
yes, update
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r820249448
##########
File path: apisix/plugin.lua
##########
@@ -805,4 +810,32 @@ function _M.run_global_rules(api_ctx, global_rules, phase_name)
end
+function _M.rerun_plugins_of_consumer(plugins, api_ctx)
+ for i = 1, #plugins, 2 do
+ -- no need to rerun the auth plugins
+ if plugins[i + 1]["_from_consumer"] and plugins[i].type ~= "auth" then
Review comment:
The main reason is that the `plugins` here are used in the `api_ctx` in order to reduce the table overhead, so use the judgment condition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r819432139
##########
File path: apisix/init.lua
##########
@@ -456,6 +456,8 @@ function _M.http_access_phase()
api_ctx.matched_route = route
core.table.clear(api_ctx.plugins)
api_ctx.plugins = plugin.filter(api_ctx, route, api_ctx.plugins)
+ -- rerun rewrite phase for newly added plugins in consumer
+ plugin.rerun_plugins_of_consumer(api_ctx.plugins, api_ctx)
Review comment:
In fact, no plug-in will run twice in this case. The reason for this misunderstanding is that
1. APISIX runs the rewrite phase of plugins on route first;
2. APISIX handles the logic for consumer authentication based on the auth plugins on the route;
3. then APISIX merges the plugins on consumer and route;
4. this causes the rewrite phase to be missed for some non-auth plugins configured on the consumer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r819432139
##########
File path: apisix/init.lua
##########
@@ -456,6 +456,8 @@ function _M.http_access_phase()
api_ctx.matched_route = route
core.table.clear(api_ctx.plugins)
api_ctx.plugins = plugin.filter(api_ctx, route, api_ctx.plugins)
+ -- rerun rewrite phase for newly added plugins in consumer
+ plugin.rerun_plugins_of_consumer(api_ctx.plugins, api_ctx)
Review comment:
In fact, no plugin will run twice in this case. The reason for this misunderstanding is that
1. APISIX runs the rewrite phase of plugins on route first;
2. APISIX handles the logic for consumer authentication based on the auth plugins on the route;
3. then APISIX merges the plugins on consumer and route;
4. this causes the rewrite phase to be missed for some non-auth plugins configured on the consumer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r819374235
##########
File path: apisix/init.lua
##########
@@ -454,8 +454,15 @@ function _M.http_access_phase()
if changed then
api_ctx.matched_route = route
+ local runned_plugins = core.table.deepcopy(api_ctx.plugins)
core.table.clear(api_ctx.plugins)
- api_ctx.plugins = plugin.filter(api_ctx, route, api_ctx.plugins)
+ local unrunn_plugins
Review comment:
update
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r820222375
##########
File path: apisix/plugin.lua
##########
@@ -503,6 +503,11 @@ local function merge_consumer_route(route_conf, consumer_conf)
end
Review comment:
we can use `if new_route_conf.value.plugins[name] == nil then conf._from_consumer = true`?
##########
File path: apisix/plugin.lua
##########
@@ -805,4 +810,32 @@ function _M.run_global_rules(api_ctx, global_rules, phase_name)
end
+function _M.rerun_plugins_of_consumer(plugins, api_ctx)
+ for i = 1, #plugins, 2 do
+ -- no need to rerun the auth plugins
+ if plugins[i + 1]["_from_consumer"] and plugins[i].type ~= "auth" then
Review comment:
Would be better to merge it in https://github.com/apache/apisix/blob/492f782d9fae719b3a20e0d65c275962bcdcaab9/apisix/plugin.lua#L736?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r820397697
##########
File path: apisix/plugin.lua
##########
@@ -503,6 +503,11 @@ local function merge_consumer_route(route_conf, consumer_conf)
end
Review comment:
What about this change?
```
diff --git apisix/plugin.lua apisix/plugin.lua
index d66ec045..190b0833 100644
--- apisix/plugin.lua
+++ apisix/plugin.lua
@@ -502,12 +502,11 @@ local function merge_consumer_route(route_conf, consumer_conf)
new_route_conf.value.plugins = {}
end
- new_route_conf.value.plugins[name] = conf
-
- if (route_conf and route_conf.value and route_conf.value.plugins)
- and not route_conf.value.plugins[name] then
- new_route_conf.value.plugins[name]["_from_consumer"] = true
+ if new_route_conf.value.plugins[name] == nil then
+ conf._from_consumer = true
end
+
+ new_route_conf.value.plugins[name] = conf
end
core.log.info("merged conf : ", core.json.delay_encode(new_route_conf))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6502: feat: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#discussion_r819327079
##########
File path: apisix/init.lua
##########
@@ -454,8 +454,15 @@ function _M.http_access_phase()
if changed then
api_ctx.matched_route = route
+ local runned_plugins = core.table.deepcopy(api_ctx.plugins)
core.table.clear(api_ctx.plugins)
- api_ctx.plugins = plugin.filter(api_ctx, route, api_ctx.plugins)
+ local unrunn_plugins
Review comment:
Maybe we can add a flag in https://github.com/apache/apisix/blob/c4229d15efe8cac6677f8124cf92d3512d1b0180/apisix/plugin.lua#L505 so that we can save allocation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #6502: fix: rerun rewrite phase for newly added plugins in consumer
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on pull request #6502:
URL: https://github.com/apache/apisix/pull/6502#issuecomment-1058044585
only call core.consumer.attach_consumer is considered to be an auth class plugin.
ref: https://github.com/apache/apisix/pull/2898
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org