Fixing code review comments.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc

@@ -236,7 +236,7 @@ XdsRoutingLb::PickResult XdsRoutingLb::RoutePicker::Pick(PickArgs args) {
   result.error =
       grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
                              "xds routing picker: no matching route"),
-                         GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAVAILABLE);
+                         GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_INTERNAL);
   return result;
 }
 
@@ -610,7 +610,7 @@ class XdsRoutingLbFactory : public LoadBalancingPolicyFactory {
     std::vector<grpc_error*> error_list;
     // action map.
     XdsRoutingLbConfig::ActionMap action_map;
-    std::set<std::string /*action_name*/> action_in_use;
+    std::set<std::string /*action_name*/> action_to_be_used;
     auto it = json.object_value().find("actions");
     if (it == json.object_value().end()) {
       error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
@@ -634,13 +634,13 @@ class XdsRoutingLbFactory : public LoadBalancingPolicyFactory {
           error_list.push_back(error);
         } else {
           action_map[p.first] = std::move(child_config);
-          action_in_use.insert(p.first);
+          action_to_be_used.insert(p.first);
         }
       }
     }
-    if (action_map.size() == 0) {
+    if (action_map.empty()) {
       error_list.push_back(
-          GRPC_ERROR_CREATE_FROM_COPIED_STRING("no valid actions configured"));
+          GRPC_ERROR_CREATE_FROM_STATIC_STRING("no valid actions configured"));
     }
     XdsRoutingLbConfig::RouteTable route_table;
     it = json.object_value().find("routes");
@@ -654,8 +654,11 @@ class XdsRoutingLbFactory : public LoadBalancingPolicyFactory {
       const Json::Array& array = it->second.array_value();
       for (size_t i = 0; i < array.size(); ++i) {
         XdsRoutingLbConfig::Route route;
-        std::vector<grpc_error*> route_errors = ParseRoute(array[i], &route);
+        std::vector<grpc_error*> route_errors =
+            ParseRoute(array[i], action_map, &route, &action_to_be_used);
         if (!route_errors.empty()) {
+          // Can't use GRPC_ERROR_CREATE_FROM_VECTOR() here, because the error
+          // string is not static in this case.
           grpc_error* error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(
               absl::StrCat("field:routes element: ", i, " error").c_str());
           for (grpc_error* route_error : route_errors) {
@@ -663,30 +666,21 @@ class XdsRoutingLbFactory : public LoadBalancingPolicyFactory {
           }
           error_list.push_back(error);
         }
-        // Validate action exists and mark it as used.
-        if (action_map.find(route.action) == action_map.end()) {
-          grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-              absl::StrCat("field: routes element: ", i, " error: action ",
-                           route.action, " does not exist")
-                  .c_str());
-          error_list.push_back(error);
-        }
-        action_in_use.erase(route.action);
         route_table.emplace_back(std::move(route));
       }
     }
-    if (route_table.size() == 0) {
+    if (route_table.empty()) {
       grpc_error* error =
           GRPC_ERROR_CREATE_FROM_STATIC_STRING("no valid routes configured");
       error_list.push_back(error);
     }
-    if (!(route_table[route_table.size() - 1].matcher.service.empty() &&
-          route_table[route_table.size() - 1].matcher.method.empty())) {
+    if (!route_table.back().matcher.service.empty() ||
+        !route_table.back().matcher.method.empty()) {
       grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
           "default route must not contain service or method");
       error_list.push_back(error);
     }
-    if (!action_in_use.empty()) {
+    if (!action_to_be_used.empty()) {
       error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
           "some actions were not referenced by any route"));
     }
@@ -733,7 +727,7 @@ class XdsRoutingLbFactory : public LoadBalancingPolicyFactory {
     std::vector<grpc_error*> error_list;
     if (json.type() != Json::Type::OBJECT) {
       error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-          "field:methodName should be of type object"));
+          "value should be of type object"));
       return error_list;
     }
     // Parse service
@@ -758,47 +752,53 @@ class XdsRoutingLbFactory : public LoadBalancingPolicyFactory {
     }
     if (route_config->service.empty() && !route_config->method.empty()) {
       error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-          "field:methodName error: service is empty when method is "
-          "not"));
+          "service is empty when method is not"));
     }
     return error_list;
   }
 
-  static std::vector<grpc_error*> ParseRoute(const Json& json,
-                                             XdsRoutingLbConfig::Route* route) {
+  static std::vector<grpc_error*> ParseRoute(
+      const Json& json, const XdsRoutingLbConfig::ActionMap& action_map,
+      XdsRoutingLbConfig::Route* route,
+      std::set<std::string /*action_name*/>* action_to_be_used) {
     std::vector<grpc_error*> error_list;
     if (json.type() != Json::Type::OBJECT) {
       error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-          "field:route element should be of type object"));
+          "value should be of type object"));
       return error_list;
     }
     // Parse MethodName.
     auto it = json.object_value().find("methodName");
     if (it == json.object_value().end()) {
       error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-          "field:routes element: methodName is required"));
+          "field:methodName error:required field missing"));
     } else {
-      std::vector<grpc_error*> route_errors =
+      std::vector<grpc_error*> method_name_errors =
           ParseMethodName(it->second, &route->matcher);
-      if (!route_errors.empty()) {
-        grpc_error* error =
-            GRPC_ERROR_CREATE_FROM_COPIED_STRING("field:route element error");
-        for (grpc_error* route_error : route_errors) {
-          error = grpc_error_add_child(error, route_error);
-        }
-        error_list.push_back(error);
+      if (!method_name_errors.empty()) {
+        error_list.push_back(GRPC_ERROR_CREATE_FROM_VECTOR(
+            "field:methodName", &method_name_errors));
       }
     }
     // Parse action.
     it = json.object_value().find("action");
     if (it == json.object_value().end()) {
       error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-          "field:route element: action is required"));
+          "field:action error:required field missing"));
     } else if (it->second.type() != Json::Type::STRING) {
       error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-          "field:route element error action should be of type string"));
+          "field:action error:should be of type string"));
     } else {
       route->action = it->second.string_value();
+      // Validate action exists and mark it as used.
+      if (!route->action.empty() &&
+          action_map.find(route->action) == action_map.end()) {
+        error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+            absl::StrCat("field:action error:", route->action,
+                         " does not exist")
+                .c_str()));
+      }
+      action_to_be_used->erase(route->action);
     }
     return error_list;
   }

src/core/ext/filters/client_channel/xds/xds_api.cc

@@ -1051,12 +1051,6 @@ grpc_error* RouteConfigParse(
             "Path is not empty string, prefix cannot also be non-empty.");
       }
     }
-    if (i == (size - 1)) {
-      if (!(rds_route.service.empty() && rds_route.method.empty())) {
-        return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-            "Default route must have empty service and method");
-      }
-    }
     if (!envoy_api_v2_route_Route_has_route(route)) {
       return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
           "No RouteAction found in route.");
@@ -1073,6 +1067,15 @@ grpc_error* RouteConfigParse(
     rds_route.cluster_name = std::string(action.data, action.size);
     rds_update->routes.emplace_back(std::move(rds_route));
   }
+  if (rds_update->routes.empty()) {
+    return GRPC_ERROR_CREATE_FROM_STATIC_STRING("No valid routes specified.");
+  } else {
+    if (!rds_update->routes.back().service.empty() ||
+        !rds_update->routes.back().method.empty()) {
+      return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+          "Default route must have empty service and method");
+    }
+  }
   return GRPC_ERROR_NONE;
 }
 

src/core/ext/filters/client_channel/xds/xds_client.cc

