Skip to content

GitLab

T
tangled
Commit 13955294 authored by Joe Downing's avatar Joe Downing Committed by Chromium LUCI CQ
Browse files
Options

Update ProtobufHttpClient to treat OAuth network errors as 'unavailable' 

During a recent service outage, most of our hosts came back online but
some did not.  Looking at the logs, the hosts which failed to come
online were treating HTTP 500 errors as 'UNAUTHENTICATED' which is a
permanent error that caused the hosts to go offline.

Instead of returning UNAUTHENTICATED in this case, UNAVAILABLE should
be returned.  This is a retryable error and is handled as such in the
various classes which use the ProtobufHttpClient class.

Change-Id: Idcfe98082f382b4053dfd6dbb2b4570621375f15
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2595521Reviewed-by: default avatarYuwei Huang <yuweih@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837705}
parent f2eecd43
Hide whitespace changes
Inline Side-by-side
Showing with 127 additions and 24 deletions
remoting/base/protobuf_http_client.cc
View file @ 13955294
......@@ -78,8 +78,19 @@ void ProtobufHttpClient::DoExecuteRequest(
std::string error_message =
base::StringPrintf("Failed to fetch access token. Status: %d", status);
LOG(ERROR) << error_message;
request->OnAuthFailed(ProtobufHttpStatus(
ProtobufHttpStatus::Code::UNAUTHENTICATED, error_message));
ProtobufHttpStatus::Code code;
switch (status) {
case OAuthTokenGetter::Status::AUTH_ERROR:
code = ProtobufHttpStatus::Code::UNAUTHENTICATED;
break;
case OAuthTokenGetter::Status::NETWORK_ERROR:
code = ProtobufHttpStatus::Code::UNAVAILABLE;
break;
default:
NOTREACHED() << "Unknown OAuthTokenGetter Status: " << status;
code = ProtobufHttpStatus::Code::UNKNOWN;
}
request->OnAuthFailed(ProtobufHttpStatus(code, error_message));
return;
}
......
remoting/base/protobuf_http_client_unittest.cc
View file @ 13955294
......@@ -140,7 +140,9 @@ std::string CreateSerializedStreamBodyWithStatusCode(
class ProtobufHttpClientTest : public testing::Test {
protected:
void ExpectCallWithToken(bool success);
void ExpectCallWithTokenSuccess();
void ExpectCallWithTokenAuthError();
void ExpectCallWithTokenNetworkError();
base::test::SingleThreadTaskEnvironment task_environment_{
base::test::TaskEnvironment::TimeSource::MOCK_TIME};
......@@ -153,12 +155,22 @@ class ProtobufHttpClientTest : public testing::Test {
test_shared_loader_factory_};
};
void ProtobufHttpClientTest::ExpectCallWithToken(bool success) {
void ProtobufHttpClientTest::ExpectCallWithTokenSuccess() {
EXPECT_CALL(mock_token_getter_, CallWithToken(_))
.WillOnce(RunOnceCallback<0>(success
? OAuthTokenGetter::Status::SUCCESS
: OAuthTokenGetter::Status::AUTH_ERROR,
"", success ? kFakeAccessToken : ""));
.WillOnce(RunOnceCallback<0>(OAuthTokenGetter::Status::SUCCESS, "",
kFakeAccessToken));
}
void ProtobufHttpClientTest::ExpectCallWithTokenAuthError() {
EXPECT_CALL(mock_token_getter_, CallWithToken(_))
.WillOnce(
RunOnceCallback<0>(OAuthTokenGetter::Status::AUTH_ERROR, "", ""));
}
void ProtobufHttpClientTest::ExpectCallWithTokenNetworkError() {
EXPECT_CALL(mock_token_getter_, CallWithToken(_))
.WillOnce(
RunOnceCallback<0>(OAuthTokenGetter::Status::NETWORK_ERROR, "", ""));
}
// Unary request tests.
......@@ -166,7 +178,7 @@ void ProtobufHttpClientTest::ExpectCallWithToken(bool success) {
TEST_F(ProtobufHttpClientTest, SendRequestAndDecodeResponse) {
base::RunLoop run_loop;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
MockEchoResponseCallback response_callback;
EXPECT_CALL(response_callback, Run(HasErrorCode(ProtobufHttpStatus::Code::OK),
......@@ -223,7 +235,7 @@ TEST_F(ProtobufHttpClientTest,
FailedToFetchAuthToken_RejectsWithUnauthorizedError) {
base::RunLoop run_loop;
ExpectCallWithToken(/* success= */ false);
ExpectCallWithTokenAuthError();
MockEchoResponseCallback response_callback;
EXPECT_CALL(response_callback,
......@@ -239,10 +251,30 @@ TEST_F(ProtobufHttpClientTest,
ASSERT_FALSE(client_.HasPendingRequests());
}
TEST_F(ProtobufHttpClientTest,
FailedToFetchAuthToken_RejectsWithUnavailableError) {
base::RunLoop run_loop;
ExpectCallWithTokenNetworkError();
MockEchoResponseCallback response_callback;
EXPECT_CALL(response_callback,
Run(HasErrorCode(ProtobufHttpStatus::Code::UNAVAILABLE),
IsNullResponse()))
.WillOnce([&]() { run_loop.Quit(); });
auto request = CreateDefaultTestRequest();
request->SetResponseCallback(response_callback.Get());
client_.ExecuteRequest(std::move(request));
run_loop.Run();
ASSERT_FALSE(client_.HasPendingRequests());
}
TEST_F(ProtobufHttpClientTest, FailedToParseResponse_GetsInvalidResponseError) {
base::RunLoop run_loop;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
MockEchoResponseCallback response_callback;
EXPECT_CALL(
......@@ -263,7 +295,7 @@ TEST_F(ProtobufHttpClientTest, FailedToParseResponse_GetsInvalidResponseError) {
TEST_F(ProtobufHttpClientTest, ServerRespondsWithErrorStatusMessage) {
base::RunLoop run_loop;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
MockEchoResponseCallback response_callback;
EXPECT_CALL(response_callback,
......@@ -292,7 +324,7 @@ TEST_F(ProtobufHttpClientTest, ServerRespondsWithErrorStatusMessage) {
TEST_F(ProtobufHttpClientTest, ServerRespondsWithHttpErrorCode) {
base::RunLoop run_loop;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
MockEchoResponseCallback response_callback;
EXPECT_CALL(response_callback,
......@@ -339,7 +371,7 @@ TEST_F(ProtobufHttpClientTest,
CancelPendingRequestsAfterTokenCallback_CallbackNotCalled) {
base::RunLoop run_loop;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
client_.ExecuteRequest(CreateDefaultTestRequest());
......@@ -356,7 +388,7 @@ TEST_F(ProtobufHttpClientTest,
TEST_F(ProtobufHttpClientTest, RequestTimeout_ReturnsDeadlineExceeded) {
base::RunLoop run_loop;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
MockEchoResponseCallback response_callback;
EXPECT_CALL(response_callback,
......@@ -379,7 +411,7 @@ TEST_F(ProtobufHttpClientTest, RequestTimeout_ReturnsDeadlineExceeded) {
}
TEST_F(ProtobufHttpClientTest, DeletesRequestHolderWhenRequestIsCanceled) {
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
MockEchoResponseCallback never_called_response_callback;
......@@ -405,7 +437,7 @@ TEST_F(ProtobufHttpClientTest, DeletesRequestHolderWhenRequestIsCanceled) {
TEST_F(ProtobufHttpClientTest, DeletesRequestHolderAfterResponseIsReceived) {
base::RunLoop run_loop;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
MockEchoResponseCallback response_callback;
EXPECT_CALL(response_callback, Run(HasErrorCode(ProtobufHttpStatus::Code::OK),
......@@ -441,7 +473,7 @@ TEST_F(ProtobufHttpClientTest,
base::RunLoop run_loop;
ExpectCallWithToken(/* success= */ false);
ExpectCallWithTokenAuthError();
MockEchoResponseCallback response_callback;
EXPECT_CALL(stream_closed_callback,
......@@ -458,6 +490,31 @@ TEST_F(ProtobufHttpClientTest,
ASSERT_FALSE(client_.HasPendingRequests());
}
TEST_F(ProtobufHttpClientTest,
StreamRequestFailedToFetchAuthToken_RejectsWithUnavailableError) {
base::MockOnceClosure stream_ready_callback;
MockEchoMessageCallback message_callback;
MockStreamClosedCallback stream_closed_callback;
base::RunLoop run_loop;
ExpectCallWithTokenNetworkError();
MockEchoResponseCallback response_callback;
EXPECT_CALL(stream_closed_callback,
Run(HasErrorCode(ProtobufHttpStatus::Code::UNAVAILABLE)))
.WillOnce([&]() { run_loop.Quit(); });
auto request = CreateDefaultTestStreamRequest();
request->SetStreamReadyCallback(stream_ready_callback.Get());
request->SetMessageCallback(message_callback.Get());
request->SetStreamClosedCallback(stream_closed_callback.Get());
client_.ExecuteRequest(std::move(request));
run_loop.Run();
ASSERT_FALSE(client_.HasPendingRequests());
}
TEST_F(ProtobufHttpClientTest, StartStreamRequestAndDecodeMessages) {
base::MockOnceClosure stream_ready_callback;
MockEchoMessageCallback message_callback;
......@@ -466,7 +523,7 @@ TEST_F(ProtobufHttpClientTest, StartStreamRequestAndDecodeMessages) {
{
InSequence s;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
EXPECT_CALL(stream_ready_callback, Run());
EXPECT_CALL(message_callback, Run(IsResponseText("response text 1")));
EXPECT_CALL(message_callback, Run(IsResponseText("response text 2")));
......@@ -507,7 +564,7 @@ TEST_F(ProtobufHttpClientTest, InvalidStreamData_Ignored) {
{
InSequence s;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
EXPECT_CALL(stream_ready_callback, Run());
EXPECT_CALL(stream_closed_callback,
Run(HasErrorCode(ProtobufHttpStatus::Code::OK)))
......@@ -536,7 +593,7 @@ TEST_F(ProtobufHttpClientTest, SendHttpStatusOnly_StreamClosesWithHttpStatus) {
{
InSequence s;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
EXPECT_CALL(stream_closed_callback,
Run(HasErrorCode(ProtobufHttpStatus::Code::UNAUTHENTICATED)))
.WillOnce([&]() { run_loop.Quit(); });
......@@ -563,7 +620,7 @@ TEST_F(ProtobufHttpClientTest, SendStreamStatusAndHttpStatus_StreamStatusWins) {
{
InSequence s;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
EXPECT_CALL(stream_ready_callback, Run());
EXPECT_CALL(stream_closed_callback,
Run(HasErrorCode(ProtobufHttpStatus::Code::CANCELLED)))
......@@ -593,7 +650,7 @@ TEST_F(ProtobufHttpClientTest, StreamReadyTimeout) {
{
InSequence s;
ExpectCallWithToken(/* success= */ true);
ExpectCallWithTokenSuccess();
EXPECT_CALL(stream_closed_callback,
Run(HasErrorCode(ProtobufHttpStatus::Code::DEADLINE_EXCEEDED)));
}
......
remoting/host/heartbeat_sender.cc
View file @ 13955294
......@@ -352,7 +352,6 @@ void HeartbeatSender::OnResponse(
// Calculate delay before sending the next message.
base::TimeDelta delay;
// See CoreErrorDomainTranslator.java for the mapping
switch (status.error_code()) {
case ProtobufHttpStatus::Code::OK:
delay = base::TimeDelta::FromSeconds(response->set_interval_seconds());
......
remoting/host/heartbeat_sender_unittest.cc
View file @ 13955294
......@@ -285,6 +285,42 @@ TEST_F(HeartbeatSenderTest, FailedToHeartbeat_Backoff) {
ASSERT_EQ(0, GetBackoff().failure_count());
}
TEST_F(HeartbeatSenderTest, HostComesBackOnlineAfterServiceOutage) {
// Each call will simulate ~10 minutes of time (at max backoff duration).
// We want to simulate a long outage (~3 hours) so run through 20 iterations.
int retry_attempts = 20;
{
InSequence sequence;
EXPECT_CALL(*mock_client_, Heartbeat(_, _))
.Times(retry_attempts)
.WillRepeatedly([&](std::unique_ptr<apis::v1::HeartbeatRequest> request,
HeartbeatResponseCallback callback) {
ValidateHeartbeat(std::move(request), true);
std::move(callback).Run(
ProtobufHttpStatus(ProtobufHttpStatus::Code::UNAVAILABLE,
"unavailable"),
nullptr);
});
EXPECT_CALL(*mock_client_, Heartbeat(_, _))
.WillOnce(DoValidateHeartbeatAndRespondOk(true));
}
EXPECT_CALL(*mock_observer_, OnHeartbeatSent()).WillRepeatedly(Return());
ASSERT_EQ(0, GetBackoff().failure_count());
signal_strategy_->Connect();
for (int i = 1; i <= retry_attempts; i++) {
ASSERT_EQ(i, GetBackoff().failure_count());
task_environment_.FastForwardBy(GetBackoff().GetTimeUntilRelease());
}
// Host successfully back online.
ASSERT_EQ(0, GetBackoff().failure_count());
}
TEST_F(HeartbeatSenderTest, Unauthenticated) {
int heartbeat_count = 0;
EXPECT_CALL(*mock_client_, Heartbeat(_, _))
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment