http: switching from 100 to 1xx by alyssawilk · Pull Request #18904 · envoyproxy/envoy

alyssawilk · 2021-11-04T19:41:46Z

Changing envoy APIs to reflect upcoming 102 support. This should be a functional no-op other than making every filter change their API calls :-/

Risk Level: Medium
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Part of #18844

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk · 2021-11-04T19:43:04Z

Two notes. First I hate HeaderUtility::isSpecial1xx, but is1xxButNot101 seems awful too. Better ideas appreciated.
Second, I don't like making everyone implement encode1xx when they're not going to use it. If I'd had my way with encode100Continue being Continue in the base class, not everyone would have to change their APIs. What do we think of implementing 1xx in the base class so at least folks don't have to implement it this time around?

Signed-off-by: Alyssa Wilk <[email protected]>

snowp

Thanks, a few questions on how this all works

snowp · 2021-11-08T16:41:48Z

source/common/http/http1/codec_impl.cc

+    if (HeaderUtility::isSpecial1xx(*headers)) {
+      pending_response_.value().decoder_->decode1xxHeaders(std::move(headers));


So any other 1xx header would currently just be handled as regular headers?

yeah this is intentional for 101s but other codes get dropped in UpstreamRequest::decodeHeaders
honestly I think it'd be more clean to handle in the codecs, but then we'd be doing it in 3x places.

snowp · 2021-11-08T16:42:17Z

source/common/quic/envoy_quic_client_stream.cc

  }

-  if (status == enumToInt(Http::Code::Continue) && !decoded_100_continue_) {
+  bool is_special_1xx = Http::HeaderUtility::isSpecial1xx(*headers);


snowp · 2021-11-08T16:44:48Z

source/common/quic/envoy_quic_client_stream.cc

+  if (is_special_1xx && !decoded_1xx_) {
    // This is 100 Continue, only decode it once to support Expect:100-Continue header.
-    decoded_100_continue_ = true;
-    response_decoder_->decode100ContinueHeaders(std::move(headers));
-  } else if (status != enumToInt(Http::Code::Continue)) {
+    decoded_1xx_ = true;
+    response_decoder_->decode1xxHeaders(std::move(headers));
+  } else if (!is_special_1xx) {


For my understanding: we allow multiple 100 on the decode path, but we just decode the first one and drop subsequent 100 headers? Is that something that actually happens?

yeah, if we haven't decoded_1xx_ we pass it up, and otherwise we only encode if it's not a special 1xx.

snowp · 2021-11-08T16:46:40Z

source/common/router/upstream_request.cc


-  // We drop 1xx other than 101 on the floor; 101 upgrade headers need to be passed to the client as
-  // part of the final response. 100-continue headers are handled in onUpstream100ContinueHeaders.
+  // We unsupported 1xx on the floor here. 101 upgrade headers need to be passed to the client as


missing drop

snowp · 2021-11-08T16:48:47Z

envoy/http/filter.h

   *
   * This is not folded into encodeHeaders because most Envoy users and filters will not be proxying
-   * 100-continue and with it split out, can ignore the complexity of multiple encodeHeaders calls.
+   * 10s and with it split out, can ignore the complexity of multiple encodeHeaders calls.


snowp · 2021-11-08T16:49:37Z

envoy/http/filter.h


  /**
-   * Called with 100-Continue headers to be encoded.
+   * Called with 1xx headers to be encoded.


Based on the impl it seems we only pass a specific set of "special" 1xx headers here, should this be included in the comments?

yeah I was trying to be lazy and not have to update all the comments in the next PR. I'll stop being lazy :-P

Signed-off-by: Alyssa Wilk <[email protected]>

snowp

/wait

snowp · 2021-11-09T16:28:17Z

envoy/http/codec.h

 public:
  /**
   * Encode supported 1xx headers.
+   * Currently only 1000-Continue headers are supported.


Signed-off-by: Alyssa Wilk <[email protected]>

snowp

Thanks!

* main: (71 commits) bazel: fix macOS build (envoyproxy#18920) http: switching from 100 to 1xx (envoyproxy#18904) grpc: implement BufferedAsyncClient for bidirectional gRPC stream (envoyproxy#18129) bazel: add repository arg to benchmark_test (envoyproxy#18795) rocketmq_proxy: Improvement for map find (envoyproxy#18909) tls: unit test: spiffe signed by intermediate cert (envoyproxy#18914) Test for FilterConfigPerRoute dtor called on worker thread. (envoyproxy#18927) deps: Bump `com_google_protobuf` -> 3.19.1 (envoyproxy#18930) deps: Bump `com_googlesource_code_re2` -> 2021-11-01 (envoyproxy#18933) cvescan: Move cvescan data to yaml (envoyproxy#18947) remove unnecessary file level not unimplemented hide annotation (envoyproxy#18924) test: moving echo test (envoyproxy#18938) test: fixing a test flake (envoyproxy#18899) deps: Revert pyparsing bump (envoyproxy#18946) deps: Bump `build_bazel_rules_apple` -> 0.32.0 (envoyproxy#18932) deps: Bump `com_github_bazelbuild_buildtools` -> 4.2.3 (envoyproxy#18931) build(deps): bump pycparser from 2.20 to 2.21 in /tools/dependency (envoyproxy#18936) quic: supporting connections with zero initial available streams (envoyproxy#18775) test: moving proxy proto (envoyproxy#18939) build(deps): bump pyparsing from 3.0.4 to 3.0.5 in /tools/dependency (envoyproxy#18937) ... Signed-off-by: Michael Puncel <[email protected]>

- updates to `.bazelrc` - rename of decode100ContinueHeaders to decode1xxHeaders according to envoyproxy/envoy#18904 Signed-off-by: tomjzzhang <[email protected]>

http: switching from 100 to 1xx

688c51d

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk assigned snowp Nov 4, 2021

alyssawilk requested review from dio, htuch, lizan, mathetake, mattklein123, rojkov, snowp, yanavlasov and zuercher as code owners November 4, 2021 19:41

tidy

8dea0dd

Signed-off-by: Alyssa Wilk <[email protected]>

snowp suggested changes Nov 8, 2021

View reviewed changes

comments

5c5b6b1

Signed-off-by: Alyssa Wilk <[email protected]>

snowp suggested changes Nov 9, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 9, 2021

yanavlasov self-assigned this Nov 9, 2021

off by zero

79d18b5

Signed-off-by: Alyssa Wilk <[email protected]>

repokitteh-read-only bot removed the waiting label Nov 9, 2021

snowp approved these changes Nov 10, 2021

View reviewed changes

alyssawilk merged commit d9c3927 into envoyproxy:main Nov 10, 2021

tomjzzhang mentioned this pull request Nov 15, 2021

Update envoy to a6fc192 envoyproxy/nighthawk#759

Merged

alyssawilk deleted the 1xx branch August 4, 2022 01:13

		if (HeaderUtility::isSpecial1xx(*headers)) {
		pending_response_.value().decoder_->decode1xxHeaders(std::move(headers));

Comments

Conversation

alyssawilk commented Nov 4, 2021

Uh oh!

alyssawilk commented Nov 4, 2021

Uh oh!

snowp 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!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants