Skip to content

health check: gracefully handle GOAWAY in grpc health checker #11324

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

Merged
snowp merged 20 commits into from
Jun 24, 2020
Merged

health check: gracefully handle GOAWAY in grpc health checker #11324

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e4ddb8e
health check: gracefully handle GOAWAY in grpc health checker
jmuia May 26, 2020
70263a6
Close connection after logging and add unit test for remote close
jmuia May 27, 2020
a069698
wip
murray-stripe Jun 5, 2020
725caf8
wip - plumb through
jmuia Jun 9, 2020
58d2143
Merge branch 'envoyproxy/master' into jmuia.grpc-hc-goaway
jmuia Jun 10, 2020
00a1fea
fix format with 'tools/code_format/check_format.py fix'
jmuia Jun 10, 2020
1828a2b
Revert "fix format with 'tools/code_format/check_format.py fix'"
murray-stripe Jun 10, 2020
64c6aef
Merge pull request #1 from jmuia/wip/grpc-hc-goaway
murray-stripe Jun 10, 2020
6ba45a4
only handle GOAWAY NO_ERROR gracefully
jmuia Jun 10, 2020
c5958dc
add inline comments
jmuia Jun 10, 2020
b474edd
make quic error mapper consistent with http2
jmuia Jun 10, 2020
9171180
fix up declaration
jmuia Jun 11, 2020
73e7308
run onGoAway callbacks for each GOAWAY received
jmuia Jun 11, 2020
dc5effd
rename ErrorCode to GoAwayErrorCode
jmuia Jun 15, 2020
7c7d930
rename received_goaway_ to received_no_error_goaway_
jmuia Jun 15, 2020
41ea1e1
fix format
jmuia Jun 16, 2020
a66c176
add comment to GoAwayErrorCode enum
jmuia Jun 16, 2020
bf9c59c
Revert "run onGoAway callbacks for each GOAWAY received"
jmuia Jun 24, 2020
878bd4a
Add TODO for handling multiple GOAWAY frames
jmuia Jun 24, 2020
541eb54
kick ci
jmuia Jun 24, 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
7 changes: 6 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 @@ -36,6 +36,11 @@ const char MaxResponseHeadersCountOverrideKey[] =

class Stream;

enum class GoAwayErrorCode {

@snowp snowp Jun 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sorry should have mention this but this should have a doc comment as its in include/

NoError,
Other,
};

/**
* Stream encoder options specific to HTTP/1.
*/
Expand Down Expand Up @@ -324,7 +329,7 @@ class ConnectionCallbacks {
/**
* Fires when the remote indicates "go away." No new streams should be created.
*/
virtual void onGoAway() PURE;
virtual void onGoAway(GoAwayErrorCode error_code) PURE;
};

/**
Expand Down
4 changes: 2 additions & 2 deletions source/common/http/codec_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,9 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,
Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher);

// Http::ConnectionCallbacks
void onGoAway() override {
void onGoAway(GoAwayErrorCode error_code) override {
if (codec_callbacks_) {
codec_callbacks_->onGoAway();
codec_callbacks_->onGoAway(error_code);
}
}

Expand Down
2 changes: 1 addition & 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 @@ -464,7 +464,7 @@ void ConnectionManagerImpl::doConnectionClose(
}
}

void ConnectionManagerImpl::onGoAway() {
void ConnectionManagerImpl::onGoAway(GoAwayErrorCode) {
// Currently we do nothing with remote go away frames. In the future we can decide to no longer
// push resources if applicable.
}
Expand Down
2 changes: 1 addition & 1 deletion source/common/http/conn_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
void initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) override;

// Http::ConnectionCallbacks
void onGoAway() override;
void onGoAway(GoAwayErrorCode error_code) override;

// Http::ServerConnectionCallbacks
RequestDecoder& newStream(ResponseEncoder& response_encoder,
Expand Down
20 changes: 14 additions & 6 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 @@ -471,7 +471,7 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat
http2_options.max_inbound_priority_frames_per_stream().value()),
max_inbound_window_update_frames_per_data_frame_sent_(
http2_options.max_inbound_window_update_frames_per_data_frame_sent().value()),
dispatching_(false), raised_goaway_(false), pending_deferred_reset_(false) {}
dispatching_(false), pending_deferred_reset_(false) {}

ConnectionImpl::~ConnectionImpl() { nghttp2_session_del(session_); }

Expand Down Expand Up @@ -563,6 +563,16 @@ int ConnectionImpl::onBeforeFrameReceived(const nghttp2_frame_hd* hd) {
return 0;
}

ABSL_MUST_USE_RESULT
enum GoAwayErrorCode ngHttp2ErrorCodeToErrorCode(uint32_t code) noexcept {
switch (code) {
case NGHTTP2_NO_ERROR:
return GoAwayErrorCode::NoError;
default:
return GoAwayErrorCode::Other;
}
}

int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) {
ENVOY_CONN_LOG(trace, "recv frame type={}", connection_, static_cast<uint64_t>(frame->hd.type));

Expand All @@ -577,12 +587,10 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) {
}
}

// Only raise GOAWAY once, since we don't currently expose stream information. Shutdown
// notifications are the same as a normal GOAWAY.
if (frame->hd.type == NGHTTP2_GOAWAY && !raised_goaway_) {
// Shutdown notifications are the same as a normal GOAWAY.
if (frame->hd.type == NGHTTP2_GOAWAY) {

@jmuia jmuia Jun 12, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is fine or if we want to track which goaway error codes we've already seen, and run the callbacks once per error code.

@snowp snowp Jun 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it expected to get multiple GOAWAY frames with different error codes? If yes, it seems bad only look at the first one, if no then we can probably keep the old logic.

@jmuia jmuia Jun 15, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is possible to receive GOAWAY frames with different error codes:

An endpoint MAY send multiple GOAWAY frames if circumstances change. For instance, an endpoint that sends GOAWAY with NO_ERROR during graceful shutdown could subsequently encounter a condition that requires immediate termination of the connection.

(from https://http2.github.io/http2-spec/#GOAWAY)

@snowp snowp Jun 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm in that case should the logic for failing the HC be that we see a NO_ERROR + no error GOAWAYs?

Also I'm not sure what the implications are of making onGoAway trigger multiple times per stream, maybe @mattklein123 or @alyssawilk has an idea? I'd be worried about invariants elsewhere relying on this being called at most once.

@jmuia jmuia Jun 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm in that case should the logic for failing the HC be that we see a NO_ERROR + no error GOAWAYs?

Hm, I don't quite follow, could you rephrase?

I'd be worried about invariants elsewhere relying on this being called at most once.

Agreed. It seems it was added in #103. I'm not sure if that was an optimization back then. I'm also not sure if it has become an expected behaviour (intentionally or not) since then.

@snowp snowp Jun 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  if (request_encoder_ && error_code == Http::GoAwayErrorCode::NoError) {
    received_no_error_goaway_ = true;
    return;
  }

if a GOAWAY error follows a NO_ERROR then received_no_error_goaway_ would still be true. I was thinking that the logic should reset this flag if it saw an error frame.

@jmuia jmuia Jun 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. I don't expect that to change the behaviour as it stands:

  • If there isn't an active probe, the first (NO_ERROR) GOAWAY would've caused the connection to be closed (and we only set received_no_error_goaway_ = true; when there's an active probe).
  • If there is an active probe, the stream reset will end up resetting state (resetState() will be called by onResetStream()).
  • In general, I believe a connection close will end up resetting state because the onResetStream() callbacks will run (from here).

I admit this isn't obvious / does feel a bit brittle -- happy to add a comment and/or reset the flag anyways.

@jmuia jmuia Jun 19, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Friendly ping @mattklein123 @alyssawilk

@mattklein123 mattklein123 Jun 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for the delay. My preference would be to not raise multiple goaways from the codec as I'm guessing there will be code that won't expect this and will break.

Can we just keep the previous behavior and not handle the case where multiple go away frames are received? We can potentially deal with this in a follow up if someone requests it?

@jmuia jmuia Jun 24, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback - that seems reasonable to me. I've reverted that specific commit and added a TODO instead.

ASSERT(frame->hd.stream_id == 0);
raised_goaway_ = true;
callbacks().onGoAway();
callbacks().onGoAway(ngHttp2ErrorCodeToErrorCode(frame->goaway.error_code));
return 0;
}

Expand Down
1 change: 0 additions & 1 deletion source/common/http/http2/codec_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,6 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
void releaseOutboundControlFrame(const Buffer::OwnedBufferFragmentImpl* fragment);

bool dispatching_ : 1;
bool raised_goaway_ : 1;
bool pending_deferred_reset_ : 1;
};

Expand Down
2 changes: 1 addition & 1 deletion source/common/http/http2/conn_pool.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ ConnPoolImpl::~ConnPoolImpl() { destructAllConnections(); }
ConnPoolImplBase::ActiveClientPtr ConnPoolImpl::instantiateActiveClient() {
return std::make_unique<ActiveClient>(*this);
}
void ConnPoolImpl::onGoAway(ActiveClient& client) {
void ConnPoolImpl::onGoAway(ActiveClient& client, Http::GoAwayErrorCode) {
ENVOY_CONN_LOG(debug, "remote goaway", *client.codec_client_);
host_->cluster().stats().upstream_cx_close_notify_.inc();
if (client.state_ != ActiveClient::State::DRAINING) {
Expand Down
6 changes: 4 additions & 2 deletions source/common/http/http2/conn_pool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,16 @@ class ConnPoolImpl : public ConnPoolImplBase {
}

// Http::ConnectionCallbacks
void onGoAway() override { parent().onGoAway(*this); }
void onGoAway(Http::GoAwayErrorCode error_code) override {
parent().onGoAway(*this, error_code);
}

bool closed_with_active_rq_{};
};

uint64_t maxRequestsPerConnection();
void movePrimaryClientToDraining();
void onGoAway(ActiveClient& client);
void onGoAway(ActiveClient& client, Http::GoAwayErrorCode error_code);
void onStreamDestroy(ActiveClient& client);
void onStreamReset(ActiveClient& client, Http::StreamResetReason reason);

Expand Down
29 changes: 23 additions & 6 deletions source/common/upstream/health_checker_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,19 +703,22 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onInterval() {
void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onResetStream(Http::StreamResetReason,
absl::string_view) {
const bool expected_reset = expect_reset_;
const bool goaway = received_no_error_goaway_;
resetState();

if (expected_reset) {
// Stream reset was initiated by us (bogus gRPC response, timeout or cluster host is going
// away). In these cases health check failure has already been reported, so just return.
// away). In these cases health check failure has already been reported and a GOAWAY (if any)
// has already been handled, so just return.
return;
}

ENVOY_CONN_LOG(debug, "connection/stream error health_flags={}", *client_,
HostUtility::healthFlagsToString(*host_));

if (!parent_.reuse_connection_) {
// Stream reset was unexpected, so we haven't closed the connection yet.
if (goaway || !parent_.reuse_connection_) {
// Stream reset was unexpected, so we haven't closed the connection
// yet in response to a GOAWAY or due to disabled connection reuse.
client_->close();
}

Expand All @@ -727,9 +730,19 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onResetStream(Http::St
handleFailure(envoy::data::core::v3::NETWORK);
}

void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onGoAway() {
void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onGoAway(
Http::GoAwayErrorCode error_code) {
ENVOY_CONN_LOG(debug, "connection going away health_flags={}", *client_,
HostUtility::healthFlagsToString(*host_));
// If we have an active health check probe and receive a GOAWAY indicating
// graceful shutdown, allow the probe to complete before closing the connection.
// The connection will be closed when the active check completes or another
// terminal condition occurs, such as a timeout or stream reset.
if (request_encoder_ && error_code == Http::GoAwayErrorCode::NoError) {
received_no_error_goaway_ = true;
return;
}

// Even if we have active health check probe, fail it on GOAWAY and schedule new one.
if (request_encoder_) {
handleFailure(envoy::data::core::v3::NETWORK);
Expand Down Expand Up @@ -762,6 +775,9 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onRpcComplete(
handleFailure(envoy::data::core::v3::ACTIVE);
}

// Read the value as we may call resetState() and clear it.
const bool goaway = received_no_error_goaway_;

// |end_stream| will be false if we decided to stop healthcheck before HTTP stream has ended -
// invalid gRPC payload, unexpected message stream or wrong content-type.
if (end_stream) {
Expand All @@ -772,7 +788,7 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onRpcComplete(
request_encoder_->getStream().resetStream(Http::StreamResetReason::LocalReset);
}

if (!parent_.reuse_connection_) {
if (!parent_.reuse_connection_ || goaway) {
client_->close();
}
}
Expand All @@ -782,13 +798,14 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::resetState() {
request_encoder_ = nullptr;
decoder_ = Grpc::Decoder();
health_check_response_.reset();
received_no_error_goaway_ = false;
}

void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onTimeout() {
ENVOY_CONN_LOG(debug, "connection/stream timeout health_flags={}", *client_,
HostUtility::healthFlagsToString(*host_));
expect_reset_ = true;
Comment thread
snowp marked this conversation as resolved.
if (!parent_.reuse_connection_) {
if (received_no_error_goaway_ || !parent_.reuse_connection_) {
client_->close();
} else {
request_encoder_->getStream().resetStream(Http::StreamResetReason::LocalReset);
Expand Down
7 changes: 5 additions & 2 deletions source/common/upstream/health_checker_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase {
void onBelowWriteBufferLowWatermark() override {}

void onEvent(Network::ConnectionEvent event);
void onGoAway();
void onGoAway(Http::GoAwayErrorCode error_code);

class ConnectionCallbackImpl : public Network::ConnectionCallbacks {
public:
Expand All @@ -341,7 +341,7 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase {
public:
HttpConnectionCallbackImpl(GrpcActiveHealthCheckSession& parent) : parent_(parent) {}
// Http::ConnectionCallbacks
void onGoAway() override { parent_.onGoAway(); }
void onGoAway(Http::GoAwayErrorCode error_code) override { parent_.onGoAway(error_code); }

private:
GrpcActiveHealthCheckSession& parent_;
Expand All @@ -358,6 +358,9 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase {
// e.g. remote reset. In this case healthcheck status has already been reported, only state
// cleanup is required.
bool expect_reset_ = false;
// If true, we received a GOAWAY (NO_ERROR code) and are deferring closing the connection
// until the active probe completes.
bool received_no_error_goaway_ = false;
};

virtual Http::CodecClientPtr createCodecClient(Upstream::Host::CreateConnectionData& data) PURE;
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "extensions/quic_listeners/quiche/envoy_quic_client_session.h"

#include "extensions/quic_listeners/quiche/envoy_quic_utils.h"

namespace Envoy {
namespace Quic {

Expand Down Expand Up @@ -51,7 +53,7 @@ void EnvoyQuicClientSession::OnGoAway(const quic::QuicGoAwayFrame& frame) {
quic::QuicErrorCodeToString(frame.error_code), frame.reason_phrase);
quic::QuicSpdyClientSession::OnGoAway(frame);
if (http_connection_callbacks_ != nullptr) {
http_connection_callbacks_->onGoAway();
http_connection_callbacks_->onGoAway(quicErrorCodeToEnvoyErrorCode(frame.error_code));
}
}

Expand Down
9 changes: 9 additions & 0 deletions source/extensions/quic_listeners/quiche/envoy_quic_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,15 @@ Http::StreamResetReason quicErrorCodeToEnvoyResetReason(quic::QuicErrorCode erro
}
}

Http::GoAwayErrorCode quicErrorCodeToEnvoyErrorCode(quic::QuicErrorCode error) noexcept {
switch (error) {
case quic::QUIC_NO_ERROR:
return Http::GoAwayErrorCode::NoError;
default:
return Http::GoAwayErrorCode::Other;
}
}

Network::ConnectionSocketPtr
createConnectionSocket(Network::Address::InstanceConstSharedPtr& peer_addr,
Network::Address::InstanceConstSharedPtr& local_addr,
Expand Down
4 changes: 4 additions & 0 deletions source/extensions/quic_listeners/quiche/envoy_quic_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ Http::StreamResetReason quicRstErrorToEnvoyResetReason(quic::QuicRstStreamErrorC
// Called when underlying QUIC connection is closed either locally or by peer.
Http::StreamResetReason quicErrorCodeToEnvoyResetReason(quic::QuicErrorCode error);

// Called when a GOAWAY frame is received.
ABSL_MUST_USE_RESULT
Http::GoAwayErrorCode quicErrorCodeToEnvoyErrorCode(quic::QuicErrorCode error) noexcept;

// Create a connection socket instance and apply given socket options to the
// socket. IP_PKTINFO and SO_RXQ_OVFL is always set if supported.
Network::ConnectionSocketPtr
Expand Down
2 changes: 1 addition & 1 deletion test/common/http/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class CodecClientForTest : public Http::CodecClient {
destroy_cb_(this);
}
}
void raiseGoAway() { onGoAway(); }
void raiseGoAway(Http::GoAwayErrorCode error_code) { onGoAway(error_code); }
Event::Timer* idleTimer() { return idle_timer_.get(); }

DestroyCb destroy_cb_;
Expand Down
5 changes: 3 additions & 2 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 @@ -317,7 +317,8 @@ TEST_P(Http2CodecImplTest, ShutdownNotice) {
EXPECT_CALL(request_decoder_, decodeHeaders_(_, true));
request_encoder_->encodeHeaders(request_headers, true);

EXPECT_CALL(client_callbacks_, onGoAway());
// Called once from the shutdown notice and once from the goaway.
EXPECT_CALL(client_callbacks_, onGoAway(_)).Times(2);
server_->shutdownNotice();
server_->goAway();

Expand Down Expand Up @@ -1456,7 +1457,7 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersExceedPerHeaderLimit) {
request_headers.addCopy("big", long_string);

EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(0);
EXPECT_CALL(client_callbacks_, onGoAway());
EXPECT_CALL(client_callbacks_, onGoAway(_));

@jmuia jmuia Jun 12, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't completely follow why this test doesn't trigger onGoAway(_) to be called twice like the other one.

server_->shutdownNotice();
server_->goAway();
request_encoder_->encodeHeaders(request_headers, true);
Expand Down
2 changes: 1 addition & 1 deletion test/common/http/http2/conn_pool_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -971,7 +971,7 @@ TEST_F(Http2ConnPoolImplTest, GoAway) {
r1.inner_decoder_->decodeHeaders(
ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true);

test_clients_[0].codec_client_->raiseGoAway();
test_clients_[0].codec_client_->raiseGoAway(Http::GoAwayErrorCode::NoError);

expectClientCreate();
ActiveTestRequest r2(*this, 1, false);
Expand Down
Loading