Fixup: Sort paths based on length rather than lexi

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.
+

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"),
 					},
 				),
 			),

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"),
+					},
 				),
 			),
 		),

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

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"),

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": {