Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: the traffic-split plugin is invalid to bind upstream via upstream_id #3758

Merged
merged 8 commits into from
Mar 15, 2021

Conversation

tzssangglass
Copy link
Member

@tzssangglass tzssangglass commented Mar 4, 2021

What this PR does / why we need it:

fix: #3740

Pre-submission checklist:

the cause of the bug reference: #3740 (comment)
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.
when the route is configured with an upstream_id, the upstream_id is passed into the server_list.

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

apisix/plugins/traffic-split.lua Outdated Show resolved Hide resolved
apisix/plugins/traffic-split.lua Outdated Show resolved Hide resolved
@spacewander
Copy link
Member

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.

@tzssangglass
Copy link
Member Author

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

elseif upstream and upstream ~= "plugin#upstream#is#empty" then
ctx.matched_route.value.upstream_id = upstream

@spacewander
Copy link
Member

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.

@tzssangglass
Copy link
Member Author

tzssangglass commented Mar 9, 2021

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.

@tokers tokers merged commit 210ca46 into apache:master Mar 15, 2021
@tzssangglass tzssangglass deleted the IssueNo3740 branch October 26, 2021 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: the traffic-split plugin is invalid to bind upstream via upstream_id
5 participants