[GitHub] [apisix] tzssangglass commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
tzssangglass commented on a change in pull request #3758: URL: https://github.com/apache/apisix/pull/3758#discussion_r592844344 ## File path: apisix/plugins/traffic-split.lua ## @@ -309,7 +314,8 @@ function _M.access(conf, ctx) return end -local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams) +local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams, Review comment: > So we can ensure either route_upstream_id or weighted_upstreams changes, the version will change. got! 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
tzssangglass commented on a change in pull request #3758: URL: https://github.com/apache/apisix/pull/3758#discussion_r592508330 ## File path: apisix/plugins/traffic-split.lua ## @@ -309,7 +314,8 @@ function _M.access(conf, ctx) return end -local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams) +local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams, Review comment: update. I know that using `core.lrucache.plugin_ctx` is more in line with the common way, but I don't understand the benefit of using `api_ctx.conf_type` and `api_ctx.conf_id` as lrucache keys over `weighted_upstreams`. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
tzssangglass commented on a change in pull request #3758: URL: https://github.com/apache/apisix/pull/3758#discussion_r592285312 ## File path: apisix/plugins/traffic-split.lua ## @@ -309,7 +314,8 @@ function _M.access(conf, ctx) return end -local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams) +local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams, Review comment: try ```lua local rr_up, err = core.lrucache.plugin_ctx(lrucache, ctx, route_upstream_id, new_rr_obj, weighted_upstreams, route_upstream_id) ``` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
tzssangglass commented on a change in pull request #3758: URL: https://github.com/apache/apisix/pull/3758#discussion_r592063112 ## File path: apisix/plugins/traffic-split.lua ## @@ -309,7 +314,8 @@ function _M.access(conf, ctx) return end -local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams) +local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams, Review comment: I tried it and it still doesn't work, because this plugin still need to control the weight by modifying the `upstream_id` in the route, even if `core.lrucache.plugin_ctx` is used, since `route_upstream_id` is in the key and `route_upstream_id` will be overwritten by the `plugin_upstream_id`, it will result in two keys and inaccurate data obtained. use `weighted_upstreams` as key because `weighted_upstreams` do not change. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
tzssangglass commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r588243727
##
File path: t/plugin/traffic-split.t
##
@@ -1615,7 +1615,134 @@ passed
-=== TEST 46: the upstream_id is used in the plugin
+=== TEST 46: set route(id: 1, upstream_id: 1, upstream_id in plugin: 2), and
`weighted_upstreams` does not have a structure with only `weight`
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/routes/1',
+ngx.HTTP_PUT,
+[=[{
+"uri": "/hello*",
+"plugins": {
+"traffic-split": {
+"rules": [
+{
+"match": [
+{
+"vars": [["uri", "==", "/hello"]]
+}
+],
+"weighted_upstreams": [
+{"upstream_id": 2}
+]
+}
+]
+}
+},
+"upstream_id":"1"
+}]=]
+)
+
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 47: when `match` rule passed, use the `upstream_id` in plugin, and
when it failed, use the `upstream_id` in route
+--- pipelined_requests eval
+["GET /hello", "GET /hello1", "GET /hello", "GET /hello1", "GET /hello", "GET
/hello1"]
+--- response_body eval
+["hello world\n", "hello1 world\n", "hello world\n", "hello1 world\n", "hello
world\n", "hello1 world\n"]
+--- grep_error_log_out eval
+[
+"match_flag: true",
+"upstream_id: 2",
+"match_flag: false",
+"original_uid: 1"
+]
+
+
+
+=== TEST 48: set route(id: 1, upstream_id: 1, upstream_id in plugin: 2), and
`weighted_upstreams` has a structure with only `weight`
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/routes/1',
+ngx.HTTP_PUT,
+[=[{
+"uri": "/server_port",
+"plugins": {
+"traffic-split": {
+"rules": [
+{
+"match": [
+{
+"vars": [["uri", "==",
"/server_port"]]
+}
+],
+"weighted_upstreams": [
+{"upstream_id": 2, "weight": 1},
+{"weight": 1}
+]
+}
+]
+}
+},
+"upstream_id":"1"
+}]=]
+)
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 49: all requests `match` rule passed, proxy requests to the upstream
of route based on the structure with only `weight` in `weighted_upstreams`
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local bodys = {}
+local headers = {}
Review comment:
update
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
tzssangglass commented on a change in pull request #3758: URL: https://github.com/apache/apisix/pull/3758#discussion_r588056323 ## File path: apisix/plugins/traffic-split.lua ## @@ -309,7 +314,8 @@ function _M.access(conf, ctx) return end -local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams) +local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams, Review comment: update 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
tzssangglass commented on a change in pull request #3758: URL: https://github.com/apache/apisix/pull/3758#discussion_r588053853 ## File path: apisix/plugins/traffic-split.lua ## @@ -309,7 +314,8 @@ function _M.access(conf, ctx) return end -local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams) +local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams, Review comment: you mean ```lua local route_upstream_id = ctx.matched_route.value.upstream_id local rr_up, err = lrucache(weighted_upstreams, nil, new_rr_obj, weighted_upstreams, route_upstream_id) ``` ? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
tzssangglass commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r587953009
##
File path: apisix/plugins/traffic-split.lua
##
@@ -320,6 +324,7 @@ function _M.access(conf, ctx)
core.log.info("upstream: ", core.json.encode(upstream))
return set_upstream(upstream, ctx)
elseif upstream and upstream ~= "plugin#upstream#is#empty" then
+ctx.matched_route.value["original_uid"] =
ctx.matched_route.value.upstream_id
Review comment:
yes, it will be overridden, I continue to fix 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
tzssangglass commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r587295638
##
File path: apisix/plugins/traffic-split.lua
##
@@ -320,6 +324,7 @@ function _M.access(conf, ctx)
core.log.info("upstream: ", core.json.encode(upstream))
return set_upstream(upstream, ctx)
elseif upstream and upstream ~= "plugin#upstream#is#empty" then
+ctx.matched_route.value["original_uid"] =
ctx.matched_route.value.upstream_id
Review comment:
I don't think so, because here it will directly return
https://github.com/apache/apisix/blob/4820ad93e5fc0550addbd7029a20eab6b7ea800e/apisix/plugins/traffic-split.lua#L322-L325
##
File path: apisix/plugins/traffic-split.lua
##
@@ -306,6 +306,10 @@ function _M.access(conf, ctx)
core.log.info("match_flag: ", match_flag)
if not match_flag then
+if ctx.matched_route.value["original_uid"] then
Review comment:
got
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
