config: remove redundant checks and add schema tests by danielhochman · Pull Request #715 · envoyproxy/envoy

danielhochman · 2017-04-06T23:08:26Z

JSON Schema validation obviates the need for many of the configuration checks we were previously doing in code.

Fixes #454

TODO:

replace deleted tests with schema tests for same condition

mattklein123 · 2017-04-06T23:46:24Z

@ccaraman can do the review, but 2 high level comments:

I'm not sure I would delete the tests. They still provide schema coverage, even if it's not explicit.
You probably already did this, but make sure you take a look at the coverage build to see if there is anything missing that should also be removed or tested.

ccaraman · 2017-04-07T18:20:40Z

source/server/configuration_impl.cc

-      throw EnvoyException(fmt::format("unknown rate limit service type '{}'", type));
-    }
+    ASSERT(type == "grpc_service");
+    ratelimit_client_factory_.reset(new RateLimit::GrpcFactoryImpl(


You'll most likely get the unused param for type. add UNREFERENCED_PARAMETER

ccaraman · 2017-04-07T23:02:00Z

source/server/configuration_impl.cc

+    ASSERT(type == "grpc_service");
+    ratelimit_client_factory_.reset(new RateLimit::GrpcFactoryImpl(
+        *rate_limit_service_config->getObject("config"), *cluster_manager_));
+


nit:no new line

ccaraman · 2017-04-07T23:03:14Z

source/common/http/access_log/access_log_impl.cc

-    }
-  } else {
-    throw EnvoyException(fmt::format("Filter list cannot be empty in OperatorFilter"));
+  for (Json::ObjectPtr& filter : filters) {


nit: this can be done in one line with getObjectArray

…cleanup

RomanDzhabarov · 2017-04-10T04:06:16Z

source/server/configuration_impl.cc

-    }
+    ASSERT(type == "grpc_service");
+    ratelimit_client_factory_.reset(new RateLimit::GrpcFactoryImpl(
+        *rate_limit_service_config->getObject("config"), *cluster_manager_));


you also need to fix line "throw EnvoyException(fmt::format("unsupported driver type: '{}'", type));" down below.

RomanDzhabarov · 2017-04-10T04:13:13Z

With the schema introduction and ipv4/ipv6 format, is this still possible? https://github.com/lyft/envoy/blob/cec2bdfbe86870f654ac8697d0042a120e827e3e/source/common/network/utility.cc#L18
if not we need to clean up

RomanDzhabarov · 2017-04-10T04:28:35Z

source/common/http/filter/fault_filter.cc

+  const Json::ObjectPtr config_delay = json_config.getObject("delay", true);

-  if (abort->empty() && delay->empty()) {
+  if (config_abort->empty() && config_delay->empty()) {


i think, we can get rid of this condition if we say oneOf for "abort" or "delay" in the json schema.

potentially same thing with the route rules, where it's either prefix or path needs to be supplied:

bool has_prefix = route->hasObject("prefix"); bool has_path = route->hasObject("path"); if (!(has_prefix ^ has_path)) { throw EnvoyException("routes must specify either prefix or path"); }

yeah, that should work for abort or delay case. don't think that it will work for the route rules since there are two xor keys among a lot of other keys.

actually there are other keys on the same level for abort and delay as well. so i don't think we can use the schema to validate this condition.

danielhochman · 2017-04-10T16:52:21Z

@RomanDzhabarov i looked at the ipv4 validator previously and from what i can tell it won't understand the mask portion, just dot-quad

got it, thanks for checking

…cleanup

mattklein123

very cool. Mainly comments around docs and maintainability.

mattklein123 · 2017-04-18T01:25:52Z

test/common/json/test_data/generate_test_data.py

+
+def write_test_file(name, schema, data, throws):
+    test_filename = os.path.join(
+        os.environ['TEST_TMPDIR'],


To avoid clashing in the coverage test, can we use a subdir of TEST_TMPDIR. E.g., ${TEST_TMPDIR}/json_schema_test.

mattklein123 · 2017-04-18T01:27:12Z

test/common/json/test_data/generate_test_data.py

@@ -0,0 +1,339 @@
+#!/usr/bin/env python


This file is going to get huge and hard to maintain. At minimum, can we add comments for folks on best practices on adding a new schema test (best would be comments in here and some type of README.me). Better I think would be to break this program into multiple Python files.

mattklein123 · 2017-04-18T01:30:15Z

test/common/json/config_schemas_test.cc

+
+namespace Json {
+
+std::vector<std::string> generateTestInputs() {


This is basically the same code that is used in example_config_tests. It might be worth it to factor it out so that we can use it in both places (basically func to return a list of JSON that can be fed into TEST_P).

mattklein123 · 2017-04-18T01:32:34Z

test/common/json/config_schemas_test.cc

+  if (json->getBoolean("throws", false)) {
+    EXPECT_THROW(json->getObject("data")->validateSchema(schema), Exception);
+  } else {
+    json->getObject("data")->validateSchema(schema);


I might try/catch here, catch exception, fail, and then print some info about what failed (which test file, etc.), since I think it's going to be almost impossible to figure it out if something starts failing.

mattklein123

this is awesome, thanks. 2 small nits.

mattklein123 · 2017-04-18T20:20:18Z

test/config_test/config_test.cc

-    Logger::Registry::getLog(Logger::Id::testing).info("testing config: {}", file_name);
-    ConfigTest config(file_name);
+  uint32_t num_tested = 0;
+  for (std::string filename : TestUtility::listFiles(directory, true)) {


const std::string&

mattklein123 · 2017-04-18T20:20:57Z

test/common/json/config_schemas_test_data/test_listener_schema.py

@@ -0,0 +1,24 @@
+


extra new line

Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of proxy This PR will be merged automatically once checks are successful. ```release-note none ```

Description: updates envoy ref to include #10137. Exposes client builder function to add dns failure refresh rate. Risk Level: low, new configuration option Testing: local apps, CI Docs Changes: created #715 to create a section in the docs for all the builder options. Fixes #673 Signed-off-by: JP Simard <jp@jpsim.com>

Daniel Hochman added 2 commits April 6, 2017 16:06

remove redundant configurations checks

246c373

fix format

3e0e484

ccaraman reviewed Apr 7, 2017

View reviewed changes

UNREFERENCED_PARAMETER

a90adfc

ccaraman reviewed Apr 7, 2017

View reviewed changes

Merge branch 'master' of github.com:lyft/envoy into schema-redundant-…

ae16018

…cleanup

RomanDzhabarov reviewed Apr 10, 2017

View reviewed changes

Daniel Hochman added 2 commits April 10, 2017 14:01

fix bazel build and address comments

27acfe1

fix_format

b249eb6

mattklein123 mentioned this pull request Apr 15, 2017

ssl: require non-empty "cipher_suites" in configuration. #776

Closed

Daniel Hochman added 4 commits April 17, 2017 11:27

Merge branch 'master' of github.com:lyft/envoy into schema-redundant-…

6ff9c71

…cleanup

framework for schema testing

9ed276a

replace all old tests with new schema tests

ad9b018

Merge branch 'master' of github.com:lyft/envoy into schema-redundant-…

7f5c712

…cleanup

mattklein123 reviewed Apr 18, 2017

View reviewed changes

frameworkify schema tests

2507227

danielhochman changed the title ~~remove redundant configuration checks~~ config: remove redundant checks and add schema tests Apr 18, 2017

Daniel Hochman added 2 commits April 18, 2017 12:29

better paths

70fa6e4

factor out ls into utility class

cbbde1a

mattklein123 reviewed Apr 18, 2017

View reviewed changes

review comments

cd39dc0

mattklein123 approved these changes Apr 18, 2017

View reviewed changes

mattklein123 merged commit 523f091 into master Apr 18, 2017

mattklein123 deleted the schema-redundant-cleanup branch April 18, 2017 22:33

mattklein123 mentioned this pull request Apr 18, 2017

Json Schema: Add tests for JSON Schemas #572

Closed

mathetake pushed a commit that referenced this pull request Mar 3, 2026

build(deps): bump github.com/openai/openai-go from 1.3.0 to 1.5.0 (#715)

cc0f15e


		namespace Json {

		std::vector<std::string> generateTestInputs() {

Conversation

danielhochman commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccaraman Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RomanDzhabarov commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhochman commented Apr 10, 2017 • edited by RomanDzhabarov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danielhochman commented Apr 6, 2017 •

edited

Loading

ccaraman Apr 7, 2017 •

edited

Loading

RomanDzhabarov commented Apr 10, 2017 •

edited

Loading

danielhochman commented Apr 10, 2017 •

edited by RomanDzhabarov

Loading