feat: add public api plugin by bzp2010 · Pull Request #6145 · apache/apisix

bzp2010 · 2022-01-19T04:16:41Z

What this PR does / why we need it:

Implement: #6137

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

spacewander · 2022-01-21T02:06:42Z

+    local local_conf = core.config.local_conf()
+
+    -- overwrite the uri in the ctx when the user has set the target uri
+    ctx.var.uri = conf.uri and conf.uri or ctx.var.uri


Suggested change

ctx.var.uri = conf.uri and conf.uri or ctx.var.uri

ctx.var.uri = conf.uri or ctx.var.uri

spacewander · 2022-01-21T02:07:26Z

+
+    -- overwrite the uri in the ctx when the user has set the target uri
+    ctx.var.uri = conf.uri and conf.uri or ctx.var.uri
+    local skip = local_conf and local_conf.apisix.global_rule_skip_internal_api


We don't need to run global rules twice?

apisix/apisix/init.lua

Lines 376 to 445 in 64c47df

router.router_http.match(api_ctx)

local route = api_ctx.matched_route

if not route then

-- whether the public API run global rules is

-- controlled by the configuration file

if router.api.has_route_not_under_apisix() or

core.string.has_prefix(uri, "/apisix/")

then

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

-- run global rule when there is no matching route

plugin.run_global_rules(api_ctx, router.global_rules, nil)

core.log.info("not find any matched route")

return core.response.exit(404,

{error_msg = "404 Route Not Found"})

end

core.log.info("matched route: ",

core.json.delay_encode(api_ctx.matched_route, true))

local enable_websocket = route.value.enable_websocket

if route.value.plugin_config_id then

local conf = plugin_config.get(route.value.plugin_config_id)

if not conf then

core.log.error("failed to fetch plugin config by ",

"id: ", route.value.plugin_config_id)

return core.response.exit(503)

end

route = plugin_config.merge(route, conf)

end

if route.value.service_id then

local service = service_fetch(route.value.service_id)

if not service then

core.log.error("failed to fetch service configuration by ",

"id: ", route.value.service_id)

return core.response.exit(404)

end

route = plugin.merge_service_route(service, route)

api_ctx.matched_route = route

api_ctx.conf_type = "route&service"

api_ctx.conf_version = route.modifiedIndex .. "&" .. service.modifiedIndex

api_ctx.conf_id = route.value.id .. "&" .. service.value.id

api_ctx.service_id = service.value.id

api_ctx.service_name = service.value.name

if enable_websocket == nil then

enable_websocket = service.value.enable_websocket

end

else

api_ctx.conf_type = "route"

api_ctx.conf_version = route.modifiedIndex

api_ctx.conf_id = route.value.id

end

api_ctx.route_id = route.value.id

api_ctx.route_name = route.value.name

-- run global rule

plugin.run_global_rules(api_ctx, router.global_rules, nil)

Hi, @spacewander.

Based on this part of the code, we can presume to have such a situation.

HTTP match, and public-api plugin is enabled.
This will be controlled by the configuration file whether to enforce global rules for public API calls. After the match, the subsequent request processing flow will be completely taken over by the public API handler, bypassing the rest of the code execution in the access phase.

HTTP not matched, public API route matched.
This will also be controlled by the configuration file whether to enforce global rules or not.

Both HTTP and public APIs do not match
It will force the global rule to run and return 404 Not Route.

Also, as you said in this issue #6137 (comment), we will adjust the public API to not be exposed by default, which will be done by completely removing the public API route match in the access phase. (Currently HTTP matching is performed followed by public API matching as a backup) In that case, the matching and processing of the public API will be done by the public-api plugin only, and if a match is made the global rules will be enforced according to the configuration.

In that case, the matching and processing of the public API will be done by the public-api plugin only, and if a match is made the global rules will be enforced according to the configuration.

We can adjust it when we retire the router? The current implementation will run global rules twice in some cases.

I will try to fix it.

Update

After checking again, I think there may not be a duplicate calls problem.

Details

There are 4 calls to run_global_rules executed in the current code. One of them is in api_router, which is controlled by the configuration file (this is the router used by the public API), and the remaining ones are executed in the http_access_phase of init.

both HTTP and public API do not match, will run and return a 404 status code, stop progress

run after HTTP matched

run in common_phase for before_proxy

In fact I didn't add global_rules to init.lua, they are still being executed according to the original logic.

If I have missed anything, please help point it out. Thanks.

ping @spacewander

spacewander · 2022-01-21T02:10:09Z

+
+=== TEST 3: hit route (custom-jwt-sign)
+--- request
+GET /gen_token?key=user-key


Let's add assertion to make sure the jwt sign is working

added, It will check the first part of the JWT in the response body, however, because json encode cannot guarantee the order of the data in it, it matches both cases.

spacewander · 2022-01-21T02:12:05Z

+GET /apisix/plugin/wolf-rbac/user_info
+--- error_code: 401
+--- error_log
+custom-jwt-sign was triggered


Let's add more cases:

public api not match

match without optional uri

add auth to protect public api

added

I didn't understand what you meant, is it a case where there is no uri configuration in the schema? If so it is there now.

