Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Feb 18, 2021
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e4584fa
network: Enable the Kill Request filter to honor Route-level configur…
xiaobinxbw Feb 3, 2021
acfcf75
Fix format.
xiaobinxbw Feb 4, 2021
f7c1716
Add test coverage for kill_request.
xiaobinxbw Feb 5, 2021
1322b31
Refactor the test messages to make them more readable.
xiaobinxbw Feb 9, 2021
bc7c9ae
Addrees the comments.
xiaobinxbw Feb 16, 2021
22c62c3
Fix format and add coverage exception.
xiaobinxbw Feb 17, 2021
c424229
Fix build.
xiaobinxbw Feb 17, 2021
f9c674e
Fix format.
xiaobinxbw Feb 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions source/extensions/filters/http/kill_request/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ envoy_cc_library(
"//source/common/http:header_utility_lib",
"//source/common/http:headers_lib",
"//source/common/protobuf:utility_lib",
"//source/extensions/filters/http:well_known_names",
"@envoy_api//envoy/extensions/filters/http/kill_request/v3:pkg_cc_proto",
],
)
Expand Down
22 changes: 22 additions & 0 deletions source/extensions/filters/http/kill_request/kill_request_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,22 @@
#include "extensions/filters/http/kill_request/kill_request_filter.h"

#include <csignal>
#include <string>

#include "common/protobuf/utility.h"

#include "extensions/filters/http/well_known_names.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace KillRequest {

using ::envoy::extensions::filters::http::kill_request::v3::KillRequest;

KillSettings::KillSettings(const KillRequest& kill_request)
: kill_probability_(kill_request.probability()) {}

bool KillRequestFilter::isKillRequestEnabled() {
return ProtobufPercentHelper::evaluateFractionalPercent(kill_request_.probability(),
random_generator_.random());
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

const auto* route_entry = decoder_callbacks_->route()->routeEntry();

const auto* per_route_kill_settings =
route_entry->mostSpecificPerFilterConfigTyped<KillSettings>(name);

if (per_route_kill_settings) {
envoy::type::v3::FractionalPercent probability = per_route_kill_settings->getProbability();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

kill_request_.set_allocated_probability(&probability);
}
}

if (is_kill_request && isKillRequestEnabled()) {
// Crash Envoy.
raise(SIGABRT);
Expand Down
28 changes: 26 additions & 2 deletions source/extensions/filters/http/kill_request/kill_request_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "envoy/http/filter.h"
#include "envoy/http/header_map.h"

#include "common/http/header_utility.h"
#include "common/http/headers.h"

namespace Envoy {
Expand Down Expand Up @@ -52,7 +53,9 @@ class KillRequestFilter : public Http::StreamFilter, Logger::Loggable<Logger::Id
return Http::FilterTrailersStatus::Continue;
}

void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks&) override {}
void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override {
decoder_callbacks_ = &callbacks;
}

// Http::StreamEncoderFilter
Http::FilterHeadersStatus encode100ContinueHeaders(Http::ResponseHeaderMap&) override {
Expand Down Expand Up @@ -82,8 +85,29 @@ class KillRequestFilter : public Http::StreamFilter, Logger::Loggable<Logger::Id
// equaling true.
bool isKillRequestEnabled();

const envoy::extensions::filters::http::kill_request::v3::KillRequest kill_request_;
envoy::extensions::filters::http::kill_request::v3::KillRequest kill_request_;
Random::RandomGenerator& random_generator_;
Http::StreamDecoderFilterCallbacks* decoder_callbacks_{};
};

/**
* Configuration for fault injection.
*/
class KillSettings : public Router::RouteSpecificFilterConfig {
public:
KillSettings(const envoy::extensions::filters::http::kill_request::v3::KillRequest& kill_request);

const std::vector<Http::HeaderUtility::HeaderDataPtr>& filterHeaders() const {
return kill_request_filter_headers_;
}

const envoy::type::v3::FractionalPercent getProbability() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return std::move(kill_probability_);
}

