You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "Sn0rt (via GitHub)" <gi...@apache.org> on 2023/04/28 02:22:22 UTC
[GitHub] [apisix] Sn0rt opened a new pull request, #9388: feature: proxy-mirror support grpc and grpcs
Sn0rt opened a new pull request, #9388:
URL: https://github.com/apache/apisix/pull/9388
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes https://github.com/apache/apisix/issues/7437 https://github.com/apache/apisix/issues/9245
### 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)
<!--
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 commented on pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9388:
URL: https://github.com/apache/apisix/pull/9388#issuecomment-1535074233
No test cases?
--
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 #9388: feat: proxy-mirror support grpc and grpcs
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1188031073
##########
apisix/init.lua:
##########
@@ -688,6 +689,13 @@ function _M.grpc_access_phase()
core.log.error("failed to set grpcs upstream param: ", err)
core.response.exit(code)
end
+
+ if api_ctx.var.upstream_mirror_scheme == "grpc" or
+ api_ctx.var.upstream_mirror_scheme == "grpcs" then
+ if has_mod then
+ apisix_ngx_client.enable_mirror()
+ end
+ end
Review Comment:
I think removing this code is enough
--
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] Sn0rt commented on a diff in pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1189277500
##########
t/APISIX.pm:
##########
@@ -743,6 +744,8 @@ _EOC_
}
location / {
+ set \$upstream_mirror_scheme '';
Review Comment:
yep. not need this var anymore. I will rollback some change.
##########
apisix/plugins/proxy-mirror.lua:
##########
@@ -114,12 +117,14 @@ function _M.rewrite(conf, ctx)
if conf.sample_ratio == 1 then
enable_mirror(ctx, conf)
+ ctx.enable_mirror = 1
Review Comment:
of course.
--
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 #9388: feat: proxy-mirror support grpc and grpcs
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1189473600
##########
apisix/core/ctx.lua:
##########
@@ -190,6 +190,8 @@ do
upstream_connection = true,
upstream_uri = true,
+ upstream_mirror_scheme = true,
Review Comment:
Why not delete 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
[GitHub] [apisix] Sn0rt commented on a diff in pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1189265668
##########
docs/zh/latest/plugins/proxy-mirror.md:
##########
@@ -40,9 +40,9 @@ description: 本文介绍了 Apache APISIX proxy-mirror 插件的相关操作，
| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
| ---- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------- |
-| host | string | 是 | | | 指定镜像服务的地址，地址中需要包含 `schema`（`http` 或 `https`），但不能包含 `path` 部分。例如 `http://127.0.0.1:9797`。 |
-| path | string | 否 | | | 指定镜像请求的路径。如果不指定，则默认会使用当前路径。 |
-| path_concat_mode | string | 否 | replace | ["replace", "prefix"] | 当指定镜像请求的路径时，设置请求路径的拼接模式。`replace` 模式将会直接使用 `path` 作为镜像请求的路径。`prefix` 模式将会使用 `path` + `来源请求 URI` 作为镜像请求的路径。 |
+| host | string | 是 | | | 指定镜像服务的地址，地址中需要包含 `schema`（`http(s)` 或 `grpc(s)`），但不能包含 `path` 部分。例如 `http://127.0.0.1:9797`。 |
+| path | string | 否 | | | 指定镜像请求的路径。如果不指定，则默认会使用当前路径。如果是为了镜像 grpc 流量，这个选项不再适用。|
Review Comment:
At present, the document format is a case
--
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] Sn0rt commented on pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on PR #9388:
URL: https://github.com/apache/apisix/pull/9388#issuecomment-1539432186
@monkeyDluffy6017 pls help to re-try the CI
--
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] Sn0rt commented on a diff in pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1189268288
##########
apisix/init.lua:
##########
@@ -711,11 +712,16 @@ function _M.grpc_access_phase()
return
end
+
Review Comment:
done
--
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 #9388: feat: proxy-mirror support grpc and grpcs
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1187136254
##########
apisix/init.lua:
##########
@@ -688,6 +689,13 @@ function _M.grpc_access_phase()
core.log.error("failed to set grpcs upstream param: ", err)
core.response.exit(code)
end
+
+ if api_ctx.var.upstream_mirror_scheme == "grpc" or
+ api_ctx.var.upstream_mirror_scheme == "grpcs" then
+ if has_mod then
+ apisix_ngx_client.enable_mirror()
+ end
+ end
Review Comment:
Why do you enable mirror here? it should be set in `proxy-mirror` plugin
If you enable here, I don't think the `sample_ratio` will work
--
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] Sn0rt commented on a diff in pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1187381126
##########
apisix/init.lua:
##########
@@ -688,6 +689,13 @@ function _M.grpc_access_phase()
core.log.error("failed to set grpcs upstream param: ", err)
core.response.exit(code)
end
+
+ if api_ctx.var.upstream_mirror_scheme == "grpc" or
+ api_ctx.var.upstream_mirror_scheme == "grpcs" then
+ if has_mod then
+ apisix_ngx_client.enable_mirror()
+ end
+ end
Review Comment:
You are right, if you want to support the plug-in list like http, the code may look like the following.
```diff
diff --git a/apisix/init.lua b/apisix/init.lua
index 56228670..f4ed438c 100644
--- a/apisix/init.lua
+++ b/apisix/init.lua
@@ -690,12 +690,20 @@ function _M.grpc_access_phase()
core.response.exit(code)
end
- if api_ctx.var.upstream_mirror_scheme == "grpc" or
- api_ctx.var.upstream_mirror_scheme == "grpcs" then
- if has_mod then
- apisix_ngx_client.enable_mirror()
- end
+ local route = api_ctx.matched_route
+ if not route then
+ 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 plugins = plugin.filter(api_ctx, route)
+ api_ctx.plugins = plugins
+
+ plugin.run_plugin("rewrite", plugins, api_ctx)
end
```
--
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] Sn0rt commented on pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on PR #9388:
URL: https://github.com/apache/apisix/pull/9388#issuecomment-1536035696
@monkeyDluffy6017 pls help me to re-try the ci.
--
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] Sn0rt commented on a diff in pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1189479381
##########
apisix/core/ctx.lua:
##########
@@ -190,6 +190,8 @@ do
upstream_connection = true,
upstream_uri = true,
+ upstream_mirror_scheme = true,
Review Comment:
loss. now 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] leslie-tsang merged pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "leslie-tsang (via GitHub)" <gi...@apache.org>.
leslie-tsang merged PR #9388:
URL: https://github.com/apache/apisix/pull/9388
--
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] Sn0rt commented on pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on PR #9388:
URL: https://github.com/apache/apisix/pull/9388#issuecomment-1535563774
> No test cases?
sorry. The PR is POC. I push a new PR by force. thx for you review.
--
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] Sn0rt commented on a diff in pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1187381126
##########
apisix/init.lua:
##########
@@ -688,6 +689,13 @@ function _M.grpc_access_phase()
core.log.error("failed to set grpcs upstream param: ", err)
core.response.exit(code)
end
+
+ if api_ctx.var.upstream_mirror_scheme == "grpc" or
+ api_ctx.var.upstream_mirror_scheme == "grpcs" then
+ if has_mod then
+ apisix_ngx_client.enable_mirror()
+ end
+ end
Review Comment:
You are right, if you want to support the plug-in list like http, the code may look like the following.
```diff
diff --git a/apisix/init.lua b/apisix/init.lua
index 56228670..f4ed438c 100644
--- a/apisix/init.lua
+++ b/apisix/init.lua
@@ -690,12 +690,20 @@ function _M.grpc_access_phase()
core.response.exit(code)
end
- if api_ctx.var.upstream_mirror_scheme == "grpc" or
- api_ctx.var.upstream_mirror_scheme == "grpcs" then
- if has_mod then
- apisix_ngx_client.enable_mirror()
- end
+ local route = api_ctx.matched_route
+ if not route then
+ 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 plugins = plugin.filter(api_ctx, route)
+ api_ctx.plugins = plugins
+
+ plugin.run_plugin("rewrite", plugins, api_ctx)
end
```
--
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 #9388: feat: proxy-mirror support grpc and grpcs
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1187136254
##########
apisix/init.lua:
##########
@@ -688,6 +689,13 @@ function _M.grpc_access_phase()
core.log.error("failed to set grpcs upstream param: ", err)
core.response.exit(code)
end
+
+ if api_ctx.var.upstream_mirror_scheme == "grpc" or
+ api_ctx.var.upstream_mirror_scheme == "grpcs" then
+ if has_mod then
+ apisix_ngx_client.enable_mirror()
+ end
+ end
Review Comment:
Why do you enable mirror here? it should be set in `proxy-mirror` plugin
--
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 #9388: feat: proxy-mirror support grpc and grpcs
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1188748118
##########
apisix/plugins/proxy-mirror.lua:
##########
@@ -114,12 +117,14 @@ function _M.rewrite(conf, ctx)
if conf.sample_ratio == 1 then
enable_mirror(ctx, conf)
+ ctx.enable_mirror = 1
Review Comment:
Is `true` 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] Sn0rt commented on a diff in pull request #9388: feat: proxy-mirror support grpc and grpcs
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1189265668
##########
docs/zh/latest/plugins/proxy-mirror.md:
##########
@@ -40,9 +40,9 @@ description: 本文介绍了 Apache APISIX proxy-mirror 插件的相关操作，
| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
| ---- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------- |
-| host | string | 是 | | | 指定镜像服务的地址，地址中需要包含 `schema`（`http` 或 `https`），但不能包含 `path` 部分。例如 `http://127.0.0.1:9797`。 |
-| path | string | 否 | | | 指定镜像请求的路径。如果不指定，则默认会使用当前路径。 |
-| path_concat_mode | string | 否 | replace | ["replace", "prefix"] | 当指定镜像请求的路径时，设置请求路径的拼接模式。`replace` 模式将会直接使用 `path` 作为镜像请求的路径。`prefix` 模式将会使用 `path` + `来源请求 URI` 作为镜像请求的路径。 |
+| host | string | 是 | | | 指定镜像服务的地址，地址中需要包含 `schema`（`http(s)` 或 `grpc(s)`），但不能包含 `path` 部分。例如 `http://127.0.0.1:9797`。 |
+| path | string | 否 | | | 指定镜像请求的路径。如果不指定，则默认会使用当前路径。如果是为了镜像 grpc 流量，这个选项不再适用。|
Review Comment:
At present, the document format is one case for one plugin.
--
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 #9388: feat: proxy-mirror support grpc and grpcs
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1188746769
##########
apisix/init.lua:
##########
@@ -711,11 +712,16 @@ function _M.grpc_access_phase()
return
end
+
Review Comment:
remove this blank
--
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 #9388: feat: proxy-mirror support grpc and grpcs
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1188786154
##########
t/APISIX.pm:
##########
@@ -743,6 +744,8 @@ _EOC_
}
location / {
+ set \$upstream_mirror_scheme '';
Review Comment:
upstream_mirror_scheme is not used?
--
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 #9388: feat: proxy-mirror support grpc and grpcs
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9388:
URL: https://github.com/apache/apisix/pull/9388#discussion_r1188789599
##########
docs/zh/latest/plugins/proxy-mirror.md:
##########
@@ -40,9 +40,9 @@ description: 本文介绍了 Apache APISIX proxy-mirror 插件的相关操作，
| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
| ---- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------- |
-| host | string | 是 | | | 指定镜像服务的地址，地址中需要包含 `schema`（`http` 或 `https`），但不能包含 `path` 部分。例如 `http://127.0.0.1:9797`。 |
-| path | string | 否 | | | 指定镜像请求的路径。如果不指定，则默认会使用当前路径。 |
-| path_concat_mode | string | 否 | replace | ["replace", "prefix"] | 当指定镜像请求的路径时，设置请求路径的拼接模式。`replace` 模式将会直接使用 `path` 作为镜像请求的路径。`prefix` 模式将会使用 `path` + `来源请求 URI` 作为镜像请求的路径。 |
+| host | string | 是 | | | 指定镜像服务的地址，地址中需要包含 `schema`（`http(s)` 或 `grpc(s)`），但不能包含 `path` 部分。例如 `http://127.0.0.1:9797`。 |
+| path | string | 否 | | | 指定镜像请求的路径。如果不指定，则默认会使用当前路径。如果是为了镜像 grpc 流量，这个选项不再适用。|
Review Comment:
could you give an example about how to use grpc mirror in doc?
--
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