From 062a8ea6952a50dd3bf5cf734de37e669112bf6c Mon Sep 17 00:00:00 2001 From: JeremyWhite Date: Mon, 17 Sep 2018 13:29:16 -0500 Subject: [PATCH 1/7] Issue #548 added enable/disable glob matching --- config/config.go | 21 ++++++++++---------- config/default.go | 1 + config/load.go | 1 + fabio.properties | 11 +++++++++++ main.go | 2 +- proxy/http_integration_test.go | 29 ++++++++++++++++++++------- proxy/listen_test.go | 4 +++- proxy/ws_integration_test.go | 7 +++++-- route/issue57_test.go | 6 +++++- route/route_bench_test.go | 5 ++++- route/table.go | 36 ++++++++++++++++++++++++++-------- route/table_test.go | 9 +++++++-- 12 files changed, 99 insertions(+), 33 deletions(-) diff --git a/config/config.go b/config/config.go index f8ad7e4c2..8d0fee04c 100644 --- a/config/config.go +++ b/config/config.go @@ -7,16 +7,17 @@ import ( ) type Config struct { - Proxy Proxy - Registry Registry - Listen []Listen - Log Log - Metrics Metrics - UI UI - Runtime Runtime - ProfileMode string - ProfilePath string - Insecure bool + Proxy Proxy + Registry Registry + Listen []Listen + Log Log + Metrics Metrics + UI UI + Runtime Runtime + ProfileMode string + ProfilePath string + Insecure bool + GlobMatching bool } type CertSource struct { diff --git a/config/default.go b/config/default.go index 45f68424d..55fed35bb 100644 --- a/config/default.go +++ b/config/default.go @@ -77,4 +77,5 @@ var defaultConfig = &Config{ Color: "light-green", Access: "rw", }, + GlobMatching: true, } diff --git a/config/load.go b/config/load.go index d75188471..7992a446a 100644 --- a/config/load.go +++ b/config/load.go @@ -186,6 +186,7 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c f.StringVar(&cfg.UI.Title, "ui.title", defaultConfig.UI.Title, "optional title for the UI") f.StringVar(&cfg.ProfileMode, "profile.mode", defaultConfig.ProfileMode, "enable profiling mode, one of [cpu, mem, mutex, block]") f.StringVar(&cfg.ProfilePath, "profile.path", defaultConfig.ProfilePath, "path to profile dump file") + f.BoolVar(&cfg.GlobMatching, "glob.matching.enabled", defaultConfig.GlobMatching, "Enable/Disable Glob Matching on routes, one of [true, false]") // deprecated flags var proxyLogRoutes string diff --git a/fabio.properties b/fabio.properties index dae2caab0..2dc80dc85 100644 --- a/fabio.properties +++ b/fabio.properties @@ -743,6 +743,17 @@ # registry.consul.checksRequired = one +# glob.matching.enabled Enables glob matching on route lookups +# If glob matching is enabled there is a performance decrease +# for every route lookup. At a large number of services (> 500) this +# can have a significant impact on performance. If glob matching is disabled +# Fabio performs a static string compare for route lookups. +# +# The default is +# +# glob.matching.enabled = true + + # metrics.target configures the backend the metrics values are # sent to. # diff --git a/main.go b/main.go index 264c30978..769a68997 100644 --- a/main.go +++ b/main.go @@ -183,7 +183,7 @@ func newHTTPProxy(cfg *config.Config) http.Handler { Transport: newTransport(nil), InsecureTransport: newTransport(&tls.Config{InsecureSkipVerify: true}), Lookup: func(r *http.Request) *route.Target { - t := route.GetTable().Lookup(r, r.Header.Get("trace"), pick, match) + t := route.GetTable().Lookup(r, r.Header.Get("trace"), pick, match, cfg) if t == nil { notFound.Inc(1) log.Print("[WARN] No route for ", r.Host, r.URL) diff --git a/proxy/http_integration_test.go b/proxy/http_integration_test.go index ddc6c4c4c..e67280ba2 100644 --- a/proxy/http_integration_test.go +++ b/proxy/http_integration_test.go @@ -175,6 +175,8 @@ func TestProxyNoRouteStatus(t *testing.T) { } func TestProxyStripsPath(t *testing.T) { + //Glob Matching True + globMatching := config.Config{GlobMatching: true} server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.RequestURI { case "/bar": @@ -188,7 +190,7 @@ func TestProxyStripsPath(t *testing.T) { Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add mock /foo/bar " + server.URL + ` opts "strip=/foo"`) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"]) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) }, }) defer proxy.Close() @@ -215,6 +217,8 @@ func TestProxyHost(t *testing.T) { routes += "route add mock /hostcustom http://b.com/ opts \"host=bar.com\"\n" routes += "route add mock / http://a.com/" tbl, _ := route.NewTable(routes) + //Glob Matching True + globMatching := config.Config{GlobMatching: true} proxy := httptest.NewServer(&HTTPProxy{ Transport: &http.Transport{ @@ -224,7 +228,7 @@ func TestProxyHost(t *testing.T) { }, }, Lookup: func(r *http.Request) *route.Target { - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"]) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) }, }) defer proxy.Close() @@ -266,12 +270,14 @@ func TestHostRedirect(t *testing.T) { routes := "route add https-redir *:80 https://$host$path opts \"redirect=301\"\n" tbl, _ := route.NewTable(routes) + //Glob Matching True + globMatching := config.Config{GlobMatching: true} proxy := httptest.NewServer(&HTTPProxy{ Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { r.Host = "c.com" - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"]) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) }, }) defer proxy.Close() @@ -306,11 +312,13 @@ func TestPathRedirect(t *testing.T) { routes += "route add mock /foo http://a.com/abc opts \"redirect=301\"\n" routes += "route add mock /bar http://b.com/$path opts \"redirect=302 strip=/bar\"\n" tbl, _ := route.NewTable(routes) + //Glob Matching True + globMatching := config.Config{GlobMatching: true} proxy := httptest.NewServer(&HTTPProxy{ Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"]) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) }, }) defer proxy.Close() @@ -472,13 +480,15 @@ func TestProxyHTTPSUpstream(t *testing.T) { server.TLS = tlsServerConfig() server.StartTLS() defer server.Close() + //Glob Matching True + globMatching := config.Config{GlobMatching: true} proxy := httptest.NewServer(&HTTPProxy{ Config: config.Proxy{}, Transport: &http.Transport{TLSClientConfig: tlsClientConfig()}, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add srv / " + server.URL + ` opts "proto=https"`) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"]) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) }, }) defer proxy.Close() @@ -497,6 +507,8 @@ func TestProxyHTTPSUpstreamSkipVerify(t *testing.T) { server.TLS = &tls.Config{} server.StartTLS() defer server.Close() + //Glob Matching True + globMatching := config.Config{GlobMatching: true} proxy := httptest.NewServer(&HTTPProxy{ Config: config.Proxy{}, @@ -506,7 +518,7 @@ func TestProxyHTTPSUpstreamSkipVerify(t *testing.T) { }, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add srv / " + server.URL + ` opts "proto=https tlsskipverify=true"`) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"]) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) }, }) defer proxy.Close() @@ -699,6 +711,9 @@ func BenchmarkProxyLogger(b *testing.B) { b.Fatal("logger.NewHTTPLogger:", err) } + //Glob Matching True + globMatching := config.Config{GlobMatching: true} + proxy := &HTTPProxy{ Config: config.Proxy{ LocalIP: "1.1.1.1", @@ -707,7 +722,7 @@ func BenchmarkProxyLogger(b *testing.B) { Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add mock / " + server.URL) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"]) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) }, Logger: l, } diff --git a/proxy/listen_test.go b/proxy/listen_test.go index 607461641..ae307d27a 100644 --- a/proxy/listen_test.go +++ b/proxy/listen_test.go @@ -22,6 +22,8 @@ func TestGracefulShutdown(t *testing.T) { })) defer srv.Close() + //Glob Matching True + globMatching := config.Config{GlobMatching: true} // start proxy addr := "127.0.0.1:57777" var wg sync.WaitGroup @@ -32,7 +34,7 @@ func TestGracefulShutdown(t *testing.T) { Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add svc / " + srv.URL) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"]) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) }, } l := config.Listen{Addr: addr} diff --git a/proxy/ws_integration_test.go b/proxy/ws_integration_test.go index a03977481..f1a9d8035 100644 --- a/proxy/ws_integration_test.go +++ b/proxy/ws_integration_test.go @@ -33,6 +33,9 @@ func TestProxyWSUpstream(t *testing.T) { defer wssServer.Close() t.Log("Started WSS server: ", wssServer.URL) + //Glob Matching True + globMatching := config.Config{GlobMatching: true} + routes := "route add ws /ws " + wsServer.URL + "\n" routes += "route add ws /wss " + wssServer.URL + ` opts "proto=https"` + "\n" routes += "route add ws /insecure " + wssServer.URL + ` opts "proto=https tlsskipverify=true"` + "\n" @@ -44,7 +47,7 @@ func TestProxyWSUpstream(t *testing.T) { InsecureTransport: &http.Transport{TLSClientConfig: tlsInsecureConfig()}, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable(routes) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"]) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) }, }) defer httpProxy.Close() @@ -56,7 +59,7 @@ func TestProxyWSUpstream(t *testing.T) { InsecureTransport: &http.Transport{TLSClientConfig: tlsInsecureConfig()}, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable(routes) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"]) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) }, }) httpsProxy.TLS = tlsServerConfig() diff --git a/route/issue57_test.go b/route/issue57_test.go index a0f8fb2cc..c52835996 100644 --- a/route/issue57_test.go +++ b/route/issue57_test.go @@ -1,6 +1,7 @@ package route import ( + "github.com/fabiolb/fabio/config" "net/http" "testing" ) @@ -24,6 +25,9 @@ func TestIssue57(t *testing.T) { route del svcb`, } + //Glob Matching True + globMatching := config.Config{GlobMatching: true} + req := &http.Request{URL: mustParse("/foo")} want := "http://foo.com:800" @@ -32,7 +36,7 @@ func TestIssue57(t *testing.T) { if err != nil { t.Fatalf("%d: got %v want nil", i, err) } - target := tbl.Lookup(req, "", rrPicker, prefixMatcher) + target := tbl.Lookup(req, "", rrPicker, prefixMatcher, &globMatching) if target == nil { t.Fatalf("%d: got %v want %v", i, target, want) } diff --git a/route/route_bench_test.go b/route/route_bench_test.go index 2f9126710..e11dd32be 100644 --- a/route/route_bench_test.go +++ b/route/route_bench_test.go @@ -2,6 +2,7 @@ package route import ( "fmt" + "github.com/fabiolb/fabio/config" "net/http" "sync" "testing" @@ -123,8 +124,10 @@ func makeRequests(t Table) []*http.Request { func benchmarkGet(t Table, match matcher, pick picker, pb *testing.PB) { reqs := makeRequests(t) k, n := len(reqs), 0 + //Glob Matching True + globMatching := config.Config{GlobMatching: true} for pb.Next() { - t.Lookup(reqs[n%k], "", pick, match) + t.Lookup(reqs[n%k], "", pick, match, &globMatching) n++ } } diff --git a/route/table.go b/route/table.go index 117543e77..caaf6b18d 100644 --- a/route/table.go +++ b/route/table.go @@ -12,6 +12,7 @@ import ( "sync" "sync/atomic" + "github.com/fabiolb/fabio/config" "github.com/fabiolb/fabio/metrics" "github.com/gobwas/glob" ) @@ -295,13 +296,32 @@ func normalizeHost(host string, tls bool) string { // matchingHosts returns all keys (host name patterns) from the // routing table which match the normalized request hostname. -func (t Table) matchingHosts(req *http.Request) (hosts []string) { +func (t Table) matchingHosts(req *http.Request, cfg *config.Config) (hosts []string) { host := normalizeHost(req.Host, req.TLS != nil) - for pattern := range t { - normpat := normalizeHost(pattern, req.TLS != nil) - g := glob.MustCompile(normpat) - if g.Match(host) { - hosts = append(hosts, pattern) + + // Issue 548 Glob matching causes performance decrease. + // + // Updated config to allow for disabling of Glob Matches + // Standard string compare is used if disabled + // glob.matching.enabled is false + if !cfg.GlobMatching { + for pattern := range t { + normpat := normalizeHost(pattern, req.TLS != nil) + if normpat == host { + //log.Printf("DEBUG Matched %s and %s", normpat, host) + hosts = append(hosts, pattern) + return + } + } + } else { //glob.matching.enabled is true (default) Performance hit + for pattern := range t { + normpat := normalizeHost(pattern, req.TLS != nil) + // TODO setup compiled GLOBs in a separate MAP as routes are added/deleted + // TODO Issue #548 + g := glob.MustCompile(normpat) + if g.Match(host) { + hosts = append(hosts, pattern) + } } } @@ -342,7 +362,7 @@ func Reverse(s string) string { // or nil if there is none. It first checks the routes for the host // and if none matches then it falls back to generic routes without // a host. This is useful for a catch-all '/' rule. -func (t Table) Lookup(req *http.Request, trace string, pick picker, match matcher) (target *Target) { +func (t Table) Lookup(req *http.Request, trace string, pick picker, match matcher, cfg *config.Config) (target *Target) { if trace != "" { if len(trace) > 16 { trace = trace[:15] @@ -352,7 +372,7 @@ 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) + hosts := t.matchingHosts(req, cfg) if trace != "" { log.Printf("[TRACE] %s Matching hosts: %v", trace, hosts) } diff --git a/route/table_test.go b/route/table_test.go index 808c9168b..e0ac39407 100644 --- a/route/table_test.go +++ b/route/table_test.go @@ -3,6 +3,7 @@ package route import ( "crypto/tls" "fmt" + "github.com/fabiolb/fabio/config" "math" "net/http" "reflect" @@ -492,6 +493,8 @@ func TestTableLookupIssue448(t *testing.T) { route add mock ccc.com:443/bar https://ccc.com/baz opts "redirect=301" route add mock / http://foo.com/ ` + //Glob Matching True + globMatching := config.Config{GlobMatching: true} tbl, err := NewTable(s) if err != nil { @@ -551,7 +554,7 @@ func TestTableLookupIssue448(t *testing.T) { } for i, tt := range tests { - if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher).URL.String(), tt.dst; got != want { + if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher, &globMatching).URL.String(), tt.dst; got != want { t.Errorf("%d: got %v want %v", i, got, want) } } @@ -573,6 +576,8 @@ func TestTableLookup(t *testing.T) { route add svc *.bbb.abc.com/ http://foo.com:6100 route add svc xyz.com:80/ https://xyz.com ` + //Glob Matching True + globMatching := config.Config{GlobMatching: true} tbl, err := NewTable(s) if err != nil { @@ -626,7 +631,7 @@ func TestTableLookup(t *testing.T) { } for i, tt := range tests { - if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher).URL.String(), tt.dst; got != want { + if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher, &globMatching).URL.String(), tt.dst; got != want { t.Errorf("%d: got %v want %v", i, got, want) } } From 054be87eb902561882d6a5b65321c0a644680a75 Mon Sep 17 00:00:00 2001 From: JeremyWhite Date: Wed, 19 Sep 2018 11:24:23 -0500 Subject: [PATCH 2/7] updated docs for glob.matching.enabled --- docs/content/ref/glob.matching.enabled | 0 docs/content/ref/glob.matching.enabled.md | 11 +++++++++++ 2 files changed, 11 insertions(+) create mode 100644 docs/content/ref/glob.matching.enabled create mode 100644 docs/content/ref/glob.matching.enabled.md diff --git a/docs/content/ref/glob.matching.enabled b/docs/content/ref/glob.matching.enabled new file mode 100644 index 000000000..e69de29bb diff --git a/docs/content/ref/glob.matching.enabled.md b/docs/content/ref/glob.matching.enabled.md new file mode 100644 index 000000000..9f7cdac82 --- /dev/null +++ b/docs/content/ref/glob.matching.enabled.md @@ -0,0 +1,11 @@ +--- +title: "glob.matching.enabled" +--- + +`glob.matching.enabled` Enables/Disables glob matching on route lookups. + +Valid options are `true`, `false` + +The default is + + glob.matching.enabled = true From eda6cf412fd0ffe971b30d1d9f85a86f87422ba1 Mon Sep 17 00:00:00 2001 From: JeremyWhite Date: Wed, 19 Sep 2018 11:30:22 -0500 Subject: [PATCH 3/7] Updated Docs for glob.matching.enabled --- docs/content/ref/glob.matching.enabled | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 docs/content/ref/glob.matching.enabled diff --git a/docs/content/ref/glob.matching.enabled b/docs/content/ref/glob.matching.enabled deleted file mode 100644 index e69de29bb..000000000 From ea75fb8d3056af0033a42bb49145b1050ebf7425 Mon Sep 17 00:00:00 2001 From: JeremyWhite Date: Wed, 19 Sep 2018 16:40:44 -0500 Subject: [PATCH 4/7] updated to pass in bool instead of config.Config to route package --- config/config.go | 22 +++---- config/default.go | 2 +- config/load.go | 2 +- docs/content/ref/glob.matching.disabled.md | 11 ++++ docs/content/ref/glob.matching.enabled.md | 11 ---- fabio.properties | 4 +- main.go | 3 +- proxy/http_integration_test.go | 34 +++++----- proxy/listen_test.go | 6 +- proxy/ws_integration_test.go | 7 +-- route/issue57_test.go | 6 +- route/route_bench_test.go | 5 +- route/table.go | 11 ++-- route/table_test.go | 73 +++++++++++----------- 14 files changed, 92 insertions(+), 105 deletions(-) create mode 100644 docs/content/ref/glob.matching.disabled.md delete mode 100644 docs/content/ref/glob.matching.enabled.md diff --git a/config/config.go b/config/config.go index 8d0fee04c..495614725 100644 --- a/config/config.go +++ b/config/config.go @@ -7,17 +7,17 @@ import ( ) type Config struct { - Proxy Proxy - Registry Registry - Listen []Listen - Log Log - Metrics Metrics - UI UI - Runtime Runtime - ProfileMode string - ProfilePath string - Insecure bool - GlobMatching bool + Proxy Proxy + Registry Registry + Listen []Listen + Log Log + Metrics Metrics + UI UI + Runtime Runtime + ProfileMode string + ProfilePath string + Insecure bool + DisableGlobMatching bool } type CertSource struct { diff --git a/config/default.go b/config/default.go index 55fed35bb..d46ba9bc6 100644 --- a/config/default.go +++ b/config/default.go @@ -77,5 +77,5 @@ var defaultConfig = &Config{ Color: "light-green", Access: "rw", }, - GlobMatching: true, + DisableGlobMatching: false, } diff --git a/config/load.go b/config/load.go index 7992a446a..47f012c58 100644 --- a/config/load.go +++ b/config/load.go @@ -186,7 +186,7 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c f.StringVar(&cfg.UI.Title, "ui.title", defaultConfig.UI.Title, "optional title for the UI") f.StringVar(&cfg.ProfileMode, "profile.mode", defaultConfig.ProfileMode, "enable profiling mode, one of [cpu, mem, mutex, block]") f.StringVar(&cfg.ProfilePath, "profile.path", defaultConfig.ProfilePath, "path to profile dump file") - f.BoolVar(&cfg.GlobMatching, "glob.matching.enabled", defaultConfig.GlobMatching, "Enable/Disable Glob Matching on routes, one of [true, false]") + f.BoolVar(&cfg.DisableGlobMatching, "glob.matching.disabled", defaultConfig.DisableGlobMatching, "Disable Glob Matching on routes, one of [true, false]") // deprecated flags var proxyLogRoutes string diff --git a/docs/content/ref/glob.matching.disabled.md b/docs/content/ref/glob.matching.disabled.md new file mode 100644 index 000000000..5ab2fd06d --- /dev/null +++ b/docs/content/ref/glob.matching.disabled.md @@ -0,0 +1,11 @@ +--- +title: "glob.matching.disabled" +--- + +`glob.matching.disabled` Disables glob matching on route lookups. + +Valid options are `true`, `false` + +The default is + + glob.matching.disabled = false diff --git a/docs/content/ref/glob.matching.enabled.md b/docs/content/ref/glob.matching.enabled.md deleted file mode 100644 index 9f7cdac82..000000000 --- a/docs/content/ref/glob.matching.enabled.md +++ /dev/null @@ -1,11 +0,0 @@ ---- -title: "glob.matching.enabled" ---- - -`glob.matching.enabled` Enables/Disables glob matching on route lookups. - -Valid options are `true`, `false` - -The default is - - glob.matching.enabled = true diff --git a/fabio.properties b/fabio.properties index 2dc80dc85..28f7d994d 100644 --- a/fabio.properties +++ b/fabio.properties @@ -743,7 +743,7 @@ # registry.consul.checksRequired = one -# glob.matching.enabled Enables glob matching on route lookups +# glob.matching.disabled Disables glob matching on route lookups # If glob matching is enabled there is a performance decrease # for every route lookup. At a large number of services (> 500) this # can have a significant impact on performance. If glob matching is disabled @@ -751,7 +751,7 @@ # # The default is # -# glob.matching.enabled = true +# glob.matching.disabled = false # metrics.target configures the backend the metrics values are diff --git a/main.go b/main.go index 769a68997..041102a27 100644 --- a/main.go +++ b/main.go @@ -137,6 +137,7 @@ func main() { func newHTTPProxy(cfg *config.Config) http.Handler { var w io.Writer + globDisabled := cfg.DisableGlobMatching switch cfg.Log.AccessTarget { case "": log.Printf("[INFO] Access logging disabled") @@ -183,7 +184,7 @@ func newHTTPProxy(cfg *config.Config) http.Handler { Transport: newTransport(nil), InsecureTransport: newTransport(&tls.Config{InsecureSkipVerify: true}), Lookup: func(r *http.Request) *route.Target { - t := route.GetTable().Lookup(r, r.Header.Get("trace"), pick, match, cfg) + t := route.GetTable().Lookup(r, r.Header.Get("trace"), pick, match, globDisabled) if t == nil { notFound.Inc(1) log.Print("[WARN] No route for ", r.Host, r.URL) diff --git a/proxy/http_integration_test.go b/proxy/http_integration_test.go index e67280ba2..92cbcce8b 100644 --- a/proxy/http_integration_test.go +++ b/proxy/http_integration_test.go @@ -176,7 +176,7 @@ func TestProxyNoRouteStatus(t *testing.T) { func TestProxyStripsPath(t *testing.T) { //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.RequestURI { case "/bar": @@ -190,7 +190,7 @@ func TestProxyStripsPath(t *testing.T) { Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add mock /foo/bar " + server.URL + ` opts "strip=/foo"`) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) }, }) defer proxy.Close() @@ -217,8 +217,7 @@ func TestProxyHost(t *testing.T) { routes += "route add mock /hostcustom http://b.com/ opts \"host=bar.com\"\n" routes += "route add mock / http://a.com/" tbl, _ := route.NewTable(routes) - //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false proxy := httptest.NewServer(&HTTPProxy{ Transport: &http.Transport{ @@ -228,7 +227,7 @@ func TestProxyHost(t *testing.T) { }, }, Lookup: func(r *http.Request) *route.Target { - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) }, }) defer proxy.Close() @@ -270,14 +269,13 @@ func TestHostRedirect(t *testing.T) { routes := "route add https-redir *:80 https://$host$path opts \"redirect=301\"\n" tbl, _ := route.NewTable(routes) - //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false proxy := httptest.NewServer(&HTTPProxy{ Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { r.Host = "c.com" - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) }, }) defer proxy.Close() @@ -312,13 +310,12 @@ func TestPathRedirect(t *testing.T) { routes += "route add mock /foo http://a.com/abc opts \"redirect=301\"\n" routes += "route add mock /bar http://b.com/$path opts \"redirect=302 strip=/bar\"\n" tbl, _ := route.NewTable(routes) - //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false proxy := httptest.NewServer(&HTTPProxy{ Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) }, }) defer proxy.Close() @@ -480,15 +477,14 @@ func TestProxyHTTPSUpstream(t *testing.T) { server.TLS = tlsServerConfig() server.StartTLS() defer server.Close() - //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false proxy := httptest.NewServer(&HTTPProxy{ Config: config.Proxy{}, Transport: &http.Transport{TLSClientConfig: tlsClientConfig()}, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add srv / " + server.URL + ` opts "proto=https"`) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) }, }) defer proxy.Close() @@ -507,8 +503,7 @@ func TestProxyHTTPSUpstreamSkipVerify(t *testing.T) { server.TLS = &tls.Config{} server.StartTLS() defer server.Close() - //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false proxy := httptest.NewServer(&HTTPProxy{ Config: config.Proxy{}, @@ -518,7 +513,7 @@ func TestProxyHTTPSUpstreamSkipVerify(t *testing.T) { }, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add srv / " + server.URL + ` opts "proto=https tlsskipverify=true"`) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) }, }) defer proxy.Close() @@ -711,8 +706,7 @@ func BenchmarkProxyLogger(b *testing.B) { b.Fatal("logger.NewHTTPLogger:", err) } - //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false proxy := &HTTPProxy{ Config: config.Proxy{ @@ -722,7 +716,7 @@ func BenchmarkProxyLogger(b *testing.B) { Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add mock / " + server.URL) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) }, Logger: l, } diff --git a/proxy/listen_test.go b/proxy/listen_test.go index ae307d27a..c795e51b2 100644 --- a/proxy/listen_test.go +++ b/proxy/listen_test.go @@ -22,8 +22,8 @@ func TestGracefulShutdown(t *testing.T) { })) defer srv.Close() - //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false + // start proxy addr := "127.0.0.1:57777" var wg sync.WaitGroup @@ -34,7 +34,7 @@ func TestGracefulShutdown(t *testing.T) { Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add svc / " + srv.URL) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) }, } l := config.Listen{Addr: addr} diff --git a/proxy/ws_integration_test.go b/proxy/ws_integration_test.go index f1a9d8035..853371ca2 100644 --- a/proxy/ws_integration_test.go +++ b/proxy/ws_integration_test.go @@ -33,8 +33,7 @@ func TestProxyWSUpstream(t *testing.T) { defer wssServer.Close() t.Log("Started WSS server: ", wssServer.URL) - //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false routes := "route add ws /ws " + wsServer.URL + "\n" routes += "route add ws /wss " + wssServer.URL + ` opts "proto=https"` + "\n" @@ -47,7 +46,7 @@ func TestProxyWSUpstream(t *testing.T) { InsecureTransport: &http.Transport{TLSClientConfig: tlsInsecureConfig()}, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable(routes) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) }, }) defer httpProxy.Close() @@ -59,7 +58,7 @@ func TestProxyWSUpstream(t *testing.T) { InsecureTransport: &http.Transport{TLSClientConfig: tlsInsecureConfig()}, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable(routes) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], &globMatching) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) }, }) httpsProxy.TLS = tlsServerConfig() diff --git a/route/issue57_test.go b/route/issue57_test.go index c52835996..e8d412462 100644 --- a/route/issue57_test.go +++ b/route/issue57_test.go @@ -1,7 +1,6 @@ package route import ( - "github.com/fabiolb/fabio/config" "net/http" "testing" ) @@ -25,8 +24,7 @@ func TestIssue57(t *testing.T) { route del svcb`, } - //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false req := &http.Request{URL: mustParse("/foo")} want := "http://foo.com:800" @@ -36,7 +34,7 @@ func TestIssue57(t *testing.T) { if err != nil { t.Fatalf("%d: got %v want nil", i, err) } - target := tbl.Lookup(req, "", rrPicker, prefixMatcher, &globMatching) + target := tbl.Lookup(req, "", rrPicker, prefixMatcher, globDisabled) if target == nil { t.Fatalf("%d: got %v want %v", i, target, want) } diff --git a/route/route_bench_test.go b/route/route_bench_test.go index e11dd32be..2a5b511be 100644 --- a/route/route_bench_test.go +++ b/route/route_bench_test.go @@ -2,7 +2,6 @@ package route import ( "fmt" - "github.com/fabiolb/fabio/config" "net/http" "sync" "testing" @@ -125,9 +124,9 @@ func benchmarkGet(t Table, match matcher, pick picker, pb *testing.PB) { reqs := makeRequests(t) k, n := len(reqs), 0 //Glob Matching True - globMatching := config.Config{GlobMatching: true} + globDisabled := false for pb.Next() { - t.Lookup(reqs[n%k], "", pick, match, &globMatching) + t.Lookup(reqs[n%k], "", pick, match, globDisabled) n++ } } diff --git a/route/table.go b/route/table.go index caaf6b18d..e4485f6f4 100644 --- a/route/table.go +++ b/route/table.go @@ -12,7 +12,6 @@ import ( "sync" "sync/atomic" - "github.com/fabiolb/fabio/config" "github.com/fabiolb/fabio/metrics" "github.com/gobwas/glob" ) @@ -296,7 +295,7 @@ func normalizeHost(host string, tls bool) string { // matchingHosts returns all keys (host name patterns) from the // routing table which match the normalized request hostname. -func (t Table) matchingHosts(req *http.Request, cfg *config.Config) (hosts []string) { +func (t Table) matchingHosts(req *http.Request, globDisabled bool) (hosts []string) { host := normalizeHost(req.Host, req.TLS != nil) // Issue 548 Glob matching causes performance decrease. @@ -304,7 +303,7 @@ func (t Table) matchingHosts(req *http.Request, cfg *config.Config) (hosts []str // Updated config to allow for disabling of Glob Matches // Standard string compare is used if disabled // glob.matching.enabled is false - if !cfg.GlobMatching { + if globDisabled { for pattern := range t { normpat := normalizeHost(pattern, req.TLS != nil) if normpat == host { @@ -316,7 +315,7 @@ func (t Table) matchingHosts(req *http.Request, cfg *config.Config) (hosts []str } else { //glob.matching.enabled is true (default) Performance hit for pattern := range t { normpat := normalizeHost(pattern, req.TLS != nil) - // TODO setup compiled GLOBs in a separate MAP as routes are added/deleted + // TODO setup compiled GLOBs in a separate MAP // TODO Issue #548 g := glob.MustCompile(normpat) if g.Match(host) { @@ -362,7 +361,7 @@ func Reverse(s string) string { // or nil if there is none. It first checks the routes for the host // and if none matches then it falls back to generic routes without // a host. This is useful for a catch-all '/' rule. -func (t Table) Lookup(req *http.Request, trace string, pick picker, match matcher, cfg *config.Config) (target *Target) { +func (t Table) Lookup(req *http.Request, trace string, pick picker, match matcher, globDisabled bool) (target *Target) { if trace != "" { if len(trace) > 16 { trace = trace[:15] @@ -372,7 +371,7 @@ 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, cfg) + hosts := t.matchingHosts(req, globDisabled) if trace != "" { log.Printf("[TRACE] %s Matching hosts: %v", trace, hosts) } diff --git a/route/table_test.go b/route/table_test.go index e0ac39407..d9ef05429 100644 --- a/route/table_test.go +++ b/route/table_test.go @@ -3,7 +3,6 @@ package route import ( "crypto/tls" "fmt" - "github.com/fabiolb/fabio/config" "math" "net/http" "reflect" @@ -493,8 +492,6 @@ func TestTableLookupIssue448(t *testing.T) { route add mock ccc.com:443/bar https://ccc.com/baz opts "redirect=301" route add mock / http://foo.com/ ` - //Glob Matching True - globMatching := config.Config{GlobMatching: true} tbl, err := NewTable(s) if err != nil { @@ -502,8 +499,9 @@ func TestTableLookupIssue448(t *testing.T) { } var tests = []struct { - req *http.Request - dst string + req *http.Request + dst string + globDisabled bool }{ { req: &http.Request{ @@ -554,7 +552,7 @@ func TestTableLookupIssue448(t *testing.T) { } for i, tt := range tests { - if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher, &globMatching).URL.String(), tt.dst; got != want { + if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher, tt.globDisabled).URL.String(), tt.dst; got != want { t.Errorf("%d: got %v want %v", i, got, want) } } @@ -576,8 +574,6 @@ func TestTableLookup(t *testing.T) { route add svc *.bbb.abc.com/ http://foo.com:6100 route add svc xyz.com:80/ https://xyz.com ` - //Glob Matching True - globMatching := config.Config{GlobMatching: true} tbl, err := NewTable(s) if err != nil { @@ -585,53 +581,54 @@ func TestTableLookup(t *testing.T) { } var tests = []struct { - req *http.Request - dst string + req *http.Request + dst string + globDisabled bool }{ // match on host and path with and without trailing slash - {&http.Request{Host: "abc.com", URL: mustParse("/")}, "http://foo.com:1000"}, - {&http.Request{Host: "abc.com", URL: mustParse("/bar")}, "http://foo.com:1000"}, - {&http.Request{Host: "abc.com", URL: mustParse("/foo")}, "http://foo.com:1500"}, - {&http.Request{Host: "abc.com", URL: mustParse("/foo/")}, "http://foo.com:2000"}, - {&http.Request{Host: "abc.com", URL: mustParse("/foo/bar")}, "http://foo.com:2500"}, - {&http.Request{Host: "abc.com", URL: mustParse("/foo/bar/")}, "http://foo.com:3000"}, + {&http.Request{Host: "abc.com", URL: mustParse("/")}, "http://foo.com:1000", false}, + {&http.Request{Host: "abc.com", URL: mustParse("/bar")}, "http://foo.com:1000", false}, + {&http.Request{Host: "abc.com", URL: mustParse("/foo")}, "http://foo.com:1500", false}, + {&http.Request{Host: "abc.com", URL: mustParse("/foo/")}, "http://foo.com:2000", false}, + {&http.Request{Host: "abc.com", URL: mustParse("/foo/bar")}, "http://foo.com:2500", false}, + {&http.Request{Host: "abc.com", URL: mustParse("/foo/bar/")}, "http://foo.com:3000", false}, // do not match on host but maybe on path - {&http.Request{Host: "def.com", URL: mustParse("/")}, "http://foo.com:800"}, - {&http.Request{Host: "def.com", URL: mustParse("/bar")}, "http://foo.com:800"}, - {&http.Request{Host: "def.com", URL: mustParse("/foo")}, "http://foo.com:900"}, + {&http.Request{Host: "def.com", URL: mustParse("/")}, "http://foo.com:800", false}, + {&http.Request{Host: "def.com", URL: mustParse("/bar")}, "http://foo.com:800", false}, + {&http.Request{Host: "def.com", URL: mustParse("/foo")}, "http://foo.com:900", false}, // strip default port - {&http.Request{Host: "abc.com:80", URL: mustParse("/")}, "http://foo.com:1000"}, - {&http.Request{Host: "abc.com:443", URL: mustParse("/"), TLS: &tls.ConnectionState{}}, "http://foo.com:1000"}, + {&http.Request{Host: "abc.com:80", URL: mustParse("/")}, "http://foo.com:1000", false}, + {&http.Request{Host: "abc.com:443", URL: mustParse("/"), TLS: &tls.ConnectionState{}}, "http://foo.com:1000", false}, // not using default port - {&http.Request{Host: "abc.com:443", URL: mustParse("/")}, "http://foo.com:800"}, - {&http.Request{Host: "abc.com:80", URL: mustParse("/"), TLS: &tls.ConnectionState{}}, "http://foo.com:800"}, + {&http.Request{Host: "abc.com:443", URL: mustParse("/")}, "http://foo.com:800", false}, + {&http.Request{Host: "abc.com:80", URL: mustParse("/"), TLS: &tls.ConnectionState{}}, "http://foo.com:800", false}, // glob match the host - {&http.Request{Host: "x.abc.com", URL: mustParse("/")}, "http://foo.com:4000"}, - {&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"}, - {&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.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"}, + {&http.Request{Host: "x.abc.com", URL: mustParse("/")}, "http://foo.com:4000", false}, + {&http.Request{Host: "y.abc.com", URL: mustParse("/abc")}, "http://foo.com:4000", false}, + {&http.Request{Host: "x.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", false}, + {&http.Request{Host: "y.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", false}, + {&http.Request{Host: ".abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", false}, + {&http.Request{Host: "x.y.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", false}, + {&http.Request{Host: "y.abc.com:80", URL: mustParse("/foo/")}, "http://foo.com:5000", false}, + {&http.Request{Host: "x.aaa.abc.com", URL: mustParse("/")}, "http://foo.com:6000", false}, + {&http.Request{Host: "x.aaa.abc.com", URL: mustParse("/foo")}, "http://foo.com:6000", false}, + {&http.Request{Host: "x.bbb.abc.com", URL: mustParse("/")}, "http://foo.com:6100", false}, + {&http.Request{Host: "x.bbb.abc.com", URL: mustParse("/foo")}, "http://foo.com:6100", false}, + {&http.Request{Host: "y.abc.com:443", URL: mustParse("/foo/"), TLS: &tls.ConnectionState{}}, "http://foo.com:5000", false}, // exact match has precedence over glob match - {&http.Request{Host: "z.abc.com", URL: mustParse("/foo/")}, "http://foo.com:3100"}, + {&http.Request{Host: "z.abc.com", URL: mustParse("/foo/")}, "http://foo.com:3100", false}, // explicit port on route - {&http.Request{Host: "xyz.com", URL: mustParse("/")}, "https://xyz.com"}, + {&http.Request{Host: "xyz.com", URL: mustParse("/")}, "https://xyz.com", false}, } for i, tt := range tests { - if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher, &globMatching).URL.String(), tt.dst; got != want { + if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher, tt.globDisabled).URL.String(), tt.dst; got != want { t.Errorf("%d: got %v want %v", i, got, want) } } From 1eb2e36e40964e34d71df57fc926d78b6345c573 Mon Sep 17 00:00:00 2001 From: JeremyWhite Date: Thu, 20 Sep 2018 09:54:59 -0500 Subject: [PATCH 5/7] added separate func for no glob matching --- route/table.go | 63 +++++++++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/route/table.go b/route/table.go index e4485f6f4..8c07fb9b0 100644 --- a/route/table.go +++ b/route/table.go @@ -295,32 +295,15 @@ func normalizeHost(host string, tls bool) string { // matchingHosts returns all keys (host name patterns) from the // routing table which match the normalized request hostname. -func (t Table) matchingHosts(req *http.Request, globDisabled bool) (hosts []string) { +func (t Table) matchingHosts(req *http.Request,) (hosts []string) { host := normalizeHost(req.Host, req.TLS != nil) - - // Issue 548 Glob matching causes performance decrease. - // - // Updated config to allow for disabling of Glob Matches - // Standard string compare is used if disabled - // glob.matching.enabled is false - if globDisabled { - for pattern := range t { - normpat := normalizeHost(pattern, req.TLS != nil) - if normpat == host { - //log.Printf("DEBUG Matched %s and %s", normpat, host) - hosts = append(hosts, pattern) - return - } - } - } else { //glob.matching.enabled is true (default) Performance hit - for pattern := range t { - normpat := normalizeHost(pattern, req.TLS != nil) - // TODO setup compiled GLOBs in a separate MAP - // TODO Issue #548 - g := glob.MustCompile(normpat) - if g.Match(host) { - hosts = append(hosts, pattern) - } + for pattern := range t { + normpat := normalizeHost(pattern, req.TLS != nil) + // TODO setup compiled GLOBs in a separate MAP + // TODO Issue #548 + g := glob.MustCompile(normpat) + if g.Match(host) { + hosts = append(hosts, pattern) } } @@ -346,6 +329,25 @@ func (t Table) matchingHosts(req *http.Request, globDisabled bool) (hosts []stri return } + +// Issue 548 - Added separate func +// +// matchingHostNoGlob returns the route from the +// routing table which matches the normalized request hostname. +func (t Table) matchingHostNoGlob(req *http.Request) (hosts []string) { + host := normalizeHost(req.Host, req.TLS != nil) + + for pattern := range t { + normpat := normalizeHost(pattern, req.TLS != nil) + if normpat == host { + //log.Printf("DEBUG Matched %s and %s", normpat, host) + hosts = append(hosts, pattern) + return + } + } + return +} + // Reverse returns its argument string reversed rune-wise left to right. // // taken from https://github.com/golang/example/blob/master/stringutil/reverse.go @@ -362,6 +364,8 @@ func Reverse(s string) string { // and if none matches then it falls back to generic routes without // a host. This is useful for a catch-all '/' rule. func (t Table) Lookup(req *http.Request, trace string, pick picker, match matcher, globDisabled bool) (target *Target) { + + var hosts []string if trace != "" { if len(trace) > 16 { trace = trace[:15] @@ -371,7 +375,14 @@ 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, globDisabled) + // if globDisabled then match without Glob + // Issue 548 + if globDisabled { + hosts = t.matchingHostNoGlob(req) + } else { + hosts = t.matchingHosts(req) + } + if trace != "" { log.Printf("[TRACE] %s Matching hosts: %v", trace, hosts) } From da6726851704258a98d60bae7edc5608f249ae86 Mon Sep 17 00:00:00 2001 From: JeremyWhite Date: Thu, 20 Sep 2018 14:43:25 -0500 Subject: [PATCH 6/7] updated test with CONST, var clean up --- config/default.go | 1 - docs/content/ref/glob.matching.disabled.md | 2 +- fabio.properties | 2 +- main.go | 3 +- proxy/http_integration_test.go | 31 +++++----- route/issue57_test.go | 3 +- route/route_bench_test.go | 3 +- route/table_test.go | 68 ++++++++++++---------- 8 files changed, 57 insertions(+), 56 deletions(-) diff --git a/config/default.go b/config/default.go index d46ba9bc6..45f68424d 100644 --- a/config/default.go +++ b/config/default.go @@ -77,5 +77,4 @@ var defaultConfig = &Config{ Color: "light-green", Access: "rw", }, - DisableGlobMatching: false, } diff --git a/docs/content/ref/glob.matching.disabled.md b/docs/content/ref/glob.matching.disabled.md index 5ab2fd06d..ec1e41e00 100644 --- a/docs/content/ref/glob.matching.disabled.md +++ b/docs/content/ref/glob.matching.disabled.md @@ -2,7 +2,7 @@ title: "glob.matching.disabled" --- -`glob.matching.disabled` Disables glob matching on route lookups. +`glob.matching.disabled` disables glob matching on route lookups. Valid options are `true`, `false` diff --git a/fabio.properties b/fabio.properties index 28f7d994d..207c17da3 100644 --- a/fabio.properties +++ b/fabio.properties @@ -743,7 +743,7 @@ # registry.consul.checksRequired = one -# glob.matching.disabled Disables glob matching on route lookups +# glob.matching.disabled disables glob matching on route lookups # If glob matching is enabled there is a performance decrease # for every route lookup. At a large number of services (> 500) this # can have a significant impact on performance. If glob matching is disabled diff --git a/main.go b/main.go index 041102a27..3ccf43b97 100644 --- a/main.go +++ b/main.go @@ -137,7 +137,6 @@ func main() { func newHTTPProxy(cfg *config.Config) http.Handler { var w io.Writer - globDisabled := cfg.DisableGlobMatching switch cfg.Log.AccessTarget { case "": log.Printf("[INFO] Access logging disabled") @@ -184,7 +183,7 @@ func newHTTPProxy(cfg *config.Config) http.Handler { Transport: newTransport(nil), InsecureTransport: newTransport(&tls.Config{InsecureSkipVerify: true}), Lookup: func(r *http.Request) *route.Target { - t := route.GetTable().Lookup(r, r.Header.Get("trace"), pick, match, globDisabled) + t := route.GetTable().Lookup(r, r.Header.Get("trace"), pick, match, cfg.DisableGlobMatching) if t == nil { notFound.Inc(1) log.Print("[WARN] No route for ", r.Host, r.URL) diff --git a/proxy/http_integration_test.go b/proxy/http_integration_test.go index 92cbcce8b..4f223ffb2 100644 --- a/proxy/http_integration_test.go +++ b/proxy/http_integration_test.go @@ -27,6 +27,13 @@ import ( "github.com/pascaldekloe/goe/verify" ) + +const ( + // helper constants for the Lookup function + globEnabled = false + globDisabled = true +) + func TestProxyProducesCorrectXForwardedSomethingHeader(t *testing.T) { var hdr http.Header = make(http.Header) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -175,8 +182,6 @@ func TestProxyNoRouteStatus(t *testing.T) { } func TestProxyStripsPath(t *testing.T) { - //Glob Matching True - globDisabled := false server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.RequestURI { case "/bar": @@ -190,7 +195,7 @@ func TestProxyStripsPath(t *testing.T) { Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add mock /foo/bar " + server.URL + ` opts "strip=/foo"`) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globEnabled) }, }) defer proxy.Close() @@ -217,7 +222,6 @@ func TestProxyHost(t *testing.T) { routes += "route add mock /hostcustom http://b.com/ opts \"host=bar.com\"\n" routes += "route add mock / http://a.com/" tbl, _ := route.NewTable(routes) - globDisabled := false proxy := httptest.NewServer(&HTTPProxy{ Transport: &http.Transport{ @@ -227,7 +231,7 @@ func TestProxyHost(t *testing.T) { }, }, Lookup: func(r *http.Request) *route.Target { - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globEnabled) }, }) defer proxy.Close() @@ -269,13 +273,12 @@ func TestHostRedirect(t *testing.T) { routes := "route add https-redir *:80 https://$host$path opts \"redirect=301\"\n" tbl, _ := route.NewTable(routes) - globDisabled := false proxy := httptest.NewServer(&HTTPProxy{ Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { r.Host = "c.com" - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globEnabled) }, }) defer proxy.Close() @@ -310,12 +313,11 @@ func TestPathRedirect(t *testing.T) { routes += "route add mock /foo http://a.com/abc opts \"redirect=301\"\n" routes += "route add mock /bar http://b.com/$path opts \"redirect=302 strip=/bar\"\n" tbl, _ := route.NewTable(routes) - globDisabled := false proxy := httptest.NewServer(&HTTPProxy{ Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globEnabled) }, }) defer proxy.Close() @@ -477,14 +479,13 @@ func TestProxyHTTPSUpstream(t *testing.T) { server.TLS = tlsServerConfig() server.StartTLS() defer server.Close() - globDisabled := false proxy := httptest.NewServer(&HTTPProxy{ Config: config.Proxy{}, Transport: &http.Transport{TLSClientConfig: tlsClientConfig()}, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add srv / " + server.URL + ` opts "proto=https"`) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globEnabled) }, }) defer proxy.Close() @@ -503,8 +504,6 @@ func TestProxyHTTPSUpstreamSkipVerify(t *testing.T) { server.TLS = &tls.Config{} server.StartTLS() defer server.Close() - globDisabled := false - proxy := httptest.NewServer(&HTTPProxy{ Config: config.Proxy{}, Transport: http.DefaultTransport, @@ -513,7 +512,7 @@ func TestProxyHTTPSUpstreamSkipVerify(t *testing.T) { }, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add srv / " + server.URL + ` opts "proto=https tlsskipverify=true"`) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globEnabled) }, }) defer proxy.Close() @@ -706,7 +705,7 @@ func BenchmarkProxyLogger(b *testing.B) { b.Fatal("logger.NewHTTPLogger:", err) } - globDisabled := false + proxy := &HTTPProxy{ Config: config.Proxy{ @@ -716,7 +715,7 @@ func BenchmarkProxyLogger(b *testing.B) { Transport: http.DefaultTransport, Lookup: func(r *http.Request) *route.Target { tbl, _ := route.NewTable("route add mock / " + server.URL) - return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globDisabled) + return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"], globEnabled) }, Logger: l, } diff --git a/route/issue57_test.go b/route/issue57_test.go index e8d412462..17082b4d5 100644 --- a/route/issue57_test.go +++ b/route/issue57_test.go @@ -24,7 +24,6 @@ func TestIssue57(t *testing.T) { route del svcb`, } - globDisabled := false req := &http.Request{URL: mustParse("/foo")} want := "http://foo.com:800" @@ -34,7 +33,7 @@ func TestIssue57(t *testing.T) { if err != nil { t.Fatalf("%d: got %v want nil", i, err) } - target := tbl.Lookup(req, "", rrPicker, prefixMatcher, globDisabled) + target := tbl.Lookup(req, "", rrPicker, prefixMatcher, globEnabled) if target == nil { t.Fatalf("%d: got %v want %v", i, target, want) } diff --git a/route/route_bench_test.go b/route/route_bench_test.go index 2a5b511be..e5693c572 100644 --- a/route/route_bench_test.go +++ b/route/route_bench_test.go @@ -124,9 +124,8 @@ func benchmarkGet(t Table, match matcher, pick picker, pb *testing.PB) { reqs := makeRequests(t) k, n := len(reqs), 0 //Glob Matching True - globDisabled := false for pb.Next() { - t.Lookup(reqs[n%k], "", pick, match, globDisabled) + t.Lookup(reqs[n%k], "", pick, match, globEnabled) n++ } } diff --git a/route/table_test.go b/route/table_test.go index d9ef05429..7d20ecfc6 100644 --- a/route/table_test.go +++ b/route/table_test.go @@ -11,6 +11,12 @@ import ( "testing" ) +const ( + // helper constants for the Lookup function + globEnabled = false + globDisabled = true +) + func TestTableParse(t *testing.T) { genRoutes := func(n int, format string) (a []string) { for i := 0; i < n; i++ { @@ -501,7 +507,7 @@ func TestTableLookupIssue448(t *testing.T) { var tests = []struct { req *http.Request dst string - globDisabled bool + globEnabled bool }{ { req: &http.Request{ @@ -552,7 +558,7 @@ func TestTableLookupIssue448(t *testing.T) { } for i, tt := range tests { - if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher, tt.globDisabled).URL.String(), tt.dst; got != want { + if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher, globEnabled).URL.String(), tt.dst; got != want { t.Errorf("%d: got %v want %v", i, got, want) } } @@ -583,52 +589,52 @@ func TestTableLookup(t *testing.T) { var tests = []struct { req *http.Request dst string - globDisabled bool + globEnabled bool }{ // match on host and path with and without trailing slash - {&http.Request{Host: "abc.com", URL: mustParse("/")}, "http://foo.com:1000", false}, - {&http.Request{Host: "abc.com", URL: mustParse("/bar")}, "http://foo.com:1000", false}, - {&http.Request{Host: "abc.com", URL: mustParse("/foo")}, "http://foo.com:1500", false}, - {&http.Request{Host: "abc.com", URL: mustParse("/foo/")}, "http://foo.com:2000", false}, - {&http.Request{Host: "abc.com", URL: mustParse("/foo/bar")}, "http://foo.com:2500", false}, - {&http.Request{Host: "abc.com", URL: mustParse("/foo/bar/")}, "http://foo.com:3000", false}, + {&http.Request{Host: "abc.com", URL: mustParse("/")}, "http://foo.com:1000", globEnabled}, + {&http.Request{Host: "abc.com", URL: mustParse("/bar")}, "http://foo.com:1000", globEnabled}, + {&http.Request{Host: "abc.com", URL: mustParse("/foo")}, "http://foo.com:1500", globEnabled}, + {&http.Request{Host: "abc.com", URL: mustParse("/foo/")}, "http://foo.com:2000", globEnabled}, + {&http.Request{Host: "abc.com", URL: mustParse("/foo/bar")}, "http://foo.com:2500", globEnabled}, + {&http.Request{Host: "abc.com", URL: mustParse("/foo/bar/")}, "http://foo.com:3000", globEnabled}, // do not match on host but maybe on path - {&http.Request{Host: "def.com", URL: mustParse("/")}, "http://foo.com:800", false}, - {&http.Request{Host: "def.com", URL: mustParse("/bar")}, "http://foo.com:800", false}, - {&http.Request{Host: "def.com", URL: mustParse("/foo")}, "http://foo.com:900", false}, + {&http.Request{Host: "def.com", URL: mustParse("/")}, "http://foo.com:800", globEnabled}, + {&http.Request{Host: "def.com", URL: mustParse("/bar")}, "http://foo.com:800", globEnabled}, + {&http.Request{Host: "def.com", URL: mustParse("/foo")}, "http://foo.com:900", globEnabled}, // strip default port - {&http.Request{Host: "abc.com:80", URL: mustParse("/")}, "http://foo.com:1000", false}, - {&http.Request{Host: "abc.com:443", URL: mustParse("/"), TLS: &tls.ConnectionState{}}, "http://foo.com:1000", false}, + {&http.Request{Host: "abc.com:80", URL: mustParse("/")}, "http://foo.com:1000", globEnabled}, + {&http.Request{Host: "abc.com:443", URL: mustParse("/"), TLS: &tls.ConnectionState{}}, "http://foo.com:1000", globEnabled}, // not using default port - {&http.Request{Host: "abc.com:443", URL: mustParse("/")}, "http://foo.com:800", false}, - {&http.Request{Host: "abc.com:80", URL: mustParse("/"), TLS: &tls.ConnectionState{}}, "http://foo.com:800", false}, + {&http.Request{Host: "abc.com:443", URL: mustParse("/")}, "http://foo.com:800", globEnabled}, + {&http.Request{Host: "abc.com:80", URL: mustParse("/"), TLS: &tls.ConnectionState{}}, "http://foo.com:800", globEnabled}, // glob match the host - {&http.Request{Host: "x.abc.com", URL: mustParse("/")}, "http://foo.com:4000", false}, - {&http.Request{Host: "y.abc.com", URL: mustParse("/abc")}, "http://foo.com:4000", false}, - {&http.Request{Host: "x.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", false}, - {&http.Request{Host: "y.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", false}, - {&http.Request{Host: ".abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", false}, - {&http.Request{Host: "x.y.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", false}, - {&http.Request{Host: "y.abc.com:80", URL: mustParse("/foo/")}, "http://foo.com:5000", false}, - {&http.Request{Host: "x.aaa.abc.com", URL: mustParse("/")}, "http://foo.com:6000", false}, - {&http.Request{Host: "x.aaa.abc.com", URL: mustParse("/foo")}, "http://foo.com:6000", false}, - {&http.Request{Host: "x.bbb.abc.com", URL: mustParse("/")}, "http://foo.com:6100", false}, - {&http.Request{Host: "x.bbb.abc.com", URL: mustParse("/foo")}, "http://foo.com:6100", false}, - {&http.Request{Host: "y.abc.com:443", URL: mustParse("/foo/"), TLS: &tls.ConnectionState{}}, "http://foo.com:5000", false}, + {&http.Request{Host: "x.abc.com", URL: mustParse("/")}, "http://foo.com:4000", globEnabled}, + {&http.Request{Host: "y.abc.com", URL: mustParse("/abc")}, "http://foo.com:4000", globEnabled}, + {&http.Request{Host: "x.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", globEnabled}, + {&http.Request{Host: "y.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", globEnabled}, + {&http.Request{Host: ".abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", globEnabled}, + {&http.Request{Host: "x.y.abc.com", URL: mustParse("/foo/")}, "http://foo.com:5000", globEnabled}, + {&http.Request{Host: "y.abc.com:80", URL: mustParse("/foo/")}, "http://foo.com:5000", globEnabled}, + {&http.Request{Host: "x.aaa.abc.com", URL: mustParse("/")}, "http://foo.com:6000", globEnabled}, + {&http.Request{Host: "x.aaa.abc.com", URL: mustParse("/foo")}, "http://foo.com:6000", globEnabled}, + {&http.Request{Host: "x.bbb.abc.com", URL: mustParse("/")}, "http://foo.com:6100", globEnabled}, + {&http.Request{Host: "x.bbb.abc.com", URL: mustParse("/foo")}, "http://foo.com:6100", globEnabled}, + {&http.Request{Host: "y.abc.com:443", URL: mustParse("/foo/"), TLS: &tls.ConnectionState{}}, "http://foo.com:5000", globEnabled}, // exact match has precedence over glob match - {&http.Request{Host: "z.abc.com", URL: mustParse("/foo/")}, "http://foo.com:3100", false}, + {&http.Request{Host: "z.abc.com", URL: mustParse("/foo/")}, "http://foo.com:3100", globEnabled}, // explicit port on route - {&http.Request{Host: "xyz.com", URL: mustParse("/")}, "https://xyz.com", false}, + {&http.Request{Host: "xyz.com", URL: mustParse("/")}, "https://xyz.com", globEnabled}, } for i, tt := range tests { - if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher, tt.globDisabled).URL.String(), tt.dst; got != want { + if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher, tt.globEnabled).URL.String(), tt.dst; got != want { t.Errorf("%d: got %v want %v", i, got, want) } } From 89201539183df7ba05f8a1cfc3704297914b38fb Mon Sep 17 00:00:00 2001 From: JeremyWhite Date: Thu, 20 Sep 2018 14:48:50 -0500 Subject: [PATCH 7/7] go fmt of files --- proxy/http_integration_test.go | 5 +---- route/issue57_test.go | 1 - route/table.go | 3 +-- route/table_test.go | 10 +++++----- 4 files changed, 7 insertions(+), 12 deletions(-) diff --git a/proxy/http_integration_test.go b/proxy/http_integration_test.go index 4f223ffb2..2132d3dd9 100644 --- a/proxy/http_integration_test.go +++ b/proxy/http_integration_test.go @@ -27,10 +27,9 @@ import ( "github.com/pascaldekloe/goe/verify" ) - const ( // helper constants for the Lookup function - globEnabled = false + globEnabled = false globDisabled = true ) @@ -705,8 +704,6 @@ func BenchmarkProxyLogger(b *testing.B) { b.Fatal("logger.NewHTTPLogger:", err) } - - proxy := &HTTPProxy{ Config: config.Proxy{ LocalIP: "1.1.1.1", diff --git a/route/issue57_test.go b/route/issue57_test.go index 17082b4d5..c87dd8b00 100644 --- a/route/issue57_test.go +++ b/route/issue57_test.go @@ -24,7 +24,6 @@ func TestIssue57(t *testing.T) { route del svcb`, } - req := &http.Request{URL: mustParse("/foo")} want := "http://foo.com:800" diff --git a/route/table.go b/route/table.go index 8c07fb9b0..4e12fc182 100644 --- a/route/table.go +++ b/route/table.go @@ -295,7 +295,7 @@ func normalizeHost(host string, tls bool) string { // matchingHosts returns all keys (host name patterns) from the // routing table which match the normalized request hostname. -func (t Table) matchingHosts(req *http.Request,) (hosts []string) { +func (t Table) matchingHosts(req *http.Request) (hosts []string) { host := normalizeHost(req.Host, req.TLS != nil) for pattern := range t { normpat := normalizeHost(pattern, req.TLS != nil) @@ -329,7 +329,6 @@ func (t Table) matchingHosts(req *http.Request,) (hosts []string) { return } - // Issue 548 - Added separate func // // matchingHostNoGlob returns the route from the diff --git a/route/table_test.go b/route/table_test.go index 7d20ecfc6..824693c48 100644 --- a/route/table_test.go +++ b/route/table_test.go @@ -13,7 +13,7 @@ import ( const ( // helper constants for the Lookup function - globEnabled = false + globEnabled = false globDisabled = true ) @@ -505,8 +505,8 @@ func TestTableLookupIssue448(t *testing.T) { } var tests = []struct { - req *http.Request - dst string + req *http.Request + dst string globEnabled bool }{ { @@ -587,8 +587,8 @@ func TestTableLookup(t *testing.T) { } var tests = []struct { - req *http.Request - dst string + req *http.Request + dst string globEnabled bool }{ // match on host and path with and without trailing slash