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

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 1 commit
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
43 changes: 22 additions & 21 deletions apisix/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
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)
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
Expand All @@ -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,
Expand Down
29 changes: 28 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,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
tokers marked this conversation as resolved.
Show resolved Hide resolved
--- 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 {
Expand Down