You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/09/28 14:02:06 UTC
[GitHub] [apisix] liuhengloveyou opened a new pull request #2340: feature: limit-count use redis cluster
liuhengloveyou opened a new pull request #2340:
URL: https://github.com/apache/apisix/pull/2340
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] juzhiyuan commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502846095
##########
File path: doc/plugins/limit-count.md
##########
@@ -39,11 +39,12 @@ Limit request rate by a fixed number of requests in a given time window.
| time_window | integer | required | | [0,...] | the time window in seconds before the request count is reset. |
| key | string | required | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | the user specified key to limit the rate. |
| rejected_code | integer | optional | 503 | [200,600] | The HTTP status code returned when the request exceeds the threshold is rejected, default 503. |
-| policy | string | optional | "local" | ["local", "redis"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
+| policy | string | optional | "local" | ["local", "redis", "redis-cluster"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
Review comment:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503152106
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,130 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+ __index = _M
+}
+
+
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-redis-cluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, conf_item in ipairs(conf.redis_cluster_nodes) do
+ local host, port, err = core.utils.parse_addr(conf_item)
+ if err then
+ error("limit-count-redis: redis_cluster_nodes configure err " .. conf_item, err)
+ end
+
+ config.serv_list[i] = {ip = host, port = port}
+ end
+
+ local red_cli, err = rediscluster:new(config)
+ if not red_cli then
+ error("limit-count-redis: connect to redis cluster failed: ", err)
+ end
+
+ return red_cli
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+ assert(limit > 0 and window > 0)
+
+ local red_cli = new_redis_cluster(conf)
+
+ local self = {limit = limit, window = window, conf = conf,
+ plugin_name = plugin_name, red_cli =red_cli}
+
+ return setmetatable(self, mt)
+end
+
+
+function _M.incoming(self, key)
Review comment:
Nowhere to call this function?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502743834
##########
File path: doc/zh-cn/plugins/limit-count.md
##########
@@ -26,17 +26,18 @@
## 参数
-| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
-| -------------- | ------- | ------------ | ------- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
-| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
-| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
-| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
-| policy | string | 可选 | "local" | ["local", "redis"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
-| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
-| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
-| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| --------------- | -------- | ------------ | ------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
+| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
+| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
+| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
+| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
+| policy | string | 可选 | "local" | ["local", "redis", "redis-cluster"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速)；以及`redis-cluster`，跟redis功能一样，只是使用redis集群方式。 |
Review comment:
ping @liuhengloveyou
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ read_timeout = conf.redis_timeout or 1000,
+ enable_slave_read = true,
+ serv_list = {},
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+ if conf.redis_password then
+ config.auth = conf.redis_password --set password while setting auth
+ end
+
+ local redis_cluster = require "resty.rediscluster"
Review comment:
ping @liuhengloveyou
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503155878
##########
File path: t/APISIX.pm
##########
@@ -236,6 +236,7 @@ _EOC_
lua_shared_dict balancer_ewma 1m;
lua_shared_dict balancer_ewma_locks 1m;
lua_shared_dict balancer_ewma_last_touched_at 1m;
+ lua_shared_dict redis_cluster_slot_locks 100k;
Review comment:
where to use it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502804881
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -70,11 +74,29 @@ local schema = {
type = "string", minLength = 0,
},
redis_timeout = {
- type = "integer", minimum = 1,
- default = 1000,
+ type = "integer", minimum = 1, default = 1000,
},
},
required = {"redis_host"},
+ },
+ {
+ properties = {
+ policy = {
+ enum = {"redis-cluster"},
+ },
+ redis_cluster_nodes = {
+ type = "array",
+ minItems = 2,
+ items = {type = "string", minLength = 2, maxLength = 100},
Review comment:
updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502787623
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
Review comment:
this version is useless, we can remove it
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -119,6 +148,11 @@ local function create_limit_obj(conf)
conf.count, conf.time_window, conf)
end
+ if conf.policy == "redis-cluster" then
+ return limit_redis_cluster_new("plugin-" .. plugin_name,conf.count,
Review comment:
style: need a space between different argument items
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, c in ipairs(conf.redis_serv_list) do
+ config.serv_list[i] = {ip = c.host, port = c.port}
+ end
+
+ local red_cli = rediscluster:new(config)
+ if not red_cli then
+ error("connect to redis cluster fails")
Review comment:
Please confirm if we can capture more error messages
##########
File path: doc/zh-cn/plugins/limit-count.md
##########
@@ -26,17 +26,18 @@
## 参数
-| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
-| -------------- | ------- | ------------ | ------- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
-| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
-| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
-| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
-| policy | string | 可选 | "local" | ["local", "redis"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
-| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
-| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
-| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| --------------- | -------- | ------------ | ------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
+| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
+| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
+| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
+| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
+| policy | string | 可选 | "local" | ["local", "redis", "redis-cluster"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速)；以及`redis-cluster`，跟redis功能一样，只是使用redis集群方式。 |
+| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
+| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
+| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
+| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| redis_serv_list | array<object>[] | 可选 | | | 当使用 `redis-cluster` 限速策略时，该属性是 Redis 集群服务节点的地址列表。 |
Review comment:
we need to add an example for the field `redis_serv_list`
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, c in ipairs(conf.redis_serv_list) do
Review comment:
`c` is a bad name, please change a better one
##########
File path: doc/zh-cn/plugins/limit-count.md
##########
@@ -26,17 +26,18 @@
## 参数
-| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
-| -------------- | ------- | ------------ | ------- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
-| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
-| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
-| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
-| policy | string | 可选 | "local" | ["local", "redis"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
-| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
-| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
-| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| --------------- | -------- | ------------ | ------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
+| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
+| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
+| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
+| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
+| policy | string | 可选 | "local" | ["local", "redis", "redis-cluster"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速)；以及`redis-cluster`，跟redis功能一样，只是使用redis集群方式。 |
Review comment:
ok, got it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502789195
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, c in ipairs(conf.redis_serv_list) do
+ config.serv_list[i] = {ip = c.host, port = c.port}
+ end
+
+ local red_cli = rediscluster:new(config)
+ if not red_cli then
+ error("connect to redis cluster fails")
Review comment:
updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-706539087
> @liuhengloveyou is this PR still a draft?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] juzhiyuan commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502799124
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -70,11 +74,29 @@ local schema = {
type = "string", minLength = 0,
},
redis_timeout = {
- type = "integer", minimum = 1,
- default = 1000,
+ type = "integer", minimum = 1, default = 1000,
},
},
required = {"redis_host"},
+ },
+ {
+ properties = {
+ policy = {
+ enum = {"redis-cluster"},
+ },
+ redis_cluster_nodes = {
+ type = "array",
+ minItems = 2,
+ items = {type = "string", minLength = 2, maxLength = 100},
Review comment:
Looks like we need format those codes to have a uniform code style?
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -70,11 +74,29 @@ local schema = {
type = "string", minLength = 0,
},
redis_timeout = {
- type = "integer", minimum = 1,
- default = 1000,
+ type = "integer", minimum = 1, default = 1000,
},
},
required = {"redis_host"},
+ },
+ {
+ properties = {
+ policy = {
+ enum = {"redis-cluster"},
Review comment:
@liuhengloveyou Because this plugin would be shown by UI components on the frontend, could you please have a test on this site[1] using this new schema? or could you please provide the full JSON Schema for this plugin then paste here? I could have a try myself.
[1] https://rjsf-team.github.io/react-jsonschema-form/
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+ __index = _M
+}
+
+
+local function split(inputstr, sep)
+ if sep == nil then
+ sep = "%s"
+ end
+
+ local t={}
+ for str in string.gmatch(inputstr, "([^"..sep.."]+)") do
+ table.insert(t, str)
+ end
+
+ return t
+ end
+
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, conf_item in ipairs(conf.redis_cluster_nodes) do
+ local ip, port = split(conf_item, ":")
+ config.serv_list[i] = {ip = ip, port = port}
+ end
+
+ local red_cli = rediscluster:new(config)
+ if not red_cli then
+ error("limit-count-redis: connect to redis cluster fails")
Review comment:
failed
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+ __index = _M
+}
+
+
+local function split(inputstr, sep)
+ if sep == nil then
+ sep = "%s"
+ end
+
+ local t={}
+ for str in string.gmatch(inputstr, "([^"..sep.."]+)") do
+ table.insert(t, str)
+ end
+
+ return t
+ end
+
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
Review comment:
Should we use a uniform name?
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+ __index = _M
+}
+
+
+local function split(inputstr, sep)
+ if sep == nil then
+ sep = "%s"
+ end
+
+ local t={}
Review comment:
Would better add 2 spaces around the `=`.
##########
File path: doc/plugins/limit-count.md
##########
@@ -39,11 +39,12 @@ Limit request rate by a fixed number of requests in a given time window.
| time_window | integer | required | | [0,...] | the time window in seconds before the request count is reset. |
| key | string | required | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | the user specified key to limit the rate. |
| rejected_code | integer | optional | 503 | [200,600] | The HTTP status code returned when the request exceeds the threshold is rejected, default 503. |
-| policy | string | optional | "local" | ["local", "redis"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
+| policy | string | optional | "local" | ["local", "redis", "redis-cluster"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
Review comment:
usually **use** it to xxx
##########
File path: doc/plugins/limit-count.md
##########
@@ -39,11 +39,12 @@ Limit request rate by a fixed number of requests in a given time window.
| time_window | integer | required | | [0,...] | the time window in seconds before the request count is reset. |
| key | string | required | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | the user specified key to limit the rate. |
| rejected_code | integer | optional | 503 | [200,600] | The HTTP status code returned when the request exceeds the threshold is rejected, default 503. |
-| policy | string | optional | "local" | ["local", "redis"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
+| policy | string | optional | "local" | ["local", "redis", "redis-cluster"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
| redis_host | string | required for `redis` | | | When using the `redis` policy, this property specifies the address of the Redis server. |
| redis_port | integer | optional | 6379 | [1,...] | When using the `redis` policy, this property specifies the port of the Redis server. |
| redis_password | string | optional | | | When using the `redis` policy, this property specifies the password of the Redis server. |
| redis_timeout | integer | optional | 1000 | [1,...] | When using the `redis` policy, this property specifies the timeout in milliseconds of any command submitted to the Redis server. |
+| redis_serv_list | array | optional | | | When using `redis-cluster` policy，This property is a list of addresses of Redis cluster service nodes. |
Review comment:
Just have a try on the `Grammarly` extension (Chrome).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502807891
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, c in ipairs(conf.redis_serv_list) do
+ config.serv_list[i] = {ip = c.host, port = c.port}
+ end
+
+ local red_cli = rediscluster:new(config)
+ if not red_cli then
+ error("connect to redis cluster fails")
Review comment:
updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503015456
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, c in ipairs(conf.redis_serv_list) do
Review comment:
updated to conf_item
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r501548890
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -132,7 +132,9 @@ function _M.check_schema(conf)
end
if conf.policy == "redis-cluster" then
- -- TODO
+ if not conf.redis_serv_list then
+ return false, "missing valid redis option serv_list"
Review comment:
that is a wrong way, we can use JSONSchema to check this
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -132,7 +132,9 @@ function _M.check_schema(conf)
end
if conf.policy == "redis-cluster" then
- -- TODO
+ if not conf.redis_serv_list then
+ return false, "missing valid redis option serv_list"
Review comment:
we can use a better way, use JSONSchema to check this
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,138 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ read_timeout = conf.redis_timeout or 1000,
+ enable_slave_read = true,
+ serv_list = {},
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+ if conf.redis_password then
+ config.auth = conf.redis_password --set password while setting auth
+ end
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
+
+ return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+ assert(limit > 0 and window > 0)
+
+ _M.red_c = new_redis_cluster(conf)
+
+ local self = {limit = limit, window = window, conf = conf,
+ plugin_name = plugin_name}
+ return setmetatable(self, mt)
+
+end
+
+
+function _M.incoming(self, key)
+ local conf = self.conf
+ local red = _M.red_c
+ core.log.info("ttl key: ", key, " timeout: ", conf.redis_timeout or 1000)
Review comment:
use JSONSchema to set the default value
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,138 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ read_timeout = conf.redis_timeout or 1000,
+ enable_slave_read = true,
+ serv_list = {},
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+ if conf.redis_password then
+ config.auth = conf.redis_password --set password while setting auth
+ end
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
+
+ return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+ assert(limit > 0 and window > 0)
+
+ _M.red_c = new_redis_cluster(conf)
+
+ local self = {limit = limit, window = window, conf = conf,
+ plugin_name = plugin_name}
+ return setmetatable(self, mt)
+
+end
+
+
+function _M.incoming(self, key)
+ local conf = self.conf
+ local red = _M.red_c
+ core.log.info("ttl key: ", key, " timeout: ", conf.redis_timeout or 1000)
+
+ ngx.log(ngx.ERR, ">>>>>>>>>>>>>>", key, require("cjson.safe").encode(self))
Review comment:
should use `core.log` always.
##########
File path: doc/zh-cn/plugins/limit-count.md
##########
@@ -26,17 +26,18 @@
## 参数
-| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
-| -------------- | ------- | ------------ | ------- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
-| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
-| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
-| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
-| policy | string | 可选 | "local" | ["local", "redis"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
-| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
-| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
-| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| --------------- | -------- | ------------ | ------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
+| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
+| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
+| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
+| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
+| policy | string | 可选 | "local" | ["local", "redis", "redis-cluster"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速)；以及`redis-cluster`，跟redis功能一样，只是使用redis集群方式。 |
Review comment:
we should only change the doc we need to update. small PR is easy for reviewing.
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ read_timeout = conf.redis_timeout or 1000,
+ enable_slave_read = true,
+ serv_list = {},
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+ if conf.redis_password then
+ config.auth = conf.redis_password --set password while setting auth
+ end
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
Review comment:
I think it may fail, need to capture the error message
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
Review comment:
can we use the default value here?
##########
File path: doc/plugins/limit-count.md
##########
@@ -109,6 +110,41 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
}'
```
+If using redis-cluster:
+
+```shell
+curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "uri": "/index.html",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_serv_list": [
+ {
Review comment:
how about this style? it seems simpler:
```
redis_serv_list: ["127.0.0.1:6373", "127.0.0.1:6374"]
```
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ read_timeout = conf.redis_timeout or 1000,
+ enable_slave_read = true,
+ serv_list = {},
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+ if conf.redis_password then
+ config.auth = conf.redis_password --set password while setting auth
+ end
+
+ local redis_cluster = require "resty.rediscluster"
Review comment:
local cache
##########
File path: doc/plugins/limit-count.md
##########
@@ -109,6 +110,41 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
}'
```
+If using redis-cluster:
+
+```shell
+curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "uri": "/index.html",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_serv_list": [
+ {
Review comment:
avoid using the `tab` key, we always use spaces.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502804539
##########
File path: doc/zh-cn/plugins/limit-count.md
##########
@@ -26,17 +26,18 @@
## 参数
-| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
-| -------------- | ------- | ------------ | ------- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
-| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
-| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
-| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
-| policy | string | 可选 | "local" | ["local", "redis"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
-| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
-| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
-| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| --------------- | -------- | ------------ | ------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
+| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
+| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
+| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
+| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
Review comment:
Should be formatted by the editor, the content has not changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502804384
##########
File path: doc/plugins/limit-count.md
##########
@@ -39,11 +39,12 @@ Limit request rate by a fixed number of requests in a given time window.
| time_window | integer | required | | [0,...] | the time window in seconds before the request count is reset. |
| key | string | required | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | the user specified key to limit the rate. |
| rejected_code | integer | optional | 503 | [200,600] | The HTTP status code returned when the request exceeds the threshold is rejected, default 503. |
-| policy | string | optional | "local" | ["local", "redis"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
+| policy | string | optional | "local" | ["local", "redis", "redis-cluster"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
| redis_host | string | required for `redis` | | | When using the `redis` policy, this property specifies the address of the Redis server. |
| redis_port | integer | optional | 6379 | [1,...] | When using the `redis` policy, this property specifies the port of the Redis server. |
| redis_password | string | optional | | | When using the `redis` policy, this property specifies the password of the Redis server. |
| redis_timeout | integer | optional | 1000 | [1,...] | When using the `redis` policy, this property specifies the timeout in milliseconds of any command submitted to the Redis server. |
+| redis_serv_list | array | optional | | | When using `redis-cluster` policy，This property is a list of addresses of Redis cluster service nodes. |
Review comment:
thand you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503159420
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,130 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+ __index = _M
+}
+
+
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-redis-cluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, conf_item in ipairs(conf.redis_cluster_nodes) do
+ local host, port, err = core.utils.parse_addr(conf_item)
+ if err then
+ error("limit-count-redis: redis_cluster_nodes configure err " .. conf_item, err)
+ end
+
+ config.serv_list[i] = {ip = host, port = port}
+ end
+
+ local red_cli, err = rediscluster:new(config)
+ if not red_cli then
+ error("limit-count-redis: connect to redis cluster failed: ", err)
+ end
+
+ return red_cli
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+ assert(limit > 0 and window > 0)
+
+ local red_cli = new_redis_cluster(conf)
+
+ local self = {limit = limit, window = window, conf = conf,
+ plugin_name = plugin_name, red_cli =red_cli}
+
+ return setmetatable(self, mt)
+end
+
+
+function _M.incoming(self, key)
Review comment:
<img width="652" alt="WX20201012-172254@2x" src="https://user-images.githubusercontent.com/5400196/95729618-b5923480-0caf-11eb-9d77-7fb10742891a.png">
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502994334
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, c in ipairs(conf.redis_serv_list) do
Review comment:
updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503160705
##########
File path: t/APISIX.pm
##########
@@ -236,6 +236,7 @@ _EOC_
lua_shared_dict balancer_ewma 1m;
lua_shared_dict balancer_ewma_locks 1m;
lua_shared_dict balancer_ewma_last_touched_at 1m;
+ lua_shared_dict redis_cluster_slot_locks 100k;
Review comment:
Use by resty-redis-cluster library
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-703211444
@liuhengloveyou is this PR still a draft?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503707079
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,130 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+ __index = _M
+}
+
+
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-redis-cluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, conf_item in ipairs(conf.redis_cluster_nodes) do
+ local host, port, err = core.utils.parse_addr(conf_item)
+ if err then
+ error("limit-count-redis: redis_cluster_nodes configure err " .. conf_item, err)
Review comment:
I this is wrong
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,130 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+ __index = _M
+}
+
+
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-redis-cluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, conf_item in ipairs(conf.redis_cluster_nodes) do
+ local host, port, err = core.utils.parse_addr(conf_item)
+ if err then
+ error("limit-count-redis: redis_cluster_nodes configure err " .. conf_item, err)
+ end
+
+ config.serv_list[i] = {ip = host, port = port}
+ end
+
+ local red_cli, err = rediscluster:new(config)
+ if not red_cli then
+ error("limit-count-redis: connect to redis cluster failed: ", err)
Review comment:
ditto
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502721349
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
Review comment:
ping @liuhengloveyou
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503373211
##########
File path: bin/apisix
##########
@@ -190,6 +190,7 @@ http {
lua_shared_dict balancer_ewma 10m;
lua_shared_dict balancer_ewma_locks 10m;
lua_shared_dict balancer_ewma_last_touched_at 10m;
+ lua_shared_dict redis_cluster_slot_locks 100k;
Review comment:
that is not a good plugin name, we should add a prefix about the plugin name
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis closed pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis closed pull request #2340:
URL: https://github.com/apache/apisix/pull/2340
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r501548890
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -132,7 +132,9 @@ function _M.check_schema(conf)
end
if conf.policy == "redis-cluster" then
- -- TODO
+ if not conf.redis_serv_list then
+ return false, "missing valid redis option serv_list"
Review comment:
that is a wrong way, we can use JSONSchema to check this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-707031088
The limit-count plugin used to support REIDS, the new Redis-Cluster has the same functional logic except that redis connections are differently.
Now added new code, in order not to change the old code, copy part of the functional logic code. It's not pretty.
We need to come up with a solution to refactor the old code and reuse the common parts. @moonming @membphis
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502730066
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
Review comment:
I deleted the dict_name configuration and tested it with no problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-706541484
> please fix the old issue first
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou removed a comment on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou removed a comment on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-706539087
> @liuhengloveyou is this PR still a draft?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502807866
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -70,11 +74,36 @@ local schema = {
type = "string", minLength = 0,
},
redis_timeout = {
- type = "integer", minimum = 1,
- default = 1000,
+ type = "integer", minimum = 1, default = 1000,
},
},
required = {"redis_host"},
+ },
+ {
+ properties = {
+ policy = {
+ enum = {"redis-cluster"},
+ },
+ redis_serv_list = {
Review comment:
updated to redis_cluster_nodes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-706828797
> @liuhengloveyou no test case?
added
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503707079
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,130 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+ __index = _M
+}
+
+
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-redis-cluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, conf_item in ipairs(conf.redis_cluster_nodes) do
+ local host, port, err = core.utils.parse_addr(conf_item)
+ if err then
+ error("limit-count-redis: redis_cluster_nodes configure err " .. conf_item, err)
Review comment:
I think this is wrong
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou closed pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou closed pull request #2340:
URL: https://github.com/apache/apisix/pull/2340
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502993940
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -70,11 +74,29 @@ local schema = {
type = "string", minLength = 0,
},
redis_timeout = {
- type = "integer", minimum = 1,
- default = 1000,
+ type = "integer", minimum = 1, default = 1000,
},
},
required = {"redis_host"},
+ },
+ {
+ properties = {
+ policy = {
+ enum = {"redis-cluster"},
Review comment:
I tested it with the URL you provided, and it seems to be working.
```
{
"required":[
"count",
"time_window",
"key"
],
"properties":{
"time_window":{
"minimum":0,
"type":"integer"
},
"count":{
"minimum":0,
"type":"integer"
},
"rejected_code":{
"maximum":600,
"type":"integer",
"default":503,
"minimum":200
},
"key":{
"enum":[
"remote_addr",
"server_addr",
"http_x_real_ip",
"http_x_forwarded_for"
],
"type":"string"
},
"policy":{
"type":"string",
"default":"local",
"enum":[
"local",
"redis",
"redis-cluster"
]
}
},
"dependencies":{
"policy":{
"oneOf":[
{
"properties":{
"policy":{
"enum":[
"local"
]
}
}
},
{
"required":[
"redis_host"
],
"properties":{
"policy":{
"enum":[
"redis"
]
},
"redis_host":{
"minLength":2,
"type":"string"
},
"redis_timeout":{
"type":"integer",
"default":1000,
"minimum":1
},
"redis_password":{
"minLength":0,
"type":"string"
},
"redis_port":{
"type":"integer",
"default":6379,
"minimum":1
}
}
},
{
"required":[
"redis_cluster_nodes"
],
"properties":{
"redis_timeout":{
"type":"integer",
"default":1000,
"minimum":1
},
"redis_cluster_nodes":{
"items":{
"minLength":2,
"maxLength":100,
"type":"string"
},
"type":"array",
"minItems":2
},
"policy":{
"enum":[
"redis-cluster"
]
},
"redis_password":{
"minLength":0,
"type":"string"
}
}
}
]
}
},
"type":"object"
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-705425077
pls fix the code style first: https://github.com/apache/apisix/runs/1224890754#step:6:117
```
Checking apisix/plugins/limit-count/limit-count-redis-cluster.lua 6 warnings
apisix/plugins/limit-count/limit-count-redis-cluster.lua:55:1: line contains only whitespace
apisix/plugins/limit-count/limit-count-redis-cluster.lua:58:1: line contains only whitespace
apisix/plugins/limit-count/limit-count-redis-cluster.lua:113:13: setting non-standard global variable ok
apisix/plugins/limit-count/limit-count-redis-cluster.lua:114:20: accessing undefined variable ok
apisix/plugins/limit-count/limit-count-redis-cluster.lua:126:9: setting non-standard global variable ok
apisix/plugins/limit-count/limit-count-redis-cluster.lua:127:16: accessing undefined variable ok
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-707193555
When one or more Redis is abnormal, test cases need to be added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502279935
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ read_timeout = conf.redis_timeout or 1000,
+ enable_slave_read = true,
+ serv_list = {},
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+ if conf.redis_password then
+ config.auth = conf.redis_password --set password while setting auth
+ end
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
Review comment:
I think it may fail, need to capture the error message
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
Review comment:
can we use the default value here?
##########
File path: doc/plugins/limit-count.md
##########
@@ -109,6 +110,41 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
}'
```
+If using redis-cluster:
+
+```shell
+curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "uri": "/index.html",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_serv_list": [
+ {
Review comment:
how about this style? it seems simpler:
```
redis_serv_list: ["127.0.0.1:6373", "127.0.0.1:6374"]
```
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ read_timeout = conf.redis_timeout or 1000,
+ enable_slave_read = true,
+ serv_list = {},
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+ if conf.redis_password then
+ config.auth = conf.redis_password --set password while setting auth
+ end
+
+ local redis_cluster = require "resty.rediscluster"
Review comment:
local cache
##########
File path: doc/plugins/limit-count.md
##########
@@ -109,6 +110,41 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
}'
```
+If using redis-cluster:
+
+```shell
+curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "uri": "/index.html",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_serv_list": [
+ {
Review comment:
avoid using the `tab` key, we always use spaces.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502789923
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -70,11 +74,36 @@ local schema = {
type = "string", minLength = 0,
},
redis_timeout = {
- type = "integer", minimum = 1,
- default = 1000,
+ type = "integer", minimum = 1, default = 1000,
},
},
required = {"redis_host"},
+ },
+ {
+ properties = {
+ policy = {
+ enum = {"redis-cluster"},
+ },
+ redis_serv_list = {
Review comment:
`redis_serv_list` is not a good name, how about `nodes`?
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -119,6 +148,11 @@ local function create_limit_obj(conf)
conf.count, conf.time_window, conf)
end
+ if conf.policy == "redis-cluster" then
+ return limit_redis_cluster_new("plugin-" .. plugin_name,conf.count,
Review comment:
Add a space after `,`
##########
File path: doc/zh-cn/plugins/limit-count.md
##########
@@ -26,17 +26,18 @@
## 参数
-| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
-| -------------- | ------- | ------------ | ------- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
-| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
-| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
-| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
-| policy | string | 可选 | "local" | ["local", "redis"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
-| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
-| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
-| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| --------------- | -------- | ------------ | ------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
+| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
+| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
+| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
+| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
Review comment:
why change those lines?
##########
File path: doc/zh-cn/plugins/limit-count.md
##########
@@ -26,17 +26,18 @@
## 参数
-| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
-| -------------- | ------- | ------------ | ------- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
-| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
-| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
-| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
-| policy | string | 可选 | "local" | ["local", "redis"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
-| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
-| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
-| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| --------------- | -------- | ------------ | ------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
+| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
+| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
+| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
+| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
+| policy | string | 可选 | "local" | ["local", "redis", "redis-cluster"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速)；以及`redis-cluster`，跟redis功能一样，只是使用redis集群方式。 |
+| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
+| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
+| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
+| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
Review comment:
ditto
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
Review comment:
why add this comment?
##########
File path: doc/plugins/limit-count.md
##########
@@ -39,11 +39,12 @@ Limit request rate by a fixed number of requests in a given time window.
| time_window | integer | required | | [0,...] | the time window in seconds before the request count is reset. |
| key | string | required | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | the user specified key to limit the rate. |
| rejected_code | integer | optional | 503 | [200,600] | The HTTP status code returned when the request exceeds the threshold is rejected, default 503. |
-| policy | string | optional | "local" | ["local", "redis"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
+| policy | string | optional | "local" | ["local", "redis", "redis-cluster"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
| redis_host | string | required for `redis` | | | When using the `redis` policy, this property specifies the address of the Redis server. |
| redis_port | integer | optional | 6379 | [1,...] | When using the `redis` policy, this property specifies the port of the Redis server. |
| redis_password | string | optional | | | When using the `redis` policy, this property specifies the password of the Redis server. |
| redis_timeout | integer | optional | 1000 | [1,...] | When using the `redis` policy, this property specifies the timeout in milliseconds of any command submitted to the Redis server. |
+| redis_serv_list | array | optional | | | When using `redis-cluster` policy，This property is a list of addresses of Redis cluster service nodes. |
Review comment:
be careful with English grammar
##########
File path: doc/plugins/limit-count.md
##########
@@ -109,6 +110,41 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
}'
```
+If using redis-cluster:
+
+```shell
+curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "uri": "/index.html",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_serv_list": [
+ {
Review comment:
why not {'127.0.0.1:6373', '127.0.0.1:6374'}? which is the same style as upstream
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, c in ipairs(conf.redis_serv_list) do
Review comment:
`i` `c` both not good name
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, c in ipairs(conf.redis_serv_list) do
+ config.serv_list[i] = {ip = c.host, port = c.port}
+ end
+
+ local red_cli = rediscluster:new(config)
Review comment:
not return error msg？
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster",
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password
+ }
+
+ for i, c in ipairs(conf.redis_serv_list) do
+ config.serv_list[i] = {ip = c.host, port = c.port}
+ end
+
+ local red_cli = rediscluster:new(config)
+ if not red_cli then
+ error("connect to redis cluster fails")
Review comment:
Please check the grammar
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -54,6 +54,7 @@ dependencies = {
"skywalking-nginx-lua-plugin = 1.0-0",
"base64 = 1.5-2",
"dkjson = 2.5-2",
+ "resty-redis-cluster = 1.02-4",
Review comment:
the version is `1.02-4`? why so strange?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502994177
##########
File path: doc/plugins/limit-count.md
##########
@@ -39,11 +39,12 @@ Limit request rate by a fixed number of requests in a given time window.
| time_window | integer | required | | [0,...] | the time window in seconds before the request count is reset. |
| key | string | required | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | the user specified key to limit the rate. |
| rejected_code | integer | optional | 503 | [200,600] | The HTTP status code returned when the request exceeds the threshold is rejected, default 503. |
-| policy | string | optional | "local" | ["local", "redis"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
+| policy | string | optional | "local" | ["local", "redis", "redis-cluster"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
Review comment:
updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-706545779
@liuhengloveyou no test case?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502777237
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -119,6 +148,11 @@ local function create_limit_obj(conf)
conf.count, conf.time_window, conf)
end
+ if conf.policy == "redis-cluster" then
+ return limit_redis_cluster_new("plugin-" .. plugin_name,
+ conf.count, conf.time_window, conf)
Review comment:
bad indentation
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -70,11 +74,36 @@ local schema = {
type = "string", minLength = 0,
},
redis_timeout = {
- type = "integer", minimum = 1,
- default = 1000,
+ type = "integer", minimum = 1, default = 1000,
},
},
required = {"redis_host"},
+ },
+ {
+ properties = {
+ policy = {
+ enum = {"redis-cluster"},
+ },
+ redis_serv_list = {
+ type = "array",
+ minItems = 2,
+ items = {
+ type = "object",
+ properties = {
+ redis_host = {type = "string", minLength = 2},
+ redis_port = {type = "integer", minimum = 1},
Review comment:
`max` ?
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster", --rediscluster name
+ enable_slave_read = true,
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ send_timeout = 1000, --timeout while sending
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password --set password while setting auth
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
+ if not red_c then
+ error("connect to redis cluster fails")
+ end
+
+ return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+ assert(limit > 0 and window > 0)
+
+ _M.red_c = new_redis_cluster(conf)
Review comment:
that is wrong!! a bug, we need to fix it
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster", --rediscluster name
+ enable_slave_read = true,
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ send_timeout = 1000, --timeout while sending
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password --set password while setting auth
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+
Review comment:
only one blank line is required here
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster", --rediscluster name
+ enable_slave_read = true,
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ send_timeout = 1000, --timeout while sending
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password --set password while setting auth
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
+ if not red_c then
+ error("connect to redis cluster fails")
+ end
+
+ return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+ assert(limit > 0 and window > 0)
+
+ _M.red_c = new_redis_cluster(conf)
+
+ local self = {limit = limit, window = window, conf = conf,
+ plugin_name = plugin_name}
+ return setmetatable(self, mt)
+
+end
+
+
+function _M.incoming(self, key)
+ local conf = self.conf
+ local red = _M.red_c
+ core.log.info("ttl key: ", key, " timeout: ", conf.redis_timeout)
Review comment:
ditto
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster", --rediscluster name
+ enable_slave_read = true,
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ send_timeout = 1000, --timeout while sending
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password --set password while setting auth
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
+ if not red_c then
+ error("connect to redis cluster fails")
+ end
+
+ return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+ assert(limit > 0 and window > 0)
+
+ _M.red_c = new_redis_cluster(conf)
+
+ local self = {limit = limit, window = window, conf = conf,
+ plugin_name = plugin_name}
+ return setmetatable(self, mt)
+
+end
+
+
+function _M.incoming(self, key)
+ local conf = self.conf
Review comment:
need to delete this line
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster", --rediscluster name
+ enable_slave_read = true,
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ send_timeout = 1000, --timeout while sending
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password --set password while setting auth
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
Review comment:
`value` is a bad name
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster", --rediscluster name
+ enable_slave_read = true,
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ send_timeout = 1000, --timeout while sending
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password --set password while setting auth
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
Review comment:
`value['redis_host']` -> `value.redis_host`
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -70,11 +74,36 @@ local schema = {
type = "string", minLength = 0,
},
redis_timeout = {
- type = "integer", minimum = 1,
- default = 1000,
+ type = "integer", minimum = 1, default = 1000,
},
},
required = {"redis_host"},
+ },
+ {
+ properties = {
+ policy = {
+ enum = {"redis-cluster"},
+ },
+ redis_serv_list = {
+ type = "array",
+ minItems = 2,
+ items = {
+ type = "object",
+ properties = {
+ redis_host = {type = "string", minLength = 2},
+ redis_port = {type = "integer", minimum = 1},
Review comment:
I think `host`, `port` name style is better
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster", --rediscluster name
+ enable_slave_read = true,
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ send_timeout = 1000, --timeout while sending
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password --set password while setting auth
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
+ if not red_c then
+ error("connect to redis cluster fails")
+ end
+
+ return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+ assert(limit > 0 and window > 0)
+
+ _M.red_c = new_redis_cluster(conf)
+
+ local self = {limit = limit, window = window, conf = conf,
+ plugin_name = plugin_name}
+ return setmetatable(self, mt)
+
+end
+
+
+function _M.incoming(self, key)
+ local conf = self.conf
+ local red = _M.red_c
Review comment:
fix here
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster", --rediscluster name
+ enable_slave_read = true,
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ send_timeout = 1000, --timeout while sending
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password --set password while setting auth
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
+ if not red_c then
+ error("connect to redis cluster fails")
+ end
+
+ return red_c
Review comment:
`red_c` is a bad name
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ name = "apisix-rediscluster", --rediscluster name
+ enable_slave_read = true,
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ send_timeout = 1000, --timeout while sending
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ serv_list = {},
+ read_timeout = conf.redis_timeout,
+ auth = conf.redis_password --set password while setting auth
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
Review comment:
`key` is a bad name too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502789411
##########
File path: doc/zh-cn/plugins/limit-count.md
##########
@@ -26,17 +26,18 @@
## 参数
-| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
-| -------------- | ------- | ------------ | ------- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
-| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
-| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
-| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
-| policy | string | 可选 | "local" | ["local", "redis"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
-| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
-| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
-| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| --------------- | -------- | ------------ | ------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
+| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
+| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
+| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
+| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
+| policy | string | 可选 | "local" | ["local", "redis", "redis-cluster"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速)；以及`redis-cluster`，跟redis功能一样，只是使用redis集群方式。 |
+| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
+| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
+| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
+| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| redis_serv_list | array<object>[] | 可选 | | | 当使用 `redis-cluster` 限速策略时，该属性是 Redis 集群服务节点的地址列表。 |
Review comment:
The following have a example
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502805118
##########
File path: doc/plugins/limit-count.md
##########
@@ -39,11 +39,12 @@ Limit request rate by a fixed number of requests in a given time window.
| time_window | integer | required | | [0,...] | the time window in seconds before the request count is reset. |
| key | string | required | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | the user specified key to limit the rate. |
| rejected_code | integer | optional | 503 | [200,600] | The HTTP status code returned when the request exceeds the threshold is rejected, default 503. |
-| policy | string | optional | "local" | ["local", "redis"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
+| policy | string | optional | "local" | ["local", "redis", "redis-cluster"] | The rate-limiting policies to use for retrieving and incrementing the limits. Available values are `local`(the counters will be stored locally in-memory on the node) and `redis`(counters are stored on a Redis server and will be shared across the nodes, usually used it to do the global speed limit). |
Review comment:
where?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-706829329
> > @liuhengloveyou no test case?
>
> @liuhengloveyou are you kiding me? NO test cases AT ALL!
added
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r501552477
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,138 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ read_timeout = conf.redis_timeout or 1000,
+ enable_slave_read = true,
+ serv_list = {},
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+ if conf.redis_password then
+ config.auth = conf.redis_password --set password while setting auth
+ end
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
+
+ return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+ assert(limit > 0 and window > 0)
+
+ _M.red_c = new_redis_cluster(conf)
+
+ local self = {limit = limit, window = window, conf = conf,
+ plugin_name = plugin_name}
+ return setmetatable(self, mt)
+
+end
+
+
+function _M.incoming(self, key)
+ local conf = self.conf
+ local red = _M.red_c
+ core.log.info("ttl key: ", key, " timeout: ", conf.redis_timeout or 1000)
Review comment:
use JSONSchema to set the default value
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,138 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ read_timeout = conf.redis_timeout or 1000,
+ enable_slave_read = true,
+ serv_list = {},
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+ if conf.redis_password then
+ config.auth = conf.redis_password --set password while setting auth
+ end
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
+
+ return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+ assert(limit > 0 and window > 0)
+
+ _M.red_c = new_redis_cluster(conf)
+
+ local self = {limit = limit, window = window, conf = conf,
+ plugin_name = plugin_name}
+ return setmetatable(self, mt)
+
+end
+
+
+function _M.incoming(self, key)
+ local conf = self.conf
+ local red = _M.red_c
+ core.log.info("ttl key: ", key, " timeout: ", conf.redis_timeout or 1000)
+
+ ngx.log(ngx.ERR, ">>>>>>>>>>>>>>", key, require("cjson.safe").encode(self))
Review comment:
should use `core.log` always.
##########
File path: doc/zh-cn/plugins/limit-count.md
##########
@@ -26,17 +26,18 @@
## 参数
-| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
-| -------------- | ------- | ------------ | ------- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
-| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
-| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
-| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
-| policy | string | 可选 | "local" | ["local", "redis"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
-| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
-| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
-| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| --------------- | -------- | ------------ | ------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
+| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
+| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
+| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
+| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
+| policy | string | 可选 | "local" | ["local", "redis", "redis-cluster"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速)；以及`redis-cluster`，跟redis功能一样，只是使用redis集群方式。 |
Review comment:
we should only change the doc we need to update. small PR is easy for reviewing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503374195
##########
File path: t/plugin/limit-count-redis-cluster.t
##########
@@ -0,0 +1,228 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+BEGIN {
Review comment:
they were useless I think, we should remove them
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r501548890
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -132,7 +132,9 @@ function _M.check_schema(conf)
end
if conf.policy == "redis-cluster" then
- -- TODO
+ if not conf.redis_serv_list then
+ return false, "missing valid redis option serv_list"
Review comment:
we can use a better way, use JSONSchema to check this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502785396
##########
File path: doc/zh-cn/plugins/limit-count.md
##########
@@ -26,17 +26,18 @@
## 参数
-| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
-| -------------- | ------- | ------------ | ------- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
-| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
-| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
-| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
-| policy | string | 可选 | "local" | ["local", "redis"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string | `redis` 必须 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的地址。 |
-| redis_port | integer | 可选 | 6379 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的端口 |
-| redis_password | string | 可选 | | | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点的密码。 |
-| redis_timeout | integer | 可选 | 1000 | [1,...] | 当使用 `redis` 限速策略时，该属性是 Redis 服务节点以毫秒为单位的超时时间 |
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| --------------- | -------- | ------------ | ------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
+| count | integer | 必须 | | [0,...] | 指定时间窗口内的请求数量阈值 |
+| time_window | integer | 必须 | | [0,...] | 时间窗口的大小（以秒为单位），超过这个时间就会重置 |
+| key | string | 必须 | | ["remote_addr", "server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据 |
+| rejected_code | integer | 可选 | 503 | [200,600] | 当请求超过阈值被拒绝时，返回的 HTTP 状态码 |
+| policy | string | 可选 | "local" | ["local", "redis", "redis-cluster"] | 用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速)；以及`redis-cluster`，跟redis功能一样，只是使用redis集群方式。 |
Review comment:
The content is correct
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502726826
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -54,6 +54,7 @@ dependencies = {
"skywalking-nginx-lua-plugin = 1.0-0",
"base64 = 1.5-2",
"dkjson = 2.5-2",
+ "lua-resty-redis-cluster = 1.1-0",
Review comment:
updated
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -132,7 +132,9 @@ function _M.check_schema(conf)
end
if conf.policy == "redis-cluster" then
- -- TODO
+ if not conf.redis_serv_list then
+ return false, "missing valid redis option serv_list"
Review comment:
updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r499664897
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -54,6 +54,7 @@ dependencies = {
"skywalking-nginx-lua-plugin = 1.0-0",
"base64 = 1.5-2",
"dkjson = 2.5-2",
+ "lua-resty-redis-cluster = 1.1-0",
Review comment:
https://luarocks.org/modules/steve0511/resty-redis-cluster
we can use this library now. @liuhengloveyou
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-710707653
https://github.com/apache/apisix/pull/2406 has been merged. so we can close this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502795882
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -54,6 +54,7 @@ dependencies = {
"skywalking-nginx-lua-plugin = 1.0-0",
"base64 = 1.5-2",
"dkjson = 2.5-2",
+ "resty-redis-cluster = 1.02-4",
Review comment:
https://luarocks.org/modules/steve0511/resty-redis-cluster
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502807932
##########
File path: doc/plugins/limit-count.md
##########
@@ -109,6 +110,41 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
}'
```
+If using redis-cluster:
+
+```shell
+curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "uri": "/index.html",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_serv_list": [
+ {
Review comment:
updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502481627
##########
File path: doc/plugins/limit-count.md
##########
@@ -109,6 +110,41 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
}'
```
+If using redis-cluster:
+
+```shell
+curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "uri": "/index.html",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_serv_list": [
+ {
Review comment:
In the old version of the configuration, redis_host and redis_port were used.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502794290
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -54,6 +54,7 @@ dependencies = {
"skywalking-nginx-lua-plugin = 1.0-0",
"base64 = 1.5-2",
"dkjson = 2.5-2",
+ "resty-redis-cluster = 1.02-4",
Review comment:
it is the right version
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#issuecomment-705425077
pls fix the code style first: https://github.com/apache/apisix/runs/1224890754#step:6:117
```
Checking apisix/plugins/limit-count/limit-count-redis-cluster.lua 6 warnings
apisix/plugins/limit-count/limit-count-redis-cluster.lua:55:1: line contains only whitespace
apisix/plugins/limit-count/limit-count-redis-cluster.lua:58:1: line contains only whitespace
apisix/plugins/limit-count/limit-count-redis-cluster.lua:113:13: setting non-standard global variable ok
apisix/plugins/limit-count/limit-count-redis-cluster.lua:114:20: accessing undefined variable ok
apisix/plugins/limit-count/limit-count-redis-cluster.lua:126:9: setting non-standard global variable ok
apisix/plugins/limit-count/limit-count-redis-cluster.lua:127:16: accessing undefined variable ok
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502726892
##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+ __index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+ local config = {
+ dict_name = "redis_cluster_slot_locks", --shared dictionary name for locks
+ name = "apisix-rediscluster", --rediscluster name
+ keepalive_timeout = 60000, --redis connection pool idle timeout
+ keepalive_cons = 1000, --redis connection pool size
+ connect_timeout = 1000, --timeout while connecting
+ max_redirection = 5, --maximum retry attempts for redirection
+ max_connection_attempts = 1, --maximum retry attempts for connection
+ read_timeout = conf.redis_timeout or 1000,
+ enable_slave_read = true,
+ serv_list = {},
+ }
+
+ for key, value in ipairs(conf.redis_serv_list) do
+ if value['redis_host'] and value['redis_port'] then
+ config.serv_list[key] = {ip = value['redis_host'], port = value['redis_port']}
+ end
+ end
+
+ if conf.redis_password then
+ config.auth = conf.redis_password --set password while setting auth
+ end
+
+ local redis_cluster = require "resty.rediscluster"
+ local red_c = redis_cluster:new(config)
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503374436
##########
File path: t/plugin/limit-count-redis-cluster.t
##########
@@ -0,0 +1,228 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+BEGIN {
+ if ($ENV{TEST_NGINX_CHECK_LEAK}) {
+ $SkipReason = "unavailable for the hup tests";
+
+ } else {
+ $ENV{TEST_NGINX_USE_HUP} = 1;
+ undef $ENV{TEST_NGINX_USE_STAP};
+ }
+}
+
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_shuffle();
+no_root_location();
+run_tests;
Review comment:
one blank line before this line
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liuhengloveyou commented on a change in pull request #2340: feature: limit-count use redis cluster
Posted by GitBox <gi...@apache.org>.
liuhengloveyou commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502784661
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -70,11 +74,36 @@ local schema = {
type = "string", minLength = 0,
},
redis_timeout = {
- type = "integer", minimum = 1,
- default = 1000,
+ type = "integer", minimum = 1, default = 1000,
},
},
required = {"redis_host"},
+ },
+ {
+ properties = {
+ policy = {
+ enum = {"redis-cluster"},
+ },
+ redis_serv_list = {
+ type = "array",
+ minItems = 2,
+ items = {
+ type = "object",
+ properties = {
+ redis_host = {type = "string", minLength = 2},
+ redis_port = {type = "integer", minimum = 1},
Review comment:
updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org