diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index de9f4f53095..0bd05e5d17c 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5616,6 +5616,14 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel allErrs = append(allErrs, field.Forbidden(specPath, "Pod running on node without support for resize")) } + // The rest of the validation assumes that the containers are in the same order, + // so we proceed only if that assumption is true. + containerOrderErrs := validatePodResizeContainerOrdering(newPod, oldPod, specPath) + allErrs = append(allErrs, containerOrderErrs...) + if containerOrderErrs != nil { + return allErrs + } + // Do not allow removing resource requests/limits on resize. if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) { for ix, ctr := range oldPod.Spec.InitContainers { @@ -5685,6 +5693,31 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel return allErrs } +// validatePodResizeContainerOrdering validates container ordering for a resize request. +// We do not allow adding, removing, re-ordering, or renaming containers on resize. +func validatePodResizeContainerOrdering(newPod, oldPod *core.Pod, specPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) { + allErrs = append(allErrs, field.Forbidden(specPath.Child("containers"), "containers may not be added or removed on resize")) + } else { + for i, oldCtr := range oldPod.Spec.Containers { + if newPod.Spec.Containers[i].Name != oldCtr.Name { + allErrs = append(allErrs, field.Forbidden(specPath.Child("containers").Index(i).Child("name"), "containers may not be renamed or reordered on resize")) + } + } + } + if len(newPod.Spec.InitContainers) != len(oldPod.Spec.InitContainers) { + allErrs = append(allErrs, field.Forbidden(specPath.Child("initContainers"), "initContainers may not be added or removed on resize")) + } else { + for i, oldCtr := range oldPod.Spec.InitContainers { + if newPod.Spec.InitContainers[i].Name != oldCtr.Name { + allErrs = append(allErrs, field.Forbidden(specPath.Child("initContainers").Index(i).Child("name"), "initContainers may not be renamed or reordered on resize")) + } + } + } + return allErrs +} + // dropCPUMemoryResourcesFromContainer deletes the cpu and memory resources from the container, and copies them from the old pod container resources if present. func dropCPUMemoryResourcesFromContainer(container *core.Container, oldPodSpecContainer *core.Container) { dropCPUMemoryUpdates := func(resourceList, oldResourceList core.ResourceList) core.ResourceList { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index f269cbaf96b..36024951421 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -26116,6 +26116,83 @@ func TestValidatePodResize(t *testing.T) { old: mkPodWithInitContainers(getResources("100m", "0", "1Gi", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways), new: mkPodWithInitContainers(getResources("100m", "0", "2Gi", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways), err: "spec: Forbidden: only cpu and memory resources for sidecar containers are mutable", + }, { + test: "pod container addition", + old: podtest.MakePod("pod", podtest.SetContainers( + podtest.MakeContainer( + "c1", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + )), + new: podtest.MakePod("pod", podtest.SetContainers( + podtest.MakeContainer( + "c1", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + podtest.MakeContainer( + "c2", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + )), + err: "spec.containers: Forbidden: containers may not be added or removed on resize", + }, { + test: "pod container removal", + old: podtest.MakePod("pod", podtest.SetContainers( + podtest.MakeContainer( + "c1", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + podtest.MakeContainer( + "c2", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + )), + new: podtest.MakePod("pod", podtest.SetContainers( + podtest.MakeContainer( + "c1", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + )), + err: "spec.containers: Forbidden: containers may not be added or removed on resize", + }, { + test: "pod container reorder", + old: podtest.MakePod("pod", podtest.SetContainers( + podtest.MakeContainer( + "c1", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + podtest.MakeContainer( + "c2", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + )), + new: podtest.MakePod("pod", podtest.SetContainers( + podtest.MakeContainer( + "c2", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + podtest.MakeContainer( + "c1", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + )), + err: "spec.containers[0].name: Forbidden: containers may not be renamed or reordered on resize, spec.containers[1].name: Forbidden: containers may not be renamed or reordered on resize", + }, + { + test: "pod container rename", + old: podtest.MakePod("pod", podtest.SetContainers( + podtest.MakeContainer( + "c1", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + )), + new: podtest.MakePod("pod", podtest.SetContainers( + podtest.MakeContainer( + "c2", + podtest.SetContainerResources(core.ResourceRequirements{}), + ), + )), + err: "spec.containers[0].name: Forbidden: containers may not be renamed or reordered on resize", }, { test: "change resize restart policy", old: mkPod(getResources("100m", "0", "1Gi", ""), core.ResourceList{}, resizePolicy(core.ResourceCPU, core.NotRequired)), diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index 5cd2ee19a05..5320526fcf7 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -244,16 +244,26 @@ func (podStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime. type podEphemeralContainersStrategy struct { podStrategy + + resetFieldsFilter fieldpath.Filter } // EphemeralContainersStrategy wraps and exports the used podStrategy for the storage package. -var EphemeralContainersStrategy = podEphemeralContainersStrategy{Strategy} +var EphemeralContainersStrategy = podEphemeralContainersStrategy{ + podStrategy: Strategy, + resetFieldsFilter: fieldpath.NewIncludeMatcherFilter( + fieldpath.MakePrefixMatcherOrDie("spec", "ephemeralContainers"), + ), +} // dropNonEphemeralContainerUpdates discards all changes except for pod.Spec.EphemeralContainers and certain metadata func dropNonEphemeralContainerUpdates(newPod, oldPod *api.Pod) *api.Pod { - pod := dropPodUpdates(newPod, oldPod) - pod.Spec.EphemeralContainers = newPod.Spec.EphemeralContainers - return pod + newEphemeralContainerSpec := newPod.Spec.EphemeralContainers + newPod.Spec = oldPod.Spec + newPod.Status = oldPod.Status + metav1.ResetObjectMetaForStatus(&newPod.ObjectMeta, &oldPod.ObjectMeta) + newPod.Spec.EphemeralContainers = newEphemeralContainerSpec + return newPod } func (podEphemeralContainersStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { @@ -278,6 +288,14 @@ func (podEphemeralContainersStrategy) WarningsOnUpdate(ctx context.Context, obj, return nil } +// GetResetFieldsFilter returns a set of fields filter reset by the strategy +// and should not be modified by the user. +func (p podEphemeralContainersStrategy) GetResetFieldsFilter() map[fieldpath.APIVersion]fieldpath.Filter { + return map[fieldpath.APIVersion]fieldpath.Filter{ + "v1": p.resetFieldsFilter, + } +} + type podResizeStrategy struct { podStrategy @@ -290,44 +308,54 @@ var ResizeStrategy = podResizeStrategy{ resetFieldsFilter: fieldpath.NewIncludeMatcherFilter( fieldpath.MakePrefixMatcherOrDie("spec", "containers", fieldpath.MatchAnyPathElement(), "resources"), fieldpath.MakePrefixMatcherOrDie("spec", "containers", fieldpath.MatchAnyPathElement(), "resizePolicy"), + fieldpath.MakePrefixMatcherOrDie("spec", "initContainers", fieldpath.MatchAnyPathElement(), "resources"), + fieldpath.MakePrefixMatcherOrDie("spec", "initContainers", fieldpath.MatchAnyPathElement(), "resizePolicy"), ), } // dropNonResizeUpdates discards all changes except for pod.Spec.Containers[*].Resources, pod.Spec.InitContainers[*].Resources, ResizePolicy and certain metadata func dropNonResizeUpdates(newPod, oldPod *api.Pod) *api.Pod { - pod := dropPodUpdates(newPod, oldPod) - - // Containers are not allowed to be re-ordered, but in case they were, - // we don't want to corrupt them here. It will get caught in validation. - oldCtrToIndex := make(map[string]int) - oldInitCtrToIndex := make(map[string]int) - for idx, ctr := range pod.Spec.Containers { - oldCtrToIndex[ctr.Name] = idx - } - for idx, ctr := range pod.Spec.InitContainers { - oldInitCtrToIndex[ctr.Name] = idx + // Containers are not allowed to be added, removed, re-ordered, or renamed. + // If we detect any of these changes, we will return new podspec as-is and + // allow the validation to catch the error and drop the update. + if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) || len(newPod.Spec.InitContainers) != len(oldPod.Spec.InitContainers) { + return newPod } - for _, ctr := range newPod.Spec.Containers { - idx, ok := oldCtrToIndex[ctr.Name] - if !ok { - continue - } - pod.Spec.Containers[idx].Resources = ctr.Resources - pod.Spec.Containers[idx].ResizePolicy = ctr.ResizePolicy - } + containers := dropNonResizeUpdatesForContainers(newPod.Spec.Containers, oldPod.Spec.Containers) + initContainers := dropNonResizeUpdatesForContainers(newPod.Spec.InitContainers, oldPod.Spec.InitContainers) + newPod.Spec = oldPod.Spec + newPod.Status = oldPod.Status + metav1.ResetObjectMetaForStatus(&newPod.ObjectMeta, &oldPod.ObjectMeta) + + newPod.Spec.Containers = containers if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) { - for _, ctr := range newPod.Spec.InitContainers { - idx, ok := oldInitCtrToIndex[ctr.Name] - if !ok { - continue - } - pod.Spec.InitContainers[idx].Resources = ctr.Resources - pod.Spec.InitContainers[idx].ResizePolicy = ctr.ResizePolicy - } + newPod.Spec.InitContainers = initContainers } - return pod + + return newPod +} + +func dropNonResizeUpdatesForContainers(new, old []api.Container) []api.Container { + if len(new) == 0 { + return new + } + + oldCopyWithMergedResources := make([]api.Container, len(old)) + copy(oldCopyWithMergedResources, old) + + for i, ctr := range new { + if oldCopyWithMergedResources[i].Name != new[i].Name { + // This is an attempt to reorder or rename a container, which is not allowed. + // Allow validation to catch this error. + return new + } + oldCopyWithMergedResources[i].Resources = ctr.Resources + oldCopyWithMergedResources[i].ResizePolicy = ctr.ResizePolicy + } + + return oldCopyWithMergedResources } func (podResizeStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { @@ -361,17 +389,6 @@ func (p podResizeStrategy) GetResetFieldsFilter() map[fieldpath.APIVersion]field } } -// dropPodUpdates drops any changes in the pod. -func dropPodUpdates(newPod, oldPod *api.Pod) *api.Pod { - pod := oldPod.DeepCopy() - pod.Name = newPod.Name - pod.Namespace = newPod.Namespace - pod.ResourceVersion = newPod.ResourceVersion - pod.UID = newPod.UID - - return pod -} - // GetAttrs returns labels and fields of a given object for filtering purposes. func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) { pod, ok := obj.(*api.Pod) diff --git a/pkg/registry/core/pod/strategy_test.go b/pkg/registry/core/pod/strategy_test.go index 6255c526b43..1f3a2289e88 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/pkg/registry/core/pod/strategy_test.go @@ -2643,15 +2643,25 @@ func TestPodResizePrepareForUpdate(t *testing.T) { ), expected: podtest.MakePod("test-pod", podtest.SetResourceVersion("2"), - podtest.SetContainers(podtest.MakeContainer("container1", - podtest.SetContainerResources(api.ResourceRequirements{ - Requests: api.ResourceList{ - api.ResourceCPU: resource.MustParse("100m"), - api.ResourceMemory: resource.MustParse("1Gi"), - }, - }), - )), - podtest.SetGeneration(1), + podtest.SetContainers( + podtest.MakeContainer("container1", + podtest.SetContainerResources(api.ResourceRequirements{ + Requests: api.ResourceList{ + api.ResourceCPU: resource.MustParse("100m"), + api.ResourceMemory: resource.MustParse("1Gi"), + }, + }), + ), + podtest.MakeContainer("container2", + podtest.SetContainerResources(api.ResourceRequirements{ + Requests: api.ResourceList{ + api.ResourceCPU: resource.MustParse("100m"), + api.ResourceMemory: resource.MustParse("1Gi"), + }, + }), + ), + ), + podtest.SetGeneration(2), podtest.SetStatus(podtest.MakePodStatus( podtest.SetContainerStatuses(podtest.MakeContainerStatus("container1", api.ResourceList{ @@ -2713,17 +2723,26 @@ func TestPodResizePrepareForUpdate(t *testing.T) { ), expected: podtest.MakePod("test-pod", podtest.SetResourceVersion("2"), - podtest.SetContainers(podtest.MakeContainer("container1", - podtest.SetContainerResources(api.ResourceRequirements{ - Requests: api.ResourceList{ - api.ResourceCPU: resource.MustParse("100m"), - api.ResourceMemory: resource.MustParse("2Gi"), // Updated resource - }, - }), - )), + podtest.SetContainers( + podtest.MakeContainer("container1", + podtest.SetContainerResources(api.ResourceRequirements{ + Requests: api.ResourceList{ + api.ResourceCPU: resource.MustParse("100m"), + api.ResourceMemory: resource.MustParse("2Gi"), // Updated resource + }, + }), + ), + podtest.MakeContainer("container2", + podtest.SetContainerResources(api.ResourceRequirements{ + Requests: api.ResourceList{ + api.ResourceCPU: resource.MustParse("100m"), + api.ResourceMemory: resource.MustParse("1Gi"), + }, + }), + ), + ), podtest.SetGeneration(2), podtest.SetStatus(podtest.MakePodStatus( - podtest.SetResizeStatus(api.PodResizeStatusProposed), // Resize status set podtest.SetContainerStatuses(podtest.MakeContainerStatus("container1", api.ResourceList{ api.ResourceCPU: resource.MustParse("100m"), @@ -2809,15 +2828,7 @@ func TestPodResizePrepareForUpdate(t *testing.T) { expected: podtest.MakePod("test-pod", podtest.SetResourceVersion("2"), podtest.SetContainers( - podtest.MakeContainer("container1", - podtest.SetContainerResources(api.ResourceRequirements{ - Requests: api.ResourceList{ - api.ResourceCPU: resource.MustParse("200m"), // Updated resource - api.ResourceMemory: resource.MustParse("4Gi"), // Updated resource - }, - }), - ), - podtest.MakeContainer("container2", + podtest.MakeContainer("container2", // Order changed podtest.SetContainerResources(api.ResourceRequirements{ Requests: api.ResourceList{ api.ResourceCPU: resource.MustParse("200m"), // Updated resource @@ -2825,10 +2836,17 @@ func TestPodResizePrepareForUpdate(t *testing.T) { }, }), ), + podtest.MakeContainer("container1", // Order changed + podtest.SetContainerResources(api.ResourceRequirements{ + Requests: api.ResourceList{ + api.ResourceCPU: resource.MustParse("200m"), // Updated resource + api.ResourceMemory: resource.MustParse("4Gi"), // Updated resource + }, + }), + ), ), podtest.SetGeneration(2), podtest.SetStatus(podtest.MakePodStatus( - podtest.SetResizeStatus(api.PodResizeStatusProposed), // Resize status set podtest.SetContainerStatuses( podtest.MakeContainerStatus("container1", api.ResourceList{ diff --git a/test/e2e/common/node/pod_resize.go b/test/e2e/common/node/pod_resize.go index 81f86b47b9e..5863980a191 100644 --- a/test/e2e/common/node/pod_resize.go +++ b/test/e2e/common/node/pod_resize.go @@ -1291,6 +1291,34 @@ func doPodResizeErrorTests() { }, }, }, + { + name: "Burstable QoS pod, two containers with cpu & memory requests + limits - reorder containers", + containers: []e2epod.ResizableContainerInfo{ + { + Name: "c1", + Resources: &e2epod.ContainerResources{CPUReq: originalCPU, CPULim: originalCPULimit, MemReq: originalMem, MemLim: originalMemLimit}, + }, + { + Name: "c2", + Resources: &e2epod.ContainerResources{CPUReq: originalCPU, CPULim: originalCPULimit, MemReq: originalMem, MemLim: originalMemLimit}, + }, + }, + patchString: fmt.Sprintf(`{"spec":{"containers":[ + {"name":"c2", "resources":{"requests":{"cpu":"%s","memory":"%s"},"limits":{"cpu":"%s","memory":"%s"}}}, + {"name":"c1", "resources":{"requests":{"cpu":"%s","memory":"%s"},"limits":{"cpu":"%s","memory":"%s"}}} + ]}}`, originalCPU, originalMem, originalCPULimit, originalMemLimit, originalCPU, originalMem, originalCPULimit, originalMemLimit), + patchError: "spec.containers[0].name: Forbidden: containers may not be renamed or reordered on resize, spec.containers[1].name: Forbidden: containers may not be renamed or reordered on resize", + expected: []e2epod.ResizableContainerInfo{ + { + Name: "c1", + Resources: &e2epod.ContainerResources{CPUReq: originalCPU, CPULim: originalCPULimit, MemReq: originalMem, MemLim: originalMemLimit}, + }, + { + Name: "c2", + Resources: &e2epod.ContainerResources{CPUReq: originalCPU, CPULim: originalCPULimit, MemReq: originalMem, MemLim: originalMemLimit}, + }, + }, + }, { name: "Burstable QoS pod with memory requests + limits - decrease memory limit", containers: []e2epod.ResizableContainerInfo{