From 454918da671d94412f1c1a62c0ec13041f81682e Mon Sep 17 00:00:00 2001 From: "simon.mittag" Date: Sat, 15 Apr 2023 12:10:33 +1000 Subject: [PATCH] updated route matcher for exact and prefix matching. added lots of test cases from Kubernetes --- config.go | 4 +- config_test.go | 14 ++++++ route.go | 11 ++++- route_test.go | 129 ++++++++++++++++++++++++++++++------------------- 4 files changed, 106 insertions(+), 52 deletions(-) diff --git a/config.go b/config.go index 9da9494..a3f6438 100644 --- a/config.go +++ b/config.go @@ -171,6 +171,8 @@ func (config Config) reApplyResourceNames() *Config { var routePathTypes = NewRoutePathTypes() +const prefixS = "prefix" + func (config Config) validateRoutes() *Config { //prep routes with leading slash for i, _ := range config.Routes { @@ -183,7 +185,7 @@ func (config Config) validateRoutes() *Config { } } if len(config.Routes[i].PathType) == 0 { - config.Routes[i].PathType = "prefix" + config.Routes[i].PathType = prefixS } else { if !routePathTypes.isValid(config.Routes[i].PathType) { config.panic(fmt.Sprintf("path type %s invalid, not one of ['prefix', 'exact']", config.Routes[i].PathType)) diff --git a/config_test.go b/config_test.go index 9cdbdaf..5a8124a 100644 --- a/config_test.go +++ b/config_test.go @@ -1057,3 +1057,17 @@ func TestParseDisableXRequestInfoFalse(t *testing.T) { t.Error("X-Request-Info should be enabled") } } + +func TestConfigValidationPanicsForInvalidRouteType(t *testing.T) { + defer func() { + if r := recover(); r == nil { + t.Errorf("config should have panicked") + } + }() + + config := new(Config).readYmlFile("./j8acfg.yml") + //thou shall not pass! + config.Routes[0].PathType = "blah" + + config = config.validateRoutes() +} diff --git a/route.go b/route.go index 87316ef..45b4e08 100644 --- a/route.go +++ b/route.go @@ -52,8 +52,17 @@ func (route *Route) compilePath() error { // TODO this matches a request to a route but it depends on sort order of multiple // routes matched, it will match the first one. +const slashS = "/" + func (route Route) matchURI(request *http.Request) bool { - return route.CompiledPathRegex.MatchString(request.RequestURI) + match := route.CompiledPathRegex.MatchString(request.URL.Path) + if !match && + route.PathType == prefixS && + len(request.URL.Path) > 0 && + string(request.URL.Path[len(request.URL.Path)-1]) != slashS { + match = route.CompiledPathRegex.MatchString(request.URL.Path + slashS) + } + return match } const upstreamResourceMapped = "upstream resource mapped" diff --git a/route_test.go b/route_test.go index 566bad5..ed7f023 100644 --- a/route_test.go +++ b/route_test.go @@ -7,86 +7,115 @@ import ( ) func TestRouteMatchRoot(t *testing.T) { - routeNoMatch("/", "", t) - routeMatch("/", "/some", t) - routeMatch("/", "/", t) - routeMatch("/", "/some/more", t) - routeMatch("/", "/some/more?param", t) - routeMatch("/", "/some/more?param=value", t) - routeMatch("/", "/some/more?param=value¶m2=value2", t) + routePrefixNoMatch("/", "", t) + routePrefixMatch("/", "/some", t) + routePrefixMatch("/", "/", t) + routePrefixMatch("/", "/some/more", t) + routePrefixMatch("/", "/some/more?param", t) + routePrefixMatch("/", "/some/more?param=value", t) + routePrefixMatch("/", "/some/more?param=value¶m2=value2", t) //path is never empty string, http server inserts "/" } func TestRouteMatchWithSlug(t *testing.T) { - routeNoMatch("/so", "so", t) - routeNoMatch("/so", "/os", t) - routeMatch("/so", "/some", t) - routeMatch("/so", "/some/more", t) - routeMatch("/so", "/some/more?param", t) - routeMatch("/so", "/some/more?param=value", t) - routeMatch("/so", "/some/more?param=value¶m2=value2", t) + routePrefixNoMatch("/so", "so", t) + routePrefixNoMatch("/so", "/os", t) + routePrefixMatch("/so", "/some", t) + routePrefixMatch("/so", "/some/more", t) + routePrefixMatch("/so", "/some/more?param", t) + routePrefixMatch("/so", "/some/more?param=value", t) + routePrefixMatch("/so", "/some/more?param=value¶m2=value2", t) } func TestRouteMatchWithTerminatedSlug(t *testing.T) { - routeNoMatch("/some/", "some", t) - routeNoMatch("/some/", "", t) - routeNoMatch("/some/", "/", t) - routeNoMatch("/some/", "/some", t) - routeNoMatch("/some/", "/want/some/", t) - routeMatch("/some/", "/some/", t) - routeMatch("/some/", "/some/more", t) - routeMatch("/some/", "/some/more?param", t) - routeMatch("/some/", "/some/more?param=value", t) - routeMatch("/some/", "/some/more?param=value¶m2=value2", t) + routePrefixNoMatch("/some/", "some", t) + routePrefixNoMatch("/some/", "", t) + routePrefixNoMatch("/some/", "/", t) + routePrefixMatch("/some/", "/some", t) + routePrefixNoMatch("/some/", "/want/some/", t) + routePrefixMatch("/some/", "/some/", t) + routePrefixMatch("/some/", "/some/more", t) + routePrefixMatch("/some/", "/some/more?param", t) + routePrefixMatch("/some/", "/some/more?param=value", t) + routePrefixMatch("/some/", "/some/more?param=value¶m2=value2", t) } func TestRouteMatchWithWildcardSlug(t *testing.T) { - routeNoMatch("*/some/", "some", t) - routeNoMatch("*/some/", "", t) - routeNoMatch("*/some/", "/", t) - routeNoMatch("*/some/", "/some", t) - routeMatch("*/some/", "/want/some/", t) - routeMatch("*/some/", "/really/want/some/", t) - routeMatch("*/some/", "/really/want/some/more", t) - routeMatch("*/some/", "/really/want/some/more?with=params", t) - routeMatch("*/some/", "/really/want/some/more/", t) - routeMatch("*/some/", "/some/", t) - routeMatch("*/some/", "/some/more", t) - routeMatch("*/some/", "/some/more?param", t) - routeMatch("*/some/", "/some/more?param=value", t) - routeMatch("*/some/", "/some/more?param=value¶m2=value2", t) + routePrefixNoMatch("*/some/", "some", t) + routePrefixNoMatch("*/some/", "", t) + routePrefixNoMatch("*/some/", "/", t) + routePrefixMatch("*/some/", "/want/some/", t) + routePrefixMatch("*/some/", "/really/want/some/", t) + routePrefixMatch("*/some/", "/really/want/some/more", t) + routePrefixMatch("*/some/", "/really/want/some/more?with=params", t) + routePrefixMatch("*/some/", "/really/want/some/more/", t) + //trailing slash is appended, then matches + routePrefixMatch("*/some/", "/some", t) + routePrefixMatch("*/some/", "/some/", t) + routePrefixMatch("*/some/", "/some/more", t) + routePrefixMatch("*/some/", "/some/more?param", t) + routePrefixMatch("*/some/", "/some/more?param=value", t) + routePrefixMatch("*/some/", "/some/more?param=value¶m2=value2", t) } // some of these do not match because regex greedy matches to "/" func TestRouteMatchWithAbsoluteWildcardSlug(t *testing.T) { - routeNoMatch("/*/some/", "/want/some/", t) - routeNoMatch("/*/some/", "/really/want/some/", t) - routeNoMatch("/*/some/", "/really/want/some/more", t) - routeNoMatch("/*/some/", "/really/want/some/more?with=params", t) - routeNoMatch("/*/some/", "/really/want/some/more/", t) + routePrefixNoMatch("/*/some/", "/want/some/", t) + routePrefixNoMatch("/*/some/", "/really/want/some/", t) + routePrefixNoMatch("/*/some/", "/really/want/some/more", t) + routePrefixNoMatch("/*/some/", "/really/want/some/more?with=params", t) + routePrefixNoMatch("/*/some/", "/really/want/some/more/", t) } -// TODO: test trailing slashes better. Do we want matches? -// TODO: test query strings func TestRouteExactMatch(t *testing.T) { + routeExactMatch("/some/", "/some/?k=v", t) routeExactMatch("/some/", "/some/", t) routeExactNoMatch("/some/", "/some", t) routeExactMatch("/some/index.html", "/some/index.html", t) + routeExactMatch("/some/index.html", "/some/index.html?k=v", t) + routeExactMatch("/some/index.html", "/some/index.html?k=v&k2=v2", t) routeExactNoMatch("/some/index.html", "/some/index", t) routeExactNoMatch("/some/index.html", "/some/index.", t) routeExactNoMatch("/some/index.html", "/some/index.htm", t) } -func routeMatch(route string, path string, t *testing.T) { - r := routeFactory(route) +func TestKubernetesIngressExamples(t *testing.T) { + routePrefixMatch("/", "/any/thing?k=v", t) + routeExactMatch("/foo", "/foo", t) + routeExactNoMatch("/foo", "/bar", t) + routeExactNoMatch("/foo", "/foo/", t) + routeExactNoMatch("/foo/", "/foo", t) + routePrefixMatch("/foo", "/foo", t) + routePrefixMatch("/foo", "/foo/", t) + //appended by matcher for prefix mode + routePrefixMatch("/foo/", "/foo", t) + //but can't match in exact mode + routeExactNoMatch("/foo/", "/foo", t) + routePrefixMatch("/foo/", "/foo/", t) + + //we match the last element of a path as substring, kube doesn't, see Kube ingress: https://kubernetes.io/docs/concepts/services-networking/ingress/ + routePrefixMatch("/aaa/bb", "/aaa/bbb", t) + routePrefixMatch("/aaa/bbb", "/aaa/bbb", t) + //ignores trailing slash + routePrefixMatch("/aaa/bbb/", "/aaa/bbb", t) + //matches trailing slash + routePrefixMatch("/aaa/bbb", "/aaa/bbb/", t) + routePrefixMatch("/aaa/bbb", "/aaa/bbb/ccc", t) + //we match the last element of a path as substring, kube doesn't, see Kube ingress: https://kubernetes.io/docs/concepts/services-networking/ingress/ + routePrefixMatch("/aaa/bbb", "/aaa/bbbxyz", t) + routePrefixMatch("/aaa/bbb", "/aaa/bbbxyz", t) +} + +func routePrefixMatch(route string, path string, t *testing.T) { + r := routeFactory(route, "prefix") req := requestFactory(path) if !r.matchURI(req) { t.Errorf("route %v did not match desired path: %v", route, path) } } -func routeNoMatch(route string, path string, t *testing.T) { - r := routeFactory(route) +func routePrefixNoMatch(route string, path string, t *testing.T) { + r := routeFactory(route, "prefix") req := requestFactory(path) if r.matchURI(req) { t.Errorf("route %v did match undesired path: %v", route, path) @@ -120,7 +149,7 @@ func routeFactory(args ...string) Route { r := Route{ Path: args[0], } - if len(args) > 1 && "exact" == args[1] { + if len(args) > 1 { r.PathType = args[1] } r.compilePath()