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
Open

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

Show file tree
Hide file tree
Changes from 16 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
13 changes: 5 additions & 8 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 @@ -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
9 changes: 5 additions & 4 deletions kong/pdk/service/response.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,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 ctx = ngx.ctx
if not ctx.buffered_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 +345,7 @@ local function new(pdk, major_version)
end
end

local body = response.get_raw_body()
local body = ctx.buffered_body or ""
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 +354,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 = ctx.buffered_body or ""
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 +363,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 = ctx.buffered_body or ""

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, headers)
local username, password
local authorization_header = kong.request.get_header(header_name)
local authorization_header = headers[header_name]

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 headers = kong.request.get_headers()

-- If both headers are missing, return 401
if not (kong.request.get_header("authorization") or kong.request.get_header("proxy-authorization")) then
if not (headers["authorization"] or headers["proxy-authorization"]) 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, headers)
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, headers)
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, headers)
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 = headers["origin"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you confirm it is ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a tricky point, here we want to use headers result as param to reduce duplicated get_header call. Although origin header should not be a multiple value logically, but users may still send http http://xxx/yyy Origin:o1 Origin:o2 from downstream which doesn't quite make sense as I see(also I feel like two Content-Type header in one http request is also not quite logically correct...).
But anyway in this case kong.request.get_header("origin") will get only first value o1 and get_headers()["origin"] will get {o1, o2}.
So the get_header API guarantees we only get a single value no matter it's multiple value or not. But in the other side, its pointless to run get_header multiple times since the header has not been changed. Maybe we could just change this to:
type(headers["origin"]) == "table" and headers["origin"][1] or headers["origin"]
which is the implementation to make header value always single(see https://github.com/Kong/kong/blob/master/kong/tools/http.lua#L564).
@bungle @chronolaw

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, headers)
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 = headers["origin"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you confirm it is ok?

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 headers = kong.request.get_headers()
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 headers["Origin"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you confirm it is ok?

or not headers["Access-Control-Request-Method"]
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, headers)
configure_credentials(conf, allow_all, false, headers)

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 = headers["Access-Control-Request-Headers"]
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
headers["Access-Control-Request-Private-Network"] == '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 headers = kong.request.get_headers()
local allow_all = configure_origin(conf, true, headers)
configure_credentials(conf, allow_all, true, headers)

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
18 changes: 9 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 @@ -199,8 +199,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, headers)
local date = headers[date_header_name]
if not date then
return false
end
Expand All @@ -219,14 +219,13 @@ local function validate_clock_skew(date_header_name, allowed_clock_skew)
end


local function validate_body()
local function validate_body(digest_received)
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)
if not digest_received then
-- if there is no digest and no body, it is ok
return body == ""
Expand Down Expand Up @@ -281,8 +280,9 @@ end


local function do_authentication(conf)
local authorization = kong_request.get_header(AUTHORIZATION)
local proxy_authorization = kong_request.get_header(PROXY_AUTHORIZATION)
local headers = kong_request.get_headers()
local authorization = headers[AUTHORIZATION]
local proxy_authorization = headers[PROXY_AUTHORIZATION]
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 +291,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, headers) or
validate_clock_skew(DATE, conf.clock_skew, headers)) then
return false, unauthorized(
"HMAC signature cannot be verified, a valid date or " ..
"x-date header is required for HMAC Authentication",
Expand Down Expand Up @@ -334,7 +334,7 @@ local function do_authentication(conf)
end

-- If request body validation is enabled, then verify digest.
if conf.validate_request_body and not validate_body() then
if conf.validate_request_body and not validate_body(headers[DIGEST]) then
kong.log.debug("digest validation failed")
return false, unauthorized(SIGNATURE_NOT_SAME, www_auth_content)
end
Expand Down
2 changes: 1 addition & 1 deletion kong/plugins/oauth2/access.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,7 +877,7 @@ local function parse_access_token(conf)
parameters[ACCESS_TOKEN] = nil
kong.service.request.set_query(parameters)

local content_type = kong.request.get_header("content-type")
local content_type = ngx.var.http_content_type
chronolaw marked this conversation as resolved.
Show resolved Hide resolved
local is_form_post = content_type and
string_find(content_type, "application/x-www-form-urlencoded", 1, true)

Expand Down
9 changes: 5 additions & 4 deletions kong/plugins/request-transformer/access.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ local table_insert = table.insert
local get_uri_args = kong.request.get_query
local set_uri_args = kong.service.request.set_query
local clear_header = kong.service.request.clear_header
local get_header = kong.request.get_header
local var = ngx.var
chronolaw marked this conversation as resolved.
Show resolved Hide resolved
local set_header = kong.service.request.set_header
local get_headers = kong.request.get_headers
local set_headers = kong.service.request.set_headers
Expand All @@ -31,7 +31,6 @@ local template_cache = setmetatable( {}, { __mode = "k" })

local DEBUG = ngx.DEBUG
local CONTENT_LENGTH = "content-length"
local CONTENT_TYPE = "content-type"
local HOST = "host"
local JSON, MULTI, ENCODED = "json", "multi_part", "form_encoded"
local EMPTY = require("kong.tools.table").EMPTY
Expand Down Expand Up @@ -422,7 +421,8 @@ local function transform_multipart_body(conf, body, content_length, content_type
end

local function transform_body(conf, template_env)
local content_type_value = get_header(CONTENT_TYPE)
-- content-type is a builtin single value header, use var to fetch is faster.
local content_type_value = var.http_content_type
local content_type = get_content_type(content_type_value)
if content_type == nil or #conf.rename.body < 1 and
#conf.remove.body < 1 and #conf.replace.body < 1 and
Expand Down Expand Up @@ -456,7 +456,8 @@ local function transform_method(conf)
if conf.http_method then
set_method(conf.http_method:upper())
if conf.http_method == "GET" or conf.http_method == "HEAD" or conf.http_method == "TRACE" then
local content_type_value = get_header(CONTENT_TYPE)
-- content-type is a builtin single value header, use var to fetch is faster.
local content_type_value = var.http_content_type
local content_type = get_content_type(content_type_value)
if content_type == ENCODED then
-- Also put the body into querystring
Expand Down
Loading
Loading