Skip to content

fix(traffic-split): configure multiple "rules", the request will be confused between upstream#4092

Merged
spacewander merged 5 commits into
apache:masterfrom
Firstsawyou:t-f-lrucache
Apr 22, 2021
Merged

fix(traffic-split): configure multiple "rules", the request will be confused between upstream#4092
spacewander merged 5 commits into
apache:masterfrom
Firstsawyou:t-f-lrucache

Conversation

@Firstsawyou

@Firstsawyou Firstsawyou commented Apr 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 fix(traffic-split): when the rules array has multiple upstream conditions (weighted_upstreams + match), the request will be confused among multiple upstreams Apr 20, 2021
@Firstsawyou

Firstsawyou commented Apr 21, 2021

Copy link
Copy Markdown
Contributor Author

Related issues: #4090

@Firstsawyou
Firstsawyou marked this pull request as ready for review April 21, 2021 04:17
spacewander
spacewander requested changes Apr 21, 2021
View reviewed changes
Comment thread docs/en/latest/plugins/traffic-split.md Outdated
Comment thread docs/en/latest/plugins/traffic-split.md Outdated
Comment thread t/plugin/traffic-split2.t Outdated
@tokers

tokers commented Apr 21, 2021

Copy link
Copy Markdown
Contributor

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 fix(traffic-split): when multiple rules are configured, 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 fix(traffic-split): configure multiple "rules", the request will be confused between upstream Apr 21, 2021
@Firstsawyou

Firstsawyou commented Apr 21, 2021

Copy link
Copy Markdown
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
@spacewander spacewander mentioned this pull request Aug 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spacewander spacewander spacewander approved these changes
@tokers tokers tokers approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Firstsawyou @tokers @spacewander