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

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

Merged
merged 3 commits into from May 7, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af77ac1
xds: nack route configuration with regexes that don't compile
easwars May 5, 2021
ebdd243
store compiled regex in internal data structure
easwars May 6, 2021
1fcf3db
add test, remove json tags
easwars May 7, 2021
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
25 changes: 14 additions & 11 deletions xds/internal/client/client.go
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@ import (
"context"
"errors"
"fmt"
"regexp"
"sync"
"time"

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
113 changes: 98 additions & 15 deletions xds/internal/client/rds_test.go
View file
Open in desktop
Expand Up @@ -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) {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
Expand All @@ -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
})
}
}
Expand Down
15 changes: 13 additions & 2 deletions xds/internal/client/xds.go
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@ import (
"errors"
"fmt"
"net"
"regexp"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -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)
}
Expand All @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking we can be a bit more aggressive here.

Options:

  1. (probably not a good idea): we turn the match fields into a matcher, and put that in the RDS update struct, so the resolver can use directly.
    • this is bad because the log probably won't be as easy to read, and also it may be hard to compare RDS updates in the tests
  2. (better and easier than 1?): change the type of header.RegexMatch to regexp.Regexp
    • this saves the regexp.Compile() in resolver (and the error handling)
    • this has the similar problem for tests. But I think we can just compare the result of String()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm surprised that we don't need to fix tests for this (the newer commit). Maybe we don't have Regex success tests, and the two Regex failure tests aren't affected. Can you double check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test for the regex success case.

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,
Expand Down
15 changes: 3 additions & 12 deletions xds/internal/resolver/matcher.go
View file
Open in desktop
Expand Up @@ -20,7 +20,6 @@ package resolver

import (
"fmt"
"regexp"
"strings"

"google.golang.org/grpc/internal/grpcrand"
Expand All @@ -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:
Expand All @@ -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 != "":
Expand Down