[http1 codec] Allow requests with Transfer-Encoding and Content-Length headers set. #12349

veshij · 2020-07-29T06:05:32Z

Commit Message: [http1 codec] Allow HTTP/1 requests/responses with Transfer-Encoding and Content-Length headers set.
Additional Description: opt-in for serving requests/responses with Content-Length and Transfer-Encoding: chunked. Per RFC remove Content-Length header before forwarding it to upstream.
Update http-parser version to include fix for nodejs/http-parser#517.
Risk Level: Medium
Testing: unit tests, update integration test
Docs Changes: updated docs/root/version_history/current.rst
Release Notes: http: added allow_chunked_length configuration option for HTTP/1 codec to allow processing requests with both Content-Length and Transfer-Encoding: chunked. If enabled - per RFC Content-Length is removed before sending to upstream.

Fixes #11398

Signed-off-by: Oleg Guba <[email protected]>

alyssawilk

Cool, thanks for tackling this - I'm sure someone else down the line is going to have traffic which hits this unusual corner case.

source/common/http/http1/codec_impl.cc

source/common/http/http1/codec_impl.h

bazel/repository_locations.bzl

mattklein123

Thanks for driving this forward. A few comments.

/wait

bazel/repository_locations.bzl

include/envoy/http/codec.h

source/common/http/http1/codec_impl.cc

antoniovicente · 2020-07-29T18:25:14Z

/assign @antoniovicente

Signed-off-by: Oleg Guba <[email protected]>

veshij · 2020-07-29T18:49:41Z

/wait

Signed-off-by: Oleg Guba <[email protected]>

repokitteh-read-only · 2020-07-29T23:50:00Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12349 was synchronize by veshij.

see: more, trace.

Signed-off-by: Oleg Guba <[email protected]>

veshij · 2020-07-30T00:08:30Z

/wait

Signed-off-by: Oleg Guba <[email protected]>

veshij · 2020-07-30T00:19:58Z

/wait

Signed-off-by: Oleg Guba <[email protected]>

mattklein123

Thanks, this LGTM. Will give @PiotrSikora @htuch some more time to take a look again if they want. @antoniovicente is out till Monday so all things being equal I would prefer to wait for him to get back to take a final look. cc @asraa also for the H1 codec error handling changes.

alyssawilk

Looks good from my end - just two minor nits.

source/common/http/http1/codec_impl_legacy.cc

source/common/http/http1/codec_impl.cc

antoniovicente

Looked at changes since my last set of comments. Thanks for adding tests for the response cases.

antoniovicente · 2020-08-18T19:24:35Z

source/common/http/http1/codec_impl.cc

@@ -1086,7 +1086,8 @@ bool ClientConnectionImpl::cannotHaveBody() {
    ASSERT(!pending_response_done_);
    return true;
  } else if (parser_.status_code == 204 || parser_.status_code == 304 ||
-             (parser_.status_code >= 200 && parser_.content_length == 0)) {
+             (parser_.status_code >= 200 && parser_.content_length == 0 &&
+              !(parser_.flags & F_CHUNKED))) {


Out of curiosity: what coverage do we have for 1xx responses that include content-length or transfer-encoding: chunked headers?

…e code to use it Signed-off-by: Oleg Guba <[email protected]>

Signed-off-by: Oleg Guba <[email protected]>

veshij · 2020-08-26T01:39:29Z

/wait

Signed-off-by: Oleg Guba <[email protected]>

mattklein123

Thanks LGTM. @antoniovicente @alyssawilk any further comments?

…h headers set. (envoyproxy#12349) opt-in for serving requests/responses with Content-Length and Transfer-Encoding: chunked. Per RFC remove Content-Length header before forwarding it to upstream. Signed-off-by: Oleg Guba <[email protected]> Signed-off-by: Clara Andrew-Wani <[email protected]>

ceastman-ibm · 2020-09-18T14:59:09Z

When would this PR get into envoyproxy/envoy-wasm so that istio can pick it up? We are actually hitting this edge case. @alyssawilk

alyssawilk · 2020-09-21T13:19:58Z

Up to folks on that side.
Did @jplevyak 's last merge sort this out for you? Looks like it landed the same day you pinged here.

ceastman-ibm · 2020-09-21T13:36:37Z

@alyssawilk we won't be able to test this until this version of envoyproxy gets into istio and then into our IBM managed istio offering.

veshij added 2 commits July 26, 2020 21:40

[http1 codec] remove Content-Length is Transfer-Encoding is chunked

886c5f0

Signed-off-by: Oleg Guba <[email protected]>

enable http parser option by default and do all validation in Envoy

6572594

Signed-off-by: Oleg Guba <[email protected]>

alyssawilk reviewed Jul 29, 2020

View reviewed changes

source/common/http/http1/codec_impl.cc Show resolved Hide resolved

source/common/http/http1/codec_impl.h Outdated Show resolved Hide resolved

bazel/repository_locations.bzl Show resolved Hide resolved

mattklein123 requested changes Jul 29, 2020

View reviewed changes

bazel/repository_locations.bzl Show resolved Hide resolved

include/envoy/http/codec.h Outdated Show resolved Hide resolved

source/common/http/http1/codec_impl.cc Show resolved Hide resolved

source/common/http/http1/codec_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jul 29, 2020

mattklein123 assigned mattklein123 and alyssawilk Jul 29, 2020

repokitteh-read-only bot assigned antoniovicente Jul 29, 2020

address comments: typo/spelling/comments

8cc229c

Signed-off-by: Oleg Guba <[email protected]>

repokitteh-read-only bot removed the waiting label Jul 29, 2020

repokitteh-read-only bot added the waiting label Jul 29, 2020

add allow_chunked_length to Http1ProtocolOoptions

542e6e3

Signed-off-by: Oleg Guba <[email protected]>

repokitteh-read-only bot added api and removed waiting labels Jul 29, 2020

add allow_chunked_length to Http1ProtocolOoptions

b59bcea

Signed-off-by: Oleg Guba <[email protected]>

repokitteh-read-only bot added the waiting label Jul 30, 2020

Merge branch 'chunked' of github.com:veshij/envoy into chunked

ac88d96

Signed-off-by: Oleg Guba <[email protected]>

repokitteh-read-only bot removed the waiting label Jul 30, 2020

repokitteh-read-only bot added the waiting label Jul 30, 2020

add unit tests

7c91d03

Signed-off-by: Oleg Guba <[email protected]>

repokitteh-read-only bot removed the waiting label Jul 30, 2020

typo fix

0cd4cb7

Signed-off-by: Oleg Guba <[email protected]>

mattklein123 added the waiting label Jul 30, 2020

veshij added 2 commits July 30, 2020 13:11

release notes update

4a88b62

Signed-off-by: Oleg Guba <[email protected]>

typo fix

4119c78

Signed-off-by: Oleg Guba <[email protected]>

mattklein123 previously approved these changes Aug 13, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 13, 2020

alyssawilk reviewed Aug 17, 2020

View reviewed changes

source/common/http/http1/codec_impl_legacy.cc Outdated Show resolved Hide resolved

source/common/http/http1/codec_impl.cc Show resolved Hide resolved

mattklein123 added the waiting label Aug 17, 2020

antoniovicente reviewed Aug 18, 2020

View reviewed changes

veshij added 3 commits August 24, 2020 12:17

define Http1ResponseCodeDetails::get().ChunkedContentLength and updat…

536f724

…e code to use it Signed-off-by: Oleg Guba <[email protected]>

Merge remote-tracking branch 'upstream/master' into chunked

ee525a0

Signed-off-by: Oleg Guba <[email protected]>

simple integration test

ac1d0e1

Signed-off-by: Oleg Guba <[email protected]>

veshij dismissed mattklein123’s stale review via ac1d0e1 August 26, 2020 00:34

repokitteh-read-only bot added api and removed waiting labels Aug 26, 2020

typo fix

2549c62

Signed-off-by: Oleg Guba <[email protected]>

repokitteh-read-only bot added the waiting label Aug 26, 2020

veshij added 2 commits August 25, 2020 20:05

server/client integration tests

62ef94a

Signed-off-by: Oleg Guba <[email protected]>

format fix

85864ec

Signed-off-by: Oleg Guba <[email protected]>

repokitteh-read-only bot removed the waiting label Aug 26, 2020

veshij added 3 commits August 26, 2020 11:15

Kick CI

bd9d235

Signed-off-by: Oleg Guba <[email protected]>

Kick CI

c959d58

Signed-off-by: Oleg Guba <[email protected]>

Merge remote-tracking branch 'upstream/master' into chunked

8cf9af0

Signed-off-by: Oleg Guba <[email protected]>

mattklein123 approved these changes Aug 28, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 28, 2020

alyssawilk approved these changes Aug 31, 2020

View reviewed changes

mattklein123 merged commit 954c93c into envoyproxy:master Aug 31, 2020

ceastman-ibm mentioned this pull request Sep 24, 2020

when will https://github.com/envoyproxy/envoy/pull/12349 be merged into envoy-wasm? envoyproxy/envoy-wasm#645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[http1 codec] Allow requests with Transfer-Encoding and Content-Length headers set. #12349

[http1 codec] Allow requests with Transfer-Encoding and Content-Length headers set. #12349

veshij commented Jul 29, 2020 •

edited

Loading

alyssawilk left a comment

mattklein123 left a comment

antoniovicente commented Jul 29, 2020

veshij commented Jul 29, 2020

repokitteh-read-only bot commented Jul 29, 2020

veshij commented Jul 30, 2020

veshij commented Jul 30, 2020

mattklein123 left a comment

alyssawilk left a comment

antoniovicente left a comment

antoniovicente Aug 18, 2020

veshij commented Aug 26, 2020

mattklein123 left a comment

ceastman-ibm commented Sep 18, 2020

alyssawilk commented Sep 21, 2020

ceastman-ibm commented Sep 21, 2020

[http1 codec] Allow requests with Transfer-Encoding and Content-Length headers set. #12349

[http1 codec] Allow requests with Transfer-Encoding and Content-Length headers set. #12349

Conversation

veshij commented Jul 29, 2020 • edited Loading

alyssawilk left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

antoniovicente commented Jul 29, 2020

veshij commented Jul 29, 2020

repokitteh-read-only bot commented Jul 29, 2020

veshij commented Jul 30, 2020

veshij commented Jul 30, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

antoniovicente left a comment

Choose a reason for hiding this comment

antoniovicente Aug 18, 2020

Choose a reason for hiding this comment

veshij commented Aug 26, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

ceastman-ibm commented Sep 18, 2020

alyssawilk commented Sep 21, 2020

ceastman-ibm commented Sep 21, 2020

veshij commented Jul 29, 2020 •

edited

Loading