fix(traffic-split): binding upstream via upstream_id is invalid (#3842)

apache · Mar 17, 2021 · faab4ba · faab4ba
1 parent ae2db15
commit faab4ba
Show file tree

Hide file tree

Showing 3 changed files with 130 additions and 30 deletions.
diff --git a/apisix/init.lua b/apisix/init.lua
@@ -387,6 +387,12 @@ function _M.http_access_phase()
     end
 
     local up_id = route.value.upstream_id
+
+    -- used for the traffic-split plugin
+    if api_ctx.upstream_id then
+        up_id = api_ctx.upstream_id
+    end
+
     if up_id then
         local upstreams = core.config.fetch_created_obj("/upstreams")
         if upstreams then

diff --git a/apisix/plugins/traffic-split.lua b/apisix/plugins/traffic-split.lua
@@ -226,7 +226,7 @@ local function set_upstream(upstream_info, ctx)
 end
 
 
-local function new_rr_obj(weighted_upstreams, route_upstream_id)
+local function new_rr_obj(weighted_upstreams)
     local server_list = {}
     for i, upstream_obj in ipairs(weighted_upstreams) do
         if upstream_obj.upstream_id then
@@ -239,12 +239,8 @@ local function new_rr_obj(weighted_upstreams, route_upstream_id)
             -- If the upstream object has only the weight value, it means
             -- that the upstream weight value on the default route has been reached.
             -- Mark empty upstream services in the plugin.
-            if route_upstream_id then
-                server_list[route_upstream_id] = upstream_obj.weight
-            else
-                upstream_obj.upstream = "plugin#upstream#is#empty"
-                server_list[upstream_obj.upstream] = upstream_obj.weight
-            end
+            upstream_obj.upstream = "plugin#upstream#is#empty"
+            server_list[upstream_obj.upstream] = upstream_obj.weight
 
         end
     end
@@ -285,9 +281,8 @@ function _M.access(conf, ctx)
         return
     end
 
-    local route_upstream_id = ctx.matched_route.value.upstream_id
     local rr_up, err = core.lrucache.plugin_ctx(lrucache, ctx, nil, new_rr_obj,
-                                                weighted_upstreams,route_upstream_id)
+                                                weighted_upstreams)
     if not rr_up then
         core.log.error("lrucache roundrobin failed: ", err)
         return 500
@@ -298,13 +293,13 @@ 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.upstream_id = upstream
+        ctx.upstream_id = upstream
         core.log.info("upstream_id: ", upstream)
         return
     end
 
+    ctx.upstream_id = nil
     core.log.info("route_up: ", upstream)
-    ctx.matched_route.value.upstream_id = nil
     return
 end
 

diff --git a/t/plugin/traffic-split.t b/t/plugin/traffic-split.t
@@ -1623,14 +1623,14 @@ passed
             local code, body = t('/apisix/admin/routes/1',
                 ngx.HTTP_PUT,
                 [=[{
-                    "uri": "/hello*",
+                    "uri": "/server_port",
                     "plugins": {
                         "traffic-split": {
                             "rules": [
                                 {
                                     "match": [
                                         {
-                                            "vars": [["uri", "==", "/hello"]]
+                                            "vars": [["arg_name", "==", "James"]]
                                         }
                                     ],
                                     "weighted_upstreams": [
@@ -1660,16 +1660,115 @@ passed
 
 
 === 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"]
+--- config
+location /t {
+    content_by_lua_block {
+        local t = require("lib.test_admin").test
+        local bodys = {}
+
+        for i = 1, 5, 2 do
+            -- match rule passed
+            local _, _, body = t('/server_port?name=James', ngx.HTTP_GET)
+            bodys[i] = body
+
+             -- match rule failed
+            local _, _, body = t('/server_port', ngx.HTTP_GET)
+            bodys[i+1] = body
+        end
+
+        table.sort(bodys)
+        ngx.say(table.concat(bodys, ", "))
+    }
+}
+--- request
+GET /t
+--- response_body
+1981, 1981, 1981, 1982, 1982, 1982
+--- no_error_log
+[error]
+
+
+
+=== TEST 48: set route(use upstream for route and upstream_id for plugin), 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": "/server_port",
+                    "plugins": {
+                        "traffic-split": {
+                            "rules": [
+                                {
+                                    "match": [
+                                        {
+                                            "vars": [["arg_name", "==", "James"]]
+                                        }
+                                    ],
+                                    "weighted_upstreams": [
+                                        {"upstream_id": 1}
+                                    ]
+                                }
+                            ]
+                        }
+                    },
+                    "upstream": {
+                        "type": "roundrobin",
+                        "nodes": {
+                            "127.0.0.1:1980": 1
+                        }
+                    }
+                }]=]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 49: when `match` rule passed, use the `upstream_id` in plugin, and when it failed, use the `upstream` in route
+--- config
+location /t {
+    content_by_lua_block {
+        local t = require("lib.test_admin").test
+        local bodys = {}
+
+        for i = 1, 5, 2 do
+            -- match rule passed
+            local _, _, body = t('/server_port?name=James', ngx.HTTP_GET)
+            bodys[i] = body
+
+             -- match rule failed
+            local _, _, body = t('/server_port', ngx.HTTP_GET)
+            bodys[i+1] = body
+        end
+
+        table.sort(bodys)
+        ngx.say(table.concat(bodys, ", "))
+    }
+}
+--- request
+GET /t
+--- response_body
+1980, 1980, 1980, 1981, 1981, 1981
 --- no_error_log
 [error]
 
 
 
-=== TEST 48: set route(id: 1, upstream_id: 1, upstream_id in plugin: 2), and `weighted_upstreams` has a structure with only `weight`
+=== TEST 50: 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 {
@@ -1713,7 +1812,7 @@ passed
 
 
 
-=== TEST 49: all requests `match` rule passed, proxy requests to the upstream of route based on the structure with only `weight` in `weighted_upstreams`
+=== TEST 51: 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 {
@@ -1736,7 +1835,7 @@ GET /t
 
 
 
-=== TEST 50: the upstream_id is used in the plugin
+=== TEST 52: the upstream_id is used in the plugin
 --- config
     location /t {
         content_by_lua_block {
@@ -1786,7 +1885,7 @@ passed
 
 
 
-=== TEST 51: `match` rule passed(upstream_id)
+=== TEST 53: `match` rule passed(upstream_id)
 --- config
 location /t {
     content_by_lua_block {
@@ -1810,7 +1909,7 @@ GET /t
 
 
 
-=== TEST 52: only use upstream_id in the plugin
+=== TEST 54: only use upstream_id in the plugin
 --- config
     location /t {
         content_by_lua_block {
@@ -1859,7 +1958,7 @@ passed
 
 
 
-=== TEST 53: `match` rule passed(only use upstream_id)
+=== TEST 55: `match` rule passed(only use upstream_id)
 --- config
 location /t {
     content_by_lua_block {
@@ -1882,7 +1981,7 @@ GET /t
 
 
 
-=== TEST 54: use upstream and upstream_id in the plugin
+=== TEST 56: use upstream and upstream_id in the plugin
 --- config
     location /t {
         content_by_lua_block {
@@ -1932,7 +2031,7 @@ passed
 
 
 
-=== TEST 55: `match` rule passed(upstream + upstream_id)
+=== TEST 57: `match` rule passed(upstream + upstream_id)
 --- config
 location /t {
     content_by_lua_block {
@@ -1957,7 +2056,7 @@ GET /t
 
 
 
-=== TEST 56: set route + upstream (two upstream node: one healthy + one unhealthy)
+=== TEST 58: set route + upstream (two upstream node: one healthy + one unhealthy)
 --- config
     location /t {
         content_by_lua_block {
@@ -2032,7 +2131,7 @@ passed
 
 
 
-=== TEST 57: hit routes, ensure the checker is bound to the upstream
+=== TEST 59: hit routes, ensure the checker is bound to the upstream
 --- config
 location /t {
     content_by_lua_block {
@@ -2086,7 +2185,7 @@ qr/\([^)]+\) unhealthy .* for '.*'/
 
 
 
-=== TEST 58: set upstream(id: 1), by default retries count = number of nodes
+=== TEST 60: set upstream(id: 1), by default retries count = number of nodes
 --- config
     location /t {
         content_by_lua_block {
@@ -2118,7 +2217,7 @@ passed
 
 
 
-=== TEST 59: set route(id: 1, upstream_id: 1)
+=== TEST 61: set route(id: 1, upstream_id: 1)
 --- config
     location /t {
         content_by_lua_block {
@@ -2162,7 +2261,7 @@ passed
 
 
 
-=== TEST 60: hit routes
+=== TEST 62: hit routes
 --- request
 GET /hello
 --- error_code: 502