-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: rerun rewrite phase for newly added plugins in consumer #6502
Conversation
Closes apache#6405 Signed-off-by: tzssangglass <tzssangglass@gmail.com>
…be an auth class plugin Signed-off-by: tzssangglass <tzssangglass@gmail.com>
only call core.consumer.attach_consumer is considered to be an auth class plugin. ref: #2898 |
apisix/init.lua
Outdated
core.table.clear(api_ctx.plugins) | ||
api_ctx.plugins = plugin.filter(api_ctx, route, api_ctx.plugins) | ||
local unrunn_plugins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add a flag in
Line 505 in c4229d1
new_route_conf.value.plugins[name] = conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update
Signed-off-by: tzssangglass <tzssangglass@gmail.com>
@@ -503,6 +503,11 @@ local function merge_consumer_route(route_conf, consumer_conf) | |||
end | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use if new_route_conf.value.plugins[name] == nil then conf._from_consumer = true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried, Test 2 will fail. We need to merge the `new_route_conf.value.plugins["proxy-rewrite"], it will not be nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, update
apisix/plugin.lua
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to merge it in
Line 736 in 492f782
if phase_func then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add another fake phase (rewrite_in_consumer) in run_plugin
so we don't need to duplicate the conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update
Signed-off-by: tzssangglass <tzssangglass@gmail.com>
Closes #6405
Signed-off-by: tzssangglass tzssangglass@gmail.com
What this PR does / why we need it:
Pre-submission checklist: