HTTP: Associate Response crashes with the corresponding Request. by KBaichoo · Pull Request #15029 · envoyproxy/envoy

KBaichoo · 2021-02-11T21:17:01Z

Commit Message: Implement dumping corresponding request for upstream response crashes.
Additional Description: We now track the stream that's being processed for H2 at the first callback that nghttp2 invokes and on crash for client stream we dump state of the responseDecoder which is an UpstreamRequest to dump the originating Request. Handles the case for H2 and H1.
Risk Level: Medium
Testing: Unit tests
Docs Changes: N/A
Release Notes: Included
Platform Specific Features: N/A
Related Issue #14249

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2021-02-11T21:17:18Z

/assign @antoniovicente

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2021-02-12T17:14:47Z

Tests are failing due to another PR #14924 which I'll need to merge in.

antoniovicente

Looks pretty good. Just some things about RTTI use.

antoniovicente · 2021-02-12T22:56:11Z

source/common/http/http2/codec_impl.cc

+    if (stream->stream_id_ == current_stream_id_.value()) {
+      ClientStreamImpl* client_stream = static_cast<ClientStreamImpl*>(stream.get());
+      Router::UpstreamToDownstream* upstream_to_downstream =
+          dynamic_cast<Router::UpstreamToDownstream*>(&client_stream->response_decoder_);


Should we consider adding a dumpState method to Http::ResponseDecoder that can be implemented by Router::UpstreamToDownstream instead of doing this dynamic cast? Google style avoids this kind of use of rtti.

Yes, that's a good idea. I'll have Http::ResponseDecoder have such a method.

antoniovicente · 2021-02-12T23:26:43Z

source/common/http/http2/codec_impl.cc

+  os << spaces << "Dumping corresponding downstream request for upstream stream "
+     << current_stream_id_.value() << ":\n";
+
+  for (auto& stream : active_streams_) {


Consider ConnectionImpl::getStream instead.

I can see iterating over active_streams_ being lower dependency and thus preferable when running from a crash handler.

That's a good suggestion.

I'm a bit torn how to go on this, given that LL are terrible performance wise, and we're iterating over streams when we can just grab the one we want from the map.

Given that the max # of concurrent streams is configurable, and a reasonable number is likely 100, iterating over 100 streams isn't too bad. If this increases significantly though, then I think getStream would become more of a clear winner.

Given the expected size of say ~100 and fewer dependencies (i.e. doesn't depend on nghttp2's map), I think iterating is worth the trade-off. Will add a TODO here perhaps consider switching to getStream if we have 1000s of active streams in a connection.

Thoughts?

I would use getStream.

antoniovicente · 2021-02-12T23:27:43Z

source/common/http/http2/codec_impl.cc

+      const auto addressProvider = upstream_to_downstream->connection().addressProviderSharedPtr();
+      const Http::RequestHeaderMap* request_headers = upstream_to_downstream->downstreamHeaders();
+      DUMP_DETAILS(addressProvider);
+      DUMP_DETAILS(request_headers);


Would be better to have ResponseDecoder dumpState dump these details.

100% agree. Will do.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo

Thanks for the feedback @antoniovicente.

Made the changes discussed below, and will open a PR with just these changes now that #14923 is in.

KBaichoo · 2021-02-16T16:57:32Z

source/common/http/http2/codec_impl.cc

+  os << spaces << "Dumping corresponding downstream request for upstream stream "
+     << current_stream_id_.value() << ":\n";
+
+  for (auto& stream : active_streams_) {


That's a good suggestion.

I'm a bit torn how to go on this, given that LL are terrible performance wise, and we're iterating over streams when we can just grab the one we want from the map.

Given that the max # of concurrent streams is configurable, and a reasonable number is likely 100, iterating over 100 streams isn't too bad. If this increases significantly though, then I think getStream would become more of a clear winner.

Given the expected size of say ~100 and fewer dependencies (i.e. doesn't depend on nghttp2's map), I think iterating is worth the trade-off. Will add a TODO here perhaps consider switching to getStream if we have 1000s of active streams in a connection.

Thoughts?

KBaichoo · 2021-02-16T20:05:52Z

source/common/http/http2/codec_impl.cc

+      const auto addressProvider = upstream_to_downstream->connection().addressProviderSharedPtr();
+      const Http::RequestHeaderMap* request_headers = upstream_to_downstream->downstreamHeaders();
+      DUMP_DETAILS(addressProvider);
+      DUMP_DETAILS(request_headers);


100% agree. Will do.

KBaichoo · 2021-02-16T20:12:07Z

source/common/http/http2/codec_impl.cc

+    if (stream->stream_id_ == current_stream_id_.value()) {
+      ClientStreamImpl* client_stream = static_cast<ClientStreamImpl*>(stream.get());
+      Router::UpstreamToDownstream* upstream_to_downstream =
+          dynamic_cast<Router::UpstreamToDownstream*>(&client_stream->response_decoder_);


Yes, that's a good idea. I'll have Http::ResponseDecoder have such a method.

antoniovicente · 2021-02-17T20:13:50Z

Thanks for the feedback @antoniovicente.

Made the changes discussed below, and will open a PR with just these changes now that #14923 is in.

Just do a main merge, we shouldn't need to start a new PR from scratch.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2021-03-02T15:07:04Z

Merged, thanks @snowp !

KBaichoo · 2021-03-02T18:49:11Z

/retest

macos timed out

repokitteh-read-only · 2021-03-02T18:49:15Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15029 (comment) was created by @KBaichoo.

see: more, trace.

KBaichoo · 2021-03-02T22:09:40Z

/retest

macos was cancelled again

mattklein123

I think needs a release notes fix, thank you.

/wait

docs/root/version_history/current.rst

repokitteh-read-only · 2021-03-02T22:09:44Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15029 (comment) was created by @KBaichoo.

see: more, trace.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

mattklein123 · 2021-03-02T22:20:23Z

Can you merge main also? It will likely fix OSX CI and we can make sure the release notes look good.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2021-03-02T22:25:08Z

Merged

mattklein123 · 2021-03-02T22:29:05Z

docs/root/version_history/current.rst

 * grpc_json_transcoder: added :ref:`request_validation_options <envoy_v3_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.request_validation_options>` to reject invalid requests early.
 * grpc_json_transcoder: filter can now be configured on per-route/per-vhost level as well. Leaving empty list of services in the filter configuration disables transcoding on the specific route.
-* http: added support for `Envoy::ScopeTrackedObject` for HTTP/1 and HTTP/2 dispatching. Crashes while inside the dispatching loop should dump debug information.
+* http: added an option to not add content-length: 0 for requests which should not have bodies. This behavior can be enabled by setting `envoy.reloadable_features.dont_add_content_length_for_bodiless_requests` true.


I think you still have a merge issue? Why is this being added?

/wait

Ah, it was originally in new features but got moved to minor behavior changes on line 45.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2021-03-03T12:43:56Z

/retest

repokitteh-read-only · 2021-03-03T12:43:59Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15029 (comment) was created by @KBaichoo.

see: more, trace.

KBaichoo · 2021-03-03T20:48:35Z

/retest

N-th times the charm mac os? 🤞

repokitteh-read-only · 2021-03-03T20:48:39Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15029 (comment) was created by @KBaichoo.

see: more, trace.

mattklein123

Thanks!

KBaichoo added 7 commits February 3, 2021 03:21

Implement H2 Dump.

5668414

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Use EndsWith instead of HasSubstr; try to appease msvc compiler.

930ab22

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Fix cross platform address issue.

6e92b59

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Escape null char.

337f40a

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Formatting output dump.

53068d9

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Merge remote-tracking branch 'upstream/main' into h2-rebase

1db1ac2

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Implement dumping corresponding request for upstream response crashes.

15cae64

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot assigned antoniovicente Feb 11, 2021

KBaichoo changed the title ~~Associate req~~ HTTP2: Associate Response crashes with the corresponding Request. Feb 11, 2021

Spelling.

07fed49

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente reviewed Feb 12, 2021

View reviewed changes

KBaichoo added 3 commits February 16, 2021 18:51

Merge remote-tracking branch 'upstream/main' into h2-rebase

f3b0502

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Update test from protocol_constraints change from merge.

fd6ea43

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Merged.

6a90ff4

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo commented Feb 17, 2021

View reviewed changes

antoniovicente added the waiting label Feb 17, 2021

KBaichoo added 2 commits February 17, 2021 20:20

Interface changes to avoid RTTI hackery.

354ecf1

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Merge main.

ab13b1a

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot removed the waiting label Feb 17, 2021

KBaichoo marked this pull request as ready for review February 17, 2021 20:28

KBaichoo added 7 commits February 17, 2021 20:38

use getStream.

50b7478

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Release notes.

f393a37

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Fixed includes.

7c66bae

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Format fix.

ef9f6a4

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Invoke the original dump state.

0987c07

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Formatting.

2bf1317

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

clangtidy.

04429a6

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

snowp added the waiting label Mar 2, 2021

Merge remote-tracking branch 'upstream/main' into associateReq

ade30ec

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo dismissed snowp’s stale review via ade30ec March 2, 2021 15:06

repokitteh-read-only bot removed the waiting label Mar 2, 2021

snowp previously approved these changes Mar 2, 2021

View reviewed changes

mattklein123 requested changes Mar 2, 2021

View reviewed changes

docs/root/version_history/current.rst Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Mar 2, 2021

mattklein123 self-assigned this Mar 2, 2021

Fixed release notes.

d073cf4

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo dismissed snowp’s stale review via d073cf4 March 2, 2021 22:17

repokitteh-read-only bot removed the waiting label Mar 2, 2021

Merge remote-tracking branch 'upstream/main' into associateReq

4f6be29

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

mattklein123 reviewed Mar 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 2, 2021

Release notes fix.

4cce60b

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot removed the waiting label Mar 2, 2021

mattklein123 approved these changes Mar 4, 2021

View reviewed changes

mattklein123 merged commit 15f6b7a into envoyproxy:main Mar 4, 2021

Conversation

KBaichoo commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KBaichoo commented Feb 11, 2021

Uh oh!

KBaichoo commented Feb 12, 2021

Uh oh!

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

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

antoniovicente commented Feb 17, 2021

Uh oh!

KBaichoo commented Mar 2, 2021

Uh oh!

KBaichoo commented Mar 2, 2021

Uh oh!

repokitteh-read-only bot commented Mar 2, 2021

Uh oh!

KBaichoo commented Mar 2, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

repokitteh-read-only bot commented Mar 2, 2021

Uh oh!

mattklein123 commented Mar 2, 2021

Uh oh!

KBaichoo commented Mar 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KBaichoo commented Mar 3, 2021

Uh oh!

repokitteh-read-only bot commented Mar 3, 2021

Uh oh!

KBaichoo commented Mar 3, 2021

Uh oh!

repokitteh-read-only bot commented Mar 3, 2021

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

KBaichoo commented Feb 11, 2021 •

edited

Loading