Skip to content

Add an explicit state for whether filter chain should continue or not. #3334

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

Merged
htuch merged 5 commits into from
May 16, 2018
Merged

Add an explicit state for whether filter chain should continue or not. #3334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
334a70e
Add an explicit state for whether filter chain should continue or not.
May 9, 2018
6d17ca8
Add comment about state variables.
May 11, 2018
7e4e871
Fix formatting issue.
May 11, 2018
4cc32a7
Use sendLocalReply
May 16, 2018
33de19c
Merge remote-tracking branch 'upstream/master' into authz-filter-crash
May 16, 2018
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
20 changes: 13 additions & 7 deletions source/extensions/filters/http/ext_authz/ext_authz.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,25 +36,28 @@ void Filter::initiateCall(const Http::HeaderMap& headers) {
check_request_);

state_ = State::Calling;
// Don't let the filter chain continue as we are going to invoke check call.
filter_return_ = FilterReturn::StopDecoding;
initiating_call_ = true;
client_->check(*this, check_request_, callbacks_->activeSpan());
initiating_call_ = false;
}

Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool) {
initiateCall(headers);
return state_ == State::Calling ? Http::FilterHeadersStatus::StopIteration
: Http::FilterHeadersStatus::Continue;
return filter_return_ == FilterReturn::StopDecoding ? Http::FilterHeadersStatus::StopIteration
Copy link
Member

@htuch htuch May 10, 2018

Choose a reason for hiding this comment

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

Thanks for the fix. IMHO, we have a lot of complexity here, since we now have 3 state variables, and I can't see the clear relationship between them. The variables are filter_return_, initiating_call_ and state_. Can you write a comment explaining how they relate? Is there no way to simplify this?

Copy link
Contributor Author

@saumoh saumoh May 11, 2018

Choose a reason for hiding this comment

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

@htuch (cc @gsagula ) I added comments about these 3 states. I think it is possible to combine all of these and initially i had done some of that (see: #3259 (comment)). But then i found it harder to reason about all the states at the various locations that we use them.
I think its more important for others to read and understand the use of these states (and not just me) so any recommendations/suggestion from either of you would be quite helpful.

Copy link
Member

@gsagula gsagula May 14, 2018

Choose a reason for hiding this comment

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

@saumoh It looks clear to me. I will merge it with #3162 later tonight. Thanks!

: Http::FilterHeadersStatus::Continue;
}

Http::FilterDataStatus Filter::decodeData(Buffer::Instance&, bool) {
return state_ == State::Calling ? Http::FilterDataStatus::StopIterationAndWatermark
: Http::FilterDataStatus::Continue;
return filter_return_ == FilterReturn::StopDecoding
? Http::FilterDataStatus::StopIterationAndWatermark
: Http::FilterDataStatus::Continue;
}

Http::FilterTrailersStatus Filter::decodeTrailers(Http::HeaderMap&) {
return state_ == State::Calling ? Http::FilterTrailersStatus::StopIteration
: Http::FilterTrailersStatus::Continue;
return filter_return_ == FilterReturn::StopDecoding ? Http::FilterTrailersStatus::StopIteration
: Http::FilterTrailersStatus::Continue;
}

void Filter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) {
Expand Down Expand Up @@ -82,6 +85,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::CheckStatus status) {
case CheckStatus::Error:
cluster_->statsScope().counter("ext_authz.error").inc();
break;

case CheckStatus::Denied:
cluster_->statsScope().counter("ext_authz.denied").inc();
Http::CodeUtility::ResponseStatInfo info{config_->scope(),
Expand All @@ -106,12 +110,14 @@ void Filter::onComplete(Filters::Common::ExtAuthz::CheckStatus status) {
callbacks_->requestInfo().setResponseFlag(
RequestInfo::ResponseFlag::UnauthorizedExternalService);
} else {
// Let the filter chain continue.
filter_return_ = FilterReturn::ContinueDecoding;
if (config_->failureModeAllow() && status == CheckStatus::Error) {
// Status is Error and yet we are allowing the request. Click a counter.
cluster_->statsScope().counter("ext_authz.failure_mode_allowed").inc();
}
// We can get completion inline, so only call continue if that isn't happening.
if (!initiating_call_) {
// We got completion async. Let the filter chain continue.
callbacks_->continueDecoding();
}
}
Expand Down
9 changes: 9 additions & 0 deletions source/extensions/filters/http/ext_authz/ext_authz.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,23 @@ class Filter : public Http::StreamDecoderFilter,
void onComplete(Filters::Common::ExtAuthz::CheckStatus status) override;

private:
// State of this filter's communication with the external authorization service.
// The filter has either not started calling the external service, in the middle of calling
// it or has completed.
enum class State { NotStarted, Calling, Complete };
// FilterReturn is used to capture what the return code should be to the filter chain.
// if this filter is either in the middle of calling the service or the result is denied then
// the filter chain should stop. Otherwise the filter chain can continue to the next filter.
enum class FilterReturn { ContinueDecoding, StopDecoding };
void initiateCall(const Http::HeaderMap& headers);

FilterConfigSharedPtr config_;
Filters::Common::ExtAuthz::ClientPtr client_;
Http::StreamDecoderFilterCallbacks* callbacks_{};
State state_{State::NotStarted};
FilterReturn filter_return_{FilterReturn::ContinueDecoding};
Upstream::ClusterInfoConstSharedPtr cluster_;
// Used to identify if the callback to onComplete() is synchronous (on the stack) or asynchronous.
bool initiating_call_{};
envoy::service::auth::v2alpha::CheckRequest check_request_{};
};
Expand Down
6 changes: 4 additions & 2 deletions source/extensions/filters/network/ext_authz/ext_authz.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ Network::FilterStatus Filter::onData(Buffer::Instance&, bool /* end_stream */) {
// sufficient information to fillout the checkRequest_.
callCheck();
}
return status_ == Status::Calling ? Network::FilterStatus::StopIteration
: Network::FilterStatus::Continue;
return filter_return_ == FilterReturn::Stop ? Network::FilterStatus::StopIteration
: Network::FilterStatus::Continue;
}

