Probe port different than container port

pkg/apis/serving/k8s_validation.go

@@ -779,16 +779,16 @@ func validateProbe(p *corev1.Probe, port corev1.ContainerPort) *apis.FieldError
 		handlers = append(handlers, "httpGet")
 		errs = errs.Also(apis.CheckDisallowedFields(*h.HTTPGet, *HTTPGetActionMask(h.HTTPGet))).ViaField("httpGet")
 		getPort := h.HTTPGet.Port
-		if (getPort.StrVal != "" && getPort.StrVal != port.Name) || (getPort.IntVal != 0 && getPort.IntVal != port.ContainerPort) {
-			errs = errs.Also(apis.ErrInvalidValue(getPort.String(), "httpGet.port", "May only probe containerPort"))
+		if getPort.StrVal != "" && getPort.StrVal != port.Name {
+			errs = errs.Also(apis.ErrInvalidValue(getPort.String(), "httpGet.port", "Probe port must match container port"))
 		}
 	}
 	if h.TCPSocket != nil {
 		handlers = append(handlers, "tcpSocket")
 		errs = errs.Also(apis.CheckDisallowedFields(*h.TCPSocket, *TCPSocketActionMask(h.TCPSocket))).ViaField("tcpSocket")
 		tcpPort := h.TCPSocket.Port
-		if (tcpPort.StrVal != "" && tcpPort.StrVal != port.Name) || (tcpPort.IntVal != 0 && tcpPort.IntVal != port.ContainerPort) {
-			errs = errs.Also(apis.ErrInvalidValue(tcpPort.String(), "tcpSocket.port", "May only probe containerPort"))
+		if tcpPort.StrVal != "" && tcpPort.StrVal != port.Name {
+			errs = errs.Also(apis.ErrInvalidValue(tcpPort.String(), "tcpSocket.port", "Probe port must match container port"))
 		}
 	}
 	if h.Exec != nil {

pkg/apis/serving/k8s_validation_test.go

@@ -1498,7 +1498,7 @@ func TestContainerValidation(t *testing.T) {
 			},
 			want: apis.ErrMultipleOneOf("readinessProbe.exec", "readinessProbe.tcpSocket", "readinessProbe.httpGet"),
 		}, {
-			name: "invalid readiness http probe (has wrong port)",
+			name: "valid readiness http probe with a different container port",
 			c: corev1.Container{
 				Image: "foo",
 				ReadinessProbe: &corev1.Probe{
@@ -1514,7 +1514,22 @@ func TestContainerValidation(t *testing.T) {
 					},
 				},
 			},
-			want: apis.ErrInvalidValue(5000, "readinessProbe.httpGet.port", "May only probe containerPort"),
+		}, {
+			name: "valid readiness tcp probe with a different container port",
+			c: corev1.Container{
+				Image: "foo",
+				ReadinessProbe: &corev1.Probe{
+					PeriodSeconds:    1,
+					TimeoutSeconds:   1,
+					SuccessThreshold: 1,
+					FailureThreshold: 3,
+					Handler: corev1.Handler{
+						TCPSocket: &corev1.TCPSocketAction{
+							Port: intstr.FromInt(5000),
+						},
+					},
+				},
+			},
 		}, {
 			name: "valid readiness http probe with port",
 			c: corev1.Container{
@@ -1614,7 +1629,7 @@ func TestContainerValidation(t *testing.T) {
 					},
 				},
 			},
-			want: apis.ErrInvalidValue("imap", "livenessProbe.tcpSocket.port", "May only probe containerPort"),
+			want: apis.ErrInvalidValue("imap", "livenessProbe.tcpSocket.port", "Probe port must match container port"),
 		}, {
 			name: "valid liveness tcp probe with correct port",
 			c: corev1.Container{
@@ -1632,6 +1647,22 @@ func TestContainerValidation(t *testing.T) {
 					},
 				},
 			},
