http:: sending http/1.1 errors via HCM by alyssawilk · Pull Request #11714 · envoyproxy/envoy

alyssawilk · 2020-06-23T14:27:27Z

Solving the problem where early response errors look different, and don't have HCM tracking, by sending them through the hcm.
This means early responses will get all of the usual Envoy standard header additions, get accounted for in listener stats, be visible in access logs, etc.

Risk Level: high (changes to HTTP early response path)
Testing: new unit tests
Docs Changes: n/a
Release Notes: inline
Runtime guard: envoy.reloadable_features.early_errors_via_hcm
Fixes #8545
Part of #9846

asraa · 2020-06-23T14:55:43Z

source/common/http/http1/codec_impl.cc

@yanavlasov Correct me if I'm wrong, this is my read on this

sendLocalReply will encodeHeaders, which currently may throw and will be moved to RELEASE_ASSERTs and then proper clean-up with the exception removal process. Before this PR, these were outside a dispatching callstack and are uncaught.

Here, we are in a dispatching callstack. We would need to do two things to mesh well with this PR. (1) add return status for sendProtocolError/sendLocalReply and handle the way we've been handling error statuses while dispatching and (2) wrap RELEASE_ASSERTs in things like encodeHeaders that add for future clean-up (https://github.com/envoyproxy/envoy/pull/11575/files#r443142918) with a test for isNotDispatching().

Have we started work for the HTTP/1 codec? I saw Yan's PR for the HTTP/2 codec but I wasn't sure what the plans were for HTTP/1. Is he doing both? If so I assume I can just land as it'll be at least a few weeks until HTTP/2 is done and HTTP/1 changes matter

I made an H/1 PR a while ago (it was reverted, it is not merged) #11101
There is one exception still there in that PR in encodeHeaders (non-dispatching), which I will handle however Yan decides to settle the RELEASE_ASSERT todo

I think going ahead makes sense, I can implement the status return values for sendProtocolError I mentioned above if this PR is added.

I did not think that H/1 codec's encodeHeader() was throwing. If it did not throw before then there should be no problem with this change.

Oh, maybe it will only call the responseencoder's? If so, that makes this easier.

envoy/source/common/http/http1/codec_impl.cc

Line 390 in 77efb47

throw CodecClientException(":method and :path must be specified");

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

mattklein123

Nice! Just some small nits.

/wait

mattklein123 · 2020-06-24T20:00:49Z

docs/root/version_history/current.rst

 * build: run as non-root inside Docker containers. Existing behaviour can be restored by setting the environment variable `ENVOY_UID` to `0`. `ENVOY_UID` and `ENVOY_GID` can be used to set the envoy user's `uid` and `gid` respectively.
 * health check: in the health check filter the :ref:`percentage of healthy servers in upstream clusters <envoy_api_field_config.filter.http.health_check.v2.HealthCheck.cluster_min_healthy_percentages>` is now interpreted as an integer.
 * hot restart: added the option :option:`--use-dynamic-base-id` to select an unused base ID at startup and the option :option:`--base-id-path` to write the base id to a file (for reuse with later hot restarts).
+* http: changed early error path for HTTP/1.1 so that responses consistently flow through the http connection manager, and the http filter chains. This behavior may be temporarily reverted by setitng runtime feature `envoy.reloadable_features.early_errors_via_hcm` to false.


Suggested change

* http: changed early error path for HTTP/1.1 so that responses consistently flow through the http connection manager, and the http filter chains. This behavior may be temporarily reverted by setitng runtime feature `envoy.reloadable_features.early_errors_via_hcm` to false.

* http: changed early error path for HTTP/1.1 so that responses consistently flow through the http connection manager, and the http filter chains. This behavior may be temporarily reverted by setting runtime feature `envoy.reloadable_features.early_errors_via_hcm` to false.

mattklein123 · 2020-06-24T20:04:19Z

source/common/http/http1/codec_impl.cc

+      is_grpc_request =
+          Grpc::Common::isGrpcRequestHeaders(*absl::get<RequestHeaderMapPtr>(headers_or_trailers_));
+    }
+    bool is_head_request = parser_.method == HTTP_HEAD;


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

Solving the problem where early response errors look different, and don't have HCM tracking, by sending them through the hcm. This means early responses will get all of the usual Envoy standard header additions, get accounted for in listener stats, be visible in access logs, etc. Risk Level: high (changes to HTTP early response path) Testing: new unit tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.early_errors_via_hcm Fixes envoyproxy#8545 Part of envoyproxy#9846 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

This unifies HTTP/1.1 and HTTP/2 stream error on invalid messaging. Previously HTTP/1.1 defaulted permissive and HTTP/2 defaulted to strict. This defaults both to strict, resetting connections on invalid requests. This will have a major latency impact if downstream is sending a mix of valid and invalid requests over HTTP/1.1 Additional Description: This change is runtime guarded per default behavioral change rules. It can also be reverted by setting the default to permissive (for prior HTTP/1 behvior) then overriding HTTP/2 to struct (for prior HTTP/2 behavior). This works in conjunction with #11714, as the HTTP connection manager enforces the strictness, so the responses need to be sent via the HTTP connection manager to have strictness applied correctly. Risk Level: High (HCM changes) Testing: new unit tests, updated integration tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.hcm_stream_error_on_invalid_message Fixes #9846 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Solving the problem where early response errors look different, and don't have HCM tracking, by sending them through the hcm. This means early responses will get all of the usual Envoy standard header additions, get accounted for in listener stats, be visible in access logs, etc. Risk Level: high (changes to HTTP early response path) Testing: new unit tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.early_errors_via_hcm Fixes envoyproxy#8545 Part of envoyproxy#9846 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

This unifies HTTP/1.1 and HTTP/2 stream error on invalid messaging. Previously HTTP/1.1 defaulted permissive and HTTP/2 defaulted to strict. This defaults both to strict, resetting connections on invalid requests. This will have a major latency impact if downstream is sending a mix of valid and invalid requests over HTTP/1.1 Additional Description: This change is runtime guarded per default behavioral change rules. It can also be reverted by setting the default to permissive (for prior HTTP/1 behvior) then overriding HTTP/2 to struct (for prior HTTP/2 behavior). This works in conjunction with envoyproxy#11714, as the HTTP connection manager enforces the strictness, so the responses need to be sent via the HTTP connection manager to have strictness applied correctly. Risk Level: High (HCM changes) Testing: new unit tests, updated integration tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.hcm_stream_error_on_invalid_message Fixes envoyproxy#9846 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

This unifies HTTP/1.1 and HTTP/2 stream error on invalid messaging. Previously HTTP/1.1 defaulted permissive and HTTP/2 defaulted to strict. This defaults both to strict, resetting connections on invalid requests. This will have a major latency impact if downstream is sending a mix of valid and invalid requests over HTTP/1.1 Additional Description: This change is runtime guarded per default behavioral change rules. It can also be reverted by setting the default to permissive (for prior HTTP/1 behvior) then overriding HTTP/2 to struct (for prior HTTP/2 behavior). This works in conjunction with envoyproxy#11714, as the HTTP connection manager enforces the strictness, so the responses need to be sent via the HTTP connection manager to have strictness applied correctly. Risk Level: High (HCM changes) Testing: new unit tests, updated integration tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.hcm_stream_error_on_invalid_message Fixes envoyproxy#9846 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

alyssawilk assigned asraa Jun 23, 2020

asraa reviewed Jun 23, 2020

View reviewed changes

alyssawilk force-pushed the hcm branch from dc8d978 to dd9733c Compare June 23, 2020 20:52

alyssawilk changed the title ~~WIP: http/1.1 errors via HCM~~ http:: sending http/1.1 errors via HCM Jun 23, 2020

TODone.

847a406

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

alyssawilk force-pushed the hcm branch from dd9733c to 847a406 Compare June 23, 2020 20:57

alyssawilk assigned mattklein123 Jun 24, 2020

alyssawilk mentioned this pull request Jun 24, 2020

http: require stream_error_on_invalid_http_messaging to be set for every HCM #9846

Closed

yanavlasov self-assigned this Jun 24, 2020

yanavlasov previously approved these changes Jun 24, 2020

View reviewed changes

mattklein123 requested changes Jun 24, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 24, 2020

reviewer comments

4377be1

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

alyssawilk dismissed yanavlasov’s stale review via 4377be1 June 24, 2020 20:17

repokitteh-read-only bot removed the waiting label Jun 24, 2020

mattklein123 approved these changes Jun 24, 2020

View reviewed changes

alyssawilk mentioned this pull request Jun 25, 2020

http: stream error on invalid messaging #11748

Merged

alyssawilk merged commit f46325f into envoyproxy:master Jun 25, 2020

rulex123 mentioned this pull request Aug 12, 2020

http: stream error on invalid HTTP messaging for HTTP/1 #11884

Merged

alyssawilk deleted the hcm branch September 30, 2020 15:46

mattklein123 mentioned this pull request Jan 11, 2021

envoy.reloadable_features.early_errors_via_hcm deprecation #14641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http:: sending http/1.1 errors via HCM#11714

http:: sending http/1.1 errors via HCM#11714
alyssawilk merged 2 commits intoenvoyproxy:masterfrom
alyssawilk:hcm

alyssawilk commented Jun 23, 2020 •

edited

Loading

Uh oh!

asraa Jun 23, 2020

Uh oh!

alyssawilk Jun 23, 2020 •

edited

Loading

Uh oh!

asraa Jun 23, 2020

Uh oh!

asraa Jun 23, 2020

Uh oh!

yanavlasov Jun 24, 2020

Uh oh!

asraa Jun 24, 2020

Uh oh!

mattklein123 left a comment

Uh oh!

mattklein123 Jun 24, 2020

Uh oh!

mattklein123 Jun 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	* http: changed early error path for HTTP/1.1 so that responses consistently flow through the http connection manager, and the http filter chains. This behavior may be temporarily reverted by setitng runtime feature `envoy.reloadable_features.early_errors_via_hcm` to false.
	* http: changed early error path for HTTP/1.1 so that responses consistently flow through the http connection manager, and the http filter chains. This behavior may be temporarily reverted by setting runtime feature `envoy.reloadable_features.early_errors_via_hcm` to false.

Conversation

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

Uh oh!

Uh oh!

asraa Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

alyssawilk Jun 23, 2020 • 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!

asraa Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

asraa Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

yanavlasov Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

asraa Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alyssawilk commented Jun 23, 2020 •

edited

Loading

alyssawilk Jun 23, 2020 •

edited

Loading