Add retriable_serving_statuses to grpc health check to support retrying NOT_SERVING

api/bazel/external_proto_deps.bzl

@@ -13,6 +13,7 @@ EXTERNAL_PROTO_IMPORT_BAZEL_DEP_MAP = {
     "google/api/expr/v1alpha1/syntax.proto": "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto",
     "io/prometheus/client/metrics.proto": "@prometheus_metrics_model//:client_model",
     "opentelemetry/proto/common/v1/common.proto": "@opentelemetry-proto//:common_proto",
+    "grpc/health/v1/health.proto": "@com_github_grpc_grpc//src/proto/grpc/health/v1:health_proto",
 }
 
 # This maps from the Bazel proto_library target to the Go language binding target for external dependencies.
@@ -35,6 +36,7 @@ EXTERNAL_PROTO_GO_BAZEL_DEP_MAP = {
     "@opentelemetry-proto//:metrics_proto": "@opentelemetry-proto//:metrics_proto_go",
     "@opentelemetry-proto//:metrics_service_proto": "@opentelemetry-proto//:metrics_service_grpc_go",
     "@opentelemetry-proto//:common_proto": "@opentelemetry-proto//:common_proto_go",
+    "@com_github_grpc_grpc//src/proto/grpc/health/v1:health_proto": "@org_golang_google_grpc//health/grpc_health_v1:go_default_library",
 }
 
 # This maps from the Bazel proto_library target to the C++ language binding target for external dependencies.
@@ -48,4 +50,5 @@ EXTERNAL_PROTO_CC_BAZEL_DEP_MAP = {
     "@opentelemetry-proto//:metrics_proto": "@opentelemetry-proto//:metrics_proto_cc",
     "@opentelemetry-proto//:metrics_service_proto": "@opentelemetry-proto//:metrics_service_grpc_cc",
     "@opentelemetry-proto//:common_proto": "@opentelemetry-proto//:common_proto_cc",
+    "@com_github_grpc_grpc//src/proto/grpc/health/v1:health_proto": "@com_github_grpc_grpc//src/proto/grpc/health/v1:health_cc_proto",
 }

api/buf.lock

@@ -2,20 +2,26 @@
 version: v2
 deps:
   - name: buf.build/cncf/xds
-    commit: c313df85559e44248d0115969f2c8c24
-    digest: b5:89db966d59cc9686658a12f40ba0bb38017d0bccf3e651d025ff6747d18f1a647814e0bd6dd534218d08fe949be33371ef506a1ac8a32c4978b6f9a288231bde
+    commit: 2456e1dfc0664e7596d7f3b20f4b62d0
+    digest: b5:412ce915a722e7e653426107531cfbfea5a263afd438b2778c03f1dc8f75628e5baf791e513324e5ccff0197be2f91aa174629b8353966d38c295bd86bc2cebb
   - name: buf.build/envoyproxy/protoc-gen-validate
-    commit: 6607b10f00ed4a3d98f906807131c44a
-    digest: b5:ade405ac4328bd0a2cf83c93bcb4bc389d4897afd86a0096df4537b180916882da4e4f0c2d45f0b1554d7a6c87f6c5bc94b71b3555ca71cc31a9a8baed26a9f9
+    commit: daf171c6cdb54629b5f51e345a79e4dd
+    digest: b5:c745e1521879f43740230b1df673d0729f55704efefdcfc489d4a0a2d40c92a26cacfeab62813403040a8b180142d53b398c7ca784a065e43823605ee49681de
   - name: buf.build/gogo/protobuf
-    commit: 5461a3dfa9d941da82028ab185dc2a0e
-    digest: b5:8e9cf66be25ae4fac7fe0867742f41d008491cf1e0f49c488b42ce2fadb663970d4179472f8835dd58a61448464ba0ad9c2d19f464553f86ed137d9f59ec5dff
+    commit: e1dbca2775a74a89955a99990de45a53
+    digest: b5:9be593f336e8b0d35761a2fb5ebc4460aaee3f2d21e9d75b7fddbecb1dd53ea58a0bb447c20c33387f4e07fa7e1e439f7125663a624517cfde97c8bec68e4ecf
+  - name: buf.build/google/cel-spec
+    commit: 96eff7bcf453468dbd44b80598bfe1bf
+    digest: b5:660c244ec1e8a0f26c5431e1901f1d52d322c7ee3fd240cb6832f44d0694384a36ded008af457a9e5268c73adfa00170e972ac4fe44752a693424b3479f30b82
   - name: buf.build/googleapis/googleapis
     commit: 62f35d8aed1149c291d606d958a7ce32
     digest: b5:d66bf04adc77a0870bdc9328aaf887c7188a36fb02b83a480dc45ef9dc031b4d39fc6e9dc6435120ccf4fe5bfd5c6cb6592533c6c316595571f9a31420ab47fe
