change(limit-count): ensure redis cluster name is set correctly by han6565 · Pull Request #3910 · apache/apisix

han6565 · 2021-03-25T12:39:09Z

…ess test prompting ' ttl: ERR invalid passwor'

What this PR does / why we need it:

during the stress test, I found that if the limit-count plugin configures multiple different redis-cluster data sources, the following error will appear failed to get redis plugin-limit-countxx.xx.xx.xxroute122 ttl: ERR invalid password so I Fixed it

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

…ess test prompting ' ttl: ERR invalid passwor'

tokers · 2021-03-26T02:21:48Z

Could you please add some test cases to cover the scene that you said?

spacewander · 2021-03-26T02:47:46Z

 local function new_redis_cluster(conf)
    local config = {
-        name = "apisix-redis-cluster",
+        name = "apisix-redis-cluster-" .. conf.name,


I am confused by the solution. What is the relation between the password and name?

one name Mapping one routes，if all redis_cluster config the same name. When concurrent lrucache can not find the correct redis_cluster in pool.
This is my personal opinion ，maybe it is wrong.😄

I read through the resty.rediscluster. Look like this library uses a module-level slot_cache, and the key is the config.name.

So it is no doubted that different configurations with the same name will collide.

~~What about using "apisix-redis-cluster-" .. tostring(conf) directly?~~
Better to add a comment about this change as future maintainers may not read through the library.

~~Different routes using the same cluster will still need to share the slot_cache. Would it be better to use crc32(nodes) to generate the suffix?~~

Sorry for my fickle mind. It seems the library doesn't provide a way to clean up stale cached slots. So using auto-generated name is unsafe. My latest idea is to require user to specify the name of redis cluster. It is a break change but it is reasonable to introduce break change to fix bug.

han6565 · 2021-03-26T03:17:22Z

Could you please add some test cases to cover the scene that you said?

ok,I can paste some of my test configuration and test results
in etcd one route like this:
{
"id": "347382218030582023",
"create_time": 1616585537,
"update_time": 1616590678,
"uris": [
"/test"
],
"name": "limit",
"methods": [
"GET",
"HEAD",
"POST",
"PUT",
"DELETE",
"OPTIONS",
"PATCH"
],
"plugins": {
"limit-count": {
"count": 1000,
"time_window": 1,
"rejected_code": 503,
"policy": "redis-cluster",
"redis_cluster_nodes": [
"cluster1-node1:6379",
"cluster1-node2:6379",
"cluster1-node3:6379"
],
"redis_password": "cluster1-password"
}
},
"upstream": {
"nodes": [
{
"host": "myip",
"port": 9999,
"weight": 1
}
],
"timeout": {
"connect": 6000,
"read": 6000,
"send": 6000
},
"type": "roundrobin",
"pass_host": "pass"
},
"status": 1
}
other route like this:
{
"id": "347382218030582024",
"create_time": 1616585538,
"update_time": 1616590679,
"uris": [
"/test2"
],
"name": "limit2",
"methods": [
"GET",
"HEAD",
"POST",
"PUT",
"DELETE",
"OPTIONS",
"PATCH"
],
"plugins": {
"limit-count": {
"count": 1000,
"time_window": 1,
"rejected_code": 503,
"policy": "redis-cluster",
"redis_cluster_nodes": [
"cluster2-node1:6379",
"cluster2-node2:6379",
"cluster2-node3:6379"
],
"redis_password": "cluster2-password"
}
},
"upstream": {
"nodes": [
{
"host": "myip",
"port": 9999,
"weight": 1
}
],
"timeout": {
"connect": 6000,
"read": 6000,
"send": 6000
},
"type": "roundrobin",
"pass_host": "pass"
},
"status": 1
}

then I stress test two routes at the same time，i use the go-stress-testing test the url like this:
./go-stress-testing-mac -c 20 -n 100000 -u http://apisix_ip:9080/test
./go-stress-testing-mac -c 20 -n 100000 -u http://apisix_ip:9080/test2

you can see my error.log it output errror like this:
2021/03/26 11:07:03 [error] 28109#28109: *6443 [lua] limit-count.lua:174: phase_func(): failed to limit req: failed to get redis plugin-limit-count10.73.42.1route122 ttl: ERR invalid password, client: 10.73.42.1, server: , request: "GET /test HTTP/1.1", host: "172.20.4.213:9080"
but the password is correct。
I will upload the error log
error.log

spacewander · 2021-03-26T07:20:38Z

 local function new_redis_cluster(conf)
    local config = {
-        name = "apisix-redis-cluster",
+        name = "apisix-redis-cluster-" .. conf.name,


I read through the resty.rediscluster. Look like this library uses a module-level slot_cache, and the key is the config.name.

So it is no doubted that different configurations with the same name will collide.

~~What about using "apisix-redis-cluster-" .. tostring(conf) directly?~~
Better to add a comment about this change as future maintainers may not read through the library.

~~Different routes using the same cluster will still need to share the slot_cache. Would it be better to use crc32(nodes) to generate the suffix?~~

Sorry for my fickle mind. It seems the library doesn't provide a way to clean up stale cached slots. So using auto-generated name is unsafe. My latest idea is to require user to specify the name of redis cluster. It is a break change but it is reasonable to introduce break change to fix bug.

Co-authored-by: Yuelin Zheng <[email protected]>

spacewander · 2021-03-30T11:12:18Z

Don't forget to update the example: https://github.com/apache/apisix/blob/master/docs/en/latest/plugins/limit-count.md

"plugins": {
        "limit-count": {
            "count": 2,
            "time_window": 60,
            "rejected_code": 503,
            "key": "remote_addr",
            "policy": "redis-cluster",
            "redis_cluster_nodes": [
              "127.0.0.1:5000",
              "127.0.0.1:5001"
            ]
        }
    },

fix the problem of multiple different redis-cluster data sources, str…

85407f7

…ess test prompting ' ttl: ERR invalid passwor'

han6565 changed the title ~~fix the problem of multiple different redis-cluster data sources, str…~~ fix: limit-count plugin connect error Mar 25, 2021

spacewander reviewed Mar 26, 2021

View reviewed changes

spacewander requested changes Mar 26, 2021

View reviewed changes

set redis cluster name by etcd config

9cc3be1

spacewander requested changes Mar 30, 2021

View reviewed changes

Comment thread apisix/plugins/limit-count/limit-count-redis-cluster.lua Outdated

Comment thread apisix/plugins/limit-count.lua

Comment thread apisix/plugins/limit-count.lua Outdated

hanjinqi added 2 commits March 30, 2021 11:05

set redis cluster name by etcd config

f7a5436

redis_cluster_name is required parameter

3c1035f

spacewander reviewed Mar 30, 2021

View reviewed changes

Comment thread docs/en/latest/plugins/limit-count.md Outdated

Firstsawyou suggested changes Mar 30, 2021

View reviewed changes

Comment thread apisix/plugins/limit-count.lua Outdated

Comment thread docs/en/latest/plugins/limit-count.md Outdated

han6565 and others added 3 commits March 30, 2021 16:37

fix bad style

bab152b

Co-authored-by: Yuelin Zheng <[email protected]>

fix English grammar issues

c554a1b

Co-authored-by: Yuelin Zheng <[email protected]>

test for redis_cluster_name

499d6fa

update the example

257d1de

spacewander reviewed Mar 31, 2021

View reviewed changes

Comment thread docs/en/latest/plugins/limit-count.md Outdated

Comment thread docs/zh/latest/plugins/limit-count.md Outdated

hanjinqi added 4 commits March 31, 2021 11:20

Merge remote-tracking branch 'upstream/master'

b5ce549

Merge remote-tracking branch 'upstream/master'

0aa6ed6

update the doc

0157968

update the doc

338d2c4

spacewander changed the title ~~fix: limit-count plugin connect error~~ change(limit-count): ensure redis cluster name is set correctly Apr 2, 2021

spacewander approved these changes Apr 2, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master'

c77b409

Firstsawyou approved these changes Apr 2, 2021

View reviewed changes

tokers reviewed Apr 3, 2021

View reviewed changes

Comment thread docs/en/latest/plugins/limit-count.md Outdated

Comment thread docs/zh/latest/plugins/limit-count.md Outdated

hanjinqi added 2 commits April 6, 2021 10:21

Merge remote-tracking branch 'upstream/master'

33b65a0

update the doc

eb88001

tokers approved these changes Apr 7, 2021

View reviewed changes

spacewander merged commit 4156a73 into apache:master Apr 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

change(limit-count): ensure redis cluster name is set correctly#3910

change(limit-count): ensure redis cluster name is set correctly#3910
spacewander merged 15 commits into
apache:masterfrom
han6565:master

han6565 commented Mar 25, 2021 •

edited

Loading

Uh oh!

tokers commented Mar 26, 2021

Uh oh!

spacewander Mar 26, 2021

Uh oh!

han6565 Mar 26, 2021

Uh oh!

spacewander Mar 26, 2021 •

edited

Loading

Uh oh!

han6565 commented Mar 26, 2021

Uh oh!

spacewander Mar 26, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spacewander commented Mar 30, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

han6565 commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

tokers commented Mar 26, 2021

Uh oh!

spacewander Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

han6565 Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

spacewander Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

han6565 commented Mar 26, 2021

Uh oh!

spacewander Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spacewander commented Mar 30, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

han6565 commented Mar 25, 2021 •

edited

Loading

spacewander Mar 26, 2021 •

edited

Loading

spacewander Mar 26, 2021 •

edited

Loading