JSON schema support and schema validation#365
Conversation
| Network::ReadFilterPtr{new Filter::Auth::ClientSsl::Instance(config)}); | ||
| }; | ||
|
|
||
| const std::string ClientSslAuthConfigFactory::CLIENT_SSL_SCHEMA( |
There was a problem hiding this comment.
Please use literal strings for all the schemas. It will be much easier to read.
include/envoy/json/json_object.h
Outdated
|
|
||
| /** | ||
| * Validates JSON object against passed in schema. | ||
| * @param schema supplies the string format of the schema. A Json::Exception will be thrown, if |
| GetParseError_En(document.GetParseError()))); | ||
| } | ||
|
|
||
| rapidjson::SchemaDocument schema_document(document); |
There was a problem hiding this comment.
Don't you need to check some type of error here and throw if the schema itself is not valid?
There was a problem hiding this comment.
The schema is 'validated' by being converted into a rapidjson::Document in line 159. Line 159 ensures that is a properly formatted json object. There are no other checks for a proper json schema.
There was a problem hiding this comment.
This can't be the case / doesn't make sense. You can have a valid JSON document which is not a valid schema. So there must be a way of catching an invalid schema error somewhere, at the very least when you do line 169. I would be shocked if there is no way to check that the schema is valid syntax.
There was a problem hiding this comment.
I've added a test with an invalid schema that is in proper json format.
|
|
||
| json_config.validateSchema(CLIENT_SSL_SCHEMA); | ||
|
|
||
| Filter::Auth::ClientSsl::ConfigPtr config(Filter::Auth::ClientSsl::Config::create( |
There was a problem hiding this comment.
The schema validation should be done closer to the relevant config code, so that unit tests are covered also. For example, in this case, it should be done inside Filter::Auth::ClientSsl::Config constructor. (Same applies for all the other cases). As long as you already did the h/cc splits for the filters, please keep that, and make sure all of the config stuff has code coverage, since we were missing that previously.
There was a problem hiding this comment.
-All checks have been moved into the config constructor except for mongo proxy which does the check in network/mongo_proxy.cc
-Only bad config tests have been moved to the appropriate test files
| } | ||
| }, | ||
| "required": ["auth_api_cluster", "stat_prefix"], | ||
| "additionalProperties": false |
There was a problem hiding this comment.
Can you see if there is any way to have this be the default when the validator runs, vs. having to specify it each time (since we are always going to specify it).
There was a problem hiding this comment.
Unfortunately, there is no way to set it globally for schemas in rapidjson or in JSON schema.
source/server/configuration_impl.cc
Outdated
| "type": {"type" : "string", "enum" :["read", "write", "both"]}, | ||
| "name" : { | ||
| "type": "string", | ||
| "enum" : ["client_ssl_auth", "echo", "mongo_proxy", "ratelimit", "tcp_proxy", "http_connection_manager"] |
There was a problem hiding this comment.
Other people may compile in other filters. We can't specify the enum here.
test/common/json/json_loader_test.cc
Outdated
| } | ||
|
|
||
| TEST(JsonLoaderTest, Schema) { | ||
| ObjectPtr json1 = Factory::LoadFromString("{\"value1\": 10, \"value2\" : \"test\" }"); |
| "items" : { | ||
| "type": "string", | ||
| "format" : "ipv4" | ||
| } |
There was a problem hiding this comment.
You might want to enforce additional constrains here, such as
"uniqueItems" : true,Not sure if minItems:1 makes sense here.
There was a problem hiding this comment.
Good idea. Updated.
| "value" : {"type" : "string"} | ||
| }, | ||
| "additionalProperties": false | ||
| } |
There was a problem hiding this comment.
Same as above. uniqueItems: true and minItems: 1 ?
|
Thinking more about this, may I suggest a different approach? Benefits:
|
|
@rshriram we have been back and forth on this. @ccaraman can follow up more. The issue is that people will compile in filters that are not present in the open source, which makes a single schema difficult/impossible. I'm also not crazy about having the user specify the schema file on the command line and somehow shipping it to them. These doesn't seem very user friendly. The compromise that I discussed with @ccaraman is that all of the schema definitions will be inside a single CC file that we can document (we will likely eventually put this as a JSON file into the source distro and compile it into the exe). I think this will balance flexibility with discoverability. |
mattklein123
left a comment
There was a problem hiding this comment.
few small things otherwise looks good
include/envoy/json/json_object.h
Outdated
|
|
||
| /** | ||
| * Validates JSON object against passed in schema. | ||
| * @param schema supplies the schema in string format. A Json::Exception will be thrown, if |
There was a problem hiding this comment.
"A Json::Exception will be thrown if the JSON object doesn't conform to the supplied schema or the schema itself is not valid." (no comma after "will be thrown")
source/common/json/config_schemas.cc
Outdated
| } | ||
| )EOF"); | ||
|
|
||
| const std::string Json::Schema::CLIENT_SSL_SCHEMA(R"EOF( |
There was a problem hiding this comment.
CLIENT_SSL_NETWORK_FILTER_SCHEMA
source/common/json/config_schemas.cc
Outdated
| } | ||
| )EOF"); | ||
|
|
||
| const std::string Json::Schema::MONGO_PROXY_SCHEMA(R"EOF( |
There was a problem hiding this comment.
MONGO_PROXY_NETWORK_FILTER_SCHEMA
source/common/json/config_schemas.cc
Outdated
| } | ||
| )EOF"); | ||
|
|
||
| const std::string Json::Schema::RATELIMIT_SCHEMA(R"EOF( |
There was a problem hiding this comment.
RATELIMIT_NETWORK_FILTER_SCHEMA
| "items": { | ||
| "type": "object", | ||
| "properties": { | ||
| "key" : {"type" : "string"}, |
There was a problem hiding this comment.
made both key and value required
source/common/json/config_schemas.cc
Outdated
| } | ||
| )EOF"); | ||
|
|
||
| const std::string Json::Schema::REDIS_PROXY_SCHEMA(R"EOF( |
There was a problem hiding this comment.
REDIS_PROXY_NETWORK_FILTER_SCHEMA
source/common/json/config_schemas.cc
Outdated
| } | ||
| )EOF"); | ||
|
|
||
| const std::string Json::Schema::TCP_PROXY_SCHEMA(R"EOF( |
There was a problem hiding this comment.
TCP_PROXY_NETWORK_FILTER_SCHEMA
source/common/json/config_schemas.h
Outdated
| static const std::string LISTENER_SCHEMA; | ||
|
|
||
| // Network Filter Schemas | ||
| static const std::string CLIENT_SSL_SCHEMA; |
There was a problem hiding this comment.
From a naming perspective I mentioned to rename them to include NETWORK_FILTER. The other option is to move this into a sub-struct called NetworkFilter. Either way.
There was a problem hiding this comment.
Added 'NETWORK_FILTER' to all of these names and updated call sites.
source/server/configuration_impl.h
Outdated
| void createFilterChain(Network::Connection& connection) override; | ||
|
|
||
| private: | ||
| static const std::string LISTENER_SCHEMA; |
There was a problem hiding this comment.
removed. Missed this one :)
| /** | ||
| * Validates JSON object against passed in schema. | ||
| * @param schema supplies the schema in string format. A Json::Exception will be thrown, if | ||
| * @param schema supplies the schema in string format. A Json::Exception will be thrown if |
There was a problem hiding this comment.
I already commented on this, but the exception will also be thrown if the schema itself is not valid. Please update docs.
include/envoy/json/json_object.h
Outdated
| * Validates JSON object against passed in schema. | ||
| * @param schema supplies the schema in string format. A Json::Exception will be thrown if | ||
| * the JSON object doesn't conform to the supplied schema or the schema itself is not | ||
| * valid. |
* Added service_configs map to server config * Defined message ServiceConfigTrafficPercentage * Removed config_id from the ServerConfig * Removed deprecated comment * Removed the service_name * Applied service_config.proto changes, disabled config_manager
Use LTO for Wasm code. Benchmarks show that it results in noticeable improvement.
Kotlin unary/stremaing interfaces implementation Tests verify the interaction between the `Envoy` and the `EnvoyEngine` along with the underlying `EnvoyHTTPStream` objects Signed-off-by: Alan Chiu <achiu@lyft.com> For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md) Description: kotlin: implement unary and streaming interfaces Risk Level: low Testing: Unit Docs Changes: n/a Release Notes: n/a [Optional Fixes #Issue] [Optional Deprecated:] Signed-off-by: JP Simard <jp@jpsim.com>
Kotlin unary/stremaing interfaces implementation Tests verify the interaction between the `Envoy` and the `EnvoyEngine` along with the underlying `EnvoyHTTPStream` objects Signed-off-by: Alan Chiu <achiu@lyft.com> For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md) Description: kotlin: implement unary and streaming interfaces Risk Level: low Testing: Unit Docs Changes: n/a Release Notes: n/a [Optional Fixes #Issue] [Optional Deprecated:] Signed-off-by: JP Simard <jp@jpsim.com>
**Commit Message** Fix CSS styling for button placement and text size on the home page hero for small screens. Signed-off-by: Erica Hughberg <erica.sundberg.90@gmail.com>
Scheme validation has been implemented for listeners and the network filters. In addition, the network filters have been split into h/cc to improve testability.
Next PR will provide schema validation for HTTP Connection Manager Filter.