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

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

BUILD, line 1466 at r1 (raw file):

        "grpc_client_channel",
        "grpc_xds_api_header",
        "grpc_http_filters",

Instead of having this depend on the grpc_http_filters target, how about moving the user_agent_string_from_args() function into its own library, which can be a dependency of both grpc_lb_policy_xds_routing and grpc_http_filters?

src/core/ext/filters/client_channel/lb_policy.h, line 197 at r1 (raw file):

    /// The user agent string.  Constructed using primary and secondary user
    /// agent string.
    std::string user_agent;

I don't think this is something that we want to expose to every LB policy. This functionality should be wholly contained within the xds_routing policy.

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

  RouteTable route_table_;
  std::string
      user_agent_;  // Storing user_agent generated from args from http layer.

This should not be stored in the config object, because it's not something that will change when the config changes. Instead, it should be stored as a data member of XdsRoutingLb. We can compute the string exactly once when the xds_routing policy is instantiated, and then we never need to change it again.

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

  void ExitIdleLocked() override;
  void ResetBackoffLocked() override;
  void SetUserAgent(std::string user_agent) {

This is not necessary.

(Even if we did want to store the user agent string in the config object, this method would not be necessary. This is being called only from UpdateLocked(), which is already a member of XdsRoutingLb, so it can just access config_ directly.)

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

    using RouteTable = std::vector<Route>;

    explicit RoutePicker(RouteTable route_table,

This should take the user agent string as an additional argument. The string can be stored as a data member and then passed to HeaderMatchHelper() when doing a pick.

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

    using RouteTable = std::vector<Route>;

    explicit RoutePicker(RouteTable route_table,

Not a new issue in this PR, but this does not to be explicit, since there's already more than one argument.

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

bool HeaderMatchHelper(
    LoadBalancingPolicy::PickArgs args,

Instead of passing in PickArgs, we should continue to pass in just the MetadataInterface, and we should add a separate parameter for the user agent string.

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

      header_matcher.name == "grpc-previous-rpc-attempts") {
    value = absl::nullopt;
  } else if (header_matcher.name == "path") {

This case can be removed. We don't want to support the ":path" header (which was already addressed in #23417), and we definitely don't want to add a separate "path" header.

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

bool HeadersMatch(
    LoadBalancingPolicy::PickArgs args,

Not a new problem with this PR, but this should not take PickArgs either; it should just take the MetadataInterface as a parameter.

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

XdsRoutingLb::PickResult XdsRoutingLb::RoutePicker::Pick(PickArgs args) {
  // Set user_agent string to the stored string constrcuted from args.
  args.user_agent = config_->UserAgent();

Instead of storing the user agent string in PickArgs, which pollutes the LB policy API, we should just pass the string into HeadersMatch() as a separate argument.

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

    if (route.matchers->fraction_per_million.has_value() &&
        !UnderFraction(route.matchers->fraction_per_million.value()))
      continue;

Not directly related to this PR, but please put braces around this to avoid merge problems.

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

  // Store user agent string constructed from args.
  xds_routing_policy_->SetUserAgent(
      user_agent_string_from_args(args, "chttp2"));

This should not be called here in UpdateLocked(). Instead, it should be called just once in the XdsRoutingLb ctor.

src/core/ext/filters/http/client/http_client_filter.h, line 31 at r1 (raw file):

#define GRPC_ARG_MAX_PAYLOAD_SIZE_FOR_GET "grpc.max_payload_size_for_get"

std::string user_agent_string_from_args(const grpc_channel_args* args,

We shouldn't have any exported symbols in the global namespace that do not start with grpc_.

In this case, the right thing to do is to move this into the grpc_core namespace.

Please change the function name to a C++-style name at the same time.

test/cpp/end2end/xds_end2end_test.cc, line 3645 at r1 (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/");

Using a separate prefix here doesn't actually add anything to this test. I think we only need two routes, both with the default prefix. The first one should have the non-matching content type, and the second one should have the matching content type. Then we can just send one set of RPCs and verify that they all use the second route.

Same thing for the user-agent test.

test/cpp/end2end/xds_end2end_test.cc, line 3728 at r1 (raw file):

  header_matcher2->set_name("user-agent");
  header_matcher2->set_exact_match(
      "grpc-c++/1.31.0-dev grpc-c/11.0.0 (linux; chttp2)");

This test will pass only on Linux. When the test runs on Windows or Mac, the string will be different.

fixed code review comments; and added grpc-timeout; I have some questions inline below.

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

BUILD, line 1466 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy.h, line 197 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy.h, line 196 at r2 (raw file):

    absl::string_view path;
    /// The grpc deadline.
    grpc_millis deadline;

I removed user_agent, but added deadline here. As deadline is from grpc_metadata_batch which is accessible in PickSubchannelLocked and can be stored here. Let me know if this is ok?

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

Done.

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

Done.

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

Done.

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

Done.

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

Done.

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

Done.

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

Done.

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

Done.

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

Done.

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

Done.

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

    : user_agent_(GenerateUserAgentFromArgs(args.args, "chttp2")),
      LoadBalancingPolicy(std::move(args)) {}

I think I have a small problem here: LoadBalancingPolicy will get initialized first and args is moved. So when I initialize user_agent_ , accessing args.args is not safe right?

I tried not using the move and make a copy, but args does not have copy constructor . Not sure how best to solve this.

src/core/ext/filters/http/client/http_client_filter.h, line 31 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3645 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3728 at r1 (raw file):

using prefix instead

test/cpp/end2end/xds_end2end_test.cc, line 3714 at r2 (raw file):

}

TEST_P(LdsRdsTest, XdsRoutingHeadersMatchingSpecialHeaderGrpcTimeout) {

I wrote this test, even tho I wasn't sure how the header for grpc-timeout will be: will it be of type range like I have in the test? and if the timeout calculated based on deadline falls in the range, we consider it a match?

This looks much cleaner!

Remaining comments are mostly minor, except for merging in the changes from #23460 for handling deadline.

Please let me know if you have any questions.

Reviewed 5 of 7 files at r2, 17 of 17 files at r3.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @donnadionne)

src/core/ext/filters/client_channel/lb_policy.h, line 196 at r2 (raw file):

I'm not sure whether we want to make this part of the public LB policy API. Unlike the user-agent string, I could imagine this being useful to other policies at some point, but I'd still rather not add it until we know we need it, because it's always easier to add things than to remove them.

For now, let's do this in an xDS-specific way. We can make this available via the experimental call attributes interface, populated by a ConfigSelector returned by the xds resolver. Note that this is basically the same approach we're going to have to use for the timeout functionality, so this is a useful step in the right direction anyway.

I've sent you #23460 to implement the ConfigSelector and expose the deadline via a call attribute. Once you've merged those changes, you can access the deadline string from PickArgs like this:

args.call_state->ExperimentalGetCallAttribute(kCallAttributeDeadline);

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

The args.args field is just a pointer, so it's not affected by std::move(). It's fine to access it afterwards.

I do agree that this is a bit ugly. I've been meaning to make some changes to the LB policy API to improve this, but it hasn't bubbled up to the top of the to-do list.

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

bool HeaderMatchHelper(
    const std::string& user_agent, const grpc_millis deadline,

Let's put these arguments after the initial_metadata argument, since they're just other fields to be matched against.

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

    value = user_agent;
  } else if (header_matcher.name == "grpc-timeout") {
    value = absl::StrFormat("%ld", deadline - grpc_core::ExecCtx::Get()->Now());

The format actually needs to be different here. This should use grpc_http2_encode_timeout().

This issue will be moot once you merge in the changes from #23460.

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

bool HeadersMatch(
    const std::string& user_agent, const grpc_millis deadline,

Same as above: Please move these two arguments to the end.

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

XdsRoutingLb::PickResult XdsRoutingLb::RoutePicker::Pick(PickArgs args) {
  // Set user_agent string to the stored string constrcuted from args.

This comment can go away now.

src/core/ext/filters/http/client/http_client_filter.h, line 31 at r3 (raw file):

#define GRPC_ARG_MAX_PAYLOAD_SIZE_FOR_GET "grpc.max_payload_size_for_get"

std::string user_agent_string_from_args(const grpc_channel_args* args,

This can be removed now.

src/core/ext/filters/http/client/util.h, line 1 at r3 (raw file):

/*

Please use C++-style comments.

src/core/ext/filters/http/client/util.h, line 27 at r3 (raw file):

#include "src/core/lib/channel/channel_stack.h"

#define GRPC_ARG_MAX_PAYLOAD_SIZE_FOR_GET "grpc.max_payload_size_for_get"

This is not needed here.

src/core/ext/filters/http/client/util.cc, line 1 at r3 (raw file):

/*

Please use C++-style comments.

src/core/ext/filters/http/client/util.cc, line 2 at r3 (raw file):

/*
 * Copyright 2020 gRPC authors.

Please preserve original copyright year, since this code is really just being moved from another file.

test/cpp/end2end/xds_end2end_test.cc, line 3714 at r2 (raw file):

Given that the value is not strictly an integer (see grpc_http2_encode_timeout() -- it will have an "S" suffix), I don't think it can use a range match.

The best thing might be to do a regex match (e.g., to see if the timeout has enough digits).

test/cpp/end2end/xds_end2end_test.cc, line 3652 at r3 (raw file):

  CheckRpcSendOk(kNumEchoRpcs);
  EXPECT_EQ(kNumEchoRpcs, backends_[0]->backend_service()->request_count());
  EXPECT_EQ(0, backends_[0]->backend_service1()->request_count());

I don't think we need to check the counts for backend_service1 or backend_service2, since we're not sending any RPCs for those services.

test/cpp/end2end/xds_end2end_test.cc, line 3690 at r3 (raw file):

  auto* header_matcher1 = route1->mutable_match()->add_headers();
  header_matcher1->set_name("user-agent");
  header_matcher1->set_prefix_match("not-grpc-c++/1.31.0-dev grpc-c/11.0.0");

Hard-coding the versions here means that this test will break every time we do a new release. Maybe a regex match would be a better choice here?

test/cpp/end2end/xds_end2end_test.cc, line 3704 at r3 (raw file):

  CheckRpcSendOk(kNumEchoRpcs);
  EXPECT_EQ(kNumEchoRpcs, backends_[0]->backend_service()->request_count());
  EXPECT_EQ(0, backends_[0]->backend_service1()->request_count());

No need to check counts for backend_service1 or backend_service2.

test/cpp/end2end/xds_end2end_test.cc, line 3758 at r3 (raw file):

  CheckRpcSendOk(kNumEchoRpcs);
  EXPECT_EQ(kNumEchoRpcs, backends_[0]->backend_service()->request_count());
  EXPECT_EQ(0, backends_[0]->backend_service1()->request_count());

No need to check counts for backend_service1 or backend_service2.

Reviewable status: 12 of 23 files reviewed, 13 unresolved discussions (waiting on @markdroth)

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

Done.

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

Done.

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

Done.

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

Done.

src/core/ext/filters/http/client/http_client_filter.h, line 31 at r3 (raw file):

Done.

src/core/ext/filters/http/client/util.h, line 1 at r3 (raw file):

Done.

src/core/ext/filters/http/client/util.h, line 27 at r3 (raw file):

Done.

src/core/ext/filters/http/client/util.cc, line 1 at r3 (raw file):

Done.

src/core/ext/filters/http/client/util.cc, line 2 at r3 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3652 at r3 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3690 at r3 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3704 at r3 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3758 at r3 (raw file):

Done.

This looks great! Just a couple of minor comments remaining.

Reviewed 9 of 11 files at r4, 5 of 7 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @donnadionne)

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

    if (!HeadersMatch(route.matchers->header_matchers, args.initial_metadata,
                      user_agent_,
                      std::string(args.call_state->ExperimentalGetCallAttribute(

Let's change the deadline parameter on HeadersMatch() and HeaderMatchHelper() from const std::string& to absl::string_view. That way, we can avoid an unnecessary allocation here by converting to std::string.

test/cpp/end2end/xds_end2end_test.cc, line 3687 at r5 (raw file):

  header_matcher1->set_name("user-agent");
  // user-agent string is a 2-part string like "grpc-c++/1.31.0-dev
  // grpc-c/11.0.0"; not 3-part.

The discussion about 2-part vs 3-part is a little confusing. Why not just use a clearly non-matching regex like "does_not_match.*"?

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @markdroth)

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

Done.

test/cpp/end2end/xds_end2end_test.cc, line 3687 at r5 (raw file):

Done.