Skip to content

extensions/filters: ext_authz filter to use StopAllIterationAndWatermark status #6565

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
lizan merged 1 commit into from
Apr 15, 2019
Merged

extensions/filters: ext_authz filter to use StopAllIterationAndWatermark status #6565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 15 additions & 10 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 @@ -81,33 +81,38 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
}

initiateCall(headers);
return filter_return_ == FilterReturn::StopDecoding ? Http::FilterHeadersStatus::StopIteration
: Http::FilterHeadersStatus::Continue;
return filter_return_ == FilterReturn::StopDecoding
? Http::FilterHeadersStatus::StopAllIterationAndWatermark

@gsagula gsagula Apr 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I understood correctly, StopAllIterationAndWatermark will hold headers, data and trailers. It will also return an HTTP 413 if the buffer limit is exceeded. If that's correct, isn't a change of behaviour?

@soya3129 soya3129 Apr 14, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! StopAllIterationAndWatermark behaves the same as if StopIterationAndWater is returned by decodeData() (the original behavior), instead of returning 413, reading will be disabled until buffered data are processed. Does it make sense?

@gsagula gsagula Apr 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No problem! Sorry, I was looking at the description for StopAllIterationAndBuffer. Thanks!

: Http::FilterHeadersStatus::Continue;
}

Http::FilterDataStatus Filter::decodeData(Buffer::Instance&, bool end_stream) {
if (buffer_data_) {
if (end_stream || isBufferFull()) {
ENVOY_STREAM_LOG(debug, "ext_authz filter finished buffering the request", *callbacks_);
initiateCall(*request_headers_);
return filter_return_ == FilterReturn::StopDecoding
? Http::FilterDataStatus::StopIterationAndWatermark
: Http::FilterDataStatus::Continue;
} else {
return Http::FilterDataStatus::StopIterationAndBuffer;
}
}

return filter_return_ == FilterReturn::StopDecoding
? Http::FilterDataStatus::StopIterationAndWatermark
: Http::FilterDataStatus::Continue;
return Http::FilterDataStatus::Continue;
}

Http::FilterTrailersStatus Filter::decodeTrailers(Http::HeaderMap&) {
if (buffer_data_ && filter_return_ != FilterReturn::StopDecoding) {
ENVOY_STREAM_LOG(debug, "ext_authz filter finished buffering the request", *callbacks_);
initiateCall(*request_headers_);
if (buffer_data_) {
if (filter_return_ != FilterReturn::StopDecoding) {
ENVOY_STREAM_LOG(debug, "ext_authz filter finished buffering the request", *callbacks_);
initiateCall(*request_headers_);
}
return filter_return_ == FilterReturn::StopDecoding ? Http::FilterTrailersStatus::StopIteration
: Http::FilterTrailersStatus::Continue;
}

return filter_return_ == FilterReturn::StopDecoding ? Http::FilterTrailersStatus::StopIteration
: Http::FilterTrailersStatus::Continue;
return Http::FilterTrailersStatus::Continue;
}

void Filter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) {
Expand Down
51 changes: 25 additions & 26 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 @@ -174,7 +174,7 @@ TEST_F(HttpFilterTest, ErrorFailClose) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));
EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0);

Expand Down Expand Up @@ -204,7 +204,7 @@ TEST_F(HttpFilterTest, ErrorOpen) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));
EXPECT_CALL(filter_callbacks_, continueDecoding());

Expand Down Expand Up @@ -408,10 +408,9 @@ TEST_F(HttpFilterTest, HeaderOnlyRequest) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, true));
EXPECT_EQ(Http::FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(data_, true));
EXPECT_EQ(Http::FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_headers_));
// decodeData() and decodeTrailers() will not be called since request is header only.
}

// Checks that filter does not buffer data on upgrade WebSocket request.
Expand All @@ -438,10 +437,9 @@ TEST_F(HttpFilterTest, UpgradeWebsocketRequest) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));
EXPECT_EQ(Http::FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(data_, false));
EXPECT_EQ(Http::FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_headers_));
// decodeData() and decodeTrailers() will not be called until continueDecoding() is called.
}

