Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route Checker tool Fix code coverage bug in proto based schema #8101

Merged
merged 10 commits into from
Aug 30, 2019
+103 −28
Merged

Route Checker tool Fix code coverage bug in proto based schema #8101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
75524e1
deprecated check
Aug 27, 2019
2217838
docs
Aug 27, 2019
abd24d9
docs
Aug 27, 2019
b8f5dd4
fixtests
Aug 29, 2019
0737691
Merge branch 'master' into upstream
Aug 29, 2019
51b9d27
tests
Aug 30, 2019
856751c
tests
Aug 30, 2019
e8decd5
eof
Aug 30, 2019
ab5d18c
eof
Aug 30, 2019
e139575
100
Aug 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions test/tools/router_check/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,13 @@ bool RouterCheckTool::compareCluster(ToolConfig& tool_config, const std::string&

bool RouterCheckTool::compareCluster(
ToolConfig& tool_config, const envoy::RouterCheckToolSchema::ValidationAssert& expected) {
if (expected.cluster_name().empty()) {
if (!expected.has_cluster_name()) {
return true;
}
if (tool_config.route_ == nullptr) {
return compareResults("", expected.cluster_name(), "cluster_name");
return compareResults("", expected.cluster_name().value(), "cluster_name");
}
return compareCluster(tool_config, expected.cluster_name());
return compareCluster(tool_config, expected.cluster_name().value());
}

bool RouterCheckTool::compareVirtualCluster(ToolConfig& tool_config, const std::string& expected) {
Expand All @@ -251,13 +251,13 @@ bool RouterCheckTool::compareVirtualCluster(ToolConfig& tool_config, const std::

bool RouterCheckTool::compareVirtualCluster(
ToolConfig& tool_config, const envoy::RouterCheckToolSchema::ValidationAssert& expected) {
if (expected.virtual_cluster_name().empty()) {
if (!expected.has_virtual_cluster_name()) {
return true;
}
if (tool_config.route_ == nullptr) {
return compareResults("", expected.virtual_cluster_name(), "virtual_cluster_name");
return compareResults("", expected.virtual_cluster_name().value(), "virtual_cluster_name");
}
return compareVirtualCluster(tool_config, expected.virtual_cluster_name());
return compareVirtualCluster(tool_config, expected.virtual_cluster_name().value());
}

bool RouterCheckTool::compareVirtualHost(ToolConfig& tool_config, const std::string& expected) {
Expand All @@ -275,13 +275,13 @@ bool RouterCheckTool::compareVirtualHost(ToolConfig& tool_config, const std::str

bool RouterCheckTool::compareVirtualHost(
ToolConfig& tool_config, const envoy::RouterCheckToolSchema::ValidationAssert& expected) {
if (expected.virtual_host_name().empty()) {
if (!expected.has_virtual_host_name()) {
return true;
}
if (tool_config.route_ == nullptr) {
return compareResults("", expected.virtual_host_name(), "virtual_host_name");
return compareResults("", expected.virtual_host_name().value(), "virtual_host_name");
}
return compareVirtualHost(tool_config, expected.virtual_host_name());
return compareVirtualHost(tool_config, expected.virtual_host_name().value());
}

bool RouterCheckTool::compareRewritePath(ToolConfig& tool_config, const std::string& expected) {
Expand All @@ -306,13 +306,13 @@ bool RouterCheckTool::compareRewritePath(ToolConfig& tool_config, const std::str

bool RouterCheckTool::compareRewritePath(
ToolConfig& tool_config, const envoy::RouterCheckToolSchema::ValidationAssert& expected) {
if (expected.path_rewrite().empty()) {
if (!expected.has_path_rewrite()) {
return true;
}
if (tool_config.route_ == nullptr) {
return compareResults("", expected.path_rewrite(), "path_rewrite");
return compareResults("", expected.path_rewrite().value(), "path_rewrite");
}
return compareRewritePath(tool_config, expected.path_rewrite());
return compareRewritePath(tool_config, expected.path_rewrite().value());
}

bool RouterCheckTool::compareRewriteHost(ToolConfig& tool_config, const std::string& expected) {
Expand All @@ -337,13 +337,13 @@ bool RouterCheckTool::compareRewriteHost(ToolConfig& tool_config, const std::str

bool RouterCheckTool::compareRewriteHost(
ToolConfig& tool_config, const envoy::RouterCheckToolSchema::ValidationAssert& expected) {
if (expected.host_rewrite().empty()) {
if (!expected.has_host_rewrite()) {
return true;
}
if (tool_config.route_ == nullptr) {
return compareResults("", expected.host_rewrite(), "host_rewrite");
return compareResults("", expected.host_rewrite().value(), "host_rewrite");
}
return compareRewriteHost(tool_config, expected.host_rewrite());
return compareRewriteHost(tool_config, expected.host_rewrite().value());
}

bool RouterCheckTool::compareRedirectPath(ToolConfig& tool_config, const std::string& expected) {
Expand All @@ -361,13 +361,13 @@ bool RouterCheckTool::compareRedirectPath(ToolConfig& tool_config, const std::st

bool RouterCheckTool::compareRedirectPath(
ToolConfig& tool_config, const envoy::RouterCheckToolSchema::ValidationAssert& expected) {
if (expected.path_redirect().empty()) {
if (!expected.has_path_redirect()) {
return true;
}
if (tool_config.route_ == nullptr) {
return compareResults("", expected.path_redirect(), "path_redirect");
return compareResults("", expected.path_redirect().value(), "path_redirect");
}
return compareRedirectPath(tool_config, expected.path_redirect());
return compareRedirectPath(tool_config, expected.path_redirect().value());
}

bool RouterCheckTool::compareHeaderField(
Expand Down
63 changes: 63 additions & 0 deletions test/tools/router_check/test/config/ComprehensiveRoutes.golden.proto.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
{
"tests": [
{
"test_name": "Test 1",
"input": {
"authority": "www.lyft.com",
"path": "/new_endpoint",
"method": "GET"
},
"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": "/",
"method": "GET"
},
"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",
"method": "GET"
},
"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"
}
}
]
}
9 changes: 7 additions & 2 deletions test/tools/router_check/test/config/ComprehensiveRoutes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ virtual_hosts:
route:
cluster: www2
virtual_clusters:
- pattern: ^/users/\d+$
method: PUT
- headers:
- name: :path
safe_regex_match:
google_re2: {}
regex: ^/users/\d+$
- name: :method
exact_match: PUT
name: update_user
4 changes: 2 additions & 2 deletions test/tools/router_check/test/config/Weighted.golden.proto.pb_text
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ tests {
method: "GET"
}
validate: {
path_redirect: ""
path_redirect: { value: "" }
}
}

Expand All @@ -21,6 +21,6 @@ tests {
random_value: 115
}
validate: {
cluster_name: "cluster1"
cluster_name: { value: "cluster1" }
}
}
6 changes: 6 additions & 0 deletions test/tools/router_check/test/route_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ if [[ "${COVERAGE_OUTPUT}" != *"Current route coverage: "* ]] ; then
exit 1
fi

COMP_COVERAGE_CMD="${PATH_BIN} -c ${PATH_CONFIG}/ComprehensiveRoutes.yaml -t ${PATH_CONFIG}/ComprehensiveRoutes.golden.proto.json --details --useproto -f "
COVERAGE_OUTPUT=$($COMP_COVERAGE_CMD "100" "--covall" 2>&1) || echo "${COVERAGE_OUTPUT:-no-output}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We missed this code path before and the --useproto was calculating incorrect coverage

if [[ "${COVERAGE_OUTPUT}" != *"Current route coverage: 100%"* ]] ; 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
Expand Down
13 changes: 7 additions & 6 deletions test/tools/router_check/validation.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ syntax = "proto3";
package envoy.RouterCheckToolSchema;

import "envoy/api/v2/core/base.proto";
import "google/protobuf/wrappers.proto";
import "validate/validate.proto";

// [#protodoc-title: RouterCheckTool Validation]
Expand Down Expand Up @@ -78,22 +79,22 @@ message ValidationInput {
// For example, to test that no cluster match is expected use {“cluster_name”: “”}.
message ValidationAssert {
// Match the cluster name.
string cluster_name = 1;
google.protobuf.StringValue cluster_name = 1;

// Match the virtual cluster name.
string virtual_cluster_name = 2;
google.protobuf.StringValue virtual_cluster_name = 2;

// Match the virtual host name.
string virtual_host_name = 3;
google.protobuf.StringValue virtual_host_name = 3;

// Match the host header field after rewrite.
string host_rewrite = 4;
google.protobuf.StringValue host_rewrite = 4;

// Match the path header field after rewrite.
string path_rewrite = 5;
google.protobuf.StringValue path_rewrite = 5;

// Match the returned redirect path.
string path_redirect = 6;
google.protobuf.StringValue path_redirect = 6;

// Match the listed header fields.
// Examples header fields include the “:path”, “cookie”, and “date” fields.
Expand Down