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.

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

Router check tool with two input json files #682

Merged
merged 20 commits into from
Apr 17, 2017
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
2 changes: 2 additions & 0 deletions bazel/envoy_build_system.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ def envoy_cc_library(name,
def envoy_cc_binary(name,
srcs = [],
data = [],
testonly = 0,
visibility = None,
deps = []):
native.cc_binary(
Expand All @@ -70,6 +71,7 @@ def envoy_cc_binary(name,
"-pthread",
"-lrt",
],
testonly = testonly,
linkstatic = 1,
visibility = visibility,
deps = deps + [
Expand Down
1 change: 1 addition & 0 deletions docs/configuration/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ Configuration reference
http_conn_man/http_conn_man
http_filters/http_filters
cluster_manager/cluster_manager
tools/router_check
109 changes: 109 additions & 0 deletions docs/configuration/tools/router_check.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
.. _config_tools_router_check_tool:

Route table check tool
======================

**NOTE: The following configuration is for the router table check tool only and is not part of the Envoy binary.
The route table check tool is used for testing purposes only.**
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: The router check tool is a standalone binary that can be used to verify Envoy's routing for a given configuration file.


Input for the route table check tool. The route table check tool checks if the route returned
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you intend for Input for the route table check tool to be a header? Otherwise it doesn't make too much sense the way the sentence is structured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I can expand to a full sentence.

Copy link
Contributor

@ccaraman ccaraman Apr 7, 2017

Choose a reason for hiding this comment

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

Please split this paragraph into a few sections:

  • what the tool is meant to do, what is supported, etc
  • how to configure the tool (example what are the required inputs, min fields, etc)
  • sample input/ouput

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add a link to the install rst page and rephrase.

by a router matches what is expected. The tool can be used to check cluster name, virtual cluster name,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit for router: add a link to the configuration.

virtual host name, path rewrite, host rewrite, and path redirect matches. Extensions for other
test cases can be added. The "check" field specifies the expected values in each test case. At least one test
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/host rewrite/manual and automatic host rewrite/
(and while doing so, please add a test case for automatic host rewrite. We use this flag in Istio to link K8S clusters to external services).

case is required. In addition, the authority and path fields specify the url sent to the router
and are also required. A simple configuration has one test case and is writen as follows. The test
expects a cluster name match of "instant-server".::

[
{
"authority":"api.lyft.com",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use proper header names, ex authority ->:authority

"path": "/api/locations",
"check": {"cluster_name": "instant-server"}
}
]
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this simple example, I enjoy seeing concrete examples when reading docs :)


.. code-block:: json

[
{
Copy link
Contributor

@ccaraman ccaraman Apr 7, 2017

Choose a reason for hiding this comment

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

suggestion: I would break the object into three parts

  • name for the test case
  • input value
  • expected output value

The reason for a tests name is once there are many objects it will be difficult to identify which individual test case failed. Grouping all of the input into an object makes it cleaner to understand what is being passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add this change. In that case would --details print out 4 fields or 5?
P/F test_name test_case expected actual OR
P/F test_name expected actual

Copy link
Contributor

Choose a reason for hiding this comment

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

I was doing a bit of thinking about this, and maybe the print output could be changed to something that looks like

Test Case: <test case name>
P/F <test_case> <expected> <actual>
...

The one thing that I'm still not sure about is if we should print the test cases that passed.

Copy link
Contributor Author

@hennna hennna Apr 11, 2017

Choose a reason for hiding this comment

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

In the most recent commit I've decided not to output details for the pass cases. The resulting print out would be:

<test case name>
<expected> <actual> <test_case>
...

where <expected> <actual> <test_case> are only printed for failure cases.

"authority": "...",
"path": "...",
"additional_headers": [
{
"name": "...",
"value": "..."
},
{
"..."
}
],
"method": "...",
"random_value" : "...",
"ssl" : "...",
"internal" : "...",
"check": {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe change check to expected_output?

Copy link
Member

Choose a reason for hiding this comment

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

or validate perhaps? I would also prefix the field with __ to indicate that its special.

"cluster_name": "...",
"virtual_cluster_name": "...",
"virtual_host_name": "...",
"path_rewrite": "...",
"host_rewrite": "...",
"path_redirect": "..."
Copy link
Contributor

Choose a reason for hiding this comment

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

Can there be a todo/gh issue to check the headers after a route has been chosen?

Copy link
Member

Choose a reason for hiding this comment

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

auto_host_rewrite missing

}
},
{
"..."
}
]

authority
*(required, string)* The url authority. This value along with the path parameter define
the url to be matched. An example authority value is "api.lyft.com".

path
*(required, string)* The url path. An example path value is "/foo".

additional_headers
*(optional, array)* Additional headers to be added as input for route determination. The :authority,
:path, :method, x-forwarded-proto, and x-envoy-internal fields are specified by the other config
options and should not be set here.

method
*(optional, string)* The request method. If not specified, the default method is GET. The options
are GET, PUT, or POST.

random_value
*(optional, integer)* An integer used to supply the random seed to use if a runtime choice is
Copy link
Member

Choose a reason for hiding this comment

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

Based on https://github.com/lyft/envoy/blob/master/source/common/router/config_impl.cc#L288, I think a better description would be: An integer used to supply the random seed for weighted cluster selection.. No need to mention runtime choice I think.

Copy link
Member

Choose a reason for hiding this comment

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

nit: @htuch I suggest keeping the name as is, as it serves two purposes (weighted_clusters and a previous incarnation of the same where the route blocks were repeated, with different target clusters and runtime values). @mattklein123 and @ccaraman - do you folks still use the old runtime style way of shifting traffic?

And btw, random_value is not even the seed. It "is" the random number that Envoy runtime uses (this number modulo 100 to obtain the weight for the route and then identify the target cluster)

required. The default value of random_value is 0.

ssl
*(optional, boolean)* A flag that determines whether to set x-forwarded-proto to https or http.
Copy link
Member

@htuch htuch Apr 6, 2017

Choose a reason for hiding this comment

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

So, naively I would have thought that the way to probe for SSL handling would by by setting the :scheme pseudo-header. However, looking at Envoy, it seems to only consider require_ssl by using x-forwarded-proto rather than :scheme in https://github.com/lyft/envoy/blob/master/source/common/router/config_impl.cc#L497.

I think this is because ConnectionManagerUtility::mutateRequestHeaders(), invoked on the request decode path, converts the :scheme into x-forwarded-proto.

So, I think we should actually make the ssl option here set :scheme: https, as if it's the simple case of a non-proxied request, and then have x-forwarded-proto available for testing via the additional_headers mechanism, no need to make it an explicit option.

@mattklein123 for confirmation.

Copy link
Member

Choose a reason for hiding this comment

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

@hennna and I were just discussing this. I now understand why we need to use x-forwarded-proto vs. :scheme in the test - it's a result of the fact we're mocking out the rest of Envoy and don't have the :scheme conversion applied to the header prior to passing to the router.

Copy link
Member

Choose a reason for hiding this comment

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

As a second sentence: "By setting x-forwarded-proto to a given protocol, the tool is able to simulate the behavior of a client issuing a request via http or https." It's still a bit low-level in explanation, the user really just cares about whether it's an http or https request, not how it works internally.

By setting x-forwarded-proto to a given protocol, the tool is able to simulate the behavior of
a client issuing a request via http or https. By default ssl is false which corresponds to
x-forwarded-proto set to http.

internal
*(optional, boolean)* A flag that determines whether to set x-envoy-internal to "true".
If not specified, or if internal is equal to false, x-envoy-internal is not set.

Copy link
Member

Choose a reason for hiding this comment

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

you might want to add request_headers_to_add field here to the list of header-related TODOs that you and @ccaraman were discussing about.

check
*(required, object)* The check object specifies the returned router parameters to match. At least one
test parameter must be specificed. Use "none" to indicate that no return value is expected. For example,
to test that no cluster match is expected use {"cluster_name": "none"}.

cluster_name
*(optional, string)* Match the cluster name.

virutal_cluster_name
*(optional, string)* Match the virtual cluster name.

virtual_host_name
*(optional, string)* Match the virtual host name.

path_rewrite
*(optional, string)* Match the path header field after rewrite.

host_rewrite
*(optional, string)* Match the host header field after rewrite.

path_redirect
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so looking at how the tool works, I think we shouldn't change the actual input behavior based on whether the output is specified as path_redirect. We can talk about this in person a bit, but I think the cleaner design is to just specify redirect behavior as a combination of the input path/authority and route config table, then measure the behavior here via the above checked results, with no special case mutation of the headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@htuch I've updated the tool so path_redirect does not have special config cases. Instead, method is set to GET by default and x-forwarded-proto is set to http by default in all test cases.

*(optional, string)* Match the returned redirect path.
1 change: 1 addition & 0 deletions docs/install/install.rst
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ Building and installation
installation
ref_configs
sandboxes
tools
58 changes: 58 additions & 0 deletions docs/install/tools.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
.. _install_tools:

Router Table Check Tool
Copy link
Contributor

Choose a reason for hiding this comment

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

Please be consistent with the tooling name. I prefer Route over Router.

=======================

The router check tool checks whether the route parameters returned by a router match what is expected.
The tool can also be used to check whether a path redirect, path rewrite, or host rewrite
match what is expected.

Input
The tool expects two input JSON files:

1. A router config JSON file. The router config JSON file schema is found :ref:`here <config_http_conn_man_route_table>`.

2. A tool config JSON file. The tool config JSON file schema is found :ref:`here <config_tools_router_check_tool>`.
The tool config input file specifies urls (composed of authorities and paths)
and expected route parameter values. Additonal parameters such as additonal headers are optional.

Output
The program exits with status EXIT_FAILURE if any test case does not match the expected route parameter
value.

The ``--details`` option prints out details for each test case. The first field indicates
a P for a correct match and a F for a failed match. The second field is the expected route parameter value.
Copy link
Contributor

Choose a reason for hiding this comment

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

About the second field, it isn't clear to me what the route parameter value means? One way to link to which test case fails is to have a name for each testing object.

Copy link
Contributor

@ccaraman ccaraman Apr 7, 2017

Choose a reason for hiding this comment

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

Or if the fourth field were the second field.

The third field is the actual route parameter value. The fourth field indicates the parameter that is
compared. For example: ::

P instant-server instant-server cluster_name
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the output for a successful test case should just print the test case name.

F default other virtual_host_name
P api.lyft.com api.lyft.com host_rewrite
P https://redirect.lyft.com/new_bar https://redirect.lyft.com/new_bar path_redirect
F locations ats cluster_name
P locations locations cluster_name

Testing with valid :ref:`runtime values <config_http_conn_man_route_table_route>` is not currently supported but can be added.
Copy link
Member

Choose a reason for hiding this comment

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

s/but can be added/, this may be added in future work./


Building
The tool can be built locally using Bazel. ::

bazel build //test/tools/router_check:router_check_tool

Running
The tool takes two input json files and an optional command line parameter ``--details``. The
expected order of command line arguements is:
1. The router configuration json file.
2. The tool configuration json file.
3. The optional details flag. ::

bazel-bin/test/tools/router_check/router_check_tool router_config.json tool_config.json

bazel-bin/test/tools/router_check/router_check_tool router_config.json tool_config.json --details

Testing
A bash shell script test can be run with bazel. The test compares routes using different router and
tool configuration json files. The configuration json files can be found in
test/tools/router_check/test/config/... . ::

bazel test //test/tools/router_check/...
7 changes: 5 additions & 2 deletions source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -494,10 +494,13 @@ RouteMatcher::RouteMatcher(const Json::Object& json_config, const ConfigImpl& gl
RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const Http::HeaderMap& headers,
uint64_t random_value) const {
// First check for ssl redirect.
if (ssl_requirements_ == SslRequirements::ALL && headers.ForwardedProto()->value() != "https") {
if (ssl_requirements_ == SslRequirements::ALL && headers.ForwardedProto() != nullptr &&
Copy link
Member

Choose a reason for hiding this comment

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

I don't mind the changes in this file, but FYI the connection manager enforces that ForwardedProto is always set. Not sure if we should change this or your tool should mimic that behavior and always set ForwardedProto to something by default?

headers.ForwardedProto()->value() != "https") {

return SSL_REDIRECT_ROUTE;
} else if (ssl_requirements_ == SslRequirements::EXTERNAL_ONLY &&
headers.ForwardedProto()->value() != "https" && !headers.EnvoyInternalRequest()) {
headers.ForwardedProto() != nullptr && headers.ForwardedProto()->value() != "https" &&
!headers.EnvoyInternalRequest()) {
return SSL_REDIRECT_ROUTE;
}

Expand Down
2 changes: 2 additions & 0 deletions source/common/router/config_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,8 @@ class ConfigImpl : public Config {
std::list<std::pair<Http::LowerCaseString, std::string>> request_headers_to_add_;
};

typedef std::unique_ptr<ConfigImpl> ConfigImplPtr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?


/**
* Implementation of Config that is empty.
*/
Expand Down
34 changes: 34 additions & 0 deletions test/tools/router_check/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package(default_visibility = ["//visibility:public"])

load("//bazel:envoy_build_system.bzl", "envoy_cc_binary", "envoy_cc_test_library")

envoy_cc_binary(
name = "router_check_tool",
testonly = 1,
deps = [":router_check_main_lib"],
)

envoy_cc_test_library(
name = "router_check_main_lib",
Copy link
Member

Choose a reason for hiding this comment

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

You could optionally combined roucher_check_main_lib and router_tool_lib.

srcs = ["router_check.cc"],
deps = [
":router_tool_lib",
],
)

envoy_cc_test_library(
name = "router_tool_lib",
srcs = ["router.cc"],
hdrs = ["router.h"],
deps = [
"//source/common/http:header_map_lib",
"//source/common/http:headers_lib",
"//source/common/json:json_loader_lib",
"//source/common/router:config_lib",
"//test/mocks/runtime:runtime_mocks",
"//test/mocks/upstream:upstream_mocks",
"//test/test_common:printers_lib",
"//test/test_common:utility_lib",
"//test/tools/router_check/json:tool_config_schemas_lib",
],
)
9 changes: 9 additions & 0 deletions test/tools/router_check/json/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package(default_visibility = ["//visibility:public"])

load("//bazel:envoy_build_system.bzl", "envoy_cc_library")

envoy_cc_library(
name = "tool_config_schemas_lib",
srcs = ["tool_config_schemas.cc"],
hdrs = ["tool_config_schemas.h"],
)
52 changes: 52 additions & 0 deletions test/tools/router_check/json/tool_config_schemas.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
#include "test/tools/router_check/json/tool_config_schemas.h"

const std::string& Json::ToolSchema::routerCheckSchema() {
static const std::string* router_check_schema = new std::string(R"EOF(
Copy link
Member

Choose a reason for hiding this comment

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

Great, this is what I was talking about it.

{
"$schema": "http://json-schema.org/schema#",
"type": "array",
"minItems": 1,
"items": {
"type": "object",
"properties": {
"authority": {"type": "string"},
"path": {"type": "string"},
"additional_headers": {
"type": "array",
"items": {
"type": "object",
"properties": {
"name": {"type": "string"},
"value": {"type": "string"}
},
"additionalProperties": false,
"required": ["name", "value"],
"maxProperties": 2
}
},
"method": {"type": "string", "enum": ["GET", "PUT", "POST"]},
"random_value": {"type": "integer"},
"ssl": {"type": "boolean"},
"internal": {"type": "boolean"},
"check": {
"type": "object",
"properties": {
"cluster_name": {"type": "string"},
"virtual_cluster_name": {"type": "string"},
"virtual_host_name": {"type": "string"},
"path_rewrite": {"type": "string"},
"host_rewrite": {"type": "string"},
"path_redirect": {"type": "string"}
},
"minProperties": 1,
"additionalProperties": false
}
},
"additionalProperties": false,
"required": ["authority", "path", "check"]
}
}
)EOF");

return *router_check_schema;
}
15 changes: 15 additions & 0 deletions test/tools/router_check/json/tool_config_schemas.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#pragma once

namespace Json {

class ToolSchema {

Copy link
Member

Choose a reason for hiding this comment

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

nit: del newline

public:
/**
* Obtain the router check json schema
* @return const std::string& of the schema string
*/
static const std::string& routerCheckSchema();
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@hennna hennna Apr 12, 2017

Choose a reason for hiding this comment

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

Originally I followed the definition pattern in config_shemas.h, but I changed to wrapping the variable in a function in this commit: 34d1dcb.

The reasoning was in response to comments pointing out that static const string types are dangerous due to random ordering of constructor and destructor calls.

};

} // Json
Loading