Support path routes with port number in Host header #8490

elyscape · 2016-04-12T23:43:47Z

This deals with #8471 by adding a duplicate entry to the map files that also includes the port, which will match values of base that result when a client includes the port number in the Host header.

This deals with #8471 by removing ports from the Host header send by clients.

Fixes #8471.

This will ensure things don't break if openshift/origin#8490 is merged.

knobunc · 2016-04-21T00:08:22Z

LGTM, @ramr PTAL

smarterclayton · 2016-04-21T00:55:59Z

HAProxy doesn't handle this automatically?

ramr · 2016-04-21T01:47:53Z

images/router/haproxy/conf/haproxy-config.template

@@ -254,6 +254,7 @@ backend be_secure_{{$cfgIdx}}
 {{     range $idx, $cfg := $serviceUnit.ServiceAliasConfigs }}
 {{       if and (ne $cfg.Host "") (eq $cfg.TLSTermination "")}}
 {{$cfg.Host}}{{$cfg.Path}} {{$idx}}
+{{$cfg.Host}}{{env "ROUTER_SERVICE_HTTP_PORT" "80"}}{{$cfg.Path}} {{$idx}}


Missing separator between host and port. {{$cfg.Host}}:{{env "ROUTER_SERVICE_HTTP_PORT" "80"}}{{$cfg.Path}}.

ramr · 2016-04-21T02:11:38Z

So one issue I can see here (apart from the missing separator between host and port) is that we are adding another rule for the host:port combination - effectively doubling the number of rules. This would have a bigger performance impact as the number of routes/rules gets larger.
We use this to search the map using the base uri ala:
use_backend be_edge_http_%[base,map_beg(/var/lib/haproxy/conf/os_edge_http_be. map)] or in other ACLs checking specific maps.
Probably better, if we can use a single rule with the port always added and have the matcher build the host:port/path in lieu of using base in the example above.

smarterclayton · 2016-04-21T02:17:47Z

I would expect HAProxy to have some normalization possible.

On Wed, Apr 20, 2016 at 10:11 PM, Ram Ranganathan [email protected]
wrote:

So one issue I can see here (apart from the missing separator between host
and port) is that we are adding another rule for the host:port
combination - effectively doubling the number of rules. This would have a
bigger performance impact as the number of routes/rules gets larger.
We use this to search the map using the base uri ala:
use_backend
be_edge_http_%[base,map_beg(/var/lib/haproxy/conf/os_edge_http_be.
map)] or in other ACLs checking specific maps.
Probably better, if we can use a single rule with the port always added
and have the matcher build the host:port/path in lieu of using base in
the example above.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#8490 (comment)

ramr · 2016-04-21T02:20:49Z

@smarterclayton currently the base fetch method is used for matching - ala use_backend be_edge_http_%[base,map_beg(/var/lib/haproxy/conf/os_edge_http_be. map)].
Base is basically hdr(host) + path which fails when the Host header contains a port number.

smarterclayton · 2016-04-21T02:53:21Z

Is there no specific host method in haproxy that performs normalization?

On Wed, Apr 20, 2016 at 10:20 PM, Ram Ranganathan [email protected]
wrote:

@smarterclayton https://github.com/smarterclayton currently the base
fetch method is used for matching - ala use_backend
be_edge_http_%[base,map_beg(/var/lib/haproxy/conf/os_edge_http_be. map)].
Base is basically hdr(host) + path which fails when the Host header
contains a port number.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8490 (comment)

elyscape · 2016-04-21T04:36:24Z

Good catch on the missing port separator. Fixed. I agree that finding a way to normalize base would be better from a performance standpoint but, based on some experiments I performed, it looks like altering the Host header does not update base. I could be mistaken, though.

ramr · 2016-04-21T07:26:25Z

Hmm, gave this some thought - we might be able to simplify this to use the existing map file entry. So what we really want here is that base should contain just the equivalent of hdr(host)[path] with the port number stripped off. So we could set a new custom header to that value and use it in the match.

For a first pass, I'd suggest verifying that if you do something like add:
http-request replace-value Host (.*):.* \1
in the public and public_ssl backends - does that solves your issue. I suspect it should as its just the match with the port number that is failing in your case. That above directive is basically stripping out the port number from the host header.

Now, we could very well use that as as solution - because the XFF headers to the backend would anyway indicate what port the request arrived on + the host. Other alternatives here would be:

A wee bit less riskier would be to set a custom header with the equivalent of hdr(host)[path] and the port number stripped off. And use the custom header for matching in the maps.
Alternatively have a knob for tuning that adds that http-request replace-value Host (.*):.* \1 to the generated haproxy config.
I guess we could also push and pop the original Host header after we do the match - push in the frontend and restore it in the backend.

I'll mull on this a wee bit and see if any other ideas crop up.

Edited typos

elyscape · 2016-04-21T07:39:49Z

For a first pass, I'd suggest verifying that if you do something like add:
http-request replace-value Host (.*):.* \1

I'm pretty sure I tried this and it didn't work, but I'll try it again.