private:
envoy::type::v3::FractionalPercent kill_probability_;
const std::vector<Http::HeaderUtility::HeaderDataPtr> kill_request_filter_headers_;
};

} // namespace KillRequest
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/http/kill_request/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ envoy_extension_cc_test(
deps = [
"//include/envoy/http:metadata_interface",
"//source/common/buffer:buffer_lib",
"//source/extensions/filters/http:well_known_names",
"//source/extensions/filters/http/kill_request:kill_request_filter_lib",
"//test/mocks:common_lib",
"//test/mocks/http:http_mocks",
"//test/test_common:utility_lib",
"@envoy_api//envoy/extensions/filters/http/kill_request/v3:pkg_cc_proto",
"@envoy_api//envoy/type/v3:pkg_cc_proto",
Expand Down
42 changes: 42 additions & 0 deletions test/extensions/filters/http/kill_request/kill_request_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
#include "common/buffer/buffer_impl.h"

#include "extensions/filters/http/kill_request/kill_request_filter.h"
#include "extensions/filters/http/well_known_names.h"

#include "test/mocks/common.h"
#include "test/mocks/http/mocks.h"
#include "test/test_common/utility.h"

#include "gmock/gmock.h"
Expand All @@ -18,17 +20,23 @@ namespace HttpFilters {
namespace KillRequest {
namespace {

using ::testing::AnyNumber;
using ::testing::Return;

class KillRequestFilterTest : public testing::Test {
protected:
void
setUpTest(const envoy::extensions::filters::http::kill_request::v3::KillRequest& kill_request) {
filter_ = std::make_unique<KillRequestFilter>(kill_request, random_generator_);

filter_->setDecoderFilterCallbacks(decoder_filter_callbacks_);
EXPECT_CALL(decoder_filter_callbacks_.dispatcher_, pushTrackedObject(_)).Times(AnyNumber());
EXPECT_CALL(decoder_filter_callbacks_.dispatcher_, popTrackedObject(_)).Times(AnyNumber());
}

std::unique_ptr<KillRequestFilter> filter_;
testing::NiceMock<Random::MockRandomGenerator> random_generator_;
testing::NiceMock<Http::MockStreamDecoderFilterCallbacks> decoder_filter_callbacks_;
Http::TestRequestHeaderMapImpl request_headers_;
};

Expand Down Expand Up @@ -74,6 +82,40 @@ TEST_F(KillRequestFilterTest, KillRequestDisabledWhenIsKillRequestEnabledReturns
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));
}

TEST_F(KillRequestFilterTest,
KillRequestDisabledWhenIsKillRequestEnabledReturnsTrueFromRouteLevelConfiguration) {
envoy::extensions::filters::http::kill_request::v3::KillRequest kill_request;
kill_request.mutable_probability()->set_numerator(0);
setUpTest(kill_request);
request_headers_.addCopy("x-envoy-kill-request", "true");

envoy::extensions::filters::http::kill_request::v3::KillRequest route_level_kill_request;
route_level_kill_request.mutable_probability()->set_numerator(1);
route_level_kill_request.set_kill_request_header("x-custom-kill-request");

KillSettings kill_settings = KillSettings(route_level_kill_request);

ON_CALL(random_generator_, random()).WillByDefault(Return(0));
ON_CALL(decoder_filter_callbacks_.route_->route_entry_,
perFilterConfig(Extensions::HttpFilters::HttpFilterNames::get().KillRequest))
.WillByDefault(Return(&kill_settings));
EXPECT_DEATH(filter_->decodeHeaders(request_headers_, false), "");
}

TEST_F(KillRequestFilterTest,
KillRequestDisabledWhenIsKillRequestEnabledReturnsFalseFromRouteLevelConfiguration) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

envoy::extensions::filters::http::kill_request::v3::KillRequest kill_request;
kill_request.mutable_probability()->set_numerator(0);
setUpTest(kill_request);
request_headers_.addCopy("x-envoy-kill-request", "true");

ON_CALL(random_generator_, random()).WillByDefault(Return(0));
ON_CALL(decoder_filter_callbacks_.route_->route_entry_,
perFilterConfig(Extensions::HttpFilters::HttpFilterNames::get().KillRequest))
.WillByDefault(Return(nullptr));
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));
}

TEST_F(KillRequestFilterTest, KillRequestDisabledWhenHeaderIsMissing) {
envoy::extensions::filters::http::kill_request::v3::KillRequest kill_request;
kill_request.mutable_probability()->set_numerator(100);
Expand Down