diff --git a/CHANGELOG.md b/CHANGELOG.md index 0db935bf428..76059b96066 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -139,6 +139,11 @@ - Fix an issue where empty value of URI argument `custom_id` crashes `/consumer`. [#10475](https://github.com/Kong/kong/pull/10475) +#### Plugins +- **Request-Transformer**: fix an issue where requests would intermittently + be proxied with incorrect query parameters. + [10539](https://github.com/Kong/kong/pull/10539) + ### Changed #### Core diff --git a/kong/plugins/request-transformer/access.lua b/kong/plugins/request-transformer/access.lua index 982b210d59f..f0772995025 100644 --- a/kong/plugins/request-transformer/access.lua +++ b/kong/plugins/request-transformer/access.lua @@ -26,7 +26,6 @@ local pl_copy_table = pl_tablex.deepcopy local _M = {} local template_cache = setmetatable( {}, { __mode = "k" }) -local template_environment local DEBUG = ngx.DEBUG local CONTENT_LENGTH = "content-length" @@ -70,54 +69,7 @@ local function get_content_type(content_type) end end --- meta table for the sandbox, exposing lazily loaded values -local __meta_environment = { - __index = function(self, key) - local lazy_loaders = { - headers = function(self) - return get_headers() or EMPTY - end, - query_params = function(self) - return get_uri_args() or EMPTY - end, - uri_captures = function(self) - return (ngx.ctx.router_matches or EMPTY).uri_captures or EMPTY - end, - shared = function(self) - return ((kong or EMPTY).ctx or EMPTY).shared or EMPTY - end, - } - local loader = lazy_loaders[key] - if not loader then - -- we don't have a loader, so just return nothing - return - end - -- set the result on the table to not load again - local value = loader() - rawset(self, key, value) - return value - end, - __newindex = function(self) - error("This environment is read-only.") - end, -} - -template_environment = setmetatable({ - -- here we can optionally add functions to expose to the sandbox, eg: - -- tostring = tostring, -- for example - -- because headers may contain array elements such as duplicated headers - -- type is a useful function in these cases. See issue #25. - type = type, -}, __meta_environment) - -local function clear_environment(conf) - rawset(template_environment, "headers", nil) - rawset(template_environment, "query_params", nil) - rawset(template_environment, "uri_captures", nil) - rawset(template_environment, "shared", nil) -end - -local function param_value(source_template, config_array) +local function param_value(source_template, config_array, template_env) if not source_template or source_template == "" then return nil end @@ -139,10 +91,10 @@ local function param_value(source_template, config_array) compiled_templates[source_template] = compiled_template end - return compiled_template:render(template_environment) + return compiled_template:render(template_env) end -local function iter(config_array) +local function iter(config_array, template_env) return function(config_array, i, previous_name, previous_value) i = i + 1 local current_pair = config_array[i] @@ -156,7 +108,7 @@ local function iter(config_array) return i, current_name end - local res, err = param_value(current_value, config_array) + local res, err = param_value(current_value, config_array, template_env) if err then return error("[request-transformer] failed to render the template " .. current_value .. ", error:" .. err) @@ -182,14 +134,14 @@ local function append_value(current_value, value) end end -local function transform_headers(conf) +local function transform_headers(conf, template_env) local headers = get_headers() local headers_to_remove = {} headers.host = nil -- Remove header(s) - for _, name, value in iter(conf.remove.headers) do + for _, name, value in iter(conf.remove.headers, template_env) do name = name:lower() if headers[name] then headers[name] = nil @@ -198,7 +150,7 @@ local function transform_headers(conf) end -- Rename headers(s) - for _, old_name, new_name in iter(conf.rename.headers) do + for _, old_name, new_name in iter(conf.rename.headers, template_env) do old_name = old_name:lower() local value = headers[old_name] if value then @@ -209,7 +161,7 @@ local function transform_headers(conf) end -- Replace header(s) - for _, name, value in iter(conf.replace.headers) do + for _, name, value in iter(conf.replace.headers, template_env) do name = name:lower() if headers[name] or name == HOST then headers[name] = value @@ -217,14 +169,14 @@ local function transform_headers(conf) end -- Add header(s) - for _, name, value in iter(conf.add.headers) do + for _, name, value in iter(conf.add.headers, template_env) do if not headers[name] and name:lower() ~= HOST then headers[name] = value end end -- Append header(s) - for _, name, value in iter(conf.append.headers) do + for _, name, value in iter(conf.append.headers, template_env) do local name_lc = name:lower() if name_lc ~= HOST and name ~= name_lc and headers[name] ~= nil then @@ -247,7 +199,7 @@ local function transform_headers(conf) set_headers(headers) end -local function transform_querystrings(conf) +local function transform_querystrings(conf, template_env) if not (#conf.remove.querystring > 0 or #conf.rename.querystring > 0 or #conf.replace.querystring > 0 or #conf.add.querystring > 0 or @@ -255,41 +207,41 @@ local function transform_querystrings(conf) return end - local querystring = pl_copy_table(template_environment.query_params) + local querystring = pl_copy_table(template_env.query_params) -- Remove querystring(s) - for _, name, value in iter(conf.remove.querystring) do + for _, name, value in iter(conf.remove.querystring, template_env) do querystring[name] = nil end -- Rename querystring(s) - for _, old_name, new_name in iter(conf.rename.querystring) do + for _, old_name, new_name in iter(conf.rename.querystring, template_env) do local value = querystring[old_name] querystring[new_name] = value querystring[old_name] = nil end - for _, name, value in iter(conf.replace.querystring) do + for _, name, value in iter(conf.replace.querystring, template_env) do if querystring[name] then querystring[name] = value end end -- Add querystring(s) - for _, name, value in iter(conf.add.querystring) do + for _, name, value in iter(conf.add.querystring, template_env) do if not querystring[name] then querystring[name] = value end end -- Append querystring(s) - for _, name, value in iter(conf.append.querystring) do + for _, name, value in iter(conf.append.querystring, template_env) do querystring[name] = append_value(querystring[name], value) end set_uri_args(querystring) end -local function transform_json_body(conf, body, content_length) +local function transform_json_body(conf, body, content_length, template_env) local removed, renamed, replaced, added, appended = false, false, false, false, false local content_length = (body and #body) or 0 local parameters = parse_json(body) @@ -301,14 +253,14 @@ local function transform_json_body(conf, body, content_length) end if content_length > 0 and #conf.remove.body > 0 then - for _, name, value in iter(conf.remove.body) do + for _, name, value in iter(conf.remove.body, template_env) do parameters[name] = nil removed = true end end if content_length > 0 and #conf.rename.body > 0 then - for _, old_name, new_name in iter(conf.rename.body) do + for _, old_name, new_name in iter(conf.rename.body, template_env) do local value = parameters[old_name] parameters[new_name] = value parameters[old_name] = nil @@ -317,7 +269,7 @@ local function transform_json_body(conf, body, content_length) end if content_length > 0 and #conf.replace.body > 0 then - for _, name, value in iter(conf.replace.body) do + for _, name, value in iter(conf.replace.body, template_env) do if parameters[name] then parameters[name] = value replaced = true @@ -326,7 +278,7 @@ local function transform_json_body(conf, body, content_length) end if #conf.add.body > 0 then - for _, name, value in iter(conf.add.body) do + for _, name, value in iter(conf.add.body, template_env) do if not parameters[name] then parameters[name] = value added = true @@ -335,7 +287,7 @@ local function transform_json_body(conf, body, content_length) end if #conf.append.body > 0 then - for _, name, value in iter(conf.append.body) do + for _, name, value in iter(conf.append.body, template_env) do local old_value = parameters[name] parameters[name] = append_value(old_value, value) appended = true @@ -347,19 +299,19 @@ local function transform_json_body(conf, body, content_length) end end -local function transform_url_encoded_body(conf, body, content_length) +local function transform_url_encoded_body(conf, body, content_length, template_env) local renamed, removed, replaced, added, appended = false, false, false, false, false local parameters = decode_args(body) if content_length > 0 and #conf.remove.body > 0 then - for _, name, value in iter(conf.remove.body) do + for _, name, value in iter(conf.remove.body, template_env) do parameters[name] = nil removed = true end end if content_length > 0 and #conf.rename.body > 0 then - for _, old_name, new_name in iter(conf.rename.body) do + for _, old_name, new_name in iter(conf.rename.body, template_env) do local value = parameters[old_name] parameters[new_name] = value parameters[old_name] = nil @@ -368,7 +320,7 @@ local function transform_url_encoded_body(conf, body, content_length) end if content_length > 0 and #conf.replace.body > 0 then - for _, name, value in iter(conf.replace.body) do + for _, name, value in iter(conf.replace.body, template_env) do if parameters[name] then parameters[name] = value replaced = true @@ -377,7 +329,7 @@ local function transform_url_encoded_body(conf, body, content_length) end if #conf.add.body > 0 then - for _, name, value in iter(conf.add.body) do + for _, name, value in iter(conf.add.body, template_env) do if parameters[name] == nil then parameters[name] = value added = true @@ -386,7 +338,7 @@ local function transform_url_encoded_body(conf, body, content_length) end if #conf.append.body > 0 then - for _, name, value in iter(conf.append.body) do + for _, name, value in iter(conf.append.body, template_env) do local old_value = parameters[name] parameters[name] = append_value(old_value, value) appended = true @@ -398,12 +350,12 @@ local function transform_url_encoded_body(conf, body, content_length) end end -local function transform_multipart_body(conf, body, content_length, content_type_value) +local function transform_multipart_body(conf, body, content_length, content_type_value, template_env) local removed, renamed, replaced, added, appended = false, false, false, false, false local parameters = multipart(body and body or "", content_type_value) if content_length > 0 and #conf.rename.body > 0 then - for _, old_name, new_name in iter(conf.rename.body) do + for _, old_name, new_name in iter(conf.rename.body, template_env) do if parameters:get(old_name) then local value = parameters:get(old_name).value parameters:set_simple(new_name, value) @@ -414,14 +366,14 @@ local function transform_multipart_body(conf, body, content_length, content_type end if content_length > 0 and #conf.remove.body > 0 then - for _, name, value in iter(conf.remove.body) do + for _, name, value in iter(conf.remove.body, template_env) do parameters:delete(name) removed = true end end if content_length > 0 and #conf.replace.body > 0 then - for _, name, value in iter(conf.replace.body) do + for _, name, value in iter(conf.replace.body, template_env) do if parameters:get(name) then parameters:delete(name) parameters:set_simple(name, value) @@ -431,7 +383,7 @@ local function transform_multipart_body(conf, body, content_length, content_type end if #conf.add.body > 0 then - for _, name, value in iter(conf.add.body) do + for _, name, value in iter(conf.add.body, template_env) do if not parameters:get(name) then parameters:set_simple(name, value) added = true @@ -444,7 +396,7 @@ local function transform_multipart_body(conf, body, content_length, content_type end end -local function transform_body(conf) +local function transform_body(conf, template_env) local content_type_value = get_header(CONTENT_TYPE) local content_type = get_content_type(content_type_value) if content_type == nil or #conf.rename.body < 1 and @@ -454,16 +406,19 @@ local function transform_body(conf) end -- Call req_read_body to read the request body first - local body = get_raw_body() + local body, err = get_raw_body() + if err then + kong.log.warn(err) + end local is_body_transformed = false local content_length = (body and #body) or 0 if content_type == ENCODED then - is_body_transformed, body = transform_url_encoded_body(conf, body, content_length) + is_body_transformed, body = transform_url_encoded_body(conf, body, content_length, template_env) elseif content_type == MULTI then - is_body_transformed, body = transform_multipart_body(conf, body, content_length, content_type_value) + is_body_transformed, body = transform_multipart_body(conf, body, content_length, content_type_value, template_env) elseif content_type == JSON then - is_body_transformed, body = transform_json_body(conf, body, content_length) + is_body_transformed, body = transform_json_body(conf, body, content_length, template_env) end if is_body_transformed then @@ -505,10 +460,10 @@ local function transform_method(conf) end end -local function transform_uri(conf) +local function transform_uri(conf, template_env) if conf.replace.uri then - local res, err = param_value(conf.replace.uri, conf.replace) + local res, err = param_value(conf.replace.uri, conf.replace, template_env) if err then error("[request-transformer] failed to render the template " .. tostring(conf.replace.uri) .. ", error:" .. err) @@ -524,12 +479,51 @@ local function transform_uri(conf) end function _M.execute(conf) - clear_environment() - transform_uri(conf) + -- meta table for the sandbox, exposing lazily loaded values + local __meta_environment = { + __index = function(self, key) + local lazy_loaders = { + headers = function(self) + return get_headers() or EMPTY + end, + query_params = function(self) + return get_uri_args() or EMPTY + end, + uri_captures = function(self) + return (ngx.ctx.router_matches or EMPTY).uri_captures or EMPTY + end, + shared = function(self) + return ((kong or EMPTY).ctx or EMPTY).shared or EMPTY + end, + } + local loader = lazy_loaders[key] + if not loader then + -- we don't have a loader, so just return nothing + return + end + -- set the result on the table to not load again + local value = loader() + rawset(self, key, value) + return value + end, + __newindex = function(self) + error("This environment is read-only.") + end, + } + + local template_env = setmetatable({ + -- here we can optionally add functions to expose to the sandbox, eg: + -- tostring = tostring, -- for example + -- because headers may contain array elements such as duplicated headers + -- type is a useful function in these cases. See issue #25. + type = type, + }, __meta_environment) + + transform_uri(conf, template_env) transform_method(conf) - transform_headers(conf) - transform_body(conf) - transform_querystrings(conf) + transform_headers(conf, template_env) + transform_body(conf, template_env) + transform_querystrings(conf, template_env) end return _M diff --git a/spec/03-plugins/36-request-transformer/02-access_spec.lua b/spec/03-plugins/36-request-transformer/02-access_spec.lua index ccf61457626..760964b9699 100644 --- a/spec/03-plugins/36-request-transformer/02-access_spec.lua +++ b/spec/03-plugins/36-request-transformer/02-access_spec.lua @@ -2227,4 +2227,114 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function() end) end) end) + +describe("Plugin: request-transformer (thread safety) [#" .. strategy .. "]", function() + local db_strategy = strategy ~= "off" and strategy or nil + + lazy_setup(function() + local bp = helpers.get_db_utils(db_strategy, { + "routes", + "services", + "plugins", + }, { "request-transformer", "pre-function" }) + + local route = bp.routes:insert({ + hosts = { "test_thread_safety.test" } + }) + + bp.plugins:insert { + route = { id = route.id }, + name = "pre-function", + config = { + access = { + [[ + local delay = kong.request.get_header("slow_body_delay") + local orig_read_body = ngx.req.read_body + ngx.ctx.orig_read_body = orig_read_body + ngx.req.read_body = function() + ngx.sleep(tonumber(delay)) + return orig_read_body() + end + ]] + }, + header_filter = { + [[ + ngx.req.read_body = ngx.ctx.orig_read_body or ngx.req.read_body + ]] + }, + } + } + + bp.plugins:insert { + route = { id = route.id }, + name = "request-transformer", + config = { + add = { + querystring = { "added_q:yes_q" }, + headers = { "added_h:yes_h" }, + body = { "added_b:yes_b" } + } + } + } + + assert(helpers.start_kong({ + database = db_strategy, + plugins = "bundled, request-transformer", + nginx_conf = "spec/fixtures/custom_nginx.template", + nginx_worker_processes = 1 + })) + end) + + lazy_teardown(function() + helpers.stop_kong() + end) + + it("sends requests with the expected values for headers, body, query", function() + local race_conditions = "" + + local get_handler = function(header_val, body_param_val, query_param_val, delay) + return function() + local tmp_client = helpers.proxy_client() + local r = assert(tmp_client:send({ + method = "POST", + path = "/request", + headers = { + ["Content-Type"] = "application/json", + host = "test_thread_safety.test", + slow_body_delay = delay, + h = header_val + }, + body = { + k = body_param_val + }, + query = { + q = query_param_val + } + })) + + assert.response(r).has.status(200) + local header = assert.request(r).has.header("h") + local body_param = assert.request(r).has.jsonbody().params.k + local query_param = assert.request(r).has.queryparam("q") + if header_val ~= header then + race_conditions = race_conditions .. fmt("expected: %s, received: %s", header_val, header) + end + if body_param_val ~= body_param then + race_conditions = race_conditions .. fmt("expected: %s, received: %s", body_param_val, body_param) + end + if query_param ~= query_param_val then + race_conditions = race_conditions .. fmt("expected: %s, received: %s", query_param, query_param_val) + end + tmp_client:close() + end + end + + local thread_1 = ngx.thread.spawn(get_handler("vh1", "b1", "vq1", 2)) + local thread_2 = ngx.thread.spawn(get_handler("vh2", "b2", "vq2", 0)) + ngx.thread.wait(thread_1) + ngx.thread.wait(thread_2) + + assert.equals("", race_conditions) + end) +end) end