ramr · 2016-04-22T00:43:08Z

@elyscape I tried that this am and it worked for me - so give it a whirl.

elyscape · 2016-04-25T21:21:57Z

@ramr I just tried that again and it didn't work for me. Where in the frontend public and frontend public_ssl sections did you put it? I put it immediately after the tcp-request lines.

ramr · 2016-04-25T21:50:24Z

Adding a link to the changes on a gist (was on https://paste.fedoraproject.org/359696/16206581/):
https://gist.github.com/ramr/0559d1d3aa538ad6370571c488984cfd

elyscape · 2016-04-25T22:35:46Z

It doesn't work regardless of whether I put http-request after tcp-request or, as in the gist, before tcp-request.

elyscape · 2016-04-25T22:52:23Z

As far as I can tell, http-request replace-value Host and http-request replace-header Host do not affect the value of base.

elyscape · 2016-04-25T23:02:31Z

This is interesting. That does change the value of base, but only via connections that do not use SSL/TLS.

elyscape · 2016-04-25T23:35:32Z

Looks like the problem was that the public_ssl frontend didn't have mode http set, which was intentional. It doesn't use path maps, though, instead sending to either be_tcp_$ROUTE, be_sni, or be_no_sni depending on if the route is set to just pass through traffic and whether it uses SNI. Passthrough routes don't support path-based routing, so we don't need to worry about them. Backends be_sni and be_no_sni simply redirect to frontends fe_sni and fe_no_sni, which do set mode http. Putting the http-request replace-value Host line in those makes this work. I'll update my pull request to make use of this instead.

elyscape · 2016-04-25T23:46:38Z

Updated to use http-request replace-header Host instead.

elyscape · 2016-04-25T23:55:40Z

I also went ahead and rebased it onto master.

ramr · 2016-04-26T18:44:38Z

@elyscape thanks, those changes look good. An integration test for this would be good - something in this test: https://github.com/openshift/origin/blob/master/test/integration/router_test.go#L390

The simplest check might be to do something similar to this test at: https://github.com/openshift/origin/blob/master/test/integration/router_test.go#L610

if err := waitForRoute(routeAddress, "www.example.com", "http", nil, tr.HelloPodAlternate); err != nil {   
        t.Fatalf("unexpected response: %q", err)   
}

The fourth parameter to the waitForRoute call is the http headers - so just a check that sending a
request with headers ala Host: www.example.com:80 behaves as expected.

elyscape · 2016-04-26T20:51:37Z

Added some integration tests. Please review. I'll squash the commits if everything checks out.

elyscape · 2016-04-26T20:54:54Z

test/integration/router_test.go

@@ -464,6 +464,10 @@ func TestRouterPathSpecificity(t *testing.T) {
 	if _, err := getRoute(routeAddress, "www.example.com", "http", nil, ""); err != ErrUnavailable {
 		t.Fatalf("unexpected response: %q", err)
 	}
+	//ensure you can curl path with port in Host header
+	if err := waitForRoute(routeTestAddress, "www.example.com:80", "http", nil, tr.HelloPodPath); err != nil {


The second argument here gets put into the Request.Host field, which is what determines the Host header.

elyscape · 2016-04-26T21:01:19Z

As a side note, I noticed that the router's path-based integration tests only really go in-depth via HTTP. Since the issue can manifest on HTTP without manifesting on HTTPS and vice versa, it would be a good idea to add testing on that as well. Additionally, there is no path-based route testing for WebSockets.

I'll add these in a separate pull request once this is merged.

ramr · 2016-04-27T06:09:57Z

test/integration/router_test.go

@@ -526,6 +533,9 @@ func TestRouterPathSpecificity(t *testing.T) {
 	if err := waitForRoute(routeAddress, "www.example.com", "http", nil, tr.HelloPod); err != nil {
 		t.Fatalf("unexpected response: %q", err)
 	}
+	if err := waitForRoute(routeTestAddress, "www.example.com:80", "http", nil, tr.HelloPodPath); err != nil {


expected response should be tr.HelloPod - same as above and not tr.HelloPodPath.

I'm not sure I understand. These new lines should be making requests against the path route. That's why it's testing against routeTestAddress instead of routeAddress. It looks like I should probably be using getRoute() instead of waitForRoute(), but that's a minor thing that I can fix.

my bad - its late ... ignore my comments - I glossed over the routeTestAddress part.

ramr · 2016-04-27T06:12:26Z

@elyscape some comments re: the tests. Running them for you here so you can see the results.

ramr · 2016-04-27T06:13:18Z

[test]

This is done to support path-based routing for clients that include the port number in the Host header. While unusual, doing so isn't against the HTTP spec, and some clients force it. Fixes openshift#8471.

elyscape · 2016-04-27T07:10:00Z

test/integration/router_test.go

@@ -610,6 +626,9 @@ func TestRouterPathSpecificity(t *testing.T) {
 	if err := waitForRoute(routeAddress, "www.example.com", "http", nil, tr.HelloPodAlternate); err != nil {
 		t.Fatalf("unexpected response: %q", err)
 	}
+	if err := waitForRoute(routeTestAddress, "www.example.com:80", "http", nil, tr.HelloPodPath); err != nil {


Aha, I see. This needed to be tr.HelloPodAlternate. Fixed.

elyscape · 2016-04-27T07:10:15Z

@ramr, the tests should be fixed now.

ramr · 2016-04-27T07:59:25Z

re[test]

ramr · 2016-04-27T17:48:34Z

[test]

openshift-bot · 2016-04-27T17:50:28Z

Evaluated for origin test up to 7b25ad6

openshift-bot · 2016-04-27T19:01:39Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3357/)

ramr · 2016-04-27T21:46:38Z

changes LGTM
@smarterclayton / @rajatchopra - merge magic please.

rajatchopra · 2016-04-28T18:19:52Z

LGTM [merge]

openshift-bot · 2016-04-28T18:25:24Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3357/) (Image: devenv-rhel7_4057)

openshift-bot · 2016-04-28T18:25:25Z

Evaluated for origin merge up to 7b25ad6

elyscape mentioned this pull request Apr 12, 2016

Path-based routes fail when the Host header includes a port #8471

Closed

elyscape added a commit to elyscape/origin-router-regexp that referenced this pull request Apr 20, 2016

Add support for mappings with ports

46a1a90

This will ensure things don't break if openshift/origin#8490 is merged.

ramr reviewed Apr 21, 2016
View reviewed changes

elyscape force-pushed the 8471-path-route-port branch from 310dc6c to fa4ca04 Compare April 21, 2016 04:38

elyscape force-pushed the 8471-path-route-port branch from fa4ca04 to 7a73124 Compare April 25, 2016 23:40

elyscape force-pushed the 8471-path-route-port branch from 7a73124 to 429f3d0 Compare April 25, 2016 23:55

elyscape reviewed Apr 26, 2016
View reviewed changes

ramr reviewed Apr 27, 2016
View reviewed changes

Remove port from request's Host header

7b25ad6

This is done to support path-based routing for clients that include the port number in the Host header. While unusual, doing so isn't against the HTTP spec, and some clients force it. Fixes openshift#8471.

elyscape force-pushed the 8471-path-route-port branch from 41b80f5 to 7b25ad6 Compare April 27, 2016 07:08

elyscape reviewed Apr 27, 2016
View reviewed changes

openshift-bot merged commit 79b4182 into openshift:master Apr 28, 2016

elyscape deleted the 8471-path-route-port branch April 28, 2016 20:34

elyscape mentioned this pull request Apr 28, 2016

Test path-based routing via HTTPS and WebSockets #8678

Merged

Support path routes with port number in Host header #8490

Support path routes with port number in Host header #8490

Uh oh!

Conversation

elyscape commented Apr 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knobunc commented Apr 21, 2016

Uh oh!

smarterclayton commented Apr 21, 2016

Uh oh!

ramr Apr 21, 2016

Choose a reason for hiding this comment

Uh oh!

ramr commented Apr 21, 2016

Uh oh!

smarterclayton commented Apr 21, 2016

Uh oh!

ramr commented Apr 21, 2016

Uh oh!

smarterclayton commented Apr 21, 2016

Uh oh!

elyscape commented Apr 21, 2016

Uh oh!

ramr commented Apr 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elyscape commented Apr 21, 2016

Uh oh!

ramr commented Apr 22, 2016

Uh oh!

elyscape commented Apr 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramr commented Apr 25, 2016

Uh oh!

elyscape commented Apr 25, 2016

Uh oh!

elyscape commented Apr 25, 2016

Uh oh!

elyscape commented Apr 25, 2016

Uh oh!

elyscape commented Apr 25, 2016

Uh oh!

elyscape commented Apr 25, 2016

Uh oh!

elyscape commented Apr 25, 2016

Uh oh!

ramr commented Apr 26, 2016

Uh oh!

elyscape commented Apr 26, 2016

Uh oh!

elyscape Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

elyscape commented Apr 26, 2016

Uh oh!

ramr Apr 27, 2016

Choose a reason for hiding this comment

Uh oh!

elyscape Apr 27, 2016

Choose a reason for hiding this comment

Uh oh!

ramr Apr 27, 2016

Choose a reason for hiding this comment

Uh oh!

ramr commented Apr 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramr commented Apr 27, 2016

Uh oh!

elyscape Apr 27, 2016

Choose a reason for hiding this comment

Uh oh!

elyscape commented Apr 27, 2016

Uh oh!

ramr commented Apr 27, 2016

Uh oh!

ramr commented Apr 27, 2016

Uh oh!

elyscape commented Apr 12, 2016 •

edited

Loading

ramr commented Apr 21, 2016 •

edited

Loading

elyscape commented Apr 25, 2016 •

edited

Loading

ramr commented Apr 27, 2016 •

edited

Loading

openshift-bot commented Apr 28, 2016 •

edited

Loading