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

http: allow any FilterHeaderStatus::Stop* after sending a local reply. #15496

Merged
merged 6 commits into from
Mar 24, 2021
Merged

http: allow any FilterHeaderStatus::Stop* after sending a local reply. #15496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6d1e6a0
http: allow any FilterHeaderStatus::Stop* after sending a local reply.
PiotrSikora Mar 15, 2021
a779c3c
Merge remote-tracking branch 'origin/main' into PiotrSikora/fix_stop_…
PiotrSikora Mar 22, 2021
71fba33
review: address comments from Asra.
PiotrSikora Mar 22, 2021
0062c85
reviwe: reorder to fix metadata processing.
PiotrSikora Mar 23, 2021
6b9bf28
Merge remote-tracking branch 'origin/main' into PiotrSikora/fix_stop_…
PiotrSikora Mar 24, 2021
be883a1
review: add test with headers and body.
PiotrSikora Mar 24, 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
11 changes: 5 additions & 6 deletions source/common/http/filter_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,18 +507,16 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead
ASSERT(!(status == FilterHeadersStatus::ContinueAndDontEndStream && !(*entry)->end_stream_),
"Filters should not return FilterHeadersStatus::ContinueAndDontEndStream from "
"decodeHeaders when end_stream is already false");
ENVOY_BUG(
!state_.local_complete_ || status == FilterHeadersStatus::StopIteration,
"Filters should return FilterHeadersStatus::StopIteration after sending a local reply.");

state_.filter_call_state_ &= ~FilterCallState::DecodeHeaders;
ENVOY_STREAM_LOG(trace, "decode headers called: filter={} status={}", *this,
static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status));

(*entry)->decode_headers_called_ = true;

const auto continue_iteration =
(*entry)->commonHandleAfterHeadersCallback(status, end_stream) && !state_.local_complete_;
const auto continue_iteration = (*entry)->commonHandleAfterHeadersCallback(status, end_stream);
ENVOY_BUG(!continue_iteration || !state_.local_complete_,
"Filter did not return StopAll or StopIteration after sending a local reply.");

// If this filter ended the stream, decodeComplete() should be called for it.
if ((*entry)->end_stream_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to do any of this work (handle new metadata, add decoded data, etc).
Would it make more sense to early return or does that break something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly don't know the codebase well enough to answer that. Commenting out maybeContinueDecoding() below breaks a bunch of tests, but I don't know if those are real failures or tests that are not representative of the production code.

In any case, I don't think such risky change should be a part of this bugfix PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

given it's unlikely folks will be doing sendLocalReply and add decoded data / metadata I buy that as an argument.

Would you instead either throw a TODO my way or, if you want to avoid another round of CI, open a tracking issue to look into it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's skip the 3h wait on the CI, I filled #15654.

Expand All @@ -538,7 +536,8 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead
addDecodedData(*((*entry).get()), empty_data, true);
}

if (!continue_iteration && std::next(entry) != decoder_filters_.end()) {
if ((!continue_iteration || state_.local_complete_) &&
std::next(entry) != decoder_filters_.end()) {
alyssawilk marked this conversation as resolved.
Show resolved Hide resolved
// Stop iteration IFF this is not the last filter. If it is the last filter, continue with
// processing since we need to handle the case where a terminal filter wants to buffer, but
// a previous filter has added body.
Expand Down
27 changes: 24 additions & 3 deletions test/integration/protocol_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,8 @@ TEST_P(DownstreamProtocolIntegrationTest, ContinueAfterLocalReply) {
EXPECT_TRUE(response->complete());
EXPECT_EQ("200", response->headers().getStatusValue());
},
"envoy bug failure: !state_.local_complete_ || status == "
"FilterHeadersStatus::StopIteration. Details: Filters should return "
"FilterHeadersStatus::StopIteration after sending a local reply.");
"envoy bug failure: !continue_iteration || !state_.local_complete_. "
"Details: Filter did not return StopAll or StopIteration after sending a local reply.");
}

TEST_P(ProtocolIntegrationTest, AddEncodedTrailers) {
Expand Down Expand Up @@ -423,6 +422,28 @@ TEST_P(DownstreamProtocolIntegrationTest, MissingHeadersLocalReply) {
EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("InvalidHeaderFilter_ready\n"));
}

TEST_P(DownstreamProtocolIntegrationTest, MissingHeadersLocalReplyWithBody) {
useAccessLog("%RESPONSE_CODE_DETAILS%");
config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": "
"type.googleapis.com/google.protobuf.Empty } }");
initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));

// Missing method
auto response =
codec_client_->makeRequestWithBody(Http::TestRequestHeaderMapImpl{{":method", "GET"},
{":path", "/test/long/url"},
{":scheme", "http"},
{":authority", "host"},
{"remove-method", "yes"},
{"send-reply", "yes"}},
1024);
response->waitForEndStream();
EXPECT_TRUE(response->complete());
EXPECT_EQ("200", response->headers().getStatusValue());
EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("InvalidHeaderFilter_ready\n"));
}

// Regression test for https://github.com/envoyproxy/envoy/issues/10270
TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) {
// Header with at least 20kb of spaces surrounded by non-whitespace characters to ensure that
Expand Down