fix(request-transformer): unpredictable overriding result from rename header to existing header caused by pairs disorder

[windmgc]

Summary

This PR fixes a bug in the request-transformer plugin, where the rename header cannot override an already existing header steadily.

If a header A-header already exists with a value A-value in the request, and the request-transformer plugin is configured to rename B-header(with a value B-value) to A-header(with different format, like all chars are capitalized), then the upstream request will contain an A-header with possibly A-value or B-value. This unpredictable behaviour may caused by pairs function disorder behaviour(Again!).

The code flow is as follows:

A header table is fetched by ngx.req.get_headers and used later for storing upstream request headers

kong/kong/plugins/request-transformer/access.lua

Line 186 in c33d745

local headers = get_headers()

Then the rename header list will be iterated

kong/kong/plugins/request-transformer/access.lua

Lines 200 to 209 in c33d745

	-- Rename headers(s)
	for _, old_name, new_name in iter(conf.rename.headers) do
	old_name = old_name:lower()
	if headers[old_name] then
	local value = headers[old_name]
	headers[new_name] = value
	headers[old_name] = nil
	headers_to_remove[old_name] = true
	end
	end

Note that in this code block, the new_name will be directly used for setting value in the table, and the headers fetched by ngx.req.get_headers() are in lower case, so if new_name is an upper-case, then two keys with different format will exist in the header table.

And that will lead to the unpredictable iterating sequence in setting headers:

kong/kong/plugins/request-transformer/access.lua

Line 247 in c33d745

set_headers(headers)

kong/kong/pdk/service/request.lua

Lines 422 to 427 in c33d745

	for k, v in pairs(headers) do
	if string_lower(k) == "host" then
	ngx_var.upstream_host = v
	end
	ngx.req.set_header(k, normalize_multi_header(v))

Full changelog

• Fix unpredictable overriding result from rename header to existing header in request-transformer plugin, by normalizing the header name(set to lower case)
• Cleanup duplicate tests and add related test

Issue reference

Fix FTI-4319 and #9418

[hanshuebner]

To prevent such issues from happening, would it not make sense to have all headers tables have this metatable:

{
  __newindex=function (t,k,v) rawset(t,k:lower(),v) end,
  __index=function(t,k) return t[k:lower()] end,
}

That way, the normalization to lower case would happen automatically.

[windmgc]

To prevent such issues from happening, would it not make sense to have all headers tables have this metatable:

{
  __newindex=function (t,k,v) rawset(t,k:lower(),v) end,
  __index=function(t,k) return t[k:lower()] end,
}

That way, the normalization to lower case would happen automatically.

It's a good idea but needs changes in the PDK(more like a patch on the original OpenResty's ngx.req.get_headers, I think they already have __index to do the lowercase work but I don't know why they did not have the __newindex behaviour, maybe because of the ambiguity about underscore)

I feel it is better to open another PR for this change, and doing another lower case here is not very harmful, WDYT?

[chronolaw]

Need a change log entry.