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

Conversation

menghanl
Copy link
Contributor

  • 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 October 29, 2020 17:14
@menghanl menghanl added the Type: Feature New features or improvements in behavior label 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

Choose a reason for hiding this comment

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

Indicates if prefix/path matching should be case insensitive?

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

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

Choose a reason for hiding this comment

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

Ditto here with the comment.

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

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

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

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

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.

}{
{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: "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

@dfawley dfawley assigned menghanl and unassigned dfawley Nov 4, 2020
@menghanl menghanl assigned dfawley and unassigned menghanl Nov 5, 2020
@dfawley dfawley assigned menghanl and unassigned dfawley Nov 5, 2020
@menghanl menghanl merged commit 25ddfdd into grpc:master Nov 5, 2020
@menghanl menghanl deleted the case_sensitive branch November 5, 2020 23:04
davidkhala pushed a commit to Hyperledger-TWGC/grpc that referenced this pull request 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.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants