tools: add comprehensive coverage reporting to router check

docs/root/configuration/tools/router_check.rst

@@ -167,6 +167,19 @@ run will fail.
 
 .. code:: bash
 
-  > bazel-bin/test/tools/router_check/router_check_tool --config-path ... --test-path ... --useproto --fail-under 0.08
-  Current route coverage: 0.0744863
-  Failed to meet coverage requirement: 0.08
+  > bazel-bin/test/tools/router_check/router_check_tool --config-path ... --test-path ... --useproto --fail-under 8
+  Current route coverage: 7.44863%
+  Failed to meet coverage requirement: 8%
+
+
+By default the coverage report measures test coverage by checking that at least one field is
+verified for every route. However, this can leave holes in the tests where fields
+aren't validated and later changed. For more comprehensive coverage you can add a flag,
+`--covall`, which will calculate coverage taking into account all of the possible
+fields that could be tested.
+
+.. code:: bash
+
+  > bazel-bin/test/tools/router_check/router_check_tool --config-path ... --test-path ... --useproto --f 7 --covall
+  Current route coverage: 6.2948%
+  Failed to meet coverage requirement: 7%

docs/root/intro/version_history.rst

@@ -16,6 +16,7 @@ Version history
 * listeners: added :ref:`HTTP inspector listener filter <config_listener_filters_http_inspector>`.
 * router: added :ref:`rq_retry_skipped_request_not_complete <config_http_filters_router_stats>` counter stat to router stats.
 * router check tool: add coverage reporting & enforcement.
+* router check tool: add comprehensive coverage reporting.
 * tls: added verification of IP address SAN fields in certificates against configured SANs in the
   certificate validation context.
 * upstream: added network filter chains to upstream connections, see :ref:`filters<envoy_api_field_Cluster.filters>`.

test/tools/router_check/coverage.cc

