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 1 commit
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
7 changes: 7 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 @@ -395,6 +395,13 @@ class StreamEncoderFilterCallbacks : public virtual StreamFilterCallbacks {
*/
virtual void addEncodedData(Buffer::Instance& data, bool streaming_filter) PURE;

/**
* Provides a trailer map that can be used to modify the trailers for a response. Used when

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

As long as we are in here and have all of this logic paged in, can we add a similar function on the decoding side? It should be symmetrical and pretty trivial to duplicate tests for.

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

yeah, will do

* trailers need to be injected into a response and the upstream response did not contain any
* trailers (in which case encodeTrailers is not called).
*/
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
21 changes: 19 additions & 2 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 @@ -1058,6 +1058,13 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
}
}

HeaderMap& ConnectionManagerImpl::ActiveStream::addEncodedTrailers() {
if (!response_trailers_) {

@snowp snowp Jul 28, 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.

this whole interaction ends up becoming somewhat complicated so i wanted to put up the PR before spending too much time on the edge cases:

the make_unique allows calling this before encodeTrailers is called, but if there are any upstream trailers then they'll be moved into this variable, overwriting what we wrote here. This means that right now this should only be called in encodeData if there are no upstream trailers. If called in encodeTrailers it should correctly modify the right map.

This similarly applies to encodeHeaders: if there are upstream trailers they will overwrite anything that was added in encodeHeaders

so my question: should we try to make this behavior more sane (e.g. try to merge added trailers with upstream trailers?) or simply document addEncodedTrailers to spell this out?

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

I commented above. IMO this is a real filter edge case, and I think I would just go around all of this and pass the new trailers by move semantics, which then overwrite. Thoughts?

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

I think that makes sense and works with our use case. Seems cleaner than to have to reason about how it may interact with existing trailers.

response_trailers_ = std::make_unique<Http::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 @@ -1099,8 +1106,14 @@ void ConnectionManagerImpl::ActiveStream::encodeData(ActiveStreamEncoderFilter*
end_stream);

request_info_.addBytesSent(data.length());
response_encoder_->encodeData(data, end_stream);
maybeEndEncode(end_stream);
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);
response_encoder_->encodeTrailers(*response_trailers_);
maybeEndEncode(true);
} else {
response_encoder_->encodeData(data, end_stream);
maybeEndEncode(end_stream);
}
}

void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilter* filter,
Expand Down Expand Up @@ -1473,6 +1486,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
2 changes: 2 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 @@ -229,6 +229,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 @@ -272,6 +273,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
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
134 changes: 134 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,140 @@ 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{{"foo", "bar"}}};
decoder->decodeTrailers(std::move(trailers));
}));

setupFilterChain(1, 2);

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::StopIteration));

// 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);
Http::LowerCaseString trailer_key("foo");
// set up encodeTrailer expectations
std::string trailers_data("trailers");
EXPECT_CALL(*encoder_filters_[0], encodeTrailers(_))
.WillOnce(InvokeWithoutArgs([&]() -> FilterTrailersStatus {
encoder_filters_[0]->callbacks_->addEncodedTrailers().addCopy(trailer_key, trailers_data);
return FilterTrailersStatus::Continue;
}));

EXPECT_CALL(*encoder_filters_[1], encodeTrailers(_))
.WillOnce(Invoke([&](Http::HeaderMap& trailers) -> FilterTrailersStatus {
auto v = trailers.get(trailer_key);
EXPECT_NE(v, nullptr);
EXPECT_EQ(v->value().getStringView(), trailers_data);
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, false);

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

setupFilterChain(1, 2);

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::StopIteration));

// 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(Return(FilterDataStatus::Continue));
std::string trailers_data("trailers");
Http::LowerCaseString trailer_key("foo");
EXPECT_CALL(*encoder_filters_[1], encodeData(_, true))
.WillOnce(InvokeWithoutArgs([&]() -> FilterDataStatus {
encoder_filters_[1]->callbacks_->addEncodedTrailers().addCopy(trailer_key, trailers_data);
return FilterDataStatus::Continue;
}));

EXPECT_CALL(response_encoder_, encodeData(_, false));
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