Simplify regex path match to reduce program size

changelogs/unreleased/4197-sunjayBhatia-small.md

@@ -0,0 +1,2 @@
+Very long (~100 characters) Ingress path prefix matches should now no longer be rejected by Envoy.
+See [this issue](https://github.com/projectcontour/contour/issues/4191) for context.

internal/envoy/v3/route.go

@@ -59,7 +59,7 @@ func RouteAuthzContext(settings map[string]string) *any.Any {
 	)
 }
 
-const prefixPathMatchSegmentRegex = `((\/).*)?`
+const prefixPathMatchSegmentRegex = `(?:[\/].*)*`
 
 var _ = regexp.MustCompile(prefixPathMatchSegmentRegex)
 
@@ -69,7 +69,9 @@ func RouteMatch(route *dag.Route) *envoy_route_v3.RouteMatch {
 	case *dag.RegexMatchCondition:
 		return &envoy_route_v3.RouteMatch{
 			PathSpecifier: &envoy_route_v3.RouteMatch_SafeRegex{
-				SafeRegex: SafeRegexMatch(c.Regex),
+				// Add an anchor since we at the very least have a / as a string literal prefix.
+				// Reduces regex program size so Envoy doesn't reject long prefix matches.
+				SafeRegex: SafeRegexMatch("^" + c.Regex),
 			},
 			Headers: headerMatcher(route.HeaderMatchConditions),
 		}
@@ -78,7 +80,9 @@ func RouteMatch(route *dag.Route) *envoy_route_v3.RouteMatch {
 		case dag.PrefixMatchSegment:
 			return &envoy_route_v3.RouteMatch{
 				PathSpecifier: &envoy_route_v3.RouteMatch_SafeRegex{
-					SafeRegex: SafeRegexMatch(regexp.QuoteMeta(c.Prefix) + prefixPathMatchSegmentRegex),
+					// Add anchor since we have a string literal prefix.
+					// Reduces regex program size so Envoy doesn't reject long prefix matches.
+					SafeRegex: SafeRegexMatch("^" + regexp.QuoteMeta(c.Prefix) + prefixPathMatchSegmentRegex),
 				},
 				Headers: headerMatcher(route.HeaderMatchConditions),
 			}

internal/envoy/v3/route_test.go

@@ -1145,7 +1145,7 @@ func TestRouteMatch(t *testing.T) {
 			},
 			want: &envoy_route_v3.RouteMatch{
 				PathSpecifier: &envoy_route_v3.RouteMatch_SafeRegex{
-					SafeRegex: SafeRegexMatch(`/foo((\/).*)?`),
+					SafeRegex: SafeRegexMatch(`^/foo(?:[\/].*)*`),
 				},
 			},
 		},
@@ -1158,7 +1158,7 @@ func TestRouteMatch(t *testing.T) {
 			},
 			want: &envoy_route_v3.RouteMatch{
 				PathSpecifier: &envoy_route_v3.RouteMatch_SafeRegex{
-					SafeRegex: SafeRegexMatch(`/foo\.bar((\/).*)?`),
+					SafeRegex: SafeRegexMatch(`^/foo\.bar(?:[\/].*)*`),
 				},
 			},
 		},
@@ -1185,7 +1185,9 @@ func TestRouteMatch(t *testing.T) {
 					// note, unlike header conditions this is not a quoted regex because
 					// the value comes directly from the Ingress.Paths.Path value which
 					// is permitted to be a bare regex.
-					SafeRegex: SafeRegexMatch("/v.1/*"),
+					// We add an anchor since we should always have a / prefix to reduce
+					// complexity.
+					SafeRegex: SafeRegexMatch("^/v.1/*"),
 				},
 			},
 		},

internal/featuretests/v3/envoy.go

@@ -125,7 +125,7 @@ func routeSegmentPrefix(prefix string) *envoy_route_v3.RouteMatch {
 				EngineType: &matcher.RegexMatcher_GoogleRe2{
 					GoogleRe2: &matcher.RegexMatcher_GoogleRE2{},
 				},
-				Regex: regexp.QuoteMeta(prefix) + `((\/).*)?`,
+				Regex: "^" + regexp.QuoteMeta(prefix) + `(?:[\/].*)*`,
 			},
 		},
 	}

internal/xdscache/v3/route_test.go

@@ -3311,7 +3311,7 @@ func routeRegex(regex string, headers ...dag.HeaderMatchCondition) *envoy_route_
 func routePrefixIngress(prefix string, headers ...dag.HeaderMatchCondition) *envoy_route_v3.RouteMatch {
 	return envoy_v3.RouteMatch(&dag.Route{
 		PathMatchCondition: &dag.RegexMatchCondition{
-			Regex: regexp.QuoteMeta(prefix) + `((\/).*)?`,
+			Regex: regexp.QuoteMeta(prefix) + `(?:[\/].*)*`,
 		},
 		HeaderMatchConditions: headers,
 	})

test/e2e/ingress/ingress_test.go

@@ -181,6 +181,8 @@ var _ = Describe("Ingress", func() {
 			testBackendTLS(namespace)
 		})
 	})
+
+	f.NamespacedTest("long-path-match", testLongPathMatch)
 })
 
 func pathTypePtr(val networkingv1.PathType) *networkingv1.PathType {

test/e2e/ingress/long_path_match_test.go

@@ -0,0 +1,113 @@
+// Copyright Project Contour Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build e2e
+// +build e2e
+
+package ingress
+
+import (
+	"context"
+	"net/http"
+	"strings"
+
+	. "github.com/onsi/ginkgo"
+	"github.com/projectcontour/contour/test/e2e"
+	"github.com/stretchr/testify/require"
+	networkingv1 "k8s.io/api/networking/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func testLongPathMatch(namespace string) {
+	Specify("long path matches should be properly programmed", func() {
+		f.Fixtures.Echo.Deploy(namespace, "echo")
+
+		// Just on the edge, should be RE2 program size 101 before regex optimizations.
+		longPrefixMatch := "/" + strings.Repeat("a", 82)
+		reallyLongPrefixMatch := "/" + strings.Repeat("b", 500)
+		longRegexMatch := "/" + strings.Repeat("c", 200) + ".*"
+
+		i := &networkingv1.Ingress{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: namespace,
+				Name:      "long-patch-match",
+			},
+			Spec: networkingv1.IngressSpec{
+				Rules: []networkingv1.IngressRule{
+					{
+						Host: "long-patch-match.ingress.projectcontour.io",
+						IngressRuleValue: networkingv1.IngressRuleValue{
+							HTTP: &networkingv1.HTTPIngressRuleValue{
+								Paths: []networkingv1.HTTPIngressPath{
+									{
+										PathType: pathTypePtr(networkingv1.PathTypePrefix),
+										Path:     longPrefixMatch,
+										Backend: networkingv1.IngressBackend{
+											Service: &networkingv1.IngressServiceBackend{
+												Name: "echo",
+												Port: networkingv1.ServiceBackendPort{
+													Number: 80,
+												},
+											},
+										},
+									},
+									{
+										PathType: pathTypePtr(networkingv1.PathTypePrefix),
+										Path:     reallyLongPrefixMatch,
+										Backend: networkingv1.IngressBackend{
+											Service: &networkingv1.IngressServiceBackend{
+												Name: "echo",
+												Port: networkingv1.ServiceBackendPort{
+													Number: 80,
+												},
+											},
+										},
+									},
+									{
+										PathType: pathTypePtr(networkingv1.PathTypeImplementationSpecific),
+										Path:     longRegexMatch,
+										Backend: networkingv1.IngressBackend{
+											Service: &networkingv1.IngressServiceBackend{
+												Name: "echo",
+												Port: networkingv1.ServiceBackendPort{
+													Number: 80,
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		}
+		require.NoError(f.T(), f.Client.Create(context.TODO(), i))
+
+		cases := []string{
+			longPrefixMatch,
+			reallyLongPrefixMatch,
+			// Cut off end .* when making request.
+			longRegexMatch[:len(longRegexMatch)-2],
+		}
+		for _, path := range cases {
+			res, ok := f.HTTP.RequestUntil(&e2e.HTTPRequestOpts{
+				Host:      i.Spec.Rules[0].Host,
+				Path:      path,
+				Condition: e2e.HasStatusCode(http.StatusOK),
+			})
+			require.NotNil(f.T(), res, "request never succeeded")
+			require.Truef(f.T(), ok, "expected %d response code, got %d", http.StatusOK, res.StatusCode)
+		}
+	})
+}