http/2: correctly handle 100-continue #562

mattklein123 · 2017-03-11T00:22:07Z

Previously we did not correctly handle "expect: 100-continue" in the http/2
codec. Because of protocol differences, we handle 100-continue directly in the
codec. This change implements support in the http/2 codec similar to http/1.1
where we immediately send a continue response.

This commit also adds partial support for the client codec for 1xx responses.
The code will raise decodeHeaders(...) twice in the 1xx response case. This
is good enough for testing, but will probably crash higher layers if a client
actually responded this way. Right now we effectively trust clients so this
can be handled in a follow up if necessary.

fixes #561

Previously we did not correctly handle "expect: 100-continue" in the http/2 codec. Because of protocol differences, we handle 100-continue directly in the codec. This change implements support in the http/2 codec similar to http/1.1 where we immediately send a continue response. This commit also adds partial support for the client codec for 1xx responses. The code will raise decodeHeaders(...) twice in the 1xx response case. This is good enough for testing, but will probably crash higher layers if a client actually responded this way. Right now we effectively trust clients so this can be handled in a follow up if necessary. fixes #561

mattklein123 · 2017-03-11T00:23:18Z

@lyft/network-team @jpinner-lyft @andrewwebber

ccaraman · 2017-03-11T00:34:07Z

source/common/http/http2/codec_impl.cc

+          stream->waiting_for_final_headers_ = false;
+
+          // This can only happen in the client case in a response, when we received a 1xx to
+          // start out with. In this case, raise as headers. nghttp2 message checking guarentees


typo guarantees

mattklein123 · 2017-03-11T03:41:36Z

@htuch could you give this a look also please.

htuch

Do any Envoy use cases motivate forwarding the 100-continue to the backend? It seems that in general the original intent of the expect header was to have the proxy forward it and then determine from the backend whether or not to reply 100.

This might be legacy by now, and it certainly would hurt performance to round-trip unconditionally.

htuch · 2017-03-13T21:14:41Z

source/common/http/filter/ratelimit.cc

-const Http::HeaderMapPtr Filter::TOO_MANY_REQUESTS_HEADER{new Http::HeaderMapImpl{
-    {Http::Headers::get().Status, std::to_string(enumToInt(Code::TooManyRequests))}}};
+const std::unique_ptr<const Http::HeaderMap> Filter::TOO_MANY_REQUESTS_HEADER{
+    new Http::HeaderMapImpl{


The Google C++ style guide doesn't like globals like this, since unjoined threads might observe destruction when main() is done. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables.

Depending on whether this is something we want in the Envoy coding style (maybe we can assume we always have sane threading behavior and everything joins before the end?), this could be addressed by making this a function that returns the results of a static new* allocation of the object.

We do correctly join all threads before exiting main. I'm not a huge fan of the raw pointer approach because AFAIK this will cause the heap leak detector to fail on standard settings. I don't feel super strongly about this, but we do this in other places also. Thoughts?

There's also the concern about non-deterministic ordering of initialization. Not really a concern for simple local objects like this.

I think that when done sanely (e.g. don't initialize static data that depends on other static data), and given we have proper thread joining on exit, it's probably fine.

It would make sense to document this in https://github.com/lyft/envoy/blob/master/CONTRIBUTING.md though.

Yes, for init, we need to more careful, and we do handle more complex cases in init by having functions that return static variables. I will do a doc update.

htuch · 2017-03-13T21:15:50Z

source/common/http/http2/codec_impl.cc

@@ -28,6 +32,9 @@ bool Utility::reconstituteCrumbledCookies(const HeaderString& key, const HeaderS

 ConnectionImpl::Http2Callbacks ConnectionImpl::http2_callbacks_;
 ConnectionImpl::Http2Options ConnectionImpl::http2_options_;
+const std::unique_ptr<const Http::HeaderMap> ConnectionImpl::CONTINUE_HEADER{


Same as above (regarding static non-POD).

htuch · 2017-03-13T21:24:41Z

source/common/http/http2/codec_impl.cc

+      StreamImpl::buildHeaders(final_headers, *CONTINUE_HEADER);
+      int rc = nghttp2_submit_headers(session_, 0, stream->stream_id_, nullptr, &final_headers[0],
+                                      final_headers.size(), nullptr);
+      ASSERT(rc == 0);


Is this definitely a failure that can be ignored if not-debug?

Based on logic flow, this function should never fail, so I made it a debug only assert. We do this in a few other places in this file.

What about NGHTTP2_ERR_NOMEM?

None of the Envoy code is safe for memory allocation failure, so we don't handle similar errors anywhere and assume we will shortly crash if OOM. (This is a design philosophy, we should discuss this outside of the context of this change if there are concerns).

Sure, we can discuss this elsewhere (I do worry about libraries using ENOMEM in non-OOM scenarios, as opposed to tcmalloc). In any case, could you also include this design philosophy in https://github.com/lyft/envoy/blob/master/CONTRIBUTING.md when updating? Thanks.

Yes that is a valid concern and might necessitate some paranoia checking in certain cases which would crash us if we receive ENOMEM, etc. I will open a GH issue to track as well as updating contributing guide.

htuch · 2017-03-13T22:22:25Z

test/common/http/http2/codec_impl_test.cc

+  response_encoder->encodeHeaders(response_headers, true);
+}
+
+TEST_P(Http2CodecImplTest, ExpectContinueTrailersResponse) {


Do you cover the case where the non-100 :status is in the HEADERS immediately following the 100 :status?

What do you mean exactly? Like if Envoy immediately responds with 404 right after sending 100?

Yeah, but thinking about it a bit more, I think you have good coverage of the new code paths in these tests, so it's good as is.

mattklein123 · 2017-03-13T23:10:35Z

Do any Envoy use cases motivate forwarding the 100-continue to the backend? It seems that in general the original intent of the expect header was to have the proxy forward it and then determine from the backend whether or not to reply 100.

This might be legacy by now, and it certainly would hurt performance to round-trip unconditionally.

I don't know of any currently, and we already do direct response for HTTP/1.1. Being able to correctly proxy the 1xx through from server to client would be a giant change. My preference would be to do this and then if we actually find a case that requires 100 continue server support (I really doubt this will happen) we can cover it then.

htuch · 2017-03-14T13:50:29Z

source/common/http/http2/codec_impl.cc

+      StreamImpl::buildHeaders(final_headers, *CONTINUE_HEADER);
+      int rc = nghttp2_submit_headers(session_, 0, stream->stream_id_, nullptr, &final_headers[0],
+                                      final_headers.size(), nullptr);
+      ASSERT(rc == 0);


What about NGHTTP2_ERR_NOMEM?

htuch · 2017-03-14T13:57:48Z

source/common/http/http2/codec_impl.cc

+    case NGHTTP2_HCAT_RESPONSE: {
+      uint64_t response_code = Http::Utility::getResponseStatus(*stream->headers_);
+      if (CodeUtility::is1xx(response_code)) {
+        stream->waiting_for_final_headers_ = true;


Suggest renaming to waiting_for_non_informational_headers_, "informational" is the terminology used in RFCs. Although I notice nghttp2 uses the terminology final/non-final - if you want to stick with that then maybe a comment where this field is defined.

htuch · 2017-03-14T13:59:01Z

source/common/http/http2/codec_impl.cc

+
+    switch (frame->headers.cat) {
+    case NGHTTP2_HCAT_RESPONSE: {
+      uint64_t response_code = Http::Utility::getResponseStatus(*stream->headers_);


General preference to const for an intermediate value like this.

htuch · 2017-03-14T14:08:19Z

source/common/http/http2/codec_impl.cc

+          // proper flow here.
+          // TODO(mattklein123): Higher layers don't currently deal with a double decodeHeaders()
+          // call and will probably crash. We do this for testing the server case. We correctly
+          // handle the server case. In the future, if needed, we can properly handle 1xx in higher


Not clear what is meant by the server case here - how does the server receive a 100 response, isn't it the one generating the 100 response?

htuch · 2017-03-14T14:12:00Z

test/common/http/http2/codec_impl_test.cc

+
+  MockStreamDecoder request_decoder;
+  StreamEncoder* response_encoder;
+  EXPECT_CALL(server_callbacks_, newStream(_))


Suggest refactoring this encoder/decoder setup into the Http2CodecImplTest class, it seems the boiler plate is now in quite a few test cases.

htuch · 2017-03-14T14:17:10Z

test/common/http/http2/codec_impl_test.cc

+  response_encoder->encodeHeaders(response_headers, true);
+}
+
+TEST_P(Http2CodecImplTest, ExpectContinueTrailersResponse) {


Yeah, but thinking about it a bit more, I think you have good coverage of the new code paths in these tests, so it's good as is.

htuch · 2017-03-14T14:24:04Z

test/common/upstream/outlier_detection_impl_test.cc

@@ -77,6 +77,7 @@ TEST_F(OutlierDetectorImplTest, DetectorStaticConfig) {
  )EOF";

  Json::ObjectPtr custom_config = Json::Factory::LoadFromString(json);
+  EXPECT_CALL(*interval_timer_, enableTimer(std::chrono::milliseconds(100)));


Is this a related change?

Just a missing EXPECT_CALL from a previous change. I added it here since innocuous. Can move to a separate PR if you prefer.

Sure, sneak it in, just curious what was happening.

mattklein123 · 2017-03-16T16:29:38Z

@lyft/network-team @htuch updated

ccaraman reviewed Mar 11, 2017

View reviewed changes

fix

8d8b0df

htuch reviewed Mar 13, 2017

View reviewed changes

Merge branch 'master' into http2_expect_continue

b75429a

htuch reviewed Mar 14, 2017

View reviewed changes

mattklein123 added 3 commits March 15, 2017 15:15

Merge remote-tracking branch 'origin/master' into http2_expect_continue

3dc432a

Merge remote-tracking branch 'origin/master' into http2_expect_continue

e83f44e

comments

ad0afc5

ccaraman approved these changes Mar 16, 2017

View reviewed changes

mattklein123 merged commit 1d5035f into master Mar 16, 2017

mattklein123 deleted the http2_expect_continue branch March 16, 2017 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http/2: correctly handle 100-continue #562

http/2: correctly handle 100-continue #562

mattklein123 commented Mar 11, 2017

mattklein123 commented Mar 11, 2017

ccaraman Mar 11, 2017

mattklein123 commented Mar 11, 2017

htuch left a comment

htuch Mar 13, 2017

mattklein123 Mar 13, 2017

htuch Mar 14, 2017

mattklein123 Mar 14, 2017 •

edited

Loading

htuch Mar 13, 2017

htuch Mar 13, 2017

mattklein123 Mar 13, 2017

htuch Mar 14, 2017

mattklein123 Mar 14, 2017

htuch Mar 14, 2017

mattklein123 Mar 14, 2017

htuch Mar 13, 2017

mattklein123 Mar 13, 2017 •

edited

Loading

htuch Mar 14, 2017

mattklein123 commented Mar 13, 2017

htuch Mar 14, 2017

htuch Mar 14, 2017

htuch Mar 14, 2017

htuch Mar 14, 2017

htuch Mar 14, 2017

htuch Mar 14, 2017

htuch Mar 14, 2017

mattklein123 Mar 14, 2017

htuch Mar 15, 2017

mattklein123 commented Mar 16, 2017

http/2: correctly handle 100-continue #562

http/2: correctly handle 100-continue #562

Conversation

mattklein123 commented Mar 11, 2017

mattklein123 commented Mar 11, 2017

Choose a reason for hiding this comment

mattklein123 commented Mar 11, 2017

htuch 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 Mar 14, 2017 • 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

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 Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Mar 13, 2017

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 commented Mar 16, 2017

mattklein123 Mar 14, 2017 •

edited

Loading

mattklein123 Mar 13, 2017 •

edited

Loading