rds: allow case_insensitive path matching #3997

menghanl · 2020-10-29T17:14:44Z

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

- in xds_client, accept (not NACK) RDS resp with case_insensitive=true - pass case_insensitive to xds resolver and routing balancer - after the config selector change, the routing balancer will be removed, and this will be handled in the resolver config selector

xds/internal/balancer/xdsrouting/matcher_path.go

xds/internal/balancer/xdsrouting/matcher_test.go

easwars · 2020-11-03T21:32:13Z

xds/internal/balancer/xdsrouting/routing_config.go

@@ -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 sensitive. The default


Indicates if prefix/path matching should be case insensitive?

easwars · 2020-11-03T21:32:48Z

xds/internal/client/client.go

-	Headers             []*HeaderMatcher
-	Fraction            *uint32
-	Action              map[string]uint32 // action is weighted clusters.
+	// Indicates if prefix/path matching should be case sensitive. The default


Ditto here with the comment.

easwars · 2020-11-03T21:34:58Z

xds/internal/client/client_rds_test.go

@@ -280,7 +280,14 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) {
 							Route: &v3routepb.RouteAction{
 								ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName},
 							}}}}}}},
-			wantError: true,
+			wantUpdate: RouteConfigUpdate{


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?

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.

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.

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

easwars · 2020-11-04T15:44:19Z

xds/internal/client/client_rds_test.go

@@ -280,7 +280,14 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) {
 							Route: &v3routepb.RouteAction{
 								ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName},
 							}}}}}}},
-			wantError: true,
+			wantUpdate: RouteConfigUpdate{


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.

easwars · 2020-11-04T15:50:50Z

xds/internal/client/client_xds.go

@@ -193,6 +189,10 @@ func routesProtoToSlice(routes []*v3routepb.Route, logger *grpclog.PrefixLogger)
 			continue
 		}

+		if caseSensitive := match.GetCaseSensitive(); caseSensitive != nil {


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.

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.

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.

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

dfawley · 2020-11-04T22:31:53Z

xds/internal/balancer/xdsrouting/matcher_path_test.go

 	}{
 		{name: "match", fullPath: "/s/m", path: "/s/m", want: true},
+		{name: "case insensitive match", fullPath: "/s/m", caseInsensitive: true, path: "/S/m", want: true},


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

dfawley · 2020-11-04T22:32:17Z

xds/internal/balancer/xdsrouting/matcher_path_test.go

 	}{
 		{name: "match", prefix: "/s/", path: "/s/m", want: true},
+		{name: "case insensitive match", prefix: "/s/", caseInsensitive: true, path: "/S/m", want: true},


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

- 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

menghanl requested a review from dfawley October 29, 2020 17:14

menghanl assigned dfawley Oct 29, 2020

menghanl added the Type: Feature label Oct 29, 2020

menghanl added this to the 1.34 Release milestone Oct 29, 2020

easwars reviewed Nov 3, 2020

View reviewed changes

[case_sensitive] c1

18d454e

easwars reviewed Nov 4, 2020

View reviewed changes

dfawley reviewed Nov 4, 2020

View reviewed changes

dfawley assigned menghanl and unassigned dfawley Nov 4, 2020

[case_sensitive] more tests

287618c

menghanl assigned dfawley and unassigned menghanl Nov 5, 2020

dfawley approved these changes Nov 5, 2020

View reviewed changes

dfawley assigned menghanl and unassigned dfawley Nov 5, 2020

menghanl merged commit 25ddfdd into grpc:master Nov 5, 2020

menghanl deleted the case_sensitive branch November 5, 2020 23:04

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rds: allow case_insensitive path matching #3997

rds: allow case_insensitive path matching #3997

menghanl commented Oct 29, 2020

easwars Nov 3, 2020

menghanl Nov 4, 2020

easwars Nov 3, 2020

menghanl Nov 4, 2020

easwars Nov 3, 2020

menghanl Nov 4, 2020

easwars Nov 4, 2020

menghanl Nov 4, 2020

easwars Nov 4, 2020

easwars Nov 4, 2020 •

edited

menghanl Nov 4, 2020

easwars Nov 5, 2020

menghanl Nov 5, 2020

dfawley Nov 4, 2020

menghanl Nov 5, 2020

dfawley Nov 4, 2020

menghanl Nov 5, 2020

rds: allow case_insensitive path matching #3997

rds: allow case_insensitive path matching #3997

Conversation

menghanl commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars Nov 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars Nov 4, 2020 •

edited