From 546d42e6458efde229d713d1fdd5903dd0116bc6 Mon Sep 17 00:00:00 2001 From: nic-chen Date: Fri, 22 Jan 2021 16:15:03 +0800 Subject: [PATCH 1/5] chore: global rules should not be executed on the internal api --- apisix/init.lua | 43 ++++++++++++++++++++++--------------------- t/node/global-rule.t | 29 ++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 22 deletions(-) diff --git a/apisix/init.lua b/apisix/init.lua index a33248f0d8ed..7568b9b80449 100644 --- a/apisix/init.lua +++ b/apisix/init.lua @@ -324,6 +324,28 @@ function _M.http_access_phase() core.ctx.set_vars_meta(api_ctx) + 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 + api_ctx.var.uri = str_sub(api_ctx.var.uri, 1, #uri - 1) + core.log.info("remove the end of uri '/', current uri: ", + api_ctx.var.uri) + end + end + + if router.api.has_route_not_under_apisix() or + core.string.has_prefix(uri, "/apisix/") + then + local matched = router.api.match(api_ctx) + if matched then + return + end + end + + router.router_http.match(api_ctx) + + local route = api_ctx.matched_route + -- load and run global rule if router.global_rules and router.global_rules.values and #router.global_rules.values > 0 then @@ -349,27 +371,6 @@ function _M.http_access_phase() 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 - api_ctx.var.uri = str_sub(api_ctx.var.uri, 1, #uri - 1) - core.log.info("remove the end of uri '/', current uri: ", - api_ctx.var.uri) - end - end - - if router.api.has_route_not_under_apisix() or - core.string.has_prefix(uri, "/apisix/") - then - local matched = router.api.match(api_ctx) - if matched then - return - end - end - - router.router_http.match(api_ctx) - - local route = api_ctx.matched_route if not route then core.log.info("not find any matched route") return core.response.exit(404, diff --git a/t/node/global-rule.t b/t/node/global-rule.t index d2bc9fac20f4..738e15e9a8ed 100644 --- a/t/node/global-rule.t +++ b/t/node/global-rule.t @@ -120,7 +120,34 @@ GET /hello -=== TEST 6: delete global rule +=== TEST 6: not limited +--- request +GET /hello +--- error_code: 200 +--- no_error_log +[error] + + + +=== TEST 7: not limited +--- request +GET /hello +--- error_code: 200 +--- no_error_log +[error] + + + +=== TEST 8: internal api should not be forbidden +--- request +GET /apisix/nginx_status +--- error_code: 200 +--- no_error_log +[error] + + + +=== TEST 9: delete global rule --- config location /t { content_by_lua_block { From e048fd6f202152722ce507f6baaf67376e6deb04 Mon Sep 17 00:00:00 2001 From: nic-chen Date: Fri, 22 Jan 2021 17:08:52 +0800 Subject: [PATCH 2/5] fix typo --- t/node/global-rule.t | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/node/global-rule.t b/t/node/global-rule.t index 738e15e9a8ed..df5cff96e531 100644 --- a/t/node/global-rule.t +++ b/t/node/global-rule.t @@ -122,7 +122,7 @@ GET /hello === TEST 6: not limited --- request -GET /hello +GET /apisix/nginx_status --- error_code: 200 --- no_error_log [error] @@ -131,7 +131,7 @@ GET /hello === TEST 7: not limited --- request -GET /hello +GET /apisix/nginx_status --- error_code: 200 --- no_error_log [error] From 5afdc5072419f69749dd7f561a21515564004aa6 Mon Sep 17 00:00:00 2001 From: nic-chen Date: Tue, 9 Feb 2021 17:38:36 +0800 Subject: [PATCH 3/5] feat: allow config global rule skip internal api or not --- apisix/init.lua | 13 ++++++++----- conf/config-default.yaml | 2 ++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/apisix/init.lua b/apisix/init.lua index 642ee118fa75..ba4d7f47725c 100644 --- a/apisix/init.lua +++ b/apisix/init.lua @@ -334,13 +334,11 @@ function _M.http_access_phase() end end + local matched_internal_api = false if router.api.has_route_not_under_apisix() or core.string.has_prefix(uri, "/apisix/") then - local matched = router.api.match(api_ctx) - if matched then - return - end + matched_internal_api = router.api.match(api_ctx) end router.router_http.match(api_ctx) @@ -348,7 +346,8 @@ function _M.http_access_phase() local route = api_ctx.matched_route -- load and run global rule - if router.global_rules and router.global_rules.values + if not (matched_internal_api and local_conf.apisix.global_rule_skip_internal_api) + and 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 @@ -372,6 +371,10 @@ function _M.http_access_phase() api_ctx.global_rules = router.global_rules end + if matched_internal_api then + return + end + if not route then core.log.info("not find any matched route") return core.response.exit(404, diff --git a/conf/config-default.yaml b/conf/config-default.yaml index 356d3fcaae8d..f3864965babc 100644 --- a/conf/config-default.yaml +++ b/conf/config-default.yaml @@ -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 + # 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) From 9f489e4b48af89575854e7f29211525d6f854d70 Mon Sep 17 00:00:00 2001 From: nic-chen Date: Wed, 10 Feb 2021 00:02:17 +0800 Subject: [PATCH 4/5] change: code refactoring of global rule --- apisix/api_router.lua | 13 +++-- apisix/init.lua | 118 ++++++------------------------------------ apisix/plugin.lua | 74 ++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 106 deletions(-) diff --git a/apisix/api_router.lua b/apisix/api_router.lua index f382258dc974..e68b261db449 100644 --- a/apisix/api_router.lua +++ b/apisix/api_router.lua @@ -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") @@ -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) @@ -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) @@ -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") @@ -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 diff --git a/apisix/init.lua b/apisix/init.lua index ba4d7f47725c..cf219c7b40a4 100644 --- a/apisix/init.lua +++ b/apisix/init.lua @@ -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 @@ -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 @@ -334,47 +291,23 @@ function _M.http_access_phase() end end - local matched_internal_api = false if router.api.has_route_not_under_apisix() or core.string.has_prefix(uri, "/apisix/") then - matched_internal_api = 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) - local route = api_ctx.matched_route - - -- load and run global rule - if not (matched_internal_api and local_conf.apisix.global_rule_skip_internal_api) - and 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 - - if matched_internal_api then - return - end + -- 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") return core.response.exit(404, @@ -496,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( @@ -513,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 @@ -561,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 @@ -851,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 @@ -876,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 diff --git a/apisix/plugin.lua b/apisix/plugin.lua index 9241a6bc3fa5..54bdf473cb00 100644 --- a/apisix/plugin.lua +++ b/apisix/plugin.lua @@ -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 From bba9da8268d6fa95531c3ef2bd6719ee14b15f6f Mon Sep 17 00:00:00 2001 From: nic-chen Date: Wed, 10 Feb 2021 11:23:48 +0800 Subject: [PATCH 5/5] test: add test cases --- t/node/global-rule.t | 41 ++++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/t/node/global-rule.t b/t/node/global-rule.t index df5cff96e531..ccf97ab24b78 100644 --- a/t/node/global-rule.t +++ b/t/node/global-rule.t @@ -120,34 +120,61 @@ GET /hello -=== TEST 6: not limited +=== TEST 6: skip global rule for internal api (not limited) +--- yaml_config +plugins: + - limit-count + - node-status --- request -GET /apisix/nginx_status +GET /apisix/status --- error_code: 200 --- no_error_log [error] -=== TEST 7: not limited +=== TEST 7: skip global rule for internal api - 2 (not limited) +--- yaml_config +plugins: + - limit-count + - node-status --- request -GET /apisix/nginx_status +GET /apisix/status --- error_code: 200 --- no_error_log [error] -=== TEST 8: internal api should not be forbidden +=== TEST 8: skip global rule for internal api - 3 (not limited) +--- yaml_config +plugins: + - limit-count + - node-status --- request -GET /apisix/nginx_status +GET /apisix/status --- error_code: 200 --- no_error_log [error] -=== TEST 9: delete global rule +=== 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 {