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

rds: allow case_insensitive path matching #3997

Merged
merged 3 commits into from Nov 5, 2020
Merged

rds: allow case_insensitive path matching #3997

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
00069bb
[case_sensitive] rds: allow case_insensitive path matching
menghanl Oct 28, 2020
18d454e
[case_sensitive] c1
menghanl Nov 4, 2020
287618c
[case_sensitive] more tests
menghanl Nov 5, 2020
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
40 changes: 32 additions & 8 deletions xds/internal/balancer/xdsrouting/matcher_path.go
View file
Open in desktop
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)
easwars marked this conversation as resolved.
Show resolved Hide resolved
}
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
Open in desktop
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},
Copy link
Member

Choose a reason for hiding this comment

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

Also: fullPath: "/s/M", path: "/S/m"

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

{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},
Copy link
Member

Choose a reason for hiding this comment

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

Similar: prefix: "/S/", path: "/s/m"

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

{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
Open in desktop
Expand Up @@ -38,7 +38,17 @@ func TestAndMatcherMatch(t *testing.T) {
}{
{
name: "both match",
pm: newPathExactMatcher("/a/b"),
pm: newPathExactMatcher("/a/b", false),
easwars marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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{
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm ... does it make sense for our internal types to match the xDS proto in the sense that we also store whether the match is CaseSensitive instead of the other way around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used case insensitive in the field because the default value of bool is false, and it's a sane default.

If I make it case sensitive, I will need to make it a *bool, so when it's not set, nil means case sensitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

I used case insensitive in the field because the default value of bool is false, and it's a sane default.

It looks to me with the way the proto is structured that they want the default to be case sensitive. This is comment on the field:

  // Indicates that prefix/path matching should be case sensitive. The default
  // is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If nothing is configured for case-sensitive-or-not, matching should be case-sensitive == true.

If the field is CaseSensitive, it has to be a *bool, so if nothing is configured, its default value is nil, and we can handle it as case-sensitive == true

If the field is CaseInsensitive, when nothing is configured, it's default value is false, same as case-sensitive == true

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
Open in desktop
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 {
Copy link
Contributor

@easwars easwars Nov 4, 2020 •
edited

Choose a reason for hiding this comment

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

Based on the comment on the proto field, if the field is not set, we should assume a value of true, right?

 // Indicates that prefix/path matching should be case sensitive. The default
 // is true.

Hmm ... I guess that's what we end up with because of the double negation with the caseInsensitive. If we can always set the value of our field correctly here, wouldn't it be better to use a field which matches the proto?
I just feel that this inconsistency might bite us later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we really want to be consistent, the CaseSensitive field has to be a boolean pointer. I really don't like using pointers when we can have a default value that just works, because you will need to check nil before reading its value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really suggesting that we use a *bool. But, given that when we process the received route configuration, we are anyways looking to see whether this field is set or not? So, we could still use a caseSensitive bool and set it up as we want based on whether this field exists, and if it exists, based on its value.

But if you are really happy with the way things are, then that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm keeping it for now. If we later find this is causing problems, we can update. This is not a public API.

route.CaseInsensitive = !caseSensitive.Value
}

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