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.

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: add public api plugin #6145

Merged
merged 17 commits into from
Jan 24, 2022
Merged

Conversation

bzp2010
Copy link
Contributor

@bzp2010 bzp2010 commented Jan 19, 2022

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

@bzp2010 bzp2010 added enhancement New feature or request plugin labels Jan 19, 2022
@bzp2010 bzp2010 self-assigned this Jan 19, 2022
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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


-- 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
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to run global rules twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

  1. 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.
  2. HTTP not matched, public API route matched.
    This will also be controlled by the configuration file whether to enforce global rules or not.
  3. 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to fix it.

Copy link
Contributor Author

@bzp2010 bzp2010 Jan 23, 2022

Choose a reason for hiding this comment

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

Update

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

Details

image

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.

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

  2. run after HTTP matched
    image

  3. 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


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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

Let's add more cases:

  1. public api not match
  2. match without optional uri
  3. add auth to protect public api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. added
  2. 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.
  3. added, protect jwt sign by key-auth

@bzp2010 bzp2010 linked an issue Jan 21, 2022 that may be closed by this pull request
=== TEST 5: missing route
--- request
GET /apisix/plugin/balalbala
--- error_code: 404
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, adding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@shuaijinchao
Copy link
Member

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

@bzp2010
Copy link
Contributor Author

bzp2010 commented Jan 21, 2022

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.

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Approve except #6145 (comment)

@spacewander spacewander merged commit f81788b into apache:master Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: support more access control for plugin's public API
3 participants