Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong route for multiple matching host glob patterns #506

Closed
arrodriguez opened this issue Jun 10, 2018 · 14 comments · Fixed by #507
Closed

Wrong route for multiple matching host glob patterns #506

arrodriguez opened this issue Jun 10, 2018 · 14 comments · Fixed by #507
Labels
Milestone

Comments

@arrodriguez
Copy link

arrodriguez commented Jun 10, 2018

@magiconair @marcosnils @xetorthio
Hello guys :

i am wondering which route fabio selects when we have configure this prefix:

urlprefix-*.edge.foo.com/gizmo, urlprefix-*.foo.com/gizmo

if use api.edge.foo.com in the host header, What is the expect behaviour?
It will use *.edge.foo.com/gizmo or *.foo.com/gizmo ?

I was doing a couple of tests and always select the route * foo.com

Regards!

@marcosnils
Copy link

marcosnils commented Jun 10, 2018

IIUC fabio should match the routes from the most specific to the most generic one. So, in this particular case I'd expect to match the *.edge.foo.com for the hostname api.edge.foo.com

@arrodriguez
Copy link
Author

arrodriguez commented Jun 10, 2018

Yes, i would expect the same result, but navigating old issues and doing some testing , the results were this:

  • if the host is a complete match, it take precedence over the glob route.
Test:

host: 
api.edge.foo.com/lalala

routes:

urlprefix-api.edge.foo.com/lalala ,  urlprefix-*.foo.com/lalala
  • When two or more glob routes are compiting , the most general one is taking precedence over the more specific.
Test

host: 
api.edge.foo.com/lalala

routes:

urlprefix-*.edge.foo.com/lalala ,  urlprefix-*.foo.com/lalala

In this scenario urlprefix-*.foo.com/lalala  matches

Fabio version is v1.5.6.

Regards

@aaronhurt
Copy link
Member

Could you see if you can reproduce the same behavior and b the current version? I thought this was covered in a router test case.

@magiconair
Copy link
Contributor

I can't reproduce this. When I add this test to the 1.5.6 codebase it works:

