Set host header in order to use it in another route lookup

[szuecs]

If you add dropRequestHeader("Host") -> setRequestHeader("Host", "bar") -> <loopback> the new host "bar" is not replacing the host header, but appending it. This works differently from the network backend.

Example

./bin/skipper -inline-routes='
r: HostAny("foo") -> logHeader() -> preserveHost("false") -> dropRequestHeader("Host") -> setRequestHeader("Host", "bar") -> <loopback>; 
r1: HostAny("bar") -> status(200) -> inlineContent("bar") -> <shunt>;'
[APP]INFO[0002] GET /foo HTTP/1.1
Host: foo                 <----
User-Agent: curl/7.49.0
Accept: */*

[APP]INFO[0002] GET /foo HTTP/1.1
Host: foo                 <----
Accept: */*
Host: bar                 <----
User-Agent: curl/7.49.0

[APP]INFO[0002] GET /foo HTTP/1.1
Host: foo                 <----
Host: bar                 <----
User-Agent: curl/7.49.0
Accept: */*

...
[APP]ERRO[0002] error while proxying after 5.67122ms, route r with backend loopback ://, status code 500: max loopbacks reached, remote host: ::1, request: "GET /foo HTTP/1.1", user agent: "curl/7.49.0"
::1 - - [07/Jul/2022:15:24:03 +0200] "GET /foo HTTP/1.1" 500 22 "-" "curl/7.49.0" 5 foo - -

network backend:

% ./bin/skipper -inline-routes='
r: HostAny("foo") -> logHeader() -> preserveHost("false") -> setRequestHeader("Host", "bar") -> "http://127.0.0.1:9090"; 
r1: HostAny("bar") -> status(200) -> inlineContent("bar") -> <shunt>;'

[APP]INFO[0001] GET /foo HTTP/1.1
Host: foo
User-Agent: curl/7.49.0
Accept: */*

127.0.0.1 - - [07/Jul/2022:15:29:40 +0200] "GET /foo HTTP/1.1" 200 3 "-" "curl/7.49.0" 1 bar - -
::1 - - [07/Jul/2022:15:29:40 +0200] "GET /foo HTTP/1.1" 200 3 "-" "curl/7.49.0" 3 foo - -

[AlexanderYastrebov]

The thing is that go http.Server does not put "Host" into http.Request.Header but sets http.Request.Host, see https://pkg.go.dev/net/http#Request

// For incoming requests, the Host header is promoted to the
// Request.Host field and removed from the Header map.

For setRequestHeader (and friends) we have a special case for "Host" which sets context OutgoingHost field and (rather incorrectly) adds Host to the Headers:

skipper/filters/builtin/headerfilter.go

Lines 262 to 269 in 7cb3dc2

	case setRequestHeader:
	value, ok := f.template.ApplyContext(ctx)
	if ok {
	header.Set(f.key, value)
	if strings.ToLower(f.key) == "host" {
	ctx.SetOutgoingHost(value)
	}
	}

This is how example ends up logging two Hosts: one is req.Host and another is from Headers:

skipper/filters/diag/logheader.go

Lines 102 to 115 in 7cb3dc2

	buf.WriteString(req.Host)
	buf.WriteString("\r\n")
	for k, v := range req.Header {
	if strings.ToLower(k) == "authorization" {
	buf.WriteString(k)
	buf.WriteString(": ")
	buf.WriteString("TRUNCATED\r\n")
	} else {
	buf.WriteString(k)
	buf.WriteString(": ")
	buf.WriteString(strings.Join(v, " "))
	buf.WriteString("\r\n")
	}
	}

Loopback routing is based on the ctx.Request().Host and does not use ctx.OutgoingHost field.
I think we can create a new setHost("<string>") filter that sets ctx.Request().Host.

[szuecs]

Should not we better "fix" the setRequestHeader("Host", ..) part?
I mean it's a bit unexpected if you are skipper user and not a Go/skipper expert. Even if we have to have an if f.header == "Host" {}, it's better for the UX.
One open question of course is, if this result in an incident somewhere, because maybe there are users that expect the current behavior for some unknown reason.

[AlexanderYastrebov]

Should not we better "fix" the setRequestHeader("Host", ..) part?

Yeah, I think we can also set ctx.Request().Host.

One open question of course is, if this result in an incident somewhere, because maybe there are users that expect the current behavior for some unknown reason.

Not adding Host to Headers could be a breaking change so I guess we should keep it like it is.