+  - name: buf.build/grpc/grpc
+    commit: e126be52bace42e49491e4015e464b59
+    digest: b5:a01d4326f3e6590547c7cbb3bff8d6dab5cf564eeadd7c7c0d32ea739931a1d492322e6aa21312c9f419538044beea6740f9a6c51bb44647c627044936c55f36
   - name: buf.build/opentelemetry/opentelemetry
-    commit: 43554dfbbfbd4873bde8993d32ea8332
-    digest: b5:027a738207c1e66e1b628e49923dade94b8832049537f0a05375b086122ccc5db4351f86e4297ac0fe441b4351c901ada85e03e7f34a09a401e46e00f8c8062b
+    commit: 648a3e2f02e14fe187656ea4ac3befa1
+    digest: b5:a0514be587ab2e8598f7102dfdaba5e985138b8c041c707e470a9c8b877410ada6e60cf2359352302f08c0504de685379f7f7a085d4699d69a2e6ed7035e78d9
   - name: buf.build/prometheus/client-model
-    commit: 1d56a02d481a412a83b3c4984eb90c2e
-    digest: b5:030705921923d3a04902cb71f946d39db4ca6bb03d1da7fcb381b1f907b693572519f73fa9f9a0994d61cedb8935989675aca0c222e05e7c8790c808b2a14e47
+    commit: e171c0b235c546d5a9a597c2961bd357
+    digest: b5:c26899a8592079d1f0cc168dee57c70df022d07c450ad0560fac8a1c6b5b9f73cbc6c44a21a67afbeb32e1e2127cf02c4e7f34368c8685a33b56bd936ea4273e

api/buf.yaml

@@ -4,6 +4,7 @@ deps:
 - buf.build/envoyproxy/protoc-gen-validate
 - buf.build/gogo/protobuf
 - buf.build/googleapis/googleapis:62f35d8aed1149c291d606d958a7ce32
+- buf.build/grpc/grpc
 - buf.build/opentelemetry/opentelemetry
 - buf.build/prometheus/client-model
 breaking:

api/envoy/config/core/v3/BUILD

@@ -9,6 +9,7 @@ api_proto_package(
         "//envoy/annotations:pkg",
         "//envoy/type/matcher/v3:pkg",
         "//envoy/type/v3:pkg",
+        "@com_github_grpc_grpc//src/proto/grpc/health/v1:health_proto",
         "@xds//udpa/annotations:pkg",
         "@xds//xds/annotations/v3:pkg",
         "@xds//xds/core/v3:pkg",

api/envoy/config/core/v3/health_check.proto

@@ -15,6 +15,8 @@ import "google/protobuf/duration.proto";
 import "google/protobuf/struct.proto";
 import "google/protobuf/wrappers.proto";
 
+import "grpc/health/v1/health.proto";
+
 import "envoy/annotations/deprecation.proto";
 import "udpa/annotations/status.proto";
 import "udpa/annotations/versioning.proto";
@@ -227,6 +229,16 @@ message HealthCheck {
     // see the documentation on :ref:`custom request headers
     // <config_http_conn_man_headers_custom_request_headers>`.
     repeated HeaderValueOption initial_metadata = 3 [(validate.rules).repeated = {max_items: 1000}];
+
+    // Specifies a list of gRPC `grpc.health.v1.HealthCheckResponse.ServingStatus
+    // <https://github.com/grpc/grpc/blob/master/src/proto/grpc/health/v1/health.proto#L33>`_ that will be treated
+    // as retriable health check failures and respect the unhealthy threshold instead of immediately marking the
+    // host unhealthy. SERVING status is not retriable because that is a successful health check.
+    repeated grpc.health.v1.HealthCheckResponse.ServingStatus retriable_serving_statuses = 4
+        [(validate.rules).repeated = {
+          unique: true
+          items {enum {defined_only: true not_in: 1}}
+        }];
   }
 
   // Custom health check.

bazel/grpc.patch

@@ -12,7 +12,7 @@ index 9e067aa29e..089a447ec4 100644
  )
 
 diff --git a/bazel/grpc_build_system.bzl b/bazel/grpc_build_system.bzl
