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

[han6565]

…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

[tokers]

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

[spacewander]

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

[han6565]

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.😄

[spacewander]

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]

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]

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.

[spacewander]

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"
            ]
        }
    },