http: add Kill Request HTTP filter by qqustc · Pull Request #14170 · envoyproxy/envoy

qqustc · 2020-11-24T21:46:43Z

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

repokitteh-read-only · 2020-11-24T21:46:49Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14170 was opened by qqustc.

see: more, trace.

Signed-off-by: Qin Qin <qqin@google.com>

htuch · 2020-11-24T23:19:34Z

docs/root/configuration/http/http_filters/kill_request_filter.rst

+===============
+
+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()*.


Nit: s/extenstion/extension/

htuch · 2020-11-24T23:21:06Z

source/extensions/all_extensions.bzl

    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]


Can you refactor so we only have to add the filter once to the denylist?

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

Just create a GLOBAL_DENYLIST definition at the top and use it in both places.

htuch · 2020-11-24T23:22:22Z

source/extensions/extensions_build_config.bzl

    "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",


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?

Thanks Harvey. Added comment.

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.

Thanks Harvey! Added the filter to DISABLED_BY_DEFAULT_EXTENSIONS.

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.

ah :) thank you Harvey for the explanation! Removed the filter from EXTENSIONS and updated the consumers to make the CI doc test pass. PTAL

htuch · 2020-11-24T23:24:02Z

test/config/integration/google_com_proxy_port_0_with_kill_request_filter.yaml

+                route: { host_rewrite_literal: www.google.com, cluster: service_google }
+          http_filters:
+          - name: envoy.filters.http.kill_request
+            typed_config:


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?

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.

Or place in test/exe/testdata.

Thanks Harvey. Moved this file to test/exe/testdata and also made it simplified

Signed-off-by: Qin Qin <qqin@google.com>

qqustc · 2020-11-25T14:25:53Z

/retest-circle

qqustc · 2020-11-25T14:26:20Z

/retest

repokitteh-read-only · 2020-11-25T14:26:24Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14170 (comment) was created by @qqustc.

see: more, trace.

Signed-off-by: Qin Qin <qqin@google.com>

htuch

LGTM modulo two small nits

htuch · 2020-11-30T21:43:43Z

source/extensions/all_extensions.bzl

-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()]


Nit: DISABLED_BY_DEFAULT.keys()

htuch · 2020-11-30T21:45:52Z

test/extensions/extensions_build_system.bzl

        extension_name,
        **kwargs):
-    if not extension_name in EXTENSIONS:
+    if not extension_name in EXTENSIONS and not extension_name in DISABLED_BY_DEFAULT_EXTENSIONS:


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

Signed-off-by: Qin Qin <qqin@google.com>

qqustc · 2020-11-30T22:15:01Z

LGTM modulo two small nits

Thank you Harvey for the review!

htuch

LGTM modulo tiniest nit.

htuch · 2020-11-30T22:45:46Z

source/extensions/all_extensions.bzl

 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()]


I mean you can just write: GLOBAL_DENYLIST = DISABLED_BY_DEFAULT_EXTENSIONS.keys() :)

Signed-off-by: Qin Qin <qqin@google.com>

htuch

LGTM, thanks!

qqustc · 2020-12-01T01:50:30Z

/retest

repokitteh-read-only · 2020-12-01T01:50:33Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #14170 (comment) was created by @qqustc.

see: more, trace.

qqustc · 2020-12-01T01:54:01Z

LGTM, thanks!

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>

repokitteh-read-only bot added the api label Nov 24, 2020

implement

bf6d7e9

Signed-off-by: Qin Qin <qqin@google.com>

qqustc force-pushed the kr4 branch from a20fef8 to bf6d7e9 Compare November 24, 2020 21:47

fix doc

5799de7

Signed-off-by: Qin Qin <qqin@google.com>

qqustc force-pushed the kr4 branch from 9b67260 to 5799de7 Compare November 24, 2020 22:11

qqustc mentioned this pull request Nov 24, 2020

http: add Kill Request HTTP filter #14039

Closed

htuch reviewed Nov 24, 2020

View reviewed changes

zuercher assigned htuch Nov 24, 2020

update

2fedcf8

Signed-off-by: Qin Qin <qqin@google.com>

qqustc force-pushed the kr4 branch from a9adeca to 2fedcf8 Compare November 25, 2020 03:07

fix build rule

34a1853

Signed-off-by: Qin Qin <qqin@google.com>

qqustc force-pushed the kr4 branch from 2de47a3 to 34a1853 Compare November 25, 2020 03:22

qqustc requested a review from htuch November 25, 2020 03:38

fix

a93c2f1

Signed-off-by: Qin Qin <qqin@google.com>

qqustc force-pushed the kr4 branch from 50d391d to a93c2f1 Compare November 25, 2020 22:41

fix doc

5865258

Signed-off-by: Qin Qin <qqin@google.com>

qqustc force-pushed the kr4 branch from 27240ad to 5865258 Compare November 26, 2020 00:02

fix doc

7ca49c2

Signed-off-by: Qin Qin <qqin@google.com>

qqustc force-pushed the kr4 branch from 07f5c56 to 7ca49c2 Compare November 30, 2020 16:34

htuch suggested changes Nov 30, 2020

View reviewed changes

fix

d3e0546

Signed-off-by: Qin Qin <qqin@google.com>

qqustc force-pushed the kr4 branch from daa6ade to d3e0546 Compare November 30, 2020 22:13

qqustc requested a review from htuch November 30, 2020 22:15

htuch suggested changes Nov 30, 2020

View reviewed changes

fix

410ec40

Signed-off-by: Qin Qin <qqin@google.com>

qqustc force-pushed the kr4 branch from c322040 to 410ec40 Compare November 30, 2020 23:20

qqustc requested a review from htuch November 30, 2020 23:21

htuch approved these changes Nov 30, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Nov 30, 2020

htuch merged commit 237b29d into envoyproxy:master Dec 1, 2020

qqustc deleted the kr4 branch December 4, 2020 21:23

Conversation

qqustc commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Nov 24, 2020

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!

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!

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!

qqustc commented Nov 25, 2020

Uh oh!

qqustc commented Nov 25, 2020

Uh oh!

repokitteh-read-only bot commented Nov 25, 2020

Uh oh!

htuch 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!

qqustc commented Nov 30, 2020

Uh oh!

htuch 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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

qqustc commented Dec 1, 2020

Uh oh!

repokitteh-read-only bot commented Dec 1, 2020

Uh oh!

qqustc commented Dec 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

qqustc commented Nov 24, 2020 •

edited

Loading