[Outlier Detection] Replace HTTP status with gRPC status

Signed-off-by: ZhouyihaiDing <ddyihai@google.com>

source/common/router/router.cc

@@ -875,6 +875,12 @@ void Filter::onUpstreamReset(Http::StreamResetReason reset_reason,
   ENVOY_STREAM_LOG(debug, "upstream reset: reset reason {}", *callbacks_,
                    Http::Utility::resetReasonToString(reset_reason));
 
+  // If gRPC request is finished without grpc-status code, use the one from the
+  // response headers.
+  if (grpc_request_ && !grpc_status_code_received_) {
+    upstream_request.upstream_host_->outlierDetector().putHttpResponseCode(http_status_code_);
+  }
+
   // TODO: The reset may also come from upstream over the wire. In this case it should be
   // treated as external origin error and distinguished from local origin error.
   // This matters only when running OutlierDetection with split_external_local_origin_errors config
@@ -997,13 +1003,34 @@ void Filter::resetOtherUpstreams(UpstreamRequest& upstream_request) {
   final_upstream_request->moveIntoList(std::move(final_upstream_request), upstream_requests_);
 }
 
+void Filter::maybeSetGrpcStatusToOutlierDetection(
+    UpstreamRequest& upstream_request, absl::optional<Grpc::Status::GrpcStatus> grpc_status) {
+  if (grpc_status) {
+    grpc_status_code_received_ = true;
+    if (Http::CodeUtility::is5xx(Grpc::Utility::grpcToHttpStatus(grpc_status.value()))) {
+      // For gRPC request, use the grpc status code for outlier detector when the mapped
+      // http code is 5xx.
+      upstream_request.upstream_host_->outlierDetector().putHttpResponseCode(
+          Grpc::Utility::grpcToHttpStatus(grpc_status.value()));
+    } else {
+      upstream_request.upstream_host_->outlierDetector().putHttpResponseCode(http_status_code_);
+    }
+  }
+}
+
 void Filter::onUpstreamHeaders(uint64_t response_code, Http::HeaderMapPtr&& headers,
                                UpstreamRequest& upstream_request, bool end_stream) {
   ENVOY_STREAM_LOG(debug, "upstream headers complete: end_stream={}", *callbacks_, end_stream);
 
   modify_headers_(*headers);
 
-  upstream_request.upstream_host_->outlierDetector().putHttpResponseCode(response_code);
+  if (!grpc_request_) {
+    upstream_request.upstream_host_->outlierDetector().putHttpResponseCode(response_code);
+  } else {
+    http_status_code_ = response_code;
+    absl::optional<Grpc::Status::GrpcStatus> grpc_status = Grpc::Common::getGrpcStatus(*headers);
+    maybeSetGrpcStatusToOutlierDetection(upstream_request, grpc_status);
+  }
 
   if (headers->EnvoyImmediateHealthCheckFail() != nullptr) {
     upstream_request.upstream_host_->healthChecker().setUnhealthy();
@@ -1148,6 +1175,11 @@ void Filter::onUpstreamTrailers(Http::HeaderMapPtr&& trailers, UpstreamRequest&
     }
   }
 
+  if (grpc_request_) {
+    absl::optional<Grpc::Status::GrpcStatus> grpc_status = Grpc::Common::getGrpcStatus(*trailers);
+    maybeSetGrpcStatusToOutlierDetection(upstream_request, grpc_status);
+  }
+
   onUpstreamComplete(upstream_request);
 
   callbacks_->encodeTrailers(std::move(trailers));
@@ -1161,6 +1193,13 @@ void Filter::onUpstreamComplete(UpstreamRequest& upstream_request) {
   if (!downstream_end_stream_) {
     upstream_request.resetStream();
   }
+
+  // If gRPC request is finished without grpc-status code, use the one from the
+  // response headers.
+  if (grpc_request_ && !grpc_status_code_received_) {
+    upstream_request.upstream_host_->outlierDetector().putHttpResponseCode(http_status_code_);
+  }
+
   callbacks_->streamInfo().setUpstreamTiming(final_upstream_request_->upstream_timing_);
 
   if (config_.emit_dynamic_stats_ && !callbacks_->streamInfo().healthCheck() &&

source/common/router/router.h

@@ -233,7 +233,8 @@ class Filter : Logger::Loggable<Logger::Id::router>,
   Filter(FilterConfig& config)
       : config_(config), final_upstream_request_(nullptr), downstream_response_started_(false),
         downstream_end_stream_(false), do_shadowing_(false), is_retry_(false),
-        attempting_internal_redirect_with_complete_stream_(false) {}
+        attempting_internal_redirect_with_complete_stream_(false),
+        grpc_status_code_received_(false) {}
 
   ~Filter() override;
 
@@ -512,6 +513,8 @@ class Filter : Logger::Loggable<Logger::Id::router>,
   // downstream if appropriate.
   void onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_flag,
                        absl::string_view body, bool dropped, absl::string_view details);
+  void maybeSetGrpcStatusToOutlierDetection(UpstreamRequest& upstream_request,
+                                            absl::optional<Grpc::Status::GrpcStatus> grpc_status);
   void onUpstreamHeaders(uint64_t response_code, Http::HeaderMapPtr&& headers,
                          UpstreamRequest& upstream_request, bool end_stream);
   void onUpstreamData(Buffer::Instance& data, UpstreamRequest& upstream_request, bool end_stream);
@@ -572,6 +575,8 @@ class Filter : Logger::Loggable<Logger::Id::router>,
   bool is_retry_ : 1;
   bool include_attempt_count_ : 1;
   bool attempting_internal_redirect_with_complete_stream_ : 1;
+  bool grpc_status_code_received_ : 1;
+  uint64_t http_status_code_{200};
   uint32_t attempt_count_{1};
   uint32_t pending_retries_{0};
 };

test/common/router/router_test.cc

@@ -1199,6 +1199,31 @@ TEST_F(RouterTest, GrpcAlreadyExistsTrailersOnly) {
   EXPECT_TRUE(verifyHostUpstreamStats(1, 0));
 }
 
+// Validate outlier detections records failure when gRPC response status is Unavailable.
+TEST_F(RouterTest, GrpcOutlierDetectionUnavailableStatusCode) {
+  NiceMock<Http::MockStreamEncoder> encoder1;
+  Http::StreamDecoder* response_decoder = nullptr;
+  EXPECT_CALL(cm_.conn_pool_, newStream(_, _))
+      .WillOnce(Invoke([&](Http::StreamDecoder& decoder, Http::ConnectionPool::Callbacks& callbacks)
+                           -> Http::ConnectionPool::Cancellable* {
+        response_decoder = &decoder;
+        callbacks.onPoolReady(encoder1, cm_.conn_pool_.host_);
+        return nullptr;
+      }));
+  expectResponseTimerCreate();
+
+  Http::TestHeaderMapImpl headers{{"content-type", "application/grpc"}, {"grpc-timeout", "20S"}};
+  HttpTestUtility::addDefaultHeaders(headers);
+  router_.decodeHeaders(headers, true);
+
+  Http::HeaderMapPtr response_headers(
+      new Http::TestHeaderMapImpl{{":status", "200"}, {"grpc-status", "14"}});
+  // Outlier detector will use the gRPC response status code.
+  EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(503));
+  response_decoder->decodeHeaders(std::move(response_headers), true);
+  EXPECT_TRUE(verifyHostUpstreamStats(0, 1));
+}
+
 // Validate gRPC Internal response stats are sane when response is trailers only.
 TEST_F(RouterTest, GrpcInternalTrailersOnly) {
   NiceMock<Http::MockStreamEncoder> encoder1;
@@ -1218,7 +1243,7 @@ TEST_F(RouterTest, GrpcInternalTrailersOnly) {
 
   Http::HeaderMapPtr response_headers(
       new Http::TestHeaderMapImpl{{":status", "200"}, {"grpc-status", "13"}});
-  EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200));
+  EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(500));
   response_decoder->decodeHeaders(std::move(response_headers), true);
   EXPECT_TRUE(verifyHostUpstreamStats(0, 1));
 }
@@ -1326,10 +1351,10 @@ TEST_F(RouterTest, GrpcInternal) {
   router_.decodeHeaders(headers, true);
 
   Http::HeaderMapPtr response_headers(new Http::TestHeaderMapImpl{{":status", "200"}});
-  EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200));
   response_decoder->decodeHeaders(std::move(response_headers), false);
   EXPECT_TRUE(verifyHostUpstreamStats(0, 0));
   Http::HeaderMapPtr response_trailers(new Http::TestHeaderMapImpl{{"grpc-status", "13"}});
+  EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(500));
   response_decoder->decodeTrailers(std::move(response_trailers));
   EXPECT_TRUE(verifyHostUpstreamStats(0, 1));
 }