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.

Sign up for GitHub

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

Jump to bottom

change: global rules should not be executed on the internal api #3396

Merged
merged 6 commits into from
Feb 10, 2021
Merged

change: global rules should not be executed on the internal api #3396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
546d42e
chore: global rules should not be executed on the internal api
johzchen Jan 22, 2021
e048fd6
fix typo
johzchen Jan 22, 2021
2fee219
Merge branch 'master' into order-global-rule
johzchen Feb 9, 2021
5afdc50
feat: allow config global rule skip internal api or not
johzchen Feb 9, 2021
9f489e4
change: code refactoring of global rule
johzchen Feb 9, 2021
bba9da8
test: add test cases
johzchen Feb 10, 2021
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
13 changes: 10 additions & 3 deletions apisix/api_router.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
--
local require = require
local router = require("apisix.utils.router")
local apisix_router = require("apisix.router")
local plugin_mod = require("apisix.plugin")
local ip_restriction = require("apisix.plugins.ip-restriction")
local core = require("apisix.core")
Expand Down Expand Up @@ -103,7 +104,7 @@ function fetch_api_router()
core.table.insert(routes, {
methods = route.methods,
paths = route.uri,
handler = function (api_ctx)
handler = function (api_ctx, skip_global_rule)
local code, body

local metadata = plugin_mod.plugin_metadata(name)
Expand All @@ -121,6 +122,12 @@ function fetch_api_router()
end
end

if not skip_global_rule then
plugin_mod.run_global_rules(api_ctx,
apisix_router.global_rules, "access")
api_ctx.global_rules = apisix_router.global_rules
end

code, body = route.handler(api_ctx)
if code or body then
core.response.exit(code, body)
Expand All @@ -146,7 +153,7 @@ function _M.has_route_not_under_apisix()
end


function _M.match(api_ctx)
function _M.match(api_ctx, skip_global_rule)
local api_router = core.lrucache.global("api_router", plugin_mod.load_times, fetch_api_router)
if not api_router then
core.log.error("failed to fetch valid api router")
Expand All @@ -156,7 +163,7 @@ function _M.match(api_ctx)
core.table.clear(match_opts)
match_opts.method = api_ctx.var.request_method

local ok = api_router:dispatch(api_ctx.var.uri, match_opts, api_ctx)
local ok = api_router:dispatch(api_ctx.var.uri, match_opts, api_ctx, skip_global_rule)
return ok
end

Expand Down
108 changes: 12 additions & 96 deletions apisix/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
--
local require = require
local core = require("apisix.core")
local config_util = require("apisix.core.config_util")
local plugin = require("apisix.plugin")
local script = require("apisix.script")
local service_fetch = require("apisix.http.service").get
Expand Down Expand Up @@ -134,48 +133,6 @@ function _M.http_init_worker()
end


local function run_plugin(phase, plugins, api_ctx)
api_ctx = api_ctx or ngx.ctx.api_ctx
if not api_ctx then
return
end

plugins = plugins or api_ctx.plugins
if not plugins or #plugins == 0 then
return api_ctx
end

if phase ~= "log"
and phase ~= "header_filter"
and phase ~= "body_filter"
then
for i = 1, #plugins, 2 do
local phase_func = plugins[i][phase]
if phase_func then
local code, body = phase_func(plugins[i + 1], api_ctx)
if code or body then
if code >= 400 then
core.log.warn(plugins[i].name, " exits with http status code ", code)
end

core.response.exit(code, body)
end
end
end
return api_ctx
end

for i = 1, #plugins, 2 do
local phase_func = plugins[i][phase]
if phase_func then
phase_func(plugins[i + 1], api_ctx)
end
end

return api_ctx
end


function _M.http_ssl_phase()
local ngx_ctx = ngx.ctx
local api_ctx = ngx_ctx.api_ctx
Expand Down Expand Up @@ -325,31 +282,6 @@ function _M.http_access_phase()

core.ctx.set_vars_meta(api_ctx)

-- load and run global rule
if router.global_rules and router.global_rules.values
and #router.global_rules.values > 0 then
local plugins = core.tablepool.fetch("plugins", 32, 0)
local values = router.global_rules.values
for _, global_rule in config_util.iterate_values(values) do
api_ctx.conf_type = "global_rule"
api_ctx.conf_version = global_rule.modifiedIndex
api_ctx.conf_id = global_rule.value.id

core.table.clear(plugins)
api_ctx.plugins = plugin.filter(global_rule, plugins)
run_plugin("rewrite", plugins, api_ctx)
run_plugin("access", plugins, api_ctx)
end

core.tablepool.release("plugins", plugins)
api_ctx.plugins = nil
api_ctx.conf_type = nil
api_ctx.conf_version = nil
api_ctx.conf_id = nil

api_ctx.global_rules = router.global_rules
end

local uri = api_ctx.var.uri
if local_conf.apisix and local_conf.apisix.delete_uri_tail_slash then
if str_byte(uri, #uri) == str_byte("/") then
Expand All @@ -362,14 +294,19 @@ function _M.http_access_phase()
if router.api.has_route_not_under_apisix() or
Copy link
Member

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 configuration in the config.yaml for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@spacewander do you mean a switch for it ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

@spacewander
I thought about it again. There shouldn't have a switch here. If skip the if branch and continue execution, a 404 will occur because the route does not exist.

And I think if need to configure the plugin for the internal api, it is more appropriate to configure a route and then configure the plugin instead of using the global rule.

What do you think ? Looking forward to your opinion. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Better to provide a switch to execute the global rule before matching internal API or don't execute the global rule. It is not skipped the internal API matched.

And I think if need to configure the plugin for the internal api, it is more appropriate to configure a route and then configure the plugin instead of using the global rule.

We can't configure a plugin for internal API, this is the real problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

@spacewander Thanks for for the explanation. I know that.., emm, will update later.

Copy link
Member

Choose a reason for hiding this comment

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

OK. We can wait for it.

core.string.has_prefix(uri, "/apisix/")
then
local matched = router.api.match(api_ctx)
local skip = local_conf and local_conf.apisix.global_rule_skip_internal_api
local matched = router.api.match(api_ctx, skip)
if matched then
return
end
end

router.router_http.match(api_ctx)

-- run global rule
plugin.run_global_rules(api_ctx, router.global_rules, "access")
api_ctx.global_rules = router.global_rules

local route = api_ctx.matched_route
if not route then
core.log.info("not find any matched route")
Expand Down Expand Up @@ -492,7 +429,7 @@ function _M.http_access_phase()
local plugins = plugin.filter(route)
api_ctx.plugins = plugins

run_plugin("rewrite", plugins, api_ctx)
plugin.run_plugin("rewrite", plugins, api_ctx)
if api_ctx.consumer then
local changed
route, changed = plugin.merge_consumer_route(
Expand All @@ -509,7 +446,7 @@ function _M.http_access_phase()
api_ctx.plugins = plugin.filter(route, api_ctx.plugins)
end
end
run_plugin("access", plugins, api_ctx)
plugin.run_plugin("access", plugins, api_ctx)
end

if route.value.service_protocol == "grpc" then
Expand Down Expand Up @@ -557,33 +494,12 @@ local function common_phase(phase_name)
return
end

if api_ctx.global_rules then
local orig_conf_type = api_ctx.conf_type
local orig_conf_version = api_ctx.conf_version
local orig_conf_id = api_ctx.conf_id

local plugins = core.tablepool.fetch("plugins", 32, 0)
local values = api_ctx.global_rules.values
for _, global_rule in config_util.iterate_values(values) do
api_ctx.conf_type = "global_rule"
api_ctx.conf_version = global_rule.modifiedIndex
api_ctx.conf_id = global_rule.value.id

core.table.clear(plugins)
plugins = plugin.filter(global_rule, plugins)
run_plugin(phase_name, plugins, api_ctx)
end
core.tablepool.release("plugins", plugins)

api_ctx.conf_type = orig_conf_type
api_ctx.conf_version = orig_conf_version
api_ctx.conf_id = orig_conf_id
end
plugin.run_global_rules(api_ctx, api_ctx.global_rules, phase_name)

if api_ctx.script_obj then
script.run(phase_name, api_ctx)
else
run_plugin(phase_name, nil, api_ctx)
plugin.run_plugin(phase_name, nil, api_ctx)
end

return api_ctx
Expand Down Expand Up @@ -847,7 +763,7 @@ function _M.stream_preread_phase()
api_ctx.conf_version = matched_route.modifiedIndex
api_ctx.conf_id = matched_route.value.id

run_plugin("preread", plugins, api_ctx)
plugin.run_plugin("preread", plugins, api_ctx)

local code, err = set_upstream(matched_route, api_ctx)
if code then
Expand All @@ -872,7 +788,7 @@ end
function _M.stream_log_phase()
core.log.info("enter stream_log_phase")
-- core.ctx.release_vars(api_ctx)
run_plugin("log")
plugin.run_plugin("log")
end


Expand Down
74 changes: 74 additions & 0 deletions apisix/plugin.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,4 +640,78 @@ function _M.stream_plugin_checker(item)
end


function _M.run_plugin(phase, plugins, api_ctx)
api_ctx = api_ctx or ngx.ctx.api_ctx
if not api_ctx then
return
end

plugins = plugins or api_ctx.plugins
if not plugins or #plugins == 0 then
return api_ctx
end

if phase ~= "log"
and phase ~= "header_filter"
and phase ~= "body_filter"
then
for i = 1, #plugins, 2 do
local phase_func = plugins[i][phase]
if phase_func then
local code, body = phase_func(plugins[i + 1], api_ctx)
if code or body then
if code >= 400 then
core.log.warn(plugins[i].name, " exits with http status code ", code)
end

core.response.exit(code, body)
end
end
end
return api_ctx
end

for i = 1, #plugins, 2 do
local phase_func = plugins[i][phase]
if phase_func then
phase_func(plugins[i + 1], api_ctx)
end
end

return api_ctx
end


function _M.run_global_rules(api_ctx, global_rules, phase_name)
if global_rules and global_rules.values
and #global_rules.values > 0 then
local orig_conf_type = api_ctx.conf_type
local orig_conf_version = api_ctx.conf_version
local orig_conf_id = api_ctx.conf_id

local plugins = core.tablepool.fetch("plugins", 32, 0)
local values = global_rules.values
for _, global_rule in config_util.iterate_values(values) do
api_ctx.conf_type = "global_rule"
api_ctx.conf_version = global_rule.modifiedIndex
api_ctx.conf_id = global_rule.value.id

core.table.clear(plugins)
plugins = _M.filter(global_rule, plugins)
if phase_name == "access" then
_M.run_plugin("rewrite", plugins, api_ctx)
_M.run_plugin("access", plugins, api_ctx)
else
_M.run_plugin(phase_name, plugins, api_ctx)
end
end
core.tablepool.release("plugins", plugins)

api_ctx.conf_type = orig_conf_type
api_ctx.conf_version = orig_conf_version
api_ctx.conf_id = orig_conf_id
end
end


return _M
2 changes: 2 additions & 0 deletions conf/config-default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ apisix:
role: viewer

delete_uri_tail_slash: false # delete the '/' at the end of the URI
global_rule_skip_internal_api: true # does not run global rule in internal apis
Copy link
Member

Choose a reason for hiding this comment

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

Can we add test when global_rule_skip_internal_api is false?

Copy link
Member

Choose a reason for hiding this comment

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

We need to add test when global_rule_skip_internal_api is false.

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 need to add test when global_rule_skip_internal_api is false.

sure, test failed now, submit it after solving it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@spacewander teat case added. please help review when you have time, thanks.

# api that path starts with "/apisix" is considered to be internal api
router:
http: 'radixtree_uri' # radixtree_uri: match route by uri(base on radixtree)
# radixtree_host_uri: match route by host + uri(base on radixtree)
Expand Down
56 changes: 55 additions & 1 deletion t/node/global-rule.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,61 @@ GET /hello



=== TEST 6: delete global rule
=== TEST 6: skip global rule for internal api (not limited)
--- yaml_config
plugins:
- limit-count
- node-status
--- request
GET /apisix/status
--- error_code: 200
--- no_error_log
[error]



=== TEST 7: skip global rule for internal api - 2 (not limited)
--- yaml_config
plugins:
- limit-count
- node-status
--- request
GET /apisix/status
--- error_code: 200
--- no_error_log
[error]



=== TEST 8: skip global rule for internal api - 3 (not limited)
--- yaml_config
plugins:
- limit-count
- node-status
--- request
GET /apisix/status
--- error_code: 200
--- no_error_log
[error]



=== TEST 9: doesn't skip global rule for internal api (should limit)
--- yaml_config
apisix:
global_rule_skip_internal_api: false
plugins:
- limit-count
- node-status
--- request
GET /apisix/status
--- error_code: 503
--- no_error_log
[error]



=== TEST 10: delete global rule
--- config
location /t {
content_by_lua_block {
Expand Down