Skip to content

Commit aa3573d

Browse files
subnetmarcosubnetmarco
authored
hotfix(rate-limiting) backporting #1843 fix for fault tolerancy (#1999)
1 parent 87539de commit aa3573d

File tree

4 files changed

+151
-15
lines changed

4 files changed

+151
-15
lines changed

‎kong/plugins/rate-limiting/policies.lua‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,11 @@ return {
3333
local _, err = cache.incr(cache_key, value)
3434
if err then
3535
ngx_log("[rate-limiting] could not increment counter for period '"..period.."': "..tostring(err))
36+
return nil, err
3637
end
3738
end
39+
40+
return true
3841
end,
3942
usage = function(conf, api_id, identifier, current_timestamp, name)
4043
local periods = timestamp.get_timestamps(current_timestamp)
@@ -68,14 +71,14 @@ return {
6871
local ok, err = red:connect(conf.redis_host, conf.redis_port)
6972
if not ok then
7073
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
71-
return
74+
return nil, err
7275
end
7376

7477
if conf.redis_password and conf.redis_password ~= "" then
7578
local ok, err = red:auth(conf.redis_password)
7679
if not ok then
7780
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
78-
return
81+
return nil, err
7982
end
8083
end
8184

@@ -85,7 +88,7 @@ return {
8588
local exists, err = red:exists(cache_key)
8689
if err then
8790
ngx_log(ngx.ERR, "failed to query Redis: ", err)
88-
return
91+
return nil, err
8992
end
9093

9194
red:init_pipeline((not exists or exists == 0) and 2 or 1)
@@ -97,30 +100,32 @@ return {
97100
local _, err = red:commit_pipeline()
98101
if err then
99102
ngx_log(ngx.ERR, "failed to commit pipeline in Redis: ", err)
100-
return
103+
return nil, err
101104
end
102105
end
103106

104107
local ok, err = red:set_keepalive(10000, 100)
105108
if not ok then
106109
ngx_log(ngx.ERR, "failed to set Redis keepalive: ", err)
107-
return
110+
return nil, err
108111
end
112+
113+
return true
109114
end,
110115
usage = function(conf, api_id, identifier, current_timestamp, name)
111116
local red = redis:new()
112117
red:set_timeout(conf.redis_timeout)
113118
local ok, err = red:connect(conf.redis_host, conf.redis_port)
114119
if not ok then
115120
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
116-
return
121+
return nil, err
117122
end
118123

119124
if conf.redis_password and conf.redis_password ~= "" then
120125
local ok, err = red:auth(conf.redis_password)
121126
if not ok then
122127
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
123-
return
128+
return nil, err
124129
end
125130
end
126131

‎kong/plugins/response-ratelimiting/policies.lua‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,12 @@ return {
3232

3333
local _, err = cache.incr(cache_key, value)
3434
if err then
35-
ngx_log("[rate-limiting] could not increment counter for period '"..period.."': "..tostring(err))
35+
ngx_log("[response-ratelimiting] could not increment counter for period '"..period.."': "..tostring(err))
36+
return nil, err
3637
end
3738
end
39+
40+
return true
3841
end,
3942
usage = function(conf, api_id, identifier, current_timestamp, period, name)
4043
local periods = timestamp.get_timestamps(current_timestamp)
@@ -68,14 +71,14 @@ return {
6871
local ok, err = red:connect(conf.redis_host, conf.redis_port)
6972
if not ok then
7073
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
71-
return
74+
return nil, err
7275
end
7376

7477
if conf.redis_password and conf.redis_password ~= "" then
7578
local ok, err = red:auth(conf.redis_password)
7679
if not ok then
7780
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
78-
return
81+
return nil, err
7982
end
8083
end
8184

@@ -85,7 +88,7 @@ return {
8588
local exists, err = red:exists(cache_key)
8689
if err then
8790
ngx_log(ngx.ERR, "failed to query Redis: ", err)
88-
return
91+
return nil, err
8992
end
9093

9194
red:init_pipeline((not exists or exists == 0) and 2 or 1)
@@ -97,30 +100,32 @@ return {
97100
local _, err = red:commit_pipeline()
98101
if err then
99102
ngx_log(ngx.ERR, "failed to commit pipeline in Redis: ", err)
100-
return
103+
return nil, err
101104
end
102105
end
103106

104107
local ok, err = red:set_keepalive(10000, 100)
105108
if not ok then
106109
ngx_log(ngx.ERR, "failed to set Redis keepalive: ", err)
107-
return
110+
return nil, err
108111
end
112+
113+
return true
109114
end,
110115
usage = function(conf, api_id, identifier, current_timestamp, period, name)
111116
local red = redis:new()
112117
red:set_timeout(conf.redis_timeout)
113118
local ok, err = red:connect(conf.redis_host, conf.redis_port)
114119
if not ok then
115120
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
116-
return
121+
return nil, err
117122
end
118123

119124
if conf.redis_password and conf.redis_password ~= "" then
120125
local ok, err = red:auth(conf.redis_password)
121126
if not ok then
122127
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
123-
return
128+
return nil, err
124129
end
125130
end
126131

‎spec/03-plugins/98-rate-limiting/04-access_spec.lua‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,64 @@ for i, policy in ipairs({"local", "cluster", "redis"}) do
434434
assert.falsy(res.headers["x-ratelimit-remaining-minute"])
435435
end)
436436
end)
437+
438+
elseif policy == "redis" then
439+
describe("Fault tolerancy", function()
440+
441+
before_each(function()
442+
helpers.kill_all()
443+
helpers.dao:drop_schema()
444+
assert(helpers.dao:run_migrations())
445+
446+
local api1 = assert(helpers.dao.apis:insert {
447+
request_host = "failtest3.com",
448+
upstream_url = "http://mockbin.com"
449+
})
450+
assert(helpers.dao.plugins:insert {
451+
name = "rate-limiting",
452+
api_id = api1.id,
453+
config = { minute = 6, policy = policy, redis_host = "5.5.5.5", fault_tolerant = false }
454+
})
455+
456+
local api2 = assert(helpers.dao.apis:insert {
457+
request_host = "failtest4.com",
458+
upstream_url = "http://mockbin.com"
459+
})
460+
assert(helpers.dao.plugins:insert {
461+
name = "rate-limiting",
462+
api_id = api2.id,
463+
config = { minute = 6, policy = policy, redis_host = "5.5.5.5", fault_tolerant = true }
464+
})
465+
466+
assert(helpers.start_kong())
467+
end)
468+
469+
it("does not work if an error occurs", function()
470+
-- Make another request
471+
local res = assert(helpers.proxy_client():send {
472+
method = "GET",
473+
path = "/status/200/",
474+
headers = {
475+
["Host"] = "failtest3.com"
476+
}
477+
})
478+
local body = assert.res_status(500, res)
479+
assert.are.equal([[{"message":"An unexpected error occurred"}]], body)
480+
end)
481+
it("keeps working if an error occurs", function()
482+
-- Make another request
483+
local res = assert(helpers.proxy_client():send {
484+
method = "GET",
485+
path = "/status/200/",
486+
headers = {
487+
["Host"] = "failtest4.com"
488+
}
489+
})
490+
assert.res_status(200, res)
491+
assert.falsy(res.headers["x-ratelimit-limit-minute"])
492+
assert.falsy(res.headers["x-ratelimit-remaining-minute"])
493+
end)
494+
end)
437495
end
438496

439497
describe("Expirations", function()

‎spec/03-plugins/98-response-rate-limiting/04-access_spec.lua‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -532,6 +532,74 @@ for i, policy in ipairs({"local", "cluster", "redis"}) do
532532
assert.is_nil(res.headers["x-ratelimit-remaining-video-minute"])
533533
end)
534534
end)
535+
536+
elseif policy == "redis" then
537+
describe("Fault tolerancy", function()
538+
539+
before_each(function()
540+
helpers.kill_all()
541+
helpers.dao:drop_schema()
542+
assert(helpers.dao:run_migrations())
543+
544+
local api1 = assert(helpers.dao.apis:insert {
545+
request_host = "failtest3.com",
546+
upstream_url = "http://mockbin.com"
547+
})
548+
assert(helpers.dao.plugins:insert {
549+
name = "response-ratelimiting",
550+
api_id = api1.id,
551+
config = {
552+
fault_tolerant = false,
553+
policy = policy,
554+
redis_host = "5.5.5.5",
555+
limits = {video = {minute = 6}}
556+
}
557+
})
558+
559+
local api2 = assert(helpers.dao.apis:insert {
560+
request_host = "failtest4.com",
561+
upstream_url = "http://mockbin.com"
562+
})
563+
assert(helpers.dao.plugins:insert {
564+
name = "response-ratelimiting",
565+
api_id = api2.id,
566+
config = {
567+
fault_tolerant = true,
568+
policy = policy,
569+
redis_host = "5.5.5.5",
570+
limits = {video = {minute = 6}}
571+
}
572+
})
573+
574+
assert(helpers.start_kong())
575+
end)
576+
577+
it("does not work if an error occurs", function()
578+
-- Make another request
579+
local res = assert(helpers.proxy_client():send {
580+
method = "GET",
581+
path = "/status/200/",
582+
headers = {
583+
["Host"] = "failtest3.com"
584+
}
585+
})
586+
local body = assert.res_status(500, res)
587+
assert.are.equal([[{"message":"An unexpected error occurred"}]], body)
588+
end)
589+
it("keeps working if an error occurs", function()
590+
-- Make another request
591+
local res = assert(helpers.proxy_client():send {
592+
method = "GET",
593+
path = "/status/200/",
594+
headers = {
595+
["Host"] = "failtest4.com"
596+
}
597+
})
598+
assert.res_status(200, res)
599+
assert.falsy(res.headers["x-ratelimit-limit-video-minute"])
600+
assert.falsy(res.headers["x-ratelimit-remaining-video-minute"])
601+
end)
602+
end)
535603
end
536604

537605
describe("Expirations", function()

0 commit comments

Comments
 (0)