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 2021/03/04 08:35:16 UTC
[GitHub] [apisix] tzssangglass opened a new pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
tzssangglass opened a new pull request #3758:
URL: https://github.com/apache/apisix/pull/3758
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
fix: #3740
### Pre-submission checklist:
the cause of the bug reference: https://github.com/apache/apisix/issues/3740#issuecomment-789852741
this change temporarily stores the route's `upstream_id` in the `matched_route` table. If the current request `match` fails and the temporarily stored `upstream_id` exists in the `matched_route` table, the route's `upstream_id` is restored to the temporarily stored `upstream_id`.
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r592092002
##########
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?
```diff
diff --git apisix/plugins/traffic-split.lua apisix/plugins/traffic-split.lua
index eb7a2028..966275f9 100644
--- apisix/plugins/traffic-split.lua
+++ apisix/plugins/traffic-split.lua
@@ -315,8 +315,8 @@ function _M.access(conf, ctx)
end
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)
+ local rr_up, err = core.lrucache.plugin_ctx(lrucache, ctx, nil, new_rr_obj, weighted_upstreams,
+ route_upstream_id)
if not rr_up then
core.log.error("lrucache roundrobin failed: ", err)
return 500
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r588037942
##########
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:
Need to use `ctx.matched_route.value.upstream_id` as the extra 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.
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r588063801
##########
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:
`lrucache` accepts an extra key as the second argument.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r587375455
##########
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 mean, if upstream == "plugin#upstream#is#empty", will the original upstream_id be overridden?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#issuecomment-792443816
After apply this change:
```
diff --git apisix/plugins/traffic-split.lua apisix/plugins/traffic-split.lua
index eb7a2028..3d5e7d8f 100644
--- apisix/plugins/traffic-split.lua
+++ apisix/plugins/traffic-split.lua
@@ -315,8 +315,7 @@ function _M.access(conf, ctx)
end
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)
+ local rr_up, err = new_rr_obj(weighted_upstreams, route_upstream_id)
if not rr_up then
core.log.error("lrucache roundrobin failed: ", err)
return 500
```
many test cases are failed. Look like `new_rr_obj` corrupts the route's configuration. This may happen once the cache is evicted.
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r589897285
##########
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:
@tzssangglass
Personally, we need to add `route_upstream_id` to the key as `route_upstream_id` and `weighted_upstreams` may change differently. As using `route_upstream_id` in the second argument will hit the bug I mention, maybe we can switch to use https://github.com/apache/apisix/blob/c530135c5a179c99637d52d99e0e8a45ce4cc8b8/apisix/plugins/limit-req.lua#L69?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#issuecomment-793235239
This PR does not do this work. We should open another issue to discuss it if needed, because this time it involves refactoring the plugin's implementation logic.
> There should be a way to do the same thing without corrupting the data. Otherwise once the cache is evicted we will be in trouble.
This PR does not do this work. If needed, we should open another issue to discuss it, as it would involve refactoring the plugin's implementation logic.
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [apisix] tzssangglass edited a comment on pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
tzssangglass edited a comment on pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#issuecomment-793235239
> There should be a way to do the same thing without corrupting the data. Otherwise once the cache is evicted we will be in trouble.
This PR does not do this work. If needed, we should open another issue to discuss it, as it would involve refactoring the plugin's implementation logic.
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#issuecomment-792658706
> Look like new_rr_obj corrupts the route's configuration. This may happen once the cache is evicted.
yes, according to my understanding, I think the design idea of `traffic-split` itself is to achieve the goal by corrupts the route's configuration.
take a look at
https://github.com/apache/apisix/blob/6cbaf55aab7c572410adfc4981f42f725d058ccb/apisix/plugins/traffic-split.lua#L322-L323
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r587280426
##########
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:
Better to use value.original_upstream_id
##########
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:
Should do the same thing with the code below?
```
core.log.info("route_up: ", upstream)
ctx.matched_route.value.upstream_id = 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.
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [apisix] tokers merged pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
tokers merged pull request #3758:
URL: https://github.com/apache/apisix/pull/3758
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#issuecomment-793221849
There should be a way to do the same thing without corrupting the data. Otherwise once the cache is evicted we will be in trouble.
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r588085262
##########
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:
I think it is not used by `headers` and we should remove 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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r588284177
##########
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:
Need to solve 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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3758: fix: the traffic-split plugin is invalid to bind upstream via upstream_id
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3758:
URL: https://github.com/apache/apisix/pull/3758#discussion_r592841252
##########
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:
The default key should ensure if the configuration of the route changes, it will change too. So we can ensure either route_upstream_id or weighted_upstreams changes, the version will change.
The corruption of the route configuration is another story (which should be fixed)...
----------------------------------------------------------------
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:
users@infra.apache.org