diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 0441a9a8702..781e9787e8e 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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 - specPath := field.NewPath("spec").Child("ephemeralContainers") - - vols := make(map[string]core.VolumeSource) - for _, vol := range spec.Volumes { - vols[vol.Name] = vol.VolumeSource - } - allErrs := validateEphemeralContainers(spec.EphemeralContainers, spec.Containers, spec.InitContainers, vols, specPath, opts) + // Part 1: Validate newPod's spec and updates to metadata + fldPath := field.NewPath("metadata") + allErrs := ValidateObjectMetaUpdate(&newPod.ObjectMeta, &oldPod.ObjectMeta, fldPath) + allErrs = append(allErrs, validatePodMetadataAndSpec(newPod, opts)...) + allErrs = append(allErrs, ValidatePodSpecificAnnotationUpdates(newPod, oldPod, fldPath.Child("annotations"), 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] } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index cc2c52845c8..76c995ed2fb 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/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 - old []core.EphemeralContainer - err string - test string + name string + new, old *core.Pod + err string }{ - {[]core.EphemeralContainer{}, []core.EphemeralContainer{}, "", "nothing"}, { - []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", - }, - }}, + "no ephemeral containers", + makePod([]core.EphemeralContainer{}), + makePod([]core.EphemeralContainer{}), "", + }, + { "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", - }, - { - []core.EphemeralContainer{{ + makePod([]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{}, - []core.EphemeralContainer{{ + "Remove an Ephemeral Container", + 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 { - new := core.Pod{Spec: core.PodSpec{EphemeralContainers: test.new}} - old := core.Pod{Spec: core.PodSpec{EphemeralContainers: test.old}} - errs := ValidatePodEphemeralContainersUpdate(&new, &old, PodValidationOptions{}) - if test.err == "" { + for _, tc := range tests { + errs := ValidatePodEphemeralContainersUpdate(tc.new, tc.old, opts) + if tc.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) - } else if actualErr := errs.ToAggregate().Error(); !strings.Contains(actualErr, test.err) { - t.Errorf("unexpected error message: %s\nExpected error: %s\nActual error: %s", test.test, test.err, actualErr) + 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, tc.err) { + t.Errorf("unexpected error message: %s\nExpected error: %s\nActual error: %s", tc.name, tc.err, actualErr) } } }