You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "piglei (via GitHub)" <gi...@apache.org> on 2023/05/15 02:59:32 UTC
[GitHub] [apisix] piglei opened a new pull request, #9482: Inform user when request /plugins/reload/ using wrong HTTP methods
piglei opened a new pull request, #9482:
URL: https://github.com/apache/apisix/pull/9482
### Description
Inform user when request /plugins/reload/ using wrong HTTP methods. More details in https://github.com/apache/apisix/issues/9481
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
- **confirmation required.**
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 merged pull request #9482: feat: Inform user when request /plugins/reload/ using wrong HTTP methods
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 merged PR #9482:
URL: https://github.com/apache/apisix/pull/9482
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on pull request #9482: feat: Inform user when request /plugins/reload/ using wrong HTTP methods
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9482:
URL: https://github.com/apache/apisix/pull/9482#issuecomment-1566433208
@piglei Could you merge the master?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] piglei commented on a diff in pull request #9482: feat: Inform user when request /plugins/reload/ using wrong HTTP methods
Posted by "piglei (via GitHub)" <gi...@apache.org>.
piglei commented on code in PR #9482:
URL: https://github.com/apache/apisix/pull/9482#discussion_r1203602268
##########
apisix/admin/init.lua:
##########
@@ -142,14 +162,8 @@ end
local function run()
- local api_ctx = {}
- core.ctx.set_vars_meta(api_ctx)
- ngx.ctx.api_ctx = api_ctx
-
- local ok, err = check_token(api_ctx)
- if not ok then
- core.log.warn("failed to check token: ", err)
- core.response.exit(401, {error_msg = "failed to check token"})
+ if not set_ctx_and_check_token() then
Review Comment:
Thank you for pointing this out. I've updated the function to remove the value-returning logic, the function-call statements were updated as well.
PS: After this modification, I'm thinking that if "set_ctx_and_check_token" is still a good name, because the name didn't imply that the request would exit. Maybe "exit_if_token_invalid" is a better name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] piglei commented on pull request #9482: feat: Inform user when request /plugins/reload/ using wrong HTTP methods
Posted by "piglei (via GitHub)" <gi...@apache.org>.
piglei commented on PR #9482:
URL: https://github.com/apache/apisix/pull/9482#issuecomment-1566455336
> @piglei Could you merge the master?
Synced with apache/master .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] piglei commented on a diff in pull request #9482: feat: Inform user when request /plugins/reload/ using wrong HTTP methods
Posted by "piglei (via GitHub)" <gi...@apache.org>.
piglei commented on code in PR #9482:
URL: https://github.com/apache/apisix/pull/9482#discussion_r1203602268
##########
apisix/admin/init.lua:
##########
@@ -142,14 +162,8 @@ end
local function run()
- local api_ctx = {}
- core.ctx.set_vars_meta(api_ctx)
- ngx.ctx.api_ctx = api_ctx
-
- local ok, err = check_token(api_ctx)
- if not ok then
- core.log.warn("failed to check token: ", err)
- core.response.exit(401, {error_msg = "failed to check token"})
+ if not set_ctx_and_check_token() then
Review Comment:
Thank you for pointing this out. I've updated the function to remove the value-returning logic, the function-call statements were updated as well.
PS: After this modification, I'm thinking that if "set_ctx_and_check_token" is still a good name, because the name didn't imply that the request would exit. Maybe "exit_if_token_invalid" is better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] shreemaan-abhishek commented on pull request #9482: feat: Inform user when request /plugins/reload/ using wrong HTTP methods
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #9482:
URL: https://github.com/apache/apisix/pull/9482#issuecomment-1553423091
I think it would be better to allow usage of GET method for plugin reload instead of warning user about wrong method usage, WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] piglei commented on pull request #9482: feat: Inform user when request /plugins/reload/ using wrong HTTP methods
Posted by "piglei (via GitHub)" <gi...@apache.org>.
piglei commented on PR #9482:
URL: https://github.com/apache/apisix/pull/9482#issuecomment-1553773107
> I think it would be better to allow usage of GET method for plugin reload instead of warning user about wrong method usage, WDYT?
Adding support for using GET to reload the plugins is okay and convenient. But consider that the "reload" endpoint is in the "/plugins/reload/<:string>" pattern family and the result of calling other endpoints via GET is reading information of the plugins, which has no side-effect at all. So, it's better to only support the PUT method for doing a plugin reload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9482: feat: Inform user when request /plugins/reload/ using wrong HTTP methods
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9482:
URL: https://github.com/apache/apisix/pull/9482#discussion_r1203422007
##########
apisix/admin/init.lua:
##########
@@ -142,14 +162,8 @@ end
local function run()
- local api_ctx = {}
- core.ctx.set_vars_meta(api_ctx)
- ngx.ctx.api_ctx = api_ctx
-
- local ok, err = check_token(api_ctx)
- if not ok then
- core.log.warn("failed to check token: ", err)
- core.response.exit(401, {error_msg = "failed to check token"})
+ if not set_ctx_and_check_token() then
Review Comment:
no return value is needed, the `core.response.exit` will terminate the processing of the current request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org