Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: Fix ASSERT failure and infinite loop when attempting to unset readDisable state on a closed connection. #9509

Merged

Conversation

@antoniovicente
Copy link
Contributor

antoniovicente commented Dec 27, 2019

Description: Handle calls to ConnectionImpl::readDisable on Closed connections more gracefully. There are several places under source/common, including ClientConnectionImpl::onMessageComplete, where Connection::readDisable(false) is called on a loop to clear the readDisable state as part of the process of preparing upstream connections for reuse. It is possible to end up in an infinite loop if proxying an HTTP/1 response is framed by connection close, the call to onData triggers readDisable(true), and there is a pending PostIoAction::Close action. The closeSocket call on the upstreamConnection triggers the call to ClientConnectionImpl::onMessageComplete
Risk Level: Low
Testing: unit and integration tests
Docs Changes: n/a
Release Notes: n/a
Fixes #9508

…able state on a closed connection.

Signed-off-by: Antonio Vicente <avd@google.com>
…d_disabled_close

Signed-off-by: Antonio Vicente <avd@google.com>
@mattklein123 mattklein123 self-assigned this Dec 29, 2019
Copy link
Member

mattklein123 left a comment

Awesome, thanks for debugging and fixing this. In general this makes sense to me. Optimally I would like @alyssawilk to review, but if this needs to merge urgently we can do that and she can post-review. Also need to check format.

/wait

@repokitteh repokitteh bot added the waiting label Dec 29, 2019
@antoniovicente

This comment has been minimized.

Copy link
Contributor Author

antoniovicente commented Jan 2, 2020

Awesome, thanks for debugging and fixing this. In general this makes sense to me. Optimally I would like @alyssawilk to review, but if this needs to merge urgently we can do that and she can post-review. Also need to check format.

I think it would be fine to wait until @alyssawilk is back from the holidays

…d_disabled_close

Signed-off-by: Antonio Vicente <avd@google.com>
@repokitteh repokitteh bot removed the waiting label Jan 2, 2020
…rectly.

Signed-off-by: Antonio Vicente <avd@google.com>
Copy link
Contributor

alyssawilk left a comment

Good find - thanks for tackling it!

source/common/network/connection_impl.cc Show resolved Hide resolved
@@ -221,6 +221,10 @@ TEST_P(IntegrationTest, RouterUpstreamDisconnectBeforeResponseComplete) {
testRouterUpstreamDisconnectBeforeResponseComplete();
}

TEST_P(IntegrationTest, ResponseFramedByConnectionCloseWithReadLimits) {
testResponseFramedByConnectionCloseWithReadLimits();

This comment has been minimized.

Copy link
@alyssawilk

alyssawilk Jan 6, 2020

Contributor

If this is only used in one place please just put the code inline - it's not totally obvious but I'm trying to push for inline code where things are not reused.

This comment has been minimized.

Copy link
@antoniovicente

antoniovicente Jan 6, 2020

Author Contributor

Done.

auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
waitForNextUpstreamRequest();
// Disable chunking to trigger framing by connection close.
upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}, {":no-chunks", "1"}},

This comment has been minimized.

Copy link
@alyssawilk

alyssawilk Jan 6, 2020

Contributor

This is just run for HTTP/1.1 right?
ostensibly the no chunk header is never supposed to be sent on the wire - I think if you don't send a content length and send HTTP/1.0 (see existing example tests) you can force frame by close that way. Mind trying it out?

This comment has been minimized.

Copy link
@antoniovicente

antoniovicente Jan 6, 2020

Author Contributor

I'm having some trouble finding examples under test/integration that encode an HTTP/1.0 response. Do you have some links handy?

As far as I can tell ":no-chunks" is the way to tell StreamEncoderImpl::encodeHeaders in source/common/http/http1/codec_impl.cc to not chunk a response without a content-length.

This comment has been minimized.

Copy link
@alyssawilk

alyssawilk Jan 6, 2020

Contributor

grep around for config_helper_.addConfigModifier(&setAllowHttp10WithDefaultHost); ?

They all use sendRawHttpAndWaitForResponse but given this is a header only request and you're waiting for connection close I think that should be fine.

This comment has been minimized.

Copy link
@antoniovicente

antoniovicente Jan 6, 2020

Author Contributor

I'm fairly sure that the :no-chunk does not end up on the wire. One thing that I wanted to verify in some way is that the Envoy does see an upstream response framed by connection close. I'm not sure how to accomplish that, since I think envoy will add chunk encoding to the response it sends downstream.

Let's talk briefly offline.

@repokitteh repokitteh bot removed the waiting:any label Jan 6, 2020
@mattklein123 mattklein123 added the waiting label Jan 6, 2020
… readEnabled is called.

Fix typo "chunking" -> "chunk encoding"

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Antonio Vicente <avd@google.com>
@repokitteh repokitteh bot removed the waiting label Jan 6, 2020
Copy link
Member

mattklein123 left a comment

LGTM, thanks for the fixes here. Will defer to @alyssawilk for further review.

source/common/http/http1/codec_impl.cc Show resolved Hide resolved
upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}, {":no-chunks", "1"}},
false);
Comment on lines +239 to +240

This comment has been minimized.

Copy link
@mattklein123

mattklein123 Jan 7, 2020

Member

If you wanted to avoid the :no-chunks business, you could potentially just use a raw TCP fake upstream connection and manually send an HTTP/1.0 response (you could I suppose also build HTTP/1.0 handling into the fake upstream but that sounds like a lot more work), which IIRC Envoy should handle correctly with close framing. I don't feel strongly about it but I know @alyssawilk commented on this.

This comment has been minimized.

Copy link
@alyssawilk

alyssawilk Jan 7, 2020

Contributor

I take Antonio's point that :no-chunks doesn't reach the wire - I'm (warily) fine with it.

@alyssawilk alyssawilk dismissed mattklein123’s stale review Jan 7, 2020

has alyssawilk seal of approval :-P

@alyssawilk alyssawilk merged commit 9de6027 into envoyproxy:master Jan 7, 2020
17 checks passed
17 checks passed
DCO DCO
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: coverage_publish Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: filter_example_mirror Your tests passed on CircleCI!
Details
ci/circleci: go_control_plane_mirror Your tests passed on CircleCI!
Details
envoy-linux Build #20200106.52 succeeded
Details
envoy-linux (bazel asan) bazel asan succeeded
Details
envoy-linux (bazel clang_tidy) bazel clang_tidy succeeded
Details
envoy-linux (bazel compile_time_options) bazel compile_time_options succeeded
Details
envoy-linux (bazel gcc) bazel gcc succeeded
Details
envoy-linux (bazel release) bazel release succeeded
Details
envoy-linux (bazel tsan) bazel tsan succeeded
Details
envoy-linux (format) format succeeded
Details
envoy-macos Build #20200106.58 succeeded
Details
envoy-windows Build #20200106.57 succeeded
Details
@lambdai

This comment has been minimized.

Copy link
Contributor

lambdai commented Mar 9, 2020

/backport

dmitri-d added a commit to dmitri-d/envoy that referenced this pull request Mar 25, 2020
…isable state on a closed connection

This is a port of envoyproxy#9509

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
dmitri-d added a commit to dmitri-d/envoy that referenced this pull request Mar 25, 2020
…isable state on a closed connection

This is a port of envoyproxy#9509

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>
dmitri-d added a commit to dmitri-d/envoy that referenced this pull request Mar 25, 2020
…isable state on a closed connection

This is a port of envoyproxy#9509

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>
dmitri-d added a commit to dmitri-d/envoy that referenced this pull request Mar 25, 2020
…isable state on a closed connection

This is a port of envoyproxy#9509

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
@PiotrSikora

This comment has been minimized.

Copy link
Member

PiotrSikora commented Mar 26, 2020

@lambdai could you create the PR against release/v1.12? Thanks!

dmitri-d added a commit to dmitri-d/envoy that referenced this pull request Mar 26, 2020
…isable state on a closed connection

This is a port of envoyproxy#9509

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
istio-testing pushed a commit to istio/envoy that referenced this pull request Mar 26, 2020
…isable state on a closed connection (#190)

This is a port of envoyproxy#9509

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.