From 4451138bfa784942e59dc092fc8d707e73eaa284 Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Mon, 27 Sep 2021 22:39:06 +0200 Subject: [PATCH 1/2] Validate PodSpec in EphemeralContainersUpdate Previously this only validated the ephemeral containers, but it's safer to validate the entire PodSpec in case other parts of validation add logic that checks ephemeral containers. --- pkg/apis/core/validation/validation.go | 17 +++++++------- pkg/apis/core/validation/validation_test.go | 26 +++++++++++++++++++-- 2 files changed, 33 insertions(+), 10 deletions(-) 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..8de1dc8b172 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -10616,9 +10616,31 @@ func TestValidatePodEphemeralContainersUpdate(t *testing.T) { }, } + 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, + }, + } + } + for _, test := range tests { - new := core.Pod{Spec: core.PodSpec{EphemeralContainers: test.new}} - old := core.Pod{Spec: core.PodSpec{EphemeralContainers: test.old}} + new, old := makePod(test.new), makePod(test.old) errs := ValidatePodEphemeralContainersUpdate(&new, &old, PodValidationOptions{}) if test.err == "" { if len(errs) != 0 { From 8b24dc07ffc351c627bf5de82a03a1b93a5fe629 Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Thu, 30 Sep 2021 21:47:19 +0200 Subject: [PATCH 2/2] Test ephemeral container/pod conflicting fields This adds a test case to cover the scenario where the fields of an ephemeral container conflict with other fields in the pod and must be detected by full PodSpec validation. --- pkg/apis/core/validation/validation_test.go | 591 +++++++++++--------- 1 file changed, 337 insertions(+), 254 deletions(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 8de1dc8b172..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,253 +10373,10 @@ func makeValidService() core.Service { } func TestValidatePodEphemeralContainersUpdate(t *testing.T) { - tests := []struct { - new []core.EphemeralContainer - old []core.EphemeralContainer - err string - test 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 change in Ephemeral Containers", - }, - { - []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{{ - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }}, - []core.EphemeralContainer{}, - "", - "Add an Ephemeral Container", - }, - { - []core.EphemeralContainer{{ - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger1", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, { - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger2", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }}, - []core.EphemeralContainer{}, - "", - "Add two Ephemeral Containers", - }, - { - []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", - }, - }}, - "", - "Add to an existing Ephemeral Containers", - }, - { - []core.EphemeralContainer{{ - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger3", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, { - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger2", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, { - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger", - 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", - }, - }}, - "", - "Add to an existing Ephemeral Containers, list order changes", - }, - { - []core.EphemeralContainer{}, - []core.EphemeralContainer{{ - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }}, - "may not be removed", - "Remove an Ephemeral Container", - }, - { - []core.EphemeralContainer{{ - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "firstone", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }}, - []core.EphemeralContainer{{ - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "thentheother", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }}, - "may not be removed", - "Replace an Ephemeral Container", - }, - { - []core.EphemeralContainer{{ - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger1", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, { - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger2", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }}, - []core.EphemeralContainer{{ - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger1", - Image: "debian", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, { - EphemeralContainerCommon: core.EphemeralContainerCommon{ - Name: "debugger2", - Image: "busybox", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }}, - "may not be changed", - "Change an Ephemeral Containers", - }, - } + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)() - makePod := func(ephemeralContainers []core.EphemeralContainer) core.Pod { - return core.Pod{ + makePod := func(ephemeralContainers []core.EphemeralContainer) *core.Pod { + return &core.Pod{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{}, Labels: map[string]string{}, @@ -10639,18 +10398,342 @@ func TestValidatePodEphemeralContainersUpdate(t *testing.T) { } } - for _, test := range tests { - new, old := makePod(test.new), makePod(test.old) - errs := ValidatePodEphemeralContainersUpdate(&new, &old, PodValidationOptions{}) - if test.err == "" { + // 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 { + name string + new, old *core.Pod + err string + }{ + { + "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", + }, + }}), + "", + }, + { + "Ephemeral Container list order changes", + 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: "debugger2", + Image: "busybox", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, { + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debugger", + Image: "busybox", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }}), + "", + }, + { + "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", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, { + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debugger2", + Image: "busybox", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }}), + makePod([]core.EphemeralContainer{}), + "", + }, + { + "Add to an existing 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", + }, + }}), + "", + }, + { + "Add to an existing Ephemeral Containers, list order changes", + makePod([]core.EphemeralContainer{{ + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debugger3", + Image: "busybox", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, { + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debugger2", + Image: "busybox", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, { + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debugger", + 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", + }, + }}), + "", + }, + { + "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", + }, + { + "Replace an Ephemeral Container", + makePod([]core.EphemeralContainer{{ + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "firstone", + Image: "busybox", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }}), + makePod([]core.EphemeralContainer{{ + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "thentheother", + Image: "busybox", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }}), + "may not be removed", + }, + { + "Change an Ephemeral Containers", + makePod([]core.EphemeralContainer{{ + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debugger1", + Image: "busybox", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, { + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debugger2", + Image: "busybox", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }}), + makePod([]core.EphemeralContainer{{ + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debugger1", + Image: "debian", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, { + EphemeralContainerCommon: core.EphemeralContainerCommon{ + Name: "debugger2", + Image: "busybox", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }}), + "may not be changed", + }, + { + "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 _, 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) } } }