diff --git a/docs/root/configuration/tools/router_check.rst b/docs/root/configuration/tools/router_check.rst index 5a596521e95c9..1d4dd2482757a 100644 --- a/docs/root/configuration/tools/router_check.rst +++ b/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% diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index d13c8caecd40c..67751205b3a49 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -16,6 +16,7 @@ Version history * listeners: added :ref:`HTTP inspector listener filter `. * router: added :ref:`rq_retry_skipped_request_not_complete ` 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`. diff --git a/test/tools/router_check/coverage.cc b/test/tools/router_check/coverage.cc index 5f8c51a51351b..3e81b105097f1 100644 --- a/test/tools/router_check/coverage.cc +++ b/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(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(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(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 new_coverage = std::make_unique(&route); + covered_routes_.push_back(std::move(new_coverage)); + return coveredRoute(route); +}; } // namespace Envoy diff --git a/test/tools/router_check/coverage.h b/test/tools/router_check/coverage.h index 778906a89fcad..c2327449d89bb 100644 --- a/test/tools/router_check/coverage.h +++ b/test/tools/router_check/coverage.h @@ -5,14 +5,51 @@ #include "test/mocks/server/mocks.h" namespace Envoy { +class RouteCoverage : Logger::Loggable { +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 coverageFields() { + return std::vector{cluster_covered_, virtual_cluster_covered_, + virtual_host_covered_, path_rewrite_covered_, + host_rewrite_covered_, redirect_path_covered_}; + } +}; + class Coverage : Logger::Loggable { 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 seen_routes_; + RouteCoverage& coveredRoute(const Envoy::Router::RouteEntry& route); + + std::vector> covered_routes_; const envoy::api::v2::RouteConfiguration route_config_; }; } // namespace Envoy diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc index 70ed8c6fbd190..c75053792f6a2 100644 --- a/test/tools/router_check/router.cc +++ b/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 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 config_path("c", "config-path", "Path to configuration file.", false, "", "string", cmd); TCLAP::ValueArg 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(); diff --git a/test/tools/router_check/router.h b/test/tools/router_check/router.h index f4af695dcf7a8..a3395de2ad873 100644 --- a/test/tools/router_check/router.h +++ b/test/tools/router_check/router.h @@ -89,7 +89,9 @@ class RouterCheckTool : Logger::Loggable { */ 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_; }; diff --git a/test/tools/router_check/router_check.cc b/test/tools/router_check/router_check.cc index d7b27d99f3a31..e17f3eecc827f 100644 --- a/test/tools/router_check/router_check.cc +++ b/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() << "%" diff --git a/test/tools/router_check/test/config/ComprehensiveRoutes.golden.json b/test/tools/router_check/test/config/ComprehensiveRoutes.golden.json new file mode 100644 index 0000000000000..5a0e4d8725e4d --- /dev/null +++ b/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"} + } +] diff --git a/test/tools/router_check/test/config/ComprehensiveRoutes.yaml b/test/tools/router_check/test/config/ComprehensiveRoutes.yaml new file mode 100644 index 0000000000000..0613410256ca7 --- /dev/null +++ b/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 diff --git a/test/tools/router_check/test/route_tests.sh b/test/tools/router_check/test/route_tests.sh index cba937d3f7e38..0b06cb7425bf5 100755 --- a/test/tools/router_check/test/route_tests.sh +++ b/test/tools/router_check/test/route_tests.sh @@ -24,6 +24,12 @@ if [[ "${COVERAGE_OUTPUT}" != *"Current route coverage: "* ]] ; then exit 1 fi +COMP_COVERAGE_CMD="${PATH_BIN} ${PATH_CONFIG}/ComprehensiveRoutes.yaml ${PATH_CONFIG}/ComprehensiveRoutes.golden.json --details -f " +COVERAGE_OUTPUT=$($COMP_COVERAGE_CMD "100" "--covall" 2>&1) || echo "${COVERAGE_OUTPUT:-no-output}" +if [[ "${COVERAGE_OUTPUT}" != *"Current route coverage: 100%"* ]] ; then + exit 1 +fi + # Testing coverage flag fails COVERAGE_OUTPUT=$($COVERAGE_CMD "100" 2>&1) || echo "${COVERAGE_OUTPUT:-no-output}" if [[ "${COVERAGE_OUTPUT}" != *"Failed to meet coverage requirement: 100%"* ]] ; then