You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/03/21 02:23:21 UTC
[GitHub] [apisix] bin-ya opened a new pull request #6670: feat: Add the function of hiding header for key-auth plugin
bin-ya opened a new pull request #6670:
URL: https://github.com/apache/apisix/pull/6670
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Add the function of hiding header for key-auth plugin
### 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
- [x] 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)
<!--
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] bin-ya commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
bin-ya commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r840277854
##########
File path: apisix/plugins/key-auth.lua
##########
@@ -110,6 +114,12 @@ function _M.rewrite(conf, ctx)
end
core.log.info("consumer: ", core.json.delay_encode(consumer))
+ if conf.hide_credentials then
+ core.request.set_header(ctx, conf.header, nil)
+ core.request.get_uri_args(ctx)
+ core.request.set_uri_args(ctx, {})
Review comment:
Hide information from query
--
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] tzssangglass commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r840368606
##########
File path: apisix/plugins/key-auth.lua
##########
@@ -110,6 +114,12 @@ function _M.rewrite(conf, ctx)
end
core.log.info("consumer: ", core.json.delay_encode(consumer))
+ if conf.hide_credentials then
+ core.request.set_header(ctx, conf.header, nil)
+ core.request.get_uri_args(ctx)
+ core.request.set_uri_args(ctx, {})
Review comment:
I think we don't necessary to remove the all query params
If the key is in the header, we only need to delete the key in the headers.
If the key is in the query, we only need to delete the key in the query params.
cc @spacewander right?
--
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] bin-ya commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
bin-ya commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r839391826
##########
File path: apisix/plugins/key-auth.lua
##########
@@ -110,6 +114,10 @@ function _M.rewrite(conf, ctx)
end
core.log.info("consumer: ", core.json.delay_encode(consumer))
+ if conf.hide_credentials then
Review comment:
ok, I will.
--
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] tzssangglass commented on pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#issuecomment-1078729272
File conflicts have not been resolved @bin-ya
--
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] bin-ya commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
bin-ya commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r835026807
##########
File path: docs/zh/latest/plugins/key-auth.md
##########
@@ -41,6 +41,7 @@ router 端配置：
| ---- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------------- |
| header | string | 可选| apikey | | 设置我们从哪个 header 获取 key。 |
| query | string | 可选 | apikey | | 设置我们从哪个 query string 获取 key，优先级低于 `header` |
+| hide_credentials | bool | 可选 | false | | 是否将请求头传递给 upstream。 |
Review comment:
ok
--
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] tokers commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r835999872
##########
File path: docs/en/latest/plugins/key-auth.md
##########
@@ -41,6 +41,7 @@ For route side:
| ---- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------- |
| header | string | optional | apikey | | the header we get the key from |
| query | string | optional | apikey | | the query string we get the key from, which priority is lower than `header` |
+| hide_credentials | bool | optional | false | | Whether to pass the apikey request headers to the upstream. |
Review comment:
You should only handle the query that used to carry the credential.
--
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] tzssangglass commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r830884499
##########
File path: apisix/plugins/key-auth.lua
##########
@@ -110,6 +114,10 @@ function _M.rewrite(conf, ctx)
end
core.log.info("consumer: ", core.json.delay_encode(consumer))
+ if conf.hide_credentials then
+ core.request.set_header(ctx, "apikey", nil)
Review comment:
```suggestion
core.request.set_header(ctx, conf.header, nil)
```
--
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] bin-ya commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
bin-ya commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r833852347
##########
File path: docs/en/latest/plugins/key-auth.md
##########
@@ -41,6 +41,7 @@ For route side:
| ---- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------- |
| header | string | optional | apikey | | the header we get the key from |
| query | string | optional | apikey | | the query string we get the key from, which priority is lower than `header` |
+| hide_credentials | bool | optional | false | | Whether to pass the apikey request headers to the upstream. |
Review comment:
Query is the content modified by others. Can I delete it?
--
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] bin-ya commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
bin-ya commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r833040722
##########
File path: docs/en/latest/plugins/key-auth.md
##########
@@ -41,6 +41,7 @@ For route side:
| ---- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------- |
| header | string | optional | apikey | | the header we get the key from |
| query | string | optional | apikey | | the query string we get the key from, which priority is lower than `header` |
+| hide_credentials | bool | optional | false | | Whether to pass the apikey request headers to the upstream. |
Review comment:
ok
--
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] spacewander commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r840985748
##########
File path: apisix/plugins/key-auth.lua
##########
@@ -110,6 +114,12 @@ function _M.rewrite(conf, ctx)
end
core.log.info("consumer: ", core.json.delay_encode(consumer))
+ if conf.hide_credentials then
+ core.request.set_header(ctx, conf.header, nil)
+ core.request.get_uri_args(ctx)
+ core.request.set_uri_args(ctx, {})
Review comment:
Yes, we should only remove the key.
--
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] tokers commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r833025344
##########
File path: docs/en/latest/plugins/key-auth.md
##########
@@ -41,6 +41,7 @@ For route side:
| ---- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------- |
| header | string | optional | apikey | | the header we get the key from |
| query | string | optional | apikey | | the query string we get the key from, which priority is lower than `header` |
+| hide_credentials | bool | optional | false | | Whether to pass the apikey request headers to the upstream. |
Review comment:
It's confusing, the header is not necessary to be "apikey", it's specified by the `header`. Also, the query should also be removed.
--
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] spacewander commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r838046274
##########
File path: apisix/plugins/key-auth.lua
##########
@@ -110,6 +114,10 @@ function _M.rewrite(conf, ctx)
end
core.log.info("consumer: ", core.json.delay_encode(consumer))
+ if conf.hide_credentials then
Review comment:
We should remove the key according to where the key comes from.
--
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] bin-ya commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
bin-ya commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r840986537
##########
File path: apisix/plugins/key-auth.lua
##########
@@ -110,6 +114,12 @@ function _M.rewrite(conf, ctx)
end
core.log.info("consumer: ", core.json.delay_encode(consumer))
+ if conf.hide_credentials then
+ core.request.set_header(ctx, conf.header, nil)
+ core.request.get_uri_args(ctx)
+ core.request.set_uri_args(ctx, {})
Review comment:
OK, I'll revise it right away.
--
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] tzssangglass commented on pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#issuecomment-1073815767
hi @bin-ya PTAL https://github.com/apache/apisix/runs/5624966335?check_suite_focus=true
--
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] tzssangglass commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r835010278
##########
File path: docs/zh/latest/plugins/key-auth.md
##########
@@ -41,6 +41,7 @@ router 端配置：
| ---- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------------- |
| header | string | 可选| apikey | | 设置我们从哪个 header 获取 key。 |
| query | string | 可选 | apikey | | 设置我们从哪个 query string 获取 key，优先级低于 `header` |
+| hide_credentials | bool | 可选 | false | | 是否将请求头传递给 upstream。 |
Review comment:
```suggestion
| hide_credentials | bool | 可选 | false | | 是否将含有认证信息的请求头传递给 upstream。 |
```
--
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] spacewander commented on pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#issuecomment-1073508039
Let's merge master to make CI pass.
--
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] starsz commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r836120178
##########
File path: docs/en/latest/plugins/key-auth.md
##########
@@ -41,6 +41,7 @@ For route side:
| ---- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------- |
| header | string | optional | apikey | | the header we get the key from |
| query | string | optional | apikey | | the query string we get the key from, which priority is lower than `header` |
+| hide_credentials | bool | optional | false | | Whether to pass the request header containing authentication information to upstream. |
Review comment:
Need to align the `|`.
--
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] tzssangglass commented on a change in pull request #6670: feat: Add the function of hiding header for key-auth plugin
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6670:
URL: https://github.com/apache/apisix/pull/6670#discussion_r840277034
##########
File path: apisix/plugins/key-auth.lua
##########
@@ -110,6 +114,12 @@ function _M.rewrite(conf, ctx)
end
core.log.info("consumer: ", core.json.delay_encode(consumer))
+ if conf.hide_credentials then
+ core.request.set_header(ctx, conf.header, nil)
+ core.request.get_uri_args(ctx)
+ core.request.set_uri_args(ctx, {})
Review comment:
What is the purpose of doing this?
--
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