Add defaulting for all GRPC probe types (#14773)

* Add defaulting for all GRPC probe types

* Add an E2E readiness test with GRPC probes

* Rename function

pkg/apis/serving/v1/revision_defaults.go

@@ -132,7 +132,8 @@ func (rs *RevisionSpec) applyDefault(ctx context.Context, container *corev1.Cont
 	// If there are multiple containers then default probes will be applied to the container where user specified PORT
 	// default probes will not be applied for non serving containers
 	if len(rs.PodSpec.Containers) == 1 || len(container.Ports) != 0 {
-		rs.applyProbes(container)
+		rs.applyProbesWithDefaults(container)
+		rs.applyGRPCProbeDefaults(container)
 	}
 
 	if rs.PodSpec.EnableServiceLinks == nil && apis.IsInCreate(ctx) {
@@ -153,7 +154,7 @@ func (rs *RevisionSpec) applyDefault(ctx context.Context, container *corev1.Cont
 	}
 }
 
-func (*RevisionSpec) applyProbes(container *corev1.Container) {
+func (*RevisionSpec) applyProbesWithDefaults(container *corev1.Container) {
 	if container.ReadinessProbe == nil {
 		container.ReadinessProbe = &corev1.Probe{}
 	}
@@ -164,10 +165,6 @@ func (*RevisionSpec) applyProbes(container *corev1.Container) {
 		container.ReadinessProbe.TCPSocket = &corev1.TCPSocketAction{}
 	}
 
-	if container.ReadinessProbe.GRPC != nil && container.ReadinessProbe.GRPC.Service == nil {
-		container.ReadinessProbe.GRPC.Service = ptr.String("")
-	}
-
 	if container.ReadinessProbe.SuccessThreshold == 0 {
 		container.ReadinessProbe.SuccessThreshold = 1
 	}
@@ -183,6 +180,18 @@ func (*RevisionSpec) applyProbes(container *corev1.Container) {
 	}
 }
 
+func (*RevisionSpec) applyGRPCProbeDefaults(container *corev1.Container) {
+	if container.ReadinessProbe != nil && container.ReadinessProbe.GRPC != nil && container.ReadinessProbe.GRPC.Service == nil {
+		container.ReadinessProbe.GRPC.Service = ptr.String("")
+	}
+	if container.LivenessProbe != nil && container.LivenessProbe.GRPC != nil && container.LivenessProbe.GRPC.Service == nil {
+		container.LivenessProbe.GRPC.Service = ptr.String("")
+	}
+	if container.StartupProbe != nil && container.StartupProbe.GRPC != nil && container.StartupProbe.GRPC.Service == nil {
+		container.StartupProbe.GRPC.Service = ptr.String("")
+	}
+}
+
 // Upgrade SecurityContext for this container and the Pod definition to use settings
 // for the `restricted` profile when the feature flag is enabled.
 // This does not currently set `runAsNonRoot` for the restricted profile, because

pkg/queue/health/probe_test.go

@@ -35,7 +35,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	netheader "knative.dev/networking/pkg/http/header"
-	"knative.dev/pkg/ptr"
 )
 
 func TestTCPProbe(t *testing.T) {
@@ -270,7 +269,7 @@ func TestHTTPProbeResponseErrorFailure(t *testing.T) {
 	}
 }
 
-func TestGRPCProbeSuccessWithDefaultServiceName(t *testing.T) {
+func TestGRPCProbeSuccess(t *testing.T) {
 	// use ephemeral port to prevent port conflict
 	lis, err := net.Listen("tcp", "127.0.0.1:0")
 	if err != nil {
@@ -286,7 +285,7 @@ func TestGRPCProbeSuccessWithDefaultServiceName(t *testing.T) {
 	}()
 
 	assignedPort := lis.Addr().(*net.TCPAddr).Port
-	gRPCAction := newGRPCAction(t, assignedPort, "")
+	gRPCAction := newGRPCAction(t, assignedPort)
 	config := GRPCProbeConfigOptions{
 		Timeout:    time.Second,
 		GRPCAction: gRPCAction,
@@ -342,12 +341,11 @@ func newHTTPGetAction(t *testing.T, serverURL string) *corev1.HTTPGetAction {
 	}
 }
 
-func newGRPCAction(t *testing.T, port int, service string) *corev1.GRPCAction {
+func newGRPCAction(t *testing.T, port int) *corev1.GRPCAction {
 	t.Helper()
 
 	return &corev1.GRPCAction{
-		Port:    int32(port),
-		Service: ptr.String(service),
+		Port: int32(port),
 	}
 }
 

pkg/reconciler/revision/resources/deploy_test.go

@@ -350,8 +350,7 @@ func withGRPCReadinessProbe(port int) *corev1.Probe {
 	return &corev1.Probe{
 		ProbeHandler: corev1.ProbeHandler{
 			GRPC: &corev1.GRPCAction{
-				Port:    int32(port),
-				Service: nil,
+				Port: int32(port),
 			},
 		}}
 }

test/e2e/readiness_test.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 	pkgTest "knative.dev/pkg/test"
 	"knative.dev/pkg/test/spoof"
+	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	v1opts "knative.dev/serving/pkg/testing/v1"
 	"knative.dev/serving/test"
 	v1test "knative.dev/serving/test/v1"
@@ -121,5 +122,47 @@ func TestReadinessPathAndQuery(t *testing.T) {
 	); err != nil {
 		t.Fatalf("The endpoint %s for Route %s didn't serve the expected text %q: %v", url, names.Route, test.HelloWorldText, err)
 	}
+}
+
+func TestReadinessGRPCProbe(t *testing.T) {
+	t.Parallel()
+
+	clients := Setup(t)
+
+	names := test.ResourceNames{
+		Service: test.ObjectNameForTest(t),
+		Image:   test.GRPCPing,
+	}
+
+	test.EnsureTearDown(t, clients, &names)
+
+	t.Log("Creating a new Service")
+
+	resources, err := v1test.CreateServiceReady(t, clients, &names,
+		v1opts.WithNamedPort("h2c"),
+		v1opts.WithReadinessProbe(
+			&corev1.Probe{
+				ProbeHandler: corev1.ProbeHandler{
+					GRPC: &corev1.GRPCAction{
+						Port: v1.DefaultUserPort,
+					},
+				},
+			}))
+	if err != nil {
+		t.Fatalf("Failed to create initial Service: %v: %v", names.Service, err)
+	}
 
+	url := resources.Route.Status.URL.URL()
+	if _, err := pkgTest.CheckEndpointState(
+		context.Background(),
+		clients.KubeClient,
+		t.Logf,
+		url,
+		spoof.IsStatusOK,
+		"gRPCProbeReady",
+		test.ServingFlags.ResolvableDomain,
+		test.AddRootCAtoTransport(context.Background(), t.Logf, clients, test.ServingFlags.HTTPS),
+	); err != nil {
+		t.Fatalf("The endpoint %s for Route %s didn't return success: %v", url, names.Route, err)
+	}
 }

test/test_images/grpc-ping/main.go

@@ -27,6 +27,8 @@ import (
 	"time"
 
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/health"
+	"google.golang.org/grpc/health/grpc_health_v1"
 
 	"knative.dev/pkg/network"
 	ping "knative.dev/serving/test/test_images/grpc-ping/proto"
@@ -92,6 +94,7 @@ func main() {
 	}
 
 	g := grpc.NewServer()
+	grpc_health_v1.RegisterHealthServer(g, health.NewServer())
 	ping.RegisterPingServiceServer(g, &server{})
 
 	handler := func(w http.ResponseWriter, r *http.Request) {