-index a3b8923b6e..f4a2ed3f13 100644
+index a3b8923b6e..87c59a932a 100644
 --- a/bazel/grpc_build_system.bzl
 +++ b/bazel/grpc_build_system.bzl
 @@ -27,7 +27,6 @@
@@ -40,6 +40,20 @@ index a3b8923b6e..f4a2ed3f13 100644
              "//conditions:default": [name + "_native"],
          }),
          outs = [name + "_binary"],
+@@ -217,11 +211,13 @@ def grpc_internal_proto_library(
+         srcs = [],
+         deps = [],
+         visibility = None,
++        strip_import_prefix = None,
+         has_services = False):  # buildifier: disable=unused-variable
+     proto_library(
+         name = name,
+         srcs = srcs,
+         deps = deps,
++        strip_import_prefix = strip_import_prefix,
+         visibility = visibility,
+     )
+
 diff --git a/include/grpc/event_engine/memory_request.h b/include/grpc/event_engine/memory_request.h
 index 76bcbb2036..ad8cab842e 100644
 --- a/include/grpc/event_engine/memory_request.h
@@ -235,6 +249,19 @@ index 3fb5d68146..583a2f9553 100644
      return PromiseFactoryImpl(RepeatableToken{}, f_);
    }
  };
+diff --git a/src/core/load_balancing/health_check_client.cc b/src/core/load_balancing/health_check_client.cc
+index c84f6d0a48..9f8bee2c53 100644
+--- a/src/core/load_balancing/health_check_client.cc
++++ b/src/core/load_balancing/health_check_client.cc
+@@ -57,7 +57,7 @@
+ #include "src/core/util/ref_counted_ptr.h"
+ #include "src/core/util/sync.h"
+ #include "src/core/util/work_serializer.h"
+-#include "src/proto/grpc/health/v1/health.upb.h"
++#include "grpc/health/v1/health.upb.h"
+ #include "upb/base/string_view.h"
+ #include "upb/mem/arena.hpp"
+
 diff --git a/src/core/util/glob.cc b/src/core/util/glob.cc
 index 1b1c16e23d..22e54259f0 100644
 --- a/src/core/util/glob.cc