// Checks that filter does not buffer data on upgrade H2 WebSocket request.
Expand All @@ -467,10 +465,9 @@ TEST_F(HttpFilterTest, H2UpgradeRequest) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));
EXPECT_EQ(Http::FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(data_, false));
EXPECT_EQ(Http::FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_headers_));
// decodeData() and decodeTrailers() will not be called until continueDecoding() is called.
}

// Checks that filter does not buffer data when is not the end of the stream, but header-only
Expand Down Expand Up @@ -568,7 +565,7 @@ TEST_F(HttpFilterTestParam, DisabledOnRoute) {
test_disable(false);
EXPECT_CALL(*client_, check(_, _, _)).Times(1);
// Engage the filter.
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));

// test that disabling works
Expand Down Expand Up @@ -606,10 +603,8 @@ TEST_F(HttpFilterTestParam, OkResponse) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));
EXPECT_EQ(Http::FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(data_, false));
EXPECT_EQ(Http::FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_headers_));
EXPECT_CALL(filter_callbacks_, continueDecoding());
EXPECT_CALL(filter_callbacks_.stream_info_,
setResponseFlag(Envoy::StreamInfo::ResponseFlag::UnauthorizedExternalService))
Expand All @@ -619,6 +614,10 @@ TEST_F(HttpFilterTestParam, OkResponse) {
response.status = Filters::Common::ExtAuthz::CheckStatus::OK;
request_callbacks_->onComplete(std::make_unique<Filters::Common::ExtAuthz::Response>(response));
EXPECT_EQ(1U, filter_callbacks_.clusterInfo()->statsScope().counter("ext_authz.ok").value());

// decodeData() and decodeTrailers() are called after continueDecoding().
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false));
EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers_));
}

// Test that an synchronous OK response from the authorization service, on the call stack, results
Expand Down Expand Up @@ -664,11 +663,11 @@ TEST_F(HttpFilterTestParam, ImmediateDeniedResponseWithHttpAttributes) {
callbacks.onComplete(std::move(response_ptr));
})));
EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0);
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
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, filter_callbacks_.clusterInfo()->statsScope().counter("ext_authz.denied").value());
// When request is denied, no call to continueDecoding(). As a result, decodeData() and
// decodeTrailer() will not be called.
}

// Test that an synchronous ok response from the authorization service passing additional HTTP
Expand Down Expand Up @@ -726,11 +725,11 @@ TEST_F(HttpFilterTestParam, ImmediateDeniedResponse) {
callbacks.onComplete(std::make_unique<Filters::Common::ExtAuthz::Response>(response));
})));
EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0);
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
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, filter_callbacks_.clusterInfo()->statsScope().counter("ext_authz.denied").value());
// When request is denied, no call to continueDecoding(). As a result, decodeData() and
// decodeTrailer() will not be called.
}

// Test that a denied response results in the connection closing with a 401 response to the client.
Expand All @@ -743,7 +742,7 @@ TEST_F(HttpFilterTestParam, DeniedResponseWith401) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));

Http::TestHeaderMapImpl response_headers{{":status", "401"}};
Expand All @@ -770,7 +769,7 @@ TEST_F(HttpFilterTestParam, DeniedResponseWith403) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));

Http::TestHeaderMapImpl response_headers{{":status", "403"}};
Expand Down Expand Up @@ -807,7 +806,7 @@ TEST_F(HttpFilterTestParam, DestroyResponseBeforeSendLocalReply) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));

Http::TestHeaderMapImpl response_headers{{":status", "403"},
Expand Down Expand Up @@ -852,7 +851,7 @@ TEST_F(HttpFilterTestParam, OverrideEncodingHeaders) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));

Http::TestHeaderMapImpl response_headers{{":status", "403"},
Expand Down Expand Up @@ -894,7 +893,7 @@ TEST_F(HttpFilterTestParam, ResetDuringCall) {
WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, false));
EXPECT_CALL(*client_, cancel());
filter_->onDestroy();
Expand Down