Fixup: Sort path matches based on length rather than lexi (#5752)

Since Envoy is greedy matching path routes, order is important. Contour decides to sort the routes in a way that is not really intuitive and can lead to suprises. In particular even tho the comment in the code state that routes are ordered based on legnth the reality is that they are sorted based on string comparison. This PR fixes this. * I think the current behaviour doesnt make much sense and it is a bit brittle. * Updating the behaviour has significant update risk since there might be folks that rely on this routing behaviour without really knowing it. * Should we even merge this PR? I am of two minds and I would like some input: 1. Option (1): Merge it as and make a clear changelog/announcement about the fix 2. Option (2): Create a config flag with a feature-flag e.g. `route_sorting_strategy` and switch the implementation to not do sorting when the flag is present. That way it allows folks to opt-out from the sorting as they need to. Longest path based matching kinda makes sense to me now that I know about it, but it is rough edge than needs users to be familiar with contour and it is harder to socialize in larger teams. Signed-off-by: Sotiris Nanopoulos <sotiris.nanopoulos@reddit.com>
projectcontour · Sep 28, 2023 · bbccbff · bbccbff
1 parent a9885b0
commit bbccbff
Show file tree

Hide file tree

Showing 6 changed files with 110 additions and 71 deletions.
diff --git a/changelogs/unreleased/5752-davinci26-major.md 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
@@ -478,46 +478,45 @@ 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/"),
 						Action: withPrefixRewrite(routeCluster("artifactory/service/8080/da39a3ee5e"),
 							"/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
@@ -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
@@ -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
@@ -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
@@ -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": {