Sun Jun 10 20:25 frank@fancypants ((v1.5.6) *) $ git diff
diff --git a/route/table_test.go b/route/table_test.go
index eb0fdea..7d2da44 100644
--- a/route/table_test.go
+++ b/route/table_test.go
@@ -495,6 +495,7 @@ func TestTableLookup(t *testing.T) {
        route add svc z.abc.com/foo/ http://foo.com:3100
        route add svc *.abc.com/ http://foo.com:4000
        route add svc *.abc.com/foo/ http://foo.com:5000
+       route add svc *.def.abc.com/ http://foo.com:6000
        route add svc xyz.com:80/ https://xyz.com
        `

@@ -530,6 +531,7 @@ func TestTableLookup(t *testing.T) {

                // glob match the host
                {&http.Request{Host: "x.abc.com", URL: mustParse("/")}, "http://foo.com:4000"},
+               {&http.Request{Host: "x.def.abc.com", URL: mustParse("/")}, "http://foo.com:6000"},
                {&http.Request{Host: "y.abc.com", URL: mustParse("/abc")}, "http://foo.com:4000"},
                {&http.Request{Host: "x.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000"},
                {&http.Request{Host: "y.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000"},

@magiconair
Copy link
Contributor

Same for 1.5.9 and when I let the test run in a loop.

cd route ; go test -c ; while true ; do route.test -test.run TableLookup ; done

@marcosnils
Copy link

@magiconair thanks a lot for taking the time to test this. I haven't checked but in @arrodriguez example there's a path in the URL. Maybe that's causing the issue?

@magiconair
Copy link
Contributor

magiconair commented Jun 10, 2018

Hmm, I can reproduce this with the test server.

@magiconair
Copy link
Contributor

# term 1
consul agent -dev

# term 2
cd $GOPATH/src/github.com/fabiolb/fabio/demo/server
go build
./server -prefix '*.edge.foo.com/gizmo' -addr 127.0.0.1:5000

# term 3
cd $GOPATH/src/github.com/fabiolb/fabio/demo/server
./server -prefix '*.foo.com/gizmo' -addr 127.0.0.1:9999

# term 4
cd $GOPATH/src/github.com/fabiolb/fabio
go build
./fabio

# term 5
curl -H 'Host: api.edge.foo.com' http://127.0.0.1:9999/gizmo

The demo server responds with 404 not found which is a bug in the demo server but the request gets routed to the wrong backend.

@magiconair
Copy link
Contributor

Looks like a bug.

@magiconair
Copy link
Contributor

I think I know what is going on. This line doesn't do what I think it does:

https://github.com/fabiolb/fabio/blob/master/route/table.go#L306

The problem is not that fabio does not respect the longer host but that edge < foo. If you replace edge with zzz it "works".

The issue is that domain names have the most significant part at the front whereas the request URIs have the most significant part at the end. [foo.com edge.foo.com] needs to be sorted as [com.foo com.foo.edge] for this to work.

@magiconair
Copy link
Contributor

That's also the reason the test case works since def > abc

magiconair added a commit that referenced this issue Jun 10, 2018
magiconair added a commit that referenced this issue Jun 10, 2018
This patch fixes a behaviour where fabio routes to the wrong
backend when multiple glob matching routes are available since
the sorting of the host names was wrong.

Sorting DNS names requires reversing them before sorting since
they have their most significant part at the front.

Fixes #506
magiconair added a commit that referenced this issue Jun 10, 2018
This patch fixes a behavior where fabio routes to the wrong backend when
multiple glob matching routes are available since the sorting of the
host names was wrong.

Sorting DNS names requires reversing them before sorting since they have
their most significant part at the front.

Fixes #506
magiconair added a commit that referenced this issue Jun 10, 2018
This patch fixes a behavior where fabio routes to the wrong backend when
multiple glob matching routes are available since the sorting of the
host names was wrong.

Sorting DNS names requires reversing them before sorting since they have
their most significant part at the front.

Fixes #506
@arrodriguez
Copy link
Author

@magiconair Thank you for your response. I understand the problem.
How do you think that fabio has to work in this cases ?

while ago i had an issue in some project of firewall rules which uses wildcard host for matching. The difference with fabio is that this project permits the wildcard in any positiion.

The solution were to use "permuterm indexes". (https://nlp.stanford.edu/IR-book/html/htmledition/permuterm-indexes-1.html)

So in my opinion makes sense to rotate de entries in the route table index.

But i dont know if fabio wants this scope.

@magiconair
Copy link
Contributor

Fabio should always go from most specific to least specific when picking a match. However, it also allows a glob pattern anywhere in the host name. *.a.com and *.b.a.com should be sorted as *.b.a.com, *.a.com. However, I am not sure what the order should be for *.a.com and a.*.com. I am OK with leaving this undefined for now. The current implementation - which reverses the strings before sorting - would sort it as *.a.com, a.*.com. ForHost: a.a.com both matches seem equally good. (https://play.golang.org/p/uIMR2rpHOlw)

@magiconair magiconair added this to the 1.5.10 milestone Jun 10, 2018
@magiconair magiconair added the bug label Jun 10, 2018
@arrodriguez
Copy link
Author

Yes you are right. Its make sense leaving this like #507

magiconair added a commit that referenced this issue Jun 10, 2018
Issue #506: reverse domain names before sorting
@magiconair magiconair changed the title Domain precedence with glob fabio picks wrong route for multiple matching glob patterns Jun 10, 2018
@magiconair magiconair changed the title fabio picks wrong route for multiple matching glob patterns Wrong route for multiple matching glob patterns Jun 10, 2018
@magiconair magiconair changed the title Wrong route for multiple matching glob patterns Wrong route for multiple matching host glob patterns Jun 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants