From a887eadddc8c02149e3889a7c1ba6d945a60056e Mon Sep 17 00:00:00 2001 From: spacewander Date: Wed, 19 Jan 2022 20:07:08 +0800 Subject: [PATCH] fix(limit-count): keep the counter if the plugin conf is the same Signed-off-by: spacewander --- apisix/core/json.lua | 4 + apisix/plugin.lua | 16 +- apisix/plugins/limit-count.lua | 47 +++--- rockspec/apisix-master-0.rockspec | 2 +- t/plugin/limit-count-redis-cluster.t | 4 +- t/plugin/limit-count-redis.t | 2 +- t/plugin/limit-count.t | 20 +-- t/plugin/limit-count3.t | 226 +++++++++++++++++++++++++++ t/plugin/zipkin.t | 65 ++------ t/plugin/zipkin2.t | 45 ++++++ 10 files changed, 335 insertions(+), 96 deletions(-) create mode 100644 t/plugin/limit-count3.t diff --git a/apisix/core/json.lua b/apisix/core/json.lua index 7fad872bc163..2a83d71cc152 100644 --- a/apisix/core/json.lua +++ b/apisix/core/json.lua @@ -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 } diff --git a/apisix/plugin.lua b/apisix/plugin.lua index 7864a0084ddd..ca8da60a2c7b 100644 --- a/apisix/plugin.lua +++ b/apisix/plugin.lua @@ -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 @@ -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" @@ -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: ", diff --git a/apisix/plugins/limit-count.lua b/apisix/plugins/limit-count.lua index 2a5779235b91..62fa9a92db0d 100644 --- a/apisix/plugins/limit-count.lua +++ b/apisix/plugins/limit-count.lua @@ -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 @@ -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"], } } @@ -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 diff --git a/rockspec/apisix-master-0.rockspec b/rockspec/apisix-master-0.rockspec index f40bf003d632..44116062cb0f 100644 --- a/rockspec/apisix-master-0.rockspec +++ b/rockspec/apisix-master-0.rockspec @@ -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", diff --git a/t/plugin/limit-count-redis-cluster.t b/t/plugin/limit-count-redis-cluster.t index 7b2b04f5bb87..5a00b13327b5 100644 --- a/t/plugin/limit-count-redis-cluster.t +++ b/t/plugin/limit-count-redis-cluster.t @@ -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] @@ -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": [ diff --git a/t/plugin/limit-count-redis.t b/t/plugin/limit-count-redis.t index 7b3948576d85..4b2e9cf44e3e 100644 --- a/t/plugin/limit-count-redis.t +++ b/t/plugin/limit-count-redis.t @@ -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] diff --git a/t/plugin/limit-count.t b/t/plugin/limit-count.t index 671443dcba3a..46092cee855e 100644 --- a/t/plugin/limit-count.t +++ b/t/plugin/limit-count.t @@ -448,7 +448,7 @@ GET /t "plugins": { "limit-count": { "count": 2, - "time_window": 60, + "time_window": 61, "rejected_code": 503, "key": "remote_addr" } @@ -597,7 +597,7 @@ passed "plugins": { "limit-count": { "count": 2, - "time_window": 60, + "time_window": 80, "key": "remote_addr" } }, @@ -649,7 +649,7 @@ passed "plugins": { "limit-count": { "count": 1, - "time_window": 60, + "time_window": 80, "key": "remote_addr" } }, @@ -701,7 +701,7 @@ passed "plugins": { "limit-count": { "count": 2, - "time_window": 60, + "time_window": 82, "key": "remote_addr" } }, @@ -742,7 +742,7 @@ passed "plugins": { "limit-count": { "count": 1, - "time_window": 60, + "time_window": 82, "key": "remote_addr" } }, @@ -794,7 +794,7 @@ passed "plugins": { "limit-count": { "count": 2, - "time_window": 60, + "time_window": 83, "key": "remote_addr" } }, @@ -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] @@ -1041,7 +1041,7 @@ passed "plugins": { "limit-count": { "count": 2, - "time_window": 60, + "time_window": 91, "rejected_code": 503, "key": "service_id" } @@ -1138,7 +1138,7 @@ passed "plugins": { "limit-count": { "count": 2, - "time_window": 60, + "time_window": 95, "rejected_code": 503 } }, @@ -1272,7 +1272,7 @@ passed "plugins": { "limit-count": { "count": 3, - "time_window": 60, + "time_window": 99, "rejected_code": 503 } }, diff --git a/t/plugin/limit-count3.t b/t/plugin/limit-count3.t new file mode 100644 index 000000000000..4298a20bd604 --- /dev/null +++ b/t/plugin/limit-count3.t @@ -0,0 +1,226 @@ +# +# 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'; + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!$block->request) { + $block->set_value("request", "GET /t"); + } + + if (!$block->error_log && !$block->no_error_log) { + $block->set_value("no_error_log", "[error]\n[alert]"); + } +}); + +run_tests; + +__DATA__ + +=== TEST 1: set route, counter will be shared +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello", + "plugins": { + "limit-count": { + "count": 2, + "time_window": 60 + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 2: hit +--- config + location /t { + content_by_lua_block { + local json = require "t.toolkit.json" + local http = require "resty.http" + local uri = "http://127.0.0.1:" .. ngx.var.server_port + .. "/hello" + local ress = {} + for i = 1, 2 do + local httpc = http.new() + local res, err = httpc:request_uri(uri) + if not res then + ngx.say(err) + return + end + table.insert(ress, res.status) + end + ngx.say(json.encode(ress)) + } + } +--- response_body +[200,200] + + + +=== TEST 3: set route with conf not changed +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello", + "plugins": { + "limit-count": { + "count": 2, + "time_window": 60 + } + }, + "labels": {"l": "a"}, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 4: hit +--- config + location /t { + content_by_lua_block { + local json = require "t.toolkit.json" + local http = require "resty.http" + local uri = "http://127.0.0.1:" .. ngx.var.server_port + .. "/hello" + local ress = {} + for i = 1, 2 do + local httpc = http.new() + local res, err = httpc:request_uri(uri) + if not res then + ngx.say(err) + return + end + table.insert(ress, res.status) + end + ngx.say(json.encode(ress)) + } + } +--- response_body +[503,503] + + + +=== TEST 5: set route with conf changed +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello", + "plugins": { + "limit-count": { + "count": 2, + "time_window": 61 + } + }, + "labels": {"l": "a"}, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 6: hit +--- config + location /t { + content_by_lua_block { + local json = require "t.toolkit.json" + local http = require "resty.http" + local uri = "http://127.0.0.1:" .. ngx.var.server_port + .. "/hello" + local ress = {} + for i = 1, 2 do + local httpc = http.new() + local res, err = httpc:request_uri(uri) + if not res then + ngx.say(err) + return + end + table.insert(ress, res.status) + end + ngx.say(json.encode(ress)) + } + } +--- response_body +[200,200] diff --git a/t/plugin/zipkin.t b/t/plugin/zipkin.t index 04b587ab3ac0..d97556342b10 100644 --- a/t/plugin/zipkin.t +++ b/t/plugin/zipkin.t @@ -371,52 +371,7 @@ property "server_addr" validation failed: failed to match pattern "^[0-9]{1,3}.[ -=== TEST 13: check not error with limit count ---- config - location /t { - content_by_lua_block { - local t = require("lib.test_admin").test - local code, body = t('/apisix/admin/routes/1', - ngx.HTTP_PUT, - [[{ - "plugins": { - "zipkin": { - "endpoint": "http://127.0.0.1:9999/mock_zipkin", - "sample_ratio": 1, - "service_name": "APISIX" - }, - "limit-count": { - "count": 2, - "time_window": 60, - "rejected_code": 403, - "key": "remote_addr" - } - }, - "upstream": { - "nodes": { - "127.0.0.1:1980": 1 - }, - "type": "roundrobin" - }, - "uri": "/opentracing" - }]]) - - if code >= 300 then - ngx.status = code - end - ngx.say(body) - } - } ---- pipelined_requests eval -["GET /t", "GET /opentracing", "GET /opentracing", "GET /opentracing"] ---- error_code eval -[200, 200, 200, 403] ---- no_error_log -[error] - - - -=== TEST 14: check zipkin headers +=== TEST 13: check zipkin headers --- config location /t { content_by_lua_block { @@ -455,7 +410,7 @@ passed -=== TEST 15: set x-b3-sampled if sampled +=== TEST 14: set x-b3-sampled if sampled --- request GET /echo --- response_headers @@ -463,7 +418,7 @@ x-b3-sampled: 1 -=== TEST 16: don't sample if disabled +=== TEST 15: don't sample if disabled --- request GET /echo --- more_headers @@ -473,7 +428,7 @@ x-b3-sampled: 0 -=== TEST 17: don't sample if disabled (old way) +=== TEST 16: don't sample if disabled (old way) --- request GET /echo --- more_headers @@ -483,7 +438,7 @@ x-b3-sampled: 0 -=== TEST 18: sample according to the header +=== TEST 17: sample according to the header --- config location /t { content_by_lua_block { @@ -522,7 +477,7 @@ passed -=== TEST 19: don't sample by default +=== TEST 18: don't sample by default --- request GET /echo --- response_headers @@ -530,7 +485,7 @@ x-b3-sampled: 0 -=== TEST 20: sample if needed +=== TEST 19: sample if needed --- request GET /echo --- more_headers @@ -540,7 +495,7 @@ x-b3-sampled: 1 -=== TEST 21: sample if debug +=== TEST 20: sample if debug --- request GET /echo --- more_headers @@ -550,7 +505,7 @@ x-b3-sampled: 1 -=== TEST 22: sample if needed (old way) +=== TEST 21: sample if needed (old way) --- request GET /echo --- more_headers @@ -560,7 +515,7 @@ x-b3-sampled: 1 -=== TEST 23: don't cache the per-req sample ratio +=== TEST 22: don't cache the per-req sample ratio --- config location /t { content_by_lua_block { diff --git a/t/plugin/zipkin2.t b/t/plugin/zipkin2.t index 9840092943e2..3175075d32ed 100644 --- a/t/plugin/zipkin2.t +++ b/t/plugin/zipkin2.t @@ -211,3 +211,48 @@ GET /opentracing --- grep_error_log eval qr/zipkin start_child_span apisix.response_span time: nil/ --- grep_error_log_out + + + +=== TEST 11: check not error with limit count +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "zipkin": { + "endpoint": "http://127.0.0.1:9999/mock_zipkin", + "sample_ratio": 1, + "service_name": "APISIX" + }, + "limit-count": { + "count": 2, + "time_window": 60, + "rejected_code": 403, + "key": "remote_addr" + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/opentracing" + }]]) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- pipelined_requests eval +["GET /t", "GET /opentracing", "GET /opentracing", "GET /opentracing"] +--- error_code eval +[200, 200, 200, 403] +--- no_error_log +[error]