Just a couple of quick comments about the data structures, some of which you and I have already discussed. I'll hold off on reviewing the rest until you're ready.

Please let me know if you have any questions. Thanks!

Reviewed 1 of 5 files at r1.
Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @donnadionne)

src/core/ext/filters/client_channel/xds/xds_api.h, line 53 at r1 (raw file):

      // for: PathMatcher, HeaderMatcher, cluster_name and weighted_clusters
      struct PathMatcher {
        absl::optional<std::string> path;

Could use an enum here to avoid needing absl::optional<> for each element.

src/core/ext/filters/client_channel/xds/xds_api.h, line 64 at r1 (raw file):

        };
        std::string name;
        absl::optional<std::string> exact_match;

Can use the same enum approach here.

src/core/ext/filters/client_channel/xds/xds_api.h, line 74 at r1 (raw file):

      struct Fraction {
        uint32_t numberator;

s/numberator/numerator/

src/core/ext/filters/client_channel/xds/xds_api.h, line 75 at r1 (raw file):

      struct Fraction {
        uint32_t numberator;
        uint32_t denominator;

The denominator in xDS can only be three possible values: 100, 10K, or 1M. It might be simpler to just assume 1M here and convert the numerator accordingly, so that we only need to store the numerator (e.g., if xDS says 3/100, we convert that to 30K/1M and store 30K).

src/core/ext/filters/client_channel/xds/xds_api.h, line 80 at r1 (raw file):

      PathMatcher path_matcher;
      std::vector<HeaderMatcher> headers;
      Fraction match_fraction;

This should use absl::optional<>.

src/core/ext/filters/client_channel/xds/xds_api.h, line 93 at r1 (raw file):

      std::vector<ClusterWeight> weighted_clusters;

      bool operator==(const RdsRoute& other) const {

This will need to check all of the new fields above. For the ones that are structs, each struct should define its own operator==().

I have written initial code for passing the matchers all the way through into xds routing policy. Please provide initial comments.

As well, I hope to use this code review as way to ask some specific questions. Please see my comments inline.

My main questions are:

1. How to parse certain envoy structures without generated envoy methods
2. How much format checking should we do for saferegex specifier to avoid passing invalid specifier into the service config
3. How much format checking should we do for the new header matchers to avoid passing invalid specifier into the service config

Reviewable status: 0 of 7 files reviewed, 10 unresolved discussions (waiting on @markdroth)

src/core/ext/filters/client_channel/xds/xds_api.h, line 53 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 64 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 74 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 75 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 80 at r1 (raw file):

q: fraction is required?

src/core/ext/filters/client_channel/xds/xds_api.h, line 93 at r1 (raw file):

will modify once all fields are finalized

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1034 at r2 (raw file):

    rds_route->path_matcher.regex = "TODO TBD";
    const struct envoy_type_matcher_RegexMatcher* regex_matcher =
        envoy_api_v2_route_RouteMatch_safe_regex(match);

I know I am suppose to get the regex string from envoy_type_matcher_RegexMatcher; however i just have a forwarded declaration and there are no generated access methods in src/core/ext/upb-generated/envoy/api/v2/route/route_components.upb.h

I have the same question for several structures like Range and Fraction

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1037 at r2 (raw file):

    GPR_ASSERT(regex_matcher != nullptr);
    // TODO@donnadionne how do I extract regex from struct
    // envoy_type_matcher_RegexMatcher without depending on the structure

Another question I have is that assume I can get the regex string; how much format checking should I do for it?

For prefix and path we check for the right number of slashes and things like that. Anything to do here.

The same question applies to all the new header matcher strings too: prefix, suffix, exact, etc...

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1065 at r2 (raw file):

      header_matcher.content.regex_match = "TODO TBD";
      const struct envoy_type_matcher_RegexMatcher* regex_matcher =
          envoy_api_v2_route_HeaderMatcher_safe_regex_match(header);

I know I am suppose to get the regex string from envoy_type_matcher_RegexMatcher; however i just have a forwarded declaration and there are no generated access methods in src/core/ext/upb-generated/envoy/api/v2/route/route_components.upb.h

I have the same question for several structures like Range and Fraction

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1067 at r2 (raw file):

The overall structure here looks very good! I think I've answered your questions in-line, but please let me know if I missed anything, or if you have any other questions. Thanks!

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 41 unresolved discussions (waiting on @donnadionne and @markdroth)

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 53 at r2 (raw file):

class XdsRoutingLbConfig : public LoadBalancingPolicy::Config {
 public:
  struct Route {

If you take my suggestion from xds_api.h below about moving all of the matchers into their own nested struct, then we can reuse that same matcher struct here. This struct can become the following:

struct Route {
  XdsApi::RdsUpdate::RdsRoute::Matchers matchers;
  std::string action;
};

We'll still have to convert it to JSON and back again, but at least we won't have to maintain two copies of the same struct.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 151 at r2 (raw file):

  class RoutePicker : public SubchannelPicker {
   public:
    struct Route {

If you take my suggestion from xds_api.h below about moving all of the matchers into their own nested struct, then we can reuse that same matcher struct here. This struct can become the following:

struct Route {
  XdsApi::RdsUpdate::RdsRoute::Matchers matchers;
  RefCountedPtr<ChildPickerWrapper> picker;
};

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 272 at r2 (raw file):

  }
  for (const Route& route : route_table_) {
    if ((route.path_matcher_type ==

We'll obviously need to start looking at all of the match criteria here, not just the path matchers. It might be a good idea to add a private method that checks whether a given route matches the current request, so that we don't have to inline all of that code here.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 406 at r2 (raw file):

      for (const auto& config_route : config_->route_table()) {
        RoutePicker::Route route;
        route.path_matcher_type = config_route.path_matcher_type;

Now that there are so many more fields being copied here, we should consider avoiding this by having the picker take a ref to config_. Then, instead of making a copy of the matcher struct, it can just store a pointer to the relevant matcher struct in the config.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 735 at r2 (raw file):

      error_list.push_back(error);
    }
    if (!route_table.back().path_matcher.prefix.empty() ||

This check can go away now. It's totally fine for the last matcher to not be a default route.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 793 at r2 (raw file):

      return error_list;
    }
    // Parse Path Matcher: prefix.

We should reject the config if more than one of prefix, path, and regex is specified. We should also reject if none of them are present.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 867 at r2 (raw file):

              }
            }
            header_it = header_json.object_value().find("exact_match");

We should reject the config if more than one header match field is specified in the same array element.

src/core/ext/filters/client_channel/xds/xds_api.h, line 80 at r1 (raw file):

No, it's not required. That's why I said it should use absl::optional<>.

src/core/ext/filters/client_channel/xds/xds_api.h, line 49 at r2 (raw file):

  struct RdsUpdate {
    struct RdsRoute {

It might be a good idea to move all of the matchers to a nested struct here, since we would be able to reuse that in multiple places. In other words:

struct RdsRoute {
  // Matchers for this route.
  struct Matchers {
    // ...path matcher, header matchers, fraction matcher...
  };

  // Action for this route.
  std::string cluster_name;
  struct ClusterWeight {
    // ...
  };
  std::vector<ClusterWeight> weighted_clusters;
};

This will allow us to reuse the RdsRoute::Matchers struct in the xds_routing policy.

src/core/ext/filters/client_channel/xds/xds_api.h, line 52 at r2 (raw file):

      // TODO(donnadionne): When we can use absl::variant<>, consider using that
      // for: PathMatcher, HeaderMatcher, cluster_name and weighted_clusters
      enum class PathMatcherType {

This can be moved inside of the PathMatcher struct.

src/core/ext/filters/client_channel/xds/xds_api.h, line 59 at r2 (raw file):

      struct PathMatcher {
        std::string path;

As an optimization, we could just have a single string field here, since we have the enum to tell us whether the string is a path, prefix, or regexp.

src/core/ext/filters/client_channel/xds/xds_api.h, line 64 at r2 (raw file):

      };

      enum class HeaderMatcherType {

This can be moved inside of the HeaderMatcher struct.

src/core/ext/filters/client_channel/xds/xds_api.h, line 73 at r2 (raw file):

      };

      struct HeaderMatcherContent {

This doesn't need to be a separate struct; its fields can be moved directly inside of the HeaderMatcher struct.

src/core/ext/filters/client_channel/xds/xds_api.h, line 79 at r2 (raw file):

        };
        std::string name;
        std::string exact_match;

Same comment as above: As an optimization, we could combine all of these string fields into one, since we have the enum to tell us how to interpret the value.

src/core/ext/filters/client_channel/xds/xds_api.h, line 93 at r2 (raw file):

      };

      PathMatcherType path_matcher_type;

Please move this field inside of the PathMatcher struct.

src/core/ext/filters/client_channel/xds/xds_api.h, line 96 at r2 (raw file):

      PathMatcher path_matcher;
      std::vector<HeaderMatcher> headers;
      uint32_t runtime_fraction_numerator_out_of_1M;

Suggest calling this fraction_per_million.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 974 at r2 (raw file):

}

grpc_error* RouteMatchParse(const envoy_api_v2_route_RouteMatch* match,

Suggest calling this RoutePathMatchParse().

src/core/ext/filters/client_channel/xds/xds_api.cc, line 982 at r2 (raw file):

      // Prefix "/" is accepted.
      if (prefix.data[0] != '/') {
        return GRPC_ERROR_CREATE_FROM_STATIC_STRING(

Instead of rejecting the code, I think we should just ignore the route in this case, since we know it can't possibly match anything that gRPC sends.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 985 at r2 (raw file):

            "Prefix does not start with a /");
      }
      if (prefix.size > 1) {

I think this conditional should be removed. The code in this block should still be executed even if the prefix size is 1.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 992 at r2 (raw file):

            absl::string_view(prefix.data, prefix.size).substr(1),
            absl::MaxSplits('/', 1));
        if (prefix_elements.size() != 2) {

These checks need to be updated. We will now support any arbitrary prefix, even if it names a partial service or a partial method.

The only cases we should weed out are prefixes that include more than two slashes or that have two slashes in a row. And in those cases, we should ignore the route instead of rejecting the config.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1007 at r2 (raw file):

    upb_strview path = envoy_api_v2_route_RouteMatch_path(match);
    if (path.size == 0) {
      return GRPC_ERROR_CREATE_FROM_STATIC_STRING(

We can ignore the route in this case instead of rejecting the config.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1011 at r2 (raw file):

    }
    if (path.data[0] != '/') {
      return GRPC_ERROR_CREATE_FROM_STATIC_STRING(

Same here.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1018 at r2 (raw file):

    rds_route->path_matcher.path = UpbStringToStdString(path);
    std::vector<absl::string_view> path_elements =
        absl::StrSplit(absl::string_view(path.data, path.size).substr(1), '/');

Can use MaxSplits() here, just like we do in the prefix case above.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1019 at r2 (raw file):

    std::vector<absl::string_view> path_elements =
        absl::StrSplit(absl::string_view(path.data, path.size).substr(1), '/');
    if (path_elements.size() != 2) {

Same comment as above: We should just ignore any routes with paths we can't possibly match, rather than rejecting the config.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1034 at r2 (raw file):

You need to include the files that those declarations are in:

https://github.com/grpc/grpc/blob/master/src/core/ext/upb-generated/envoy/type/matcher/regex.upb.h
https://github.com/grpc/grpc/blob/master/src/core/ext/upb-generated/envoy/type/range.upb.h
https://github.com/grpc/grpc/blob/master/src/core/ext/upb-generated/envoy/type/percent.upb.h

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1037 at r2 (raw file):

There's no reasonable way to validate that a regexp will only ever match a path with only two components, so we won't bother checking for that. However, we do need to use the re2 library to compile the regexp here, to make sure it's a valid regexp. If the compilation fails, we should reject the config.

I don't think there's any special validation to be done here for the header matching strings, since headers can contain fairly arbitrary values.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1042 at r2 (raw file):

        "Invalid route path specifier specified.");
  }
  return GRPC_ERROR_NONE;

We also need to check if case_sensitive is set to true, and if so, we need to reject the config.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1065 at r2 (raw file):

See above -- you need to include the right files.

As mentioned above, note that we need to compile the regexp here to make sure it's valid.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1067 at r2 (raw file):

I'm guessing that this comment was an accident. :)

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1075 at r2 (raw file):

          envoy_api_v2_route_HeaderMatcher_range_match(header);
      GPR_ASSERT(range_matcher != nullptr);
      // TODO@donnadionne how do I extract range from struct

We'll need basic validation here, like that the lower bound is lower than the upper bound.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1183 at r2 (raw file):

    }
  } else {
    return GRPC_ERROR_CREATE_FROM_STATIC_STRING(

In this case (cluster_header is set), we should ignore the route instead of failing.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1285 at r2 (raw file):

        envoy_api_v2_route_Route_match(route);
    XdsApi::RdsUpdate::RdsRoute rds_route;
    grpc_error* error = RouteMatchParse(match, &rds_route);

We also need to check if the query_params field is set, in which case we should ignore the route.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 919 at r2 (raw file):

      gpr_log(GPR_INFO, "  RouteConfiguration contains %" PRIuPTR " routes",
              lds_update->rds_update.value().routes.size());
      // TODO @donnadionne: add printing of route details.

It might make sense to add a ToString() method on the Route struct for this.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2045 at r2 (raw file):

std::string CreateServiceConfigRoute(
    const std::string& action_name,
    const uint32_t runtime_fraction_numerator_out_of_1M,

Why is this being passed in separately, if you're already passing in the route itself? Doesn't the route include this field already?

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2053 at r2 (raw file):

      case XdsApi::RdsUpdate::RdsRoute::HeaderMatcherType::EXACT:
        header_matcher =
            absl::StrFormat("             \"%s\": \"%s\",\n", "exact_match",

There's no need for "exact_match" to be a separate string. It's already a string literal, so it can be embedded directly into the format string.

Same thing for all of the cases here.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2083 at r2 (raw file):

        break;
    }
    if (!header_matcher.empty()) {

It should never be empty, so I don't think we need to check this here.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2093 at r2 (raw file):

          "             \"%s\": %s\n"
          "           }",
          "invert_match", header.content.invert_match ? "true" : "false"));

Since the default for this field is false, there's no reason to add it unless it's set to true.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2109 at r2 (raw file):

          "           \"prefix\": \"%s\",\n"
          "           %s"
          "           \"fraction_numerator\":%d,\n"

This field is optional and should be added only if set.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2119 at r2 (raw file):

          "           \"path\": \"%s\",\n"
          "           %s"
          "           \"fraction_numerator\":%d,\n"

Instead of duplicating the code for the fraction, headers, and action for each path matcher type, I suggest something like this:

std::string path_match_str;
switch (route.path_matcher_type) {
  case XdsApi::RdsUpdate::RdsRoute::PathMatcherType::PREFIX:
    path_match_str = absl::StrFormat("\"prefix\": \"%s\",\n", route.path_matcher.prefix);
    break;
  case XdsApi::RdsUpdate::RdsRoute::PathMatcherType::PATH:
    path_match_str = absl::StrFormat("\"path\": \"%s\",\n", route.path_matcher.path);
    break;
  case XdsApi::RdsUpdate::RdsRoute::PathMatcherType::REGEX:
    path_match_str = absl::StrFormat("\"regex\": \"%s\",\n", route.path_matcher.regex);
    break;
}

Then construct the resulting string just once using path_match_str.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2345 at r2 (raw file):

      "}");
  std::string json = absl::StrJoin(config_parts, "");
  gpr_log(GPR_INFO, "donna service config json %s", json.c_str());

I assume this is just for debugging and will be removed before merging.

I have finished implementing everything, except for integrating with re2; I will work on that PR now (just 1 more set of build test: distribution test still failing)

I would like for you to take a look to see if there are any major that needs addressing

I will also add more tests once i figure out the initial metadata bug

Reviewable status: 1 of 8 files reviewed, 40 unresolved discussions (waiting on @donnadionne and @markdroth)

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 53 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 151 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 272 at r2 (raw file):

yes, restructuring

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 406 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 735 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 793 at r2 (raw file):

using a counter to ensure there is one and only one.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 867 at r2 (raw file):

here I am only ever looking for one type of header match, if found i try to only construct that header matcher. is that sufficient? or are you suggesting something else?

src/core/ext/filters/client_channel/xds/xds_api.h, line 80 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 93 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 49 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 52 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 59 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 64 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 73 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 79 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 93 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 96 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 974 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 982 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 985 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 992 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1007 at r2 (raw file):

I implemented logic to ignore roots: to continue in the loop where we iterate all routes so that we don't get to the end of the loop where we add the route: rds_update->routes.emplace_back(std::move(rds_route));

Because i have several helper functions to help to parse the different parts of the route: path, headers, fraction, and actions, each helper function can set ignore_route flag.

Each helper fucntion already return errors which I handle by rejecting the config

tests are changed for the ignored routes. We will still get a NACK but it will be because the single route configured got ignored so we have no valid routes.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1011 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1018 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1019 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1037 at r2 (raw file):

added todo for re2 compile (will add it after re2 is integrated

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1042 at r2 (raw file):

addded test too; may separate out to a separate PR

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1065 at r2 (raw file):

will add after re2 is integrated

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1067 at r2 (raw file):

removed

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1075 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1183 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1285 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 919 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2045 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2053 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2083 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2093 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2109 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2119 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2345 at r2 (raw file):

yes

This looks really good! I have a lot of comments, but they're all relatively minor things -- no significant structural problems.

Please let me know if you have any questions. Thanks!

Reviewed 19 of 19 files at r4.
Reviewable status: all files reviewed, 44 unresolved discussions (waiting on @donnadionne and @markdroth)

BUILD, line 1448 at r4 (raw file):

        "grpc_base",
        "grpc_client_channel",
        "grpc_xds_client_secure",

The grpc_lb_policy_xds_routing target is included in both secure and insecure builds, so it can't depend on grpc_xds_client_secure.

Our normal solution to this problem is to have two variants of the grpc_lb_policy_xds_routing rule, one for secure builds that depends on grpc_xds_client_secure and another for insecure builds that depends on grpc_xds_client.

However, since all we actually need to depend on here is the struct definition from xds_api.h, and that file has nothing in it that differs between secure and insecure builds, it might be better to move that one file into its own rule, which can be a dependency of grpc_xds_client_secure, grpc_xds_client, and grpc_lb_policy_xds_routing.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 867 at r2 (raw file):

What happens if a single element in the array specifies both (e.g.) "regex_match" and "range_match"? We should reject the config in that case.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 220 at r4 (raw file):

// XdsRoutingLb::RoutePicker
//
absl::string_view GetMetadataValue(const std::string& key,

Please add a blank line before this function.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 220 at r4 (raw file):

// XdsRoutingLb::RoutePicker
//
absl::string_view GetMetadataValue(const std::string& key,

This return type is not sufficient, because it does not allow us to differentiate between a header whose value is the empty string and a header that is absent.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 221 at r4 (raw file):

//
absl::string_view GetMetadataValue(const std::string& key,
                                   LoadBalancingPolicy::PickArgs args) {

This should take just the MetadataInterface, not the PickArgs struct, since it doesn't need any of the other args.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 240 at r4 (raw file):

    case XdsApi::RdsUpdate::RdsRoute::Matchers::PathMatcher::PathMatcherType::
        PREFIX:
      if (absl::StartsWith(path, path_matcher.path_matcher)) {

This can just be return absl::StartsWith(...).

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 247 at r4 (raw file):

    case XdsApi::RdsUpdate::RdsRoute::Matchers::PathMatcher::PathMatcherType::
        PATH:
      if (path == path_matcher.path_matcher) {

This can just be return path == path_matcher.path_matcher.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 264 at r4 (raw file):

    const std::vector<XdsApi::RdsUpdate::RdsRoute::Matchers::HeaderMatcher>&
        header_matchers) {
  for (const auto& header_it : header_matchers) {

This should be named header_matcher. It's not an iterator.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 270 at r4 (raw file):

          XdsApi::RdsUpdate::RdsRoute::Matchers::HeaderMatcher::
              HeaderMatcherType::PRESENT) {
        if (!header_it.present_match ^ header_it.invert_match) {

I think that using ^ header_it.invert_match in all of these rules makes the expressions harder to understand. (I actually had to look up the ^ operator, since I use it so infrequently.)

Instead, I suggest having a separate function to check whether an individual HeaderMatcher matches, and then apply invert_match to the result:

for (const auto& header_matcher : header_matchers) {
  bool match = HeaderMatches(args, header_matcher);
  if (header_matcher.invert_match) match = !match;
  if (!match) return false;
}

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 284 at r4 (raw file):

      case XdsApi::RdsUpdate::RdsRoute::Matchers::HeaderMatcher::
          HeaderMatcherType::EXACT:
        if (value == header_it.header_matcher ^ header_it.invert_match) {

For each of these rules, instead of saying this:

if (expression) {
  continue;
} else {
  return false;
}
break;

we can just say:

if (!expression) {
  return false;
}
break;

The continue isn't actually needed, because that will happen anyway after the following break statement.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 335 at r4 (raw file):

  absl::BitGen random;
  uint32_t random_number =
      absl::uniform_int_distribution<int>(0, 1000000)(random);

Oooh, nice -- I didn't know absl provided a random API. Maybe we should also use that in weighted_target? I'm wondering if that might address some of the test flakiness we were looking at. (We can do this in a separate PR.)

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 336 at r4 (raw file):

  uint32_t random_number =
      absl::uniform_int_distribution<int>(0, 1000000)(random);
  if (random_number < fraction_per_million) return true;

return random_number < fraction_per_million

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 478 at r4 (raw file):

      for (const auto& config_route : config_->route_table()) {
        RoutePicker::Route route;
        route.matchers = &(config_route.matchers);

No need for the parens here.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 979 at r4 (raw file):

              } else {
                auto range_it =
                    header_it->second.object_value().find("range_start");

As per the gRFC, these fields should be called "start" and "end", not "range_start" and "range_end".

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 1067 at r4 (raw file):

    }
    // Parse Fraction numerator.
    it = json.object_value().find("fraction_numerator");

This should be called "match_fraction", as per the gRFC.

src/core/ext/filters/client_channel/xds/xds_api.h, line 119 at r4 (raw file):

      bool operator==(const RdsRoute& other) const {
        // TODO @donnadionne: implement this properly with all the fields

I think this TODO can go away now.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 163 at r4 (raw file):

      break;
  }
  return absl::StrFormat("Path Matcher:%s, of type %s", path_matcher,

Let's try to make all of these strings more concise and easily readable. In particular, let's put the type of matcher toward the front of the line instead of at the end, since readers need to know the type in order to interpret the value.

I suggest using the following format:

"(Path|Header) (path_type|header_match_type): (parameters)"

For example:

"Path prefix match: /foo/"
"Header range match: [min, max)"
"Header prefix match: (not) foo" (for the invert case)

src/core/ext/filters/client_channel/xds/xds_api.cc, line 169 at r4 (raw file):

std::string XdsApi::RdsUpdate::RdsRoute::Matchers::HeaderMatcher::ToString()
    const {
  std::string header_type_string;

This variable isn't necessary. Its value can be inlined directly into the format strings that we are passing to absl::StrFormat().

src/core/ext/filters/client_channel/xds/xds_api.cc, line 170 at r4 (raw file):

    const {
  std::string header_type_string;
  std::string invert = invert_match ? " inverted match" : "";

This can be const char* to avoid an unnecessary allocation.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 217 at r4 (raw file):

std::string XdsApi::RdsUpdate::RdsRoute::ClusterWeight::ToString() const {
  return absl::StrFormat("ClusterWeight %s of weight %d", name, weight);

This can just say something like "{cluster=%s, weight=%d}".

src/core/ext/filters/client_channel/xds/xds_api.cc, line 223 at r4 (raw file):

  std::vector<std::string> contents;
  contents.push_back(matchers.ToString());
  contents.push_back(absl::StrFormat("Cluster name: %s", cluster_name));

This line should be included only if cluster_name is non-empty.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1021 at r4 (raw file):

    upb_strview prefix = envoy_api_v2_route_RouteMatch_prefix(match);
    // Empty prefix "" is accepted.
    rds_route->matchers.path_matcher.path_type = XdsApi::RdsUpdate::RdsRoute::

I suggest moving these two statements down to the end of this block, since there's no point in executing them if we are going to ignore the route.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1037 at r4 (raw file):

                         absl::MaxSplits('/', 2));
      if (prefix_elements.size() > 2) {
        return GRPC_ERROR_CREATE_FROM_STATIC_STRING(

In this case, we should ignore the route instead of rejecting the config.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1040 at r4 (raw file):

            "Prefix cannot have more than 2 slashes");
      } else if (prefix_elements.size() == 2 && prefix_elements[0].empty()) {
        return GRPC_ERROR_CREATE_FROM_STATIC_STRING(

Same here.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1057 at r4 (raw file):

      return GRPC_ERROR_NONE;
    }
    rds_route->matchers.path_matcher.path_type = XdsApi::RdsUpdate::RdsRoute::

Suggest moving these two statements down to the end of this block, since there's no point executing them if we're going to ignore the route.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1080 at r4 (raw file):

    }
  } else if (envoy_api_v2_route_RouteMatch_has_safe_regex(match)) {
    rds_route->matchers.path_matcher.path_type = XdsApi::RdsUpdate::RdsRoute::

Suggest moving this statement down to the end of this block, right before we set the path_matcher field, since there's no point executing this if we're going to fail the config.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1097 at r4 (raw file):

grpc_error* RouteHeaderMatchersParse(const envoy_api_v2_route_RouteMatch* match,
                                     XdsApi::RdsUpdate::RdsRoute* rds_route,
                                     bool* ignore_route) {

This parameter does not seem to be needed here.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1128 at r4 (raw file):

      header_matcher.range_end = envoy_type_Int64Range_end(range_matcher);
      if (header_matcher.range_end < header_matcher.range_start) {
        return GRPC_ERROR_CREATE_FROM_STATIC_STRING(

Please add a test for this case.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1160 at r4 (raw file):

grpc_error* RouteRuntimeFractionParse(
    const envoy_api_v2_route_RouteMatch* match,
    XdsApi::RdsUpdate::RdsRoute* rds_route, bool* ignore_route) {

The ignore_route parameter does not appear to be needed here.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1364 at r4 (raw file):

    const envoy_api_v2_route_RouteMatch* match =
        envoy_api_v2_route_Route_match(route);
    const google_protobuf_BoolValue* case_sensitive =

I think we should move this check down to the end of this block, because we should not reject the config if case_sensitive is set on a route that we're going to be ignoring anyway.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1375 at r4 (raw file):

        match, &query_parameters_size));
    if (query_parameters_size > 0) {
      // Query parameters are never used, so we ignore this route.

We need a test for this case.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2044 at r4 (raw file):

            "              \"range_end\":%d\n"
            "             }",
            "range_match", header.range_start, header.range_end);

"range_match" does not need to be a separate parameter; it should be moved directly into the format string.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2114 at r4 (raw file):

      path_match_str, absl::StrJoin(headers_service_config, ""),
      route.matchers.fraction_per_million.has_value()
          ? absl::StrFormat("\"fraction_numerator\":%d,\n",

As per the gRFC, this field is supposed to be called "match_fraction".

test/cpp/end2end/xds_end2end_test.cc, line 2400 at r4 (raw file):

      balancers_[0]->ads_service()->default_route_config();
  auto* route1 = route_config.mutable_virtual_hosts(0)->mutable_routes(0);
  route1->mutable_match()->set_prefix("/blah");

This statement isn't needed. We can leave the prefix as-is for this test.

test/cpp/end2end/xds_end2end_test.cc, line 2436 at r4 (raw file):

// Tests that LDS client should send a NACK if the single route match has a
// prefix string with no "/".
TEST_P(LdsRdsTest, RouteMatchHasInvalidPrefixNonEmptyNoSlash) {

This test looks functionally equivalent to the next one. I think it can be removed.

test/cpp/end2end/xds_end2end_test.cc, line 2470 at r4 (raw file):

}

// Tests that LDS client should send a NACK if route match has a prefix

This route should be ignored rather than causing the config to be rejected.

test/cpp/end2end/xds_end2end_test.cc, line 2491 at r4 (raw file):

// Tests that LDS client should send a NACK if route match has a prefix
// string "//".
TEST_P(LdsRdsTest, RouteMatchHasInvalidPrefixNoContent) {

s/NoContent/DoubleSlash/

test/cpp/end2end/xds_end2end_test.cc, line 2508 at r4 (raw file):

}

// Tests that LDS client should send a NACK if the single route match has path

Instead of saying "the single" here, let's explicitly note that what we're testing is that we ignore routes with these properties. The fact that the test has a single route is because that's the easiest way to ensure that we ignore the route, not because we care specifically about that particular test case.

Same thing for all of these tests.

test/cpp/end2end/xds_end2end_test.cc, line 2546 at r4 (raw file):

// Tests that LDS client should send a NACK if the single route match has path
// string that ends with "/".
TEST_P(LdsRdsTest, RouteMatchHasInvalidPathEndsWithSlash) {

s/EndsWithSlash/TooManySlashes/

test/cpp/end2end/xds_end2end_test.cc, line 2564 at r4 (raw file):

// Tests that LDS client should send a NACK if the single route match has path
// string that misses "/" between service and method.
TEST_P(LdsRdsTest, RouteMatchHasInvalidPathMissingMiddleSlash) {

s/MissingMiddleSlash/OnlyOneSlash/

test/cpp/end2end/xds_end2end_test.cc, line 3377 at r4 (raw file):

  route1->mutable_match()->set_prefix("/grpc.testing.EchoTest1Service/");
  route1->mutable_match()
      ->mutable_runtime_fraction()

Why are we setting runtime_fraction in a test about header matching? We should have a separate test for this matcher.

test/cpp/end2end/xds_end2end_test.cc, line 3403 at r4 (raw file):

  default_route->mutable_match()->set_prefix("");
  default_route->mutable_match()
      ->mutable_runtime_fraction()

Same question here.

Addressed code review comments; added tests; temporarily patched over re2 import work that would just get bazel working so that I could fully implement regex and add regex tests as well.

Really trying to finish all the implementation so that we are all ready to go as soon as re2 is successfully imported

Reviewable status: 0 of 15 files reviewed, 43 unresolved discussions (waiting on @apolcyn, @gnossen, @jtattermusch, @markdroth, @nicolasnoble, and @veblush)

BUILD, line 1448 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 867 at r2 (raw file):

ok fixed. instead of continue we will check for other types (just like path). if another one is found: even if there are errors in them we will consider it found and won't let another one

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 220 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 220 at r4 (raw file):

using absl::optional

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 221 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 240 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 247 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 264 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 270 at r4 (raw file):

if (header_matcher.invert_match) match = !match;
if (!match) return false;
cool! easier to undertand. I am calling HeaderMatches HeaderMatchHelper so that it looks more different from HeadersMatch

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 284 at r4 (raw file):

right! :)

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 335 at r4 (raw file):

unfortunately i cna't use this , not yet anyway :(

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 336 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 478 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 979 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 1067 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 119 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 163 at r4 (raw file):

updated to look like :
Path prefix match://grpc.testing.EchoTest1Service//
Header exact match: /header1:POST/
Header regex match: /header2:[a-z]{1}/
Header range match: /header3:[1, 1000]/
Header present match: /header4:false
Header prefix Match: /header5:/grpc/
Header suffix match: not /header6:.pdf/

src/core/ext/filters/client_channel/xds/xds_api.cc, line 169 at r4 (raw file):

removed

src/core/ext/filters/client_channel/xds/xds_api.cc, line 170 at r4 (raw file):

removed

src/core/ext/filters/client_channel/xds/xds_api.cc, line 217 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 223 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1021 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1037 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1040 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1057 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1080 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1097 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1128 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1160 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1364 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1375 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2044 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2114 at r4 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2400 at r4 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2436 at r4 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2470 at r4 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2491 at r4 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2508 at r4 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2546 at r4 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2564 at r4 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3377 at r4 (raw file):

separated out a test

test/cpp/end2end/xds_end2end_test.cc, line 3403 at r4 (raw file):

question here is should default have a fraction? which route would the rpcs take if they are not within the fraction?

This is looking really good!

Most of my comments are minor. The only really substantive issue is the need to store the compiled regex instead of recompiling it every time we use it.

Please let me know if you have any questions. Thanks!

Reviewed 1 of 7 files at r5, 14 of 25 files at r7.
Reviewable status: all files reviewed, 34 unresolved discussions (waiting on @apolcyn, @donnadionne, @gnossen, @jtattermusch, @nicolasnoble, and @veblush)

.gitmodules, line 52 at r7 (raw file):

	path = third_party/libuv
	url = https://github.com/libuv/libuv.git
[submodule "re2"]

This looks like a duplicate of the one added above, and the path doesn't seem right. I suspect this should just be removed.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 227 at r7 (raw file):

  // build:
  //*(args.initial_metadata) is returning values not references.
  GPR_DEBUG_ASSERT(initial_metadata);

!= nullptr

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 240 at r7 (raw file):

    const XdsApi::RdsUpdate::RdsRoute::Matchers::PathMatcher& path_matcher) {
  if (path_matcher.path_matcher.empty()) return true;
  RE2 regex(path_matcher.path_matcher);

Doing this here means that we're compiling the regexp every time we evaluate it, which is going to be very bad for performance.

Instead, I suggest that we add an RE2 object to the XdsApi::RdsUpdate::RdsRoute::Matchers::PathMatcher struct. In other words, when path_type is REGEX, instead of storing the pattern as a string in the path_matcher field, we will store the compiled regex in an RE regex field.

Note that you can get the original pattern string back from the RE2 object via the pattern() method, so we can still get the pattern string when logging the configuration.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 271 at r7 (raw file):

    }
  }
  RE2 regex(header_matcher.header_matcher);

Same comment as above. We definitely do not want to compile the regex every time we evaluate it.

Instead, let's add an RE2 field to the XdsApi::RdsUpdate::RdsRoute::Matchers::HeaderMatcher struct, and when header_type is REGEX, we can store the compiled regex in the new field instead of using the existing header_matcher string field.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 286 at r7 (raw file):

      }
      return int_value >= header_matcher.range_start &&
             int_value <= header_matcher.range_end;

Integer ranges are exclusive of range_end. In other words, the <= should be just <.

https://github.com/envoyproxy/envoy/blob/dcf34972d1bc15324835c40dfd7a780e8fc69d72/api/envoy/type/range.proto#L14

Please make sure we have a test covering this edge case.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 833 at r7 (raw file):

    // Parse and ensure one and only one path matcher is set: prefix, path, or
    // regex.
    size_t path_matcher_count = 0;

We never increment this more than once, so we could just replace it with bool patch_matcher_seen = false.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 924 at r7 (raw file):

            // Parse and ensure one and only one header matcher is set per
            // header matcher.
            size_t header_matcher_count = 0;

Same as above: we never increment this more than once, so we could replace it with bool header_matcher_seen = false.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 931 at r7 (raw file):

                    "field:exact_match error: should be string"));
              } else {
                ++header_matcher_count;

Please move up to line 926, because we should do this even if the field is the wrong type.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 951 at r7 (raw file):

                      "field:regex_match error: should be string"));
                } else {
                  ++header_matcher_count;

Already doing this a few lines above, so this can be removed.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 163 at r4 (raw file):

The slashes are really confusing here. They make paths that include slashes hard to read, and they also imply regex matching even when that's not what's happening. Let's just remove them. I think the following would be much more readable:

Path prefix match:/grpc.testing.EchoTest1Service/
Header exact match: header1:POST
Header regex match: header2:[a-z]{1}
Header range match: header3:[1, 1000)
Header present match: header4:false
Header prefix match: header5:/grpc
Header suffix match: not header6:.pdf

src/core/ext/filters/client_channel/xds/xds_api.cc, line 153 at r7 (raw file):

  switch (path_type) {
    case PathMatcherType::PATH:
      path_type_string = "patch match";

s/patch/path/

src/core/ext/filters/client_channel/xds/xds_api.cc, line 177 at r7 (raw file):

                             invert_match ? " not" : "", name, header_matcher);
    case HeaderMatcherType::RANGE:
      return absl::StrFormat("Header range match:%s /%s:[%d, %d]/",

The range notation should use [ at the start and ) at the end, to indicate that the range is inclusive of the start value and exclusive of the end value.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 185 at r7 (raw file):

                             present_match ? "true" : "false");
    case HeaderMatcherType::PREFIX:
      return absl::StrFormat("Header prefix Match:%s /%s:%s/",

s/Match/match/ for consistency with other messages.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1112 at r7 (raw file):

          envoy_api_v2_route_HeaderMatcher_exact_match(header));
    } else if (envoy_api_v2_route_HeaderMatcher_has_safe_regex_match(header)) {
      header_matcher.header_type = XdsApi::RdsUpdate::RdsRoute::Matchers::

Suggest moving this statement down to the end of the block, since there's no point in executing it if we're going to fail.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1124 at r7 (raw file):

            "Invalid regex string specified.");
      }
      header_matcher.header_matcher = matcher;

This can use std::move().

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1134 at r7 (raw file):

      if (header_matcher.range_end < header_matcher.range_start) {
        return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
            "Invalid range header matcher specifier specified: range_end "

s/range_end/end/

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1135 at r7 (raw file):

        return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
            "Invalid range header matcher specifier specified: range_end "
            "cannot be smaller than range_start.");

s/range_start/start/

test/cpp/end2end/xds_end2end_test.cc, line 2400 at r4 (raw file):

Why not just remove this statement completely? It doesn't matter what the prefix is for the purpose of this test, so we can leave it as "/", which is the default.

test/cpp/end2end/xds_end2end_test.cc, line 3403 at r4 (raw file):

With runtime_fraction, we basically roll the dice, and if the result is less than the specified fraction, the match is true; otherwise the match is false, in which case we continue on to the next rule to see if that one matches. The default rule does not need a fraction, because it will always match.

test/cpp/end2end/xds_end2end_test.cc, line 2796 at r7 (raw file):

  auto* header_matcher1 = route1->mutable_match()->add_headers();
  header_matcher1->set_name("header1");
  header_matcher1->mutable_safe_regex_match()->set_regex("a[z-a]");

This doesn't seem like an invalid regex. Why is this failing to compile?

test/cpp/end2end/xds_end2end_test.cc, line 3494 at r7 (raw file):

  default_route->mutable_route()->set_cluster(kDefaultResourceName);
  SetRouteConfiguration(0, route_config);
  std::vector<std::pair<std::string, std::string>> metadata;

Can write this as:

std::vector<std::pair<std::string, std::string>> metadata = {
  {"header1", "POST"},
  {"header2", "blah"},
  {"header3", "5"},
  {"header5", "/grpc.testing.EchoTest1Service/"},
  {"header6", "grpc.java"},
};

test/cpp/end2end/xds_end2end_test.cc, line 3502 at r7 (raw file):

  metadata.push_back(std::make_pair("header6", "grpc.java"));
  CheckRpcSendOk(kNumEchoRpcs,
                 RpcOptions().set_wait_for_ready(true).set_metadata(metadata));

Why are we passing the metadata here? These RPCs won't match the path, so it shouldn't matter.

test/cpp/end2end/xds_end2end_test.cc, line 3502 at r7 (raw file):

  metadata.push_back(std::make_pair("header6", "grpc.java"));
  CheckRpcSendOk(kNumEchoRpcs,
                 RpcOptions().set_wait_for_ready(true).set_metadata(metadata));

I don't think wait_for_ready buys us anything here. Instead, I think we should use this:

const auto header_match_rpc_options = RpcOptions()
    .set_rpc_service(SERVICE_ECHO1)
    .set_rpc_method(METHOD_ECHO1)
    .set_metadata(std::move(metadata));
// Make sure all backends are up.
WaitForAllBackends(0, 1);
WaitForAllBackends(1, 2, true, header_match_rpc_options);
// Send RPCs.
CheckRpcSendOk(kNumEchoRpcs);
CheckRpcSendOk(kNumEcho1Rpcs, header_match_rpc_options);

test/cpp/end2end/xds_end2end_test.cc, line 3506 at r7 (raw file):

                                    .set_rpc_service(SERVICE_ECHO1)
                                    .set_rpc_method(METHOD_ECHO1)
                                    .set_metadata(metadata)

std::move()

test/cpp/end2end/xds_end2end_test.cc, line 3519 at r7 (raw file):

}

TEST_P(LdsRdsTest, XdsRoutingHeadersMatchingWithRuntimeFraction) {

Let's just call this XdsRoutingRuntimeFractionMatching. This test doesn't actually have anything to do with header matching.

test/cpp/end2end/xds_end2end_test.cc, line 3522 at r7 (raw file):

  gpr_setenv("GRPC_XDS_EXPERIMENTAL_ROUTING", "true");
  const char* kNewCluster1Name = "new_cluster_1";
  const size_t kNumEcho1Rpcs = 1000;

Both of these can be combined into one kNumRpcs.

test/cpp/end2end/xds_end2end_test.cc, line 3545 at r7 (raw file):

      balancers_[0]->ads_service()->default_route_config();
  auto* route1 = route_config.mutable_virtual_hosts(0)->mutable_routes(0);
  route1->mutable_match()->set_prefix("/grpc.testing.EchoTest1Service/");

No need for a different prefix here. Both routes can use the same prefix of "".

test/cpp/end2end/xds_end2end_test.cc, line 3549 at r7 (raw file):

      ->mutable_runtime_fraction()
      ->mutable_default_value()
      ->set_numerator(50);

Let's set this to 25 instead of 50, so that we can verify that it's not just being handled as a round-robin between the two backends.

test/cpp/end2end/xds_end2end_test.cc, line 3550 at r7 (raw file):

      ->mutable_default_value()
      ->set_numerator(50);
  auto* header_matcher1 = route1->mutable_match()->add_headers();

I don't think we need any matcher here other than the runtime_fraction matcher.

test/cpp/end2end/xds_end2end_test.cc, line 3558 at r7 (raw file):

  default_route->mutable_route()->set_cluster(kDefaultResourceName);
  SetRouteConfiguration(0, route_config);
  std::vector<std::pair<std::string, std::string>> metadata;

This should not be needed.

test/cpp/end2end/xds_end2end_test.cc, line 3561 at r7 (raw file):

  metadata.push_back(std::make_pair("header1", "POST"));
  CheckRpcSendOk(kNumEchoRpcs,
                 RpcOptions().set_wait_for_ready(true).set_metadata(metadata));

I don't think wait_for_ready helps here. I think we need to do this:

WaitForAllBackends(0, 2);
CheckRpcSendOk(kNumRpcs);

test/cpp/end2end/xds_end2end_test.cc, line 3567 at r7 (raw file):

                                    .set_metadata(metadata)
                                    .set_wait_for_ready(true));
  EXPECT_EQ(kNumEchoRpcs, backends_[0]->backend_service()->request_count());

Here we can check that backend 0 received 75% of the traffic and backend 1 received 25%.

test/cpp/end2end/xds_end2end_test.cc, line 3657 at r7 (raw file):

  default_route->mutable_route()->set_cluster(kDefaultResourceName);
  SetRouteConfiguration(0, route_config);
  std::vector<std::pair<std::string, std::string>> metadata;

std::vector<std::pair<std::string, std::string>> metadata = {
  {"header1", "POST1"},
  {"header2", "1001"},
  {"header3", "blah1"},
};

Addressed all the comments, ready for another round of review including the RE2 code.

Once this PR is free of comments, I will take out the RE2 code and submit the rest.

RE2 will be submitted with the import PR as per suggestion in that PR.

Reviewable status: 7 of 15 files reviewed, 32 unresolved discussions (waiting on @apolcyn, @gnossen, @jtattermusch, @markdroth, @nicolasnoble, and @veblush)

.gitmodules, line 52 at r7 (raw file):

I'll fix it with the import PR; i think I need this one and can delete the one above

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 227 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 240 at r7 (raw file):

updated according to suggestion. The only difference is because RE2 does not have default constructor, copy, etc...; storing it as a object is a problem , so using a unique ptr instead

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 271 at r7 (raw file):

done; doing the compiling at parsing time and then storing. In xds_api.cc i did do a compile check but didn't bother store as we have to pass it as a string in service config anyway

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 286 at r7 (raw file):

done; and updated tests: range is [1..1000) and using value 1 for match and 1000 for unmatch

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 833 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 924 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 931 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 951 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 163 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 153 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 177 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 185 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1112 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1124 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1134 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 1135 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2400 at r4 (raw file):

done

test/cpp/end2end/xds_end2end_test.cc, line 3403 at r4 (raw file):

should we prevent the default rule to have a fraction configured?

test/cpp/end2end/xds_end2end_test.cc, line 2796 at r7 (raw file):

it's z-a as oppose to a-z; i took it from the re2 reject test list https://github.com/google/re2/blob/master/re2/testing/re2_test.cc#L1161

test/cpp/end2end/xds_end2end_test.cc, line 3494 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3502 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3502 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3506 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3519 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3522 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3545 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3549 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3550 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3558 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3561 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3567 at r7 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3657 at r7 (raw file):

Done.

This looks really good! The only remaining issues are related to regex handling, which should probably be moved to the other PR anyway.

Please let me know if you have any questions.

Reviewed 4 of 18 files at r8, 1 of 3 files at r9, 2 of 3 files at r10, 1 of 1 files at r11.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @apolcyn, @donnadionne, @gnossen, @jtattermusch, @nicolasnoble, and @veblush)

.gitmodules, line 52 at r7 (raw file):

I'm pretty sure you've got this backwards. We want the submodule to be in ./third_party/re2, not ./re2.

BUILD, line 1302 at r11 (raw file):

    ],
    language = "c++",
    external_deps = [

Since you've moved these dependencies here, I think you can remove them from the grpc_xds_client and grpc_xds_client_secure targets.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 249 at r11 (raw file):

    case XdsApi::RdsUpdate::RdsRoute::Matchers::PathMatcher::PathMatcherType::
        REGEX:
      return RE2::FullMatch(path.data(), *(path_matcher.regex_path_matcher));

No need for the parens after the * here.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 277 at r11 (raw file):

        HeaderMatcherType::REGEX:
      return RE2::FullMatch(value.value().data(),
                            *(header_matcher.regex_match));

No need for parens here.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 875 at r11 (raw file):

          route->matchers.path_matcher.path_type = XdsApi::RdsUpdate::RdsRoute::
              Matchers::PathMatcher::PathMatcherType::REGEX;
          route->matchers.path_matcher.path_matcher = it->second.string_value();

We should not set path_matcher in this case. We are already storing the compiled regex, so we don't need to store the pattern as a separate string.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 955 at r11 (raw file):

                  header_matcher.header_type = XdsApi::RdsUpdate::RdsRoute::
                      Matchers::HeaderMatcher::HeaderMatcherType::REGEX;
                  header_matcher.header_matcher =

No need to set header_matcher in this case. We're already storing the compiled regex, so we don't need to store it as a separte string.

src/core/ext/filters/client_channel/xds/xds_api.h, line 59 at r11 (raw file):

        struct PathMatcher {
          enum class PathMatcherType {
            PATH,

Please add a comment next to each of these saying which field below the data is stored in. For REGEX, that's regex_path_matcher, and for the other two, it's path_matcher.

src/core/ext/filters/client_channel/xds/xds_api.h, line 74 at r11 (raw file):

        struct HeaderMatcher {
          enum class HeaderMatcherType {
            EXACT,

Same as above: Please add a comment next to each of these saying which field(s) below the data is stored in.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 635 at r11 (raw file):

    rds_route->matchers.path_matcher.path_type = XdsApi::RdsUpdate::RdsRoute::
        Matchers::PathMatcher::PathMatcherType::REGEX;
    rds_route->matchers.path_matcher.path_matcher = std::move(matcher);

We should store the compiled regex here, not the string.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 662 at r11 (raw file):

          envoy_api_v2_route_HeaderMatcher_safe_regex_match(header);
      GPR_ASSERT(regex_matcher != nullptr);
      header_matcher.header_type = XdsApi::RdsUpdate::RdsRoute::Matchers::

Please move this down to line 672.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 671 at r11 (raw file):

            "Invalid regex string specified.");
      }
      header_matcher.header_matcher = std::move(matcher);

We should store the compiled regex here, not the string.

test/cpp/end2end/xds_end2end_test.cc, line 3403 at r4 (raw file):

Are you asking if we should change the XdsApi code to reject a config where the last route specifies a RuntimeFraction matcher? If so, the answer is no. When xds routing is enabled, it's perfectly legal for the last route to have whatever matchers it wants, and it's perfectly legal for a request to not match any of the routes in the list, in which case we fail it.

test/cpp/end2end/xds_end2end_test.cc, line 2796 at r7 (raw file):

Ah! Okay. Sorry, I didn't spot that when I first read it.

I fixed all the comments; saved those changes in another branch.

I then removed all re2 stuff

So this PR is almost ready for submission ! :)

Let me know if you have any further comments, I'll fix them when i get back!

Thanks!

Reviewable status: 13 of 15 files reviewed, 12 unresolved discussions (waiting on @apolcyn, @gnossen, @jtattermusch, @markdroth, @nicolasnoble, and @veblush)

.gitmodules, line 52 at r7 (raw file):

you are right, it's the other one, my other PR has it correct. This will be reverted for the first submission (without RE2)

BUILD, line 1302 at r11 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 249 at r11 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 277 at r11 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 875 at r11 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 955 at r11 (raw file):

Done. and removed the setting of path_matcher for REGEX case too

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 899 at r12 (raw file):

                "value should be of type object"));
          } else {
            XdsApi::RdsUpdate::RdsRoute::Matchers::HeaderMatcher header_matcher;

even tho only one type of header will fill in header_matcher, i still had to make a copy before putting it onto the header list, as oppose to use std::move()

This is because sanity check thinks that we have more cases to execute after and we are using the same header_matcher after the std::move()

src/core/ext/filters/client_channel/xds/xds_api.h, line 59 at r11 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 74 at r11 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 635 at r11 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 662 at r11 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 671 at r11 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3403 at r4 (raw file):

i see thanks

This looks really good!

There are still a bunch of lingering traces of the regex code, and I think we should move all of that to the next PR. Otherwise, all of my comments are fairly cosmetic.

Please let me know if you have any questions.

Reviewed 1 of 2 files at r12, 7 of 12 files at r13.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @apolcyn, @donnadionne, @gnossen, @jtattermusch, @nicolasnoble, and @veblush)

BUILD, line 1299 at r13 (raw file):

    name = "grpc_xds_api_header",
    hdrs = [
        "src/core/ext/filters/client_channel/xds/xds_api.h",

Can you also move the .cc files for all of these modules into this target? That way, we won't have to duplicate them in both the grpc_xds_client and grpc_xds_client_secure targets.

If that works, then I suggest renaming this target to just grpc_xds_api.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 899 at r12 (raw file):

Another option would be to add the element to the array before we start, and then just update it in place, so that there's no need to push it onto the array later. In other words, we can do something like this:

route->matchers.header_matchers.emplace_back();
XdsApi::RdsUpdate::RdsRoute::Matchers::HeaderMatcher& header_matcher =
    route->matchers.header_matchers.back();

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 246 at r13 (raw file):

        PATH:
      return path == path_matcher.path_matcher;
    case XdsApi::RdsUpdate::RdsRoute::Matchers::PathMatcher::PathMatcherType::

Let's remove this case. We can add it in the next PR.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 273 at r13 (raw file):

        HeaderMatcherType::EXACT:
      return value.value() == header_matcher.header_matcher;
    case XdsApi::RdsUpdate::RdsRoute::Matchers::HeaderMatcher::

Let's remove this case. We can add it in the next PR.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 860 at r13 (raw file):

      }
    }
    it = json.object_value().find("regex");

Let's remove the code for handling the regex field. We can add this in the next PR.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 935 at r13 (raw file):

              }
            }
            header_it = header_json.object_value().find("regex_match");

Let's remove the code for the regex_match field. We can add this in the next PR.

src/core/ext/filters/client_channel/xds/xds_api.h, line 59 at r11 (raw file):

Please add the comments next to the enum elements instead of adding it separately below. In other words:

enum class PathMatcherType {
  PATH,  // path stored in path_matcher field
  PREFIX,  // prefix stored in path_matcher field
};

src/core/ext/filters/client_channel/xds/xds_api.h, line 74 at r11 (raw file):

Same as above, comments should be inline:

enum class HeaderMatcherType {
  EXACT,  // value stored in header_matcher field
  RANGE,  // uses range_start and range_end fields
  PRESENT,  // uses present_match field
  PREFIX,  // prefix stored in header_matcher field
  SUFFIX,  // suffix stored in header_matcher field
};

src/core/ext/filters/client_channel/xds/xds_api.h, line 60 at r13 (raw file):

            PATH,
            PREFIX,
            REGEX,

Let's remove this enum value for now. We can add it in as part of the next PR.

src/core/ext/filters/client_channel/xds/xds_api.h, line 62 at r13 (raw file):

            REGEX,
          };
          PathMatcherType path_type;

Suggest renaming this to just type.

src/core/ext/filters/client_channel/xds/xds_api.h, line 64 at r13 (raw file):

          PathMatcherType path_type;
          // path_matcher field is used when PathMatcherType is PATH or PREFIX.
          std::string path_matcher;

Suggest renaming this to string_matcher.

src/core/ext/filters/client_channel/xds/xds_api.h, line 74 at r13 (raw file):

          enum class HeaderMatcherType {
            EXACT,
            REGEX,

Let's remove this enum value for now. We can add it in the next PR.

src/core/ext/filters/client_channel/xds/xds_api.h, line 81 at r13 (raw file):

          };
          std::string name;
          HeaderMatcherType header_type;

Suggest renaming this to just type.

src/core/ext/filters/client_channel/xds/xds_api.h, line 88 at r13 (raw file):

          // header_matcher field is used when HeaderMatcherType is EXACT,
          // PREFIX, or SUFFIX.
          std::string header_matcher;

Suggest renaming this to string_matcher.

src/core/ext/filters/client_channel/xds/xds_api.h, line 91 at r13 (raw file):

          // present_match field is used when HeaderMatcherType is PRESENT.
          bool present_match;
          // invert_match field may or may not exisit, so initialize it to 0.

s/0/false/

src/core/ext/filters/client_channel/xds/xds_api.cc, line 183 at r13 (raw file):

      return absl::StrFormat("Header exact match:%s %s:%s",
                             invert_match ? " not" : "", name, header_matcher);
    case HeaderMatcherType::REGEX:

Let's just remove this case. We can add it in the next PR.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 621 at r13 (raw file):

        Matchers::PathMatcher::PathMatcherType::PATH;
    rds_route->matchers.path_matcher.path_matcher = UpbStringToStdString(path);
  } else if (envoy_api_v2_route_RouteMatch_has_safe_regex(match)) {

Let's remove this block. We can add it in the next PR.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 651 at r13 (raw file):

      header_matcher.header_matcher = UpbStringToStdString(
          envoy_api_v2_route_HeaderMatcher_exact_match(header));
    } else if (envoy_api_v2_route_HeaderMatcher_has_safe_regex_match(header)) {

Let's remove this block. We can add it in the next PR.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2033 at r13 (raw file):

        break;
      case XdsApi::RdsUpdate::RdsRoute::Matchers::HeaderMatcher::
          HeaderMatcherType::REGEX:

Let's remove this case. We can add it in the next PR.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2099 at r13 (raw file):

      break;
    case XdsApi::RdsUpdate::RdsRoute::Matchers::PathMatcher::PathMatcherType::
        REGEX:

Let's remove this case. We can add it in the next PR.

Reviewable status: 6 of 9 files reviewed, 19 unresolved discussions (waiting on @apolcyn, @gnossen, @jtattermusch, @markdroth, @nicolasnoble, and @veblush)

BUILD, line 1299 at r13 (raw file):

xds_api.cc needs grpc_client_channel
xds_client_stats.cc needs xds_client.h

the only one that can be easily moved over the xds_bootstrap.cc

not sure if that is necessary?

i did remoce unnecessary .h from grpc_xds_client_secure

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 246 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 273 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 860 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 935 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 60 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 62 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 64 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 74 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 81 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 88 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 91 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 183 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 621 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.cc, line 651 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2033 at r13 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 2099 at r13 (raw file):

Done.

This looks really good! Just one minor issue remaining. Feel free to squash commits and merge after addressing.

Reviewed 1 of 3 files at r14, 6 of 6 files at r15.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @apolcyn, @donnadionne, @gnossen, @jtattermusch, @nicolasnoble, and @veblush)

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 238 at r15 (raw file):

    const absl::string_view& path,
    const XdsApi::RdsUpdate::RdsRoute::Matchers::PathMatcher& path_matcher) {
  if (path_matcher.string_matcher.empty()) return true;

This line seems wrong. Why would we assume a match if the string field is empty? Shouldn't we just let it go through to the matching code below?

For PREFIX match, an empty string should match anyway. But for a PATH match, the empty string should not match.

I think this line should just be removed.

src/core/ext/filters/client_channel/xds/xds_api.h, line 82 at r15 (raw file):

          int64_t range_end;
          std::string string_matcher;
          // present_match field is used when HeaderMatcherType is PRESENT.

This comment isn't needed; it's covered above.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @apolcyn, @gnossen, @jtattermusch, @markdroth, @nicolasnoble, and @veblush)

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 238 at r15 (raw file):

Done.

src/core/ext/filters/client_channel/xds/xds_api.h, line 82 at r15 (raw file):

Done.