From 88e85e42d3e43b07582e031c0b183cc48edab500 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Mon, 23 Mar 2020 19:10:23 +0100 Subject: [PATCH 1/5] route requests based on pattern or query parameters --- changelog/unreleased/advanced-route-matching.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/unreleased/advanced-route-matching.md diff --git a/changelog/unreleased/advanced-route-matching.md b/changelog/unreleased/advanced-route-matching.md new file mode 100644 index 000000000..815add61f --- /dev/null +++ b/changelog/unreleased/advanced-route-matching.md @@ -0,0 +1,6 @@ +Change: Route requests based on regex or query parameters + +Some requests needed to be distinguished based on a pattern or a query parameter. +We've implemented the functionality to route requests based on different conditions. + +https://github.com/owncloud/ocis-proxy/issues/21 From a7187777476cbf974ac0ee7f3011ee3dda149cd6 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Mon, 23 Mar 2020 19:13:46 +0100 Subject: [PATCH 2/5] route requests based on pattern or query parameters --- pkg/config/config.go | 20 ++++++++++ pkg/proxy/proxy.go | 94 ++++++++++++++++++++++++++++++++++++-------- 2 files changed, 98 insertions(+), 16 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index eca858189..426e705ba 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -44,11 +44,31 @@ type Policy struct { // Route define forwarding routes type Route struct { + Type RouteType Endpoint string Backend string ApacheVHost bool `mapstructure:"apache-vhost"` } +// RouteType defines the type of a route +type RouteType string + +const ( + // PrefixRoute are routes matched by a prefix + PrefixRoute RouteType = "Prefix" + // QueryRoute are routes machted by a prefix and query parameters + QueryRoute RouteType = "Query" + // RegexRoute are routes matched by a pattern + RegexRoute RouteType = "Regex" + // DefaultRouteType is the PrefixRoute + DefaultRouteType RouteType = PrefixRoute +) + +var ( + // RouteTypes is an array of the available route types + RouteTypes []RouteType = []RouteType{QueryRoute, RegexRoute, PrefixRoute} +) + // Config combines all available configuration parts. type Config struct { File string diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index b64482036..b3fb92e48 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -4,6 +4,7 @@ import ( "net/http" "net/http/httputil" "net/url" + "regexp" "strings" "github.com/owncloud/ocis-pkg/v2/log" @@ -13,7 +14,7 @@ import ( // MultiHostReverseProxy extends httputil to support multiple hosts with diffent policies type MultiHostReverseProxy struct { httputil.ReverseProxy - Directors map[string]map[string]func(req *http.Request) + Directors map[string]map[config.RouteType]map[string]func(req *http.Request) logger log.Logger } @@ -22,7 +23,7 @@ func NewMultiHostReverseProxy(opts ...Option) *MultiHostReverseProxy { options := newOptions(opts...) rp := &MultiHostReverseProxy{ - Directors: make(map[string]map[string]func(req *http.Request)), + Directors: make(map[string]map[config.RouteType]map[string]func(req *http.Request)), logger: options.Logger, } @@ -72,9 +73,16 @@ func singleJoiningSlash(a, b string) string { func (p *MultiHostReverseProxy) AddHost(policy string, target *url.URL, rt config.Route) { targetQuery := target.RawQuery if p.Directors[policy] == nil { - p.Directors[policy] = make(map[string]func(req *http.Request)) + p.Directors[policy] = make(map[config.RouteType]map[string]func(req *http.Request)) } - p.Directors[policy][rt.Endpoint] = func(req *http.Request) { + routeType := config.DefaultRouteType + if rt.Type != "" { + routeType = rt.Type + } + if p.Directors[policy][routeType] == nil { + p.Directors[policy][routeType] = make(map[string]func(req *http.Request)) + } + p.Directors[policy][routeType][rt.Endpoint] = func(req *http.Request) { req.URL.Scheme = target.Scheme req.URL.Host = target.Host // Apache deployments host addresses need to match on req.Host and req.URL.Host @@ -107,28 +115,77 @@ func (p *MultiHostReverseProxy) ServeHTTP(w http.ResponseWriter, r *http.Request Msgf("policy %v is not configured", policy) } - for k := range p.Directors[policy] { - if strings.HasPrefix(r.URL.Path, k) && k != "/" { - p.Director = p.Directors[policy][k] - hit = true - p.logger. - Debug(). - Str("policy", policy). - Str("prefix", k). - Str("path", r.URL.Path). - Msg("director found") + for i := 0; i < len(config.RouteTypes) && !hit; i++ { + routeType := config.RouteTypes[i] + var handler func(string, url.URL) bool + switch routeType { + case config.QueryRoute: + handler = p.queryRouteHandler + case config.RegexRoute: + handler = p.regexRouteHandler + case config.PrefixRoute: + fallthrough + default: + handler = p.prefixRouteHandler + } + for endpoint := range p.Directors[policy][routeType] { + if handler(endpoint, *r.URL) { + p.Director = p.Directors[policy][routeType][endpoint] + hit = true + p.logger. + Info(). + Str("policy", policy). + Str("prefix", endpoint). + Str("path", r.URL.Path). + Str("routeType", string(routeType)). + Msg("director found") + break + } } } // override default director with root. If any - if !hit && p.Directors[policy]["/"] != nil { - p.Director = p.Directors[policy]["/"] + if !hit && p.Directors[policy][config.PrefixRoute]["/"] != nil { + p.Director = p.Directors[policy][config.PrefixRoute]["/"] } // Call upstream ServeHTTP p.ReverseProxy.ServeHTTP(w, r) } +func (p MultiHostReverseProxy) queryRouteHandler(endpoint string, target url.URL) bool { + u, _ := url.Parse(endpoint) + if strings.HasPrefix(target.Path, u.Path) && endpoint != "/" { + query := u.Query() + if len(query) != 0 { + rQuery := target.Query() + match := true + for k := range query { + v := query.Get(k) + rv := rQuery.Get(k) + if rv != v { + match = false + break + } + } + return match + } + } + return false +} + +func (p *MultiHostReverseProxy) regexRouteHandler(endpoint string, target url.URL) bool { + matched, err := regexp.MatchString(endpoint, target.String()) + if err != nil { + p.logger.Warn().Err(err).Msgf("regex with pattern %s failed", endpoint) + } + return matched +} + +func (p *MultiHostReverseProxy) prefixRouteHandler(endpoint string, target url.URL) bool { + return strings.HasPrefix(target.Path, endpoint) && endpoint != "/" +} + func defaultPolicies() []config.Policy { return []config.Policy{ config.Policy{ @@ -154,6 +211,11 @@ func defaultPolicies() []config.Policy { Endpoint: "/ocs/", Backend: "http://localhost:9140", }, + config.Route{ + Type: config.QueryRoute, + Endpoint: "/remote.php/?preview=1", + Backend: "http://localhost:9115", + }, config.Route{ Endpoint: "/remote.php/", Backend: "http://localhost:9140", From eb539bc78ed222149ba8ae1ce4da1d143f6e1c41 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 24 Mar 2020 10:42:47 +0100 Subject: [PATCH 3/5] implement review feedback --- pkg/config/config.go | 6 +++--- pkg/proxy/proxy.go | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 426e705ba..f397c25ff 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -55,11 +55,11 @@ type RouteType string const ( // PrefixRoute are routes matched by a prefix - PrefixRoute RouteType = "Prefix" + PrefixRoute RouteType = "prefix" // QueryRoute are routes machted by a prefix and query parameters - QueryRoute RouteType = "Query" + QueryRoute RouteType = "query" // RegexRoute are routes matched by a pattern - RegexRoute RouteType = "Regex" + RegexRoute RouteType = "regex" // DefaultRouteType is the PrefixRoute DefaultRouteType RouteType = PrefixRoute ) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index b3fb92e48..748105cea 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -120,20 +120,20 @@ func (p *MultiHostReverseProxy) ServeHTTP(w http.ResponseWriter, r *http.Request var handler func(string, url.URL) bool switch routeType { case config.QueryRoute: - handler = p.queryRouteHandler + handler = p.queryRouteMatcher case config.RegexRoute: - handler = p.regexRouteHandler + handler = p.regexRouteMatcher case config.PrefixRoute: fallthrough default: - handler = p.prefixRouteHandler + handler = p.prefixRouteMatcher } for endpoint := range p.Directors[policy][routeType] { if handler(endpoint, *r.URL) { p.Director = p.Directors[policy][routeType][endpoint] hit = true p.logger. - Info(). + Debug(). Str("policy", policy). Str("prefix", endpoint). Str("path", r.URL.Path). @@ -153,7 +153,7 @@ func (p *MultiHostReverseProxy) ServeHTTP(w http.ResponseWriter, r *http.Request p.ReverseProxy.ServeHTTP(w, r) } -func (p MultiHostReverseProxy) queryRouteHandler(endpoint string, target url.URL) bool { +func (p MultiHostReverseProxy) queryRouteMatcher(endpoint string, target url.URL) bool { u, _ := url.Parse(endpoint) if strings.HasPrefix(target.Path, u.Path) && endpoint != "/" { query := u.Query() @@ -174,7 +174,7 @@ func (p MultiHostReverseProxy) queryRouteHandler(endpoint string, target url.URL return false } -func (p *MultiHostReverseProxy) regexRouteHandler(endpoint string, target url.URL) bool { +func (p *MultiHostReverseProxy) regexRouteMatcher(endpoint string, target url.URL) bool { matched, err := regexp.MatchString(endpoint, target.String()) if err != nil { p.logger.Warn().Err(err).Msgf("regex with pattern %s failed", endpoint) @@ -182,7 +182,7 @@ func (p *MultiHostReverseProxy) regexRouteHandler(endpoint string, target url.UR return matched } -func (p *MultiHostReverseProxy) prefixRouteHandler(endpoint string, target url.URL) bool { +func (p *MultiHostReverseProxy) prefixRouteMatcher(endpoint string, target url.URL) bool { return strings.HasPrefix(target.Path, endpoint) && endpoint != "/" } From 1cd3f8936d765fcc13996a39e8c259decb52f234 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 24 Mar 2020 10:43:01 +0100 Subject: [PATCH 4/5] add unit tests --- pkg/proxy/proxy_test.go | 112 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 pkg/proxy/proxy_test.go diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go new file mode 100644 index 000000000..7a5d3d64b --- /dev/null +++ b/pkg/proxy/proxy_test.go @@ -0,0 +1,112 @@ +package proxy + +import ( + "net/url" + "testing" + + "github.com/owncloud/ocis-proxy/pkg/config" +) + +func TestPrefixRouteMatcher(t *testing.T) { + cfg := config.New() + p := NewMultiHostReverseProxy(Config(cfg)) + + endpoint := "/foobar" + u, _ := url.Parse("/foobar/baz/some/url") + + matched := p.prefixRouteMatcher(endpoint, *u) + if !matched { + t.Errorf("Endpoint %s and URL %s should match", endpoint, u.String()) + } +} + +func TestQueryRouteMatcher(t *testing.T) { + cfg := config.New() + p := NewMultiHostReverseProxy(Config(cfg)) + + endpoint := "/foobar?parameter=true" + u, _ := url.Parse("/foobar/baz/some/url?parameter=true") + + matched := p.queryRouteMatcher(endpoint, *u) + if !matched { + t.Errorf("Endpoint %s and URL %s should match", endpoint, u.String()) + } +} + +func TestQueryRouteMatcherWithoutParameters(t *testing.T) { + cfg := config.New() + p := NewMultiHostReverseProxy(Config(cfg)) + + endpoint := "/foobar" + u, _ := url.Parse("/foobar/baz/some/url?parameter=true") + + matched := p.queryRouteMatcher(endpoint, *u) + if matched { + t.Errorf("Endpoint %s and URL %s should not match", endpoint, u.String()) + } +} + +func TestQueryRouteMatcherWithDifferingParameters(t *testing.T) { + cfg := config.New() + p := NewMultiHostReverseProxy(Config(cfg)) + + endpoint := "/foobar?parameter=false" + u, _ := url.Parse("/foobar/baz/some/url?parameter=true") + + matched := p.queryRouteMatcher(endpoint, *u) + if matched { + t.Errorf("Endpoint %s and URL %s should not match", endpoint, u.String()) + } +} + +func TestQueryRouteMatcherWithMultipleDifferingParameters(t *testing.T) { + cfg := config.New() + p := NewMultiHostReverseProxy(Config(cfg)) + + endpoint := "/foobar?parameter=false&other=true" + u, _ := url.Parse("/foobar/baz/some/url?parameter=true") + + matched := p.queryRouteMatcher(endpoint, *u) + if matched { + t.Errorf("Endpoint %s and URL %s should not match", endpoint, u.String()) + } +} + +func TestQueryRouteMatcherWithMultipleParameters(t *testing.T) { + cfg := config.New() + p := NewMultiHostReverseProxy(Config(cfg)) + + endpoint := "/foobar?parameter=false&other=true" + u, _ := url.Parse("/foobar/baz/some/url?parameter=false&other=true") + + matched := p.queryRouteMatcher(endpoint, *u) + if !matched { + t.Errorf("Endpoint %s and URL %s should match", endpoint, u.String()) + } +} + +func TestRegexRouteMatcher(t *testing.T) { + cfg := config.New() + p := NewMultiHostReverseProxy(Config(cfg)) + + endpoint := ".*some\\/url.*parameter=true" + u, _ := url.Parse("/foobar/baz/some/url?parameter=true") + + matched := p.regexRouteMatcher(endpoint, *u) + if !matched { + t.Errorf("Endpoint %s and URL %s should match", endpoint, u.String()) + } +} + +func TestRegexRouteMatcherWithInvalidPattern(t *testing.T) { + cfg := config.New() + p := NewMultiHostReverseProxy(Config(cfg)) + + endpoint := "([\\])\\w+" + u, _ := url.Parse("/foobar/baz/some/url?parameter=true") + + matched := p.regexRouteMatcher(endpoint, *u) + if matched { + t.Errorf("Endpoint %s and URL %s should not match", endpoint, u.String()) + } +} From a86e745c75e22be9d0678c706808578b1a6005de Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 24 Mar 2020 10:55:09 +0100 Subject: [PATCH 5/5] use labeled break --- pkg/proxy/proxy.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 748105cea..3f21fbd9d 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -115,10 +115,10 @@ func (p *MultiHostReverseProxy) ServeHTTP(w http.ResponseWriter, r *http.Request Msgf("policy %v is not configured", policy) } - for i := 0; i < len(config.RouteTypes) && !hit; i++ { - routeType := config.RouteTypes[i] +Loop: + for _, rt := range config.RouteTypes { var handler func(string, url.URL) bool - switch routeType { + switch rt { case config.QueryRoute: handler = p.queryRouteMatcher case config.RegexRoute: @@ -128,18 +128,18 @@ func (p *MultiHostReverseProxy) ServeHTTP(w http.ResponseWriter, r *http.Request default: handler = p.prefixRouteMatcher } - for endpoint := range p.Directors[policy][routeType] { + for endpoint := range p.Directors[policy][rt] { if handler(endpoint, *r.URL) { - p.Director = p.Directors[policy][routeType][endpoint] + p.Director = p.Directors[policy][rt][endpoint] hit = true p.logger. Debug(). Str("policy", policy). Str("prefix", endpoint). Str("path", r.URL.Path). - Str("routeType", string(routeType)). + Str("routeType", string(rt)). Msg("director found") - break + break Loop } } }