fix sometimes returns response body to HEAD requests

[zyfjeff]

Signed-off-by: tianqian.zyf tianqian.zyf@alibaba-inc.com

Description: fix sometimes returns response body to HEAD requests
Risk Level: low
Testing: unittests and integration test
Docs Changes:
Release Notes:
[Optional Fixes #Issue] #3932
[Optional Deprecated:]

[mattklein123]

Thanks for working on this. High level comment to get started.

[mattklein123]

High level comment: Does this need to be part of the API? Would it be better if the connection manager kept track of this and did the right thing?

[zyfjeff]

You are right, I will refactor

[mattklein123]

Thanks for working on this. In general the change looks correct, but it needs tests for all the different permutations you did this for:

1. AsyncClient
2. conn_manager_impl (early response and filter response)

[mattklein123]

Method should always be set so I don't think we need to check

[mattklein123]

Method can't be null

[mattklein123]

Method can't be null

[zyfjeff]

HttpConnectionManagerImplTest.*have many test, there is no set method header, I'm going to fix it

[mattklein123]

do we need this at both levels for early response before we have filters?

[mattklein123]

revert unrelated change

[mattklein123]

revert unrelated change

[zyfjeff]

Sorry, forgot to check before submission

[alyssawilk]

I think you have a few whitespace removals you can revert

[mattklein123]

revert all changes in file

[mattklein123]

revert

[mattklein123]

revert

[zyfjeff]

@mattklein123 conn_manager_impl call sendLocalReply is head response, the body is empty, the other HTTP filter test conn_manager_impl didn't go, I don't know how to test, some HTTP filter integration testing, walked conn_manager_impl, I already inside the test case，The AsyncClient::sendLocalReply I didn't find a place to call

Can you give me some suggestions?

[mattklein123]

The AsyncClient::sendLocalReply I didn't find a place to call

For AsyncClient, sendLocalReply() can be called in certain error cases where the router does a direct response. For example in this test (https://github.com/envoyproxy/envoy/blob/master/test/common/http/async_client_impl_test.cc#L466) I think if you change that to a HEAD request you will hit the code you modified.

@mattklein123 conn_manager_impl call sendLocalReply is head response, the body is empty, the other HTTP filter test conn_manager_impl didn't go, I don't know how to test, some HTTP filter integration testing, walked conn_manager_impl, I already inside the test case

It would be nice to have a conn_manager_impl_test for this, though nice job adding the integration tests, so I'm not going to block on it, but if you feel like it would be great. Check out https://github.com/envoyproxy/envoy/blob/master/test/common/http/conn_manager_impl_test.cc#L2880 and the tests around it. You should be able to setup a test like that, and do a local reply from within various filter callbacks which should drive the various flows.

Thank you!

[zyfjeff]

@mattklein123 AsyncClient and conn_manager_impl added some unittests

[mattklein123]

This is awesome, thanks. One small nit. @alyssawilk if you have a sec can you take a quick skim to see if you have any comments?

[mattklein123]

typo "Replay". I think maybe you mean "SendLocalReplyHeadRequest" ?

[zyfjeff]

yes, spelling mistakes

[alyssawilk]

Looks great overall. As always I'm a big fan of integration tests - I think we could msotly copy-paste IdleTimeoutIntegrationTest.PerStreamIdleTimeoutAfterDownstreamHeaders for and easy end to end regression test.

[alyssawilk]

if this is a response to a HEAD request?

[zyfjeff]

Yes, I fix it

[mattklein123]

As always I'm a big fan of integration tests

There are 2 integration tests in the change (JWT filter and RBAC filter)

[alyssawilk]

Sorry, I could have said "we could also copy-paste" as I'd mildly prefer having an integration test which does not depend on filters. The one I suggested validates the behavior in the http connection manager for all 4 protocol combinations and permutations, which I believe has value.

[mattklein123]

Ah yes, that would be good, agreed.

[zyfjeff]

@mattklein123 I met some problems when doing the integration test, has not yet been solved, can you help a look.

1. I started a Head request

  IntegrationStreamDecoderPtr setupPerStreamIdleTimeoutHeadRequestTest() {
    initialize();
    fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
    codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http"))));
    auto encoder_decoder =
        codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "HEAD"},
                                                            {":path", "/test/long/url"},
                                                            {":scheme", "http"},
                                                            {":authority", "host"}});
    request_encoder_ = &encoder_decoder.first;
    auto response = std::move(encoder_decoder.second);
    fake_upstream_connection_ = fake_upstreams_[0]->waitForHttpConnection(*dispatcher_);
    upstream_request_ = fake_upstream_connection_->waitForNewStream(*dispatcher_);
    upstream_request_->waitForHeadersComplete();
    return response;
  }

2. Whether the head only request or other types of request end_stream is false

std::pair<Http::StreamEncoder&, IntegrationStreamDecoderPtr>
IntegrationCodecClient::startRequest(const Http::HeaderMap& headers) {
  auto response = std::make_unique<IntegrationStreamDecoder>(dispatcher_);
  Http::StreamEncoder& encoder = newStream(*response);
  encoder.getStream().addCallbacks(*response);
  // Here whether the head only request or with data, is false
  encoder.encodeHeaders(headers, false);
  flushWrite();
  return {encoder, std::move(response)};
}

3. This leads to the codec for HEAD request there is no set its response to the HEAD rsponse

void StreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) {
.......

  if (end_stream) {
    endEncode();
  } else {
    connection_.flushOutput();
  }
}

void StreamEncoderImpl::endEncode() {
  if (chunk_encoding_) {
    connection_.buffer().add(LAST_CHUNK);
  }

  connection_.flushOutput();
  connection_.onEncodeComplete();
}

void ClientConnectionImpl::onEncodeComplete() {
  // Transfer head request state into the pending response before we reuse the encoder.
  pending_responses_.back().head_request_ = request_encoder_->headRequest();
}

4. The final test to find the response->compete() always returns false

[2018-08-06 11:23:15.466][16169][trace][http] source/common/http/http1/codec_impl.cc:341] [C48] parsing 0 bytes
[2018-08-06 11:23:15.466][16169][info][client] source/common/http/codec_client.cc:117] [C48] protocol error: http/1.1 protocol error: HPE_INVALID_EOF_STATE
[2018-08-06 11:23:15.466][16169][debug][client] source/common/http/codec_client.cc:81] [C48] disconnect. resetting 1 pending requests
[2018-08-06 11:23:15.466][16169][debug][client] source/common/http/codec_client.cc:104] [C48] request reset
[2018-08-06 11:23:15.466][16169][trace][main] source/common/event/dispatcher_impl.cc:126] item added to deferred deletion list (size=1)
[2018-08-06 11:23:15.466][16169][trace][main] source/common/event/dispatcher_impl.cc:52] clearing deferred deletion list (size=1)
test/integration/idle_timeout_integration_test.cc:104: Failure
Value of: response->complete()
  Actual: false
Expected: true

5. I repaired by changing the code

std::pair<Http::StreamEncoder&, IntegrationStreamDecoderPtr>
IntegrationCodecClient::startRequest(const Http::HeaderMap& headers) {
  auto response = std::make_unique<IntegrationStreamDecoder>(dispatcher_);
  Http::StreamEncoder& encoder = newStream(*response);
  encoder.getStream().addCallbacks(*response);
 // Determine whether it is a HEAD request to set the value of end_stream
  bool end_stream = (headers.Method()->value().c_str() == Http::Headers::get().MethodValues.Head);
  encoder.encodeHeaders(headers, end_stream);
  flushWrite();
  return {encoder, std::move(response)};
}

6. Test found that waitForDisconnect can not return

[2018-08-06 11:28:09.849][22301][trace][http] source/common/http/http1/codec_impl.cc:341] [C48] parsing 130 bytes
[2018-08-06 11:28:09.849][22301][trace][http] source/common/http/http1/codec_impl.cc:439] [C48] message begin
[2018-08-06 11:28:09.849][22301][trace][http] source/common/http/http1/codec_impl.cc:309] [C48] completed header: key=content-length value=14
[2018-08-06 11:28:09.849][22301][trace][http] source/common/http/http1/codec_impl.cc:309] [C48] completed header: key=content-type value=text/plain
[2018-08-06 11:28:09.849][22301][trace][http] source/common/http/http1/codec_impl.cc:309] [C48] completed header: key=date value=Mon, 06 Aug 2018 11:28:09 GMT
[2018-08-06 11:28:09.849][22301][trace][http] source/common/http/http1/codec_impl.cc:405] [C48] headers complete
[2018-08-06 11:28:09.849][22301][trace][http] source/common/http/http1/codec_impl.cc:309] [C48] completed header: key=server value=envoy
[2018-08-06 11:28:09.849][22301][trace][http] source/common/http/http1/codec_impl.cc:426] [C48] message complete
[2018-08-06 11:28:09.849][22301][trace][http] source/common/http/http1/codec_impl.cc:729] [C48] message complete
[2018-08-06 11:28:09.849][22301][debug][client] source/common/http/codec_client.cc:94] [C48] response complete
[2018-08-06 11:28:09.849][22301][trace][main] source/common/event/dispatcher_impl.cc:126] item added to deferred deletion list (size=1)
[2018-08-06 11:28:09.849][22301][trace][http] source/common/http/http1/codec_impl.cc:362] [C48] parsed 130 bytes
[2018-08-06 11:28:09.849][22301][trace][main] source/common/event/dispatcher_impl.cc:52] clearing deferred deletion list (size=1)
[2018-08-06 11:28:09.849][22443][trace][connection] source/common/network/raw_buffer_socket.cc:67] [C49] write returns: 130
[2018-08-06 11:28:14.638][22438][debug][main] source/server/server.cc:119] flushing stats
[2018-08-06 11:28:19.639][22438][debug][main] source/server/server.cc:119] flushing stats
[2018-08-06 11:28:24.639][22438][debug][main] source/server/server.cc:119] flushing stats
[2018-08-06 11:28:29.639][22438][debug][main] source/server/server.cc:119] flushing stats
[2018-08-06 11:28:34.640][22438][debug][main] source/server/server.cc:119] flushing stats

[alyssawilk]

If you stick the whole test log with -l trace to pastebin I'll take a look. If there's no disconnect it's possible Envoy thinks it has handled the full request/response pair. Given startRequest I'd think it should be in "handling request" when the idle timeout comes and so end the connection. Given we (unfortunately) allow GET with body I assume we allow HEAD with body, but if I can see full logs I might be able to help you puzzle out what's going on.

[zyfjeff]

@alyssawilk test.log, Connection should be closed, I didn't see related connection through the netstat command information, through the log view, seems to be caused by connection was closed early.

I modify IntegrationCodecClient: : startRequest of encoder.EncodeHeaders (headers, false) To the encoder EncodeHeaders (headers, true) in latest master code, Also appear the same problem, log as follows
test.log.2

Modify the diff content is as follows:

diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc
index 3f93b90..c756b49 100644
--- a/test/integration/http_integration.cc
+++ b/test/integration/http_integration.cc
@@ -137,7 +137,7 @@ IntegrationCodecClient::startRequest(const Http::HeaderMap& headers) {
   auto response = std::make_unique<IntegrationStreamDecoder>(dispatcher_);
   Http::StreamEncoder& encoder = newStream(*response);
   encoder.getStream().addCallbacks(*response);
-  encoder.encodeHeaders(headers, false);
+  encoder.encodeHeaders(headers, true);
   flushWrite();
   return {encoder, std::move(response)};
 }

Thank you very much can you help me to look at this problem

[zyfjeff]

@mattklein123 @alyssawilk I agree with your view, I re-adjusted the call point of connection_.onEncodeComplete() and put it in the place where the request header was received.

[alyssawilk]

Also sorry, I should have thought of this before, but is this the same as setupPerStreamIdleTimeoutTest() apart from the method? If so, what do you think of changing setupPerStreamIdleTimeoutTest() -> setupPerStreamIdleTimeoutTest(cont char* method = "GET") to reduce similar code?

[mattklein123]

nit: needs braces around this if statement.

I'm still not a fan of calling onEncodeComplete here as the encode is not necessarily complete. Can we just pass the state directly? This is what I was referring to previously. Can the the RequestStreamEncoderImpl hold a reference to the ClientConnection such that we can transfer this state directly? I think that might be cleaner?

[zyfjeff]

You actually think that the name of onEncodeComplete will have some ambiguity. In fact, there is no complete encode.

Can I directly dynamic_cast from ConnectionImpl to ClientConnectionImpl and pass the state directly?

[mattklein123]

This should always be true. If we are going to do the dynamic cast we can just assume it is valid, though I'm not a huge fan of dynamic_cast for something like this. The other option is to store ClientConncetion in RequestStreamEncoderImpl (and server connection in ResponseStreamEncoderImpl and then have a base class virtual function connection() which returns the connection base class. @alyssawilk any opinions here?

[alyssawilk]

I'd suggest that just like we have a virtual onEncodeComplete that the client/server connections implement, we could have a pure virtual onEncodeHeaders. We could handle passing on head status in the ClientConnectionImpl and the ServerConnectionImpl could just be a no-op for now.

I think that would be consistent with the style of the rest of the code base, and make it clear what part of the request/response pipeline is being handled.

[alyssawilk]

Looking good - almost there!

[alyssawilk]

mild preference for moving this to the .h file

[alyssawilk]

onEncodeHeaders for consistency with encodeHeaders?

[alyssawilk]

Sorry, I think there was a misunderstanding on this one.
I was asking the name go from onEncodeHeader -> onEncodeHeaders (add an "s")
We don't need the same arguments, just Header with an S :-)
If you can remove the extra argument and change the name that would be great!

[alyssawilk]

LGTM once this comment is addressed :-)

[zyfjeff]

Sorry, I have never seen this comment (onEncodeHeader -> onEncodeHeaders (add an "s"), so I have not paid attention to this PR in recent days.

[alyssawilk]

Called when headers are encoded

[alyssawilk]

I think you can simplify this as
void onEncodeHeader(const HeaderMapImpl&) override { }

[alyssawilk]

I think you have a few whitespace removals you can revert

[zyfjeff]

@alyssawilk @mattklein123 Thank you for some review suggestions.

[mattklein123]

Generally looks great to me. Thank you! Will defer to @alyssawilk for further review/merge.

[zyfjeff]

@alyssawilk If this PR wants to merge, what else do I need to do?

[alyssawilk]

Cool, almost set. Please do one more master merge to pick up clang-6* and this should be good to go.

https://groups.google.com/forum/#!topic/envoy-dev/Y15Dd3MQ7Wc