http_filter: add addEncodedTrailers and addDecodedTrailers #3980

snowp · 2018-07-28T14:57:36Z

Adds a function that allows inserting trailers into the stream response
outside of the encodeTrailers callback. This allows inserting trailers
when the upstream response has no trailers to begin with (e.g. h/1.1).

Signed-off-by: Snow Pettersen snowp@squareup.com

Risk Level: Low, new optional feature
Testing: Unit tests
Docs Changes: Doc string on public interface
Release Notes: n/a

Fixes: #3966

Adds a function that allows inserting trailers into the stream response outside of the encodeTrailers callback. This allows inserting trailers when the upstream response has no trailers to begin with (e.g. h/1.1). Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp · 2018-07-28T14:59:55Z

include/envoy/http/filter.h

+   * 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;


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?

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

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

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?

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.

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

snowp · 2018-07-28T15:06:01Z

source/common/http/conn_manager_impl.cc

@@ -1058,6 +1058,13 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
  }
 }

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


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?

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?

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.

snowp · 2018-07-28T15:08:30Z

Known limitation: if the upstream response is headers only then trailers won't be written even if addEncodedTrailers is called

Can attempt to fix this if necessary

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123

Some initial comments so we can flesh out the API, then I can do into the implementation/tests. Thank you!

mattklein123 · 2018-07-30T17:35:18Z

include/envoy/http/filter.h

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


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.

yeah, will do

mattklein123 · 2018-07-30T17:36:22Z

include/envoy/http/filter.h

+   * 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;


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?

mattklein123 · 2018-07-30T17:37:10Z

source/common/http/conn_manager_impl.cc

@@ -1058,6 +1058,13 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
  }
 }

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


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?

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp · 2018-07-31T23:57:19Z

Updated this to use move semantics and include a addDecodedTrailers. as per @lizan's comment, does it make sense to restrict this to only be allowed during on[De/En]codedData? I don't really see a use case for calling this in onTrailers (presumably you'd just modify the trailers param)

mattklein123 · 2018-08-01T17:05:00Z

source/common/http/conn_manager_impl.cc

@@ -1099,18 +1109,25 @@ 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_) {


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?

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

Trailers are set during header/data when upstream has no trailers: trailers get written out at the end of [en|de]codeData(_, true)

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(_)

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

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:

Do what you said previously about detecting if there are trailers set during a decodeData call.

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

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?

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

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?

Ah, that makes perfect sense. Thanks for clarifying.

mattklein123 · 2018-08-01T17:05:37Z

as per @lizan's comment, does it make sense to restrict this to only be allowed during on[De/En]codedData?

Yes I think we should force this, but see my other comment first. I still think we need to figure out semantics.

lizan · 2018-08-02T00:49:22Z

include/envoy/http/filter.h

+  /**
+   * Adds decoded trailers. This will overwrite any existing trailers should any already exist.
+   */
+  virtual void addDecodedTrailers(HeaderMapPtr&& trailers) PURE;


still -1 to moving a pointer here, and prefer the first revision: HeaderMap& addDecodedTrailers() for reasons:

moving a pointer doesn't prevent filters from keeping reference to HeaderMap itself too long, so that still can happen. Returning a reference is already clear that Envoy core (ActiveStream) owns the HeaderMap.

the filter will now have to know implementations of HeaderMap (i.e. HeaderMapImpl) and instantiate, I know there is already a couple places doing this, mostly to deal with AsyncHttpClient, perhaps we shouldn't add another place requiring that?

@mattklein123 thoughts?

moving a pointer doesn't prevent filters from keeping reference to HeaderMap itself too long, so that still can happen. Returning a reference is already clear that Envoy core (ActiveStream) owns the HeaderMap.

IMO move semantics make it clear that the pointer is now owned by the callee. We do this in a variety of places.

the filter will now have to know implementations of HeaderMap (i.e. HeaderMapImpl) and instantiate, I know there is already a couple places doing this, mostly to deal with AsyncHttpClient, perhaps we shouldn't add another place requiring that?

I think this is a better argument. Assuming with the new semantics we have discussed it's just as easy to return a reference to either the existing trailers or a newly created trailers, it sounds reasonable to me to switch it back to what we had before.

The one thing that's unclear to me is what would HeaderPtr& addEncodedTrailers() return if called before encodeTrailers(..., true)? would we just throw/assert?

Yes, I would throw some type of bad argument exception that will cause Envoy to crash and can be checked in tests. (I would probably not derive from EnvoyException and pick a std exception for this).

also cause decodeTrailers to get triggered Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp · 2018-08-02T05:52:18Z

source/common/http/conn_manager_impl.cc

+void ConnectionManagerImpl::ActiveStream::addEncodedTrailers(HeaderMapPtr&& trailers) {
+  if (state_.filter_call_state_ & FilterCallState::LastDataFrame) {
+    response_trailers_ = std::move(trailers);
+    state_.local_complete_ = false;


this was necessary because state_.local_complete_ gets set to true at the start of encodeData, so we have to undo that so that the ASSERT in commonEncodePrefix doesn't fire at the start of encodeTrailers

If it's worth a comment in the PR it's worth a comment in the code :-)

snowp · 2018-08-02T05:56:28Z

updated to use the semantics as discussed in comments:

addEncodedTrailers/addDecodedTrailers only affects trailers during encodeData(..., true)/decodeData(..., true)
if trailers have been added, all subsequent encodeData calls will see end_stream = false
if trailers have been added, encodeTrailers/decodeTrailers will be called on the new trailers

when called outside of the required context it currently just drops the provided trailers on the floor. open to suggestions on to better handle this

alyssawilk

Very cool change - I think it'll be a helpful feature to have!

alyssawilk · 2018-08-02T14:40:23Z

source/common/http/conn_manager_impl.cc

+    bool end_stream_no_trailers = end_stream && !request_trailers_;
+    if (end_stream_no_trailers) {
+      state_.filter_call_state_ |= FilterCallState::LastDataFrame;
+    }


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

What does HCM stand for in this context?

Http Connection Manager (HCM)

alyssawilk · 2018-08-02T14:45:49Z

source/common/http/conn_manager_impl.cc

@@ -775,15 +775,36 @@ void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter*

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


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

alyssawilk · 2018-08-02T14:48:21Z

source/common/http/conn_manager_impl.cc

    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;
    }
  }
+
+  if (end_stream && request_trailers_) {


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

alyssawilk · 2018-08-02T14:50:06Z

source/common/http/conn_manager_impl.cc

+    request_trailers_ = std::move(trailers);
+  } else {
+    // do nothing
+    // todo: should this throw/assert?


I think it should :-)

I also think we should have integration tests for both the happy path and over-enthusiastic filters. I think we can start on those now, because even if the APIs change a bit the end to end tests should stay the same

+1 same as my comment elsewhere. We should be doing this in a bunch of places (see some other TODOs from me) so we might as well start doing it now w/ tests.

alyssawilk · 2018-08-02T14:51:43Z

source/common/http/conn_manager_impl.cc

+void ConnectionManagerImpl::ActiveStream::addEncodedTrailers(HeaderMapPtr&& trailers) {
+  if (state_.filter_call_state_ & FilterCallState::LastDataFrame) {
+    response_trailers_ = std::move(trailers);
+    state_.local_complete_ = false;


If it's worth a comment in the PR it's worth a comment in the code :-)

alyssawilk · 2018-08-02T14:58:08Z

include/envoy/http/filter.h

+  /**
+   * Adds decoded trailers. This will overwrite any existing trailers should any already exist.
+   */
+  virtual void addDecodedTrailers(HeaderMapPtr&& trailers) PURE;


Is this out of date? With the code as it stands now, I think we can only call under decodeData with end_stream = true, and subsequent filters will never encounter that state (end stream will be false for future invocations) so should not be able to overwrite trailers.

I do think we should document when this is safe to call but if you prefer a TODO while the design is being hashed out that's fine :-)

alyssawilk · 2018-08-02T15:00:07Z

source/common/http/conn_manager_impl.cc

+
+void ConnectionManagerImpl::ActiveStream::addDecodedTrailers(HeaderMapPtr&& trailers) {
+  if (state_.filter_call_state_ & FilterCallState::LastDataFrame) {
+    request_trailers_ = std::move(trailers);


If I'm correct above that we can only set request_trailers_once now, can we also have an invariant ASSERT that they're empty when this is called?

mattklein123

Few responses, not a full review. I think we are now converged on design, so let me know when the next set of changes are pushed and I will take a really detailed look. Thanks for working on this!

mattklein123 · 2018-08-03T16:15:22Z

include/envoy/http/filter.h

+  /**
+   * Adds decoded trailers. This will overwrite any existing trailers should any already exist.
+   */
+  virtual void addDecodedTrailers(HeaderMapPtr&& trailers) PURE;


Yes, I would throw some type of bad argument exception that will cause Envoy to crash and can be checked in tests. (I would probably not derive from EnvoyException and pick a std exception for this).

mattklein123 · 2018-08-03T16:15:49Z

source/common/http/conn_manager_impl.cc

+    bool end_stream_no_trailers = end_stream && !request_trailers_;
+    if (end_stream_no_trailers) {
+      state_.filter_call_state_ |= FilterCallState::LastDataFrame;
+    }


mattklein123 · 2018-08-03T16:16:42Z

source/common/http/conn_manager_impl.cc

+    request_trailers_ = std::move(trailers);
+  } else {
+    // do nothing
+    // todo: should this throw/assert?


+1 same as my comment elsewhere. We should be doing this in a bunch of places (see some other TODOs from me) so we might as well start doing it now w/ tests.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

alyssawilk

Looking much cleaner - thanks for all the comments! I am still looking for integration tests when you have a chance - I think it'd be good to have tests both of the happy path and things like bad filters double-adding trailers causing disconnects rather than crashes :-)

alyssawilk · 2018-08-07T14:45:05Z

include/envoy/http/filter.h

-  virtual void addDecodedTrailers(HeaderMapPtr&& trailers) PURE;
+   /**
+    * Adds decoded trailers. May only be called in decodeData when end_stream is set to true or in
+    * decodeTrailers. If called in any other context, std::logic_error will be thrown.


Can you call this under decodeTrailers? isn't decodeTrailers the wrong state (not LastDataFrame) and would have request_trailers_ non-null (exception either way)?

Also I'm not sure if we're consistent about this but can you add a javadoc style @throws here and below?

Yeah you're right - I was misunderstanding when request_trailers_ was set in the normal flow. The test failures were due to exactly this.

I'll add in the @throws as well.

alyssawilk · 2018-08-07T14:48:02Z

source/common/http/conn_manager_impl.cc

  if (state_.filter_call_state_ & FilterCallState::LastDataFrame) {
-    request_trailers_ = std::move(trailers);
+    if (request_trailers_) {
+      throw std::logic_error("decodedTrailers added more than once");


So what will catch these exceptions? Perhaps they should be CodecProtocolException so that the existing codec code will catch them and terminate the connection?

I picked a non-EnvoyException exception as per @mattklein123 suggestion in a previous comment (#3980 (comment)). If we want this to fail more gracefully then I'm happy to do so, just want to make sure we're all in agreement before switching over.

IMO this is the right exception type so that we don't catch it and crash/core dump. I think a logic error like this should be caught during development time and should be very obvious?

Sorry, to be clear, you think this exception should not be caught? I grant with our current example filters it should be fairly obvious, but once folks have custom filters which do non-trivial async work which may have timing invariants, and/or we make the pipeline more complicated with internal redirects, I think it gets less obvious. I'd strongly prefer developer errors cause disconnects and not crashes.

Yes, I think it should not be caught. If you catch this exception you will need to add stats and logging to make it obvious what is happening otherwise it will be incredibly confusing. Basically, it's a bunch of error handling and operational work that IMO should not happen in practice. If there is disagreement on using an exception for this I would switch to RELEASE_ASSERT and make it crash.

So the tl;dr here is that I should be using ASSERTs instead of exceptions, deferring better error handling to a future change?

@alyssawilk Given this, would you still want integration tests? It'd only test the happy path (unless we can test asserts somehow?), but it might still be worth having?

tl;dr sounds right!

Yeah, I think given this we're just testing the happy path. One theoretically could do a debug death test for broken filters but even I think that'd be overkill :-)

Cool I'll look into it. Could you point me to some integration tests I could extend/based mine off to cover these changes?

Out of curiosity: what would a "debug death test" look like?

We've got a bunch of tests which add filters (see config_helper_.addFilter()) so what we'll need to do is add a test (probably in http_integration.cc and then called from the various http[2][_upstream]integration_test.cc) which does makeRequestWithBody() and then verifies header/body/data get proxied downstream and then the flip side, upstream_request->encodeHeaders() and encodeData with trailers being received by H2 downstream.

The trickier bit is we don't have a sample filter which does this, so you'll need to add a test filter which always adds trailers. In the long run I'd like a test filter factory which creates MockStreamFilter which we can use that for all sorts of custom test behavior but I think that's out of scope for this PR :-) If you do the simple one I'll hopefully get some free time to convert it to the fancy version later in.

Actually it occurs to me thinking about makeRequestWithBody vs makeHeaderOnlyRequest.... we allow adding trailers on decodeData(end_stream = true) but should we also allow it in decodeHeaders (end_stream=true)? It seems more consistent for H2 but if we're really only wanting this for gRPC and we think we won't hit gRPC with no body where we want to add trailers, I'd be OK documenting it as a known limitation someone else can fix when/if they need it.

Thanks for the pointers, I'll try to get some tests set up.

I can't really think of a use case of adding trailers to request/response with no DATA frames (how is that even different from just adding additional headers?). I'd prefer to to just leave it as it is right now. Adding it in later should be fairly straightforward once all the unit tests/integration tests are in place.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

alyssawilk

Hurrah for integration tests! They've prompted a couple of new thoughts along with a few nits I missed earlier.

alyssawilk · 2018-08-08T18:33:43Z

include/envoy/http/filter.h

+   *
+   * 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.
+   */


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

alyssawilk · 2018-08-08T18:37:34Z

source/common/http/conn_manager_impl.cc

@@ -792,7 +792,8 @@ void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter*
    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


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?

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

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

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?

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?

alyssawilk · 2018-08-08T18:39:04Z

source/common/http/conn_manager_impl.cc

-  } else {
-    throw std::logic_error("addDecodedTrailers called in invalid context");
-  }
+  // traileres can only be added once


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

alyssawilk · 2018-08-08T18:40:37Z

test/integration/add_trailers_filter.h

+  Http::FilterDataStatus decodeData(Buffer::Instance&, bool end_stream) override;
+
+  Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap&) override {
+    std::cout << "trailers cb" << std::endl;


clean up debug printing?

whops yeah I'll remove

alyssawilk · 2018-08-08T18:42:55Z

test/integration/http2_integration_test.cc

@@ -38,6 +38,8 @@ TEST_P(Http2IntegrationTest, MultipleContentLengths) { testMultipleContentLength

 TEST_P(Http2IntegrationTest, ComputedHealthCheck) { testComputedHealthCheck(); }

+TEST_P(Http2IntegrationTest, AddEncodedTrailers) { testAddEncodedTrailers(); }


It's a weird request but mind adding this to integration_test as well? I'd just like end to end coverage that if we add trailers over-enthusiastically on the upstream or downstream side that we still get a complete response.

not weird at all! i was going to but it slipped my mind

this actually caught a bug, so it was a great idea

mattklein123

Thanks for iterating on this. These changes are difficult/scary but well worth it.

mattklein123 · 2018-08-08T20:28:58Z

source/common/http/conn_manager_impl.cc

@@ -792,7 +792,8 @@ void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter*
    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


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 · 2018-08-08T20:30:40Z

source/common/http/conn_manager_impl.cc

+  // 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_) {
+    decodeTrailers(filter, *request_trailers_);


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

mattklein123 · 2018-08-08T20:31:40Z

source/common/http/conn_manager_impl.cc

+  // to allow filters to process the trailers.
+  if (end_stream && response_trailers_) {
+    response_encoder_->encodeData(data, false);
+    encodeTrailers(filter, *response_trailers_);


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.

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.

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.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123

Looking very close. Very nice.

mattklein123 · 2018-08-11T17:09:35Z

source/common/http/conn_manager_impl.cc

@@ -775,15 +777,50 @@ 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


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

mattklein123 · 2018-08-11T17:10:46Z

source/common/http/conn_manager_impl.cc

@@ -767,6 +767,8 @@ void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter*
  }

  std::list<ActiveStreamDecoderFilterPtr>::iterator entry;
+  auto trailers_added_entry = decoder_filters_.end();
+  bool trailers_exists = request_trailers_ != nullptr;


s/trailers_exists/trailers_exists_at_start (or something like that)

mattklein123 · 2018-08-11T17:11:23Z

source/common/http/conn_manager_impl.cc

+
+    if (!(*entry)->commonHandleAfterDataCallback(status, data, state_.decoder_filters_streaming_) &&
+        std::next(entry) != decoder_filters_.end()) {
+      // break here to ensure that we call decodeTrailers below


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?

mattklein123 · 2018-08-11T17:12:10Z

source/common/http/conn_manager_impl.cc

+  // If trailers were adding during decodeData we need to trigger decodeTrailers in order
+  // to allow filters to process the trailers.
+  if (trailers_added_entry != decoder_filters_.end()) {
+    (*trailers_added_entry)->stopped_ = true;


Why not just call decodeTrailers(...) here? What's the reason to stop and then continue?

calling decodeTrailers seems to work just as well so I'm changing it back to that

mattklein123 · 2018-08-11T17:12:42Z

source/common/http/conn_manager_impl.cc

  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


Same comment about the comment and the local variable.

mattklein123 · 2018-08-11T17:12:59Z

source/common/http/conn_manager_impl.cc

+  // to allow filters to process the trailers.
+  if (trailers_added_entry != encoder_filters_.end()) {
+    response_encoder_->encodeData(data, false);
+    (*trailers_added_entry)->stopped_ = true;


Same comment about why stop/continue?

Signed-off-by: Snow Pettersen <snowp@squareup.com>

alyssawilk

This LGTM but I'm now officially out of my depth w.r.t http connection manger internals, so I'll defer to Matt from here on :-)

Thanks for tackling this complicated feature - it'll be a really nice enhancement for gRPC!

mattklein123

Thanks, this is awesome. Really great work. Couple of nits then let's ship!

mattklein123 · 2018-08-14T17:52:04Z

source/common/http/conn_manager_impl.cc

@@ -767,6 +767,8 @@ void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter*
  }

  std::list<ActiveStreamDecoderFilterPtr>::iterator entry;
+  auto trailers_added_entry = decoder_filters_.end();
+  bool trailers_exists_at_start = request_trailers_ != nullptr;


mattklein123 · 2018-08-14T17:52:16Z

source/common/http/conn_manager_impl.cc

@@ -1083,13 +1133,33 @@ void ConnectionManagerImpl::ActiveStream::encodeData(ActiveStreamEncoderFilter*
                                                     Buffer::Instance& data, bool end_stream) {
  resetIdleTimer();
  std::list<ActiveStreamEncoderFilterPtr>::iterator entry = commonEncodePrefix(filter, end_stream);
+  auto trailers_added_entry = encoder_filters_.end();
+
+  bool trailers_exists_at_start = response_trailers_ != nullptr;


mattklein123 · 2018-08-14T17:53:21Z

source/common/http/conn_manager_impl.h

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


nit: [En|De]codeData

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123

Nice!

* origin/master: http_filter: add addEncodedTrailers and addDecodedTrailers (envoyproxy#3980) rbac/fuzz: fix build (envoyproxy#4150) Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp commented Jul 28, 2018

View reviewed changes

mock out new method

688bc4d

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123 assigned mattklein123 and alyssawilk Jul 30, 2018

mattklein123 reviewed Jul 30, 2018

View reviewed changes

add addDecodedTrailers and use move semantics

e0b74f3

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp changed the title ~~http_filter: add addEncodedTrailers~~ http_filter: add addEncodedTrailers and addDecodedTrailers Jul 31, 2018

mattklein123 reviewed Aug 1, 2018

View reviewed changes

lizan reviewed Aug 2, 2018

View reviewed changes

only allow trailers added in decodeData(true)

acfa3d8

also cause decodeTrailers to get triggered Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp commented Aug 2, 2018

View reviewed changes

alyssawilk reviewed Aug 2, 2018

View reviewed changes

mattklein123 reviewed Aug 3, 2018

View reviewed changes

snowp added 2 commits August 3, 2018 19:11

throw exceptions on bad access + add comments

baee8aa

Signed-off-by: Snow Pettersen <snowp@squareup.com>

return by reference, throw exception on invalid usage

42443b0

Signed-off-by: Snow Pettersen <snowp@squareup.com>

alyssawilk requested changes Aug 7, 2018

View reviewed changes

snowp added 5 commits August 7, 2018 11:49

expect exception in encode/decodeTrailers + add @throws comment

52c5780

Signed-off-by: Snow Pettersen <snowp@squareup.com>

asserts instead of exceptions

8495e41

Signed-off-by: Snow Pettersen <snowp@squareup.com>

add integration test for addEncodedTrailers

36103a1

Signed-off-by: Snow Pettersen <snowp@squareup.com>

fix addDecodedTrailers

f6ca600

Signed-off-by: Snow Pettersen <snowp@squareup.com>

only check for upstream trailers when upstream supports trailers

483b48b

Signed-off-by: Snow Pettersen <snowp@squareup.com>

alyssawilk requested changes Aug 8, 2018

View reviewed changes

mattklein123 reviewed Aug 8, 2018

View reviewed changes

only execute callback for filters after filter that added trailers

5412bb8

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp added 6 commits August 9, 2018 18:19

also keep track of filter for decode

cd25726

Signed-off-by: Snow Pettersen <snowp@squareup.com>

test adding trailers in integration_test.cc

c3a900b

Signed-off-by: Snow Pettersen <snowp@squareup.com>

fixup comments

90c6233

Signed-off-by: Snow Pettersen <snowp@squareup.com>

formating

3667d6a

Signed-off-by: Snow Pettersen <snowp@squareup.com>

remove debug line

7637fa3

Signed-off-by: Snow Pettersen <snowp@squareup.com>

add @return comments

f2b6210

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123 reviewed Aug 11, 2018

View reviewed changes

update comments and other feedback

815a07b

Signed-off-by: Snow Pettersen <snowp@squareup.com>

alyssawilk previously approved these changes Aug 13, 2018

View reviewed changes

mattklein123 reviewed Aug 14, 2018

View reviewed changes

add const qualifier and update comment

2321031

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp dismissed alyssawilk’s stale review via 2321031 August 14, 2018 17:56

mattklein123 approved these changes Aug 14, 2018

View reviewed changes

mattklein123 merged commit 73bd3d9 into envoyproxy:master Aug 14, 2018

		@@ -38,6 +38,8 @@ TEST_P(Http2IntegrationTest, MultipleContentLengths) { testMultipleContentLength

		TEST_P(Http2IntegrationTest, ComputedHealthCheck) { testComputedHealthCheck(); }

		TEST_P(Http2IntegrationTest, AddEncodedTrailers) { testAddEncodedTrailers(); }

http_filter: add addEncodedTrailers and addDecodedTrailers #3980

http_filter: add addEncodedTrailers and addDecodedTrailers #3980

Conversation

snowp commented Jul 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp commented Jul 28, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp commented Jul 31, 2018

Choose a reason for hiding this comment

snowp Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp commented Aug 2, 2018 • edited Loading

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp Jul 28, 2018 •

edited

Loading

snowp Aug 1, 2018 •

edited

Loading

snowp Aug 2, 2018 •

edited

Loading

snowp commented Aug 2, 2018 •

edited

Loading

alyssawilk Aug 7, 2018 •

edited

Loading