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.

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

rds: allow case_insensitive path matching #3997

Merged
merged 3 commits into from Nov 5, 2020
Merged

Conversation

@menghanl
Copy link
Contributor

@menghanl menghanl commented Oct 29, 2020

  • 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
@menghanl menghanl requested a review from dfawley Oct 29, 2020
@menghanl menghanl added this to the 1.34 Release milestone Oct 29, 2020
@@ -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
Copy link
Contributor

@easwars easwars Nov 3, 2020

Indicates if prefix/path matching should be case insensitive?

Copy link
Contributor Author

@menghanl menghanl Nov 4, 2020

Done

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

@easwars easwars Nov 3, 2020

Ditto here with the comment.

Copy link
Contributor Author

@menghanl menghanl Nov 4, 2020

Done

@@ -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

@easwars easwars Nov 3, 2020

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

@menghanl menghanl Nov 4, 2020

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

@easwars easwars Nov 4, 2020

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

@menghanl menghanl Nov 4, 2020

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

@@ -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

@easwars easwars Nov 4, 2020

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.

@@ -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

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

@menghanl menghanl Nov 4, 2020

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

@easwars easwars Nov 5, 2020

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

@menghanl menghanl Nov 5, 2020

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

}{
{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
Contributor

@dfawley dfawley Nov 4, 2020

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

Copy link
Contributor Author

@menghanl menghanl Nov 5, 2020

Done

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

@dfawley dfawley Nov 4, 2020

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

Copy link
Contributor Author

@menghanl menghanl Nov 5, 2020

Done

@dfawley dfawley assigned menghanl and unassigned dfawley Nov 4, 2020
@menghanl menghanl assigned dfawley and unassigned menghanl Nov 5, 2020
dfawley
dfawley approved these changes Nov 5, 2020
@dfawley dfawley assigned menghanl and unassigned dfawley Nov 5, 2020
@menghanl menghanl merged commit 25ddfdd into grpc:master Nov 5, 2020
11 checks passed
@menghanl menghanl deleted the case_sensitive branch Nov 5, 2020
davidkhala added a commit to Hyperledger-TWGC/grpc that referenced this issue Dec 7, 2020
- 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
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants