http: allow decoding/encoding a header only request/response #4885

snowp · 2018-10-28T23:26:58Z

This adds a new return value to encode-/decodeHeaders that indicates that
filter iteration should immediately stop, dropping any pending data/trailers.
This results in a header only request/response, which can be used to
terminate stream processing early, eg in case of invalid
request or response.

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

Description:
Risk Level: Medium due to HCM modifications
Testing: Unit test ensuring the correct encoding happens and that further data is dropped.
Docs Changes: n/a
Release Notes: n/a
Fixes #4800

This adds a new return value to encodeHeaders that indicates that filter iteration should immediately stop, dropping any pending data/trailers. This results in a header only request/response, which can be used to terminate stream processing early, eg in case of invalid request or response. Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123

Thanks for working on this. Some initial comments to get started.

mattklein123 · 2018-10-29T17:12:56Z

test/common/http/conn_manager_impl_test.cc

@@ -2700,6 +2700,40 @@ TEST_F(HttpConnectionManagerImplTest, FilterHeadReply) {
  conn_manager_->onData(fake_input, false);
 }

+TEST_F(HttpConnectionManagerImplTest, FilterHeaderOnly) {


Please make sure you have test coverage for all permutations:

Header only request/response

Request/response ended with data frame

Request/response ended with trailers

mattklein123 · 2018-10-29T17:21:49Z

source/common/http/conn_manager_impl.cc

@@ -716,15 +716,16 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte
    entry = std::next(filter->entry());
  }

-  for (; entry != decoder_filters_.end(); entry++) {
+  for (; entry != decoder_filters_.end() && !decoding_headers_only_; entry++) {


Is this correct? I think you still need to call remaining filters with decode/encodeHeaders with end_stream true?

Not clear to me which one is the more correct. As written this change stops filter iteration completely, but I can see why you'd want subsequent filters to allow modifying the headers as well. Happy to change it if the latter makes more sense in the context of the rest of the filter API.

If we're gonna continue the filters I think we'd need to rename the enum return value, maybe something like ContinueHeadersOnly?

mattklein123 · 2018-11-03T23:46:21Z

Marking as waiting.

/wait

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

mattklein123

Thanks, generally LGTM. Few questions and small nits.

mattklein123 · 2018-11-07T18:11:55Z

include/envoy/http/filter.h

-  StopIteration
+  StopIteration,
+  // Continue iteration to remaining filters, but ignore and subsequent data or trailers. This
+  // results


nit: reflow comment

mattklein123 · 2018-11-07T18:13:46Z

source/common/http/conn_manager_impl.h

@@ -409,6 +409,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
    // is ever called, this is set to true so commonContinue resumes processing the 100-Continue.
    bool has_continue_headers_{};
    bool is_head_request_{false};
+    bool decoding_headers_only_{};


nit: Can you normalize initialization of is_head_request_ above. Either make them all {false} or all empty braces? Don't care which. (As long as you are here.)

Done. Maybe it would be possible to add a linter check to check that we're being consistent with initializing members to the default value?

mattklein123 · 2018-11-07T18:43:31Z

source/common/http/conn_manager_impl.cc

@@ -758,6 +760,11 @@ void ConnectionManagerImpl::ActiveStream::decodeData(Buffer::Instance& data, boo

 void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter* filter,
                                                     Buffer::Instance& data, bool end_stream) {
+  // If we previously decided to decode only the headers, do nothing here.


I wonder if we should do this after resetting idle timer below?

I have a more general question which I would need to do some refreshing to answer, but in the case where we switch a request/response to header only, and then "finish" the request/response, do we correct reset a downstream/upstream stream that we hasn't finished yet? From a very quick look at the router and HCM, I think we do, but could you check this also to see what you think?

This is probably because I'm not familiar enough with the stream semantics, but what do you mean by reset in this case? Are you talking about making sure the stream is properly cleaned up after we do this transformation? Or is there some kind of explicit reset action that you're expecting to happen here?

Yeah, exactly, that proper cleanup happens in all cases when we translate the request/response to headers only but we have data/trailers still coming. I'm not convinced this is completely correct. I need to spend a little time looking at code which I will do later today or tomorrow and then I can help explain my thinking better. Sorry for the delay.

I spent some time looking through the code because I also have similar concerns to @alyssawilk that we might not be handling dropping body/trailer data correctly in all the different permutations. @snowp for reference this is all the calls that ultimately end up calling doDeferredStreamDestroy() if you want to look through those call chains. This is also related to correct computation of local_complete_ and remote_complete_ which indicate whether a reset has to happen. Now, I've convinced myself that the code will handle all of this properly on both the HCM and Router side of things in the sense that if we end a request early, while body is still streaming, we will respond and then reset the stream which I think is correct. And I think we will similarly handle this on the upstream side.

With that said, I think having integration tests here that cover the various permutations are very important. So an example important integration test looks something like like:

Fake client starts request without end_stream

Filter changes to header only request

Upstream starts a response without end_stream

Downstream sends body data without end_stream

Upstream sends body with end_stream

This should result in a downstream response followed by a reset for HTTP/2 or a connection close for HTTP/1.

@snowp sorry this is pretty complicated. LMK if this is enough to go on.

Also, I do think the returns have to be after we reset the stream idle timer in all cases since I think we should count body that we are dropping as not idle. Can we test that also, unit test is fine.

After the writing the integration tests it seems like it is reseting the stream but there's a segfault happening for HTTP/1 when we try to access the response encoder to reset it in https://github.com/envoyproxy/envoy/pull/4885/files#diff-240648b2ff3a8c5e19e5c4f57d079c2eR1711. I was able to get the test to go green by by avoiding the reset by setting local_complete, but I'm not sure if that's the right thing to do

Ping on this I still think we want to return below resetIdleTimer() ?

I was able to get the test to go green by by avoiding the reset by setting local_complete, but I'm not sure if that's the right thing to do

Sorry where are we talking about?

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

lizan · 2018-11-09T08:42:25Z

include/envoy/http/filter.h

+  StopIteration,
+  // Continue iteration to remaining filters, but ignore and subsequent data or trailers. This
+  // results in creating a header only request/response.
+  ContinueHeadersOnly


I would prefer ContinueAndEndStream to HeadersOnly, matches gRPC term more (as it calls "Trailers-Only reponse"), and IMO clearer what it does. We may need ContinueAndEndStream for FilterDataStatus as well in future.

Name change that @lizan proposed SGTM.

lizan · 2018-11-09T08:46:37Z

source/common/http/conn_manager_impl.cc

@@ -721,11 +721,13 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte
    ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeHeaders));
    state_.filter_call_state_ |= FilterCallState::DecodeHeaders;
    FilterHeadersStatus status = (*entry)->decodeHeaders(
-        headers, end_stream && continue_data_entry == decoder_filters_.end());
+        headers,
+        decoding_headers_only_ || (end_stream && continue_data_entry == decoder_filters_.end()));


Do we want to allow decodeHeaders(headers, end_stream=true) to return ContinueHeadersOnly? I don't think we should, so add an ASSERT?

+1 on ASSERTing this.

alyssawilk

I'd definitely like to see end to end tests, with filters doing this both on the request path and response path if we decide to support both.

I'd specifically like to make sure we correctly ending the streams when we're not draining the body, closing up HTTP/1 connections etc. Though while this strikes me as a reasonable thing to do on the response path (to your point on gRPC translation) it's somewhat confusing on the request path. If we're ignoring a POST body, we either close the stream (bad) or should maybe stop reading, or drain? Maybe it should be a response-only return - while it's unfortunate to lose parallel handling on encode/decode I also don't know that it's worth the complexity if we don't think it's necessary.

mattklein123 · 2018-11-09T18:52:18Z

I'd definitely like to see end to end tests, with filters doing this both on the request path and response path if we decide to support both.

+1. In general this is a good opportunity to add multi-filter integration tests for HTTP which should be easy with the new fake filter stuff. See also my related comments here: #4889. cc @qiannawang. I think we could add tests that cover that PR also pretty easily in the same effort.

I'd specifically like to make sure we correctly ending the streams when we're not draining the body, closing up HTTP/1 connections etc. Though while this strikes me as a reasonable thing to do on the response path (to your point on gRPC translation) it's somewhat confusing on the request path. If we're ignoring a POST body, we either close the stream (bad) or should maybe stop reading, or drain? Maybe it should be a response-only return - while it's unfortunate to lose parallel handling on encode/decode I also don't know that it's worth the complexity if we don't think it's necessary.

This is my concern also, and I've been looking at the code this morning to make myself feel better (will comment on that separately). I think we are OK but integration tests would make me feel better. My general feeling is that even though it might be weird for a request/POST, I don't see any real difference between that and a response with a large body, so IMO we should support the symmetry.

alyssawilk · 2018-11-09T19:01:52Z

Yeah, I was planning on a second look around timers and timing metrics once e2e tests happened.

The one concern I have about large requests that doesn't translate to large responses, is around request lifetime. On the response path barring a really weird buffering/delay filter we're about to kill the stream pretty much immediately but on the request path we've got all the backend think time to be reading bytes from the client and black-holding them and doing unreported work makes me leery. I was tempted to ask that we readDisable the stream but that creates a bunch of other unfortunate conrner cases which think are more likely to cause problems than those black-holed bytes :-/

I almost want to retcon my suggestion of an extra filter response code, fix your TODO About sendLocalReply not going through filters and say that's a more optimal way to handle this.

mattklein123 · 2018-11-09T19:39:18Z

I almost want to retcon my suggestion of an extra filter response code, fix your TODO About sendLocalReply not going through filters and say that's a more optimal way to handle this.

I don't think that fully covers this case though? We are genuinely trying to change a request/response into a header only request/response? This seems like a reasonable thing to do?

I was tempted to ask that we readDisable the stream but that creates a bunch of other unfortunate conrner cases which think are more likely to cause problems than those black-holed bytes :-/

Yeah agreed. IMO if we do this feature we should black hole.

alyssawilk · 2018-11-09T19:44:06Z

#4800 specifically says the issue is converting responses so sendLocalReply wouldn't do everything this PR does but it would handle the only use case we have in the open issue

mattklein123 · 2018-11-09T19:54:20Z

#4800 specifically says the issue is converting responses so sendLocalReply wouldn't do everything this PR does but it would handle the only use case we have in the open issue

I see, got it. In general I think this is a cleaner / more general solution so my vote would be to keep going but I don't feel that strongly about it.

snowp · 2018-11-09T20:45:56Z

Happy to keep iterating on this and adding adequate integration tests. I'll probably spend some time on this tomorrow.

TODO About sendLocalReply not going through filters

What TODO is this? Would this allow sending a local reply during encoding? I recall trying that at one point and running into some ASSERTs.

alyssawilk · 2018-11-09T20:55:21Z

Oh this one - someone on my team stumbled on it the other day:
https://github.com/envoyproxy/envoy/blob/master/source/common/http/conn_manager_impl.cc#L993
It might make sense to go this route if we were going for response-only conversions. Sounds like Matt prefers the bidi solution and I'm good either way so feel free to stick with what you have :-)

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

lizan · 2018-11-11T00:35:57Z

test/common/http/conn_manager_impl_test.cc

@@ -2757,7 +2757,7 @@ TEST_F(HttpConnectionManagerImplTest, FilterContinueHeadersOnlyData) {

  EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false))
      .WillOnce(InvokeWithoutArgs(
-          [&]() -> FilterHeadersStatus { return FilterHeadersStatus::ContinueHeadersOnly; }));
+          [&]() -> FilterHeadersStatus { return FilterHeadersStatus::ContinueAndEndStream; }));


Return(FilterHeadersStatus::ContinueAndEndStream) instead of InvokeWithoutArgs (throughout)

lizan · 2018-11-11T00:39:02Z

test/common/http/conn_manager_impl_test.cc

+
+  EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void {
+    StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_);
+    HeaderMapPtr headers{


make_unique wherever possible

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

snowp · 2018-11-15T06:40:46Z

Hmm for some reason there's a test failure in one of the integration tests on ipv6 only, will dig into it.

alyssawilk

Matt's got this mostly covered - just a few extra test/comment requests from me. Thanks again for tackling such a complicated one :-)

alyssawilk · 2018-11-15T15:34:15Z

source/common/http/conn_manager_impl.h

@@ -408,7 +408,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
    // By default, we will assume there are no 100-Continue headers. If encode100ContinueHeaders
    // is ever called, this is set to true so commonContinue resumes processing the 100-Continue.
    bool has_continue_headers_{};
-    bool is_head_request_{false};
+    bool is_head_request_{};
+    bool decoding_headers_only_{};


would you mind commenting what these are used for and when they are set?

alyssawilk · 2018-11-15T15:39:52Z

source/common/http/conn_manager_impl.cc

@@ -1107,7 +1135,7 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
    (*continue_data_entry)->stopped_ = true;
    (*continue_data_entry)->continueEncoding();
  } else {
-    maybeEndEncode(end_stream);
+    maybeEndEncode(encoding_headers_only_ || end_stream);


are we handling maybeEndEncode and maybeEndDecode the same for the header-only case? If not, maybe worth a comment why we treat them differently?

I think we're handling them differently just because they're called at different locations: maybeEndDecode is called before filters are evaluated, while maybeEndEncode is called after. I'll go over this and add some comments to make this easier to understand

Actually this makes me think we might not be calling endDecode properly, will investigate

alyssawilk · 2018-11-15T15:40:50Z

source/common/http/conn_manager_impl.cc

+  } else if (status == FilterHeadersStatus::ContinueAndEndStream) {
+    // Set headers_only to true so we know to end early if necessary,
+    // but continue filter iteration so we actually write the headers/run the cleanup code.
+    headers_only = true;


Consider a debug log here - we're pretty consistent about logging headers/body/trailers so logging that we'll be swallowing them is probably helpful for debugging

alyssawilk · 2018-11-15T15:45:44Z

test/integration/http_integration.cc

+                                         128);
+  waitForNextUpstreamRequest();
+  upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "503"}}, false);
+  upstream_request_->encodeData(128, true);


Can we make sure to test that if we don't encode a full body (but Envoy sends a full header-only response) that the upstream stream/connection gets reset? ditto on the decode path

Yeah sure. Do you happen to know how to explicitly test that? Is waitForDisconnect on the upstream req what you were thinking about?

I think its waitForDisconnect/waitForReset based on upstream being H1/H2, like testRouterDownstreamDisconnectBeforeRequestComplete does.

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

mattklein123

Thanks, looks great. Two small things.

/wait

mattklein123 · 2018-11-18T19:24:13Z

include/envoy/http/filter.h

@@ -27,7 +27,10 @@ enum class FilterHeadersStatus {
  // Do not iterate to any of the remaining filters in the chain. Returning
  // FilterDataStatus::Continue from decodeData()/encodeData() or calling
  // continueDecoding()/continueEncoding() MUST be called if continued filter iteration is desired.
-  StopIteration
+  StopIteration,
+  // Continue iteration to remaining filters, but ignore and subsequent data or trailers. This


mattklein123 · 2018-11-18T19:25:27Z

source/common/http/conn_manager_impl.cc

@@ -648,6 +648,12 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,

  decodeHeaders(nullptr, *request_headers_, end_stream);

+  // Normally we end decode at the start of this function, but if the request was turned into
+  // a header only request we need to set this flag here.
+  if (encoding_headers_only_) {


Is there a test that would have caught this missing case? Sorry the review history is not showing me a delta diff so there might be.

I accidentally force pushed so I guess the history broke :(

Turns out we don't need this, it's not even right (it's checking encode_headers_only_, not decode), so I've removed it.

I thought it was related to the failures I was seeing on the ipv6 tests, but turns out those were just related to encoding data to the upstream_request after it had been closed due to the response being converted to a headers only. My change to remove the calls to encodeData(true) in the encoder integration tests fixed that, so turns out this maybeEndDecode bit was unnecessary.

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

mattklein123

Great work!

…oxy#4885) This adds a new return value to encodeHeaders that indicates that filter iteration should immediately stop, dropping any pending data/trailers. This results in a header only request/response, which can be used to terminate stream processing early, eg in case of invalid request or response. Signed-off-by: Snow Pettersen <snowp@squareup.com> Signed-off-by: Fred Douglas <fredlas@google.com>

Fixing the HCM fuzzer to avoid cases where its decodeHeaders method returns FilterHeadersStatus::ContinueAndEndStream when called with end_stream=true. This assert was added in #4885, to avoid misuse of the FilterHeadersStatus::ContinueAndEndStream value. Risk Level: Low - tests only Testing: Changed HCM fuzzer test code Fixes oss fuzz issue 21971 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…y#11497) Fixing the HCM fuzzer to avoid cases where its decodeHeaders method returns FilterHeadersStatus::ContinueAndEndStream when called with end_stream=true. This assert was added in envoyproxy#4885, to avoid misuse of the FilterHeadersStatus::ContinueAndEndStream value. Risk Level: Low - tests only Testing: Changed HCM fuzzer test code Fixes oss fuzz issue 21971 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…y#11497) Fixing the HCM fuzzer to avoid cases where its decodeHeaders method returns FilterHeadersStatus::ContinueAndEndStream when called with end_stream=true. This assert was added in envoyproxy#4885, to avoid misuse of the FilterHeadersStatus::ContinueAndEndStream value. Risk Level: Low - tests only Testing: Changed HCM fuzzer test code Fixes oss fuzz issue 21971 Signed-off-by: Adi Suissa-Peleg <adip@google.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

mattklein123 self-assigned this Oct 28, 2018

mattklein123 requested changes Oct 29, 2018

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 3, 2018

snowp added 4 commits November 5, 2018 21:07

Merge branch 'master' into header-only

259cde2

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

continue header iteration instead of returning immediately

7ea1664

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

improve test coverage

c8fb131

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

format

0122b27

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

repokitteh-read-only bot removed the waiting label Nov 6, 2018

mattklein123 reviewed Nov 7, 2018

View reviewed changes

mattklein123 added the waiting label Nov 7, 2018

comment + make initializers consistent

5eb4cd9

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

repokitteh-read-only bot removed the waiting label Nov 8, 2018

lizan reviewed Nov 9, 2018

View reviewed changes

lizan mentioned this pull request Nov 9, 2018

Turning upstream response into header only downstream response #4800

Closed

alyssawilk reviewed Nov 9, 2018

View reviewed changes

mattklein123 added the waiting label Nov 9, 2018

rename enum value and add assert

1575210

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

repokitteh-read-only bot removed the waiting label Nov 10, 2018

reset idle timer even if we are not encoding data/trailers

3f4d517

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

lizan reviewed Nov 11, 2018

View reviewed changes

add integration tests

c104aaa

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

snowp added 4 commits November 14, 2018 23:12

clang-tidy and buildifier

82d0c44

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

untidy

6c0ea6b

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

header order

e36e993

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

move idle timer up, move local_complete_ closer to filter eval

5f3ff84

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

alyssawilk reviewed Nov 15, 2018

View reviewed changes

mattklein123 added the waiting label Nov 15, 2018

call maybeEndDecode and document fields

15d0c71

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

repokitteh-read-only bot removed the waiting label Nov 16, 2018

snowp added 3 commits November 16, 2018 13:24

format

ec72f47

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

wait for reset in encoding tests

d0a771b

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

Merge branch 'header-only' of github.com:snowp/envoy into header-only

ddce29e

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

snowp force-pushed the header-only branch from ddce29e to ec72f47 Compare November 17, 2018 16:17

add debug log

36c1f08

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

mattklein123 requested changes Nov 18, 2018

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 18, 2018

remove unnecessary code, fix nit

7c14b69

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

repokitteh-read-only bot removed the waiting label Nov 19, 2018

mattklein123 approved these changes Nov 24, 2018

View reviewed changes

mattklein123 merged commit 8c6a401 into envoyproxy:master Nov 24, 2018

adisuissa mentioned this pull request Jun 8, 2020

Fixing HCM fuzzer ContinueAndEndStream with end_stream set #11497

Merged

http: allow decoding/encoding a header only request/response #4885

http: allow decoding/encoding a header only request/response #4885

Conversation

snowp commented Oct 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

mattklein123 commented Nov 3, 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

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 left a comment

Choose a reason for hiding this comment

mattklein123 commented Nov 9, 2018

alyssawilk commented Nov 9, 2018

mattklein123 commented Nov 9, 2018

alyssawilk commented Nov 9, 2018

mattklein123 commented Nov 9, 2018

snowp commented Nov 9, 2018

alyssawilk commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp commented Nov 15, 2018

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

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

mattklein123 left a comment

Choose a reason for hiding this comment