@@ -5,19 +5,80 @@
 #include "envoy/api/v2/core/base.pb.h"
 
 namespace Envoy {
-void Coverage::markCovered(const Envoy::Router::RouteEntry& route) {
-  // n.b. If we reach the end of the seen routes without finding the specified
-  // route we add it as seen, otherwise it's a duplicate.
-  if (std::find(seen_routes_.begin(), seen_routes_.end(), &route) == seen_routes_.end()) {
-    seen_routes_.push_back(&route);
+double RouteCoverage::report() {
+  uint64_t route_weight = 0;
+  for (const auto& covered_field : coverageFields()) {
+    if (covered_field) {
+      route_weight += 1;
+    }
   }
+  return static_cast<double>(route_weight) / coverageFields().size();
+}
+
+void Coverage::markClusterCovered(const Envoy::Router::RouteEntry& route) {
+  coveredRoute(route).setClusterCovered();
+}
+
+void Coverage::markVirtualClusterCovered(const Envoy::Router::RouteEntry& route) {
+  coveredRoute(route).setVirtualClusterCovered();
+}
+
+void Coverage::markVirtualHostCovered(const Envoy::Router::RouteEntry& route) {
+  coveredRoute(route).setVirtualHostCovered();
+}
+
+void Coverage::markPathRewriteCovered(const Envoy::Router::RouteEntry& route) {
+  coveredRoute(route).setPathRewriteCovered();
+}
+
+void Coverage::markHostRewriteCovered(const Envoy::Router::RouteEntry& route) {
+  coveredRoute(route).setHostRewriteCovered();
+}
+
+void Coverage::markRedirectPathCovered(const Envoy::Router::RouteEntry& route) {
+  coveredRoute(route).setRedirectPathCovered();
 }
 
 double Coverage::report() {
   uint64_t num_routes = 0;
   for (const auto& host : route_config_.virtual_hosts()) {
-    num_routes += host.routes_size();
+    for (const auto& route : host.routes()) {
+      if (route.route().has_weighted_clusters()) {
+        num_routes += route.route().weighted_clusters().clusters_size();
+      } else {
+        num_routes += 1;
+      }
+    }
+  }
+  return 100 * static_cast<double>(covered_routes_.size()) / num_routes;
+}
+
+double Coverage::detailedReport() {
+  uint64_t num_routes = 0;
+  for (const auto& host : route_config_.virtual_hosts()) {
+    for (const auto& route : host.routes()) {
+      if (route.route().has_weighted_clusters()) {
+        num_routes += route.route().weighted_clusters().clusters_size();
+      } else {
+        num_routes += 1;
+      }
+    }
+  }
+  double cumulative_coverage = 0;
+  for (auto& covered_route : covered_routes_) {
+    cumulative_coverage += covered_route->report();
   }
-  return 100 * static_cast<double>(seen_routes_.size()) / num_routes;
+  return 100 * cumulative_coverage / num_routes;
 }
+
+RouteCoverage& Coverage::coveredRoute(const Envoy::Router::RouteEntry& route) {
+  for (auto& route_coverage : covered_routes_) {
+    if (route_coverage->covers(&route)) {
+      return *route_coverage;
+    }
+  }
+  std::unique_ptr<RouteCoverage> new_coverage = std::make_unique<RouteCoverage>(&route);
+  covered_routes_.push_back(std::move(new_coverage));
+  return coveredRoute(route);
+};
 } // namespace Envoy

test/tools/router_check/coverage.h

@@ -5,14 +5,51 @@
 #include "test/mocks/server/mocks.h"
 
 namespace Envoy {
+class RouteCoverage : Logger::Loggable<Logger::Id::testing> {
+public:
+  RouteCoverage(const Envoy::Router::RouteEntry* route) : route_(*route){};
+
+  double report();
+  void setClusterCovered() { cluster_covered_ = true; }
+  void setVirtualClusterCovered() { virtual_cluster_covered_ = true; }
+  void setVirtualHostCovered() { virtual_host_covered_ = true; }
+  void setPathRewriteCovered() { path_rewrite_covered_ = true; }
+  void setHostRewriteCovered() { host_rewrite_covered_ = true; }
+  void setRedirectPathCovered() { redirect_path_covered_ = true; }
+  bool covers(const Envoy::Router::RouteEntry* route) { return &route_ == route; }
+
+private:
+  const Envoy::Router::RouteEntry& route_;
+  bool cluster_covered_{false};
+  bool virtual_cluster_covered_{false};
+  bool virtual_host_covered_{false};
+  bool path_rewrite_covered_{false};
+  bool host_rewrite_covered_{false};
+  bool redirect_path_covered_{false};
+
+  std::vector<bool> coverageFields() {
+    return std::vector<bool>{cluster_covered_,      virtual_cluster_covered_,
+                             virtual_host_covered_, path_rewrite_covered_,
+                             host_rewrite_covered_, redirect_path_covered_};
+  }
+};
+
 class Coverage : Logger::Loggable<Logger::Id::testing> {
 public:
   Coverage(envoy::api::v2::RouteConfiguration config) : route_config_(config){};
-  void markCovered(const Envoy::Router::RouteEntry& route);
+  void markClusterCovered(const Envoy::Router::RouteEntry& route);
+  void markVirtualClusterCovered(const Envoy::Router::RouteEntry& route);
+  void markVirtualHostCovered(const Envoy::Router::RouteEntry& route);
+  void markPathRewriteCovered(const Envoy::Router::RouteEntry& route);
+  void markHostRewriteCovered(const Envoy::Router::RouteEntry& route);
+  void markRedirectPathCovered(const Envoy::Router::RouteEntry& route);
   double report();
+  double detailedReport();
 
 private:
-  std::vector<const Envoy::Router::RouteEntry*> seen_routes_;
+  RouteCoverage& coveredRoute(const Envoy::Router::RouteEntry& route);
+
+  std::vector<std::unique_ptr<RouteCoverage>> covered_routes_;
   const envoy::api::v2::RouteConfiguration route_config_;
 };
 } // namespace Envoy

test/tools/router_check/router.cc

@@ -211,7 +211,11 @@ bool RouterCheckTool::compareCluster(ToolConfig& tool_config, const std::string&
   if (tool_config.route_->routeEntry() != nullptr) {
     actual = tool_config.route_->routeEntry()->clusterName();
   }
-  return compareResults(actual, expected, "cluster_name");
+  const bool matches = compareResults(actual, expected, "cluster_name");
+  if (matches && tool_config.route_->routeEntry() != nullptr) {
+    coverage_.markClusterCovered(*tool_config.route_->routeEntry());
+  }
+  return matches;
 }
 
 bool RouterCheckTool::compareCluster(
@@ -234,7 +238,11 @@ bool RouterCheckTool::compareVirtualCluster(ToolConfig& tool_config, const std::
         tool_config.route_->routeEntry()->virtualCluster(*tool_config.headers_)->statName();
     actual = tool_config.symbolTable().toString(stat_name);
   }
-  return compareResults(actual, expected, "virtual_cluster_name");
+  const bool matches = compareResults(actual, expected, "virtual_cluster_name");
+  if (matches && tool_config.route_->routeEntry() != nullptr) {
+    coverage_.markVirtualClusterCovered(*tool_config.route_->routeEntry());
+  }
+  return matches;
 }
 
 bool RouterCheckTool::compareVirtualCluster(
@@ -254,7 +262,11 @@ bool RouterCheckTool::compareVirtualHost(ToolConfig& tool_config, const std::str
     Stats::StatName stat_name = tool_config.route_->routeEntry()->virtualHost().statName();
     actual = tool_config.symbolTable().toString(stat_name);
   }
-  return compareResults(actual, expected, "virtual_host_name");
+  const bool matches = compareResults(actual, expected, "virtual_host_name");
+  if (matches && tool_config.route_->routeEntry() != nullptr) {
+    coverage_.markVirtualHostCovered(*tool_config.route_->routeEntry());
+  }
+  return matches;
 }
 
 bool RouterCheckTool::compareVirtualHost(
@@ -282,8 +294,8 @@ bool RouterCheckTool::compareRewritePath(ToolConfig& tool_config, const std::str
     actual = tool_config.headers_->get_(Http::Headers::get().Path);
   }
   const bool matches = compareResults(actual, expected, "path_rewrite");
-  if (matches) {
-    coverage_.markCovered(*tool_config.route_->routeEntry());
+  if (matches && tool_config.route_->routeEntry() != nullptr) {
+    coverage_.markPathRewriteCovered(*tool_config.route_->routeEntry());
   }
   return matches;
 }
@@ -312,7 +324,11 @@ bool RouterCheckTool::compareRewriteHost(ToolConfig& tool_config, const std::str
 
     actual = tool_config.headers_->get_(Http::Headers::get().Host);
   }
-  return compareResults(actual, expected, "host_rewrite");
+  const bool matches = compareResults(actual, expected, "host_rewrite");
+  if (matches && tool_config.route_->routeEntry() != nullptr) {
+    coverage_.markHostRewriteCovered(*tool_config.route_->routeEntry());
+  }
+  return matches;
 }
 
 bool RouterCheckTool::compareRewriteHost(
@@ -332,7 +348,11 @@ bool RouterCheckTool::compareRedirectPath(ToolConfig& tool_config, const std::st
     actual = tool_config.route_->directResponseEntry()->newPath(*tool_config.headers_);
   }
 
-  return compareResults(actual, expected, "path_redirect");
+  const bool matches = compareResults(actual, expected, "path_redirect");
+  if (matches && tool_config.route_->routeEntry() != nullptr) {
+    coverage_.markRedirectPathCovered(*tool_config.route_->routeEntry());
+  }
+  return matches;
 }
 
 bool RouterCheckTool::compareRedirectPath(
@@ -422,6 +442,8 @@ Options::Options(int argc, char** argv) {
   TCLAP::ValueArg<double> fail_under("f", "fail-under",
                                      "Fail if test coverage is under a specified amount", false,
                                      0.0, "float", cmd);
+  TCLAP::SwitchArg comprehensive_coverage(
+      "", "covall", "Measure coverage by checking all route fields", cmd, false);
   TCLAP::ValueArg<std::string> config_path("c", "config-path", "Path to configuration file.", false,
                                            "", "string", cmd);
   TCLAP::ValueArg<std::string> test_path("t", "test-path", "Path to test file.", false, "",
@@ -438,6 +460,7 @@ Options::Options(int argc, char** argv) {
   is_proto_ = is_proto.getValue();
   is_detailed_ = is_detailed.getValue();
   fail_under_ = fail_under.getValue();
+  comprehensive_coverage_ = comprehensive_coverage.getValue();
 
   if (is_proto_) {
     config_path_ = config_path.getValue();

test/tools/router_check/router.h

@@ -89,7 +89,9 @@ class RouterCheckTool : Logger::Loggable<Logger::Id::testing> {
    */
   void setShowDetails() { details_ = true; }
 
-  float coverage() { return coverage_.report(); }
+  float coverage(bool detailed) {
+    return detailed ? coverage_.detailedReport() : coverage_.report();
+  }
 
 private:
   RouterCheckTool(
@@ -181,6 +183,11 @@ class Options {
    */
   double failUnder() const { return fail_under_; }
 
+  /**
+   * @return true if test coverage should be comprehensive.
+   */
+  bool comprehensiveCoverage() const { return comprehensive_coverage_; }
+
   /**
    * @return true if proto schema test is used.
    */
@@ -197,6 +204,7 @@ class Options {
   std::string unlabelled_test_path_;
   std::string unlabelled_config_path_;
   float fail_under_;
+  bool comprehensive_coverage_;
   bool is_proto_;
   bool is_detailed_;
 };

test/tools/router_check/router_check.cc

@@ -25,8 +25,8 @@ int main(int argc, char* argv[]) {
       return EXIT_FAILURE;
     }
 
-    const double current_coverage = checktool.coverage();
-    std::cerr << "Current route coverage: " << current_coverage << "%" << std::endl;
+    const double current_coverage = checktool.coverage(options.comprehensiveCoverage());
+    std::cout << "Current route coverage: " << current_coverage << "%" << std::endl;
     if (enforce_coverage) {
       if (current_coverage < options.failUnder()) {
         std::cerr << "Failed to meet coverage requirement: " << options.failUnder() << "%"

test/tools/router_check/test/config/ComprehensiveRoutes.golden.json

@@ -0,0 +1,56 @@
+[
+  {
+    "test_name": "Test 1",
+    "input": {
+      ":authority": "www.lyft.com",
+      ":path": "/new_endpoint"
+    },
+    "validate": {
+      "cluster_name": "www2",
+      "virtual_cluster_name": "other",
+      "virtual_host_name": "www2_host",
+      "path_rewrite": "/api/new_endpoint",
+      "host_rewrite": "www.lyft.com",
+      "path_redirect": ""
+    }
+  },
+  {
+    "test_name": "Test 2",
+    "input": {
+      ":authority": "www.lyft.com",
+      ":path": "/"
+    },
+    "validate": {
+      "cluster_name": "root_www2",
+      "virtual_cluster_name": "other",
+      "virtual_host_name": "www2_host",
+      "path_rewrite": "/",
+      "host_rewrite": "www.lyft.com",
+      "path_redirect": ""
+    }
+  },
+  {
+    "test_name": "Test 3",
+    "input": {
+      ":authority": "www.lyft.com",
+      ":path": "/foobar"
+    },
+    "validate": {
+      "cluster_name": "www2",
+      "virtual_cluster_name": "other",
+      "virtual_host_name": "www2_host",
+      "path_rewrite": "/foobar",
+      "host_rewrite": "www.lyft.com",
+      "path_redirect": ""
+    }
+  },
+  {
+    "test_name": "Test 4",
+    "input": {
+      ":authority": "www.lyft.com",
+      ":path": "/users/123",
+      ":method": "PUT"
+    },
+    "validate": {"virtual_cluster_name": "update_user"}
+  }
+]

test/tools/router_check/test/config/ComprehensiveRoutes.yaml

@@ -0,0 +1,22 @@
+virtual_hosts:
+  - name: www2_host
+    domains:
+    - www.lyft.com
+    routes:
+      - match:
+          prefix: /new_endpoint
+        route:
+          cluster: www2
+          prefix_rewrite: /api/new_endpoint
+      - match:
+          path: /
+        route:
+          cluster: root_www2
+      - match:
+          prefix: /
+        route:
+          cluster: www2
+    virtual_clusters:
+      - pattern: ^/users/\d+$
+        method: PUT
+        name: update_user