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

perf(pdk): refine with optional cache to improvement performance #13981

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
+58 −43
Open

perf(pdk): refine with optional cache to improvement performance #13981

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c4aebf1
use ngx header to avoid get_headers
ProBrian Dec 3, 2024
e5626cd
use ngx header to avoid get_headers for kong.response
ProBrian Dec 3, 2024
7c9ea30
use ctx to get single header in basic auth plugin
ProBrian Dec 3, 2024
ca17150
use ctx to get single header in basic auth plugin fix get_header param
ProBrian Dec 4, 2024
e0dc602
patch read_body to reduce duplicate read_body for the same request
ProBrian Dec 4, 2024
78cb953
replace no ctx request.get_header in request-transformer by ngx.var.h…
ProBrian Dec 4, 2024
e46dffb
localise ngx.var in request-transformer
ProBrian Dec 4, 2024
ec8abf2
use ctx for some var in request pdk
ProBrian Dec 4, 2024
9c7520a
use request.get_header with ctx in proper plugins
ProBrian Dec 4, 2024
f4ce86a
use input param to reduce ctx read in get_body, and one more http_con…
ProBrian Dec 5, 2024
bcdc538
use single header get in service.request.set_body
ProBrian Dec 5, 2024
40f7218
fix aws plugin and get header test cases
ProBrian Dec 5, 2024
71e2a62
refine annotatios
ProBrian Dec 13, 2024
d5c781b
remove custom plugin for test
ProBrian Dec 13, 2024
a64667b
remove unused constant
ProBrian Dec 16, 2024
f140e01
revert interface change
ProBrian Jan 21, 2025
16b159c
perf(pdk):minor refinement on pdk usage
ProBrian Feb 7, 2025
b40a178
perf(pdk):change naming and localize method
ProBrian Feb 7, 2025
d21ef10
feat(pdk): revert var.http_content_type change due to behavior incons…
ProBrian Feb 12, 2025
edd040a
feat(pdk): keep lower case header name change
ProBrian Feb 12, 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
10 changes: 10 additions & 0 deletions kong/globalpatches.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ return function(options)
local get_uri_args = ngx.req.get_uri_args
local get_post_args = ngx.req.get_post_args
local decode_args = ngx.decode_args
local read_req_body = ngx.req.read_body

local DEFAULT_MAX_REQ_HEADERS = 100
local DEFAULT_MAX_RESP_HEADERS = 100
Expand Down Expand Up @@ -232,6 +233,15 @@ return function(options)
return decode_args_real(str, max_args or MAX_DECODE_ARGS, ...)
end
-- ]

