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(plugin) reuse redis conn for different db & auth optimize #3293

Closed
wants to merge 8 commits into from
Closed

fix(plugin) reuse redis conn for different db & auth optimize #3293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f307fa8
fix plugin reuse redis conn for different db & auth optimize
mengskysama Mar 13, 2018
1172064
style cleanup
mengskysama Mar 14, 2018
072fb4c
test(ci) add redis env for travis-ci
mengskysama Mar 14, 2018
918b0c9
test(rate-limiting) integration test for redis conn reuse
mengskysama Mar 14, 2018
1668b40
test(response-rate-limiting) integration test for redis conn reuse
mengskysama Mar 14, 2018
d442475
add comment
mengskysama Mar 15, 2018
f521cb2
test(rate-limiting) integration test for redis conn reuse
mengskysama Mar 15, 2018
e05aee8
test(response-rate-limiting) integration test for redis conn reuse
mengskysama Mar 15, 2018
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
3 changes: 3 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ jdk:
notifications:
email: false

services:
- redis-server

addons:
postgresql: "9.4"
apt:
Expand Down
38 changes: 23 additions & 15 deletions kong/plugins/rate-limiting/policies/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,20 +84,24 @@ return {
return nil, err
end

if conf.redis_password and conf.redis_password ~= "" then
local times, err = red:get_reused_times()
if err then
ngx_log(ngx.ERR, "failed to get connect reused times: ", err)
return nil, err
end
if times == 0 and conf.redis_password and conf.redis_password ~= "" then
local ok, err = red:auth(conf.redis_password)
if not ok then
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
ngx_log(ngx.ERR, "failed to auth Redis: ", err)
return nil, err
end
end

if conf.redis_database ~= nil and conf.redis_database > 0 then
local ok, err = red:select(conf.redis_database)
if not ok then
ngx_log(ngx.ERR, "failed to change Redis database: ", err)
return nil, err
end
-- redis connection pool conflicts with other plugin instance which used same reids
local ok, err = red:select(conf.redis_database or 0)
if not ok then
ngx_log(ngx.ERR, "failed to change Redis database: ", err)
return nil, err
end

local keys = {}
Expand Down Expand Up @@ -151,20 +155,24 @@ return {
return nil, err
end

if conf.redis_password and conf.redis_password ~= "" then
local times, err = red:get_reused_times()
if err then
ngx_log(ngx.ERR, "failed to get connect reused times: ", err)
return nil, err
end
if times == 0 and conf.redis_password and conf.redis_password ~= "" then
local ok, err = red:auth(conf.redis_password)
if not ok then
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
return nil, err
end
end

if conf.redis_database ~= nil and conf.redis_database > 0 then
local ok, err = red:select(conf.redis_database)
if not ok then
ngx_log(ngx.ERR, "failed to change Redis database: ", err)
return nil, err
end
-- redis connection pool conflicts with other plugin instance which used same reids
local ok, err = red:select(conf.redis_database or 0)
if not ok then
ngx_log(ngx.ERR, "failed to change Redis database: ", err)
return nil, err
end

reports.retrieve_redis_version(red)
Expand Down
40 changes: 24 additions & 16 deletions kong/plugins/response-ratelimiting/policies/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,20 +84,24 @@ return {
return nil, err
end

if conf.redis_password and conf.redis_password ~= "" then
local times, err = red:get_reused_times()
if err then
ngx_log(ngx.ERR, "failed to get connect reused times: ", err)
return nil, err
end
if times == 0 and conf.redis_password and conf.redis_password ~= "" then
local ok, err = red:auth(conf.redis_password)
if not ok then
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
ngx_log(ngx.ERR, "failed to auth Redis: ", err)
return nil, err
end
end

if conf.redis_database ~= nil and conf.redis_database > 0 then
local ok, err = red:select(conf.redis_database)
if not ok then
ngx_log(ngx.ERR, "failed to change Redis database: ", err)
return nil, err
end
-- redis connection pool conflicts with other plugin instance which used same reids
local ok, err = red:select(conf.redis_database or 0)
if not ok then
ngx_log(ngx.ERR, "failed to change Redis database: ", err)
return nil, err
end

local keys = {}
Expand Down Expand Up @@ -149,20 +153,24 @@ return {
return nil, err
end

if conf.redis_password and conf.redis_password ~= "" then
local times, err = red:get_reused_times()
if err then
ngx_log(ngx.ERR, "failed to get connect reused times: ", err)
return nil, err
end
if times == 0 and conf.redis_password and conf.redis_password ~= "" then
local ok, err = red:auth(conf.redis_password)
if not ok then
ngx_log(ngx.ERR, "failed to connect to Redis: ", err)
ngx_log(ngx.ERR, "failed to auth Redis: ", err)
return nil, err
end
end

if conf.redis_database ~= nil and conf.redis_database > 0 then
local ok, err = red:select(conf.redis_database)
if not ok then
ngx_log(ngx.ERR, "failed to change Redis database: ", err)
return nil, err
end
-- redis connection pool conflicts with other plugin instance which used same reids
local ok, err = red:select(conf.redis_database or 0)
if not ok then
ngx_log(ngx.ERR, "failed to change Redis database: ", err)
return nil, err
end

reports.retrieve_redis_version(red)
Expand Down
168 changes: 168 additions & 0 deletions spec/03-plugins/24-rate-limiting/05-integration_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
local helpers = require "spec.helpers"
local redis = require "resty.redis"

local REDIS_HOST = "127.0.0.1"
local REDIS_PORT = 6379
local REDIS_PASSWORD = ""
local REDIS_DATABASE = 1
local REDIS_DATABASE_SECOND = 0

local SLEEP_TIME = 1

local function flush_redis(db)
local red = redis:new()
red:set_timeout(2000)
local ok, err = red:connect(REDIS_HOST, REDIS_PORT)
Copy link
Member

Choose a reason for hiding this comment

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

Let's use the proper Lua idioms here and save us a lot of boilerplate:

assert(red:connect())
assert(red:auth())
assert(red:select())

if not ok then
error("failed to connect to Redis: " .. err)
end

if REDIS_PASSWORD and REDIS_PASSWORD ~= "" then
local ok, err = red:auth(REDIS_PASSWORD)
if not ok then
error("failed to connect to Redis: " .. err)
end
end

local ok, err = red:select(db)
if not ok then
error("failed to change Redis database: " .. err)
end

red:flushall()
red:close()
end

describe("Plugin: rate-limiting (integration)", function()

local client

setup(function()
helpers.run_migrations()
end)

teardown(function()
if client then client:close() end
helpers.stop_kong()
end)

describe("Redis conn select database", function()
-- Regression test for the following issue:
-- https://github.com/Kong/kong/issues/3292

setup(function()
flush_redis(REDIS_DATABASE)
flush_redis(REDIS_DATABASE_SECOND)
local api1 = assert(helpers.dao.apis:insert {
name = "redistest1_com",
hosts = { "redistest1.com" },
upstream_url = helpers.mock_upstream_url,
})
assert(helpers.dao.plugins:insert {
name = "rate-limiting",
api_id = api1.id,
config = {
minute = 1,
policy = "redis",
redis_host = REDIS_HOST,
redis_port = REDIS_PORT,
redis_password = REDIS_PASSWORD,
redis_database = REDIS_DATABASE,
fault_tolerant = false
},
})

local api2 = assert(helpers.dao.apis:insert {
name = "redistest2_com",
hosts = { "redistest2.com" },
upstream_url = helpers.mock_upstream_url,
})
assert(helpers.dao.plugins:insert {
name = "rate-limiting",
api_id = api2.id,
config = {
minute = 1,
policy = "redis",
redis_host = REDIS_HOST,
redis_port = REDIS_PORT,
redis_password = REDIS_PASSWORD,
redis_database = REDIS_DATABASE_SECOND,
fault_tolerant = false
}
})
assert(helpers.start_kong({
nginx_conf = "spec/fixtures/custom_nginx.template",
}))
client = helpers.proxy_client()
end)

it("redis conn select databases", function()
local red = redis:new()
red:set_timeout(2000)
assert(red:connect(REDIS_HOST, REDIS_PORT))
if REDIS_PASSWORD and REDIS_PASSWORD ~= "" then
assert(red:auth(REDIS_PASSWORD))
end
assert(red:select(REDIS_DATABASE))
local val1, err = red:dbsize()
if err then
error("failed to call dbsize: " .. err)
end
assert(red:select(REDIS_DATABASE_SECOND))
local val2, err = red:dbsize()
if err then
error("failed to call dbsize: " .. err)
end
assert.are.same(0, tonumber(val1))
assert.are.same(0, tonumber(val2))

local res = assert(client:send {
method = "GET",
path = "/status/200",
headers = {
["Host"] = "redistest1.com"
}
})
assert.res_status(200, res)
ngx.sleep(SLEEP_TIME) -- Wait for async timer to increment the limit

assert(red:select(REDIS_DATABASE))
local val1, err = red:dbsize()
if err then
error("failed to call dbsize: " .. err)
end
assert(red:select(REDIS_DATABASE_SECOND))
local val2, err = red:dbsize()
if err then
error("failed to call dbsize: " .. err)
end
assert.are.same(1, tonumber(val1))
assert.are.same(0, tonumber(val2))

-- rate-limiting plugin reuse api1 redis connection
Copy link
Member

Choose a reason for hiding this comment

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

Curious: did you verify that this test fails with consistency without your patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I verified this test cash get fail if not patched this commit.

spec/03-plugins/24-rate-limiting/05-integration_spec.lua:164: Expected objects to be the same.
Passed in:
(number) 2
Expected:
(number) 1

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for confirming :)

local res = assert(client:send {
method = "GET",
path = "/status/200",
headers = {
["Host"] = "redistest2.com"
}
})
assert.res_status(200, res)
ngx.sleep(SLEEP_TIME) -- Wait for async timer to increment the limit

assert(red:select(REDIS_DATABASE))
local val1, err = red:dbsize()
if err then
error("failed to call dbsize: " .. err)
end
assert(red:select(REDIS_DATABASE_SECOND))
local val2, err = red:dbsize()
if err then
error("failed to call dbsize: " .. err)
end
red:close()
assert.are.same(1, tonumber(val1))
assert.are.same(1, tonumber(val2))
end)
end)
end)
Loading