fix(request-transformer) handle Host header correctly

[hishamhm]

Source of the problem: the plugin was getting the full set of headers from kong.request.get_headers, which includes the request Host value, and was setting it (after transformations) to kong.service.request.get_headers. Effectively, this performed the equivalent of the Route attribute's preserve_host = true behavior all the time.

This commit restores the input value of the Host header from the service request to the table prior to the transformations. It also removes special-case handling of the Host header that was present in the plugin, which is now handled by the PDK itself.

A change in this behavior as opposed to 0.14 is that now the
request-transformer plugin is able to modify the value of Host:

• if preserve_host is false, you can modify it using add;
• if preserve_host is true, you can modify it using replace.

Fixes #4252.

[hishamhm]

@thibaultcha About this last bit:

A change in this behavior as opposed to 0.14 (demonstrated by the added test cases) is that now the request-transformer plugin is able to modify the value of Host, if preserve_host is false. If preserve_host is set, it still takes precedence, and preserves the host from the request.

Do you know of any gotchas that would make this behavior unwanted?

[thibaultcha]

So, now to answer this question:

Do you know of any gotchas that would make this behavior unwanted?

Which I was about to do in the previous patch, but then noted that it was because the tests were using add instead of replace.

To answer the question, I believe that it isn't an unreasonable assumption for a user to expect a plugin to override a Route's settings. As of this patch, we only allow replacing the Host header if preserve_host = false (which, arguable is even more confusing that the previous assumed behavior - which was incorrectly described, but that is of no importance).

Additionally, there is a deeper issue at hand here I think, which is that the reason why this test case passes (and why the plugin cannot replace a Host header when preserve_host = false, is because we rely on ngx.var.upstream_host, which itself relies on the router's behavior. So the reason why this does not work (or why this test case succeeds) is because we directly rely on router behavior see upstream_host is nil which causes ngx.var.upstream_host to be nil, which causes this plugin to not act on a replace = { "host:foo" } configuration value, because we guard the replace branch with if headers[name] ~= nil then.

In my opinion, this plugin could safely set the upstream header regardless of whether preserve_host is true or false. In doing so, we would avoid relying on core behavior as we currently do, which isn't regression-safe at all.
We should find another way than the headers.host = ngx.var.upstream_host. In my opinion, we are lacking a thin layer of abstraction between the core and this plugin (we may use existing PDK functions that act as this layer, or we may come up with a new PDK abstraction). This is one of these problems that #3821 is trying to solve, iirc.

[hishamhm]

Yeah, I had a bad feeling about this particular case you highlighted as well. Since the original issue was about corrupting Host when transformations were not being applied on it and the semantics of the behavior of transforming Host were unclear anyway, I pushed on with that original approach.

In my opinion, this plugin could safely set the upstream header regardless of whether preserve_host is true or false.

I like these simpler semantics.

In doing so, we would avoid relying on core behavior as we currently do, which isn't regression-safe at all.

In the latest iteration, I had to special-case the Host header a bit more in the plugin again, but it does not rely on core behavior (only on the assumptions that a Host header must always exist [to forbid add and rename] and cannot be set multiple times [to forbid append] ).

In my opinion, we are lacking a thin layer of abstraction between the core and this plugin (we may use existing PDK functions that act as this layer, or we may come up with a new PDK abstraction). This is one of these problems that #3821 is trying to solve, iirc.

Agreed, because since kong.request represents the incoming request, its value of get_header("host") is correct. Perhaps what we really want is kong.service.request.get_headers, which would represent the state of headers as they are being prepared to be sent to the outgoing request (but of course there are difficulties in crafting such an API and keeping ngx and the PDK interoperable).

I think for now the approach in this PR is sufficient to fix the related issue.

[thibaultcha]

In the latest iteration, I had to special-case the Host header a bit more in the plugin again, but it does not rely on core behavior (only on the assumptions that a Host header must always exist [to forbid add and rename] and cannot be set multiple times [to forbid append] ).

This iteration looks better in its behavior. But it's still somewhat hand-crafted based on deep Core behavior, and a thin abstraction would be nicer. Nonetheless, the patch looks good for now 👍

Perhaps what we really want is kong.service.request.get_headers, which would represent the state of headers as they are being prepared to be sent to the outgoing request

This has been proposed in #3840 already - there has been a few asks around it and it seems like a valuable addition to the PDK.

[thibaultcha]

Actually, I take that back; I think the current level of decoupling between the core and this plugin is fine as per this iteration 👍