Add multi-container probing (knative#14853)

* Add multi-container probing

* Add e2e test for multi container probing

(cherry picked from commit a194cb2)

config/core/configmaps/features.yaml

@@ -22,7 +22,7 @@ metadata:
     app.kubernetes.io/component: controller
     app.kubernetes.io/version: devel
   annotations:
-    knative.dev/example-checksum: "f2fc138e"
+    knative.dev/example-checksum: "632d47dd"
 data:
   _example: |-
     ################################
@@ -50,9 +50,15 @@ data:
     # Indicates whether multi container support is enabled
     #
     # WARNING: Cannot safely be disabled once enabled.
-    # See: https://knative.dev/docs/serving/feature-flags/#multi-containers
+    # See: https://knative.dev/docs/serving/configuration/feature-flags/#multiple-containers
     multi-container: "enabled"
 
+    # Indicates whether multi container probing is enabled
+    #
+    # WARNING: Cannot safely be disabled once enabled.
+    # See: https://knative.dev/docs/serving/configuration/feature-flags/#multiple-container-probing
+    multi-container-probing: "disabled"
+
     # Indicates whether Kubernetes affinity support is enabled
     #
     # WARNING: Cannot safely be disabled once enabled.

pkg/apis/config/features.go

@@ -54,6 +54,7 @@ const (
 func defaultFeaturesConfig() *Features {
 	return &Features{
 		MultiContainer:                   Enabled,
+		MultiContainerProbing:            Disabled,
 		PodSpecAffinity:                  Disabled,
 		PodSpecTopologySpreadConstraints: Disabled,
 		PodSpecDryRun:                    Allowed,
@@ -87,6 +88,7 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
 
 	if err := cm.Parse(data,
 		asFlag("multi-container", &nc.MultiContainer),
+		asFlag("multi-container-probing", &nc.MultiContainerProbing),
 		asFlag("kubernetes.podspec-affinity", &nc.PodSpecAffinity),
 		asFlag("kubernetes.podspec-topologyspreadconstraints", &nc.PodSpecTopologySpreadConstraints),
 		asFlag("kubernetes.podspec-dryrun", &nc.PodSpecDryRun),
@@ -124,6 +126,7 @@ func NewFeaturesConfigFromConfigMap(config *corev1.ConfigMap) (*Features, error)
 // Features specifies which features are allowed by the webhook.
 type Features struct {
 	MultiContainer                   Flag
+	MultiContainerProbing            Flag
 	PodSpecAffinity                  Flag
 	PodSpecTopologySpreadConstraints Flag
 	PodSpecDryRun                    Flag

pkg/apis/config/features_test.go

@@ -60,6 +60,7 @@ func TestFeaturesConfiguration(t *testing.T) {
 		wantErr: false,
 		wantFeatures: defaultWith(&Features{
 			MultiContainer:                   Enabled,
+			MultiContainerProbing:            Enabled,
 			PodSpecAffinity:                  Enabled,
 			PodSpecTopologySpreadConstraints: Enabled,
 			PodSpecDryRun:                    Enabled,
@@ -79,6 +80,7 @@ func TestFeaturesConfiguration(t *testing.T) {
 		}),
 		data: map[string]string{
 			"multi-container":                              "Enabled",
+			"multi-container-probing":                      "Enabled",
 			"kubernetes.podspec-affinity":                  "Enabled",
 			"kubernetes.podspec-topologyspreadconstraints": "Enabled",
 			"kubernetes.podspec-dryrun":                    "Enabled",
@@ -114,6 +116,24 @@ func TestFeaturesConfiguration(t *testing.T) {
 		data: map[string]string{
 			"multi-container": "Disabled",
 		},
+	}, {
+		name:    "multi-container-probing Allowed",
+		wantErr: false,
+		wantFeatures: defaultWith(&Features{
+			MultiContainerProbing: Allowed,
+		}),
+		data: map[string]string{
+			"multi-container-probing": "Allowed",
+		},
+	}, {
+		name:    "multi-container-probing Disabled",
+		wantErr: false,
+		wantFeatures: defaultWith(&Features{
+			MultiContainerProbing: Disabled,
+		}),
+		data: map[string]string{
+			"multi-container-probing": "Disabled",
+		},
 	}, {
 		name:    "kubernetes.podspec-affinity Allowed",
 		wantErr: false,

pkg/apis/serving/k8s_validation.go

@@ -387,7 +387,7 @@ func ValidatePodSpec(ctx context.Context, ps corev1.PodSpec) *apis.FieldError {
 	case 0:
 		errs = errs.Also(apis.ErrMissingField("containers"))
 	case 1:
-		errs = errs.Also(ValidateContainer(ctx, ps.Containers[0], volumes, port).
+		errs = errs.Also(ValidateUserContainer(ctx, ps.Containers[0], volumes, port).
 			ViaFieldIndex("containers", 0))
 	default:
 		errs = errs.Also(validateContainers(ctx, ps.Containers, volumes, port))
@@ -447,7 +447,7 @@ func validateContainers(ctx context.Context, containers []corev1.Container, volu
 			// 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, port).ViaFieldIndex("containers", i))
+			errs = errs.Also(ValidateUserContainer(WithinUserContainer(ctx), containers[i], volumes, port).ViaFieldIndex("containers", i))
 		}
 	}
 	return errs
@@ -503,14 +503,23 @@ func validateContainersPorts(containers []corev1.Container) (corev1.ContainerPor
 
 // validateSidecarContainer validate fields for non serving containers
 func validateSidecarContainer(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume) (errs *apis.FieldError) {
-	if container.LivenessProbe != nil {
-		errs = errs.Also(apis.CheckDisallowedFields(*container.LivenessProbe,
-			*ProbeMask(&corev1.Probe{})).ViaField("livenessProbe"))
-	}
-	if container.ReadinessProbe != nil {
-		errs = errs.Also(apis.CheckDisallowedFields(*container.ReadinessProbe,
-			*ProbeMask(&corev1.Probe{})).ViaField("readinessProbe"))
+	cfg := config.FromContextOrDefaults(ctx)
+	if cfg.Features.MultiContainerProbing != config.Enabled {
+		if container.LivenessProbe != nil {
+			errs = errs.Also(apis.CheckDisallowedFields(*container.LivenessProbe,
+				*ProbeMask(&corev1.Probe{})).ViaField("livenessProbe"))
+		}
+		if container.ReadinessProbe != nil {
+			errs = errs.Also(apis.CheckDisallowedFields(*container.ReadinessProbe,
+				*ProbeMask(&corev1.Probe{})).ViaField("readinessProbe"))
+		}
+	} else if cfg.Features.MultiContainerProbing == config.Enabled {
+		// Liveness Probes
+		errs = errs.Also(validateProbe(container.LivenessProbe, nil, false).ViaField("livenessProbe"))
+		// Readiness Probes
+		errs = errs.Also(validateReadinessProbe(container.ReadinessProbe, nil, false).ViaField("readinessProbe"))
 	}
+
 	return errs.Also(validate(ctx, container, volumes))
 }
 
@@ -544,12 +553,12 @@ func validateInitContainer(ctx context.Context, container corev1.Container, volu
 	return errs.Also(validate(WithinInitContainer(ctx), container, volumes))
 }
 
-// ValidateContainer validate fields for serving containers
-func ValidateContainer(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume, port corev1.ContainerPort) (errs *apis.FieldError) {
+// ValidateUserContainer validate fields for serving containers
+func ValidateUserContainer(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume, port corev1.ContainerPort) (errs *apis.FieldError) {
 	// Liveness Probes
-	errs = errs.Also(validateProbe(container.LivenessProbe, port).ViaField("livenessProbe"))
+	errs = errs.Also(validateProbe(container.LivenessProbe, &port, true).ViaField("livenessProbe"))
 	// Readiness Probes
-	errs = errs.Also(validateReadinessProbe(container.ReadinessProbe, port).ViaField("readinessProbe"))
+	errs = errs.Also(validateReadinessProbe(container.ReadinessProbe, &port, true).ViaField("readinessProbe"))
 	return errs.Also(validate(ctx, container, volumes))
 }
 
@@ -751,12 +760,12 @@ func validateContainerPortBasic(port corev1.ContainerPort) *apis.FieldError {
 	return errs
 }
 
-func validateReadinessProbe(p *corev1.Probe, port corev1.ContainerPort) *apis.FieldError {
+func validateReadinessProbe(p *corev1.Probe, port *corev1.ContainerPort, isUserContainer bool) *apis.FieldError {
 	if p == nil {
 		return nil
 	}
 
-	errs := validateProbe(p, port)
+	errs := validateProbe(p, port, isUserContainer)
 
 	if p.PeriodSeconds < 0 {
 		errs = errs.Also(apis.ErrOutOfBoundsValue(p.PeriodSeconds, 0, math.MaxInt32, "periodSeconds"))
@@ -798,7 +807,7 @@ func validateReadinessProbe(p *corev1.Probe, port corev1.ContainerPort) *apis.Fi
 	return errs
 }
 
-func validateProbe(p *corev1.Probe, port corev1.ContainerPort) *apis.FieldError {
+func validateProbe(p *corev1.Probe, port *corev1.ContainerPort, isUserContainer bool) *apis.FieldError {
 	if p == nil {
 		return nil
 	}
@@ -813,16 +822,28 @@ 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 {
-			errs = errs.Also(apis.ErrInvalidValue(getPort.String(), "httpGet.port", "Probe port must match container port"))
+		if isUserContainer {
+			if getPort.StrVal != "" && getPort.StrVal != port.Name {
+				errs = errs.Also(apis.ErrInvalidValue(getPort.String(), "httpGet.port", "Probe port must match container port"))
+			}
+		} else {
+			if getPort.StrVal == "" && getPort.IntVal == 0 {
+				errs = errs.Also(apis.ErrInvalidValue(getPort.String(), "httpGet.port", "Probe port must be specified"))
+			}
 		}
 	}
 	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 {
-			errs = errs.Also(apis.ErrInvalidValue(tcpPort.String(), "tcpSocket.port", "Probe port must match container port"))
+		if isUserContainer {
+			if tcpPort.StrVal != "" && tcpPort.StrVal != port.Name {
+				errs = errs.Also(apis.ErrInvalidValue(tcpPort.String(), "tcpSocket.port", "Probe port must match container port"))
+			}
+		} else {
+			if tcpPort.StrVal == "" && tcpPort.IntVal == 0 {
+				errs = errs.Also(apis.ErrInvalidValue(tcpPort.String(), "tcpSocket.port", "Probe port must be specified"))
+			}
 		}
 	}
 	if h.Exec != nil {