http: stream error on invalid messaging by alyssawilk · Pull Request #11748 · envoyproxy/envoy

alyssawilk · 2020-06-25T14:13:30Z

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

repokitteh-read-only · 2020-06-25T14:13:37Z

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: #11748 was opened by alyssawilk.

see: more, trace.

alyssawilk · 2020-06-25T14:16:07Z

This fully cleaned up and tested, and marked as draft because I'm not sure when we're cutting the release. If we're cutting early next week I would prefer to land only the HTTP/2 field changes (to start the Envoy deprecation / warning clock) and will defer merging this until after the release. If we're cutting in 2 weeks I don't think it makes sense to defer.

Either way I'd like to discuss the API now, so I can land whatever subsection of it makes more sense.

cc @antoniovicente

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

alyssawilk · 2020-06-25T16:35:47Z

coverage failure appears unrelated

htuch

API LGTM, just some minor comments.

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

htuch · 2020-06-25T22:20:33Z

api/envoy/config/core/v3/protocol.proto

+  // <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.upgrade_configs>`
+  // iff present.
+  //
+  // This is deprecated in favor of stream_error_on_invalid_http_message.


Can you :ref: link to the field below? Given how similar they are name wise, it would be good to have zero ambiguity on which one is being referred to.

htuch · 2020-06-25T22:21:23Z

api/envoy/config/core/v3/protocol.proto

+  //
  // See `RFC7540, sec. 8.1 <https://tools.ietf.org/html/rfc7540#section-8.1>`_ for details.
-  bool stream_error_on_invalid_http_messaging = 12;
+  google.protobuf.BoolValue stream_error_on_invalid_http_message = 14;


How about stream_error_on_invalid_http_messaging_hcm_override? That's very very long, but I worry about the similarity in names between new and deprecated.

Yeah, I agree on name similarity but I'd mildly prefer a clean name in the long run. @mattklein123 WDYT?

Meet half way on stream_error_on_invalid_http_messaging_override? :)

Haha, deal, modulo messaging -> message for consistency with the long term HCM value.

Alternately would you also be OK with override_stream_error_on_invalid_http_message? I think that reads a bit better but it's arguably easier to overlook the difference so I'm fine if you prefer stream_error_on_invalid_http_message_override

Yep, either is fine.

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

mattklein123 · 2020-06-29T20:24:57Z

At a high level this LGTM. What is your plan with this change? Do you want to land after the next release or before? Just wondering what will take it out of draft.

/wait-any

alyssawilk · 2020-06-29T20:31:15Z

I basically didn't want it landing too close to #11714 or too close to cutting the release.

Given master is frozen and most folks are out Thursday and Friday, I assume we're not cutting the release until next week and I'm cool landing it this week. I'd say if it doesn't land by Wednesday I'd split out the HTTP/2 bits and land them before release, and land the HCM and behavioral changes after.

htuch · 2020-06-29T23:38:31Z

/lgtm api

mattklein123 · 2020-06-30T16:37:47Z

Looks like legit CI issue.

/wait

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

alyssawilk · 2020-07-01T13:20:22Z

test/integration/stats_integration_test.cc

  // 2020/05/13  10531    44425       44600   Refactor resource manager
  // 2020/05/20  11223    44491       44600   Add primary clusters tracking to cluster manager.
  // 2020/06/10  11561    44491       44811   Make upstreams pluggable
-  // 2020/04/23  10661    44425       46000   per-listener connection limits


latest CI failure was this test again (second time this PR has failed CI only due to stats numbers)
I'm really not sure what to do about the per-listener limit, which had a size and date from the setec upstream PR which were both out of kilter with actual dates and sizes so I just removed it.

I still question the value of this test being worth the toil, especially if it's going to be confusing and inaccurate for things like backports :-/

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

alyssawilk · 2020-07-15T13:56:54Z

This should be ready for another pass.

snowp · 2020-07-15T16:30:09Z

source/common/http/conn_manager_impl.cc

    ENVOY_STREAM_LOG(debug, "drain closing connection", *this);
  }

+  // The BadRequest error code indicates there has been a messaging error.


Just so I understand: does this code path not get hit if the upstream returns BadRequest?

It does. I can change it so only Envoy invalid messaging triggers closes if we prefer that.

I've definitely seen people use BadRequest to indicate a malformed message body (ie invalid JSON key or whatever) out in the wild, so it might make sense to only close the connection when we know that it actually was related to invalid HTTP messages?

I'm not sure what the right thing to do here tbh, not clear what the scope of "HTTP messaging" is

snowp · 2020-07-15T16:30:27Z

source/common/http/utility.cc