@@ -248,11 +275,32 @@ index 1b1c16e23d..22e54259f0 100644
  #include "absl/strings/string_view.h"
 
  namespace grpc_core {
+diff --git a/src/cpp/server/health/default_health_check_service.cc b/src/cpp/server/health/default_health_check_service.cc
+index 492ba694c1..1cd2ee17fd 100644
+--- a/src/cpp/server/health/default_health_check_service.cc
++++ b/src/cpp/server/health/default_health_check_service.cc
+@@ -30,7 +30,7 @@
+
+ #include "absl/log/log.h"
+ #include "src/core/util/grpc_check.h"
+-#include "src/proto/grpc/health/v1/health.upb.h"
++#include "grpc/health/v1/health.upb.h"
+ #include "upb/base/string_view.h"
+ #include "upb/mem/arena.hpp"
+
 diff --git a/src/proto/grpc/health/v1/BUILD b/src/proto/grpc/health/v1/BUILD
-index f6165e0392..374653516a 100644
+index f6165e0392..90e86a5c50 100644
 --- a/src/proto/grpc/health/v1/BUILD
 +++ b/src/proto/grpc/health/v1/BUILD
-@@ -44,3 +44,10 @@ filegroup(
+@@ -24,6 +24,7 @@ grpc_package(
+ grpc_internal_proto_library(
+     name = "health_proto",
+     srcs = ["health.proto"],
++    strip_import_prefix = "/src/proto",
+     has_services = True,
+ )
+
+@@ -44,3 +45,10 @@ filegroup(
          "health.proto",
      ],
  )

bazel/pgv.patch

@@ -29,3 +29,133 @@ index 1a41f68..4d95b0d 100644
              executable = True,
              allow_single_file = True,
          ),
+diff --git a/templates/cc/register.go b/templates/cc/register.go
+index e927985..050efda 100644
+--- a/templates/cc/register.go
++++ b/templates/cc/register.go
+@@ -315,7 +315,7 @@ func (fns CCFuncs) inType(f pgs.Field, x interface{}) string {
+ 			return fns.cTypeOfString(fns.Type(f).Value().String())
+ 		}
+
+-		return fns.PackageName(fldEn).String() + "::" + fns.Type(f).Value().String()
++		return fns.packageName(fldEn) + "::" + fns.Type(f).Value().String()
+ 	default:
+ 		return fns.cType(f.Type())
+ 	}
+diff --git a/templates/cc/repeated.go b/templates/cc/repeated.go
+index 53b2a35..e7124f6 100644
+--- a/templates/cc/repeated.go
++++ b/templates/cc/repeated.go
+@@ -26,6 +26,9 @@ const repTpl = `
+ 	{{ end }}
+
+ 	{{ if $r.GetUnique }}
++	{{ if isRepeatedEnum $f }}
++	std::unordered_set<int> {{ lookup $f "Unique" }};
++	{{ else }}
+ 	// Implement comparison for wrapped reference types
+ 	struct cmp {
+ 		bool operator() (const std::reference_wrapper<{{ $typ }}> lhs, const std::reference_wrapper<{{ $typ }}> rhs) const {
+@@ -44,6 +47,7 @@ const repTpl = `
+ 	// Save a set of references to avoid copying overhead
+ 	std::unordered_set<std::reference_wrapper<{{ $typ }}>, hash, cmp> {{ lookup $f "Unique" }};
+ 	{{ end }}
++	{{ end }}
+
+ 	{{ if or $r.GetUnique (ne (.Elem "" "").Typ "none") }}
+ 		for (int i = 0; i < {{ accessor . }}.size(); i++) {
+@@ -51,7 +55,11 @@ const repTpl = `
+ 			(void)item;
+
+ 			{{ if $r.GetUnique }}
++				{{ if isRepeatedEnum $f }}
++				auto p = {{ lookup $f "Unique" }}.emplace(item);
++				{{ else }}
+ 				auto p = {{ lookup $f "Unique" }}.emplace(const_cast<{{ $typ }}&>(item));
++				{{ end }}
+ 				if (p.second == false) {
+ 					{{ errIdx . "i" "repeated value must contain unique items" }}
+ 				}
+diff --git a/templates/goshared/register.go b/templates/goshared/register.go
+index 19a2a00..6a8fac7 100644
+--- a/templates/goshared/register.go
++++ b/templates/goshared/register.go
+@@ -415,3 +415,29 @@ func (fns goSharedFuncs) enumPackages(enums []pgs.Enum) map[pgs.Name]NormalizedE
+ func (fns goSharedFuncs) snakeCase(name string) string {
+ 	return strcase.ToSnake(name)
+ }
++
++func (fns goSharedFuncs) Type(f pgs.Field) pgsgo.TypeName {
++	if f.Type().ProtoType() == pgs.EnumT {
++		ens := fns.enumPackages(fns.externalEnums(f.File()))
++		// Check if the imported name of the enum has collided and been renamed
++		if len(ens) != 0 {
++			enType := f.Type().Enum()
++			if f.Type().IsRepeated() {
++				enType = f.Type().Element().Enum()
++			}
++
++			enImportPath := fns.ImportPath(enType)
++			for pkg, en := range ens {
++				if en.FilePath == enImportPath {
++					name := fns.enumName(enType)
++					if f.Type().IsRepeated() {
++						return pgsgo.TypeName("[]" + pkg.String() + "." + name)
++					}
++					return pgsgo.TypeName(pkg.String() + "." + name)
++				}
++			}
++		}
++	}
++
++	return fns.Context.Type(f)
++}
+diff --git a/templates/shared/enums.go b/templates/shared/enums.go
+index bb5f053..79fcf39 100644
+--- a/templates/shared/enums.go
++++ b/templates/shared/enums.go
+@@ -11,6 +11,10 @@ func isEnum(f pgs.Field) bool {
+ 	return f.Type().IsEnum()
+ }
+
++func isRepeatedEnum(f pgs.Field) bool {
++	return f.Type().IsRepeated() && f.Type().Element().IsEnum()
++}
++
+ func enumNamesMap(values []pgs.EnumValue) (m map[int32]string) {
+ 	m = make(map[int32]string)
+ 	for _, v := range values {
+diff --git a/templates/shared/functions.go b/templates/shared/functions.go
+index eb10045..8ed5cd6 100644
+--- a/templates/shared/functions.go
++++ b/templates/shared/functions.go
+@@ -8,16 +8,17 @@ import (
+
+ func RegisterFunctions(tpl *template.Template, params pgs.Parameters) {
+ 	tpl.Funcs(map[string]interface{}{
+-		"disabled":  Disabled,
+-		"ignored":   Ignored,
+-		"required":  RequiredOneOf,
+-		"context":   rulesContext,
+-		"render":    Render(tpl),
+-		"has":       Has,
+-		"needs":     Needs,
+-		"fileneeds": FileNeeds,
+-		"isEnum":    isEnum,
+-		"enumList":  enumList,
+-		"enumVal":   enumVal,
++		"disabled":       Disabled,
++		"ignored":        Ignored,
++		"required":       RequiredOneOf,
++		"context":        rulesContext,
++		"render":         Render(tpl),
++		"has":            Has,
++		"needs":          Needs,
++		"fileneeds":      FileNeeds,
++		"isEnum":         isEnum,
++		"isRepeatedEnum": isRepeatedEnum,
++		"enumList":       enumList,
++		"enumVal":        enumVal,
+ 	})
+ }
+

source/common/upstream/health_checker_impl.h

@@ -23,7 +23,7 @@
 #include "source/common/stream_info/stream_info_impl.h"
 #include "source/common/upstream/health_checker_event_logger.h"
 
-#include "src/proto/grpc/health/v1/health.pb.h"
+#include "grpc/health/v1/health.pb.h"
 
 namespace Envoy {
 namespace Upstream {

source/extensions/health_checkers/grpc/health_checker_impl.cc

@@ -40,6 +40,16 @@ const std::string& getHostname(const HostSharedPtr& host,
   }
   return HealthCheckerFactory::getHostname(host, EMPTY_STRING, cluster);
 }
+
+absl::flat_hash_set<grpc::health::v1::HealthCheckResponse::ServingStatus>
+buildRetriableStatuses(const envoy::config::core::v3::HealthCheck::GrpcHealthCheck& config) {
+  absl::flat_hash_set<grpc::health::v1::HealthCheckResponse::ServingStatus> statuses;
+  statuses.reserve(config.retriable_serving_statuses_size());
+  for (const auto status : config.retriable_serving_statuses()) {
+    statuses.insert(static_cast<grpc::health::v1::HealthCheckResponse::ServingStatus>(status));
+  }
+  return statuses;
+}
 } // namespace
 
 Upstream::HealthCheckerSharedPtr GrpcHealthCheckerFactory::createCustomHealthChecker(
@@ -64,7 +74,8 @@ GrpcHealthCheckerImpl::GrpcHealthCheckerImpl(const Cluster& cluster,
           "grpc.health.v1.Health.Check")),
       request_headers_parser_(THROW_OR_RETURN_VALUE(
           Router::HeaderParser::configure(config.grpc_health_check().initial_metadata()),
-          Router::HeaderParserPtr)) {
+          Router::HeaderParserPtr)),
+      retriable_statuses_(buildRetriableStatuses(config.grpc_health_check())) {
   if (!config.grpc_health_check().service_name().empty()) {
     service_name_ = config.grpc_health_check().service_name();
   }
@@ -290,27 +301,16 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onGoAway(
   }
 }
 
-bool GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::isHealthCheckSucceeded(
-    Grpc::Status::GrpcStatus grpc_status) const {
-  if (grpc_status != Grpc::Status::WellKnownGrpcStatus::Ok) {
-    return false;
-  }
-
-  if (!health_check_response_ ||
-      health_check_response_->status() != grpc::health::v1::HealthCheckResponse::SERVING) {
-    return false;
-  }
-
-  return true;
-}
-
 void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onRpcComplete(
     Grpc::Status::GrpcStatus grpc_status, const std::string& grpc_message, bool end_stream) {
   logHealthCheckStatus(grpc_status, grpc_message);
-  if (isHealthCheckSucceeded(grpc_status)) {
+  if (grpc_status != Grpc::Status::WellKnownGrpcStatus::Ok || !health_check_response_) {
+    handleFailure(envoy::data::core::v3::ACTIVE);
+  } else if (health_check_response_->status() == grpc::health::v1::HealthCheckResponse::SERVING) {
     handleSuccess(false);
   } else {
-    handleFailure(envoy::data::core::v3::ACTIVE);
+    const bool retriable = parent_.retriable_statuses_.contains(health_check_response_->status());
+    handleFailure(envoy::data::core::v3::ACTIVE, /*retriable=*/retriable);
   }
 
   // Read the value as we may call resetState() and clear it.