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.

Sign up for GitHub

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

Jump to bottom

http: using CONNECT_ERROR for HTTP/2 #13519

Merged
merged 2 commits into from
Oct 13, 2020
Merged

http: using CONNECT_ERROR for HTTP/2 #13519

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b8b1d45
http: using CONNECT_ERROR for HTTP/1
alyssawilk Oct 12, 2020
7bbd137
details once
alyssawilk Oct 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ Bug Fixes
---------
*Changes expected to improve the state of the world and are unlikely to have negative effects*

* http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests.

Removed Config or Runtime
-------------------------
*Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
Expand Down
4 changes: 3 additions & 1 deletion include/envoy/http/codec.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,9 @@ enum class StreamResetReason {
// If the stream was locally reset due to connection termination.
ConnectionTermination,
// The stream was reset because of a resource overflow.
Overflow
Overflow,
// Either there was an early TCP error for a CONNECT request or the peer reset with CONNECT_ERROR
ConnectError
};

/**
Expand Down
25 changes: 24 additions & 1 deletion source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@
namespace Envoy {
namespace Http {

bool requestWasConnect(const RequestHeaderMapPtr& headers, Protocol protocol) {
if (!headers) {
return false;
}
if (protocol <= Protocol::Http11) {
return HeaderUtility::isConnect(*headers);
}
// All HTTP/2 style upgrades were originally connect requests.
return HeaderUtility::isConnect(*headers) || Utility::isUpgrade(*headers);
}

ConnectionManagerStats ConnectionManagerImpl::generateStats(const std::string& prefix,
Stats::Scope& scope) {
return ConnectionManagerStats(
Expand Down Expand Up @@ -177,7 +188,19 @@ void ConnectionManagerImpl::doEndStream(ActiveStream& stream) {
// TODO(snowp): This call might not be necessary, try to clean up + remove setter function.
stream.filter_manager_.setLocalComplete();
stream.state_.codec_saw_local_complete_ = true;
stream.response_encoder_->getStream().resetStream(StreamResetReason::LocalReset);

// Per https://tools.ietf.org/html/rfc7540#section-8.3 if there was an error
// with the TCP connection during a CONNECT request, it should be
// communicated via CONNECT_ERROR
if (requestWasConnect(stream.request_headers_, codec_->protocol()) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a vague sense that we do many header type checks to see if something is a connect request. Would it be better for perf if did this check once and stored this somewhere? Not a big deal but maybe something to think about in a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, but this case is quite special as most are looking at HTTP/1 style CONNECT requests (HTTP/1 connect and HTTP/2 CONNECTs-excluding-upgrades) and this particular one is doing all CONNECT methods (including HTTP/2 upgrades)

(stream.filter_manager_.streamInfo().hasResponseFlag(
StreamInfo::ResponseFlag::UpstreamConnectionFailure) ||
stream.filter_manager_.streamInfo().hasResponseFlag(
StreamInfo::ResponseFlag::UpstreamConnectionTermination))) {
stream.response_encoder_->getStream().resetStream(StreamResetReason::ConnectError);
} else {
stream.response_encoder_->getStream().resetStream(StreamResetReason::LocalReset);
}
reset_stream = true;
}

Expand Down
18 changes: 15 additions & 3 deletions source/common/http/http2/codec_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,17 @@ class Http2ResponseCodeDetailValues {
}
};

int reasonToReset(StreamResetReason reason) {
switch (reason) {
case StreamResetReason::LocalRefusedStreamReset:
return NGHTTP2_REFUSED_STREAM;
case StreamResetReason::ConnectError:
return NGHTTP2_CONNECT_ERROR;
default:
return NGHTTP2_NO_ERROR;
}
}

using Http2ResponseCodeDetails = ConstSingleton<Http2ResponseCodeDetailValues>;

bool Utility::reconstituteCrumbledCookies(const HeaderString& key, const HeaderString& value,
Expand Down Expand Up @@ -525,9 +536,7 @@ void ConnectionImpl::StreamImpl::resetStream(StreamResetReason reason) {

void ConnectionImpl::StreamImpl::resetStreamWorker(StreamResetReason reason) {
int rc = nghttp2_submit_rst_stream(parent_.session_, NGHTTP2_FLAG_NONE, stream_id_,
reason == StreamResetReason::LocalRefusedStreamReset
? NGHTTP2_REFUSED_STREAM
: NGHTTP2_NO_ERROR);
reasonToReset(reason));
ASSERT(rc == 0);
}

Expand Down Expand Up @@ -984,6 +993,9 @@ int ConnectionImpl::onStreamClose(int32_t stream_id, uint32_t error_code) {
if (error_code == NGHTTP2_REFUSED_STREAM) {
reason = StreamResetReason::RemoteRefusedStreamReset;
stream->setDetails(Http2ResponseCodeDetails::get().remote_refused);
} else if (error_code == NGHTTP2_CONNECT_ERROR) {
reason = StreamResetReason::ConnectError;
stream->setDetails(Http2ResponseCodeDetails::get().remote_reset);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use a different details string here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh, I think it's not worth the detail here (I only split out the reason because otherwise it was impossible to unit test) but I will restructure the code so we only setDetails with it once as a matter of principle :-)

} else {
reason = StreamResetReason::RemoteReset;
stream->setDetails(Http2ResponseCodeDetails::get().remote_reset);
Expand Down
18 changes: 15 additions & 3 deletions source/common/http/http2/codec_impl_legacy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,17 @@ class Http2ResponseCodeDetailValues {
}
};

int reasonToReset(StreamResetReason reason) {
switch (reason) {
case StreamResetReason::LocalRefusedStreamReset:
return NGHTTP2_REFUSED_STREAM;
case StreamResetReason::ConnectError:
return NGHTTP2_CONNECT_ERROR;
default:
return NGHTTP2_NO_ERROR;
}
}

using Http2ResponseCodeDetails = ConstSingleton<Http2ResponseCodeDetailValues>;
using Http::Http2::CodecStats;
using Http::Http2::MetadataDecoder;
Expand Down Expand Up @@ -505,9 +516,7 @@ void ConnectionImpl::StreamImpl::resetStream(StreamResetReason reason) {

void ConnectionImpl::StreamImpl::resetStreamWorker(StreamResetReason reason) {
int rc = nghttp2_submit_rst_stream(parent_.session_, NGHTTP2_FLAG_NONE, stream_id_,
reason == StreamResetReason::LocalRefusedStreamReset
? NGHTTP2_REFUSED_STREAM
: NGHTTP2_NO_ERROR);
reasonToReset(reason));
ASSERT(rc == 0);
}

Expand Down Expand Up @@ -953,6 +962,9 @@ int ConnectionImpl::onStreamClose(int32_t stream_id, uint32_t error_code) {
if (error_code == NGHTTP2_REFUSED_STREAM) {
reason = StreamResetReason::RemoteRefusedStreamReset;
stream->setDetails(Http2ResponseCodeDetails::get().remote_refused);
} else if (error_code == NGHTTP2_CONNECT_ERROR) {
reason = StreamResetReason::ConnectError;
stream->setDetails(Http2ResponseCodeDetails::get().remote_reset);
} else {
reason = StreamResetReason::RemoteReset;
stream->setDetails(Http2ResponseCodeDetails::get().remote_reset);
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,6 +819,8 @@ const std::string Utility::resetReasonToString(const Http::StreamResetReason res
return "remote reset";
case Http::StreamResetReason::RemoteRefusedStreamReset:
return "remote refused stream reset";
case Http::StreamResetReason::ConnectError:
return "remote error with CONNECT request";
}

NOT_REACHED_GCOVR_EXCL_LINE;
Expand Down
5 changes: 2 additions & 3 deletions source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,6 +968,7 @@ void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_
absl::string_view body, bool dropped, absl::string_view details) {
// If we have not yet sent anything downstream, send a response with an appropriate status code.
// Otherwise just reset the ongoing response.
callbacks_->streamInfo().setResponseFlag(response_flags);
if (downstream_response_started_ &&
!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.allow_500_after_100")) {
// This will destroy any created retry timers.
Expand All @@ -977,9 +978,6 @@ void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_
} else {
// This will destroy any created retry timers.
cleanup();

callbacks_->streamInfo().setResponseFlag(response_flags);

// sendLocalReply may instead reset the stream if downstream_response_started_ is true.
callbacks_->sendLocalReply(
code, body,
Expand Down Expand Up @@ -1092,6 +1090,7 @@ Filter::streamResetReasonToResponseFlag(Http::StreamResetReason reset_reason) {
return StreamInfo::ResponseFlag::UpstreamOverflow;
case Http::StreamResetReason::RemoteReset:
case Http::StreamResetReason::RemoteRefusedStreamReset:
case Http::StreamResetReason::ConnectError:
return StreamInfo::ResponseFlag::UpstreamRemoteReset;
}

Expand Down
4 changes: 4 additions & 0 deletions test/common/http/http2/codec_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2621,6 +2621,10 @@ TEST_P(Http2CodecImplTest, ConnectTest) {
expected_headers.setReferenceKey(Headers::get().Protocol, "bytestream");
EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false));
request_encoder_->encodeHeaders(request_headers, false);

EXPECT_CALL(callbacks, onResetStream(StreamResetReason::ConnectError, _));
EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::ConnectError, _));
response_encoder_->getStream().resetStream(StreamResetReason::ConnectError);
}

template <typename, typename> class TestNghttp2SessionFactory;
Expand Down
2 changes: 2 additions & 0 deletions test/common/http/utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,8 @@ TEST(HttpUtility, ResetReasonToString) {
EXPECT_EQ("remote reset", Utility::resetReasonToString(Http::StreamResetReason::RemoteReset));
EXPECT_EQ("remote refused stream reset",
Utility::resetReasonToString(Http::StreamResetReason::RemoteRefusedStreamReset));
EXPECT_EQ("remote error with CONNECT request",
Utility::resetReasonToString(Http::StreamResetReason::ConnectError));
}

// Verify that it resolveMostSpecificPerFilterConfigGeneric works with nil routes.
Expand Down
2 changes: 1 addition & 1 deletion test/common/router/router_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2330,9 +2330,9 @@ TEST_F(RouterTest, UpstreamPerTryTimeoutExcludesNewStream) {
EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_,
putResult(Upstream::Outlier::Result::LocalOriginTimeout, _));
EXPECT_CALL(*per_try_timeout_, disableTimer());
EXPECT_CALL(*response_timeout_, disableTimer());
EXPECT_CALL(callbacks_.stream_info_,
setResponseFlag(StreamInfo::ResponseFlag::UpstreamRequestTimeout));
EXPECT_CALL(*response_timeout_, disableTimer());
Http::TestResponseHeaderMapImpl response_headers{
{":status", "504"}, {"content-length", "24"}, {"content-type", "text/plain"}};
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false));
Expand Down
2 changes: 1 addition & 1 deletion test/integration/websocket_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ TEST_P(WebsocketIntegrationTest, WebSocketConnectionUpstreamDisconnect) {
// Verify both the data and the disconnect went through.
response_->waitForBodyData(5);
EXPECT_EQ("world", response_->body());
waitForClientDisconnectOrReset();
waitForClientDisconnectOrReset(Http::StreamResetReason::ConnectError);
}

TEST_P(WebsocketIntegrationTest, EarlyData) {
Expand Down
6 changes: 5 additions & 1 deletion test/integration/websocket_integration_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#pragma once

#include "envoy/http/codec.h"

#include "test/integration/http_protocol_integration.h"

#include "gtest/gtest.h"
Expand Down Expand Up @@ -35,9 +37,11 @@ class WebsocketIntegrationTest : public HttpProtocolIntegrationTest {
}
}

void waitForClientDisconnectOrReset() {
void waitForClientDisconnectOrReset(
Http::StreamResetReason reason = Http::StreamResetReason::RemoteReset) {
if (downstreamProtocol() != Http::CodecClient::Type::HTTP1) {
response_->waitForReset();
ASSERT_EQ(reason, response_->resetReason());
} else {
ASSERT_TRUE(codec_client_->waitForDisconnect());
}
Expand Down