Skip to content

Commit

Permalink
rds: allow case_insensitive path matching (grpc#3997)
Browse files Browse the repository at this point in the history 
- in xds_client, accept (not NACK) RDS resp with case_insensitive=true
- pass case_insensitive to xds resolver and routing balancer
  - Note that after the config selector change, the routing balancer will be removed, and
    this will be handled in the resolver config selector
  • Loading branch information
@menghanl @davidkhala
menghanl authored and davidkhala committed Dec 7, 2020
1 parent 1bdbbbb commit d4e2d0c
Show file tree
Hide file tree
Showing 11 changed files with 136 additions and 61 deletions.
40 changes: 32 additions & 8 deletions xds/internal/balancer/xdsrouting/matcher_path.go
View file
Expand Up @@ -30,14 +30,26 @@ type pathMatcherInterface interface {
}

type pathExactMatcher struct {
fullPath string
// fullPath is all upper case if caseInsensitive is true.
fullPath string
caseInsensitive bool
}

func newPathExactMatcher(p string) *pathExactMatcher {
return &pathExactMatcher{fullPath: p}
func newPathExactMatcher(p string, caseInsensitive bool) *pathExactMatcher {
ret := &pathExactMatcher{
fullPath: p,
caseInsensitive: caseInsensitive,
}
if caseInsensitive {
ret.fullPath = strings.ToUpper(p)
}
return ret
}

func (pem *pathExactMatcher) match(path string) bool {
if pem.caseInsensitive {
return pem.fullPath == strings.ToUpper(path)
}
return pem.fullPath == path
}

Expand All @@ -46,22 +58,34 @@ func (pem *pathExactMatcher) equal(m pathMatcherInterface) bool {
if !ok {
return false
}
return pem.fullPath == mm.fullPath
return pem.fullPath == mm.fullPath && pem.caseInsensitive == mm.caseInsensitive
}

func (pem *pathExactMatcher) String() string {
return "pathExact:" + pem.fullPath
}

type pathPrefixMatcher struct {
prefix string
// prefix is all upper case if caseInsensitive is true.
prefix string
caseInsensitive bool
}

func newPathPrefixMatcher(p string) *pathPrefixMatcher {
return &pathPrefixMatcher{prefix: p}
func newPathPrefixMatcher(p string, caseInsensitive bool) *pathPrefixMatcher {
ret := &pathPrefixMatcher{
prefix: p,
caseInsensitive: caseInsensitive,
}
if caseInsensitive {
ret.prefix = strings.ToUpper(p)
}
return ret
}

func (ppm *pathPrefixMatcher) match(path string) bool {
if ppm.caseInsensitive {
return strings.HasPrefix(strings.ToUpper(path), ppm.prefix)
}
return strings.HasPrefix(path, ppm.prefix)
}

Expand All @@ -70,7 +94,7 @@ func (ppm *pathPrefixMatcher) equal(m pathMatcherInterface) bool {
if !ok {
return false
}
return ppm.prefix == mm.prefix
return ppm.prefix == mm.prefix && ppm.caseInsensitive == mm.caseInsensitive
}

func (ppm *pathPrefixMatcher) String() string {
Expand Down
28 changes: 18 additions & 10 deletions xds/internal/balancer/xdsrouting/matcher_path_test.go
View file
Expand Up @@ -25,17 +25,21 @@ import (

func TestPathFullMatcherMatch(t *testing.T) {
tests := []struct {
name string
fullPath string
path string
want bool
name string
fullPath string
caseInsensitive bool
path string
want bool
}{
{name: "match", fullPath: "/s/m", path: "/s/m", want: true},
{name: "case insensitive match", fullPath: "/s/m", caseInsensitive: true, path: "/S/m", want: true},
{name: "case insensitive match 2", fullPath: "/s/M", caseInsensitive: true, path: "/S/m", want: true},
{name: "not match", fullPath: "/s/m", path: "/a/b", want: false},
{name: "case insensitive not match", fullPath: "/s/m", caseInsensitive: true, path: "/a/b", want: false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
fpm := newPathExactMatcher(tt.fullPath)
fpm := newPathExactMatcher(tt.fullPath, tt.caseInsensitive)
if got := fpm.match(tt.path); got != tt.want {
t.Errorf("{%q}.match(%q) = %v, want %v", tt.fullPath, tt.path, got, tt.want)
}
Expand All @@ -45,17 +49,21 @@ func TestPathFullMatcherMatch(t *testing.T) {

func TestPathPrefixMatcherMatch(t *testing.T) {
tests := []struct {
name string
prefix string
path string
want bool
name string
prefix string
caseInsensitive bool
path string
want bool
}{
{name: "match", prefix: "/s/", path: "/s/m", want: true},
{name: "case insensitive match", prefix: "/s/", caseInsensitive: true, path: "/S/m", want: true},
{name: "case insensitive match 2", prefix: "/S/", caseInsensitive: true, path: "/s/m", want: true},
{name: "not match", prefix: "/s/", path: "/a/b", want: false},
{name: "case insensitive not match", prefix: "/s/", caseInsensitive: true, path: "/a/b", want: false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
fpm := newPathPrefixMatcher(tt.prefix)
fpm := newPathPrefixMatcher(tt.prefix, tt.caseInsensitive)
if got := fpm.match(tt.path); got != tt.want {
t.Errorf("{%q}.match(%q) = %v, want %v", tt.prefix, tt.path, got, tt.want)
}
Expand Down
28 changes: 19 additions & 9 deletions xds/internal/balancer/xdsrouting/matcher_test.go
View file
Expand Up @@ -38,7 +38,17 @@ func TestAndMatcherMatch(t *testing.T) {
}{
{
name: "both match",
pm: newPathExactMatcher("/a/b"),
pm: newPathExactMatcher("/a/b", false),
hm: newHeaderExactMatcher("th", "tv"),
info: balancer.PickInfo{
FullMethodName: "/a/b",
Ctx: metadata.NewOutgoingContext(context.Background(), metadata.Pairs("th", "tv")),
},
want: true,
},
{
name: "both match with path case insensitive",
pm: newPathExactMatcher("/A/B", true),
hm: newHeaderExactMatcher("th", "tv"),
info: balancer.PickInfo{
FullMethodName: "/a/b",
Expand All @@ -48,7 +58,7 @@ func TestAndMatcherMatch(t *testing.T) {
},
{
name: "only one match",
pm: newPathExactMatcher("/a/b"),
pm: newPathExactMatcher("/a/b", false),
hm: newHeaderExactMatcher("th", "tv"),
info: balancer.PickInfo{
FullMethodName: "/z/y",
Expand All @@ -58,7 +68,7 @@ func TestAndMatcherMatch(t *testing.T) {
},
{
name: "both not match",
pm: newPathExactMatcher("/z/y"),
pm: newPathExactMatcher("/z/y", false),
hm: newHeaderExactMatcher("th", "abc"),
info: balancer.PickInfo{
FullMethodName: "/a/b",
Expand All @@ -68,7 +78,7 @@ func TestAndMatcherMatch(t *testing.T) {
},
{
name: "fake header",
pm: newPathPrefixMatcher("/"),
pm: newPathPrefixMatcher("/", false),
hm: newHeaderExactMatcher("content-type", "fake"),
info: balancer.PickInfo{
FullMethodName: "/a/b",
Expand All @@ -80,7 +90,7 @@ func TestAndMatcherMatch(t *testing.T) {
},
{
name: "binary header",
pm: newPathPrefixMatcher("/"),
pm: newPathPrefixMatcher("/", false),
hm: newHeaderPresentMatcher("t-bin", true),
info: balancer.PickInfo{
FullMethodName: "/a/b",
Expand Down Expand Up @@ -146,8 +156,8 @@ func TestCompositeMatcherEqual(t *testing.T) {
}{
{
name: "equal",
pm: newPathExactMatcher("/a/b"),
mm: newCompositeMatcher(newPathExactMatcher("/a/b"), nil, nil),
pm: newPathExactMatcher("/a/b", false),
mm: newCompositeMatcher(newPathExactMatcher("/a/b", false), nil, nil),
want: true,
},
{
Expand All @@ -158,9 +168,9 @@ func TestCompositeMatcherEqual(t *testing.T) {
},
{
name: "not equal",
pm: newPathExactMatcher("/a/b"),
pm: newPathExactMatcher("/a/b", false),
fm: newFractionMatcher(123),
mm: newCompositeMatcher(newPathExactMatcher("/a/b"), nil, nil),
mm: newCompositeMatcher(newPathExactMatcher("/a/b", false), nil, nil),
want: false,
},
}
Expand Down
4 changes: 2 additions & 2 deletions xds/internal/balancer/xdsrouting/routing.go
View file
Expand Up @@ -136,9 +136,9 @@ func routeToMatcher(r routeConfig) (*compositeMatcher, error) {
}
pathMatcher = newPathRegexMatcher(re)
case r.path != "":
pathMatcher = newPathExactMatcher(r.path)
pathMatcher = newPathExactMatcher(r.path, r.caseInsensitive)
default:
pathMatcher = newPathPrefixMatcher(r.prefix)
pathMatcher = newPathPrefixMatcher(r.prefix, r.caseInsensitive)
}

var headerMatchers []headerMatcherInterface
Expand Down
5 changes: 5 additions & 0 deletions xds/internal/balancer/xdsrouting/routing_config.go
View file
Expand Up @@ -52,6 +52,9 @@ type routeConfig struct {
// Path, Prefix and Regex can have at most one set. This is guaranteed by
// config parsing.
path, prefix, regex string
// Indicates if prefix/path matching should be case insensitive. The default
// is false (case sensitive).
caseInsensitive bool

headers []headerMatcher
fraction *uint32
Expand All @@ -74,6 +77,7 @@ type lbConfig struct {
type routeJSON struct {
// Path, Prefix and Regex can have at most one non-nil.
Path, Prefix, Regex *string
CaseInsensitive bool
// Zero or more header matchers.
Headers []*xdsclient.HeaderMatcher
MatchFraction *wrapperspb.UInt32Value
Expand All @@ -99,6 +103,7 @@ func (jc lbConfigJSON) toLBConfig() *lbConfig {
case r.Regex != nil:
tempR.regex = *r.Regex
}
tempR.caseInsensitive = r.CaseInsensitive
for _, h := range r.Headers {
var tempHeader headerMatcher
switch {
Expand Down
20 changes: 10 additions & 10 deletions xds/internal/balancer/xdsrouting/routing_picker_test.go
View file
Expand Up @@ -52,7 +52,7 @@ func (s) TestRoutingPickerGroupPick(t *testing.T) {
{
name: "one route no match",
routes: []route{
{m: newCompositeMatcher(newPathPrefixMatcher("/a/"), nil, nil), action: "action-0"},
{m: newCompositeMatcher(newPathPrefixMatcher("/a/", false), nil, nil), action: "action-0"},
},
pickers: map[string]*subBalancerState{
"action-0": {state: balancer.State{
Expand All @@ -66,7 +66,7 @@ func (s) TestRoutingPickerGroupPick(t *testing.T) {
{
name: "one route one match",
routes: []route{
{m: newCompositeMatcher(newPathPrefixMatcher("/a/"), nil, nil), action: "action-0"},
{m: newCompositeMatcher(newPathPrefixMatcher("/a/", false), nil, nil), action: "action-0"},
},
pickers: map[string]*subBalancerState{
"action-0": {state: balancer.State{
Expand All @@ -80,8 +80,8 @@ func (s) TestRoutingPickerGroupPick(t *testing.T) {
{
name: "two routes first match",
routes: []route{
{m: newCompositeMatcher(newPathPrefixMatcher("/a/"), nil, nil), action: "action-0"},
{m: newCompositeMatcher(newPathPrefixMatcher("/z/"), nil, nil), action: "action-1"},
{m: newCompositeMatcher(newPathPrefixMatcher("/a/", false), nil, nil), action: "action-0"},
{m: newCompositeMatcher(newPathPrefixMatcher("/z/", false), nil, nil), action: "action-1"},
},
pickers: map[string]*subBalancerState{
"action-0": {state: balancer.State{
Expand All @@ -99,8 +99,8 @@ func (s) TestRoutingPickerGroupPick(t *testing.T) {
{
name: "two routes second match",
routes: []route{
{m: newCompositeMatcher(newPathPrefixMatcher("/a/"), nil, nil), action: "action-0"},
{m: newCompositeMatcher(newPathPrefixMatcher("/z/"), nil, nil), action: "action-1"},
{m: newCompositeMatcher(newPathPrefixMatcher("/a/", false), nil, nil), action: "action-0"},
{m: newCompositeMatcher(newPathPrefixMatcher("/z/", false), nil, nil), action: "action-1"},
},
pickers: map[string]*subBalancerState{
"action-0": {state: balancer.State{
Expand All @@ -118,8 +118,8 @@ func (s) TestRoutingPickerGroupPick(t *testing.T) {
{
name: "two routes both match former more specific",
routes: []route{
{m: newCompositeMatcher(newPathExactMatcher("/a/b"), nil, nil), action: "action-0"},
{m: newCompositeMatcher(newPathPrefixMatcher("/a/"), nil, nil), action: "action-1"},
{m: newCompositeMatcher(newPathExactMatcher("/a/b", false), nil, nil), action: "action-0"},
{m: newCompositeMatcher(newPathPrefixMatcher("/a/", false), nil, nil), action: "action-1"},
},
pickers: map[string]*subBalancerState{
"action-0": {state: balancer.State{
Expand All @@ -138,8 +138,8 @@ func (s) TestRoutingPickerGroupPick(t *testing.T) {
{
name: "tow routes both match latter more specific",
routes: []route{
{m: newCompositeMatcher(newPathPrefixMatcher("/a/"), nil, nil), action: "action-0"},
{m: newCompositeMatcher(newPathExactMatcher("/a/b"), nil, nil), action: "action-1"},
{m: newCompositeMatcher(newPathPrefixMatcher("/a/", false), nil, nil), action: "action-0"},
{m: newCompositeMatcher(newPathExactMatcher("/a/b", false), nil, nil), action: "action-1"},
},
pickers: map[string]*subBalancerState{
"action-0": {state: balancer.State{
Expand Down
9 changes: 6 additions & 3 deletions xds/internal/client/client.go
View file
Expand Up @@ -167,9 +167,12 @@ type VirtualHost struct {
// indication of the action to take upon match.
type Route struct {
Path, Prefix, Regex *string
Headers []*HeaderMatcher
Fraction *uint32
Action map[string]uint32 // action is weighted clusters.
// Indicates if prefix/path matching should be case insensitive. The default
// is false (case sensitive).
CaseInsensitive bool
Headers []*HeaderMatcher
Fraction *uint32
Action map[string]uint32 // action is weighted clusters.
}

// HeaderMatcher represents header matchers.
Expand Down
27 changes: 24 additions & 3 deletions xds/internal/client/client_rds_test.go
View file
Expand Up @@ -280,7 +280,14 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) {
Route: &v3routepb.RouteAction{
ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName},
}}}}}}},
wantError: true,
wantUpdate: RouteConfigUpdate{
VirtualHosts: []*VirtualHost{
{
Domains: []string{ldsTarget},
Routes: []*Route{{Prefix: newStringP("/"), CaseInsensitive: true, Action: map[string]uint32{clusterName: 1}}},
},
},
},
},
{
name: "good-route-config-with-empty-string-route",
Expand Down Expand Up @@ -434,7 +441,7 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
gotUpdate, gotError := generateRDSUpdateFromRouteConfiguration(test.rc, nil)
if (gotError != nil) != test.wantError || !cmp.Equal(gotUpdate, test.wantUpdate, cmpopts.EquateEmpty()) {
t.Errorf("generateRDSUpdateFromRouteConfiguration(%+v, %v) = %v, want %v", test.rc, ldsTarget, gotUpdate, test.wantUpdate)
t.Errorf("generateRDSUpdateFromRouteConfiguration(%+v, %v) returned unexpected, diff (-want +got):\\n%s", test.rc, ldsTarget, cmp.Diff(test.wantUpdate, gotUpdate, cmpopts.EquateEmpty()))
}
})
}
Expand Down Expand Up @@ -654,8 +661,22 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/"},
CaseSensitive: &wrapperspb.BoolValue{Value: false},
},
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{{
Prefix: newStringP("/"),
CaseInsensitive: true,
Action: map[string]uint32{"A": 40, "B": 60},
}},
wantErr: true,
},
{
name: "good",
Expand Down
8 changes: 4 additions & 4 deletions xds/internal/client/client_xds.go
View file
Expand Up @@ -171,10 +171,6 @@ func routesProtoToSlice(routes []*v3routepb.Route, logger *grpclog.PrefixLogger)
continue
}

if caseSensitive := match.GetCaseSensitive(); caseSensitive != nil && !caseSensitive.Value {
return nil, fmt.Errorf("route %+v has case-sensitive false", r)
}

pathSp := match.GetPathSpecifier()
if pathSp == nil {
return nil, fmt.Errorf("route %+v doesn't have a path specifier", r)
Expand All @@ -193,6 +189,10 @@ func routesProtoToSlice(routes []*v3routepb.Route, logger *grpclog.PrefixLogger)
continue
}

if caseSensitive := match.GetCaseSensitive(); caseSensitive != nil {
route.CaseInsensitive = !caseSensitive.Value
}

for _, h := range match.GetHeaders() {
var header HeaderMatcher
switch ht := h.GetHeaderMatchSpecifier().(type) {
Expand Down

0 comments on commit d4e2d0c

Please sign in to comment.