internal: Remove max_program_size from regex

The Regex `max_program_size` is deprecated from Envoy and the control plane should do this validation now. Removes the max_program_size field and adds a regex compile check to the Ingress resource builder. Fixes #2933 Signed-off-by: Steve Sloka <slokas@vmware.com>
projectcontour · Nov 4, 2020 · fbd27f0 · fbd27f0
1 parent 7ecc56a
commit fbd27f0
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 30 deletions.
diff --git a/internal/dag/builder_test.go b/internal/dag/builder_test.go
@@ -780,6 +780,28 @@ func TestDAGInsert(t *testing.T) {
 		},
 	}
 
+	i15InvalidRegex := &v1beta1.Ingress{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "regex",
+			Namespace: "default",
+		},
+		Spec: v1beta1.IngressSpec{
+			Rules: []v1beta1.IngressRule{{
+				IngressRuleValue: v1beta1.IngressRuleValue{
+					HTTP: &v1beta1.HTTPIngressRuleValue{
+						Paths: []v1beta1.HTTPIngressPath{{
+							Path: "^\\/(?!\\/)(.*?)",
+							Backend: v1beta1.IngressBackend{
+								ServiceName: "kuard",
+								ServicePort: intstr.FromString("http"),
+							},
+						}},
+					},
+				},
+			}},
+		},
+	}
+
 	i16 := &v1beta1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "wildcards",
@@ -4023,6 +4045,13 @@ func TestDAGInsert(t *testing.T) {
 				},
 			),
 		},
