xds: nack route configuration with regexes that don't compile (#4388)

Author: easwars

xds/internal/client/client.go

@@ -24,6 +24,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"regexp"
 	"sync"
 	"time"
 
@@ -271,7 +272,9 @@ type VirtualHost struct {
 // Route is both a specification of how to match a request as well as an
 // indication of the action to take upon match.
 type Route struct {
-	Path, Prefix, Regex *string
+	Path   *string
+	Prefix *string
+	Regex  *regexp.Regexp
 	// Indicates if prefix/path matching should be case insensitive. The default
 	// is false (case sensitive).
 	CaseInsensitive bool
@@ -304,20 +307,20 @@ type WeightedCluster struct {
 
 // HeaderMatcher represents header matchers.
 type HeaderMatcher struct {
-	Name         string      `json:"name"`
-	InvertMatch  *bool       `json:"invertMatch,omitempty"`
-	ExactMatch   *string     `json:"exactMatch,omitempty"`
-	RegexMatch   *string     `json:"regexMatch,omitempty"`
-	PrefixMatch  *string     `json:"prefixMatch,omitempty"`
-	SuffixMatch  *string     `json:"suffixMatch,omitempty"`
-	RangeMatch   *Int64Range `json:"rangeMatch,omitempty"`
-	PresentMatch *bool       `json:"presentMatch,omitempty"`
+	Name         string
+	InvertMatch  *bool
+	ExactMatch   *string
+	RegexMatch   *regexp.Regexp
+	PrefixMatch  *string
+	SuffixMatch  *string
+	RangeMatch   *Int64Range
+	PresentMatch *bool
 }
 
 // Int64Range is a range for header range match.
 type Int64Range struct {
-	Start int64 `json:"start"`
-	End   int64 `json:"end"`
+	Start int64
+	End   int64
 }
 
 // SecurityConfig contains the security configuration received as part of the

xds/internal/client/rds_test.go

@@ -22,24 +22,26 @@ package client
 
 import (
 	"fmt"
+	"regexp"
 	"testing"
 	"time"
 
-	v2xdspb "github.com/envoyproxy/go-control-plane/envoy/api/v2"
-	v2routepb "github.com/envoyproxy/go-control-plane/envoy/api/v2/route"
-	v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
-	v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
-	v3typepb "github.com/envoyproxy/go-control-plane/envoy/type/v3"
 	"github.com/golang/protobuf/proto"
-	anypb "github.com/golang/protobuf/ptypes/any"
-	wrapperspb "github.com/golang/protobuf/ptypes/wrappers"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
-
 	"google.golang.org/grpc/internal/xds/env"
 	"google.golang.org/grpc/xds/internal/httpfilter"
 	"google.golang.org/grpc/xds/internal/version"
 	"google.golang.org/protobuf/types/known/durationpb"
+
+	v2xdspb "github.com/envoyproxy/go-control-plane/envoy/api/v2"
+	v2routepb "github.com/envoyproxy/go-control-plane/envoy/api/v2/route"
+	v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
+	v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
+	v3matcherpb "github.com/envoyproxy/go-control-plane/envoy/type/matcher/v3"
+	v3typepb "github.com/envoyproxy/go-control-plane/envoy/type/v3"
+	anypb "github.com/golang/protobuf/ptypes/any"
+	wrapperspb "github.com/golang/protobuf/ptypes/wrappers"
 )
 
 func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) {
@@ -915,6 +917,51 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
 			}},
 			wantErr: false,
 		},