+		}, {
+			name: "valid liveness tcp probe on a different container port",
+			c: corev1.Container{
+				Image: "foo",
+				LivenessProbe: &corev1.Probe{
+					PeriodSeconds:    1,
+					TimeoutSeconds:   1,
+					SuccessThreshold: 1,
+					FailureThreshold: 3,
+					Handler: corev1.Handler{
+						TCPSocket: &corev1.TCPSocketAction{
+							Port: intstr.FromInt(5000),
+						},
+					},
+				},
+			},
 		}, {
 			name: "disallowed container fields",
 			c: corev1.Container{

pkg/reconciler/revision/resources/deploy_test.go

@@ -492,7 +492,7 @@ func TestMakePodSpec(t *testing.T) {
 				Ports: []corev1.ContainerPort{{
 					ContainerPort: 8888,
 				}},
-				ReadinessProbe: withTCPReadinessProbe(v1.DefaultUserPort),
+				ReadinessProbe: withTCPReadinessProbe(8888),
 			}}),
 			WithContainerStatuses([]v1.ContainerStatus{{
 				ImageDigest: "busybox@sha256:deadbeef",
@@ -524,7 +524,7 @@ func TestMakePodSpec(t *testing.T) {
 					Name:      "asdf",
 					MountPath: "/asdf",
 				}},
-				ReadinessProbe: withTCPReadinessProbe(v1.DefaultUserPort),
+				ReadinessProbe: withTCPReadinessProbe(8888),
 			}}),
 			WithContainerStatuses([]v1.ContainerStatus{{
 				ImageDigest: "busybox@sha256:deadbeef",
@@ -752,7 +752,7 @@ func TestMakePodSpec(t *testing.T) {
 					container.Image = "busybox@sha256:deadbeef"
 				}),
 				queueContainer(
-					withEnvVar("SERVING_READINESS_PROBE", `{"tcpSocket":{"port":8080,"host":"127.0.0.1"}}`),
+					withEnvVar("SERVING_READINESS_PROBE", `{"tcpSocket":{"port":12345,"host":"127.0.0.1"}}`),
 				),
 			}),
 	}, {
@@ -952,7 +952,7 @@ func TestMakePodSpec(t *testing.T) {
 				Ports: []corev1.ContainerPort{{
 					ContainerPort: 8888,
 				}},
-				ReadinessProbe: withTCPReadinessProbe(v1.DefaultUserPort),
+				ReadinessProbe: withTCPReadinessProbe(8888),
 			}, {
 				Name:  sidecarContainerName,
 				Image: "ubuntu",

pkg/reconciler/revision/resources/queue.go

@@ -213,12 +213,20 @@ func makeQueueContainer(rev *v1.Revision, cfg *config.Config) (*corev1.Container
 	var httpProbe, execProbe *corev1.Probe
 	var userProbeJSON string
 	if container.ReadinessProbe != nil {
+		probePort := userPort
+		if container.ReadinessProbe.HTTPGet != nil && container.ReadinessProbe.HTTPGet.Port.IntValue() != 0 {
+			probePort = container.ReadinessProbe.HTTPGet.Port.IntVal
+		}
+		if container.ReadinessProbe.TCPSocket != nil && container.ReadinessProbe.TCPSocket.Port.IntValue() != 0 {
+			probePort = container.ReadinessProbe.TCPSocket.Port.IntVal
+		}
+
 		// The activator attempts to detect readiness itself by checking the Queue
 		// Proxy's health endpoint rather than waiting for Kubernetes to check and
 		// propagate the Ready state. We encode the original probe as JSON in an
 		// environment variable for this health endpoint to use.
 		userProbe := container.ReadinessProbe.DeepCopy()
-		applyReadinessProbeDefaultsForExec(userProbe, userPort)
+		applyReadinessProbeDefaultsForExec(userProbe, probePort)
 
 		var err error
 		userProbeJSON, err = readiness.EncodeProbe(userProbe)

pkg/reconciler/revision/resources/queue_test.go

@@ -101,6 +101,36 @@ func TestMakeQueueContainer(t *testing.T) {
 				"CONTAINER_CONCURRENCY": "1",
 			})
 		}),
+	}, {
+		name: "custom readiness probe port",
+		rev: revision("bar", "foo",
+			withContainers([]corev1.Container{{
+				Name: servingContainerName,
+				ReadinessProbe: &corev1.Probe{
+					Handler: corev1.Handler{
+						TCPSocket: &corev1.TCPSocketAction{
+							Host: "127.0.0.1",
+							Port: intstr.FromInt(8087),
+						},
+					},
+				},
+				Ports: []corev1.ContainerPort{{
+					ContainerPort: 1955,
+					Name:          string(networking.ProtocolH2C),
+				}},
+			}})),
+		dc: deployment.Config{
+			QueueSidecarImage: "alpine",
+		},
+		want: queueContainer(func(c *corev1.Container) {
+			c.Image = "alpine"
+			c.Ports = append(queueNonServingPorts, queueHTTP2Port)
+			c.ReadinessProbe.Handler.HTTPGet.Port.IntVal = queueHTTP2Port.ContainerPort
+			c.Env = env(map[string]string{
+				"USER_PORT":          "1955",
+				"QUEUE_SERVING_PORT": "8013",
+			})
+		}),
 	}, {
 		name: "custom sidecar image, container port, protocol",
 		rev: revision("bar", "foo",
@@ -895,7 +925,12 @@ func probeJSON(container *corev1.Container) string {
 	if container == nil {
 		return fmt.Sprintf(testProbeJSONTemplate, v1.DefaultUserPort)
 	}
-
+	if container.ReadinessProbe.TCPSocket != nil && container.ReadinessProbe.TCPSocket.Port.IntValue() != 0 {
+		return fmt.Sprintf(testProbeJSONTemplate, container.ReadinessProbe.TCPSocket.Port.IntVal)
+	}
+	if container.ReadinessProbe.HTTPGet != nil && container.ReadinessProbe.HTTPGet.Port.IntValue() != 0 {
+		return fmt.Sprintf(testProbeJSONTemplate, container.ReadinessProbe.HTTPGet.Port.IntVal)
+	}
 	if ports := container.Ports; len(ports) > 0 && ports[0].ContainerPort != 0 {
 		return fmt.Sprintf(testProbeJSONTemplate, ports[0].ContainerPort)
 	}

test/conformance.go

@@ -48,6 +48,7 @@ const (
 	PizzaPlanet2        = "pizzaplanetv2"
 	Protocols           = "protocols"
 	Readiness           = "readiness"
+	ReadinessPort       = "readinessport"
 	Runtime             = "runtime"
 	ServingContainer    = "servingcontainer"
 	SidecarContainer    = "sidecarcontainer"

test/conformance/runtime/container_test.go

@@ -24,7 +24,6 @@ import (
 
 	"github.com/davecgh/go-spew/spew"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/util/intstr"
 	"knative.dev/pkg/ptr"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	"knative.dev/serving/test"
@@ -57,48 +56,6 @@ func TestMustNotContainerConstraints(t *testing.T) {
 				MountPropagation: &propagationMode,
 			}}
 		},
-	}, {
-		name: "TestReadinessHTTPProbePort",
-		options: func(s *v1.Service) {
-			s.Spec.Template.Spec.Containers[0].ReadinessProbe = &corev1.Probe{
-				Handler: corev1.Handler{
-					HTTPGet: &corev1.HTTPGetAction{
-						Path: "/",
-						Port: intstr.FromInt(8888),
-					},
-				},
-			}
-		},
-	}, {
-		name: "TestLivenessHTTPProbePort",
-		options: func(s *v1.Service) {
-			s.Spec.Template.Spec.Containers[0].LivenessProbe = &corev1.Probe{
-				Handler: corev1.Handler{
-					HTTPGet: &corev1.HTTPGetAction{
-						Path: "/",
-						Port: intstr.FromInt(8888),
-					},
-				},
-			}
-		},
-	}, {
-		name: "TestReadinessTCPProbePort",
-		options: func(s *v1.Service) {
-			s.Spec.Template.Spec.Containers[0].ReadinessProbe = &corev1.Probe{
-				Handler: corev1.Handler{
-					TCPSocket: &corev1.TCPSocketAction{Port: intstr.FromInt(8888)},
-				},
-			}
-		},
-	}, {
-		name: "TestLivenessTCPProbePort",
-		options: func(s *v1.Service) {
-			s.Spec.Template.Spec.Containers[0].LivenessProbe = &corev1.Probe{
-				Handler: corev1.Handler{
-					TCPSocket: &corev1.TCPSocketAction{Port: intstr.FromInt(8888)},
-				},
-			}
-		},
 	}}
 
 	for _, tc := range testCases {

test/e2e/readinessport_test.go

@@ -0,0 +1,82 @@
+//go:build e2e
+// +build e2e
+
+/*
+Copyright 2021 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package e2e
+
+import (
+	"context"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+	pkgTest "knative.dev/pkg/test"
+	"knative.dev/pkg/test/spoof"
+	v1opts "knative.dev/serving/pkg/testing/v1"
+	"knative.dev/serving/test"
+	v1test "knative.dev/serving/test/v1"
+)
+
+func TestReadinessAlternatePort(t *testing.T) {
+	t.Parallel()
+
+	clients := Setup(t)
+
+	names := test.ResourceNames{
+		Service: test.ObjectNameForTest(t),
+		Image:   test.Readiness,
+	}
+
+	test.EnsureTearDown(t, clients, &names)
+
+	t.Log("Creating a new Service")
+
+	resources, err := v1test.CreateServiceReady(t, clients, &names,
+		v1opts.WithEnv(corev1.EnvVar{
+			Name:  "HEALTHCHECK_PORT",
+			Value: "8077",
+		}),
+		v1opts.WithReadinessProbe(
+			&corev1.Probe{
+				Handler: corev1.Handler{
+					HTTPGet: &corev1.HTTPGetAction{
+						Scheme: "http",
+						Port:   intstr.FromInt(8077),
+						Path:   "/",
+					},
+				},
+			}))
+	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.MatchesAllOf(spoof.IsStatusOK, spoof.MatchesBody(test.HelloWorldText)),
+		"HelloWorldServesText",
+		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 serve the expected text %q: %v", url, names.Route, test.HelloWorldText, err)
+	}
+
+}

test/test_images/readiness/README.md

@@ -3,8 +3,8 @@
 This directory contains the test image used to simulate an image that responds
 to various types of readiness probe.
 
-The image provides a /healthz endpoint which will reply with a 200 status code
-and the Hello World text only after the delay requested by the STARTUP_DELAY
+The image provides an endpoint which will reply with a 200 status code
+and the Hello World text only after the delay requested by the READY_DELAY
 environment variable has elapsed.
 
 The image also contains an exec probe when run with "probe" as its only argument.