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(response-ratelimiting): fix missing usage headers for upstream #13696

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

fix(response-ratelimiting): fix missing usage headers for upstream #13696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f178896
fix(response-ratelimiting): fix missing usage headers for upstream
t-yuki Sep 20, 2024
c8cb101
tests(response-ratelimiting): fix tests
t-yuki Sep 21, 2024
9d4e821
tests(response-ratelimiting): fix tests again
t-yuki Oct 15, 2024
4c3b9cd
Update changelog/unreleased/kong/fix-response-ratelimiting-upstream-h…
t-yuki Nov 19, 2024
e1a2aa9
Update changelog/unreleased/kong/fix-response-ratelimiting-upstream-h…
t-yuki Jan 7, 2025
4e8fc5a
Merge remote-tracking branch 'upstream/master' into fix-response-rate…
t-yuki Jan 17, 2025
b98da5e
use second limit for test
t-yuki Jan 17, 2025
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 changelog/unreleased/kong/fix-response-ratelimiting-upstream-headers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
message: "**response-ratelimiting**: fixed an issue in response rate limiting plugin where usage headers ought to be sent to upstream are lost."
type: bugfix
scope: Plugin
10 changes: 1 addition & 9 deletions kong/plugins/response-ratelimiting/access.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
local policies = require "kong.plugins.response-ratelimiting.policies"
local timestamp = require "kong.tools.timestamp"
local pdk_private_rl = require "kong.pdk.private.rate_limiting"


local kong = kong
Expand All @@ -10,10 +9,6 @@ local error = error
local tostring = tostring


local pdk_rl_store_response_header = pdk_private_rl.store_response_header
local pdk_rl_apply_response_headers = pdk_private_rl.apply_response_headers


local EMPTY = require("kong.tools.table").EMPTY
local HTTP_TOO_MANY_REQUESTS = 429
local RATELIMIT_REMAINING = "X-RateLimit-Remaining"
Expand Down Expand Up @@ -89,7 +84,6 @@ function _M.execute(conf)
end

-- Append usage headers to the upstream request. Also checks "block_on_first_violation".
local ngx_ctx = ngx.ctx
for k in pairs(conf.limits) do
local remaining
for _, lv in pairs(usage[k]) do
Expand All @@ -103,11 +97,9 @@ function _M.execute(conf)
end
end

pdk_rl_store_response_header(ngx_ctx, RATELIMIT_REMAINING .. "-" .. k, remaining)
kong.service.request.set_header(RATELIMIT_REMAINING .. "-" .. k, remaining)
end

pdk_rl_apply_response_headers(ngx_ctx)

kong.ctx.plugin.usage = usage -- For later use
end

Expand Down
81 changes: 45 additions & 36 deletions spec/03-plugins/24-response-rate-limiting/04-access_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ for _, strategy in helpers.each_strategy() do
goto continue
end

describe(fmt("#flaky Plugin: response-ratelimiting (access) with policy: #%s #%s [#%s]", redis_conf_name, policy, strategy), function()
describe(fmt("Plugin: response-ratelimiting (access) with policy: #%s #%s [#%s]", redis_conf_name, policy, strategy), function()

lazy_setup(function()
local bp = init_db(strategy, policy)
Expand Down Expand Up @@ -384,15 +384,15 @@ for _, strategy in helpers.each_strategy() do
wait()
local n = math.floor(ITERATIONS / 2)
for _ = 1, n do
local res = proxy_client():get("/response-headers?x-kong-limit=video=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1", {
headers = { Host = "test1.test" },
})
assert.res_status(200, res)
end

ngx.sleep(SLEEP_TIME) -- Wait for async timer to increment the limit

local res = proxy_client():get("/response-headers?x-kong-limit=video=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1", {
headers = { Host = "test1.test" },
})
assert.res_status(200, res)
Expand All @@ -409,7 +409,7 @@ for _, strategy in helpers.each_strategy() do
["-v"] = true,
},
}
assert.truthy(ok)
assert.truthy(ok, res)
assert.matches("x%-ratelimit%-limit%-video%-second: %d+", res)
assert.matches("x%-ratelimit%-remaining%-video%-second: %d+", res)

Expand All @@ -420,21 +420,21 @@ for _, strategy in helpers.each_strategy() do
end)

