Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(limit-count): keep the counter if the plugin conf is the same #6151

Merged
merged 1 commit into from
Jan 25, 2022
Merged

fix(limit-count): keep the counter if the plugin conf is the same #6151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions apisix/core/json.lua
View file
Open in desktop
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much needed feature : )

}


Expand Down
16 changes: 15 additions & 1 deletion apisix/plugin.lua
View file
Open in desktop
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 @@ -599,6 +600,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
Open in desktop
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
Open in desktop
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
Open in desktop
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"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error messages is confusing after changes, it cannot help API callers troubleshoot effectively, will we have some enhancements?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. We can improve it in the jsonschema library.

--- 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
Open in desktop
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
Open in desktop
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