-- READ REQUEST BODY [
_G.ngx.req.read_body = function()
-- for the same request, only one `read_body` call is needed
if not ngx.ctx.body_read then
chronolaw marked this conversation as resolved.
Show resolved Hide resolved
read_req_body()
bungle marked this conversation as resolved.
Show resolved Hide resolved
ngx.ctx.body_read = true
end
end
chronolaw marked this conversation as resolved.
Show resolved Hide resolved
end
end

Expand Down
17 changes: 7 additions & 10 deletions kong/pdk/request.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@ local function new(self)
local MIN_PORT = 1
local MAX_PORT = 65535

local CONTENT_TYPE = "Content-Type"

local CONTENT_TYPE_POST = "application/x-www-form-urlencoded"
local CONTENT_TYPE_JSON = "application/json"
local CONTENT_TYPE_FORM_DATA = "multipart/form-data"
Expand Down Expand Up @@ -98,7 +96,7 @@ local function new(self)
function _REQUEST.get_scheme()
check_phase(PHASES.request)

return var.scheme
return ngx.ctx.scheme or var.scheme
bungle marked this conversation as resolved.
Show resolved Hide resolved
end


Expand All @@ -116,7 +114,7 @@ local function new(self)
function _REQUEST.get_host()
check_phase(PHASES.request)

return var.host
return ngx.ctx.host or var.host
end


Expand Down Expand Up @@ -437,7 +435,7 @@ local function new(self)
function _REQUEST.get_raw_path()
check_phase(PHASES.request)

local uri = var.request_uri or ""
local uri = ngx.ctx.request_uri or var.request_uri or ""
local s = find(uri, "?", 2, true)
return s and sub(uri, 1, s - 1) or uri
end
Expand All @@ -456,7 +454,7 @@ local function new(self)
-- kong.request.get_path_with_query() -- "/v1/movies?movie=foo"
function _REQUEST.get_path_with_query()
check_phase(PHASES.request)
return var.request_uri or ""
return ngx.ctx.request_uri or var.request_uri or ""
end


Expand Down Expand Up @@ -619,14 +617,14 @@ local function new(self)
-- kong.request.get_header("Host") -- "foo.com"
-- kong.request.get_header("x-custom-header") -- "bla"
-- kong.request.get_header("X-Another") -- "foo bar"
function _REQUEST.get_header(name)
function _REQUEST.get_header(name, ctx)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

http_get_header supports ctx param to cache headers, expose it to get_header API so we could use cache.

Copy link
Member

@bungle bungle Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't like this either. If we add this parameter, we need to always keep the API like that, that the second parameter needs to be ctx. The benefits are so small that I would rather not do it. The most of the changes in plugins seems to be utilizing this. And that is also I think somewhat "bad" code to make people create tables of ctx and pass it here. If we cannot find better way to do this, then I would not do it.

Just an example e.g. if someone wants to add more beneficial parameter lilke default in case the header is missing the code would become:

local value = kong.request.get_header("Some-Header", nil, "default")

Perhaps we will never add that either, but just tells that we don't want these strange ctx parameters in public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if we use kong.tools.get_header(name, ctx) directly in plugins, to replace non-cached kong.request.get_header API in certain places to achieve the cache benefit? It must be better to use PDK APIs in plugins, but I do see that there are a lot of kong.tools usage in plugins(which I feel more like should be used "internally" in core or some non-plugin module?), Do you think it's a proper practice?

Copy link
Member

Choose a reason for hiding this comment

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

@ProBrian In general it would be best if we can get to state where there is no require "module" statements in plugins. That is we have a powerful PDK. But meanwhile, yes, many plugins require different modules. That also then spreads to all custom plugins as plugins are copy pasted. That then will make all our internal APIs and tools public too which we cannot break. I think for example those plugins, just call get_headers once and work with headers and call it done, rather than pass a table to somewhere that something else caches the get_headers.

check_phase(PHASES.request)

if type(name) ~= "string" then
error("header name must be a string", 2)
end

return http_get_header(name)
return http_get_header(name, ctx)
end


Expand Down Expand Up @@ -820,8 +818,7 @@ local function new(self)
-- body.age -- "42"
function _REQUEST.get_body(mimetype, max_args, max_allowed_file_size)
check_phase(before_content)

local content_type = mimetype or _REQUEST.get_header(CONTENT_TYPE)
local content_type = mimetype or ngx.var.http_content_type
bungle marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

@ProBrian

ngx.var.content_type does not seem to be single header. If you send multiple, you will get "<value1>, <value2>, ...".

Copy link
Member

@bungle bungle Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What is even more annoying is that:

ngx.req.set_header("Content-Type", "value")

is only reflected with the first value. Seems like clear_header is needed together with set_header?

Copy link
Member

Choose a reason for hiding this comment

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

This seems to work properly:

local ct = kong.request.get_header("Content-Type")
ngx.req.clear_header("Content-Type")
ngx.req.set_header("Content-Type", ct)

If I don't do clear_header, it does not work properly.

Copy link
Member

Choose a reason for hiding this comment

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

This is the request I am using:

http -v :8001 Content-Type:text/html Content-Type:text/json

Copy link
Contributor Author

@ProBrian ProBrian Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Seems there's a mismatch limitation between openresty and nginx that I've ignored, that is,

  • nginx allows multiple line values for headers like "Content-Type".

  • And openresty defines a list of headers that is "built-in single value", which includes "Content-Type". ngx.req.set_header will not allow multiple value for "Content-Type", so if we ngx.req.set_header("Content-Type", {"json1", "json2"}) , it will use the last elem as the single value, so then ngx.req.get_headers()["Content-Type"] and ngx.var.http_content_type will all get a single value "json2" .

  • But when multiple content type header is sent by http request client, not by openresty set_header API, then the header value is just handled by nginx, and get_headers will get a table {"json1", "json2"} and ngx.var.http_content_type will get the value as comma separated string: "json1, json2".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is even more annoying is that:

ngx.req.set_header("Content-Type", "value")

is only reflected with the first value. Seems like clear_header is needed together with set_header?

That's due to the implementation of openresty, it assumes that Content-Type should be single value, so in set_header implementation, the assignment will only take place in the first index of value list.

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 will rollback the var.http_content_type changes, since kong.request.get_header will only get the first value no matter content-type is multi value or not from downstream; but var.http_content_type rely on the assumption that downstream send single content-type or content-type is set by ngx.req.set_header.

if not content_type then
return nil, "missing content type"
end
Expand Down
2 changes: 1 addition & 1 deletion kong/pdk/response.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ local function new(self, major_version)
error("header name must be a string", 2)
end

local header_value = _RESPONSE.get_headers()[name]
local header_value = ngx.header[name]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep the original code here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ngx.header is the better way to get a single response header, and it returns the same type of values as get_headers()[name]: if get_headers()[name] get a table, ngx.header will get a table similarly(unlike ngx.var.sent_http_ which returns comma separated string). So we make this change for better performance.

if type(header_value) == "table" then
return header_value[1]
end
Expand Down
2 changes: 1 addition & 1 deletion kong/pdk/service/request.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ local function new(self)
error("mime must be a string", 2)
end
if not mime then
mime = ngx.req.get_headers()[CONTENT_TYPE]
mime = ngx_var.http_content_type
if not mime then
return nil, "content type was neither explicitly given " ..
"as an argument or received as a header"
Expand Down
14 changes: 8 additions & 6 deletions kong/pdk/service/response.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,10 +285,11 @@ local function new(pdk, major_version)
-- -- or `access` phase prior calling this function.
--
-- local body = kong.service.response.get_raw_body()
function response.get_raw_body()
function response.get_raw_body(buffered_proxying)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just a minor refinement so that if we could reduce the reads of ngx.ctx.buffered_proxying by passing it by param

check_phase(header_body_log)
local ctx = ngx.ctx
if not ctx.buffered_proxying then
local buffered = buffered_proxying or ctx.buffered_proxying
ProBrian marked this conversation as resolved.
Show resolved Hide resolved
if not buffered then
error("service body is only available with buffered proxying " ..
"(see: kong.service.request.enable_buffering function)", 2)
end
Expand All @@ -313,7 +314,8 @@ local function new(pdk, major_version)
-- local body = kong.service.response.get_body()
function response.get_body(mimetype, max_args)
check_phase(header_body_log)
if not ngx.ctx.buffered_proxying then
local bufferred_proxying = ngx.ctx.buffered_proxying
if not bufferred_proxying then
error("service body is only available with buffered proxying " ..
"(see: kong.service.request.enable_buffering function)", 2)
end
Expand Down Expand Up @@ -344,7 +346,7 @@ local function new(pdk, major_version)
end
end

local body = response.get_raw_body()
local body = response.get_raw_body(bufferred_proxying)
local pargs, err = ngx.decode_args(body, max_args or MAX_POST_ARGS_DEFAULT)
if not pargs then
return nil, err, CONTENT_TYPE_POST
Expand All @@ -353,7 +355,7 @@ local function new(pdk, major_version)
return pargs, nil, CONTENT_TYPE_POST

elseif find(content_type_lower, CONTENT_TYPE_JSON, 1, true) == 1 then
local body = response.get_raw_body()
local body = response.get_raw_body(bufferred_proxying)
local json = cjson.decode_with_array_mt(body)
if type(json) ~= "table" then
return nil, "invalid json body", CONTENT_TYPE_JSON
Expand All @@ -362,7 +364,7 @@ local function new(pdk, major_version)
return json, nil, CONTENT_TYPE_JSON

elseif find(content_type_lower, CONTENT_TYPE_FORM_DATA, 1, true) == 1 then
local body = response.get_raw_body()
local body = response.get_raw_body(bufferred_proxying)

local parts = multipart(body, content_type)
if not parts then
Expand Down
2 changes: 1 addition & 1 deletion kong/plugins/aws-lambda/request-util.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ local function build_request_payload(conf)
end

if conf.forward_request_body then
local content_type = kong.request.get_header("content-type")
local content_type = ngx.var.http_content_type
local body_raw = read_request_body(conf.skip_large_bodies)
local body_args, err = kong.request.get_body()
if err and err:match("content type") then
Expand Down
11 changes: 6 additions & 5 deletions kong/plugins/basic-auth/access.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ local _M = {}
-- @param {table} conf Plugin config
-- @return {string} public_key
-- @return {string} private_key
local function retrieve_credentials(header_name, conf)
local function retrieve_credentials(header_name, conf, ctx)
local username, password
local authorization_header = kong.request.get_header(header_name)
local authorization_header = kong.request.get_header(header_name, ctx)

if authorization_header then
local iterator, iter_err = re_gmatch(authorization_header, "\\s*[Bb]asic\\s*(.+)", "oj")
Expand Down Expand Up @@ -158,21 +158,22 @@ end

local function do_authentication(conf)
local www_authenticate = "Basic realm=\"" .. conf.realm .. "\""
local ctx = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use a local ctx here which is cheaper thanngx.ctx, and the cache is only fresh in the local ctx lifetime, not in the request lifetime, so we don't need ngx.ctx


-- If both headers are missing, return 401
if not (kong.request.get_header("authorization") or kong.request.get_header("proxy-authorization")) then
if not (kong.request.get_header("authorization", ctx) or kong.request.get_header("proxy-authorization", ctx)) then
return false, unauthorized("Unauthorized", www_authenticate)
end

local credential
local given_username, given_password = retrieve_credentials("proxy-authorization", conf)
local given_username, given_password = retrieve_credentials("proxy-authorization", conf, ctx)
if given_username and given_password then
credential = load_credential_from_db(given_username)
end

-- Try with the authorization header
if not credential then
given_username, given_password = retrieve_credentials("authorization", conf)
given_username, given_password = retrieve_credentials("authorization", conf, ctx)
if given_username and given_password then
credential = load_credential_from_db(given_username)
else
Expand Down
26 changes: 14 additions & 12 deletions kong/plugins/cors/handler.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ local function add_vary_header(header_filter)
end
end

local function configure_origin(conf, header_filter)
local function configure_origin(conf, header_filter, ctx)
local n_origins = conf.origins ~= nil and #conf.origins or 0
local set_header = kong.response.set_header

Expand Down Expand Up @@ -89,7 +89,7 @@ local function configure_origin(conf, header_filter)
end
end

local req_origin = kong.request.get_header("origin")
local req_origin = kong.request.get_header("origin", ctx)
if req_origin then
local cached_domains = config_cache[conf]
if not cached_domains then
Expand Down Expand Up @@ -167,7 +167,7 @@ local function configure_origin(conf, header_filter)
end


local function configure_credentials(conf, allow_all, header_filter)
local function configure_credentials(conf, allow_all, header_filter, ctx)
local set_header = kong.response.set_header

if not conf.credentials then
Expand All @@ -181,7 +181,7 @@ local function configure_credentials(conf, allow_all, header_filter)

-- Access-Control-Allow-Origin is '*', must change it because ACAC cannot
-- be 'true' if ACAO is '*'.
local req_origin = kong.request.get_header("origin")
local req_origin = kong.request.get_header("origin", ctx)
if req_origin then
add_vary_header(header_filter)
set_header("Access-Control-Allow-Origin", req_origin)
Expand All @@ -191,9 +191,10 @@ end


function CorsHandler:access(conf)
local ctx = {}
if kong.request.get_method() ~= "OPTIONS"
or not kong.request.get_header("Origin")
or not kong.request.get_header("Access-Control-Request-Method")
or not kong.request.get_header("Origin", ctx)
or not kong.request.get_header("Access-Control-Request-Method", ctx)
then
return
end
Expand All @@ -207,16 +208,16 @@ function CorsHandler:access(conf)
return
end

local allow_all = configure_origin(conf, false)
configure_credentials(conf, allow_all, false)
local allow_all = configure_origin(conf, false, ctx)
configure_credentials(conf, allow_all, false, ctx)

local set_header = kong.response.set_header

if conf.headers and #conf.headers > 0 then
set_header("Access-Control-Allow-Headers", concat(conf.headers, ","))

else
local acrh = kong.request.get_header("Access-Control-Request-Headers")
local acrh = kong.request.get_header("Access-Control-Request-Headers", ctx)
if acrh then
set_header("Access-Control-Allow-Headers", acrh)
else
Expand All @@ -234,7 +235,7 @@ function CorsHandler:access(conf)
end

if conf.private_network and
kong.request.get_header("Access-Control-Request-Private-Network") == 'true' then
kong.request.get_header("Access-Control-Request-Private-Network", ctx) == 'true' then
set_header("Access-Control-Allow-Private-Network", 'true')
end

Expand All @@ -247,8 +248,9 @@ function CorsHandler:header_filter(conf)
return
end

local allow_all = configure_origin(conf, true)
configure_credentials(conf, allow_all, true)
local ctx = {}
local allow_all = configure_origin(conf, true, ctx)
configure_credentials(conf, allow_all, true, ctx)

if conf.exposed_headers and #conf.exposed_headers > 0 then
kong.response.set_header("Access-Control-Expose-Headers",
Expand Down
3 changes: 1 addition & 2 deletions kong/plugins/grpc-web/handler.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ local ngx_arg = ngx.arg
local ngx_var = ngx.var

local kong_request_get_path = kong.request.get_path
local kong_request_get_header = kong.request.get_header
local kong_request_get_method = kong.request.get_method
local kong_request_get_raw_body = kong.request.get_raw_body
local kong_response_exit = kong.response.exit
Expand Down Expand Up @@ -51,7 +50,7 @@ function grpc_web:access(conf)
end

local dec, err = deco.new(
kong_request_get_header("Content-Type"),
ngx.var.http_content_type,
chronolaw marked this conversation as resolved.
Show resolved Hide resolved
uri, conf.proto)

if not dec then
Expand Down
20 changes: 11 additions & 9 deletions kong/plugins/hmac-auth/access.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,10 @@ local function create_hash(request_uri, hmac_params)
local hmac_headers = hmac_params.hmac_headers

local count = #hmac_headers
local ctx = {}
for i = 1, count do
local header = hmac_headers[i]
local header_value = kong.request.get_header(header)
local header_value = kong.request.get_header(header, ctx)

if not header_value then
if header == "@request-target" then
Expand Down Expand Up @@ -199,8 +200,8 @@ local function load_credential(username)
end


local function validate_clock_skew(date_header_name, allowed_clock_skew)
local date = kong_request.get_header(date_header_name)
local function validate_clock_skew(date_header_name, allowed_clock_skew, ctx)
local date = kong_request.get_header(date_header_name, ctx)
if not date then
return false
end
Expand All @@ -219,14 +220,14 @@ local function validate_clock_skew(date_header_name, allowed_clock_skew)
end


local function validate_body()
local function validate_body(ctx)
local body, err = kong_request.get_raw_body()
if err then
kong.log.debug(err)
return false
end

local digest_received = kong_request.get_header(DIGEST)
local digest_received = kong_request.get_header(DIGEST, ctx)
if not digest_received then
-- if there is no digest and no body, it is ok
return body == ""
Expand Down Expand Up @@ -281,8 +282,9 @@ end


local function do_authentication(conf)
local authorization = kong_request.get_header(AUTHORIZATION)
local proxy_authorization = kong_request.get_header(PROXY_AUTHORIZATION)
local ctx = {}
local authorization = kong_request.get_header(AUTHORIZATION, ctx)
local proxy_authorization = kong_request.get_header(PROXY_AUTHORIZATION, ctx)
local www_auth_content = conf.realm and fmt('hmac realm="%s"', conf.realm) or 'hmac'

-- If both headers are missing, return 401
Expand All @@ -291,8 +293,8 @@ local function do_authentication(conf)
end

-- validate clock skew
if not (validate_clock_skew(X_DATE, conf.clock_skew) or
validate_clock_skew(DATE, conf.clock_skew)) then
if not (validate_clock_skew(X_DATE, conf.clock_skew, ctx) or
validate_clock_skew(DATE, conf.clock_skew, ctx)) then
return false, unauthorized(
"HMAC signature cannot be verified, a valid date or " ..
"x-date header is required for HMAC Authentication",
Expand Down
5 changes: 3 additions & 2 deletions kong/plugins/ldap-auth/access.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,9 @@ local function unauthorized(message, authorization_scheme)
end

local function do_authentication(conf)
local authorization_value = kong.request.get_header(AUTHORIZATION)
local proxy_authorization_value = kong.request.get_header(PROXY_AUTHORIZATION)
local ctx = {}
local authorization_value = kong.request.get_header(AUTHORIZATION, ctx)
local proxy_authorization_value = kong.request.get_header(PROXY_AUTHORIZATION, ctx)

local scheme = conf.header_type
if scheme == "ldap" then
Expand Down
Loading
Loading