Merge pull request #105293 from verb/1.23-ec-validation

Validate PodSpec in EphemeralContainersUpdate

pkg/apis/core/validation/validation.go

@@ -4213,17 +4213,18 @@ func validatePodConditions(conditions []core.PodCondition, fldPath *field.Path)
 // ValidatePodEphemeralContainersUpdate tests that a user update to EphemeralContainers is valid.
 // newPod and oldPod must only differ in their EphemeralContainers.
 func ValidatePodEphemeralContainersUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions) field.ErrorList {
-	spec := newPod.Spec
+	// Part 1: Validate newPod's spec and updates to metadata
-	specPath := field.NewPath("spec").Child("ephemeralContainers")
+	fldPath := field.NewPath("metadata")
-
+	allErrs := ValidateObjectMetaUpdate(&newPod.ObjectMeta, &oldPod.ObjectMeta, fldPath)
-	vols := make(map[string]core.VolumeSource)
+	allErrs = append(allErrs, validatePodMetadataAndSpec(newPod, opts)...)
-	for _, vol := range spec.Volumes {
+	allErrs = append(allErrs, ValidatePodSpecificAnnotationUpdates(newPod, oldPod, fldPath.Child("annotations"), opts)...)
-		vols[vol.Name] = vol.VolumeSource
-	}
-	allErrs := validateEphemeralContainers(spec.EphemeralContainers, spec.Containers, spec.InitContainers, vols, specPath, opts)
 
+	// Part 2: Validate that the changes between oldPod.Spec.EphemeralContainers and
+	// newPod.Spec.EphemeralContainers are allowed.
+	//
 	// Existing EphemeralContainers may not be changed. Order isn't preserved by patch, so check each individually.
 	newContainerIndex := make(map[string]*core.EphemeralContainer)
+	specPath := field.NewPath("spec").Child("ephemeralContainers")
 	for i := range newPod.Spec.EphemeralContainers {
 		newContainerIndex[newPod.Spec.EphemeralContainers[i].Name] = &newPod.Spec.EphemeralContainers[i]
 	}

pkg/apis/core/validation/validation_test.go

@@ -23,8 +23,10 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	asserttestify "github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/proto"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -10371,96 +10373,166 @@ func makeValidService() core.Service {
 }
 
 func TestValidatePodEphemeralContainersUpdate(t *testing.T) {
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)()
+
+	makePod := func(ephemeralContainers []core.EphemeralContainer) *core.Pod {
+		return &core.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations:     map[string]string{},
+				Labels:          map[string]string{},
+				Name:            "pod",
+				Namespace:       "ns",
+				ResourceVersion: "1",
+			},
+			Spec: core.PodSpec{
+				Containers: []core.Container{{
+					Name:                     "cnt",
+					Image:                    "image",
+					ImagePullPolicy:          "IfNotPresent",
+					TerminationMessagePolicy: "File",
+				}},
+				DNSPolicy:           core.DNSClusterFirst,
+				EphemeralContainers: ephemeralContainers,
+				RestartPolicy:       core.RestartPolicyOnFailure,
+			},
+		}
+	}
+
+	// Some tests use Windows host pods as an example of fields that might
+	// conflict between an ephemeral container and the rest of the pod.
+	opts := PodValidationOptions{AllowWindowsHostProcessField: true}
+	capabilities.SetForTests(capabilities.Capabilities{
+		AllowPrivileged: true,
+	})
+	makeWindowsHostPod := func(ephemeralContainers []core.EphemeralContainer) *core.Pod {
+		return &core.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations:     map[string]string{},
+				Labels:          map[string]string{},
+				Name:            "pod",
+				Namespace:       "ns",
+				ResourceVersion: "1",
+			},
+			Spec: core.PodSpec{
+				Containers: []core.Container{{
+					Name:            "cnt",
+					Image:           "image",
+					ImagePullPolicy: "IfNotPresent",
+					SecurityContext: &core.SecurityContext{
+						WindowsOptions: &core.WindowsSecurityContextOptions{
+							HostProcess: proto.Bool(true),
+						},
+					},
+					TerminationMessagePolicy: "File",
+				}},
+				DNSPolicy:           core.DNSClusterFirst,
+				EphemeralContainers: ephemeralContainers,
+				RestartPolicy:       core.RestartPolicyOnFailure,
+				SecurityContext: &core.PodSecurityContext{
+					HostNetwork: true,
+					WindowsOptions: &core.WindowsSecurityContextOptions{
+						HostProcess: proto.Bool(true),
+					},
+				},
+			},
+		}
+	}
+
 	tests := []struct {
-		new  []core.EphemeralContainer
+		name     string
-		old  []core.EphemeralContainer
+		new, old *core.Pod
-		err  string
+		err      string
-		test string
 	}{
-		{[]core.EphemeralContainer{}, []core.EphemeralContainer{}, "", "nothing"},
 		{
-			[]core.EphemeralContainer{{
+			"no ephemeral containers",
-				EphemeralContainerCommon: core.EphemeralContainerCommon{
+			makePod([]core.EphemeralContainer{}),
-					Name:                     "debugger",
+			makePod([]core.EphemeralContainer{}),
-					Image:                    "busybox",
-					ImagePullPolicy:          "IfNotPresent",
-					TerminationMessagePolicy: "File",
-				},
-			}, {
-				EphemeralContainerCommon: core.EphemeralContainerCommon{
-					Name:                     "debugger2",
-					Image:                    "busybox",
-					ImagePullPolicy:          "IfNotPresent",
-					TerminationMessagePolicy: "File",
-				},
-			}},
-			[]core.EphemeralContainer{{
-				EphemeralContainerCommon: core.EphemeralContainerCommon{
-					Name:                     "debugger",
-					Image:                    "busybox",
-					ImagePullPolicy:          "IfNotPresent",
-					TerminationMessagePolicy: "File",
-				},
-			}, {
-				EphemeralContainerCommon: core.EphemeralContainerCommon{
-					Name:                     "debugger2",
-					Image:                    "busybox",
-					ImagePullPolicy:          "IfNotPresent",
-					TerminationMessagePolicy: "File",
-				},
-			}},
 			"",
+		},
+		{
 			"No change in Ephemeral Containers",
+			makePod([]core.EphemeralContainer{{
+				EphemeralContainerCommon: core.EphemeralContainerCommon{
+					Name:                     "debugger",
+					Image:                    "busybox",
+					ImagePullPolicy:          "IfNotPresent",
+					TerminationMessagePolicy: "File",
+				},
+			}, {
+				EphemeralContainerCommon: core.EphemeralContainerCommon{
+					Name:                     "debugger2",
+					Image:                    "busybox",
+					ImagePullPolicy:          "IfNotPresent",
+					TerminationMessagePolicy: "File",
+				},
+			}}),
+			makePod([]core.EphemeralContainer{{
+				EphemeralContainerCommon: core.EphemeralContainerCommon{
+					Name:                     "debugger",
+					Image:                    "busybox",
+					ImagePullPolicy:          "IfNotPresent",
+					TerminationMessagePolicy: "File",
+				},
+			}, {
+				EphemeralContainerCommon: core.EphemeralContainerCommon{
+					Name:                     "debugger2",
+					Image:                    "busybox",
+					ImagePullPolicy:          "IfNotPresent",
+					TerminationMessagePolicy: "File",
+				},
+			}}),
+			"",
 		},
 		{
-			[]core.EphemeralContainer{{
-				EphemeralContainerCommon: core.EphemeralContainerCommon{
-					Name:                     "debugger",
-					Image:                    "busybox",
-					ImagePullPolicy:          "IfNotPresent",
-					TerminationMessagePolicy: "File",
-				},
-			}, {
-				EphemeralContainerCommon: core.EphemeralContainerCommon{
-					Name:                     "debugger2",
-					Image:                    "busybox",
-					ImagePullPolicy:          "IfNotPresent",
-					TerminationMessagePolicy: "File",
-				},
-			}},
-			[]core.EphemeralContainer{{
-				EphemeralContainerCommon: core.EphemeralContainerCommon{
-					Name:                     "debugger2",
-					Image:                    "busybox",
-					ImagePullPolicy:          "IfNotPresent",
-					TerminationMessagePolicy: "File",
-				},
-			}, {
-				EphemeralContainerCommon: core.EphemeralContainerCommon{
-					Name:                     "debugger",
-					Image:                    "busybox",
-					ImagePullPolicy:          "IfNotPresent",
-					TerminationMessagePolicy: "File",
-				},
-			}},
-			"",
 			"Ephemeral Container list order changes",
-		},
+			makePod([]core.EphemeralContainer{{
-		{
-			[]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "debugger",
 					Image:                    "busybox",
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}, {
-			[]core.EphemeralContainer{},
+				EphemeralContainerCommon: core.EphemeralContainerCommon{
+					Name:                     "debugger2",
+					Image:                    "busybox",
+					ImagePullPolicy:          "IfNotPresent",
+					TerminationMessagePolicy: "File",
+				},
+			}}),
+			makePod([]core.EphemeralContainer{{
+				EphemeralContainerCommon: core.EphemeralContainerCommon{
+					Name:                     "debugger2",
+					Image:                    "busybox",
+					ImagePullPolicy:          "IfNotPresent",
+					TerminationMessagePolicy: "File",
+				},
+			}, {
+				EphemeralContainerCommon: core.EphemeralContainerCommon{
+					Name:                     "debugger",
+					Image:                    "busybox",
+					ImagePullPolicy:          "IfNotPresent",
+					TerminationMessagePolicy: "File",
+				},
+			}}),
 			"",
-			"Add an Ephemeral Container",
 		},
 		{
-			[]core.EphemeralContainer{{
+			"Add an Ephemeral Container",
+			makePod([]core.EphemeralContainer{{
+				EphemeralContainerCommon: core.EphemeralContainerCommon{
+					Name:                     "debugger",
+					Image:                    "busybox",
+					ImagePullPolicy:          "IfNotPresent",
+					TerminationMessagePolicy: "File",
+				},
+			}}),
+			makePod([]core.EphemeralContainer{}),
+			"",
+		},
+		{
+			"Add two Ephemeral Containers",
+			makePod([]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "debugger1",
 					Image:                    "busybox",
@@ -10474,13 +10546,13 @@ func TestValidatePodEphemeralContainersUpdate(t *testing.T) {
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}}),
-			[]core.EphemeralContainer{},
+			makePod([]core.EphemeralContainer{}),
 			"",
-			"Add two Ephemeral Containers",
 		},
 		{
-			[]core.EphemeralContainer{{
+			"Add to an existing Ephemeral Containers",
+			makePod([]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "debugger",
 					Image:                    "busybox",
@@ -10494,20 +10566,20 @@ func TestValidatePodEphemeralContainersUpdate(t *testing.T) {
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}}),
-			[]core.EphemeralContainer{{
+			makePod([]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "debugger",
 					Image:                    "busybox",
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}}),
 			"",
-			"Add to an existing Ephemeral Containers",
 		},
 		{
-			[]core.EphemeralContainer{{
+			"Add to an existing Ephemeral Containers, list order changes",
+			makePod([]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "debugger3",
 					Image:                    "busybox",
@@ -10528,8 +10600,8 @@ func TestValidatePodEphemeralContainersUpdate(t *testing.T) {
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}}),
-			[]core.EphemeralContainer{{
+			makePod([]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "debugger",
 					Image:                    "busybox",
@@ -10543,45 +10615,45 @@ func TestValidatePodEphemeralContainersUpdate(t *testing.T) {
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}}),
 			"",
-			"Add to an existing Ephemeral Containers, list order changes",
 		},
 		{
-			[]core.EphemeralContainer{},
+			"Remove an Ephemeral Container",
-			[]core.EphemeralContainer{{
+			makePod([]core.EphemeralContainer{}),
+			makePod([]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "debugger",
 					Image:                    "busybox",
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}}),
 			"may not be removed",
-			"Remove an Ephemeral Container",
 		},
 		{
-			[]core.EphemeralContainer{{
+			"Replace an Ephemeral Container",
+			makePod([]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "firstone",
 					Image:                    "busybox",
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}}),
-			[]core.EphemeralContainer{{
+			makePod([]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "thentheother",
 					Image:                    "busybox",
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}}),
 			"may not be removed",
-			"Replace an Ephemeral Container",
 		},
 		{
-			[]core.EphemeralContainer{{
+			"Change an Ephemeral Containers",
+			makePod([]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "debugger1",
 					Image:                    "busybox",
@@ -10595,8 +10667,8 @@ func TestValidatePodEphemeralContainersUpdate(t *testing.T) {
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}}),
-			[]core.EphemeralContainer{{
+			makePod([]core.EphemeralContainer{{
 				EphemeralContainerCommon: core.EphemeralContainerCommon{
 					Name:                     "debugger1",
 					Image:                    "debian",
@@ -10610,25 +10682,58 @@ func TestValidatePodEphemeralContainersUpdate(t *testing.T) {
 					ImagePullPolicy:          "IfNotPresent",
 					TerminationMessagePolicy: "File",
 				},
-			}},
+			}}),
 			"may not be changed",
-			"Change an Ephemeral Containers",
+		},
+		{
+			"Ephemeral container with potential conflict with regular containers, but conflict not present",
+			makeWindowsHostPod([]core.EphemeralContainer{{
+				EphemeralContainerCommon: core.EphemeralContainerCommon{
+					Name:            "debugger1",
+					Image:           "image",
+					ImagePullPolicy: "IfNotPresent",
+					SecurityContext: &core.SecurityContext{
+						WindowsOptions: &core.WindowsSecurityContextOptions{
+							HostProcess: proto.Bool(true),
+						},
+					},
+					TerminationMessagePolicy: "File",
+				},
+			}}),
+			makeWindowsHostPod(nil),
+			"",
+		},
+		{
+			"Ephemeral container with potential conflict with regular containers, and conflict is present",
+			makeWindowsHostPod([]core.EphemeralContainer{{
+				EphemeralContainerCommon: core.EphemeralContainerCommon{
+					Name:            "debugger1",
+					Image:           "image",
+					ImagePullPolicy: "IfNotPresent",
+					SecurityContext: &core.SecurityContext{
+						WindowsOptions: &core.WindowsSecurityContextOptions{
+							HostProcess: proto.Bool(false),
+						},
+					},
+					TerminationMessagePolicy: "File",
+				},
+			}}),
+			makeWindowsHostPod(nil),
+			"spec.ephemeralContainers[0].securityContext.windowsOptions.hostProcess: Invalid value: false: pod hostProcess value must be identical",
 		},
 	}
 
-	for _, test := range tests {
+	for _, tc := range tests {
-		new := core.Pod{Spec: core.PodSpec{EphemeralContainers: test.new}}
+		errs := ValidatePodEphemeralContainersUpdate(tc.new, tc.old, opts)
-		old := core.Pod{Spec: core.PodSpec{EphemeralContainers: test.old}}
+		if tc.err == "" {
-		errs := ValidatePodEphemeralContainersUpdate(&new, &old, PodValidationOptions{})
-		if test.err == "" {
 			if len(errs) != 0 {
-				t.Errorf("unexpected invalid: %s (%+v)\nA: %+v\nB: %+v", test.test, errs, test.new, test.old)
+				t.Errorf("unexpected invalid for test: %s\nErrors returned: %+v\nLocal diff of test objects (-old +new):\n%s", tc.name, errs, cmp.Diff(tc.old, tc.new))
 			}
 		} else {
 			if len(errs) == 0 {
-				t.Errorf("unexpected valid: %s\nA: %+v\nB: %+v", test.test, test.new, test.old)
+				t.Errorf("unexpected valid for test: %s\nLocal diff of test objects (-old +new):\n%s", tc.name, cmp.Diff(tc.old, tc.new))
-			} else if actualErr := errs.ToAggregate().Error(); !strings.Contains(actualErr, test.err) {
+			} else if actualErr := errs.ToAggregate().Error(); !strings.Contains(actualErr, tc.err) {
-				t.Errorf("unexpected error message: %s\nExpected error: %s\nActual error: %s", test.test, test.err, actualErr)
+				t.Errorf("unexpected error message: %s\nExpected error: %s\nActual error: %s", tc.name, tc.err, actualErr)
 			}
 		}
 	}