it("blocks if exceeding limit", function()
test_limit("/response-headers?x-kong-limit=video=1", "test1.test")
test_limit("/response-headers?x-kong-limit=video%3D1", "test1.test")
end)

it("counts against the same service register from different routes", function()
wait()
local n = math.floor(ITERATIONS / 2)
for i = 1, n do
local res = proxy_client():get("/response-headers?x-kong-limit=video=1, test=" .. ITERATIONS, {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1%2C%20test%3D" .. ITERATIONS, {
headers = { Host = "test-service1.test" },
})
assert.res_status(200, res)
end

for i = n+1, ITERATIONS do
local res = proxy_client():get("/response-headers?x-kong-limit=video=1, test=" .. ITERATIONS, {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1%2C%20test%3D" .. ITERATIONS, {
headers = { Host = "test-service2.test" },
})
assert.res_status(200, res)
Expand All @@ -443,7 +443,7 @@ for _, strategy in helpers.each_strategy() do
ngx.sleep(SLEEP_TIME) -- Wait for async timer to increment the list

-- Additional request, while limit is ITERATIONS/second
local res = proxy_client():get("/response-headers?x-kong-limit=video=1, test=" .. ITERATIONS, {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1%2C%20test%3D" .. ITERATIONS, {
headers = { Host = "test-service1.test" },
})
assert.res_status(429, res)
Expand All @@ -457,7 +457,7 @@ for _, strategy in helpers.each_strategy() do
if i == n then
ngx.sleep(SLEEP_TIME) -- Wait for async timer to increment the limit
end
res = proxy_client():get("/response-headers?x-kong-limit=video=2, image=1", {
res = proxy_client():get("/response-headers?x-kong-limit=video%3D2%2C%20image%3D1", {
headers = { Host = "test2.test" },
})
assert.res_status(200, res)
Expand All @@ -471,33 +471,39 @@ for _, strategy in helpers.each_strategy() do
assert.equal(ITERATIONS - n, tonumber(res.headers["x-ratelimit-remaining-image-second"]))

for i = n+1, ITERATIONS do
res = proxy_client():get("/response-headers?x-kong-limit=video=1, image=1", {
if i == ITERATIONS then
ngx.sleep(SLEEP_TIME) -- Wait for async timer to increment the limit
end
res = proxy_client():get("/response-headers?x-kong-limit=video%3D1%2C%20image%3D1", {
headers = { Host = "test2.test" },
})
assert.res_status(200, res)
end
assert.equal(0, tonumber(res.headers["x-ratelimit-remaining-image-second"]))
assert.equal(ITERATIONS * 4 - (n * 2) - (ITERATIONS - n), tonumber(res.headers["x-ratelimit-remaining-video-minute"]))
assert.equal(ITERATIONS * 2 - (n * 2) - (ITERATIONS - n), tonumber(res.headers["x-ratelimit-remaining-video-second"]))

ngx.sleep(SLEEP_TIME) -- Wait for async timer to increment the limit

local res = proxy_client():get("/response-headers?x-kong-limit=video=1, image=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1%2C%20image%3D1", {
headers = { Host = "test2.test" },
})

assert.equal(0, tonumber(res.headers["x-ratelimit-remaining-image-second"]))
assert.equal(ITERATIONS * 4 - (n * 2) - (ITERATIONS - n), tonumber(res.headers["x-ratelimit-remaining-video-minute"]))
assert.equal(ITERATIONS * 2 - (n * 2) - (ITERATIONS - n), tonumber(res.headers["x-ratelimit-remaining-video-second"]))
assert.equal(ITERATIONS * 4 - (n * 2) - (ITERATIONS - n) - 1, tonumber(res.headers["x-ratelimit-remaining-video-minute"]))
assert.equal(ITERATIONS * 2 - (n * 2) - (ITERATIONS - n) - 1, tonumber(res.headers["x-ratelimit-remaining-video-second"]))
assert.res_status(429, res)
end)
end)

describe("With authentication", function()
describe("API-specific plugin", function()
it("blocks if exceeding limit and a per consumer & route setting", function()
test_limit("/response-headers?apikey=apikey123&x-kong-limit=video=1", "test3.test", ITERATIONS - 2)
test_limit("/response-headers?apikey=apikey123&x-kong-limit=video%3D1", "test3.test", ITERATIONS - 2)
end)

it("blocks if exceeding limit and a per route setting", function()
test_limit("/response-headers?apikey=apikey124&x-kong-limit=video=1", "test3.test", ITERATIONS - 3)
test_limit("/response-headers?apikey=apikey124&x-kong-limit=video%3D1", "test3.test", ITERATIONS - 3)
end)
end)
end)
Expand All @@ -513,10 +519,12 @@ for _, strategy in helpers.each_strategy() do
assert.equal(ITERATIONS, tonumber(json.headers["x-ratelimit-remaining-video"]))

-- Actually consume the limits
local res = proxy_client():get("/response-headers?x-kong-limit=video=2, image=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D2%2C%20image%3D1", {
headers = { Host = "test8.test" },
})
assert.res_status(200, res)
local json2 = cjson.decode(assert.res_status(200, res))
assert.equal(ITERATIONS-1, tonumber(json2.headers["x-ratelimit-remaining-image"]))
assert.equal(ITERATIONS, tonumber(json2.headers["x-ratelimit-remaining-video"]))

ngx.sleep(SLEEP_TIME) -- Wait for async timer to increment the limit

Expand All @@ -530,6 +538,7 @@ for _, strategy in helpers.each_strategy() do

it("combines multiple x-kong-limit headers from upstream", function()
wait()
-- NOTE: this test is not working as intended because multiple response headers are merged into one comma-joined header by send_text_response function
for _ = 1, ITERATIONS do
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D2&x-kong-limit=image%3D1", {
headers = { Host = "test4.test" },
Expand All @@ -549,25 +558,25 @@ for _, strategy in helpers.each_strategy() do

assert.res_status(429, res)
assert.equal(0, tonumber(res.headers["x-ratelimit-remaining-image-second"]))
assert.equal(1, tonumber(res.headers["x-ratelimit-remaining-video-second"]))
assert.equal(0, tonumber(res.headers["x-ratelimit-remaining-video-second"]))
end)
end)

it("should block on first violation", function()
wait()
local res = proxy_client():get("/response-headers?x-kong-limit=video=2, image=4", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D2%2C%20image%3D4", {
headers = { Host = "test7.test" },
})
assert.res_status(200, res)

ngx.sleep(SLEEP_TIME) -- Wait for async timer to increment the limit

local res = proxy_client():get("/response-headers?x-kong-limit=video=2", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D2", {
headers = { Host = "test7.test" },
})
local body = assert.res_status(429, res)
local json = cjson.decode(body)
assert.same({ message = "API rate limit exceeded for 'image'" }, json)
assert.matches("API rate limit exceeded for 'image'", json.message)
end)

describe("Config with hide_client_headers", function()
Expand All @@ -584,7 +593,7 @@ for _, strategy in helpers.each_strategy() do
end)
end)

describe(fmt("#flaky Plugin: response-ratelimiting (expirations) with policy: #%s #%s [#%s]", redis_conf_name, policy, strategy), function()
describe(fmt("Plugin: response-ratelimiting (expirations) with policy: #%s #%s [#%s]", redis_conf_name, policy, strategy), function()

lazy_setup(function()
local bp = init_db(strategy, policy)
Expand Down Expand Up @@ -624,7 +633,7 @@ for _, strategy in helpers.each_strategy() do

it("expires a counter", function()
wait()
local res = proxy_client():get("/response-headers?x-kong-limit=video=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1", {
headers = { Host = "expire1.test" },
})

Expand All @@ -637,7 +646,7 @@ for _, strategy in helpers.each_strategy() do
ngx.sleep(0.01)
wait() -- Wait for counter to expire

local res = proxy_client():get("/response-headers?x-kong-limit=video=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1", {
headers = { Host = "expire1.test" },
})

Expand All @@ -649,7 +658,7 @@ for _, strategy in helpers.each_strategy() do
end)
end)

describe(fmt("#flaky Plugin: response-ratelimiting (access - global for single consumer) with policy: #%s #%s [#%s]", redis_conf_name, policy, strategy), function()
describe(fmt("Plugin: response-ratelimiting (access - global for single consumer) with policy: #%s #%s [#%s]", redis_conf_name, policy, strategy), function()

lazy_setup(function()
local bp = init_db(strategy, policy)
Expand Down Expand Up @@ -700,11 +709,11 @@ for _, strategy in helpers.each_strategy() do
end)

it("blocks when the consumer exceeds their quota, no matter what service/route used", function()
test_limit("/response-headers?apikey=apikey126&x-kong-limit=video=1", "test%d.test")
test_limit("/response-headers?apikey=apikey126&x-kong-limit=video%3D1", "test%d.test")
end)
end)

describe(fmt("#flaky Plugin: response-ratelimiting (access - global) with policy: #%s #%s [#%s]", redis_conf_name, policy, strategy), function()
describe(fmt("Plugin: response-ratelimiting (access - global) with policy: #%s #%s [#%s]", redis_conf_name, policy, strategy), function()

lazy_setup(function()
local bp = init_db(strategy, policy)
Expand Down Expand Up @@ -749,7 +758,7 @@ for _, strategy in helpers.each_strategy() do
it("blocks if exceeding limit", function()
wait()
for i = 1, ITERATIONS do
local res = proxy_client():get("/response-headers?x-kong-limit=video=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1", {
headers = { Host = fmt("test%d.test", i) },
})
assert.res_status(200, res)
Expand All @@ -758,7 +767,7 @@ for _, strategy in helpers.each_strategy() do
ngx.sleep(SLEEP_TIME) -- Wait for async timer to increment the limit

-- last query, while limit is ITERATIONS/second
local res = proxy_client():get("/response-headers?x-kong-limit=video=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1", {
headers = { Host = "test1.test" },
})
assert.res_status(429, res)
Expand All @@ -767,7 +776,7 @@ for _, strategy in helpers.each_strategy() do
end)
end)

describe(fmt("#flaky Plugin: response-ratelimiting (fault tolerance) with policy: #%s #%s [#%s]", redis_conf_name, policy, strategy), function()
describe(fmt("Plugin: response-ratelimiting (fault tolerance) with policy: #%s #%s [#%s]", redis_conf_name, policy, strategy), function()
if policy == "cluster" then
local bp, db

Expand Down Expand Up @@ -832,7 +841,7 @@ for _, strategy in helpers.each_strategy() do
end)

it("does not work if an error occurs", function()
local res = proxy_client():get("/response-headers?x-kong-limit=video=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1", {
headers = { Host = "failtest1.test" },
})
assert.res_status(200, res)
Expand All @@ -846,16 +855,16 @@ for _, strategy in helpers.each_strategy() do
-- affecting subsequent tests.

-- Make another request
local res = proxy_client():get("/response-headers?x-kong-limit=video=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1", {
headers = { Host = "failtest1.test" },
})
local body = assert.res_status(500, res)
local json = cjson.decode(body)
assert.same({ message = "An unexpected error occurred" }, json)
assert.matches("An unexpected error occurred", json.message)
end)

it("keeps working if an error occurs", function()
local res = proxy_client():get("/response-headers?x-kong-limit=video=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1", {
headers = { Host = "failtest2.test" },
})
assert.res_status(200, res)
Expand All @@ -869,7 +878,7 @@ for _, strategy in helpers.each_strategy() do
-- affecting subsequent tests.

-- Make another request
local res = proxy_client():get("/response-headers?x-kong-limit=video=1", {
local res = proxy_client():get("/response-headers?x-kong-limit=video%3D1", {
headers = { Host = "failtest2.test" },
})
assert.res_status(200, res)
Expand Down Expand Up @@ -940,7 +949,7 @@ for _, strategy in helpers.each_strategy() do
})
local body = assert.res_status(500, res)
local json = cjson.decode(body)
assert.same({ message = "An unexpected error occurred" }, json)
assert.matches("An unexpected error occurred", json.message)
end)
it("keeps working if an error occurs", function()
-- Make another request
Expand Down