network: Enable the Kill Request filter to honor Route-level configur… #14944

xiaobinxbw · 2021-02-04T18:07:30Z

network: Enable the Kill Request filter to honor Route-level configuration. (#14929)

Signed-off-by: Tommy Wang xiaobinwang@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

…ation. (envoyproxy#14929) Signed-off-by: Tommy Wang <xiaobinwang@google.com>

repokitteh-read-only · 2021-02-04T18:07:33Z

Hi @xiaobinxbw, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #14944 was opened by xiaobinxbw.

see: more, trace.

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

htuch

LGTM, thanks!

htuch · 2021-02-05T00:54:41Z

source/extensions/filters/http/kill_request/kill_request_filter.cc

@@ -29,6 +37,20 @@ Http::FilterHeadersStatus KillRequestFilter::decodeHeaders(Http::RequestHeaderMa
    return Http::FilterHeadersStatus::Continue;
  }

+  // Route-level configuration overrides filter-level configuration.
+  if (decoder_callbacks_->route() && decoder_callbacks_->route()->routeEntry()) {
+    const std::string& name = Extensions::HttpFilters::HttpFilterNames::get().KillRequest;


I think we eventually want to match with the actual name used in the filter instantiation, but this hasn't been fixed Envoy-wide to my knowledge (#12575).

xiaobinxbw · 2021-02-05T00:59:55Z

Thanks Harvey! In that case, could you please help me to click on the "Merge pull request" button? I am not authorised to merge :) Thanks, Tommy

…

On Thu, Feb 4, 2021 at 4:56 PM htuch ***@***.***> wrote: ***@***.**** approved this pull request. LGTM, thanks! ------------------------------ In source/extensions/filters/http/kill_request/kill_request_filter.cc <#14944 (comment)>: > @@ -29,6 +37,20 @@ Http::FilterHeadersStatus KillRequestFilter::decodeHeaders(Http::RequestHeaderMa return Http::FilterHeadersStatus::Continue; } + // Route-level configuration overrides filter-level configuration. + if (decoder_callbacks_->route() && decoder_callbacks_->route()->routeEntry()) { + const std::string& name = Extensions::HttpFilters::HttpFilterNames::get().KillRequest; I think we eventually want to match with the actual name used in the filter instantiation, but this hasn't been fixed Envoy-wide to my knowledge (#12575 <#12575>). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14944 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASW52MXFKPK6X3ZUUIPHTRTS5M64RANCNFSM4XDJK7TQ> .

htuch · 2021-02-05T15:25:20Z

@xiaobinxbw the coverage CI failure looks legit. Is there any way to bump test coverage?

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

xiaobinxbw · 2021-02-05T22:28:48Z

Hey Harvey,

I've added another use case which should cover the branches in my added code. I am not sure why it's still complaining about the coverage. Is there any way I can generate a coverage report? Sorry for the dumb question as it's my very first oss work :)

btw, i accidentally closed the pull request and reopened it. I will need your help with the approval again.

Thanks,
Tommy

htuch · 2021-02-07T00:38:16Z

test/extensions/filters/http/kill_request/kill_request_filter_test.cc

@@ -102,6 +102,20 @@ TEST_F(KillRequestFilterTest,
  EXPECT_DEATH(filter_->decodeHeaders(request_headers_, false), "");
 }

+TEST_F(KillRequestFilterTest,
+       KillRequestDisabledWhenIsKillRequestEnabledReturnsFalseFromRouteLevelConfiguration) {


These names for tests are kind of unreadably long. Can you replace them with a shorter name and then add a single line // comment above providing the intuition that is currently captured in the name? Here and elsewhere in these tests. Thanks.

htuch · 2021-02-07T00:41:22Z

@xiaobinxbw you can see the coverage report at https://storage.googleapis.com/envoy-pr/f7c1716/coverage/source/extensions/filters/http/kill_request/kill_request_filter.cc.gcov.html, which you get to by clicking through to the CI details and selecting the GCS link under "linux_x64 coverage" -> "Upload coverage report". You can see it's missing functional coverage of the behaviors in this PR.

To generate reports locally, follow https://github.com/envoyproxy/envoy/blob/main/bazel/README.md#coverage-builds.

xiaobinxbw · 2021-02-09T01:21:43Z

@xiaobinxbw you can see the coverage report at https://storage.googleapis.com/envoy-pr/f7c1716/coverage/source/extensions/filters/http/kill_request/kill_request_filter.cc.gcov.html, which you get to by clicking through to the CI details and selecting the GCS link under "linux_x64 coverage" -> "Upload coverage report". You can see it's missing functional coverage of the behaviors in this PR.

To generate reports locally, follow https://github.com/envoyproxy/envoy/blob/main/bazel/README.md#coverage-builds.

Thanks a lot for the instructions, Harvey.

However, the two test cases actually covers the red part in the report, and i am not sure why the coverage report doesn't think this way. My guess is that the decoder_filter_callbacks_.route_->route_entry_ call is mocked, and the actually killing part (raise(SIGABRT);) is hijacked?

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

xiaobinxbw · 2021-02-09T18:11:07Z

Hey Harvey,

Just a note that i've run the dependency check locally with the command "ci/run_envoy_docker.sh 'ci/do_ci.sh deps'", and it succeeded. I don't think the latest test comment change could break the dependencies either. I haven't found a way to re-trigger this presubmit check without committing a code change, though.

Thanks,
Tommy

htuch · 2021-02-10T14:04:35Z

@xiaobinxbw are you sure those paths are executed? I.e. verified with log messages and the tests. There may be some coverage corner cases with death tests IIRC, but just want to check you have validated that those paths are taken in the tests you describe.

xiaobinxbw · 2021-02-10T20:17:10Z

Hey Harvey, Yes, I've done some experiments with the tests, and I am sure that the red paths in the coverage report are actually covered. It's a bit tricky to tell as adding extra logs to the paths doesn't tell the truth either. Here is what I did: 1. in source/extensions/filters/http/kill_request/kill_request_filter.cc, comment out the "raise(SIGABRT)" line #56 and run the test. Now all the tests that are supposed to crash envoy fail, which means the block from line #54 to #57 in kill_request_filter.cc are covered. 2. in source/extensions/filters/http/kill_request/kill_request_filter.cc, add "is_kill_request = false;" inside the "if (per_route_kill_settings != nullptr)" block at line #49. Now the new test KillRequestEnabledFromRouteLevelConfiguration fails, which is as expected as we are mocking a non-null per_route_kill_settings value and expect the kill_request_ to be assigned a value from the per-route config. This means the block from line #48 to #51 in kill_request_filter.cc are also covered. Hope this makes sense :) Thanks, Tommy

…

On Wed, Feb 10, 2021 at 6:04 AM htuch ***@***.***> wrote: @xiaobinxbw <https://github.com/xiaobinxbw> are you sure those paths are executed? I.e. verified with log messages and the tests. There may be some coverage corner cases with death tests IIRC, but just want to check you have validated that those paths are taken in the tests you describe. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14944 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASW52MX5B22LYOMRTVXB7L3S6KHAJANCNFSM4XDJK7TQ> .

htuch

LGTM modulo comments and adding a coverage exception to https://github.com/envoyproxy/envoy/blob/main/test/per_file_coverage.sh#L5. I'm pretty sure that there was some issue around coverage and death tests from ages ago but I can't recall (maybe @dnoe remembers).

htuch · 2021-02-12T04:49:58Z

source/extensions/filters/http/kill_request/kill_request_filter.h

+    return kill_request_filter_headers_;
+  }
+
+  const envoy::type::v3::FractionalPercent getProbability() const {


This is a bit weird. Why not just make return type const envoy::type::v3::FractionalPercent& and skip the move?

htuch · 2021-02-12T04:50:44Z

source/extensions/filters/http/kill_request/kill_request_filter.cc

+        route_entry->mostSpecificPerFilterConfigTyped<KillSettings>(name);
+
+    if (per_route_kill_settings) {
+      envoy::type::v3::FractionalPercent probability = per_route_kill_settings->getProbability();


Nit: const envoy::type::v3::FractionalPercent probability = = ...

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

htuch · 2021-02-17T03:26:28Z

@xiaobinxbw looks good, can you get CI format to pass? Thanks. You will also need to add the coverage exception I mention in #14944 (review)

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

xiaobinxbw · 2021-02-17T04:18:04Z

@xiaobinxbw looks good, can you get CI format to pass? Thanks. You will also need to add the coverage exception I mention in #14944 (review)

@htuch Done!

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

htuch

LGTM, thanks!

network: Enable the Kill Request filter to honor Route-level configur…

e4584fa

…ation. (envoyproxy#14929) Signed-off-by: Tommy Wang <xiaobinwang@google.com>

xiaobinxbw requested a review from htuch as a code owner February 4, 2021 18:07

Fix format.

acfcf75

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

htuch previously approved these changes Feb 5, 2021

View reviewed changes

Add test coverage for kill_request.

f7c1716

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

xiaobinxbw dismissed htuch’s stale review via f7c1716 February 5, 2021 18:46

ggreenway assigned htuch Feb 5, 2021

xiaobinxbw closed this Feb 5, 2021

xiaobinxbw reopened this Feb 5, 2021

htuch reviewed Feb 7, 2021

View reviewed changes

htuch added the waiting label Feb 7, 2021

Refactor the test messages to make them more readable.

1322b31

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

repokitteh-read-only bot removed the waiting label Feb 9, 2021

htuch reviewed Feb 12, 2021

View reviewed changes

Addrees the comments.

bc7c9ae

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

Fix format and add coverage exception.

22c62c3

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

xiaobinxbw added 2 commits February 17, 2021 18:10

Fix build.

c424229

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

Fix format.

f9c674e

Signed-off-by: Tommy Wang <xiaobinwang@google.com>

htuch approved these changes Feb 18, 2021

View reviewed changes

htuch merged commit b2f3a11 into envoyproxy:main Feb 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network: Enable the Kill Request filter to honor Route-level configur… #14944

network: Enable the Kill Request filter to honor Route-level configur… #14944

xiaobinxbw commented Feb 4, 2021

repokitteh-read-only bot commented Feb 4, 2021

htuch left a comment

htuch Feb 5, 2021

xiaobinxbw commented Feb 5, 2021 via email

htuch commented Feb 5, 2021

xiaobinxbw commented Feb 5, 2021

htuch Feb 7, 2021

xiaobinxbw Feb 9, 2021

htuch commented Feb 7, 2021

xiaobinxbw commented Feb 9, 2021

xiaobinxbw commented Feb 9, 2021

htuch commented Feb 10, 2021

xiaobinxbw commented Feb 10, 2021 via email

htuch left a comment

htuch Feb 12, 2021

xiaobinxbw Feb 16, 2021

htuch Feb 12, 2021

xiaobinxbw Feb 16, 2021

htuch commented Feb 17, 2021

xiaobinxbw commented Feb 17, 2021

htuch left a comment

network: Enable the Kill Request filter to honor Route-level configur… #14944

network: Enable the Kill Request filter to honor Route-level configur… #14944

Conversation

xiaobinxbw commented Feb 4, 2021

repokitteh-read-only bot commented Feb 4, 2021

htuch left a comment

Choose a reason for hiding this comment

htuch Feb 5, 2021

Choose a reason for hiding this comment

xiaobinxbw commented Feb 5, 2021 via email

htuch commented Feb 5, 2021

xiaobinxbw commented Feb 5, 2021

htuch Feb 7, 2021

Choose a reason for hiding this comment

xiaobinxbw Feb 9, 2021

Choose a reason for hiding this comment

htuch commented Feb 7, 2021

xiaobinxbw commented Feb 9, 2021

xiaobinxbw commented Feb 9, 2021

htuch commented Feb 10, 2021

xiaobinxbw commented Feb 10, 2021 via email

htuch left a comment

Choose a reason for hiding this comment

htuch Feb 12, 2021

Choose a reason for hiding this comment

xiaobinxbw Feb 16, 2021

Choose a reason for hiding this comment

htuch Feb 12, 2021

Choose a reason for hiding this comment

xiaobinxbw Feb 16, 2021

Choose a reason for hiding this comment

htuch commented Feb 17, 2021

xiaobinxbw commented Feb 17, 2021

htuch left a comment

Choose a reason for hiding this comment