Router check tool with two input json files #682

hennna · 2017-04-03T22:07:48Z

The router check tool compares whether routes returned by a router match what is expected. The tool takes two configuration json files: a router config file and a tool config file. This PR is in response to Issue #538.

The router tool is built using bazel. The tool is located in a new directory: test/tools/router_check/.

test/tools/router_check/json holds the tool config json schema files

route_test.sh exercises the tool to test 8 pairs of input router/tool config json files. The config files are located in test/tools/router_check/test/config/...

htuch

This will be a very useful tool, thanks for putting it together.

htuch · 2017-04-04T13:40:54Z

bazel/envoy_build_system.bzl

 visibility = None,
 deps = []):
 native.cc_binary(
 name = name,
+ testonly = testonly,


Can you order the parameter and the use of the parameter consistently relative to other params/args? I tend to use what buildifier says or https://bazel.build/versions/master/docs/be/c-cpp.html for guidance.

htuch · 2017-04-04T13:42:15Z

docs/configuration/tools/router_check.rst

@@ -0,0 +1,98 @@
+.. _config_tools_router_check_tool:
+
+Router check tool input interface


Should this be "Route table check tool" rather than "Router check tool"? Same in various places below.

htuch · 2017-04-04T14:01:07Z

docs/configuration/tools/router_check.rst

+ "check": {
+ "type": "object",
+ "properties" : {
+ "name": {"type" : "string", "enum" : ["cluster", "virtual_cluster", "virtual_host"] },


As discussed, the expected/checked properties can be reorganized so that individual input entries can be more expressive.

htuch · 2017-04-04T14:06:18Z

docs/configuration/tools/router_check.rst

+ *(optional, integer)* A random integer used when choosing between weighted load balanced clusters. The default value is 0.
+
+ssl
+ *(optional, boolean)* A flag that determines whether to set x-forwarded-proto to https or http. If not specified, this value is not set. In the redirect case, this value is default to false.


Can you just reorder this so that the inputs come first and the "check" block comes last? This would be a bit clearer to the reader.

htuch · 2017-04-04T14:08:08Z

source/common/router/config_impl.cc

@@ -464,9 +464,11 @@ RouteMatcher::RouteMatcher(const Json::Object& config, Runtime::Loader& runtime,
 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() &&


Prefer headers.ForwardedProto() != nullptr for better readability (it's clear we're dealing with a pointer).

htuch · 2017-04-04T14:22:10Z

test/tools/router_check/router_check.cc

+int main(int argc, char* argv[]) {
+
+ if (argc < 3 || argc > 4) {
+ return -1;


prefer ::exit(1);

htuch · 2017-04-04T14:22:22Z

test/tools/router_check/router_check.cc

+ RouterCheckTool checktool;
+
+ if (!checktool.create(argv[1])) {
+ return -1;


htuch · 2017-04-04T14:23:39Z

test/tools/router_check/router_check.cc

+ checktool.compareEntriesInJson(argv[2]);
+
+ // Print out total matches and conflicts
+ std::cout << "Total Y:" << checktool.getYes() << " N:" << checktool.getNo() << std::endl;


I would simplify and get rid of the total Y/N. In the simple, automated case, just exit 0/1 depending on whether there is a match. If --details is specified, print the details. It would simplify the interface and the implementation as well. No need for the calls to increaseCounters in various places.

htuch · 2017-04-04T14:24:28Z

test/tools/router_check/router.h

+ * Compare whether redirect path matches
+ * @param tool_config holds the configuration parameters extracted from a
+ * json input file
+ */


You don't need Doxygen for internal methods like this, in particular when the comment is basically the same as the name.

htuch · 2017-04-04T14:25:12Z

test/tools/router_check/router.h

+ /* Set whether to print out match case details
+ * @ param
+ */
+ void showDetails() { details_ = true; }


setShowDetails

dnoe · 2017-04-04T14:54:06Z

test/tools/router_check/router.h

+ // TODO(hennna): Switch away from mocks depending on feedback
+ NiceMock<Runtime::MockLoader> runtime_;
+ NiceMock<Upstream::MockClusterManager> cm_;
+ Router::ConfigImpl* config_;


To match Envoy style you can use std::unique_ptr here. It will also automagically fix the (harmless) memory leak of Router::ConfigImpl.

dnoe · 2017-04-04T14:59:21Z

test/tools/router_check/router.cc

+ loader->validateSchema(Json::Schema::ROUTE_CONFIGURATION_SCHEMA);
+
+ // Router configuration
+ config_ = new Router::ConfigImpl(*loader, runtime_, cm_, false);


I think this is harmless because the Router::ConfigImpl has a lifetime that ends when this executable exits, but there's a memory leak here. std::unique_ptr will address this without having to write a custom dtor.

+1, please get into the habit of always writing leak free (and exception safe) code even in utilities like this. unique_ptr is the way to go.

htuch

This is coming together nicely, I like the thorough documentation and testing in the PR. I still have some design level feedback before getting into the implementation specifics.

htuch · 2017-04-06T12:25:41Z

bazel/envoy_build_system.bzl

@@ -186,3 +188,17 @@ def envoy_proto_library(name, srcs = [], deps = []):
 include_prefix = envoy_include_prefix(PACKAGE_NAME),
 deps = [internal_name],
 )
+
+# Envoy shell script tests


I just added a shell test in #706, so based on this experience I'm thinking we can just skip the envoy_sh_test and use sh_test directly., we don't have too much Envoy specific that's going to be shared across tests today.

htuch · 2017-04-06T12:26:11Z

docs/configuration/tools/router_check.rst

+Route table check tool
+======================
+
+**NOTE: The following configuration is for the router table check tool only and is not part of the envoy binary.


htuch · 2017-04-06T12:27:28Z

docs/configuration/tools/router_check.rst

+ "path": "/api/locations",
+ "check": {"cluster_name": "instant-server"}
+ }
+ ]


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

htuch · 2017-04-06T12:29:20Z

docs/configuration/tools/router_check.rst

+ *(required, string)* The url path. An example path value is "/foo".
+
+additional_headers
+ *(optional, array)* Additional headers to be added before a route is returned.


"to be added as input to route determination."

htuch · 2017-04-06T12:31:36Z

docs/configuration/tools/router_check.rst

+ except for the redirect path test case. In the redirect path case, this parameter is not set by default.
+
+random_lb_value
+ *(optional, integer)* A random integer used when choosing between weighted load balanced clusters.


It's a little unclear to me how to use this. When would I specify a value of 4 vs. a value of 312? What does it do? Is it a seed? Is it a weight towards a particular cluster? etc.

htuch · 2017-04-06T12:35:40Z

test/tools/router_check/BUILD

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


[":router_check_main_lib"]

htuch · 2017-04-06T12:59:55Z

docs/configuration/tools/router_check.rst

+ The default value is 0.
+
+ssl
+ *(optional, boolean)* A flag that determines whether to set x-forwarded-proto to https or http.


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.

@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.

htuch · 2017-04-06T13:02:31Z

docs/configuration/tools/router_check.rst

+
+method
+ *(optional, string)* The request method. If not specified, the default method is GET in all test cases
+ except for the redirect path test case. In the redirect path case, this parameter is not set by default.


I'm not sure (as a reader) I get why thy there is different behavior for the redirect path case, or even how to specify whether in a test case I want to exercise redirect behavior. I think avoiding special cases is best (principle of least surprise). Same applies to the ssl value below.

htuch · 2017-04-06T13:08:27Z

docs/configuration/tools/router_check.rst

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


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.

@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.

htuch

I think this is now great at the design level, so onto the implementation review. It's looking mostly good there too modulo the review comments.

htuch · 2017-04-06T20:40:59Z

docs/configuration/tools/router_check.rst

+
+random_value
+ *(optional, integer)* An integer used to supply the random seed to use if a runtime choice is
+ required. Currently testing with valid runtime values is not supported. The default value of


What's a valid runtime value?

I don't understand it fully which is why it isn't supported in the tool. My understanding is that routes can match on a runtime key in addition to matching header values. https://lyft.github.io/envoy/docs/configuration/http_conn_man/route_config/route.html#config-http-conn-man-route-table-route

In that case, this value is used to determine whether a runtime feature is enabled.
https://github.com/lyft/envoy/blob/master/source/common/router/config_impl.cc#L152

The confusion is that this value also seems to be used when choosing between weighted clusters.
https://github.com/lyft/envoy/blob/master/source/common/router/config_impl.cc#L288

htuch · 2017-04-06T20:42:45Z

docs/configuration/tools/router_check.rst

+ random_value is 0.
+
+ssl
+ *(optional, boolean)* A flag that determines whether to set x-forwarded-proto to https or http.


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.

htuch · 2017-04-06T20:44:18Z

docs/install/tools.rst

+ 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


htuch · 2017-04-06T20:45:59Z

docs/install/tools.rst

+ P -----locations ----- locations
+ F -----www2 ----- www3
+ P -----root_www2 ----- root_www2
+ P -----https://redirect.lyft.com/new_bar ----- https://redirect.lyft.com/new_bar


It's probably cleaner to use whitespace separation rather than -----, since this makes it easier to parse out in other tools that might consume this tool, e.g. running sed/cut/grep over the output of --details.

htuch · 2017-04-06T20:46:32Z

docs/install/tools.rst

+ P -----root_www2 ----- root_www2
+ P -----https://redirect.lyft.com/new_bar ----- https://redirect.lyft.com/new_bar
+
+ Testing with valid runtime values is not currently supported but can be added.


This is another place that valid runtime values comes up that the reader might not have context for.

htuch · 2017-04-06T21:13:06Z

test/tools/router_check/source/router.h

+
+ /**
+ * Compare the expected and acutal route parameter values. Print out
+ * match details is details_ flag is set


Nit: if details_ flag is set.

htuch · 2017-04-06T21:13:21Z

test/tools/router_check/source/router.h

+ * match details is details_ flag is set
+ * @param actual holds the acutal route returned by the router
+ * @param expected holds the expected parameter value of the route
+ * @return true if acutal and expected match


Nit: actual

htuch · 2017-04-06T21:14:07Z

test/tools/router_check/source/router.h

+ */
+ bool compareEntriesInJson(const std::string& expected_route_json);
+
+ // Set whether to print out match case details


Nit: This one is not Doxygen but the others are. They should ideally be consistent.

htuch · 2017-04-06T21:18:01Z

test/tools/router_check/source/router.cc

+}
+
+bool RouterCheckTool::compareCluster(ToolConfig& tool_config, const std::string expected) {
+ Router::RouteConstSharedPtr route =


I think the route determination should be done once in routerCheckTool::compareEntriesInJson, rather multiple times in each of the compare* methods. You can pass a reference to the returned route, so this is just a small refactoring.

htuch · 2017-04-06T21:20:36Z

docs/install/tools.rst

+
+ 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 actual route parameter value.
+ The third field is the expected route parameter value. For example: ::


I think there should be a fourth field indicating what is actually being compared, e.g. is it the virtual cluster, the redirect_path, etc.

htuch

This is looking great, ready for wider review. @mattklein123 would you like to take a pass?

htuch · 2017-04-07T16:07:55Z

docs/install/tools.rst


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


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

htuch · 2017-04-07T16:08:56Z

test/tools/router_check/BUILD

@@ -12,6 +12,23 @@ envoy_cc_test_library(
 name = "router_check_main_lib",


You could optionally combined roucher_check_main_lib and router_tool_lib.

htuch · 2017-04-07T16:10:14Z

test/tools/router_check/json/tool_config_schemas.cc

@@ -1,6 +1,7 @@
 #include "test/tools/router_check/json/tool_config_schemas.h"

-const std::string Json::ToolSchema::ROUTER_CHECK_SCHEMA(R"EOF(
+const std::string& Json::ToolSchema::routerCheckSchema() {
+ static const std::string* router_check_schema = new std::string(R"EOF(


Great, this is what I was talking about it.

htuch · 2017-04-07T16:10:54Z

test/tools/router_check/router.h


 /**
 * Compare the expected and acutal route parameter values. Print out
- * match details is details_ flag is set
+ * match details if details_ flag is set


Nit: the earlier line seems too shrot.

htuch · 2017-04-07T16:16:55Z

docs/configuration/tools/router_check.rst

+ are GET, PUT, or POST.
+
+random_value
+ *(optional, integer)* An integer used to supply the random seed to use if a runtime choice is


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.

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)

htuch · 2017-04-07T16:21:02Z

test/tools/router_check/router.h

+
+ bool details_{false};
+
+ // TODO(hennna): Switch away from mocks depending on feedback


I think it's fine for v1, but would be interested in hearing about what other reviewers think. The idea is we could avoid taking a gmock dependency (and requiring this to be treated as a test binary by Bazel) by using dummy objects that are explicitly written instead of mocks. It might be the case that we could reuse what @rlazarus is doing for the config validation work, so could punt until then.

Can you clarify a bit more? as a test binary for what purpose? To test Envoy during compilation process ?

htuch · 2017-04-07T16:27:48Z

test/tools/router_check/router.cc

+ // Call appropriate function for each match case
+ if (check_type->hasObject("path_redirect")) {
+ if (!compareRedirectPath(tool_config, check_type->getString("path_redirect"), route)) {
+ no_failures = false;


There's ways to make this a bit more concise, e.g. by using a map of check_type -> check function. E.g. you could consider:

const std::unorder_map<std::string, std::function<bool(ToolConfig&,const std::string&,Router::RouteConstSharedPtr)> checkers = { {"path_redirect", std::bind(&RouterCheckTool::compareRedirect, this)}, {"cluster_name", std::bind(&RouterCheckTool::compareCluster, this)}, etc.. }

and then iterating over this map and applying the functions.

This is kind of an advanced C++ thing, I think for a tool like this you could do what you have today, just offering an alternative.

This is good advice :) @htuch beat me to it. Instead of the long sequence of ifs you can write something like:

for (auto kv : function_map) { if (check_type->hasObject(kv.first)) { auto test_result = kv.second(tool_config, check_type->getString(kv.first), route); // assuming you don't want to early exit no_failures = no_failures ? test_result : false; } }

Or iterate over found objects in JSON and look up the function in the map in a similar fashion.

mattklein123 · 2017-04-07T16:31:28Z

@ccaraman can you take a first pass over this today? I can look early next week. I would try to look for any missing edge cases that we might want to test that are not covered, or couldn't be easily extended with the design.

@rshriram I think you might want to review this as well.

ccaraman · 2017-04-07T20:05:31Z

docs/configuration/tools/router_check.rst

+**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.**
+
+Input for the route table check tool. The route table check tool checks if the route returned


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.

No. I can expand to a full sentence.

ccaraman · 2017-04-07T20:20:31Z

docs/configuration/tools/router_check.rst

+The route table check tool is used for testing purposes only.**
+
+Input for the route table check tool. The route table check tool checks if the route returned
+by a router matches what is expected. The tool can be used to check cluster name, virtual cluster name,


nit for router: add a link to the configuration.

ccaraman · 2017-04-07T20:25:28Z

docs/configuration/tools/router_check.rst

+**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.**
+
+Input for the route table check tool. The route table check tool checks if the route returned


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

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

ccaraman · 2017-04-07T20:27:36Z

docs/configuration/tools/router_check.rst

+
+ [
+ {
+ "authority":"api.lyft.com",


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

ccaraman · 2017-04-07T20:29:40Z

docs/install/tools.rst

@@ -0,0 +1,58 @@
+.. _install_tools:
+
+Router Table Check Tool


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

ccaraman · 2017-04-07T21:26:46Z

docs/install/tools.rst

+ 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.


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.

Or if the fourth field were the second field.

ccaraman · 2017-04-07T21:36:06Z

docs/configuration/tools/router_check.rst

+.. code-block:: json
+
+ [
+ {


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.

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

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.

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.

ccaraman · 2017-04-07T21:36:47Z

docs/configuration/tools/router_check.rst

+ "random_value" : "...",
+ "ssl" : "...",
+ "internal" : "...",
+ "check": {


nit: maybe change check to expected_output?

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

ccaraman · 2017-04-07T21:41:12Z

test/tools/router_check/router.h

+#include "test/tools/router_check/json/tool_config_schemas.h"
+
+/**
+ * Class that store the configuration parameters of the router


typo for store ->stores

ccaraman · 2017-04-07T21:41:27Z

test/tools/router_check/router.h

+
+/**
+ * Class that store the configuration parameters of the router
+ * check tool extracted from a json input file


nit: full stop at the end of the sentence.

ccaraman · 2017-04-12T16:10:50Z

test/tools/router_check/json/tool_config_schemas.cc

+ "type": "object",
+ "properties": {
+ ":authority": {"type": "string", "required": true},
+ ":path": {"type": "string"},


Can you add a min length check?

ccaraman · 2017-04-12T16:13:36Z

docs/configuration/tools/router_check.rst

+
+
+ additional_headers
+ *(optional, array)* Additional headers to be added as input for route determination. The ":authority",


can you please expand what the content of the array are? Similar to adding a section for https://lyft.github.io/envoy/docs/configuration/http_conn_man/route_config/route.html#adding-custom-request-headers

Please do this for the expected output for headers

ccaraman · 2017-04-12T16:15:49Z

test/tools/router_check/json/tool_config_schemas.cc

+ },
+ "additionalProperties": false,
+ "required": ["field", "value"],
+ "maxProperties": 2


What does max Properties do?

I put this here because the json schema allows properties to be repeated. For example:

{
"field": ":authority",
"value": "http://google.com",
"field": ":path"
}``

is acceptable. By saying both fields are required and that only two properties are allowed, I am trying to deter json input files with erroneous schemas.

ccaraman · 2017-04-12T16:27:33Z

test/tools/router_check/json/tool_config_schemas.h

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


Can you create a static const var like https://github.com/lyft/envoy/blob/master/source/common/json/config_schemas.h#L8?

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.

ccaraman · 2017-04-12T16:29:30Z

test/tools/router_check/router.cc

+ Json::ObjectPtr loader = Json::Factory::LoadFromFile(config_json);
+ loader->validateSchema(schema);
+ return loader;
+


nit: new line

ccaraman · 2017-04-12T16:34:39Z

test/tools/router_check/router.h

+ /**
+ * @param router_config_json specifies the router config json file
+ * @return bool if json file loaded successfully and ConfigImpl object created
+ * successfully


nit: spacing. Can you aline successfully under bool? like https://github.com/lyft/envoy/blob/master/include/envoy/http/async_client.h#L133-L135

ccaraman · 2017-04-12T16:39:43Z

test/tools/router_check/test/route_tests.sh

+# Testing expected matches
+for t in "${TESTS[@]}"
+do
+ TEST_OUTPUT=$("${PATH_BIN}" "${PATH_CONFIG}/${t}.json" "${PATH_CONFIG}/${t}.golden.json" "--details")


Does the "golden" prefix for a file mean the test case?

Yes, that's correct.

ccaraman · 2017-04-12T16:42:40Z

test/tools/router_check/router.cc

+}
+
+bool RouterCheckTool::initializeFromConfig(const std::string& router_config_json) {
+


Please add a TODO to load a full config and extract the route configuration from it. I think that will be useful in the future so users can just pass in their existing config.

ccaraman · 2017-04-12T16:46:17Z

test/tools/router_check/router.cc

+
+bool RouterCheckTool::compareCluster(ToolConfig& tool_config, const std::string& expected) {
+ std::string actual = "";
+ if (tool_config.route_->routeEntry() != nullptr) {


nit: keep new line spacing consistent across Compare Methods. either had a new line here or remove the new lines in the other compare methods

ccaraman · 2017-04-12T16:46:42Z

test/tools/router_check/router.h

+ * @param config_json specifies the json config file to be loaded
+ * @param schema is the json schema to validate against
+ * @return Json::ObjectPtr pointer to router config json object. Return
+ * nullptr if load fails.


nit: spacing

ccaraman · 2017-04-12T16:48:52Z

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

+ "additional_headers": [
+ {
+ "field": "some_header",
+ "value": "some_cluster"


nit: spacing

ccaraman · 2017-04-12T16:50:55Z

test/tools/router_check/test/route_tests.sh

+
+# Failure test case
+FAILURE_OUTPUT=$("${PATH_BIN}" "${PATH_CONFIG}/TestRoutes.json" "${PATH_CONFIG}/Weighted.golden.json" "--details") ||
+ if [[ "${FAILURE_OUTPUT}" == *"cluster1 instant-server cluster_name"* ]]; then


why is there a reference to a test case output for cluster1?

This shell script exercises the route table test tool -- a test for the test tool. In this test, I mixed and matched the TestRoutes router config with the Weighted test tool config. In that case there will be failure test cases. Rather than matching on all the failure test cases, I am checking that a specific cluster_name match test case failed. The failed test case has cluster1 as the expected cluster name and instant-server as the actual cluster name returned by the router.

hennna · 2017-04-12T19:14:31Z

@ccaraman Thanks for taking the time to review! Does this PR seem to be in a good state to merge?

I've added the TODO as a comment and also in #730.

mattklein123 · 2017-04-12T19:16:16Z

@hennna I will take a look through in a few hours.

mattklein123 · 2017-04-12T19:55:19Z

docs/configuration/tools/router_check.rst

+ ":authority":"api.lyft.com",
+ ":path": "/api/locations"
+ }
+ "_validate"


out of curiosity why does validate start with a _ ?

To show that it is a special parameter. This was in response to an earlier comment.

Now I realize that the comment recommended __ (two underscores). Is there a preference for two, one, or no prefix?

I don't know the original comment. To me it seems odd to have any underscore, but I don't care that much. I would poll some folks sitting by you.

mattklein123 · 2017-04-12T19:57:39Z

source/common/router/config_impl.cc

@@ -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 &&


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?

mattklein123

@hennna thanks a lot, this is awesome. Just some high level style comments. I know this is "just a test tool" but it's a good learning experience for writing good prod side code also. Just a few fixes and I think we are good to merge.

mattklein123 · 2017-04-12T20:04:56Z

test/tools/router_check/json/tool_config_schemas.h

+namespace Json {
+
+class ToolSchema {
+


nit: del newline

mattklein123 · 2017-04-12T20:07:13Z

test/tools/router_check/router.cc

+ const std::string& schema) {
+
+ try {
+ // Json configuration


general coding style preference: There are a lot of comments in this change that are obvious. For example if you have something like this:

// do something cool
doSomethingCool();

There is no point in having a comment. Can you go through and delete comments that fall into that category (there are many).

mattklein123 · 2017-04-12T20:08:45Z

test/tools/router_check/router_check.cc

@@ -0,0 +1,29 @@
+#include "test/precompiled/precompiled_test.h"


this should be included for you, delete

mattklein123 · 2017-04-12T20:08:53Z

test/tools/router_check/router_check.cc

+#include "test/tools/router_check/router.h"
+
+int main(int argc, char* argv[]) {
+


nit: del newline

mattklein123 · 2017-04-12T20:09:49Z

test/tools/router_check/router.cc

+}
+
+bool RouterCheckTool::compareEntriesInJson(const std::string& expected_route_json) {
+ if (config_ == nullptr) {


Can this happen? Please delete error handling that can't happen.

mattklein123 · 2017-04-12T20:11:06Z

test/tools/router_check/router.cc

+ Json::ObjectPtr loader = Json::Factory::LoadFromFile(config_json);
+ loader->validateSchema(schema);
+ return loader;
+ } catch (const EnvoyException& ex) {


You have quite a bit of extra error handling after you catch this exception. Embrace exceptions, they are awesome. In this case, I would have a top level try/catch in main(), and catch EnvoyException, print an error, and exit(1). If you do this, you can get rid of quite a bit of extraneous error handling in the code.

mattklein123 · 2017-04-12T20:11:36Z

test/tools/router_check/router.cc

+ // configuration from it.
+ Json::ObjectPtr loader = loadJson(router_config_json, Json::Schema::ROUTE_CONFIGURATION_SCHEMA);
+
+ if (loader != nullptr) {


this is an example of error checking you can get rid of if you allow exceptions to bubble up. There is more.

mattklein123 · 2017-04-12T20:12:34Z

test/tools/router_check/router.cc

+ for (const Json::ObjectPtr& check_config : loader->asObjectArray()) {
+ // Load parameters from json
+ ToolConfig tool_config;
+ tool_config.parseFromJson(check_config);


Just make constructor take check_config and throw exception if there is an issue. No need for other function.

It seems folks around here aren't comfortable with exceptions being thrown in constructors. I've updated the code to follow the model in https://github.com/lyft/envoy/blob/master/source/common/network/cidr_range.h#L95 using static factory methods to create instances of ToolConfig and RouterCheckTool.

It's not worth arguing over this case, but throwing exceptions from constructors is how errors are raised from constructors. In an RAII world, cleanup, etc. happen properly. If you want to have a static creation function feel free but it's not required.

mattklein123 · 2017-04-12T20:14:43Z

test/tools/router_check/router.h

+
+ bool details_{false};
+
+ // TODO(hennna): Switch away from mocks depending on whether envoy testing


I would remove this TODO. Not sure why we would switch away from mocks? Or do you mean the work that @rlazarus is doing?

mattklein123 · 2017-04-12T20:15:00Z

test/tools/router_check/router.h

+
+#include "test/mocks/runtime/mocks.h"
+#include "test/mocks/upstream/mocks.h"
+#include "test/precompiled/precompiled_test.h"


del (might be other instances)

mattklein123

very nice work, thanks

Description: this job keeps timing out #634 for long term solution Risk Level: low Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

htuch reviewed Apr 4, 2017

View reviewed changes

dnoe reviewed Apr 4, 2017

View reviewed changes

hennna added 4 commits April 5, 2017 21:53

Router check tool with two input json files

713c585

Fixup docs

3645c0b

Response to comments

40b3394

Cleanup

21c8d8c

htuch reviewed Apr 6, 2017

View reviewed changes

hennna added 4 commits April 6, 2017 10:56

Fix build issue and clean up

cba5961

Merge with upstream

6207e11

Merge remote-tracking branch 'upstream/master'

3db3bba

Refactor and cleanup

3c02c3e

hennna force-pushed the url-cluster-match branch from bbbd9ff to 3c02c3e Compare April 6, 2017 17:32

htuch reviewed Apr 6, 2017

View reviewed changes

Response to implementation comments

34d1dcb

htuch reviewed Apr 7, 2017

View reviewed changes

mattklein123 requested review from ccaraman and rshriram April 7, 2017 16:31

Merge branch 'master' into url-cluster-match

50ac661

ccaraman reviewed Apr 7, 2017

View reviewed changes

ccaraman reviewed Apr 12, 2017

View reviewed changes

Response to comments

f8d6fe7

Fix spacing in json configs

5227402

mattklein123 reviewed Apr 12, 2017

View reviewed changes

hennna added 3 commits April 17, 2017 11:29

Merge 'upstream/master' into url-cluster-match

479055a

Response to comments

f150d22

Remove extra newline

0c6554f

mattklein123 approved these changes Apr 17, 2017

View reviewed changes

mattklein123 merged commit 70c5bc9 into envoyproxy:master Apr 17, 2017

hennna deleted the url-cluster-match branch April 18, 2017 14:41

mattklein123 mentioned this pull request Apr 18, 2017

router: support url matching regression testing #538

Closed

		@@ -0,0 +1,98 @@
		.. _config_tools_router_check_tool:

		Router check tool input interface


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

		@@ -12,6 +12,23 @@ envoy_cc_test_library(
		name = "router_check_main_lib",


		bool details_{false};

		// TODO(hennna): Switch away from mocks depending on feedback



		additional_headers
		(optional, array) Additional headers to be added as input for route determination. The ":authority",

		}

		bool RouterCheckTool::initializeFromConfig(const std::string& router_config_json) {

		@@ -0,0 +1,29 @@
		#include "test/precompiled/precompiled_test.h"

		#include "test/tools/router_check/router.h"

		int main(int argc, char* argv[]) {

Router check tool with two input json files #682

Router check tool with two input json files #682

Conversation

hennna commented Apr 3, 2017 • edited Loading

htuch left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hennna Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccaraman Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccaraman Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

ccaraman Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hennna Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hennna commented Apr 3, 2017 •

edited

Loading

htuch left a comment •

edited

Loading

htuch Apr 6, 2017 •

edited

Loading

hennna Apr 6, 2017 •

edited

Loading

htuch Apr 7, 2017 •

edited

Loading

ccaraman Apr 7, 2017 •

edited

Loading

ccaraman Apr 7, 2017 •

edited

Loading

ccaraman Apr 7, 2017 •

edited

Loading

hennna Apr 11, 2017 •

edited

Loading

hennna Apr 12, 2017 •

edited

Loading