Skip to content

http_filter: add addEncodedTrailers and addDecodedTrailers #3980

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
mattklein123 merged 20 commits into from
Aug 14, 2018
Merged

http_filter: add addEncodedTrailers and addDecodedTrailers #3980

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
34bd3d1
http_filter: add addEncodedTrailers
Jul 28, 2018
688bc4d
mock out new method
Jul 28, 2018
e0b74f3
add addDecodedTrailers and use move semantics
Jul 31, 2018
acfa3d8
only allow trailers added in decodeData(true)
Aug 2, 2018
baee8aa
throw exceptions on bad access + add comments
Aug 3, 2018
42443b0
return by reference, throw exception on invalid usage
Aug 7, 2018
52c5780
expect exception in encode/decodeTrailers + add @throws comment
Aug 7, 2018
8495e41
asserts instead of exceptions
Aug 7, 2018
36103a1
add integration test for addEncodedTrailers
Aug 8, 2018
f6ca600
fix addDecodedTrailers
Aug 8, 2018
483b48b
only check for upstream trailers when upstream supports trailers
Aug 8, 2018
5412bb8
only execute callback for filters after filter that added trailers
Aug 9, 2018
cd25726
also keep track of filter for decode
Aug 9, 2018
c3a900b
test adding trailers in integration_test.cc
Aug 9, 2018
90c6233
fixup comments
Aug 9, 2018
3667d6a
formating
Aug 9, 2018
7637fa3
remove debug line
Aug 10, 2018
f2b6210
add @return comments
Aug 10, 2018
815a07b
update comments and other feedback
Aug 13, 2018
2321031
add const qualifier and update comment
Aug 14, 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
18 changes: 18 additions & 0 deletions include/envoy/http/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,15 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks {
*/
virtual void addDecodedData(Buffer::Instance& data, bool streaming_filter) PURE;

/**
* Adds decoded trailers. May only be called in decodeData when end_stream is set to true.
* If called in any other context, an assertion will be triggered.
*
* When called in decodeData, the trailers map will be initialized to an empty map and returned by
* reference. Calling this function more than once is invalid.
*/

@alyssawilk alyssawilk Aug 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I should have called this out before, but please doc the return as @return here and below

virtual HeaderMap& addDecodedTrailers() PURE;

/**
* Create a locally generated response using the provided response_code and body_text parameters.
* If the request was a gRPC request the local reply will be encoded as a gRPC response with a 200
Expand Down Expand Up @@ -395,6 +404,15 @@ class StreamEncoderFilterCallbacks : public virtual StreamFilterCallbacks {
*/
virtual void addEncodedData(Buffer::Instance& data, bool streaming_filter) PURE;

/**
* Adds encoded trailers. May only be called in encodeData when end_stream is set to true.
* If called in any other context, an assertion will be triggered.
*
* When called in encodeData, the trailers map will be initialized to an empty map and returned by
* reference. Calling this function more than once is invalid.
*/
virtual HeaderMap& addEncodedTrailers() PURE;

@snowp snowp Jul 28, 2018

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.

unsure what the best signature was here - it seems like HeaderMap has a relatively rich API to allow calls to specify reference/copy/etc, so I wasn't sure how best to allow that without providing access to the HeaderMap itself. On the other hand, giving out an actual reference to the map might not be ideal as it opens up for more potential bugs (filter implementations keeping it around for too long). thoughts?

@htuch htuch Jul 30, 2018

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.

How does this work if the target stream protocol is not able to support trailers?

@snowp snowp Jul 30, 2018

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.

From what I understand the http/1.1 codec will just ignore the trailers: https://github.com/envoyproxy/envoy/blob/master/source/common/http/http1/codec_impl.cc#L148

@mattklein123 mattklein123 Jul 30, 2018

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.

Perhaps we should just pass a HeaderMapPtr via move? This would just overwrite existing trailers? It would be uyp to the user to figure out if trailers already exist? Thoughts?

@lizan lizan Jul 30, 2018

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.

Perhaps we should just pass a HeaderMapPtr via move? This would just overwrite existing trailers? It would be uyp to the user to figure out if trailers already exist? Thoughts?

In that case you'll leaking HeaderMapImpl into filter codes and let filter create the HeaderMapPtr. Can we somehow (e.g. ASSERT) make sure this can be only called after or in encodeData(data, end_stream=true)? So the filter doesn't have to care about potentially overwriting existing (future) trailers.

@alyssawilk alyssawilk Aug 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We will want to make sure for any protocol which doesn't support trailers we don't then lose the end_stream=true passing through the pipeline, but that's what tests are for :-)


/**
* Called when an encoder filter goes over its high watermark.
*/
Expand Down
1 change: 1 addition & 0 deletions source/common/http/async_client_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ class AsyncStreamImpl : public AsyncClient::Stream,
Tracing::Span& activeSpan() override { return active_span_; }
const Tracing::Config& tracingConfig() override { return tracing_config_; }
void continueDecoding() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
HeaderMap& addDecodedTrailers() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
void addDecodedData(Buffer::Instance&, bool) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
const Buffer::Instance* decodingBuffer() override { return buffered_body_.get(); }
void sendLocalReply(Code code, const std::string& body,
Expand Down
82 changes: 77 additions & 5 deletions source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,15 +775,44 @@ void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter*

for (; entry != decoder_filters_.end(); entry++) {
ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeData));

// We check the request_trailers_ pointer here in case addDecodedTrailers
// is called in decodeData - at which point we communicate to the filter

@mattklein123 mattklein123 Aug 11, 2018

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.

"is called in decodeData during a previous filter invocation, at which point we communicate to the current and future filters that the stream has not yet ended."

// that the stream has not yet ended.
bool end_stream_no_trailers = end_stream && !request_trailers_;

@alyssawilk alyssawilk Aug 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mind adding a comment that we check the state of end stream inside the loop in case a filter adds trailers?

if (end_stream_no_trailers) {
state_.filter_call_state_ |= FilterCallState::LastDataFrame;
}

@alyssawilk alyssawilk Aug 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd argue changing HCM state probably bumps this up to a medium-risk change.

@mattklein123 mattklein123 Aug 3, 2018

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.

+1

@snowp snowp Aug 3, 2018

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.

What does HCM stand for in this context?

@mattklein123 mattklein123 Aug 4, 2018

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.

Http Connection Manager (HCM)

state_.filter_call_state_ |= FilterCallState::DecodeData;
FilterDataStatus status = (*entry)->handle_->decodeData(data, end_stream);
FilterDataStatus status = (*entry)->handle_->decodeData(data, end_stream_no_trailers);
state_.filter_call_state_ &= ~FilterCallState::DecodeData;
if (end_stream_no_trailers) {
state_.filter_call_state_ &= ~FilterCallState::LastDataFrame;
}
ENVOY_STREAM_LOG(trace, "decode data called: filter={} status={}", *this,
static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status));
if (!(*entry)->commonHandleAfterDataCallback(status, data, state_.decoder_filters_streaming_)) {
return;
// break here to ensure that we call decodeTrailers below

@alyssawilk alyssawilk Aug 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if commonHandleAfterDataCallback fails, doesn't it mean some data filter has requested the pipeline pause iteration? Let's say we have filter A which adds trailers and filter B which waits for full body and then does an RPC. If B gets the full body and asks the pipeline to pause while it does a callback, I believe we have to pause (not process trailers) until we get commonContinue. Are the added trailers not being processed there?

@snowp snowp Aug 8, 2018

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.

Yeah I was a bit unsure how to handle this. The problem I ran into was that the router filter will stop iteration after decodeData and keep the connection alive since end_stream is false. Since there's no trailers coming in from the decoding codec, nothing ever calls continue, so the the request hangs.
,
Maybe only break when end_stream is true to cover this case? In this case we know that we won't get any more data from the codec, so by immediately calling decodeTrailers we're not skipping any data.

Open to other suggestions

@snowp snowp Aug 8, 2018

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.

Actually that might not be feasible since we'd hide some data from filters behind the one that stopped iteration...

@alyssawilk alyssawilk Aug 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh yeah, I see the problem now. Hmm.... The best workaround I can think of is fairly inelegant, which is when trailers are added to schedule a closure which basically fakes the "you have received trailers from a client" path but gets called safely from not under the stack of the connection manager pipeline. Maybe @mattklein123 has a better suggestion?

@mattklein123 mattklein123 Aug 8, 2018

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.

I think this is exactly the same situation that is handled here with body added in the headers callback? https://github.com/envoyproxy/envoy/blob/master/source/common/http/conn_manager_impl.cc#L721. Can we handle in the same way?

@mattklein123 mattklein123 Aug 11, 2018

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.

I don't think this comment is correct. We are returning. Can you replicate a similar comment as we have in the headers case about why we are checking if this is the last filter?

break;
}
}

// If trailers were adding during decodeData we need to trigger decodeTrailers in order
// to allow filters to process the trailers.
if (end_stream && request_trailers_) {

@alyssawilk alyssawilk Aug 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

again let's comment this is explicitly for the case that the remote side didn't send trailers but a filter added them.

decodeTrailers(filter, *request_trailers_);

@mattklein123 mattklein123 Aug 8, 2018

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.

Don't we need to actually only send trailers to the filter that set the trailer and filters after it? I think we need to actually track which filter added the trailers... (See how this is handled in the headers/data case).

}
}

HeaderMap& ConnectionManagerImpl::ActiveStream::addDecodedTrailers() {
// trailers can only be added during the last data frame (i.e. end_stream = true)
ASSERT(state_.filter_call_state_ & FilterCallState::LastDataFrame);

// traileres can only be added once

@alyssawilk alyssawilk Aug 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

-> Trailers can only be added once.
here and below. Also trailers->Trailers above

ASSERT(!request_trailers_);

request_trailers_ = std::make_unique<HeaderMapImpl>();
return *request_trailers_;
}

void ConnectionManagerImpl::ActiveStream::addDecodedData(ActiveStreamDecoderFilter& filter,
Expand Down Expand Up @@ -1058,6 +1087,22 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
}
}

HeaderMap& ConnectionManagerImpl::ActiveStream::addEncodedTrailers() {
// trailers can only be added during the last data frame (i.e. end_stream = true)
ASSERT(state_.filter_call_state_ & FilterCallState::LastDataFrame);

// traileres can only be added once
ASSERT(!response_trailers_);

// local_complete_ is set to true at the start of encodeData(..., true), but since
// we've now added trailers we have to undo this to prevent encodeTrailers from
// blowing up when it assert on local_complete_
state_.local_complete_ = false;

response_trailers_ = std::make_unique<HeaderMapImpl>();
return *response_trailers_;
}

void ConnectionManagerImpl::ActiveStream::addEncodedData(ActiveStreamEncoderFilter& filter,
Buffer::Instance& data, bool streaming) {
if (state_.filter_call_state_ == 0 ||
Expand Down Expand Up @@ -1085,9 +1130,20 @@ void ConnectionManagerImpl::ActiveStream::encodeData(ActiveStreamEncoderFilter*
std::list<ActiveStreamEncoderFilterPtr>::iterator entry = commonEncodePrefix(filter, end_stream);
for (; entry != encoder_filters_.end(); entry++) {
ASSERT(!(state_.filter_call_state_ & FilterCallState::EncodeData));

// We check the request_trailers_ pointer here in case addEncodedTrailers
// is called in encodeData - at which point we communicate to the filter

@mattklein123 mattklein123 Aug 11, 2018

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.

Same comment about the comment and the local variable.

// that the stream has not yet ended.
bool end_stream_no_trailers = end_stream && !response_trailers_;
state_.filter_call_state_ |= FilterCallState::EncodeData;
FilterDataStatus status = (*entry)->handle_->encodeData(data, end_stream);
if (end_stream_no_trailers) {
state_.filter_call_state_ |= FilterCallState::LastDataFrame;
}
FilterDataStatus status = (*entry)->handle_->encodeData(data, end_stream_no_trailers);
state_.filter_call_state_ &= ~FilterCallState::EncodeData;
if (end_stream_no_trailers) {
state_.filter_call_state_ &= ~FilterCallState::LastDataFrame;
}
ENVOY_STREAM_LOG(trace, "encode data called: filter={} status={}", *this,
static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status));
if (!(*entry)->commonHandleAfterDataCallback(status, data, state_.encoder_filters_streaming_)) {
Expand All @@ -1099,8 +1155,16 @@ void ConnectionManagerImpl::ActiveStream::encodeData(ActiveStreamEncoderFilter*
end_stream);

request_info_.addBytesSent(data.length());
response_encoder_->encodeData(data, end_stream);
maybeEndEncode(end_stream);

// If trailers were adding during encodeData we need to trigger decodeTrailers in order
// to allow filters to process the trailers.
if (end_stream && response_trailers_) {

@mattklein123 mattklein123 Aug 1, 2018

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.

I'm a little confused about the semantics here. When is a filter expected to add trailers for the first time? Do we want to support setting trailers during the final data frame where end_stream was otherwise true? I don't think we want to support that and we probably do? Also where is the parallel to this code on the decoding side?

@snowp snowp Aug 1, 2018 •
edited
Loading

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.

What happens right now (since you can override trailers whenever)

  1. Trailers are set during header/data when upstream has no trailers: trailers get written out at the end of [en|de]codeData(_, true)
  2. Trailers are set during onHeader/onData when upstream HAS trailers: the original trailers override the trailers set previously (because in that case the upstream trailers are moved into response_trailers_ AFTER we've gone through the header/data callbacks). Trailers are written out at the end of [en|de]codeTrailers(_)
  3. Trailers are set during onTrailers: the trailers override those provided by upstream. Trailers are written out at the end of [en|de]codeTrailers(_)

3 is definitely confusing, so I'd be happy to prevent that. The question to me that remains is whether we want trailers to be written out at the end of [en|de]codeData(_, true) or if we want setting the trailers to cause [en|de]codeTrailers to be executed. I think based on your question about the decoding side that you were thinking more along the lines of the latter, because iirc that's what would allow the router to include it in the upstream request?

What steered me away from that originally was that was that it doesn't seem like addEncodedData will cause the onEncodedData callbacks to run if called in onHeaders(_, true).

So after thinking about it some, how about this:
At the end of [en|de]codeData(_, true) we trigger [en|de]codeTrailers if the trailers ptr is non-null. This should cover both encoding and decoding and we'd end up writing out the trailers to the encoder like I'm already doing. If addTrailers is called during the [en|de]codeTrailers(_) cb we can fail with an ASSERT.

This does mean we get both a [en|de]codeData(_, true) and [en|de]codeTrailers which might be confusing, but it would allow a filter to inject trailers when it can see that there aren't already any there (because it sees [en|de]codeData(_, true)).

@mattklein123 mattklein123 Aug 1, 2018

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.

So after thinking about it some, how about this:
At the end of [en|de]codeData(, true) we trigger [en|de]codeTrailers if the trailers ptr is non-null. This should cover both encoding and decoding and we'd end up writing out the trailers to the encoder like I'm already doing. If addTrailers is called during the [en|de]codeTrailers() cb we can fail with an ASSERT.

I think this is more on the right track.

This does mean we get both a [en|de]codeData(, true) and [en|de]codeTrailers which might be confusing, but it would allow a filter to inject trailers when it can see that there aren't already any there (because it sees [en|de]codeData(, true)).

I don't think this is going to work as it will completely confuse filters that do things when end_stream is true. I think this is the behavior you want:

  1. Do what you said previously about detecting if there are trailers set during a decodeData call.
  2. If trailers get set by a decode data call, make sure that subsequent filter decodeData() calls have end_stream set to false (basically key that off of whether trailers is non-null).
  3. Then dispatch trailers.

I don't think this should be too hard to implement and should cleanly cover both encode/decode. Does that make sense?

For the error checking side of things, IMO I would probably just block trailers being added in any context other than decodeData(..., true). Does it make sense anywhere else? The otehr thing to consider is what if someone calls this when a filter has been paused and then calls continue? Will it work correctly?

@snowp snowp Aug 2, 2018

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.

Yeah that makes sense for the most part. One question:

I would probably just block trailers being added in any context other than decodeData(..., true).

Do you mean decodeData(..., false)? Allowing setting the trailers in decodeData(..., true) seems to contradict point 2

@mattklein123 mattklein123 Aug 2, 2018

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 I meant what I said, that I think the only valid context to add trailers should be within a *Data(..., true) call or outside of a direct call in a different callback before calling continue*(). What I mean by (2) is that an intermediate filter can add trailers in the context of data call with end_stream true, but subsequent filters should then see a data call with end_stream false, followed by a trailers call. I believe this captures the intent of what your filter needs to do and IMO is pretty clear. Does that make more sense?

@snowp snowp Aug 2, 2018

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.

Ah, that makes perfect sense. Thanks for clarifying.

response_encoder_->encodeData(data, false);
encodeTrailers(filter, *response_trailers_);

@mattklein123 mattklein123 Aug 8, 2018

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.

Same comment here about keeping track of which filter added the trailers?

As an aside, I'm a little confused as to why the logic in encodeHeaders() does not mirror decodeHeaders() WRT to handling continue/last filter. (And yes I know I wrote this code!) I can look into this more tomorrow or Friday, but if you feel like taking a look that would be appreciated.

@snowp snowp Aug 10, 2018

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.

I looked into it a little bit: adding the same last filter check to encodHeaders causes a test failure in HttpConnectionManagerImplTest.HitRequestBufferLimitsIntermediateFilter because of a missing expectation for a call to response_encoder.encodeHeaders, and an integration test fails due to

[2018-08-09 23:36:14.552][694871][critical][assert] bazel-out/darwin-dbg/bin/source/common/request_info/_virtual_includes/request_info_lib/common/request_info/request_info_impl.h:83] assert failure: !first_downstream_tx_byte_sent_.

which presumably comes from hitting this code more than once (towards the end of encodeHeaders):

  // Now actually encode via the codec.
  request_info_.onFirstDownstreamTxByteSent(); // <- this bit
  response_encoder_->encodeHeaders(headers,
                                   end_stream && continue_data_entry == encoder_filters_.end());

The test that's failing is IpVersions/Http2IntegrationTest.HittingDecoderFilterLimit/IPv4 - I'm guessing the filter used there normally stops iteration, but by continuing on the last filter we end up running through the last part of encodeHeaders again. Seems like the issue is that somehow encodeTrailers is being called twice, and special casing the last filter causes the second call to hit onFirstDownstreamTxByteSent.

Unclear whether this is why we don't special case the last filter in encodeHeaders, or if it's just a issue that hasn't surfaced because we don't.

@mattklein123 mattklein123 Aug 10, 2018

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.

OK, thanks for looking. I can take a look tomorrow or this weekend more. I wouldn't worry about it for now but it seemed strange to me that it wasn't symmetrical and I couldn't remember why.

} else {
response_encoder_->encodeData(data, end_stream);
maybeEndEncode(end_stream);
}
}

void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilter* filter,
Expand Down Expand Up @@ -1380,6 +1444,10 @@ Buffer::WatermarkBufferPtr ConnectionManagerImpl::ActiveStreamDecoderFilter::cre
return buffer;
}

HeaderMap& ConnectionManagerImpl::ActiveStreamDecoderFilter::addDecodedTrailers() {
return parent_.addDecodedTrailers();
}

void ConnectionManagerImpl::ActiveStreamDecoderFilter::addDecodedData(Buffer::Instance& data,
bool streaming) {
parent_.addDecodedData(*this, data, streaming);
Expand Down Expand Up @@ -1473,6 +1541,10 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::addEncodedData(Buffer::In
return parent_.addEncodedData(*this, data, streaming);
}

HeaderMap& ConnectionManagerImpl::ActiveStreamEncoderFilter::addEncodedTrailers() {
return parent_.addEncodedTrailers();
}

void ConnectionManagerImpl::ActiveStreamEncoderFilter::
onEncoderFilterAboveWriteBufferHighWatermark() {
ENVOY_STREAM_LOG(debug, "Disabling upstream stream due to filter callbacks.", parent_);
Expand Down
7 changes: 7 additions & 0 deletions source/common/http/conn_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,

// Http::StreamDecoderFilterCallbacks
void addDecodedData(Buffer::Instance& data, bool streaming) override;
HeaderMap& addDecodedTrailers() override;
void continueDecoding() override;
const Buffer::Instance* decodingBuffer() override {
return parent_.buffered_request_data_.get();
Expand Down Expand Up @@ -229,6 +230,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,

// Http::StreamEncoderFilterCallbacks
void addEncodedData(Buffer::Instance& data, bool streaming) override;
HeaderMap& addEncodedTrailers() override;
void onEncoderFilterAboveWriteBufferHighWatermark() override;
void onEncoderFilterBelowWriteBufferLowWatermark() override;
void setEncoderBufferLimit(uint32_t limit) override { parent_.setBufferLimit(limit); }
Expand Down Expand Up @@ -267,11 +269,13 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_stream);
const Network::Connection* connection();
void addDecodedData(ActiveStreamDecoderFilter& filter, Buffer::Instance& data, bool streaming);
HeaderMap& addDecodedTrailers();
void decodeHeaders(ActiveStreamDecoderFilter* filter, HeaderMap& headers, bool end_stream);
void decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instance& data, bool end_stream);
void decodeTrailers(ActiveStreamDecoderFilter* filter, HeaderMap& trailers);
void maybeEndDecode(bool end_stream);
void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming);
HeaderMap& addEncodedTrailers();
void sendLocalReply(bool is_grpc_request, Code code, const std::string& body,
std::function<void(HeaderMap& headers)> modify_headers);
void encode100ContinueHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers);
Expand Down Expand Up @@ -339,6 +343,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
// to verify we do not encode100Continue headers more than once per
// filter.
static constexpr uint32_t Encode100ContinueHeaders = 0x40;
// Used to indicate that we're processing the final [en|de]Code frame,

@mattklein123 mattklein123 Aug 14, 2018

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.

nit: [En|De]codeData

// i.e. end_stream = true
static constexpr uint32_t LastDataFrame = 0x80;
};
// clang-format on

Expand Down
177 changes: 177 additions & 0 deletions test/common/http/conn_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2271,6 +2271,183 @@ TEST_F(HttpConnectionManagerImplTest, ZeroByteDataFiltering) {
decoder_filters_[0]->callbacks_->continueDecoding();
}

TEST_F(HttpConnectionManagerImplTest, FilterAddTrailersInTrailersCallback) {
InSequence s;
setup(false, "");

EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void {
StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_);
HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}};
decoder->decodeHeaders(std::move(headers), false);

Buffer::OwnedImpl fake_data("hello");
decoder->decodeData(fake_data, false);

HeaderMapPtr trailers{new TestHeaderMapImpl{{"bazzz", "bar"}}};
decoder->decodeTrailers(std::move(trailers));
}));

setupFilterChain(2, 2);

Http::LowerCaseString trailer_key("foo");
std::string trailers_data("trailers");
EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false))
.WillOnce(Return(FilterHeadersStatus::StopIteration));
EXPECT_CALL(*decoder_filters_[0], decodeData(_, false))
.WillOnce(Return(FilterDataStatus::StopIterationAndBuffer));
EXPECT_CALL(*decoder_filters_[0], decodeTrailers(_))
.WillOnce(Return(FilterTrailersStatus::Continue));
EXPECT_CALL(*decoder_filters_[1], decodeHeaders(_, false))
.WillOnce(Return(FilterHeadersStatus::StopIteration));
EXPECT_CALL(*decoder_filters_[1], decodeData(_, false))
.WillOnce(Return(FilterDataStatus::StopIterationAndBuffer));
EXPECT_CALL(*decoder_filters_[1], decodeTrailers(_))
.WillOnce(Invoke([&](Http::HeaderMap& trailers) -> FilterTrailersStatus {
Http::LowerCaseString key("foo");
EXPECT_EQ(trailers.get(key), nullptr);
return FilterTrailersStatus::Continue;
}));

// Kick off the incoming data.
Buffer::OwnedImpl fake_input("1234");
conn_manager_->onData(fake_input, false);

// set up encodeHeaders expectations
EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false))
.WillOnce(Return(FilterHeadersStatus::Continue));
EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false))
.WillOnce(Return(FilterHeadersStatus::Continue));
EXPECT_CALL(response_encoder_, encodeHeaders(_, false));

// invoke encodeHeaders
decoder_filters_[0]->callbacks_->encodeHeaders(
HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}}}, false);

// set up encodeData expectations
EXPECT_CALL(*encoder_filters_[0], encodeData(_, false))
.WillOnce(Return(FilterDataStatus::Continue));
EXPECT_CALL(*encoder_filters_[1], encodeData(_, false))
.WillOnce(Return(FilterDataStatus::Continue));
EXPECT_CALL(response_encoder_, encodeData(_, false));

// invoke encodeData
Buffer::OwnedImpl response_body("response");
decoder_filters_[0]->callbacks_->encodeData(response_body, false);
// set up encodeTrailer expectations
EXPECT_CALL(*encoder_filters_[0], encodeTrailers(_))
.WillOnce(Return(FilterTrailersStatus::Continue));

EXPECT_CALL(*encoder_filters_[1], encodeTrailers(_))
.WillOnce(Invoke([&](Http::HeaderMap& trailers) -> FilterTrailersStatus {
// assert that the trailers set in the previous filter was ignored
Http::LowerCaseString key("foo");
EXPECT_EQ(trailers.get(key), nullptr);
return FilterTrailersStatus::Continue;
}));
EXPECT_CALL(response_encoder_, encodeTrailers(_));
expectOnDestroy();

// invoke encodeTrailers
decoder_filters_[0]->callbacks_->encodeTrailers(
HeaderMapPtr{new TestHeaderMapImpl{{"some", "trailer"}}});
}

TEST_F(HttpConnectionManagerImplTest, FilterAddTrailersInDataCallbackNoTrailers) {
InSequence s;
setup(false, "");

EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void {
StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_);
HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}};
decoder->decodeHeaders(std::move(headers), false);

Buffer::OwnedImpl fake_data("hello");
decoder->decodeData(fake_data, true);
}));

setupFilterChain(2, 2);

std::string trailers_data("trailers");
Http::LowerCaseString trailer_key("foo");
EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false))
.WillOnce(Return(FilterHeadersStatus::Continue));
EXPECT_CALL(*decoder_filters_[1], decodeHeaders(_, false))
.WillOnce(Return(FilterHeadersStatus::Continue));
EXPECT_CALL(*decoder_filters_[0], decodeData(_, true))
.WillOnce(InvokeWithoutArgs([&]() -> FilterDataStatus {
decoder_filters_[0]->callbacks_->addDecodedTrailers().addCopy(trailer_key, trailers_data);
return FilterDataStatus::Continue;
}));

// ensure that the second decodeData call sees end_stream = false
EXPECT_CALL(*decoder_filters_[1], decodeData(_, false))
.WillOnce(Return(FilterDataStatus::Continue));

// since we added trailers, we should see decodeTrailers
EXPECT_CALL(*decoder_filters_[0], decodeTrailers(_)).WillOnce(Invoke([&](HeaderMap& trailers) {
// ensure that we see the trailers set in decodeData
Http::LowerCaseString key("foo");
trailers.iterate(
[](const HeaderEntry& e, void*) -> Http::HeaderMap::Iterate {
std::cout << e.key().getStringView() << std::endl;
return Http::HeaderMap::Iterate::Continue;
},
nullptr);
auto t = trailers.get(key);
ASSERT(t);
EXPECT_EQ(t->value(), trailers_data.c_str());
return FilterTrailersStatus::Continue;
}));
EXPECT_CALL(*decoder_filters_[1], decodeTrailers(_))
.WillOnce(Return(FilterTrailersStatus::Continue));

// Kick off the incoming data.
Buffer::OwnedImpl fake_input("1234");
conn_manager_->onData(fake_input, false);

// set up encodeHeaders expectations
EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false))
.WillOnce(Return(FilterHeadersStatus::Continue));
EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false))
.WillOnce(Return(FilterHeadersStatus::Continue));
EXPECT_CALL(response_encoder_, encodeHeaders(_, false));

// invoke encodeHeaders
decoder_filters_[0]->callbacks_->encodeHeaders(
HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}}}, false);

// set up encodeData expectations
EXPECT_CALL(*encoder_filters_[0], encodeData(_, true))
.WillOnce(InvokeWithoutArgs([&]() -> FilterDataStatus {
encoder_filters_[0]->callbacks_->addEncodedTrailers().addCopy(trailer_key, trailers_data);
return FilterDataStatus::Continue;
}));
// ensure encodeData calls after setting header sees end_stream = false
EXPECT_CALL(*encoder_filters_[1], encodeData(_, false))
.WillOnce(Return(FilterDataStatus::Continue));

EXPECT_CALL(response_encoder_, encodeData(_, false));

// since we added trailers, we should see encodeTrailer callbacks
EXPECT_CALL(*encoder_filters_[0], encodeTrailers(_)).WillOnce(Invoke([&](HeaderMap& trailers) {
// ensure that we see the trailers set in decodeData
Http::LowerCaseString key("foo");
auto t = trailers.get(key);
EXPECT_EQ(t->value(), trailers_data.c_str());
return FilterTrailersStatus::Continue;
}));
EXPECT_CALL(*encoder_filters_[1], encodeTrailers(_))
.WillOnce(Return(FilterTrailersStatus::Continue));

// Ensure that we call encodeTrailers
EXPECT_CALL(response_encoder_, encodeTrailers(_));

expectOnDestroy();
// invoke encodeData
Buffer::OwnedImpl response_body("response");
decoder_filters_[0]->callbacks_->encodeData(response_body, true);
}

TEST_F(HttpConnectionManagerImplTest, FilterAddBodyInTrailersCallback) {
InSequence s;
setup(false, "");
Expand Down
Loading