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.

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

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

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.

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

prefer ::exit(1);

etc

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.

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

setShowDetails

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

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.

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.

Envoy

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

"to be added as input to route determination."

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.

[":router_check_main_lib"]

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.

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.

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.

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

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.

--details

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.

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

Nit: if details_ flag is set.

Nit: actual

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

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.

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.

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

You could optionally combined roucher_check_main_lib and router_tool_lib.

Great, this is what I was talking about it.

Nit: the earlier line seems too shrot.

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)

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 ?

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.

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.

nit for router: add a link to the configuration.

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.

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

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

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.