@@ -903,10 +903,8 @@ void XdsClient::ChannelState::AdsCallState::AcceptLdsUpdate(
                  ? lds_update->route_config_name.c_str()
                  : "<inlined>"));
     if (lds_update->rds_update.has_value()) {
-      gpr_log(GPR_INFO,
-              " [xds_client %p] LDS update received; LDS RouteConfiguration "
-              "contains %lu routes",
-              this, lds_update->rds_update.value().routes.size());
+      gpr_log(GPR_INFO, "  RouteConfiguration contains %lu routes", this,
+              lds_update->rds_update.value().routes.size());
       for (const auto& route : lds_update->rds_update.value().routes) {
         gpr_log(GPR_INFO,
                 "  route: { service=\"%s\", "
@@ -2061,29 +2059,29 @@ void XdsClient::ResetBackoff() {
 
 namespace {
 std::string CreateServiceConfigActionCluster(const std::string& cluster_name) {
-  return (
-      absl::StrFormat("      \"cds:%s\":{\n"
-                      "        \"child_policy\":[ {\n"
-                      "          \"cds_experimental\":{\n"
-                      "            \"cluster\": \"%s\"\n"
-                      "          }\n"
-                      "        } ]\n"
-                      "       }",
-                      cluster_name.c_str(), cluster_name.c_str()));
+  return absl::StrFormat(
+      "      \"cds:%s\":{\n"
+      "        \"child_policy\":[ {\n"
+      "          \"cds_experimental\":{\n"
+      "            \"cluster\": \"%s\"\n"
+      "          }\n"
+      "        } ]\n"
+      "       }",
+      cluster_name.c_str(), cluster_name.c_str());
 }
 
 std::string CreateServiceConfigRoute(const std::string& cluster_name,
                                      const std::string& service,
                                      const std::string& method) {
-  return (
-      absl::StrFormat("      { \n"
-                      "         \"methodName\": {\n"
-                      "           \"service\": \"%s\",\n"
-                      "           \"method\": \"%s\"\n"
-                      "        },\n"
-                      "        \"action\": \"cds:%s\"\n"
-                      "      }",
-                      service.c_str(), method.c_str(), cluster_name.c_str()));
+  return absl::StrFormat(
+      "      { \n"
+      "         \"methodName\": {\n"
+      "           \"service\": \"%s\",\n"
+      "           \"method\": \"%s\"\n"
+      "        },\n"
+      "        \"action\": \"cds:%s\"\n"
+      "      }",
+      service.c_str(), method.c_str(), cluster_name.c_str());
 }
 }  // namespace
 

test/cpp/end2end/xds_end2end_test.cc

@@ -2312,11 +2312,11 @@ TEST_P(LdsTest, XdsRoutingPathMatching) {
   mismatched_route2->mutable_route()->set_cluster(kNewCluster2Name);
   auto* default_route = new_route_config.mutable_virtual_hosts(0)->add_routes();
   default_route->mutable_match()->set_prefix("");
-  default_route->mutable_match()->set_path("");
   default_route->mutable_route()->set_cluster(kDefaultResourceName);
   Listener listener =
       balancers_[0]->ads_service()->BuildListener(new_route_config);
   balancers_[0]->ads_service()->SetLdsResource(listener, kDefaultResourceName);
+  CheckRpcSendOk(kNumRpcs, 1000, true);
   CheckEcho1RpcSendOk(kNumRpcs, 1000, true);
   CheckEcho2RpcSendOk(kNumRpcs, 1000, true);
   // Make sure RPCs all go to the correct backend.
@@ -2330,7 +2330,7 @@ TEST_P(LdsTest, XdsRoutingPathMatching) {
       EXPECT_EQ(0, backends_[i]->backend1_service()->request_count());
       EXPECT_EQ(kNumRpcs, backends_[i]->backend2_service()->request_count());
     } else {
-      EXPECT_EQ(0, backends_[i]->backend_service()->request_count());
+      EXPECT_EQ(kNumRpcs / 2, backends_[i]->backend_service()->request_count());
       EXPECT_EQ(0, backends_[i]->backend1_service()->request_count());
       EXPECT_EQ(0, backends_[i]->backend2_service()->request_count());
     }
@@ -2378,7 +2378,6 @@ TEST_P(LdsTest, XdsRoutingPrefixMatching) {
   matched_route->mutable_route()->set_cluster(kNewCluster2Name);
   auto* default_route = new_route_config.mutable_virtual_hosts(0)->add_routes();
   default_route->mutable_match()->set_prefix("");
-  default_route->mutable_match()->set_path("");
   default_route->mutable_route()->set_cluster(kDefaultResourceName);
   Listener listener =
       balancers_[0]->ads_service()->BuildListener(new_route_config);
@@ -2398,73 +2397,6 @@ TEST_P(LdsTest, XdsRoutingPrefixMatching) {
   }
 }
 
-// Tests that LDS client should choose the default route (with no matching
-// specified) after unable to find a match with previous routes.
-TEST_P(LdsTest, XdsRoutingDefaultRoute) {
-  ResetStub(0, 0, "", 0, 1);
-  const char* kNewCluster1Name = "new_cluster_1";
-  const char* kNewCluster2Name = "new_cluster_2";
-  const size_t kNumRpcs = 10;
-  SetNextResolution({});
-  SetNextResolutionForLbChannelAllBalancers();
-  // Populate new EDS resources.
-  AdsServiceImpl::EdsResourceArgs args({
-      {"locality0", GetBackendPorts(0, 2)},
-  });
-  AdsServiceImpl::EdsResourceArgs args1({
-      {"locality0", GetBackendPorts(2, 3)},
-  });
-  AdsServiceImpl::EdsResourceArgs args2({
-      {"locality0", GetBackendPorts(3, 4)},
-  });
-  balancers_[0]->ads_service()->SetEdsResource(
-      AdsServiceImpl::BuildEdsResource(args), kDefaultResourceName);
-  balancers_[0]->ads_service()->SetEdsResource(
-      AdsServiceImpl::BuildEdsResource(args1, kNewCluster1Name),
-      kNewCluster1Name);
-  balancers_[0]->ads_service()->SetEdsResource(
-      AdsServiceImpl::BuildEdsResource(args2, kNewCluster2Name),
-      kNewCluster2Name);
-  // Populate new CDS resources.
-  Cluster new_cluster1 = balancers_[0]->ads_service()->default_cluster();
-  new_cluster1.set_name(kNewCluster1Name);
-  balancers_[0]->ads_service()->SetCdsResource(new_cluster1, kNewCluster1Name);
-  Cluster new_cluster2 = balancers_[0]->ads_service()->default_cluster();
-  new_cluster2.set_name(kNewCluster2Name);
-  balancers_[0]->ads_service()->SetCdsResource(new_cluster2, kNewCluster2Name);
-  // Change RDS resource to set up prefix matching and path matching that do
-  // match the traffic, so traffic goes to the default cluster.
-  RouteConfiguration new_route_config =
-      balancers_[0]->ads_service()->default_route_config();
-  auto* mismatched_route1 =
-      new_route_config.mutable_virtual_hosts(0)->mutable_routes(0);
-  mismatched_route1->mutable_match()->set_prefix(
-      "/grpc.testing.EchoTestService0");
-  mismatched_route1->mutable_route()->set_cluster(kNewCluster1Name);
-  auto* mismatched_route2 =
-      new_route_config.mutable_virtual_hosts(0)->add_routes();
-  mismatched_route2->mutable_match()->set_path(
-      "/grpc.testing.EchoTestService/Echo1");
-  mismatched_route2->mutable_route()->set_cluster(kNewCluster2Name);
-  auto* default_route = new_route_config.mutable_virtual_hosts(0)->add_routes();
-  default_route->mutable_match()->set_prefix("");
-  default_route->mutable_match()->set_path("");
-  default_route->mutable_route()->set_cluster(kDefaultResourceName);
-  Listener listener =
-      balancers_[0]->ads_service()->BuildListener(new_route_config);
-  balancers_[0]->ads_service()->SetLdsResource(listener, kDefaultResourceName);
-  WaitForAllBackends(0, 2);
-  CheckRpcSendOk(kNumRpcs);
-  // Make sure RPCs all go to the correct backend.
-  for (size_t i = 0; i < 4; ++i) {
-    if (i < 2) {
-      EXPECT_EQ(kNumRpcs / 2, backends_[i]->backend_service()->request_count());
-    } else {
-      EXPECT_EQ(0, backends_[i]->backend_service()->request_count());
-    }
-  }
-}
-
 using RdsTest = BasicTest;
 
 // Tests that RDS client should send an ACK upon correct RDS response.
@@ -2783,7 +2715,7 @@ TEST_P(LocalityMapTest, NoLocalities) {
 
 // Tests that the locality map can work properly even when it contains a large
 // number of localities.
-/*TEST_P(LocalityMapTest, StressTest) {
+TEST_P(LocalityMapTest, StressTest) {
   SetNextResolution({});
   SetNextResolutionForLbChannelAllBalancers();
   const size_t kNumLocalities = 100;
@@ -2807,13 +2739,13 @@ TEST_P(LocalityMapTest, NoLocalities) {
       AdsServiceImpl::BuildEdsResource(args), 60 * 1000, kDefaultResourceName));
   // Wait until backend 0 is ready, before which kNumLocalities localities are
   // received and handled by the xds policy.
-  WaitForBackend(0, /*reset_counters=*false);
+  WaitForBackend(0, /*reset_counters=*/false);
   EXPECT_EQ(0U, backends_[1]->backend_service()->request_count());
   // Wait until backend 1 is ready, before which kNumLocalities localities are
   // removed by the xds policy.
   WaitForBackend(1);
   delayed_resource_setter.join();
-}*/
+}
 
 // Tests that the localities in a locality map are picked correctly after update
 // (addition, modification, deletion).