+                             bool hcm_stream_error_set,
+                             const Protobuf::BoolValue& hcm_stream_error) {
+  auto ret = initializeAndValidateOptions(options);
+  if (Runtime::runtimeFeatureEnabled(


Got it, that makes sense

snowp · 2020-07-15T16:34:27Z

test/mocks/runtime/mocks.h

  MOCK_METHOD(Stats::Scope&, getRootScope, ());

  testing::NiceMock<MockSnapshot> snapshot_;
+  SnapshotConstSharedPtr snapshot_ptr_;


not used? was this supposed to be used with the default ON_CALL in mocks.cc?

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

htuch · 2020-07-15T17:44:31Z

/lgtm api

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

mattklein123

LGTM. Can you add tests that cover the kill switch? (Unless I missed them.)

/wait

mattklein123 · 2020-07-15T23:49:52Z

docs/root/configuration/best_practices/level_two.rst


 **In summary, if you run level two Envoy version 1.11.1 or greater which terminates 
-HTTP/2, we strongly advise you to change the HTTP/2 configuration of your level 
+HTTP/2, we strongly advise you to change the HCM configuration of your level


nit I would spell out HCM since I'm not sure users will know what this means (same elsewhere if applicable)

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

alyssawilk · 2020-07-16T17:37:04Z

Given the delta between this and the last commit is a comment-only change and macos build was green, I'd be inclined to merge over the macos flake especially as CI has been wedged 2 hours. WDYT?

mattklein123 · 2020-07-16T17:42:09Z

@alyssawilk what about a test for the kill switch? Did I miss that?

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

alyssawilk · 2020-07-16T18:03:20Z

Oh sorry missed that comment - added.

…

On Thu, Jul 16, 2020 at 1:42 PM Matt Klein ***@***.***> wrote: @alyssawilk <https://github.com/alyssawilk> what about a test for the kill switch? Did I miss that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11748 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AELALPPH5WNNUESG3UMMS7LR343YBANCNFSM4OINHFJQ> .

mattklein123

Thanks!

addressed

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 mattklein123 and htuch Jun 25, 2020

repokitteh-read-only bot added the api label Jun 25, 2020

http: stream error on invalid messaging

f779cb0

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

alyssawilk force-pushed the stream_error branch from 453cc21 to f779cb0 Compare June 25, 2020 15:17

htuch reviewed Jun 25, 2020

View reviewed changes

alyssawilk added 5 commits June 29, 2020 09:42

Merge branch 'master' into stream_error

56844e1

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

ref

8a9bb42

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

Stats integration test strikes again

310d4cc

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

Merge branch 'master' into stream_error

4bd61b5

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

override

55cc5ea

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

repokitteh-read-only bot added the waiting:any label Jun 29, 2020

repokitteh-read-only bot removed the waiting:any label Jun 29, 2020

alyssawilk marked this pull request as ready for review June 29, 2020 20:31

alyssawilk requested a review from mattklein123 as a code owner June 29, 2020 20:31

repokitteh-read-only bot removed the api label Jun 29, 2020

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

alyssawilk added 2 commits June 30, 2020 15:26

Merge branch 'master' into stream_error

2077a28

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

overenthusiastic override

ef8b03c

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

repokitteh-read-only bot added api and removed waiting labels Jun 30, 2020

stats integration test AGAIN

01ed5dd

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

alyssawilk commented Jul 1, 2020

View reviewed changes

alyssawilk added 5 commits July 13, 2020 16:59

Merge branch 'master' into stream_error

a38a4e6

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

reviewer comments, and stats integration test x5

aadf1aa

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

missed comment

fa76aab

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

Merge branch 'master' into stream_error

e1463a9

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

Merge branch 'master' into stream_error

7af13bd

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

snowp previously requested changes Jul 15, 2020

View reviewed changes

removing unused

34ba89d

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

repokitteh-read-only bot removed the api label Jul 15, 2020

alyssawilk added 2 commits July 15, 2020 14:07

only on local replies

67ba608

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

format

cf743fa

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

mattklein123 requested changes Jul 15, 2020

View reviewed changes

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

httpconnectionmanager

49ea2c9

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

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

mattklein123 added the waiting label Jul 16, 2020

test runtime

17ab3e8

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

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

mattklein123 approved these changes Jul 16, 2020

View reviewed changes

alyssawilk merged commit 88dcb29 into envoyproxy:master Jul 16, 2020

alyssawilk deleted the stream_error branch October 26, 2020 21:16

yanavlasov mentioned this pull request Jul 26, 2023

Deprecate Http2ProtocolOptions.stream_error_on_invalid_http_messaging #9285

Closed

Conversation

alyssawilk commented Jun 25, 2020

Uh oh!

repokitteh-read-only bot commented Jun 25, 2020

Uh oh!

alyssawilk commented Jun 25, 2020

Uh oh!

alyssawilk commented Jun 25, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

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!

mattklein123 commented Jun 29, 2020

Uh oh!

alyssawilk commented Jun 29, 2020

Uh oh!

htuch commented Jun 29, 2020

Uh oh!

mattklein123 commented Jun 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jul 15, 2020

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 Jul 15, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jul 16, 2020

Uh oh!

mattklein123 commented Jul 16, 2020

Uh oh!

alyssawilk commented Jul 16, 2020 via email

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants