fix prep and validation for pod subresource updates

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 {

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)),

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)
+	newEphemeralContainerSpec := newPod.Spec.EphemeralContainers
-	pod.Spec.EphemeralContainers = newPod.Spec.EphemeralContainers
+	newPod.Spec = oldPod.Spec
-	return pod
+	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 added, removed, re-ordered, or renamed.
-
+	// If we detect any of these changes, we will return new podspec as-is and
-	// Containers are not allowed to be re-ordered, but in case they were,
+	// allow the validation to catch the error and drop the update.
-	// we don't want to corrupt them here. It will get caught in validation.
+	if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) || len(newPod.Spec.InitContainers) != len(oldPod.Spec.InitContainers) {
-	oldCtrToIndex := make(map[string]int)
+		return newPod
-	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
 	}
 
-	for _, ctr := range newPod.Spec.Containers {
+	containers := dropNonResizeUpdatesForContainers(newPod.Spec.Containers, oldPod.Spec.Containers)
-		idx, ok := oldCtrToIndex[ctr.Name]
+	initContainers := dropNonResizeUpdatesForContainers(newPod.Spec.InitContainers, oldPod.Spec.InitContainers)
-		if !ok {
-			continue
-		}
-		pod.Spec.Containers[idx].Resources = ctr.Resources
-		pod.Spec.Containers[idx].ResizePolicy = ctr.ResizePolicy
-	}
 
+	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 {
+		newPod.Spec.InitContainers = initContainers
-			idx, ok := oldInitCtrToIndex[ctr.Name]
-			if !ok {
-				continue
-			}
-			pod.Spec.InitContainers[idx].Resources = ctr.Resources
-			pod.Spec.InitContainers[idx].ResizePolicy = ctr.ResizePolicy
-		}
 	}
-	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)

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.SetContainers(
-					podtest.SetContainerResources(api.ResourceRequirements{
+					podtest.MakeContainer("container1",
-						Requests: api.ResourceList{
+						podtest.SetContainerResources(api.ResourceRequirements{
-							api.ResourceCPU:    resource.MustParse("100m"),
+							Requests: api.ResourceList{
-							api.ResourceMemory: resource.MustParse("1Gi"),
+								api.ResourceCPU:    resource.MustParse("100m"),
-						},
+								api.ResourceMemory: resource.MustParse("1Gi"),
-					}),
+							},
-				)),
+						}),
-				podtest.SetGeneration(1),
+					),
+					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.SetContainers(
-					podtest.SetContainerResources(api.ResourceRequirements{
+					podtest.MakeContainer("container1",
-						Requests: api.ResourceList{
+						podtest.SetContainerResources(api.ResourceRequirements{
-							api.ResourceCPU:    resource.MustParse("100m"),
+							Requests: api.ResourceList{
-							api.ResourceMemory: resource.MustParse("2Gi"), // Updated resource
+								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.MakeContainer("container2", // Order changed
-						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.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{

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{