Allow probes to explicitly set the port to the containerPort (knative…

…#8288)
evankanderson · Nov 4, 2021 · ba72160 · ba72160
1 parent cad72a3
commit ba72160
Show file tree

Hide file tree

Showing 5 changed files with 130 additions and 54 deletions.
diff --git a/pkg/apis/serving/fieldmask.go b/pkg/apis/serving/fieldmask.go
@@ -366,6 +366,7 @@ func HTTPGetActionMask(in *corev1.HTTPGetAction) *corev1.HTTPGetAction {
 	out.Path = in.Path
 	out.Scheme = in.Scheme
 	out.HTTPHeaders = in.HTTPHeaders
+	out.Port = in.Port
 
 	return out
 }
@@ -381,6 +382,7 @@ func TCPSocketActionMask(in *corev1.TCPSocketAction) *corev1.TCPSocketAction {
 
 	// Allowed fields
 	out.Host = in.Host
+	out.Port = in.Port
 
 	return out
 }

diff --git a/pkg/apis/serving/fieldmask_test.go b/pkg/apis/serving/fieldmask_test.go
@@ -309,6 +309,7 @@ func TestHTTPGetActionMask(t *testing.T) {
 		Path:        "/bar",
 		Scheme:      corev1.URISchemeHTTP,
 		HTTPHeaders: []corev1.HTTPHeader{{}},
+		Port:        intstr.FromInt(8080),
 	}
 	in := &corev1.HTTPGetAction{
 		Host:        "foo",
@@ -338,10 +339,11 @@ func TestHTTPGetActionMask(t *testing.T) {
 func TestTCPSocketActionMask(t *testing.T) {
 	want := &corev1.TCPSocketAction{
 		Host: "foo",
+		Port: intstr.FromString("https"),
 	}
 	in := &corev1.TCPSocketAction{
 		Host: "foo",
-		Port: intstr.FromInt(8080),
+		Port: intstr.FromString("https"),
 	}
 
 	got := TCPSocketActionMask(in)

diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go
@@ -26,6 +26,7 @@ import (
 	"github.com/google/go-containerregistry/pkg/name"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"knative.dev/pkg/apis"
@@ -324,14 +325,19 @@ func ValidatePodSpec(ctx context.Context, ps corev1.PodSpec) *apis.FieldError {
 		errs = errs.Also(err.ViaField("volumes"))
 	}
 
+	err, port := validateContainersPorts(ps.Containers)
+	if err != nil {
+		errs = errs.Also(err.ViaField("containers[*]"))
+	}
+
 	switch len(ps.Containers) {
 	case 0:
 		errs = errs.Also(apis.ErrMissingField("containers"))
 	case 1:
-		errs = errs.Also(ValidateContainer(ctx, ps.Containers[0], volumes).
+		errs = errs.Also(ValidateContainer(ctx, ps.Containers[0], volumes, port).
 			ViaFieldIndex("containers", 0))
 	default:
-		errs = errs.Also(validateContainers(ctx, ps.Containers, volumes))
+		errs = errs.Also(validateContainers(ctx, ps.Containers, volumes, port))
 	}
 	if ps.ServiceAccountName != "" {
 		for _, err := range validation.IsDNS1123Subdomain(ps.ServiceAccountName) {
@@ -356,20 +362,20 @@ func validateInitContainers(ctx context.Context, containers []corev1.Container,
 	return errs
 }
 
-func validateContainers(ctx context.Context, containers []corev1.Container, volumes map[string]corev1.Volume) (errs *apis.FieldError) {
+func validateContainers(ctx context.Context, containers []corev1.Container, volumes map[string]corev1.Volume, port intstr.IntOrString) (errs *apis.FieldError) {
 	features := config.FromContextOrDefaults(ctx).Features
 	if features.MultiContainer != config.Enabled {
 		return errs.Also(&apis.FieldError{Message: fmt.Sprintf("multi-container is off, "+
 			"but found %d containers", len(containers))})
 	}
-	errs = errs.Also(validateContainersPorts(containers).ViaField("containers"))
 	for i := range containers {
 		// Probes are not allowed on other than serving container,
 		// ref: http://bit.ly/probes-condition
 		if len(containers[i].Ports) == 0 {
+			// Note, if we allow readiness/liveness checks on sidecars, we should pass in an *empty* port here, not the main container's port.
 			errs = errs.Also(validateSidecarContainer(WithinSidecarContainer(ctx), containers[i], volumes).ViaFieldIndex("containers", i))
 		} else {
-			errs = errs.Also(ValidateContainer(WithinUserContainer(ctx), containers[i], volumes).ViaFieldIndex("containers", i))
+			errs = errs.Also(ValidateContainer(WithinUserContainer(ctx), containers[i], volumes, port).ViaFieldIndex("containers", i))
 		}
 	}
 	return errs
@@ -386,21 +392,35 @@ func AllMountedVolumes(containers []corev1.Container) sets.String {
 	return volumeNames
 }
 
-// validateContainersPorts validates port when specified multiple containers
-func validateContainersPorts(containers []corev1.Container) *apis.FieldError {
+// validateContainersPorts validates port when specified multiple containers,
+// and returns the single serving port if error is nil
+func validateContainersPorts(containers []corev1.Container) (*apis.FieldError, intstr.IntOrString) {
 	var count int
+	var port = intstr.IntOrString{IntVal: 8080, StrVal: "http"}
 	for i := range containers {
-		count += len(containers[i].Ports)
+		if c := len(containers[i].Ports); c > 0 {
+			count += c
+			if containers[i].Ports[0].ContainerPort != 0 {
+				port.IntVal = containers[i].Ports[0].ContainerPort
+			}
+			if containers[i].Ports[0].Name != "" {
+				port.StrVal = containers[i].Ports[0].Name
+			}
+		}
 	}
 	// When no container ports are specified.
-	if count == 0 {
-		return apis.ErrMissingField("ports")
+	if count == 0 && len(containers) > 1 {
+		return apis.ErrMissingField("ports"), port
 	}
 	// More than one container sections have ports.
 	if count > 1 {
-		return apis.ErrMultipleOneOf("ports")
+		return &apis.FieldError{
+			Message: "more than one container port is set",
+			Paths:   []string{"ports"},
+			Details: "Only a single port is allowed across all containers",
+		}, port
 	}
-	return nil
+	return nil, port
 }
 
 // validateSidecarContainer validate fields for non serving containers
@@ -447,27 +467,14 @@ func validateInitContainer(ctx context.Context, container corev1.Container, volu
 }
 
 // ValidateContainer validate fields for serving containers
-func ValidateContainer(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume) (errs *apis.FieldError) {
-	// Single container cannot have multiple ports
-	errs = errs.Also(portValidation(container.Ports).ViaField("ports"))
+func ValidateContainer(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume, port intstr.IntOrString) (errs *apis.FieldError) {
 	// Liveness Probes
-	errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe"))
+	errs = errs.Also(validateProbe(container.LivenessProbe, port).ViaField("livenessProbe"))
 	// Readiness Probes
-	errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe"))
+	errs = errs.Also(validateReadinessProbe(container.ReadinessProbe, port).ViaField("readinessProbe"))
 	return errs.Also(validate(ctx, container, volumes))
 }
 
-func portValidation(containerPorts []corev1.ContainerPort) *apis.FieldError {
-	if len(containerPorts) > 1 {
-		return &apis.FieldError{
-			Message: "More than one container port is set",
-			Paths:   []string{apis.CurrentField},
-			Details: "Only a single port is allowed",
-		}
-	}
-	return nil
-}
-
 func validate(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume) *apis.FieldError {
 	if equality.Semantic.DeepEqual(container, corev1.Container{}) {
 		return apis.ErrMissingField(apis.CurrentField)
@@ -653,12 +660,12 @@ func validateContainerPortBasic(port corev1.ContainerPort) *apis.FieldError {
 	return errs
 }
 
-func validateReadinessProbe(p *corev1.Probe) *apis.FieldError {
+func validateReadinessProbe(p *corev1.Probe, port intstr.IntOrString) *apis.FieldError {
 	if p == nil {
 		return nil
 	}
 
-	errs := validateProbe(p)
+	errs := validateProbe(p, port)
 
 	if p.PeriodSeconds < 0 {
 		errs = errs.Also(apis.ErrOutOfBoundsValue(p.PeriodSeconds, 0, math.MaxInt32, "periodSeconds"))
@@ -700,7 +707,7 @@ func validateReadinessProbe(p *corev1.Probe) *apis.FieldError {
 	return errs
 }
 
-func validateProbe(p *corev1.Probe) *apis.FieldError {
+func validateProbe(p *corev1.Probe, port intstr.IntOrString) *apis.FieldError {
 	if p == nil {
 		return nil
 	}
@@ -714,10 +721,18 @@ func validateProbe(p *corev1.Probe) *apis.FieldError {
 	if h.HTTPGet != nil {
 		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.StrVal) || (getPort.IntVal != 0 && getPort.IntVal != port.IntVal) {
+			errs = errs.Also(apis.ErrInvalidValue(getPort.String(), "httpGet.port", "May only probe containerPort"))
+		}
 	}
 	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.StrVal) || (tcpPort.IntVal != 0 && tcpPort.IntVal != port.IntVal) {
+			errs = errs.Also(apis.ErrInvalidValue(tcpPort.String(), "tcpSocket.port", "May only probe containerPort"))
+		}
 	}
 	if h.Exec != nil {
 		handlers = append(handlers, "exec")

diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go
@@ -277,6 +277,7 @@ func TestPodSpecValidation(t *testing.T) {
 		ps: corev1.PodSpec{
 			Containers: []corev1.Container{{
 				Image: "busybox",
+				Ports: []corev1.ContainerPort{{}},
 			}, {
 				Image: "helloworld",
 			}},
@@ -500,7 +501,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) {
 				Image: "helloworld",
 			}},
 		},
-		want: apis.ErrMissingField("containers.ports"),
+		want: apis.ErrMissingField("containers[*].ports"),
 	}, {
 		name: "flag enabled: multiple containers with multiple port",
 		ps: corev1.PodSpec{
@@ -516,7 +517,11 @@ func TestPodSpecMultiContainerValidation(t *testing.T) {
 				}},
 			}},
 		},
-		want: apis.ErrMultipleOneOf("containers.ports"),
+		want: &apis.FieldError{
+			Message: "more than one container port is set",
+			Paths:   []string{"containers[*].ports"},
+			Details: "Only a single port is allowed across all containers",
+		},
 	}, {
 		name: "flag enabled: multiple containers with multiple ports for each container",
 		ps: corev1.PodSpec{
@@ -534,11 +539,11 @@ func TestPodSpecMultiContainerValidation(t *testing.T) {
 				}},
 			}},
 		},
-		want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{
-			Message: "More than one container port is set",
-			Paths:   []string{"containers[0].ports"},
-			Details: "Only a single port is allowed",
-		}),
+		want: &apis.FieldError{
+			Message: "more than one container port is set",
+			Paths:   []string{"containers[*].ports"},
+			Details: "Only a single port is allowed across all containers",
+		},
 	}, {
 		name: "flag enabled: multiple containers with multiple port for a single container",
 		ps: corev1.PodSpec{
@@ -553,11 +558,31 @@ func TestPodSpecMultiContainerValidation(t *testing.T) {
 				Image: "helloworld",
 			}},
 		},
-		want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{
-			Message: "More than one container port is set",
-			Paths:   []string{"containers[0].ports"},
-			Details: "Only a single port is allowed",
-		}),
+		want: &apis.FieldError{
+			Message: "more than one container port is set",
+			Paths:   []string{"containers[*].ports"},
+			Details: "Only a single port is allowed across all containers",
+		},
+	}, {
+		name: "flag enabled: multiple containers with readinessProbe targeting different container's port",
+		ps: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Image: "work",
+				Ports: []corev1.ContainerPort{{
+					ContainerPort: 9999,
+				}},
+			}, {
+				Image: "health",
+				LivenessProbe: &corev1.Probe{
+					Handler: corev1.Handler{
+						HTTPGet: &corev1.HTTPGetAction{
+							Port: intstr.FromInt(9999), // This is the other container's port
+						},
+					},
+				},
+			}},
+		},
+		want: apis.ErrDisallowedFields("containers[1].livenessProbe"),
 	}, {
 		name: "flag enabled: multiple containers with illegal env variable defined for side car",
 		ps: corev1.PodSpec{
@@ -1111,9 +1136,9 @@ func TestContainerValidation(t *testing.T) {
 				}},
 			},
 			want: &apis.FieldError{
-				Message: "More than one container port is set",
+				Message: "more than one container port is set",
 				Paths:   []string{"ports"},
-				Details: "Only a single port is allowed",
+				Details: "Only a single port is allowed across all containers",
 			},
 		}, {
 			name: "has an empty port set",
@@ -1133,9 +1158,9 @@ func TestContainerValidation(t *testing.T) {
 				}},
 			},
 			want: &apis.FieldError{
-				Message: "More than one container port is set",
+				Message: "more than one container port is set",
 				Paths:   []string{"ports"},
-				Details: "Only a single port is allowed",
+				Details: "Only a single port is allowed across all containers",
 			},
 		}, {
 			name: "has tcp protocol",
@@ -1262,7 +1287,7 @@ func TestContainerValidation(t *testing.T) {
 			},
 			want: apis.ErrMultipleOneOf("readinessProbe.exec", "readinessProbe.tcpSocket", "readinessProbe.httpGet"),
 		}, {
-			name: "invalid readiness http probe (has port)",
+			name: "invalid readiness http probe (has wrong port)",
 			c: corev1.Container{
 				Image: "foo",
 				ReadinessProbe: &corev1.Probe{
@@ -1273,12 +1298,25 @@ func TestContainerValidation(t *testing.T) {
 					Handler: corev1.Handler{
 						HTTPGet: &corev1.HTTPGetAction{
 							Path: "/",
-							Port: intstr.FromInt(8080),
+							Port: intstr.FromInt(5000),
+						},
+					},
+				},
+			},
+			want: apis.ErrInvalidValue(5000, "readinessProbe.httpGet.port", "May only probe containerPort"),
+		}, {
+			name: "valid readiness http probe with port",
+			c: corev1.Container{
+				Image: "foo",
+				ReadinessProbe: &corev1.Probe{
+					SuccessThreshold: 1,
+					Handler: corev1.Handler{
+						HTTPGet: &corev1.HTTPGetAction{
+							Port: intstr.FromString("http"), // http is the default
 						},
 					},
 				},
 			},
-			want: apis.ErrDisallowedFields("readinessProbe.httpGet.port"),
 		}, {
 			name: "invalid readiness probe (has failureThreshold while using special probe)",
 			c: corev1.Container{
@@ -1360,12 +1398,29 @@ func TestContainerValidation(t *testing.T) {
 				LivenessProbe: &corev1.Probe{
 					Handler: corev1.Handler{
 						TCPSocket: &corev1.TCPSocketAction{
-							Port: intstr.FromString("http"),
+							Port: intstr.FromString("imap"),
+						},
+					},
+				},
+			},
+			want: apis.ErrInvalidValue("imap", "livenessProbe.tcpSocket.port", "May only probe containerPort"),
+		}, {
+			name: "valid liveness tcp probe with correct port",
+			c: corev1.Container{
+				Image: "foo",
+				Ports: []corev1.ContainerPort{
+					{
+						ContainerPort: 8888,
+					},
+				},
+				LivenessProbe: &corev1.Probe{
+					Handler: corev1.Handler{
+						TCPSocket: &corev1.TCPSocketAction{
+							Port: intstr.FromInt(8888),
 						},
 					},
 				},
 			},
-			want: apis.ErrDisallowedFields("livenessProbe.tcpSocket.port"),
 		}, {
 			name: "disallowed container fields",
 			c: corev1.Container{
@@ -1405,8 +1460,9 @@ func TestContainerValidation(t *testing.T) {
 				}
 				ctx = config.ToContext(ctx, cfg)
 			}
+			err, port := validateContainersPorts([]corev1.Container{test.c})
 
-			got := ValidateContainer(ctx, test.c, test.volumes)
+			got := err.Also(ValidateContainer(ctx, test.c, test.volumes, port))
 			if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" {
 				t.Errorf("ValidateContainer (-want, +got): \n%s", diff)
 			}