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(traffic-split): configure multiple "rules", the request will be confused between upstream #4092

Merged
merged 5 commits into from
Apr 22, 2021

Conversation

Firstsawyou
Copy link
Contributor

@Firstsawyou Firstsawyou commented Apr 20, 2021

What this PR does / why we need it:

When the rules array object in the traffic-split plugin has multiple upstream conditions (weighted_upstreams + match), the request will be confused among multiple upstreams. The following example:

curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
    "uri": "/hello",
    "plugins": {
        "traffic-split": {
            "rules": [
                {
                    "match": [
                        {
                            "vars": [
                                ["http_id","==","1"]
                            ]
                        }
                    ],
                    "weighted_upstreams": [
                        {
                            "upstream": {
                                "name": "upstream_A",
                                "type": "roundrobin",
                                "nodes": {
                                    "127.0.0.1:1981":1
                                }
                            },
                            "weight": 3
                        }
                    ]
                },
                {
                    "match": [
                        {
                            "vars": [
                                ["http_id","==","2"]
                            ]
                        }
                    ],
                    "weighted_upstreams": [
                        {
                            "upstream": {
                                "name": "upstream_B",
                                "type": "roundrobin",
                                "nodes": {
                                    "127.0.0.1:1982":1
                                }
                            },
                            "weight": 3
                        }
                    ]
                }
            ]
        }
    },
    "upstream": {
            "type": "roundrobin",
            "nodes": {
                "127.0.0.1:1980": 1
            }
    }
}'

Test :

Match the upstream of the 1981 port and return the 1981 port number:

for i in `seq 1 5`; do curl http://127.0.0.1:9080/hello -H 'id: 1'; done
1981
1981
1981
1981
1981

Matches the upstream of the 1982 port, and is expected to return the 1982 port number, but it actually hits the upstream of the 1981 and 1982 port:

for i in `seq 1 5`; do curl http://127.0.0.1:9080/hello -H 'id: 2'; done
1981
1981
1982
1982
1981

Does not match the upstream of the plugin, the upstream of the default route, returns the 1980 port number:

for i in `seq 1 5`; do curl http://127.0.0.1:9080/hello -H 'id: 4'; done
1980
1980
1980
1980
1980

This PR is used to fix the problem.

Pre-submission checklist:

  • 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
@Firstsawyou Firstsawyou changed the title fix(traffic-split): when there are multiple weighted_upstreams, the request is confused Apr 20, 2021
@Firstsawyou
Copy link
Contributor Author

Related issues: #4090

@Firstsawyou Firstsawyou marked this pull request as ready for review April 21, 2021 04:17
docs/en/latest/plugins/traffic-split.md Outdated Show resolved Hide resolved
docs/en/latest/plugins/traffic-split.md Outdated Show resolved Hide resolved
t/plugin/traffic-split2.t Outdated Show resolved Hide resolved
@tokers
Copy link
Contributor

tokers commented Apr 21, 2021

The PR title is too long, @Firstsawyou could you simplify it?

@Firstsawyou Firstsawyou changed the title fix(traffic-split): when the rules array has multiple upstream conditions (weighted_upstreams + match), the request will be confused among multiple upstreams Apr 21, 2021
@Firstsawyou Firstsawyou changed the title fix(traffic-split): when multiple rules are configured, the request will be confused among multiple upstreams Apr 21, 2021
@Firstsawyou
Copy link
Contributor Author

The PR title is too long, @Firstsawyou could you simplify it?

Ok, updated.

@spacewander spacewander merged commit 9456568 into apache:master Apr 22, 2021
@Firstsawyou Firstsawyou deleted the t-f-lrucache branch April 22, 2021 01:12
@Firstsawyou Firstsawyou restored the t-f-lrucache branch April 22, 2021 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants