Router forwards host urls without ports when serving on non-default ports

[miminar]

With default haproxy router, if I expose registry on port 5000, the router strips the port on each request from Host header.

When registry redirects the client as a response to POST request initiating blob upload, it will use just the url in Host header as a base for the value in Location header.

During the next client request, the Location will be used resulting in failure because port 80 is not exposed.

Version

master

Steps To Reproduce

1. deploy registry
2. deploy router like this: oadm router router --ports 5000:5000 -o json | oc env -f - --output=json ROUTER_SERVICE_HTTP_PORT=5000 | oc create -n default -f -
3. expose the registry using route: oc expose service docker-registry --hostname=registry.f24-ose.vm -n default
4. Add --insecure-registry=registry.f24-ose.vm:5000 to docker daemon's flags.
5. docker login -u unused -p $(oc whoami -t) -e unused registry.f24-ose.vm:5000
6. docker tag busybox registry.f24-ose.vm:5000/$proj/buxybox
7. docker push registry.f24-ose.vm:5000/$proj/buxybox

Current Result

The push fails on PATCH request: Upload failed: dial tcp 192.168.100.80:80: getsockopt: connection refused (extracted from docker daemon log).

Note the port 80 as a result of Location header in registry's response to POST: http://registry.f24-ose.vm/v2/pjoe/hello-world/blobs/uploads/3d70392c-0751-4190-9508-ab9efb450d99?_state=6VTnYEsxOJZCFVr8zthbXcPCV4t4S__QMsNL1OCLHaN7Ik5hbWUiOiJwam9lL2hlbGxvLXdvcmxkIiwiVVVJRCI6IjNkNzAzOTJjLTA3NTEtNDE5MC05NTA4LWFiOWVmYjQ1MGQ5OSIsIk9mZnNldCI6MCwiU3RhcnRlZEF0IjoiMjAxNi0xMC0xMlQxMzo1MDozNS4xNjI4MDQ4MzlaIn0%3D

(Note the missing :5000).

Take a look at original request from docker daemon to registry's proxy:

GET /openshift/token?account=unused&scope=repository%3Apjoe%2Fhello-world%3Apush%2Cpull HTTP/1.1
Host: registry.f24-ose.vm:5000
User-Agent: docker/1.12.1 go/go1.7 git-commit/23cf638 kernel/4.7.6-1-ARCH os/linux arch/amd64 UpstreamClient(Docker-Client/1.12.1 \(linux\))
Authorization: Basic dW51c2VkOlV1WGoyTGdTWkk4TGc0azY1bXFwc3ZlT1VKdXg1NnFDMTJXMThRdUdYOG8=
Accept-Encoding: gzip
Connection: close

And the same request going from proxy to registry application:

GET /openshift/token?account=unused&scope=repository%3Apjoe%2Fhello-world%3Apush%2Cpull HTTP/1.1
Host: registry.f24-ose.vm
User-Agent: docker/1.12.1 go/go1.7 git-commit/23cf638 kernel/4.7.6-1-ARCH os/linux arch/amd64 UpstreamClient(Docker-Client/1.12.1 \(linux\))
Authorization: Basic dW51c2VkOlV1WGoyTGdTWkk4TGc0azY1bXFwc3ZlT1VKdXg1NnFDMTJXMThRdUdYOG8=
Accept-Encoding: gzip
Connection: close
X-Forwarded-Host: registry.f24-ose.vm
X-Forwarded-Port: 5000
X-Forwarded-Proto: http
Forwarded: for=192.168.100.80;host=registry.f24-ose.vm;proto=http
X-Forwarded-For: 192.168.100.80

The registry takes just a value of X-Forwarded-Host to form redirect location url. It pays no attention to X-Forwarded-Port which is far less common.

Expected Result

Proxy should pass the Host header unmodified (including the port) to the exposed service if it is serving on non-default port. Also for and host parameters of Forwarded header need to include the same port.

Additional info

The above is addressed by #11391 which makes the registry smarter. Nevertheless, to avoid problems with ingenuous services, router should forward proper values.

This is reopened on behalf of docker/distribution upstream which considers our port forwarding wrong:

Also, it looks like the implementation of Forwarded in your examples is not following the rfc7230, in that it is missing a port.

See the whole discussion at distribution/distribution#2008.

[knobunc]

We are explicitly removing the port from the Host header. @ramr @rajatchopra do you know why we want to do that?

[ramr]

@knobunc @miminar one of the reasons we explicitly remove the port was to make path based routes to work (issue #8471) and also we did want Host: x.y.com and Host: x.y.com:80 (or :443) to use the same cookies/backends. That said I can see this being an issue - maybe a config knob to do/not do it (default do it) would help here?

[liggitt]

It has to be set for non default ports to work

[knobunc]

@JacobTanenbaum can you take a look at this please?

@ramr @liggitt can you think of a reason why we would not change this so we only do the rewrite if the ports are not the standard ones? Is there a reason to add a config knob for it? If we have to add config for it, then we should make it a global setting (with a default) and a per-route setting too. But I'd rather not do that and only rewrite for http:80 and https:443, otherwise, leave the port alone.

Thoughts?

[liggitt]

I can't think of a reason not to always include non-default ports. Regarding the above comment, in what way was path routing not working until we excluded the ports? Did that issue only apply to the standard ports?

[ramr]

Some background: as per the http spec, a client can send host [:port] in the HTTP Host header. So all of
Host: www.abc.test,
Host: www.abc.test:80 and
Host: www.abc.test:443
are valid Host header values - and could be more if you bind to other custom ports.

Now, when we do a path lookup today we check the map for something like www.abc.test/path/to/resource - if we added the other variants www.abc.test:$port/path/to/resource of this it, does increase the number of haproxy rules by atleast 3x (if not more for additional custom ports). That's okay for a small set but the power of large numbers won't work in our favor once the number of routes is large (ala for online).

A lot of this is also pre-haproxy 1.6 which added support for variables. On older version it was a wee bit costlier as we would have had to use a http header to achieve something similar.

So if we can bump up to the new haproxy version, it would allow us to keep the host header as is and artificially generate a new variable for the base path - host header (minus port) + the URI path and use that for matching. And we would also need one for the host for matching up the host for wildcards now.
I am assuming it would also add a wee bit more to the processing time per request.

[ramr]

On second thoughts (and a drive to costco), there might be a simpler solution by just using regexps to ignore the port part - got to evaluate the cost though. Post this release ok? As it could be invasive and needs more performance evaluation.

Updated to include perf info.

[knobunc]

Based on what Ram said, and the fact that we've been this way since April, and that the submitter has worked around it, I'm lowering the priority and we will get to it in the next release.