Network::FilterStatus Filter::onNewConnection() {
Expand Down Expand Up @@ -78,6 +78,8 @@ void Filter::onComplete(Filters::Common::ExtAuthz::CheckStatus status) {
config_->stats().cx_closed_.inc();
filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
} else {
// Let the filter chain continue.
filter_return_ = FilterReturn::Continue;
if (config_->failureModeAllow() && status == Filters::Common::ExtAuthz::CheckStatus::Error) {
// Status is Error and yet we are configured to allow traffic. Click a counter.
config_->stats().failure_mode_allowed_.inc();
Expand Down
9 changes: 9 additions & 0 deletions source/extensions/filters/network/ext_authz/ext_authz.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,22 @@ class Filter : public Network::ReadFilter,
void onComplete(Filters::Common::ExtAuthz::CheckStatus status) override;

private:
// State of this filter's communication with the external authorization service.
// The filter has either not started calling the external service, in the middle of calling
// it or has completed.
enum class Status { NotStarted, Calling, Complete };
// FilterReturn is used to capture what the return code should be to the filter chain.
// if this filter is either in the middle of calling the external service or the result is denied
// then the filter chain should stop. Otherwise the filter chain can continue to the next filter.
enum class FilterReturn { Stop, Continue };
void callCheck();

ConfigSharedPtr config_;
Filters::Common::ExtAuthz::ClientPtr client_;
Network::ReadFilterCallbacks* filter_callbacks_{};
Status status_{Status::NotStarted};
FilterReturn filter_return_{FilterReturn::Stop};
// Used to identify if the callback to onComplete() is synchronous (on the stack) or asynchronous.
bool calling_check_{};
envoy::service::auth::v2alpha::CheckRequest check_request_{};
};
Expand Down
25 changes: 25 additions & 0 deletions test/extensions/filters/http/ext_authz/ext_authz_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,31 @@ TEST_P(HttpExtAuthzFilterParamTest, ImmediateOkResponse) {
cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ext_authz.ok").value());
}

// Test that an synchronous denied response from the authorization service, on the call stack,
// results in request not continuing.
TEST_P(HttpExtAuthzFilterParamTest, ImmediateDeniedResponse) {
InSequence s;

ON_CALL(filter_callbacks_, connection()).WillByDefault(Return(&connection_));
EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_));
EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_));
EXPECT_CALL(*client_, check(_, _, _))
.WillOnce(
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
callbacks.onComplete(Filters::Common::ExtAuthz::CheckStatus::Denied);
})));

EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0);
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->decodeHeaders(request_headers_, false));
EXPECT_EQ(Http::FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(data_, false));
EXPECT_EQ(Http::FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_headers_));

EXPECT_EQ(
1U,
cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ext_authz.denied").value());
}

// Test that a denied response results in the connection closing with a 403 response to the client.
TEST_P(HttpExtAuthzFilterParamTest, DeniedResponse) {
InSequence s;
Expand Down
28 changes: 27 additions & 1 deletion test/extensions/filters/network/ext_authz/ext_authz_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ TEST_F(ExtAuthzFilterTest, DeniedWithOnData) {
EXPECT_CALL(*client_, cancel()).Times(0);
request_callbacks_->onComplete(Filters::Common::ExtAuthz::CheckStatus::Denied);

EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false));
EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data, false));

EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value());
EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value());
Expand Down Expand Up @@ -314,6 +314,32 @@ TEST_F(ExtAuthzFilterTest, ImmediateOK) {
EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value());
}

// Test to verify that on stack denied response from the authorization service does
// result in stopage of the filter chain.
TEST_F(ExtAuthzFilterTest, ImmediateNOK) {
InSequence s;

EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_));
EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_));
EXPECT_CALL(filter_callbacks_, continueReading()).Times(0);
EXPECT_CALL(*client_, check(_, _, _))
.WillOnce(
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
callbacks.onComplete(Filters::Common::ExtAuthz::CheckStatus::Denied);
})));

EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection());
Buffer::OwnedImpl data("hello");
EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data, false));

EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value());
EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value());
EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value());
EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.denied").value());
EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value());
EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.cx_closed").value());
}

// Test to verify that on stack Error response when failure_mode_allow is configured
// result in request being allowed.
TEST_F(ExtAuthzFilterTest, ImmediateErrorFailOpen) {
Expand Down