Skip to content

Commit

Permalink
Issue #506: reverse domain names before sorting
Browse files Browse the repository at this point in the history
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
  • Loading branch information
magiconair committed Jun 10, 2018
1 parent 4ea9d1b commit b934042
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 4 deletions.
35 changes: 34 additions & 1 deletion route/table.go
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,38 @@ func (t Table) matchingHosts(req *http.Request) (hosts []string) {
hosts = append(hosts, pattern)
}
}

if len(hosts) < 2 {
return
}

// Issue 506: multiple glob patterns hosts in wrong order
//
// DNS names have their most specific part at the front. In order to sort
// them from most specific to least specific a lexicographic sort will
// return the wrong result since it sorts by host name. *.foo.com will come
// before *.a.foo.com even though the latter is more specific. To achieve
// the correct result we need to reverse the strings, sort them and then
// reverse them again.
for i, h := range hosts {
hosts[i] = Reverse(h)
}
sort.Sort(sort.Reverse(sort.StringSlice(hosts)))
return hosts
for i, h := range hosts {
hosts[i] = Reverse(h)
}
return
}

// Reverse returns its argument string reversed rune-wise left to right.
//
// taken from https://github.com/golang/example/blob/master/stringutil/reverse.go
func Reverse(s string) string {
r := []rune(s)
for i, j := 0, len(r)-1; i < len(r)/2; i, j = i+1, j-1 {
r[i], r[j] = r[j], r[i]
}
return string(r)
}

// Lookup finds a target url based on the current matcher and picker
Expand All @@ -322,6 +352,9 @@ func (t Table) Lookup(req *http.Request, trace string, pick picker, match matche
// find matching hosts for the request
// and add "no host" as the fallback option
hosts := t.matchingHosts(req)
if trace != "" {
log.Printf("[TRACE] %s Matching hosts: %v", trace, hosts)
}
hosts = append(hosts, "")
for _, h := range hosts {
if target = t.lookup(h, req.URL.Path, trace, pick, match); target != nil {
Expand Down
9 changes: 6 additions & 3 deletions route/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,8 @@ 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 *.aaa.abc.com/ http://foo.com:6000
route add svc *.bbb.abc.com/ http://foo.com:6100
route add svc xyz.com:80/ https://xyz.com
`

Expand Down Expand Up @@ -611,8 +612,10 @@ func TestTableLookup(t *testing.T) {
{&http.Request{Host: ".abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000"},
{&http.Request{Host: "x.y.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000"},
{&http.Request{Host: "y.abc.com:80", URL: mustParse("/foo/")}, "http://foo.com:5000"},
{&http.Request{Host: "x.def.abc.com", URL: mustParse("/")}, "http://foo.com:6000"},
{&http.Request{Host: "x.def.abc.com", URL: mustParse("/foo")}, "http://foo.com:6000"},
{&http.Request{Host: "x.aaa.abc.com", URL: mustParse("/")}, "http://foo.com:6000"},
{&http.Request{Host: "x.aaa.abc.com", URL: mustParse("/foo")}, "http://foo.com:6000"},
{&http.Request{Host: "x.bbb.abc.com", URL: mustParse("/")}, "http://foo.com:6100"},
{&http.Request{Host: "x.bbb.abc.com", URL: mustParse("/foo")}, "http://foo.com:6100"},
{&http.Request{Host: "y.abc.com:443", URL: mustParse("/foo/"), TLS: &tls.ConnectionState{}}, "http://foo.com:5000"},

// exact match has precedence over glob match
Expand Down

0 comments on commit b934042

Please sign in to comment.