http: adding 100-Continue support to Envoy by alyssawilk · Pull Request #2497 · envoyproxy/envoy

alyssawilk · 2018-01-31T22:23:03Z

By default Envoy will still strip Expect: 100-Continue and send the 100 Continue response, but for each HttpConnectionManager configured to proxy_100_continue it will pass the header through and proxy the 100 Continue from upstream.

Risk Level: Medium-High (http connection manager refactor)
Testing: thorough unit tests and integration tests
Docs Changes: envoyproxy/data-plane-api#455
Fixes #2325
[API Changes:] envoyproxy/data-plane-api#433

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2018-01-31T22:25:37Z

@mattklein123 I think I have the critical tests working (integration + http connection manager resumption) so while I should add 1-2 more for coverage completeness do you mind taking a pass at this? I definitely wanted your take because even avoiding duplicate encode/decodeHeaders this was more complicated than I'd initially expected!

Sorry for the size - I couldn't figure out a way to split it up but I'm open to ideas.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

This is neat. Generally looks great to me. Some random comments. Great work!

mattklein123 · 2018-02-01T17:52:56Z

+   * Encode 100-Continue headers.
+   * @param headers supplies the 100-Continue header map to encode.
+   */
+  virtual void encode100ContinueHeaders(const HeaderMap& headers) PURE;


General design question: Do you think it's worthwhile passing around the HeaderMap everywhere? Is anyone ever going to look at it or do anything with it? I'm wondering if we could just make this void, and then have the header map that is encoded by effectively static on either side? Thoughts?

I don't ever expect to do anything with it. I couldn't find anything in the spec about 100 continue only being firstline, and IMO if it's allowed to have other headers we're better off passing it around than changing the API in a year when someone wants to add trace headers or some such.

Fair enough. SGTM.

mattklein123 · 2018-02-01T17:54:14Z

  void continueDecoding() override { NOT_IMPLEMENTED; }
  void addDecodedData(Buffer::Instance&, bool) override { NOT_IMPLEMENTED; }
  const Buffer::Instance* decodingBuffer() override { return buffered_body_.get(); }
+  // The async client won't pause if sending an Expect: 100-Continue so simply


Is it worth asserting somewhere that no one actually sends expect continue on async client? Might be a nice sanity check somewhere.

I can imagine use cases where we might legitimately get one (forwarding logging requests with original headers) so I think swallowing is the right thing to do.

mattklein123 · 2018-02-01T17:55:35Z

+  if (CodeUtility::is1xx(response_code)) {
+    connection_manager_.stats_.named_.downstream_rq_1xx_.inc();
+    connection_manager_.listener_stats_.downstream_rq_1xx_.inc();
+  }


nit: else if (or change the following to match)

this one still needs fixing.

LGTM other than this one. Can we clean this up and make it consistent one way or the other?

Oh!

I kept putting these lines together and not noticing that fix_format undid my change because I didn't change it to "else if". "But I swear I fixed this! Twice!"

Actually fixed :-)

mattklein123 · 2018-02-01T17:56:31Z

+      request_headers_->Expect()->value() == Headers::get().ExpectValues._100Continue.c_str()) {
+    // Note in the case Envoy is handling 100-Continue complexity, it skips the filter chain
+    // and sends the 100-Continue directly to the encoder.
+    continue_headers_ =


create a static continue header to use?

mattklein123 · 2018-02-01T17:56:49Z

+    // Note in the case Envoy is handling 100-Continue complexity, it skips the filter chain
+    // and sends the 100-Continue directly to the encoder.
+    continue_headers_ =
+        HeaderMapPtr{new HeaderMapImpl{{Headers::get().Status, std::to_string(100)}}};


nit: instead of 100 using the constant in codes.h would be better.

mattklein123 · 2018-02-01T19:39:33Z

-          // responds with 1xx. In the future, if needed, we can properly handle 1xx in higher layer
-          // code, or just eat it.
+          if (stream->headers_->Status() && stream->headers_->Status()->value() == "100") {
+            throw CodecProtocolException("Unexpected 100-continue in header continuation");


Can this happen? I think nghttp2 blocks this?

mattklein123 · 2018-02-01T19:39:52Z

-      stream->decoder_->decodeHeaders(std::move(stream->headers_), stream->remote_end_stream_);
+      if (stream->headers_->Status() && stream->headers_->Status()->value() == "100") {
+        if (stream->remote_end_stream_) {
+          throw CodecProtocolException("Unexpected end stream with 100-Continue headers.");


nghttp2 likely checks this. Can this happen?

mattklein123 · 2018-02-01T19:40:21Z

+
+  downstream_response_started_ = true;
+  // We will double count response codes for 100-Continue.
+  upstream_request_->upstream_host_->outlierDetector().putHttpResponseCode(100);


nit: use constant in codes.h, also can you verify the outlier detector code isn't going to do something stupid with a 100 status code and maybe add some tests there? (Not even sure if we should send 100 to the outlier detector, I guess that is worth discussing).

Now that I think of it, given 100-Continue is timing based (clients can choose to post data right away, then servers nto send it) and likely rare, I'm inclined to not send it. I don't think it's worth an extra interface and it breaks the 'calling things twice' policy

mattklein123 · 2018-02-01T19:44:48Z

+  downstream_response_started_ = true;
+  // We will double count response codes for 100-Continue.
+  upstream_request_->upstream_host_->outlierDetector().putHttpResponseCode(100);
+  // Don't send retries after 100-Continue has been sent on.


technically, I think we could still retry by resending expect: continue again, and eating the next 100 continue, but I agree this is overcomplicated and not necessary. I might expand this comment a bit though about that potential and why we are disabling retry, with an optional TODO to implement retry if you feel that would ever happen.

I'd argue that's not the right behavior. Technically someone could have different backends with different behaviors and the next would not accept. I mean it's not the end of the world but it is the start of a response. Will definitely comment.

mattklein123 · 2018-02-01T19:47:30Z

  bool generate_request_id_;
  Http::DateProvider& date_provider_;
  Http::ConnectionManagerListenerStats listener_stats_;
+  bool proxy_100_continue_;


nit: const (see if other members can be constified)

Hahaha. I added the config after I did my const check. Will double check the rest in case.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

…rame Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

LGTM, few more comments. There also still some nits from my previous review that need action but now I'm thinking you just didn't get around to them yet so I stopped commenting on them.

mattklein123 · 2018-02-02T17:30:58Z

      runtime_(runtime), local_info_(local_info), cluster_manager_(cluster_manager),
      listener_stats_(config_.listenerStats()) {}

+const std::unique_ptr<const Http::HeaderMap> ConnectionManagerImpl::CONTINUE_HEADER{


Technically I think this breaks the non-POD rule (though I don't care). Might want to expose via private function and use CONSTRUCT_ON_FIRST_USE.

Yeah, I was mentally claiming it was moving an existing non-POD header map but I should really just clean as I move it.

mattklein123 · 2018-02-02T17:31:11Z

+  if (CodeUtility::is1xx(response_code)) {
+    connection_manager_.stats_.named_.downstream_rq_1xx_.inc();
+    connection_manager_.listener_stats_.downstream_rq_1xx_.inc();
+  }


this one still needs fixing.

mattklein123 · 2018-02-02T17:34:58Z

+  }
+
+  // Stip the T-E headers etc. Defer other header additions as well as drain-close logic to the
+  // continuation headers.


Do we care that this function will get called twice in the proxy 100 continue case? Now it doesn't matter but might it matter in the future? Might be worth putting some comments in the code to watch out for this.

mattklein123 · 2018-02-02T17:37:28Z

    Buffer::WatermarkBufferPtr buffered_response_data_;
    HeaderMapPtr response_trailers_{};
-    HeaderMapPtr request_headers_;
+    HeaderMapPtr request_headers_{};


still needs fixing

htuch · 2018-02-05T03:53:02Z

Please merge master following #2495.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 · 2018-02-06T17:00:19Z

@alyssawilk FYI I think this will break envoy-filter-example. Can you fix the filter there when this merges? (Or maybe you already did that and I'm forgetting).

alyssawilk · 2018-02-06T18:11:04Z

Doesn't one of our builds test that envoy-filter-example works? I remember CI catching a failure when I was refactoring the integration tests.
Looking visually, the filter examples implement Http::StreamDecoderFilter which I'm not touching so I think that's OK.

mattklein123 · 2018-02-06T18:14:12Z

I can never remember which sequence breaks things. Ah OK I thought this change affected all filters but I guess not.

alyssawilk · 2018-02-06T18:14:51Z

Yeah, asan and tsan
cd "${ENVOY_FILTER_EXAMPLE_SRCDIR}"
then build
//:echo2_integration_test //:envoy_binary_test so we should be good to go.

junr03 · 2018-02-08T22:01:37Z

Deploying at Lyft right now and seeing crashes. In the words of @mattklein123

The issue is the Expect header is not removed here: https://github.com/envoyproxy/envoy/pull/2497/files#diff-d70ea2d9706e0246700148b2e2bb63ceR458
Like it used to be in the codecs previously

alyssawilk · 2018-02-08T22:50:43Z

Argh! Sorry :-(
#2560

This reverts commit 235d477. Signed-off-by: Matt Klein <mklein@lyft.com>

…proxy#2497) * initial raw bytes Signed-off-by: Kuat Yessenov <kuat@google.com> * add benchmark Signed-off-by: Kuat Yessenov <kuat@google.com> * better benchmark Signed-off-by: Kuat Yessenov <kuat@google.com> * better benchmark Signed-off-by: Kuat Yessenov <kuat@google.com>

alyssawilk added 2 commits January 31, 2018 17:17

Adding 100-Continue support to Envoy

7b436dc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'master' into 100-continue

2003df2

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Fixing lost test merge

c02d392

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the 100-continue branch from b256dd9 to c02d392 Compare January 31, 2018 22:45

Merge branch 'master' into 100-continue

66f25fe

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 reviewed Feb 1, 2018

View reviewed changes

alyssawilk added 3 commits February 1, 2018 15:17

Reviewer comments + improved coverage

e269ee4

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Removing throw statements for code nghttp2 handles already with Bad F…

cdbebeb

…rame Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Mildly snarky health check test

dcb0581

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned mattklein123 Feb 1, 2018

alyssawilk changed the title ~~Adding 100-Continue support to Envoy~~ http: adding 100-Continue support to Envoy Feb 1, 2018

mattklein123 reviewed Feb 2, 2018

View reviewed changes

alyssawilk added 3 commits February 5, 2018 11:17

Merge branch 'refs/heads/master' into 100-continue

805e4d2

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Reviewer comments

80e1108

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

ELSE if

be679bf

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 approved these changes Feb 6, 2018

View reviewed changes

alyssawilk merged commit 235d477 into envoyproxy:master Feb 6, 2018

cdelguercio mentioned this pull request Feb 6, 2018

gRPC streaming keepAlive ping never fails when proxied through Envoy #2086

Closed

mattklein123 added a commit that referenced this pull request Feb 9, 2018

Revert "http: adding 100-Continue support to Envoy (#2497)"

f83e32b

This reverts commit 235d477. Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 mentioned this pull request Feb 9, 2018

Revert 100 continue #2564

Closed

alyssawilk deleted the 100-continue branch May 8, 2018 13:24

JagadeeshSreeram mentioned this pull request Feb 23, 2020

Istio sidecar stripping Expect: 100-Continue header. istio/istio#21382

Closed

Conversation

alyssawilk commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Jan 31, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Feb 5, 2018

Uh oh!

mattklein123 commented Feb 6, 2018

Uh oh!

alyssawilk commented Feb 6, 2018

Uh oh!

mattklein123 commented Feb 6, 2018

Uh oh!

alyssawilk commented Feb 6, 2018

Uh oh!

junr03 commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Feb 8, 2018

Uh oh!

Reviewers

alyssawilk commented Jan 31, 2018 •

edited

Loading

alyssawilk Feb 1, 2018 •

edited

Loading

junr03 commented Feb 8, 2018 •

edited

Loading