Skip to content

Commit

Permalink
fix(pdk): service.response.get_headers behavior
Browse files Browse the repository at this point in the history 
This commit attempts to fix several issues in the
`kong.service.response.get_headers()` module.

Issues
======

1. Said method must only return response headers set by the upstream
   service. However, since its introcution, it is broken; in the
   original implementation, it added an `__index` metamethod to the full headers
   table (both Kong and upstream headers) to ensure a given header being indexed
   was found in a `ngx.var.upstream_http_<header_name> variable (ie, it was
   set by the upstream). However, it responded with the table itself, which
   contained the headers, so the `__index` metamethod would never be called.
   Summarizing, since its introduction, the method has been returning
   both Kong and upstream headers. Tests were also broken due to a
   case-sensitiveness issue (implementation described above bypased the
   original headers table metatable, effectively breaking
   case-insensitiveness implemented by the `ngx.resp.get_headers` API).

2. Buffered proxying, later added, introduced new logic to handle
   buffered headers. This logic corretly implemented the metamethod by
   using a so-called "proxy table"; this is an empty table to which the
   `__index` metamethod is added. Since it's empty, `__index` is
   correctly invoked and ensures only upstream headers are accessible
   (as described by 1.). However, being empty it does not allow the
   headers table to be iterated on, leading to inconsistent and
   unexpected behavior -- a source of bug reports (e.g., #11546, KAG-4866,
   to name two).

What does it change?
====================

1. Standardize the interface: both buffered and unbuffered respond
   with an iterable table.
2. Add more tests, for both buffered and unbuffered modes
    - test that returned table is indeed iterable
    - test that returned table only has upstream headers

Caveats
=======

- Buffered proxying already reads all headers; adding additional logic
  to limit by `max_headers` arguably does not make sense.
- Calls to this method will always respond with less than `max_headers`,
  as the call to `ngx.resp.get_headers` includes both Kong and upstream
  headers and we filter out non-upstream headers after said call. The
  ideal solution would use a native API that reads upstream only headers
  directly, and has a similar interface as `ngx.resp.get_headers`,
  taking a `max_headers` argument.
  • Loading branch information
@gszr
gszr committed Nov 3, 2024
1 parent 5cab556 commit 659bb66
Show file tree
Hide file tree
Showing 2 changed files with 390 additions and 114 deletions.
119 changes: 21 additions & 98 deletions kong/pdk/service/response.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,90 +36,25 @@ local header_body_log = phase_checker.new(PHASES.response,
PHASES.body_filter,
PHASES.log)


local attach_resp_headers_mt

local clear_non_upstream_headers

do
local resp_headers_orig_mt_index


local resp_headers_mt = {
__index = function(t, name)
if type(name) == "string" then
local var = fmt("upstream_http_%s", replace_dashes_lower(name))
if not ngx.var[var] then
return nil
end
-- pairs in a hot code path - although this hits an NYI and falls back to the
-- interpreter, it uses an optimized hand-written assembly form of loop on table
-- that does not use `next`
clear_non_upstream_headers = function (response_headers, err)
for header_name, _ in pairs(response_headers) do
local var = fmt("upstream_http_%s", replace_dashes_lower(header_name))
if not ngx.var[var] then
response_headers[header_name] = nil
end

return resp_headers_orig_mt_index(t, name)
end,
}


attach_resp_headers_mt = function(response_headers, err)
if not resp_headers_orig_mt_index then
local mt = getmetatable(response_headers)
resp_headers_orig_mt_index = mt.__index
end

setmetatable(response_headers, resp_headers_mt)

return response_headers, err
end
end


local attach_buffered_headers_mt

do
local EMPTY = {}

attach_buffered_headers_mt = function(response_headers, max_headers)
if not response_headers then
return EMPTY
end

return setmetatable({}, { __index = function(_, name)
if type(name) ~= "string" then
return nil
end

if response_headers[name] then
return response_headers[name]
end

name = lower(name)

if response_headers[name] then
return response_headers[name]
end

name = replace_dashes(name)

if response_headers[name] then
return response_headers[name]
end

local i = 1
for n, v in pairs(response_headers) do
if i > max_headers then
return nil
end

n = replace_dashes_lower(n)
if n == name then
return v
end

i = i + 1
end
end })
end
end


local function new(pdk, major_version)
local response = {}

Expand All @@ -137,8 +72,8 @@ local function new(pdk, major_version)
local MIN_HEADERS = 1
local MAX_HEADERS_DEFAULT = 100
local MAX_HEADERS = 1000
local MAX_HEADERS_CONFIGURED

local MAX_HEADERS_CONFIGURED = pdk.configuration and pdk.configuration.lua_max_resp_headers
or MAX_HEADERS_DEFAULT

---
-- Returns the HTTP status code of the response from the Service as a Lua number.
Expand Down Expand Up @@ -196,39 +131,27 @@ local function new(pdk, major_version)
-- kong.log.inspect(headers.x_another[1]) -- "foo bar"
-- kong.log.inspect(headers["X-Another"][2]) -- "baz"
-- end
-- Note that this function returns a proxy table
-- which cannot be iterated with `pairs` or used as operand of `#`.
function response.get_headers(max_headers)
check_phase(header_body_log)

local buffered_headers = ngx.ctx.buffered_headers

if max_headers == nil then
if buffered_headers then
if not MAX_HEADERS_CONFIGURED then
MAX_HEADERS_CONFIGURED = pdk and pdk.configuration and pdk.configuration.lua_max_resp_headers
end
return attach_buffered_headers_mt(buffered_headers, MAX_HEADERS_CONFIGURED or MAX_HEADERS_DEFAULT)
end

return attach_resp_headers_mt(ngx.resp.get_headers())
end
if max_headers ~= nil and type(max_headers) ~= "number" then
error("max_headers must be a number or nil", 2)

if type(max_headers) ~= "number" then
error("max_headers must be a number", 2)

elseif max_headers < MIN_HEADERS then
elseif max_headers and max_headers < MIN_HEADERS then
error("max_headers must be >= " .. MIN_HEADERS, 2)

elseif max_headers > MAX_HEADERS then
elseif max_headers and max_headers > MAX_HEADERS then
error("max_headers must be <= " .. MAX_HEADERS, 2)
end

if buffered_headers then
return attach_buffered_headers_mt(buffered_headers, max_headers)
local ctx = ngx.ctx
if ctx.buffered_proxying and ctx.buffered_headers then
-- XXX ensure metatable is the same as for non-buffered
return ctx.buffered_headers
end

return attach_resp_headers_mt(ngx.resp.get_headers(max_headers))
-- we patch ngx.resp.get_headers to honor the max headers kong config
return clear_non_upstream_headers(ngx.resp.get_headers(max_headers))
end

---
Expand Down
Loading

0 comments on commit 659bb66

Please sign in to comment.