added, protect jwt sign by key-auth

spacewander · 2022-01-21T07:54:05Z

+=== TEST 5: missing route
+--- request
+GET /apisix/plugin/balalbala
+--- error_code: 404


Let's add more test that the uri in public API plugin matches nothing.

got it, adding

shuaijinchao · 2022-01-21T10:57:10Z

I don't see the problem in the code and test cases, but I don't see the documentation for the plugin.

bzp2010 · 2022-01-21T14:47:04Z

I don't see the problem in the code and test cases, but I don't see the documentation for the plugin.

@shuaijinchao

According this comment #6145 (comment), there is a next phase of development work after this PR is completed, which will be added when everything is done.

spacewander

Approve except #6145 (comment)

bzp2010 added enhancement New feature or request plugin labels Jan 19, 2022

bzp2010 self-assigned this Jan 19, 2022

bzp2010 added 7 commits January 20, 2022 09:34

feat: add public api plugin

d2055e8

feat: add public api on http route not match

144126a

fix: logic error

7ce277d

chore: clean code

e9f3593

test: add case

2bca884

test: fix lint

261cd1a

chore: fix code style

eab8158

bzp2010 force-pushed the feat-public-api branch from 9952d03 to eab8158 Compare January 20, 2022 01:34

bzp2010 marked this pull request as ready for review January 20, 2022 01:42

bzp2010 requested review from shuaijinchao, spacewander and tzssangglass January 20, 2022 01:49

bzp2010 added 3 commits January 20, 2022 11:22

chore: add plugin list

ddcc677

fix: update global rules running condition

891b43b

test: add public api route create

64c47df

spacewander reviewed Jan 21, 2022

View reviewed changes

chore: simplify codes

31fb668

bzp2010 linked an issue Jan 21, 2022 that may be closed by this pull request

Proposal: support more access control for plugin's public API #6137

Closed

bzp2010 added 5 commits January 21, 2022 11:00

test: jwt sign result check

f8f923c

test: fix typo

0647502

test: add missing route case

80a7bde

test: refactor jwt sign result check

9020e55

test: add protect public api case

ec0aa3f

bzp2010 requested a review from spacewander January 21, 2022 05:47

spacewander reviewed Jan 21, 2022

View reviewed changes

test: add wrong public-api uri case

e66e65d

bzp2010 requested a review from spacewander January 21, 2022 09:22

spacewander approved these changes Jan 23, 2022

View reviewed changes

bzp2010 requested a review from spacewander January 24, 2022 01:06

shuaijinchao approved these changes Jan 24, 2022

View reviewed changes

spacewander merged commit f81788b into apache:master Jan 24, 2022

	ctx.var.uri = conf.uri and conf.uri or ctx.var.uri
	ctx.var.uri = conf.uri or ctx.var.uri

	router.router_http.match(api_ctx)

	local route = api_ctx.matched_route
	if not route then
	-- whether the public API run global rules is
	-- controlled by the configuration file
	if router.api.has_route_not_under_apisix() or
	core.string.has_prefix(uri, "/apisix/")
	then
	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

	-- run global rule when there is no matching route
	plugin.run_global_rules(api_ctx, router.global_rules, nil)

	core.log.info("not find any matched route")
	return core.response.exit(404,
	{error_msg = "404 Route Not Found"})
	end

	core.log.info("matched route: ",
	core.json.delay_encode(api_ctx.matched_route, true))

	local enable_websocket = route.value.enable_websocket

	if route.value.plugin_config_id then
	local conf = plugin_config.get(route.value.plugin_config_id)
	if not conf then
	core.log.error("failed to fetch plugin config by ",
	"id: ", route.value.plugin_config_id)
	return core.response.exit(503)
	end

	route = plugin_config.merge(route, conf)
	end

	if route.value.service_id then
	local service = service_fetch(route.value.service_id)
	if not service then
	core.log.error("failed to fetch service configuration by ",
	"id: ", route.value.service_id)
	return core.response.exit(404)
	end

	route = plugin.merge_service_route(service, route)
	api_ctx.matched_route = route
	api_ctx.conf_type = "route&service"
	api_ctx.conf_version = route.modifiedIndex .. "&" .. service.modifiedIndex
	api_ctx.conf_id = route.value.id .. "&" .. service.value.id
	api_ctx.service_id = service.value.id
	api_ctx.service_name = service.value.name

	if enable_websocket == nil then
	enable_websocket = service.value.enable_websocket
	end

	else
	api_ctx.conf_type = "route"
	api_ctx.conf_version = route.modifiedIndex
	api_ctx.conf_id = route.value.id
	end
	api_ctx.route_id = route.value.id
	api_ctx.route_name = route.value.name

	-- run global rule
	plugin.run_global_rules(api_ctx, router.global_rules, nil)

Uh oh!

Conversation

bzp2010 commented Jan 19, 2022

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzp2010 Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Update

Details

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shuaijinchao commented Jan 21, 2022

Uh oh!

bzp2010 commented Jan 21, 2022

Uh oh!

spacewander left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bzp2010 Jan 23, 2022 •

edited

Loading