+		"insert ingress with invalid regex route": {
+			objs: []interface{}{
+				i15InvalidRegex,
+				s1,
+			},
+			want: listeners(),
+		},
 		// issue 1234
 		"insert ingress with wildcard hostnames": {
 			objs: []interface{}{

diff --git a/internal/dag/conditions.go b/internal/dag/conditions.go
@@ -179,3 +179,10 @@ func headerMatchConditionsValid(conditions []contour_api_v1.MatchCondition) erro
 
 	return nil
 }
+
+// ValidateRegex returns an error if the supplied
+// RE2 regex syntax is invalid.
+func ValidateRegex(regex string) error {
+	_, err := regexp.Compile(regex)
+	return err
+}
diff --git a/internal/dag/ingress_processor.go b/internal/dag/ingress_processor.go
@@ -140,7 +140,15 @@ func (p *IngressProcessor) computeIngressRule(ing *v1beta1.Ingress, rule v1beta1
 			continue
 		}
 
-		r := route(ing, path, s, clientCertSecret, p.FieldLogger)
+		r, err := route(ing, path, s, clientCertSecret, p.FieldLogger)
+		if err != nil {
+			p.WithError(err).
+				WithField("name", ing.GetName()).
+				WithField("namespace", ing.GetNamespace()).
+				WithField("regex", path).
+				Errorf("path regex is not valid")
+			return
+		}
 
 		// should we create port 80 routes for this ingress
 		if annotation.TLSRequired(ing) || annotation.HTTPAllowed(ing) {
@@ -158,7 +166,7 @@ func (p *IngressProcessor) computeIngressRule(ing *v1beta1.Ingress, rule v1beta1
 }
 
 // route builds a dag.Route for the supplied Ingress.
-func route(ingress *v1beta1.Ingress, path string, service *Service, clientCertSecret *Secret, log logrus.FieldLogger) *Route {
+func route(ingress *v1beta1.Ingress, path string, service *Service, clientCertSecret *Secret, log logrus.FieldLogger) (*Route, error) {
 	log = log.WithFields(logrus.Fields{
 		"name":      ingress.Name,
 		"namespace": ingress.Namespace,
@@ -177,13 +185,17 @@ func route(ingress *v1beta1.Ingress, path string, service *Service, clientCertSe
 	}
 
 	if strings.ContainsAny(path, "^+*[]%") {
-		// path smells like a regex
+		// validate the regex
+		if err := ValidateRegex(path); err != nil {
+			return nil, err
+		}
+
 		r.PathMatchCondition = &RegexMatchCondition{Regex: path}
-		return r
+		return r, nil
 	}
 
 	r.PathMatchCondition = &PrefixMatchCondition{Prefix: path}
-	return r
+	return r, nil
 }
 
 // rulesFromSpec merges the IngressSpec's Rules with a synthetic

diff --git a/internal/envoy/v2/regex.go b/internal/envoy/v2/regex.go
@@ -15,28 +15,14 @@ package v2
 
 import (
 	matcher "github.com/envoyproxy/go-control-plane/envoy/type/matcher"
-	"github.com/projectcontour/contour/internal/protobuf"
 )
 
-// maxRegexProgramSize is the default value for the Envoy regex max
-// program size tunable. There's no way to really know what a good
-// value for this is, except that the RE2 maintainer thinks that 100
-// is low. As a rule of thumb, each '.*' expression costs about 15
-// units of program size. AFAIK, there's no obvious correlation
-// between regex size and execution time.
-//
-// See also https://github.com/envoyproxy/envoy/pull/9171#discussion_r351974033
-// and https://github.com/projectcontour/contour/issues/2240
-const maxRegexProgramSize = 1 << 20
-
-// SafeRegexMatch retruns a matcher.RegexMatcher for the supplied regex.
+// SafeRegexMatch returns a matcher.RegexMatcher for the supplied regex.
 // SafeRegexMatch does not escape regex meta characters.
 func SafeRegexMatch(regex string) *matcher.RegexMatcher {
 	return &matcher.RegexMatcher{
 		EngineType: &matcher.RegexMatcher_GoogleRe2{
-			GoogleRe2: &matcher.RegexMatcher_GoogleRE2{
-				MaxProgramSize: protobuf.UInt32(maxRegexProgramSize),
-			},
+			GoogleRe2: &matcher.RegexMatcher_GoogleRE2{},
 		},
 		Regex: regex,
 	}

diff --git a/internal/envoy/v2/regex_test.go b/internal/envoy/v2/regex_test.go
@@ -29,19 +29,15 @@ func TestSafeRegexMatch(t *testing.T) {
 			regex: "",
 			want: &matcher.RegexMatcher{
 				EngineType: &matcher.RegexMatcher_GoogleRe2{
-					GoogleRe2: &matcher.RegexMatcher_GoogleRE2{
-						MaxProgramSize: protobuf.UInt32(maxRegexProgramSize),
-					},
+					GoogleRe2: &matcher.RegexMatcher_GoogleRE2{},
 				},
 			},
 		},
 		"simple": {
 			regex: "chrome",
 			want: &matcher.RegexMatcher{
 				EngineType: &matcher.RegexMatcher_GoogleRe2{
-					GoogleRe2: &matcher.RegexMatcher_GoogleRE2{
-						MaxProgramSize: protobuf.UInt32(maxRegexProgramSize),
-					},
+					GoogleRe2: &matcher.RegexMatcher_GoogleRE2{},
 				},
 				Regex: "chrome",
 			},
@@ -50,9 +46,7 @@ func TestSafeRegexMatch(t *testing.T) {
 			regex: "[a-z]+$",
 			want: &matcher.RegexMatcher{
 				EngineType: &matcher.RegexMatcher_GoogleRe2{
-					GoogleRe2: &matcher.RegexMatcher_GoogleRE2{
-						MaxProgramSize: protobuf.UInt32(maxRegexProgramSize),
-					},
+					GoogleRe2: &matcher.RegexMatcher_GoogleRE2{},
 				},
 				Regex: "[a-z]+$", // meta characters are not escaped.
 			},

diff --git a/internal/featuretests/v2/route_test.go b/internal/featuretests/v2/route_test.go
@@ -1509,6 +1509,48 @@ func TestRoutePrefixRouteRegex(t *testing.T) {
 		TypeUrl: routeType,
 		Nonce:   "1",
 	})
+
+	// add default/kuard to translator.
+	invalid := &v1beta1.Ingress{
+		ObjectMeta: meta,
+		Spec: v1beta1.IngressSpec{
+			Backend: &v1beta1.IngressBackend{
+				ServiceName: "kuard",
+				ServicePort: intstr.FromInt(80),
+			},
+			Rules: []v1beta1.IngressRule{{
+				IngressRuleValue: v1beta1.IngressRuleValue{
+					HTTP: &v1beta1.HTTPIngressRuleValue{
+						Paths: []v1beta1.HTTPIngressPath{{
+							Path: "^\\/(?!\\/)(.*?)",
+							Backend: v1beta1.IngressBackend{
+								ServiceName: "kuard",
+								ServicePort: intstr.FromInt(80),
+							},
+						}},
+					},
+				},
+			}},
+		},
+	}
+	rh.OnAdd(invalid)
+
+	// check that it's been translated correctly.
+	c.Request(routeType).Equals(&envoy_api_v2.DiscoveryResponse{
+		VersionInfo: "1",
+		Resources: routeResources(t,
+			envoy_v2.RouteConfiguration("ingress_http",
+				envoy_v2.VirtualHost("*",
+					&envoy_api_v2_route.Route{
+						Match:  routePrefix("/"),
+						Action: routecluster("default/kuard/80/da39a3ee5e"),
+					},
+				),
+			),
+		),
+		TypeUrl: routeType,
+		Nonce:   "1",
+	})
 }
 
 func assertRDS(t *testing.T, c *Contour, versioninfo string, ingressHTTP, ingressHTTPS []*envoy_api_v2_route.VirtualHost) {