From 06f42569d883b22642d1e476d986672699937c75 Mon Sep 17 00:00:00 2001 From: Patrik Date: Tue, 21 Apr 2020 23:39:29 +0200 Subject: [PATCH] upstream: fix panic on grpc unknown_service status on healthchecks (#10863) Signed-off-by: Patrik Cyvoct Signed-off-by: pengg --- docs/root/version_history/current.rst | 1 + source/common/upstream/health_checker_impl.cc | 6 ++-- .../upstream/health_checker_impl_test.cc | 28 +++++++++++++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 40603f4b009d..f9c39971f138 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -23,6 +23,7 @@ Changes * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`. * tracing: tracing configuration has been made fully dynamic and every HTTP connection manager can now have a separate :ref:`tracing provider `. +* upstream: fixed a bug where Envoy would panic when receiving a GRPC SERVICE_UNKNOWN status on the health check. Deprecated ---------- diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 70bb4fd9493f..5ca42738d464 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -793,9 +793,11 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::logHealthCheckStatus( case grpc::health::v1::HealthCheckResponse::UNKNOWN: service_status = "unknown"; break; + case grpc::health::v1::HealthCheckResponse::SERVICE_UNKNOWN: + service_status = "service_unknown"; + break; default: - // Should not happen really, Protobuf should not parse undefined enums values. - NOT_REACHED_GCOVR_EXCL_LINE; + service_status = "unknown_healthcheck_response"; break; } } diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index c19007c76846..3ea1592bb9f9 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -4452,6 +4452,34 @@ TEST_F(GrpcHealthCheckerImplTest, GrpcFailUnknown) { cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->health()); } +// Test SERVICE_UNKNOWN health status is considered unhealthy. +TEST_F(GrpcHealthCheckerImplTest, GrpcFailServiceUnknown) { + setupHC(); + expectSingleHealthcheck(HealthTransition::Changed); + EXPECT_CALL(*event_logger_, logEjectUnhealthy(_, _, _)); + EXPECT_CALL(*event_logger_, logUnhealthy(_, _, _, true)); + + respondServiceStatus(0, grpc::health::v1::HealthCheckResponse::SERVICE_UNKNOWN); + EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthFlagGet( + Host::HealthFlag::FAILED_ACTIVE_HC)); + EXPECT_EQ(Host::Health::Unhealthy, + cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->health()); +} + +// Test non existent health status enum is considered unhealthy. +TEST_F(GrpcHealthCheckerImplTest, GrpcFailUnknownHealthStatus) { + setupHC(); + expectSingleHealthcheck(HealthTransition::Changed); + EXPECT_CALL(*event_logger_, logEjectUnhealthy(_, _, _)); + EXPECT_CALL(*event_logger_, logUnhealthy(_, _, _, true)); + + respondServiceStatus(0, static_cast(999)); + EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthFlagGet( + Host::HealthFlag::FAILED_ACTIVE_HC)); + EXPECT_EQ(Host::Health::Unhealthy, + cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->health()); +} + // Test receiving GOAWAY is interpreted as connection close event. TEST_F(GrpcHealthCheckerImplTest, GoAwayProbeInProgress) { // FailureType::Network will be issued, it will render host unhealthy only if unhealthy_threshold