diff --git a/changelogs/unreleased/5752-davinci26-major.md b/changelogs/unreleased/5752-davinci26-major.md new file mode 100644 index 00000000000..de420dff378 --- /dev/null +++ b/changelogs/unreleased/5752-davinci26-major.md @@ -0,0 +1,15 @@ +## Fix bug with algorithm used to sort Envoy regex/prefix path rules + +Envoy greedy matches routes and as a result the order route matches are presented to Envoy is important. Contour attempts to produce consistent routing tables so that the most specific route matches are given preference. This is done to facilitate consistency when using HTTPProxy inclusion and provide a uniform user experience for route matching to be inline with Ingress and Gateway API Conformance. + +This changes fixes the sorting algorithm used for `Prefix` and `Regex` based path matching. Previously the algorithm lexicographically sorted based on the path match string instead of sorting them based on the length of the `Prefix`|`Regex`. i.e. Longer prefix/regexes will be sorted first in order to give preference to more specific routes, then lexicographic sorting for things of the same length. + +Note that for prefix matching, this change is _not_ expected to change the relative ordering of more specific prefixes vs. less specific ones when the more specific prefix match string has the less specific one as a prefix, e.g. `/foo/bar` will continue to sort before `/foo`. However, relative ordering of other combinations of prefix matches may change per the above description. +### How to update safely + +Caution is advised if you update Contour and you are operating large routing tables. We advise you to: + +1. Deploy a duplicate Contour installation that parses the same CRDs +2. Port-forward to the Envoy admin interface [docs](https://projectcontour.io/docs/v1.3.0/troubleshooting/) +3. Access `http://127.0.0.1:9001/config_dump` and compare the configuration of Envoy. In particular the routes and their order. The prefix routes might be changing in order, so if they are you need to verify that the route matches as expected. + diff --git a/internal/featuretests/v3/replaceprefix_test.go b/internal/featuretests/v3/replaceprefix_test.go index bc46d400799..5b6989ad44b 100644 --- a/internal/featuretests/v3/replaceprefix_test.go +++ b/internal/featuretests/v3/replaceprefix_test.go @@ -478,16 +478,15 @@ func artifactoryDocker(t *testing.T) { Resources: resources(t, envoy_v3.RouteConfiguration("ingress_http", envoy_v3.VirtualHost("artifactory.projectcontour.io", - &envoy_route_v3.Route{ - Match: routePrefix("/v2/container-sandbox/"), + Match: routePrefix("/v2/container-external/"), Action: withPrefixRewrite(routeCluster("artifactory/service/8080/da39a3ee5e"), - "/artifactory/api/docker/container-sandbox/v2/"), + "/artifactory/api/docker/container-external/v2/"), }, &envoy_route_v3.Route{ - Match: routePrefix("/v2/container-sandbox"), + Match: routePrefix("/v2/container-sandbox/"), Action: withPrefixRewrite(routeCluster("artifactory/service/8080/da39a3ee5e"), - "/artifactory/api/docker/container-sandbox/v2"), + "/artifactory/api/docker/container-sandbox/v2/"), }, &envoy_route_v3.Route{ Match: routePrefix("/v2/container-release/"), @@ -495,29 +494,29 @@ func artifactoryDocker(t *testing.T) { "/artifactory/api/docker/container-release/v2/"), }, &envoy_route_v3.Route{ - Match: routePrefix("/v2/container-release"), + Match: routePrefix("/v2/container-external"), Action: withPrefixRewrite(routeCluster("artifactory/service/8080/da39a3ee5e"), - "/artifactory/api/docker/container-release/v2"), + "/artifactory/api/docker/container-external/v2"), }, &envoy_route_v3.Route{ - Match: routePrefix("/v2/container-public/"), + Match: routePrefix("/v2/container-sandbox"), Action: withPrefixRewrite(routeCluster("artifactory/service/8080/da39a3ee5e"), - "/artifactory/api/docker/container-public/v2/"), + "/artifactory/api/docker/container-sandbox/v2"), }, &envoy_route_v3.Route{ - Match: routePrefix("/v2/container-public"), + Match: routePrefix("/v2/container-release"), Action: withPrefixRewrite(routeCluster("artifactory/service/8080/da39a3ee5e"), - "/artifactory/api/docker/container-public/v2"), + "/artifactory/api/docker/container-release/v2"), }, &envoy_route_v3.Route{ - Match: routePrefix("/v2/container-external/"), + Match: routePrefix("/v2/container-public/"), Action: withPrefixRewrite(routeCluster("artifactory/service/8080/da39a3ee5e"), - "/artifactory/api/docker/container-external/v2/"), + "/artifactory/api/docker/container-public/v2/"), }, &envoy_route_v3.Route{ - Match: routePrefix("/v2/container-external"), + Match: routePrefix("/v2/container-public"), Action: withPrefixRewrite(routeCluster("artifactory/service/8080/da39a3ee5e"), - "/artifactory/api/docker/container-external/v2"), + "/artifactory/api/docker/container-public/v2"), }, ), ), diff --git a/internal/featuretests/v3/route_test.go b/internal/featuretests/v3/route_test.go index b17f0902929..1e92c0f1d71 100644 --- a/internal/featuretests/v3/route_test.go +++ b/internal/featuretests/v3/route_test.go @@ -1237,26 +1237,26 @@ func TestRouteWithTLS_InsecurePaths(t *testing.T) { Resources: routeResources(t, envoy_v3.RouteConfiguration("ingress_http", envoy_v3.VirtualHost("test2.test.com", - &envoy_route_v3.Route{ - Match: routePrefix("/secure"), - Action: envoy_v3.UpgradeHTTPS(), - }, &envoy_route_v3.Route{ Match: routePrefix("/insecure"), Action: routecluster("default/kuard/80/da39a3ee5e"), }, + &envoy_route_v3.Route{ + Match: routePrefix("/secure"), + Action: envoy_v3.UpgradeHTTPS(), + }, ), ), envoy_v3.RouteConfiguration("https/test2.test.com", envoy_v3.VirtualHost("test2.test.com", - &envoy_route_v3.Route{ - Match: routePrefix("/secure"), - Action: routecluster("default/svc2/80/da39a3ee5e"), - }, &envoy_route_v3.Route{ Match: routePrefix("/insecure"), Action: routecluster("default/kuard/80/da39a3ee5e"), }, + &envoy_route_v3.Route{ + Match: routePrefix("/secure"), + Action: routecluster("default/svc2/80/da39a3ee5e"), + }, ), ), ), @@ -1335,25 +1335,25 @@ func TestRouteWithTLS_InsecurePaths_DisablePermitInsecureTrue(t *testing.T) { envoy_v3.RouteConfiguration("ingress_http", envoy_v3.VirtualHost("test2.test.com", &envoy_route_v3.Route{ - Match: routePrefix("/secure"), + Match: routePrefix("/insecure"), Action: envoy_v3.UpgradeHTTPS(), }, &envoy_route_v3.Route{ - Match: routePrefix("/insecure"), + Match: routePrefix("/secure"), Action: envoy_v3.UpgradeHTTPS(), }, ), ), envoy_v3.RouteConfiguration("https/test2.test.com", envoy_v3.VirtualHost("test2.test.com", - &envoy_route_v3.Route{ - Match: routePrefix("/secure"), - Action: routecluster("default/svc2/80/da39a3ee5e"), - }, &envoy_route_v3.Route{ Match: routePrefix("/insecure"), Action: routecluster("default/kuard/80/da39a3ee5e"), }, + &envoy_route_v3.Route{ + Match: routePrefix("/secure"), + Action: routecluster("default/svc2/80/da39a3ee5e"), + }, ), ), ), @@ -1609,26 +1609,26 @@ func TestHTTPProxyRouteWithTLS_InsecurePaths(t *testing.T) { Resources: routeResources(t, envoy_v3.RouteConfiguration("ingress_http", envoy_v3.VirtualHost("test2.test.com", - &envoy_route_v3.Route{ - Match: routePrefix("/secure"), - Action: envoy_v3.UpgradeHTTPS(), - }, &envoy_route_v3.Route{ Match: routePrefix("/insecure"), Action: routecluster("default/kuard/80/da39a3ee5e"), }, + &envoy_route_v3.Route{ + Match: routePrefix("/secure"), + Action: envoy_v3.UpgradeHTTPS(), + }, ), ), envoy_v3.RouteConfiguration("https/test2.test.com", envoy_v3.VirtualHost("test2.test.com", - &envoy_route_v3.Route{ - Match: routePrefix("/secure"), - Action: routecluster("default/svc2/80/da39a3ee5e"), - }, &envoy_route_v3.Route{ Match: routePrefix("/insecure"), Action: routecluster("default/kuard/80/da39a3ee5e"), }, + &envoy_route_v3.Route{ + Match: routePrefix("/secure"), + Action: routecluster("default/svc2/80/da39a3ee5e"), + }, ), ), ), @@ -1703,25 +1703,25 @@ func TestHTTPProxyRouteWithTLS_InsecurePaths_DisablePermitInsecureTrue(t *testin envoy_v3.RouteConfiguration("ingress_http", envoy_v3.VirtualHost("test2.test.com", &envoy_route_v3.Route{ - Match: routePrefix("/secure"), + Match: routePrefix("/insecure"), Action: envoy_v3.UpgradeHTTPS(), }, &envoy_route_v3.Route{ - Match: routePrefix("/insecure"), + Match: routePrefix("/secure"), Action: envoy_v3.UpgradeHTTPS(), }, ), ), envoy_v3.RouteConfiguration("https/test2.test.com", envoy_v3.VirtualHost("test2.test.com", - &envoy_route_v3.Route{ - Match: routePrefix("/secure"), - Action: routecluster("default/svc2/80/da39a3ee5e"), - }, &envoy_route_v3.Route{ Match: routePrefix("/insecure"), Action: routecluster("default/kuard/80/da39a3ee5e"), }, + &envoy_route_v3.Route{ + Match: routePrefix("/secure"), + Action: routecluster("default/svc2/80/da39a3ee5e"), + }, ), ), ), diff --git a/internal/sorter/sorter.go b/internal/sorter/sorter.go index 5334b2bea3c..133273c5cd2 100644 --- a/internal/sorter/sorter.go +++ b/internal/sorter/sorter.go @@ -296,33 +296,47 @@ func (s routeSorter) Less(i, j int) bool { switch a := s[i].PathMatchCondition.(type) { case *dag.PrefixMatchCondition: if b, ok := s[j].PathMatchCondition.(*dag.PrefixMatchCondition); ok { - cmp := strings.Compare(a.Prefix, b.Prefix) - switch cmp { - case 1: + switch { + case len(a.Prefix) > len(b.Prefix): // Sort longest prefix first. return true - case -1: + case len(a.Prefix) < len(b.Prefix): return false default: - if a.PrefixMatchType == b.PrefixMatchType { - return compareRoutesByMethodHeaderQueryParams(s[i], s[j]) + cmp := strings.Compare(a.Prefix, b.Prefix) + switch cmp { + case 1: + return true + case -1: + return false + default: + if a.PrefixMatchType == b.PrefixMatchType { + return compareRoutesByMethodHeaderQueryParams(s[i], s[j]) + } + // Segment prefixes sort first as they are more specific. + return a.PrefixMatchType == dag.PrefixMatchSegment } - // Segment prefixes sort first as they are more specific. - return a.PrefixMatchType == dag.PrefixMatchSegment } } case *dag.RegexMatchCondition: switch b := s[j].PathMatchCondition.(type) { case *dag.RegexMatchCondition: - cmp := strings.Compare(a.Regex, b.Regex) - switch cmp { - case 1: + switch { + case len(a.Regex) > len(b.Regex): // Sort longest regex first. return true - case -1: + case len(a.Regex) < len(b.Regex): return false default: - return compareRoutesByMethodHeaderQueryParams(s[i], s[j]) + cmp := strings.Compare(a.Regex, b.Regex) + switch cmp { + case 1: + return true + case -1: + return false + default: + return compareRoutesByMethodHeaderQueryParams(s[i], s[j]) + } } case *dag.PrefixMatchCondition: return true @@ -331,9 +345,11 @@ func (s routeSorter) Less(i, j int) bool { switch b := s[j].PathMatchCondition.(type) { case *dag.ExactMatchCondition: cmp := strings.Compare(a.Path, b.Path) + // Sorting function doesn't really matter here + // since we want exact matching. Lexicographic sorting + // is ok switch cmp { case 1: - // Sort longest path first. return true case -1: return false diff --git a/internal/sorter/sorter_test.go b/internal/sorter/sorter_test.go index f42c3581179..b28e61afa8d 100644 --- a/internal/sorter/sorter_test.go +++ b/internal/sorter/sorter_test.go @@ -279,32 +279,35 @@ func TestSortRoutesPathMatch(t *testing.T) { }, // Note that regex matches sort before prefix matches. { - PathMatchCondition: matchRegex("/this/is/the/longest"), + PathMatchCondition: matchRegex("/athis/is/the/longest"), }, { PathMatchCondition: matchRegex(`/foo((\/).*)*`), }, { - PathMatchCondition: matchRegex("/"), + PathMatchCondition: matchRegex("/foo.*"), + }, + { + PathMatchCondition: matchRegex("/bar.*"), }, { - PathMatchCondition: matchRegex("."), + PathMatchCondition: matchRegex("/"), }, // Prefix segment matches sort before string matches. { - PathMatchCondition: matchPrefixSegment("/path/prefix2"), + PathMatchCondition: matchPrefixSegment("/path/prefix/a"), }, { - PathMatchCondition: matchPrefixString("/path/prefix2"), + PathMatchCondition: matchPrefixString("/path/prefix/a"), }, { - PathMatchCondition: matchPrefixSegment("/path/prefix/a"), + PathMatchCondition: matchPrefixString("/path/prf222"), }, { - PathMatchCondition: matchPrefixString("/path/prefix/a"), + PathMatchCondition: matchPrefixString("/path/prf122"), }, { - PathMatchCondition: matchPrefixString("/path/prefix"), + PathMatchCondition: matchPrefixString("/path/prfx"), }, { PathMatchCondition: matchPrefixSegment("/path/p"), @@ -389,25 +392,31 @@ func TestSortRoutesLongestHeaders(t *testing.T) { PathMatchCondition: matchExact("/pathexact"), }, { - PathMatchCondition: matchRegex("/pathregex"), + PathMatchCondition: matchRegex("/pathregex2"), + HeaderMatchConditions: []dag.HeaderMatchCondition{ + presentHeader("header-name"), + }, + }, + { + PathMatchCondition: matchRegex("/pathregex1"), HeaderMatchConditions: []dag.HeaderMatchCondition{ exactHeader("header-name", "header-value"), }, }, { - PathMatchCondition: matchRegex("/pathregex"), + PathMatchCondition: matchRegex("/pathregex1"), HeaderMatchConditions: []dag.HeaderMatchCondition{ presentHeader("header-name"), }, }, { - PathMatchCondition: matchRegex("/pathregex"), + PathMatchCondition: matchRegex("/pathregex1"), HeaderMatchConditions: []dag.HeaderMatchCondition{ exactHeader("long-header-name", "long-header-value"), }, }, { - PathMatchCondition: matchRegex("/pathregex"), + PathMatchCondition: matchRegex("/pathregex1"), }, { PathMatchCondition: matchPrefixSegment("/path"), diff --git a/internal/xdscache/v3/route_test.go b/internal/xdscache/v3/route_test.go index 9decc42e5ef..ff167ff4ee8 100644 --- a/internal/xdscache/v3/route_test.go +++ b/internal/xdscache/v3/route_test.go @@ -3722,9 +3722,9 @@ func TestSortLongestRouteFirst(t *testing.T) { PathMatchCondition: &dag.RegexMatchCondition{Regex: "/v1/.+"}, }}, want: []*dag.Route{{ - PathMatchCondition: &dag.RegexMatchCondition{Regex: "/v2"}, - }, { PathMatchCondition: &dag.RegexMatchCondition{Regex: "/v1/.+"}, + }, { + PathMatchCondition: &dag.RegexMatchCondition{Regex: "/v2"}, }}, }, "two exact matches": {