http1: do not add transfer-encoding: chunked to 1xx/204 responses by zuercher · Pull Request #11458 · envoyproxy/envoy

zuercher · 2020-06-05T00:09:56Z

Commit Message:
Follow-on to #10811. The previous change would strip the
transfer-encoding: chunked header if returned by an upstream
during upgrade, but the http/1.1 codec adds the header back.
RFC 7230, Section 3.3.1 requires that no such header appear
in a 1xx or 204 response.

Additional Description:
Unfortunately in the first attempt I failed to realize that transfer-encoding
is re-added in the HTTP/1 codec. Adds checks and a flag in that codec to
disable adding transfer-encoding: chunked to 1xx/204 responses.

Risk Level: medium, http behavior made stricter
Testing: updated tests
Docs Changes: n/a
Release Notes: added

Signed-off-by: Stephan Zuercher zuercher@gmail.com

Follow-on to envoyproxy#10811. The previous change would strip the transfer-encoding: chunked header if returned by an upstream during upgrade, but the http/1.1 codec adds the header back. RFC 7230, Section 3.3.1 requires that no such header appear in a 1xx response. Additional Description: Unfortunately in the first attempt I failed to realize that transfer-encoding is handled in the HTTP/1 codec. Adds checks and a flag in that codec to disable adding transfer-encoding: chunked to 1xx responses. Risk Level: low Testing: updated tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

zuercher · 2020-06-05T16:30:31Z

/azp run envoy-presubmit

azure-pipelines · 2020-06-05T16:30:39Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

alyssawilk

Ugh, sorry I didn't catch this either and thanks for the follow-up fix!

Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

alyssawilk

LGTM but I'd bump the risk level to medium now that this is more substantial.

zuercher · 2020-06-09T20:42:01Z

/azp run envoy-presubmit

azure-pipelines · 2020-06-09T20:42:09Z

Azure Pipelines successfully started running 1 pipeline(s).

zuercher · 2020-06-09T21:35:32Z

@mattklein123 do you want to look at this as well?

Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

mattklein123 · 2020-06-09T23:22:07Z

Sure will take a quick look tomorrow.

mattklein123

LGTM with one comment/question.

mattklein123 · 2020-06-10T03:45:56Z

+  // Enabling handling of https://tools.ietf.org/html/rfc7230#section-3.3.1 and
+  // https://tools.ietf.org/html/rfc7230#section-3.3.2. Also resets these flags
+  // if a 100 Continue is followed by another status.
+  setIs1xx(numeric_status < 200);


I vaguely recall asking @alyssawilk this when the above code was written, but can't we just pass some state directly into encodeHeadersBase such as bool never_chunk_encode and avoid the indirection with member variables? It seems simpler to understand but I don't feel strongly about it. The base function already has request/response specific logic also so it seems not even terrible to just pass the is_1xx_response and is_204_response directly also, but a generic bool seems better.

I'll take a look at this, but in another PR, if that's ok with everyone. I think the right thing to do will be to split up this chunk of logic in a way that lets us break this up -- parts of it are really only applicable to responses.

Sure sounds good.

Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

zuercher · 2020-06-10T16:36:24Z

Merged master to resolve a conflict.

Commit Message: Some flags in the StreamEncoderImpl are only necessary for the duration of calls to encodeHeadersBase. Remove the fields and pass response status as an optional value rather than setting fields ahead of the encodeHeadersBase call. Additional Description: This refactoring was promised during review of envoyproxy#11458. This is a little more complicated than passing a never-chunk-encode bool since the behavior also covers content-length and may be partially disabled via the runtime flag added in 11458. Assigned risk level medium because its the h1 encoder. Risk Level: medium Testing: existing tests pass Doc Changes: n/a Release Note: n/a Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

…voyproxy#11458) Follow-on to envoyproxy#10811. The previous change would strip the transfer-encoding: chunked header if returned by an upstream during upgrade, but the http/1.1 codec adds the header back. RFC 7230, Section 3.3.1 requires that no such header appear in a 1xx or 204 response. Use runtime feature "envoy.reloadable_features.strict_1xx_and_204_response_headers" to disable. Risk Level: medium Testing: updated tests Docs Changes: n/a Release Notes: updated Signed-off-by: Stephan Zuercher <zuercher@gmail.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

…voyproxy#11458) Follow-on to envoyproxy#10811. The previous change would strip the transfer-encoding: chunked header if returned by an upstream during upgrade, but the http/1.1 codec adds the header back. RFC 7230, Section 3.3.1 requires that no such header appear in a 1xx or 204 response. Use runtime feature "envoy.reloadable_features.strict_1xx_and_204_response_headers" to disable. Risk Level: medium Testing: updated tests Docs Changes: n/a Release Notes: updated Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

…nses (envoyproxy#11458)" This reverts commit 353c216. Signed-off-by: davidraskin <draskin@google.com>

…04 responses (envoyproxy#11458)"" This reverts commit c3e9321fa65ce3b2ff52c9040a01cc8e04a796b9. Signed-off-by: davidraskin <draskin@google.com>

…voyproxy#11458) Follow-on to envoyproxy#10811. The previous change would strip the transfer-encoding: chunked header if returned by an upstream during upgrade, but the http/1.1 codec adds the header back. RFC 7230, Section 3.3.1 requires that no such header appear in a 1xx or 204 response. Use runtime feature "envoy.reloadable_features.strict_1xx_and_204_response_headers" to disable. Risk Level: medium Testing: updated tests Docs Changes: n/a Release Notes: updated Signed-off-by: Stephan Zuercher <zuercher@gmail.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

mattklein123 assigned alyssawilk Jun 5, 2020

Merge branch 'upstream-master' into zuercher_no_transfer_encoding_on_1xx

4b3481b

Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

alyssawilk reviewed Jun 8, 2020

View reviewed changes

Comment thread source/common/http/http1/codec_impl.cc Outdated

Comment thread test/common/http/http1/codec_impl_test.cc Outdated

Comment thread test/integration/websocket_integration_test.cc Outdated

require end_stream on 204; reject t-e and c-l != 0 on 1xx/204

3577be8

Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

alyssawilk reviewed Jun 9, 2020

View reviewed changes

Comment thread source/common/http/http1/codec_impl.cc Outdated

feature flag, release notes

55503e8

Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

alyssawilk previously approved these changes Jun 9, 2020

View reviewed changes

Merge branch 'upstream-master' into zuercher_no_transfer_encoding_on_1xx

7cfb18d

Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

mattklein123 self-assigned this Jun 9, 2020

mattklein123 previously approved these changes Jun 10, 2020

View reviewed changes

Merge branch 'upstream-master' into zuercher_no_transfer_encoding_on_1xx

f117bf7

Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

zuercher dismissed stale reviews from mattklein123 and alyssawilk via f117bf7 June 10, 2020 16:35

alyssawilk approved these changes Jun 10, 2020

View reviewed changes

zuercher merged commit 353c216 into envoyproxy:master Jun 10, 2020

zuercher mentioned this pull request Jun 23, 2020

http1: pass some encoder flags as params #11704

Merged

adisuissa mentioned this pull request Jul 8, 2020

http1 204 response with body fails codec assertion #11922

Merged

davidraskin added a commit to davidraskin/envoy that referenced this pull request Jul 21, 2020

Revert "http1: do not add transfer-encoding: chunked to 1xx/204 respo…

f318572

…nses (envoyproxy#11458)" This reverts commit 353c216. Signed-off-by: davidraskin <draskin@google.com>

Monkeyanator mentioned this pull request Oct 30, 2020

Disable strict_1xx_and_204_reponse_headers in envoy bootstrap config istio/istio#28450

Merged

lambdai mentioned this pull request Nov 2, 2020

[feature request] Fault-in-standard-out option on TE header at 1xx and 204 #13868

Closed

This was referenced Jan 11, 2021

envoy.reloadable_features.stop_faking_paths deprecation #14650

Closed

envoy.reloadable_features.strict_1xx_and_204_response_headers deprecation #14651

Closed

zuercher deleted the zuercher_no_transfer_encoding_on_1xx branch March 3, 2021 21:23

Conversation

zuercher commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuercher commented Jun 5, 2020

Uh oh!

azure-pipelines Bot commented Jun 5, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher commented Jun 9, 2020

Uh oh!

azure-pipelines Bot commented Jun 9, 2020

Uh oh!

zuercher commented Jun 9, 2020

Uh oh!

mattklein123 commented Jun 9, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

zuercher Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

zuercher commented Jun 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zuercher commented Jun 5, 2020 •

edited

Loading