Skip to content

Commit

Permalink
fix(limit-count): keep the counter if the plugin conf is the same (#6151
Browse files Browse the repository at this point in the history 
)
  • Loading branch information
@spacewander
spacewander authored Jan 25, 2022
1 parent 56d37db commit a44c98e
Show file tree
Hide file tree
Showing 10 changed files with 335 additions and 96 deletions.
4 changes: 4 additions & 0 deletions apisix/core/json.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ local cached_tab = {}
local _M = {
version = 0.1,
decode = require("cjson.safe").decode,
-- This method produces the same encoded string when the input is not changed.
-- Different calls with cjson.encode will produce different string because
-- it doesn't maintain the object key order.
stably_encode = require("dkjson").encode
}


Expand Down
16 changes: 15 additions & 1 deletion apisix/plugin.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ local core = require("apisix.core")
local config_util = require("apisix.core.config_util")
local enable_debug = require("apisix.debug").enable_debug
local wasm = require("apisix.wasm")
local ngx = ngx
local crc32 = ngx.crc32_short
local ngx_exit = ngx.exit
local pkg_loaded = package.loaded
local sort_tab = table.sort
Expand All @@ -27,7 +29,6 @@ local ipairs = ipairs
local pairs = pairs
local type = type
local local_plugins = core.table.new(32, 0)
local ngx = ngx
local tostring = tostring
local error = error
local is_http = ngx.config.subsystem == "http"
Expand Down Expand Up @@ -603,6 +604,19 @@ function _M.get_all(attrs)
end


-- conf_version returns a version which only depends on the value of conf,
-- instead of where this plugin conf belongs to
function _M.conf_version(conf)
if not conf._version then
local data = core.json.stably_encode(conf)
conf._version = tostring(crc32(data))
core.log.info("init plugin-level conf version: ", conf._version, ", from ", data)
end

return conf._version
end


local function check_schema(plugins_conf, schema_type, skip_disabled_plugin)
for name, plugin_conf in pairs(plugins_conf) do
core.log.info("check plugin schema, name: ", name, ", configurations: ",
Expand Down
47 changes: 21 additions & 26 deletions apisix/plugins/limit-count.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
--
local limit_local_new = require("resty.limit.count").new
local core = require("apisix.core")
local apisix_plugin = require("apisix.plugin")
local tab_insert = table.insert
local ipairs = ipairs
local pairs = pairs
Expand Down Expand Up @@ -108,32 +109,23 @@ local schema = {
show_limit_quota_header = {type = "boolean", default = true}
},
required = {"count", "time_window"},
dependencies = {
policy = {
oneOf = {
{
properties = {
policy = {
enum = {"local"},
},
},
["if"] = {
properties = {
policy = {
enum = {"redis"},
},
},
},
["then"] = policy_to_additional_properties.redis,
["else"] = {
["if"] = {
properties = {
policy = {
enum = {"redis-cluster"},
},
core.table.merge({
properties = {
policy = {
enum = {"redis"},
},
},
}, policy_to_additional_properties.redis),
core.table.merge({
properties = {
policy = {
enum = {"redis-cluster"},
},
},
}, policy_to_additional_properties["redis-cluster"]),
}
}
},
},
["then"] = policy_to_additional_properties["redis-cluster"],
}
}

Expand Down Expand Up @@ -252,7 +244,10 @@ function _M.access(conf, ctx)

-- here we add a separator ':' to mark the boundary of the prefix and the key itself
if not conf.group then
key = ctx.conf_type .. ctx.conf_version .. ':' .. key
-- Here we use plugin-level conf version to prevent the counter from being resetting
-- because of the change elsewhere.
-- A route which reuses a previous route's ID will inherits its counter.
key = ctx.conf_type .. apisix_plugin.conf_version(conf) .. ':' .. key
else
key = conf.group .. ':' .. key
end
Expand Down
2 changes: 1 addition & 1 deletion rockspec/apisix-master-0.rockspec
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ dependencies = {
"skywalking-nginx-lua = 0.6.0",
"base64 = 1.5-2",
"binaryheap = 0.4",
"dkjson = 2.5-2",
"api7-dkjson = 0.1.1",
"resty-redis-cluster = 1.02-4",
"lua-resty-expr = 1.3.1",
"graphql = 0.0.2",
Expand Down
4 changes: 2 additions & 2 deletions t/plugin/limit-count-redis-cluster.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ __DATA__
GET /t
--- error_code: 400
--- response_body
{"error_msg":"failed to check the configuration of plugin limit-count err: failed to validate dependent schema for \"policy\": value should match only one schema, but matches none"}
{"error_msg":"failed to check the configuration of plugin limit-count err: else clause did not match"}
--- no_error_log
[error]
Expand Down Expand Up @@ -327,7 +327,7 @@ code: 200
"plugins": {
"limit-count": {
"count": ]] .. count .. [[,
"time_window": 60,
"time_window": 69,
"key": "remote_addr",
"policy": "redis-cluster",
"redis_cluster_nodes": [
Expand Down
2 changes: 1 addition & 1 deletion t/plugin/limit-count-redis.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ __DATA__
GET /t
--- error_code: 400
--- response_body
{"error_msg":"failed to check the configuration of plugin limit-count err: failed to validate dependent schema for \"policy\": value should match only one schema, but matches none"}
{"error_msg":"failed to check the configuration of plugin limit-count err: then clause did not match"}
--- no_error_log
[error]
Expand Down
20 changes: 10 additions & 10 deletions t/plugin/limit-count.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ GET /t
"plugins": {
"limit-count": {
"count": 2,
"time_window": 60,
"time_window": 61,
"rejected_code": 503,
"key": "remote_addr"
}
Expand Down Expand Up @@ -597,7 +597,7 @@ passed
"plugins": {
"limit-count": {
"count": 2,
"time_window": 60,
"time_window": 80,
"key": "remote_addr"
}
},
Expand Down Expand Up @@ -649,7 +649,7 @@ passed
"plugins": {
"limit-count": {
"count": 1,
"time_window": 60,
"time_window": 80,
"key": "remote_addr"
}
},
Expand Down Expand Up @@ -701,7 +701,7 @@ passed
"plugins": {
"limit-count": {
"count": 2,
"time_window": 60,
"time_window": 82,
"key": "remote_addr"
}
},
Expand Down Expand Up @@ -742,7 +742,7 @@ passed
"plugins": {
"limit-count": {
"count": 1,
"time_window": 60,
"time_window": 82,
"key": "remote_addr"
}
},
Expand Down Expand Up @@ -794,7 +794,7 @@ passed
"plugins": {
"limit-count": {
"count": 2,
"time_window": 60,
"time_window": 83,
"key": "remote_addr"
}
},
Expand Down Expand Up @@ -828,7 +828,7 @@ passed
--- pipelined_requests eval
["GET /t", "GET /hello", "GET /hello", "GET /hello", "GET /t", "GET /hello", "GET /hello", "GET /hello"]
--- error_code eval
[200, 200, 200, 503, 200, 200, 200, 503]
[200, 200, 200, 503, 200, 503, 503, 503]
--- no_error_log
[error]

Expand Down Expand Up @@ -1041,7 +1041,7 @@ passed
"plugins": {
"limit-count": {
"count": 2,
"time_window": 60,
"time_window": 91,
"rejected_code": 503,
"key": "service_id"
}
Expand Down Expand Up @@ -1138,7 +1138,7 @@ passed
"plugins": {
"limit-count": {
"count": 2,
"time_window": 60,
"time_window": 95,
"rejected_code": 503
}
},
Expand Down Expand Up @@ -1272,7 +1272,7 @@ passed
"plugins": {
"limit-count": {
"count": 3,
"time_window": 60,
"time_window": 99,
"rejected_code": 503
}
},
Expand Down
Loading

0 comments on commit a44c98e

Please sign in to comment.