+		{
+			name: "good with regex matchers",
+			routes: []*v3routepb.Route{
+				{
+					Match: &v3routepb.RouteMatch{
+						PathSpecifier: &v3routepb.RouteMatch_SafeRegex{SafeRegex: &v3matcherpb.RegexMatcher{Regex: "/a/"}},
+						Headers: []*v3routepb.HeaderMatcher{
+							{
+								Name:                 "th",
+								HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SafeRegexMatch{SafeRegexMatch: &v3matcherpb.RegexMatcher{Regex: "tv"}},
+							},
+						},
+						RuntimeFraction: &v3corepb.RuntimeFractionalPercent{
+							DefaultValue: &v3typepb.FractionalPercent{
+								Numerator:   1,
+								Denominator: v3typepb.FractionalPercent_HUNDRED,
+							},
+						},
+					},
+					Action: &v3routepb.Route_Route{
+						Route: &v3routepb.RouteAction{
+							ClusterSpecifier: &v3routepb.RouteAction_WeightedClusters{
+								WeightedClusters: &v3routepb.WeightedCluster{
+									Clusters: []*v3routepb.WeightedCluster_ClusterWeight{
+										{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 60}},
+										{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 40}},
+									},
+									TotalWeight: &wrapperspb.UInt32Value{Value: 100},
+								}}}},
+				},
+			},
+			wantRoutes: []*Route{{
+				Regex: func() *regexp.Regexp { return regexp.MustCompile("/a/") }(),
+				Headers: []*HeaderMatcher{
+					{
+						Name:        "th",
+						InvertMatch: newBoolP(false),
+						RegexMatch:  func() *regexp.Regexp { return regexp.MustCompile("tv") }(),
+					},
+				},
+				Fraction:         newUInt32P(10000),
+				WeightedClusters: map[string]WeightedCluster{"A": {Weight: 40}, "B": {Weight: 60}},
+			}},
+			wantErr: false,
+		},
 		{
 			name: "query is ignored",
 			routes: []*v3routepb.Route{
@@ -960,6 +1007,44 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "bad regex in path specifier",
+			routes: []*v3routepb.Route{
+				{
+					Match: &v3routepb.RouteMatch{
+						PathSpecifier: &v3routepb.RouteMatch_SafeRegex{SafeRegex: &v3matcherpb.RegexMatcher{Regex: "??"}},
+						Headers: []*v3routepb.HeaderMatcher{
+							{
+								HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PrefixMatch{PrefixMatch: "tv"},
+							},
+						},
+					},
+					Action: &v3routepb.Route_Route{
+						Route: &v3routepb.RouteAction{ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName}},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "bad regex in header specifier",
+			routes: []*v3routepb.Route{
+				{
+					Match: &v3routepb.RouteMatch{
+						PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/a/"},
+						Headers: []*v3routepb.HeaderMatcher{
+							{
+								HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SafeRegexMatch{SafeRegexMatch: &v3matcherpb.RegexMatcher{Regex: "??"}},
+							},
+						},
+					},
+					Action: &v3routepb.Route_Route{
+						Route: &v3routepb.RouteAction{ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName}},
+					},
+				},
+			},
+			wantErr: true,
+		},
 		{
 			name: "unrecognized header match specifier",
 			routes: []*v3routepb.Route{
@@ -1063,7 +1148,7 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
 	}
 
 	cmpOpts := []cmp.Option{
-		cmp.AllowUnexported(Route{}, HeaderMatcher{}, Int64Range{}),
+		cmp.AllowUnexported(Route{}, HeaderMatcher{}, Int64Range{}, regexp.Regexp{}),
 		cmpopts.EquateEmpty(),
 		cmp.Transformer("FilterConfig", func(fc httpfilter.FilterConfig) string {
 			return fmt.Sprint(fc)
@@ -1074,17 +1159,15 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			oldFI := env.FaultInjectionSupport
 			env.FaultInjectionSupport = !tt.disableFI
+			defer func() { env.FaultInjectionSupport = oldFI }()
 
 			got, err := routesProtoToSlice(tt.routes, nil, false)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("routesProtoToSlice() error = %v, wantErr %v", err, tt.wantErr)
-				return
+				t.Fatalf("routesProtoToSlice() error = %v, wantErr %v", err, tt.wantErr)
 			}
-			if !cmp.Equal(got, tt.wantRoutes, cmpOpts...) {
-				t.Errorf("routesProtoToSlice() got = %v, want %v, diff: %v", got, tt.wantRoutes, cmp.Diff(got, tt.wantRoutes, cmpOpts...))
+			if diff := cmp.Diff(got, tt.wantRoutes, cmpOpts...); diff != "" {
+				t.Fatalf("routesProtoToSlice() returned unexpected diff (-got +want):\n%s", diff)
 			}
-
-			env.FaultInjectionSupport = oldFI
 		})
 	}
 }

xds/internal/client/xds.go

@@ -22,6 +22,7 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"regexp"
 	"strconv"
 	"strings"
 	"time"
@@ -437,7 +438,12 @@ func routesProtoToSlice(routes []*v3routepb.Route, logger *grpclog.PrefixLogger,
 		case *v3routepb.RouteMatch_Path:
 			route.Path = &pt.Path
 		case *v3routepb.RouteMatch_SafeRegex:
-			route.Regex = &pt.SafeRegex.Regex
+			regex := pt.SafeRegex.GetRegex()
+			re, err := regexp.Compile(regex)
+			if err != nil {
+				return nil, fmt.Errorf("route %+v contains an invalid regex %q", r, regex)
+			}
+			route.Regex = re
 		default:
 			return nil, fmt.Errorf("route %+v has an unrecognized path specifier: %+v", r, pt)
 		}
@@ -452,7 +458,12 @@ func routesProtoToSlice(routes []*v3routepb.Route, logger *grpclog.PrefixLogger,
 			case *v3routepb.HeaderMatcher_ExactMatch:
 				header.ExactMatch = &ht.ExactMatch
 			case *v3routepb.HeaderMatcher_SafeRegexMatch:
-				header.RegexMatch = &ht.SafeRegexMatch.Regex
+				regex := ht.SafeRegexMatch.GetRegex()
+				re, err := regexp.Compile(regex)
+				if err != nil {
+					return nil, fmt.Errorf("route %+v contains an invalid regex %q", r, regex)
+				}
+				header.RegexMatch = re
 			case *v3routepb.HeaderMatcher_RangeMatch:
 				header.RangeMatch = &Int64Range{
 					Start: ht.RangeMatch.Start,

xds/internal/resolver/matcher.go

@@ -20,7 +20,6 @@ package resolver
 
 import (
 	"fmt"
-	"regexp"
 	"strings"
 
 	"google.golang.org/grpc/internal/grpcrand"
@@ -34,11 +33,7 @@ func routeToMatcher(r *xdsclient.Route) (*compositeMatcher, error) {
 	var pathMatcher pathMatcherInterface
 	switch {
 	case r.Regex != nil:
-		re, err := regexp.Compile(*r.Regex)
-		if err != nil {
-			return nil, fmt.Errorf("failed to compile regex %q", *r.Regex)
-		}
-		pathMatcher = newPathRegexMatcher(re)
+		pathMatcher = newPathRegexMatcher(r.Regex)
 	case r.Path != nil:
 		pathMatcher = newPathExactMatcher(*r.Path, r.CaseInsensitive)
 	case r.Prefix != nil:
@@ -53,12 +48,8 @@ func routeToMatcher(r *xdsclient.Route) (*compositeMatcher, error) {
 		switch {
 		case h.ExactMatch != nil && *h.ExactMatch != "":
 			matcherT = newHeaderExactMatcher(h.Name, *h.ExactMatch)
-		case h.RegexMatch != nil && *h.RegexMatch != "":
-			re, err := regexp.Compile(*h.RegexMatch)
-			if err != nil {
-				return nil, fmt.Errorf("failed to compile regex %q, skipping this matcher", *h.RegexMatch)
-			}
-			matcherT = newHeaderRegexMatcher(h.Name, re)
+		case h.RegexMatch != nil:
+			matcherT = newHeaderRegexMatcher(h.Name, h.RegexMatch)
 		case h.PrefixMatch != nil && *h.PrefixMatch != "":
 			matcherT = newHeaderPrefixMatcher(h.Name, *h.PrefixMatch)
 		case h.SuffixMatch != nil && *h.SuffixMatch != "":