-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
JSON Schema: HTTP Filter schemas #405
Conversation
@lyft/network-team @rshriram |
4049585
to
35dc4a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so awesome, just a small comment.
const Json::ObjectPtr& abort = json_config.getObject("abort"); | ||
json_config.validateSchema(Json::Schema::FAULT_HTTP_FILTER_SCHEMA); | ||
|
||
const Json::ObjectPtr& abort = json_config.getObject("abort", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reference here and next line (you are storing reference to temporary).
const std::string& stats_prefix, | ||
Server::Instance& server) { | ||
if (type != HttpFilterType::Decoder || name != "buffer") { | ||
return nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
silly question: is the name check necessary here? Is there a possibility that the code that instantiates filters calls this factory method without matching the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, shouldn't every filter be checking the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, I take it back.. Everything is checking for the name.
const std::string& stat_prefix, | ||
Server::Instance& server) { | ||
if (type != HttpFilterType::Both || name != "http_dynamo_filter") { | ||
return nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this pattern is needed, then I think fault and rate limit are missing the name checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are checked in config/http/(fault.cc/ratelimit.cc)
envoyproxy#405) non-reentrant). Signed-off-by: John Plevyak <[email protected]>
envoyproxy#405) non-reentrant). Signed-off-by: John Plevyak <[email protected]>
* Move proxy_on_delete() call out of proxy_done() call from the VM (make (envoyproxy#405) non-reentrant). Signed-off-by: John Plevyak <[email protected]> * Simplify Wasm shutdown. (envoyproxy#410) Signed-off-by: John Plevyak <[email protected]> Co-authored-by: John Plevyak <[email protected]>
…ward_proxy_filter.rst (envoyproxy#405) * feat(dynamic_forward_proxy_filter): first draft * fix(dynamic_forward_proxy_filter): revision * fix(dynamic_forward_proxy_filter): compile problem * fix(dynamic_forward_proxy_filter): compile problem * fix(dynamic_forward_proxy_filter): improve coherence Co-authored-by: 李景炤 <[email protected]>
Description: previously onError was not delivering the callback all the way to the platform. This PR wires functionality all the way and also converts envoy_error to EnvoyError. Risk Level: med - implementing core functionality Testing: built locally Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
Description: previously onError was not delivering the callback all the way to the platform. This PR wires functionality all the way and also converts envoy_error to EnvoyError. Risk Level: med - implementing core functionality Testing: built locally Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
-Breaks http configs into h/cc
-Increases tests coverage for http configs by adding server/config/http/config_test.cc
-Adds schema validation for applicable HTTP filters.
-Updates Fault Filter documentation to specify at least abort or delay must be specified in the config. The implementation has been updated to reflect expected behaviour.
-Added empty() method on JSON Objects.