Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

internal: Remove max_program_size from regex #3093

Merged
merged 1 commit into from Nov 4, 2020
Merged

internal: Remove max_program_size from regex #3093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 29 additions & 0 deletions internal/dag/builder_test.go
View file
Open in desktop
Expand Up @@ -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",
Expand Down Expand Up @@ -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{}{
Expand Down
7 changes: 7 additions & 0 deletions internal/dag/conditions.go
View file
Open in desktop
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Best to add a comment that this is specifically validating RE2 regex syntax because that I what we are programming into Envoy (though Go isn't identicall apparently?).

_, err := regexp.Compile(regex)
return err
}
22 changes: 17 additions & 5 deletions internal/dag/ingress_processor.go
View file
Open in desktop
Expand Up @@ -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) {
Expand All @@ -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,
Expand All @@ -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
Expand Down
18 changes: 2 additions & 16 deletions internal/envoy/v2/regex.go
View file
Open in desktop
Expand Up @@ -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,
}
Expand Down
12 changes: 3 additions & 9 deletions internal/envoy/v2/regex_test.go
View file
Open in desktop
Expand Up @@ -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{},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole file could be deleted probably, these tests don't really test anything.

},
},
},
"simple": {
regex: "chrome",
want: &matcher.RegexMatcher{
EngineType: &matcher.RegexMatcher_GoogleRe2{
GoogleRe2: &matcher.RegexMatcher_GoogleRE2{
MaxProgramSize: protobuf.UInt32(maxRegexProgramSize),
},
GoogleRe2: &matcher.RegexMatcher_GoogleRE2{},
},
Regex: "chrome",
},
Expand All @@ -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.
},
Expand Down
41 changes: 41 additions & 0 deletions internal/featuretests/v2/route_test.go
View file
Open in desktop
Expand Up @@ -1509,6 +1509,47 @@ func TestRoutePrefixRouteRegex(t *testing.T) {
TypeUrl: routeType,
Nonce: "1",
})

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) {
Expand Down