-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
http: add Kill Request HTTP filter #14170
Conversation
=============== | ||
|
||
The KillRequest filter can be used to crash Envoy when receiving a Kill request. | ||
By default, KillRequest filter is not built into Envoy binary since it is removed from *envoy_all_extensions()* in *all_extensions.bzl*. If you want to use this extenstion, please add it in *envoy_all_extensions()*. |
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.
Nit: s/extenstion/extension/
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.
Done
source/extensions/all_extensions.bzl
Outdated
@@ -37,7 +42,7 @@ _http_filter_prefix = "envoy.filters.http" | |||
def envoy_all_http_filters(): | |||
all_extensions = dicts.add(_required_extensions, EXTENSIONS) | |||
|
|||
return [v for k, v in all_extensions.items() if k.startswith(_http_filter_prefix)] | |||
return [v for k, v in all_extensions.items() if k.startswith(_http_filter_prefix) and k != _kill_request_http_filter] |
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.
Can you refactor so we only have to add the filter once to the denylist?
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.
I might be wrong, but I don't see a way to refactor to make it simpler since denylist is a function parameter in envoy_all_extensions()
. We always need some logic in envoy_all_http_filters
to filter out kill_request_http_filter
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.
Just create a GLOBAL_DENYLIST
definition at the top and use it in both places.
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.
Done
@@ -71,6 +71,7 @@ EXTENSIONS = { | |||
"envoy.filters.http.health_check": "//source/extensions/filters/http/health_check:config", | |||
"envoy.filters.http.ip_tagging": "//source/extensions/filters/http/ip_tagging:config", | |||
"envoy.filters.http.jwt_authn": "//source/extensions/filters/http/jwt_authn:config", | |||
"envoy.filters.http.kill_request": "//source/extensions/filters/http/kill_request:kill_request_config", |
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, you include here but then exclude later.. how does a user actually include this in a build? Can you comment on this to make it clearer?
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.
Thanks Harvey. Added comment.
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.
Do you mean all_extensions.bzl
in the comment? I still thinking adding it and then blocking by default is a bit weird.
Can we do this a bit differently and add a new set of definitions here, DISABLED_BY_DEFAULT_EXTENSIONS, that looks like the current structure but only has the kill filter. We can feed this definition into both all_extensions.bzl and also into the docs.
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.
Thanks Harvey! Added the filter to DISABLED_BY_DEFAULT_EXTENSIONS.
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.
This is much better. What I was hinting at ^^ is that we should have DISABLED_BY_DEFAULT_EXTENSIONS
as you've done, but also not have to include the kill request filter in EXTENSIONS
. This should be easy to do; you might need to modify some consumers, e.g. doc build, to consume from DISABLED_BY_DEFAULT_EXTENSIONS
. That way, you can tell folks to move their extension from DISABLED_BY_DEFAULT_EXTENSIONS
to EXTENSIONS
to enable.
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.
ah :) thank you Harvey for the explanation! Removed the filter from EXTENSIONS
and updated the consumers to make the CI doc test pass. PTAL
route: { host_rewrite_literal: www.google.com, cluster: service_google } | ||
http_filters: | ||
- name: envoy.filters.http.kill_request | ||
typed_config: |
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.
Why do you need a separate integration test config? Can't you just use helpers in https://github.com/envoyproxy/envoy/blob/master/test/config/utility.h to inject the filter on top of an existing config?
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.
I see.. you want to use it directly in main_common_test.cc
. I suggest dramatically minimizing this file and scoping the filegroup to just this one test.
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.
Or place in test/exe/testdata
.
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.
Thanks Harvey. Moved this file to test/exe/testdata
and also made it simplified
Signed-off-by: Qin Qin <qqin@google.com>
/retest-circle |
/retest |
Retrying Azure Pipelines: |
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.
LGTM modulo two small nits
source/extensions/all_extensions.bzl
Outdated
load("@envoy_build_config//:extensions_build_config.bzl", "EXTENSIONS") | ||
load("@envoy_build_config//:extensions_build_config.bzl", "DISABLED_BY_DEFAULT_EXTENSIONS", "EXTENSIONS") | ||
|
||
GLOBAL_DENYLIST = [k for k, v in DISABLED_BY_DEFAULT_EXTENSIONS.items()] |
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.
Nit: DISABLED_BY_DEFAULT.keys()
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.
Done
def envoy_extension_cc_test( | ||
name, | ||
extension_name, | ||
**kwargs): | ||
if not extension_name in EXTENSIONS: | ||
if not extension_name in EXTENSIONS and not extension_name in DISABLED_BY_DEFAULT_EXTENSIONS: |
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.
Nit: factor this out into some macro, e.g.
def extension_allowed_for_test(name):
return extension_name in EXTENSIONS or extension_name in DISABLED_BY_DEFAULT_EXTENSIONS
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.
Done
Thank you Harvey for the review! |
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.
LGTM modulo tiniest nit.
source/extensions/all_extensions.bzl
Outdated
@@ -1,7 +1,7 @@ | |||
load("@bazel_skylib//lib:dicts.bzl", "dicts") | |||
load("@envoy_build_config//:extensions_build_config.bzl", "DISABLED_BY_DEFAULT_EXTENSIONS", "EXTENSIONS") | |||
|
|||
GLOBAL_DENYLIST = [k for k, v in DISABLED_BY_DEFAULT_EXTENSIONS.items()] | |||
GLOBAL_DENYLIST = [k for k in DISABLED_BY_DEFAULT_EXTENSIONS.keys()] |
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.
I mean you can just write: GLOBAL_DENYLIST = DISABLED_BY_DEFAULT_EXTENSIONS.keys()
:)
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.
Done :)
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.
LGTM, thanks!
/retest |
Retrying Azure Pipelines: |
Thank you Harvey! All Required CI test has been passed. |
* master: (70 commits) upstream: avoid reset after end_stream in TCP HTTP upstream (envoyproxy#14106) bazelci: add fuzz coverage (envoyproxy#14179) dependencies: allowlist CVE-2020-8277 to prevent false positives. (envoyproxy#14228) cleanup: replace ad-hoc [0, 1] value types with UnitFloat (envoyproxy#14081) Update docs for skywalking tracer (envoyproxy#14210) Fix some errors in the switch statement when decode dubbo response (envoyproxy#14207) Windows: enable tests and envoy-static.exe pdb file (envoyproxy#13688) http: add Kill Request HTTP filter (envoyproxy#14170) dependencies: fix release_dates error behavior. (envoyproxy#14216) thrift filter: support skip decoding data after metadata in the thrift message (envoyproxy#13592) update cares (envoyproxy#14213) docs: clarify behavior of hedge_on_per_try_timeout (envoyproxy#12983) repokitteh: add support for randomized auto-assign. (envoyproxy#14185) [grpc] validate grpc config for illegal characters (envoyproxy#14129) server: Return nullopt when process_context is nullptr (envoyproxy#14181) [Windows] Fix thrift proxy tests (envoyproxy#13220) kafka: add missing unit tests (envoyproxy#14195) doc: mention gperftools explicitly in PPROF.md (envoyproxy#14199) Removed `--use-fake-symbol-table` option. (envoyproxy#14178) filter contract: clarification around local replies (envoyproxy#14193) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Add a KillRequest HTTP filter which can crash Envoy when receiving a Kill request. It will be used to fault inject kill request to Envoy and measure the blast radius.
The new KillRequest filter will be disabled at build time by default in Envoy by comment out the KillRequest filter extension in extensions_build_config.bzl
@htuch
PS: Original PR #14039 was closed due to being accidentally messed up.
Risk Level: Low, new feature.
Testing: Unit/integration tests.
Docs Changes: Added
Release Notes: Added
Issue: #13978