Skip to content

Commit cd451c6

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#130282 from natasha41575/podresizevalidation
Clean up preparation for pod subresource updates
2 parents 8873c7e + f91105a commit cd451c6

File tree

5 files changed

+237
-64
lines changed

5 files changed

+237
-64
lines changed

‎pkg/apis/core/validation/validation.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5625,6 +5625,14 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel
56255625
allErrs = append(allErrs, field.Forbidden(specPath, "Pod running on node without support for resize"))
56265626
}
56275627

5628+
// The rest of the validation assumes that the containers are in the same order,
5629+
// so we proceed only if that assumption is true.
5630+
containerOrderErrs := validatePodResizeContainerOrdering(newPod, oldPod, specPath)
5631+
allErrs = append(allErrs, containerOrderErrs...)
5632+
if containerOrderErrs != nil {
5633+
return allErrs
5634+
}
5635+
56285636
// Do not allow removing resource requests/limits on resize.
56295637
if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
56305638
for ix, ctr := range oldPod.Spec.InitContainers {
@@ -5694,6 +5702,31 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel
56945702
return allErrs
56955703
}
56965704

5705+
// validatePodResizeContainerOrdering validates container ordering for a resize request.
5706+
// We do not allow adding, removing, re-ordering, or renaming containers on resize.
5707+
func validatePodResizeContainerOrdering(newPod, oldPod *core.Pod, specPath *field.Path) field.ErrorList {
5708+
var allErrs field.ErrorList
5709+
if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) {
5710+
allErrs = append(allErrs, field.Forbidden(specPath.Child("containers"), "containers may not be added or removed on resize"))
5711+
} else {
5712+
for i, oldCtr := range oldPod.Spec.Containers {
5713+
if newPod.Spec.Containers[i].Name != oldCtr.Name {
5714+
allErrs = append(allErrs, field.Forbidden(specPath.Child("containers").Index(i).Child("name"), "containers may not be renamed or reordered on resize"))
5715+
}
5716+
}
5717+
}
5718+
if len(newPod.Spec.InitContainers) != len(oldPod.Spec.InitContainers) {
5719+
allErrs = append(allErrs, field.Forbidden(specPath.Child("initContainers"), "initContainers may not be added or removed on resize"))
5720+
} else {
5721+
for i, oldCtr := range oldPod.Spec.InitContainers {
5722+
if newPod.Spec.InitContainers[i].Name != oldCtr.Name {
5723+
allErrs = append(allErrs, field.Forbidden(specPath.Child("initContainers").Index(i).Child("name"), "initContainers may not be renamed or reordered on resize"))
5724+
}
5725+
}
5726+
}
5727+
return allErrs
5728+
}
5729+
56975730
// dropCPUMemoryResourcesFromContainer deletes the cpu and memory resources from the container, and copies them from the old pod container resources if present.
56985731
func dropCPUMemoryResourcesFromContainer(container *core.Container, oldPodSpecContainer *core.Container) {
56995732
dropCPUMemoryUpdates := func(resourceList, oldResourceList core.ResourceList) core.ResourceList {

‎pkg/apis/core/validation/validation_test.go‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26163,6 +26163,83 @@ func TestValidatePodResize(t *testing.T) {
2616326163
old: mkPodWithInitContainers(getResources("100m", "0", "1Gi", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways),
2616426164
new: mkPodWithInitContainers(getResources("100m", "0", "2Gi", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways),
2616526165
err: "spec: Forbidden: only cpu and memory resources for sidecar containers are mutable",
26166+
}, {
26167+
test: "pod container addition",
26168+
old: podtest.MakePod("pod", podtest.SetContainers(
26169+
podtest.MakeContainer(
26170+
"c1",
26171+
podtest.SetContainerResources(core.ResourceRequirements{}),
26172+
),
26173+
)),
26174+
new: podtest.MakePod("pod", podtest.SetContainers(
26175+
podtest.MakeContainer(
26176+
"c1",
26177+
podtest.SetContainerResources(core.ResourceRequirements{}),
26178+
),
26179+
podtest.MakeContainer(
26180+
"c2",
26181+
podtest.SetContainerResources(core.ResourceRequirements{}),
26182+
),
26183+
)),
26184+
err: "spec.containers: Forbidden: containers may not be added or removed on resize",
26185+
}, {
26186+
test: "pod container removal",
26187+
old: podtest.MakePod("pod", podtest.SetContainers(
26188+
podtest.MakeContainer(
26189+
"c1",
26190+
podtest.SetContainerResources(core.ResourceRequirements{}),
26191+
),
26192+
podtest.MakeContainer(
26193+
"c2",
26194+
podtest.SetContainerResources(core.ResourceRequirements{}),
26195+
),
26196+
)),
26197+
new: podtest.MakePod("pod", podtest.SetContainers(
26198+
podtest.MakeContainer(
26199+
"c1",
26200+
podtest.SetContainerResources(core.ResourceRequirements{}),
26201+
),
26202+
)),
26203+
err: "spec.containers: Forbidden: containers may not be added or removed on resize",
26204+
}, {
26205+
test: "pod container reorder",
26206+
old: podtest.MakePod("pod", podtest.SetContainers(
26207+
podtest.MakeContainer(
26208+
"c1",
26209+
podtest.SetContainerResources(core.ResourceRequirements{}),
26210+
),
26211+
podtest.MakeContainer(
26212+
"c2",
26213+
podtest.SetContainerResources(core.ResourceRequirements{}),
26214+
),
26215+
)),
26216+
new: podtest.MakePod("pod", podtest.SetContainers(
26217+
podtest.MakeContainer(
26218+
"c2",
26219+
podtest.SetContainerResources(core.ResourceRequirements{}),
26220+
),
26221+
podtest.MakeContainer(
26222+
"c1",
26223+
podtest.SetContainerResources(core.ResourceRequirements{}),
26224+
),
26225+
)),
26226+
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",
26227+
},
26228+
{
26229+
test: "pod container rename",
26230+
old: podtest.MakePod("pod", podtest.SetContainers(
26231+
podtest.MakeContainer(
26232+
"c1",
26233+
podtest.SetContainerResources(core.ResourceRequirements{}),
26234+
),
26235+
)),
26236+
new: podtest.MakePod("pod", podtest.SetContainers(
26237+
podtest.MakeContainer(
26238+
"c2",
26239+
podtest.SetContainerResources(core.ResourceRequirements{}),
26240+
),
26241+
)),
26242+
err: "spec.containers[0].name: Forbidden: containers may not be renamed or reordered on resize",
2616626243
}, {
2616726244
test: "change resize restart policy",
2616826245
old: mkPod(getResources("100m", "0", "1Gi", ""), core.ResourceList{}, resizePolicy(core.ResourceCPU, core.NotRequired)),

‎pkg/registry/core/pod/strategy.go‎

Lines changed: 58 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -244,16 +244,26 @@ func (podStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.
244244

245245
type podEphemeralContainersStrategy struct {
246246
podStrategy
247+
248+
resetFieldsFilter fieldpath.Filter
247249
}
248250

249251
// EphemeralContainersStrategy wraps and exports the used podStrategy for the storage package.
250-
var EphemeralContainersStrategy = podEphemeralContainersStrategy{Strategy}
252+
var EphemeralContainersStrategy = podEphemeralContainersStrategy{
253+
podStrategy: Strategy,
254+
resetFieldsFilter: fieldpath.NewIncludeMatcherFilter(
255+
fieldpath.MakePrefixMatcherOrDie("spec", "ephemeralContainers"),
256+
),
257+
}
251258

252259
// dropNonEphemeralContainerUpdates discards all changes except for pod.Spec.EphemeralContainers and certain metadata
253260
func dropNonEphemeralContainerUpdates(newPod, oldPod *api.Pod) *api.Pod {
254-
pod := dropPodUpdates(newPod, oldPod)
255-
pod.Spec.EphemeralContainers = newPod.Spec.EphemeralContainers
256-
return pod
261+
newEphemeralContainerSpec := newPod.Spec.EphemeralContainers
262+
newPod.Spec = oldPod.Spec
263+
newPod.Status = oldPod.Status
264+
metav1.ResetObjectMetaForStatus(&newPod.ObjectMeta, &oldPod.ObjectMeta)
265+
newPod.Spec.EphemeralContainers = newEphemeralContainerSpec
266+
return newPod
257267
}
258268

259269
func (podEphemeralContainersStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
@@ -278,6 +288,14 @@ func (podEphemeralContainersStrategy) WarningsOnUpdate(ctx context.Context, obj,
278288
return nil
279289
}
280290

291+
// GetResetFieldsFilter returns a set of fields filter reset by the strategy
292+
// and should not be modified by the user.
293+
func (p podEphemeralContainersStrategy) GetResetFieldsFilter() map[fieldpath.APIVersion]fieldpath.Filter {
294+
return map[fieldpath.APIVersion]fieldpath.Filter{
295+
"v1": p.resetFieldsFilter,
296+
}
297+
}
298+
281299
type podResizeStrategy struct {
282300
podStrategy
283301

@@ -290,44 +308,54 @@ var ResizeStrategy = podResizeStrategy{
290308
resetFieldsFilter: fieldpath.NewIncludeMatcherFilter(
291309
fieldpath.MakePrefixMatcherOrDie("spec", "containers", fieldpath.MatchAnyPathElement(), "resources"),
292310
fieldpath.MakePrefixMatcherOrDie("spec", "containers", fieldpath.MatchAnyPathElement(), "resizePolicy"),
311+
fieldpath.MakePrefixMatcherOrDie("spec", "initContainers", fieldpath.MatchAnyPathElement(), "resources"),
312+
fieldpath.MakePrefixMatcherOrDie("spec", "initContainers", fieldpath.MatchAnyPathElement(), "resizePolicy"),
293313
),
294314
}
295315

296316
// dropNonResizeUpdates discards all changes except for pod.Spec.Containers[*].Resources, pod.Spec.InitContainers[*].Resources, ResizePolicy and certain metadata
297317
func dropNonResizeUpdates(newPod, oldPod *api.Pod) *api.Pod {
298-
pod := dropPodUpdates(newPod, oldPod)
299-
300-
// Containers are not allowed to be re-ordered, but in case they were,
301-
// we don't want to corrupt them here. It will get caught in validation.
302-
oldCtrToIndex := make(map[string]int)
303-
oldInitCtrToIndex := make(map[string]int)
304-
for idx, ctr := range pod.Spec.Containers {
305-
oldCtrToIndex[ctr.Name] = idx
318+
// Containers are not allowed to be added, removed, re-ordered, or renamed.
319+
// If we detect any of these changes, we will return new podspec as-is and
320+
// allow the validation to catch the error and drop the update.
321+
if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) || len(newPod.Spec.InitContainers) != len(oldPod.Spec.InitContainers) {
322+
return newPod
306323
}
307-
for idx, ctr := range pod.Spec.InitContainers {
308-
oldInitCtrToIndex[ctr.Name] = idx
324+
325+
containers := dropNonResizeUpdatesForContainers(newPod.Spec.Containers, oldPod.Spec.Containers)
326+
initContainers := dropNonResizeUpdatesForContainers(newPod.Spec.InitContainers, oldPod.Spec.InitContainers)
327+
328+
newPod.Spec = oldPod.Spec
329+
newPod.Status = oldPod.Status
330+
metav1.ResetObjectMetaForStatus(&newPod.ObjectMeta, &oldPod.ObjectMeta)
331+
332+
newPod.Spec.Containers = containers
333+
if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
334+
newPod.Spec.InitContainers = initContainers
309335
}
310336

311-
for _, ctr := range newPod.Spec.Containers {
312-
idx, ok := oldCtrToIndex[ctr.Name]
313-
if !ok {
314-
continue
315-
}
316-
pod.Spec.Containers[idx].Resources = ctr.Resources
317-
pod.Spec.Containers[idx].ResizePolicy = ctr.ResizePolicy
337+
return newPod
338+
}
339+
340+
func dropNonResizeUpdatesForContainers(new, old []api.Container) []api.Container {
341+
if len(new) == 0 {
342+
return new
318343
}
319344

320-
if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
321-
for _, ctr := range newPod.Spec.InitContainers {
322-
idx, ok := oldInitCtrToIndex[ctr.Name]
323-
if !ok {
324-
continue
325-
}
326-
pod.Spec.InitContainers[idx].Resources = ctr.Resources
327-
pod.Spec.InitContainers[idx].ResizePolicy = ctr.ResizePolicy
345+
oldCopyWithMergedResources := make([]api.Container, len(old))
346+
copy(oldCopyWithMergedResources, old)
347+
348+
for i, ctr := range new {
349+
if oldCopyWithMergedResources[i].Name != new[i].Name {
350+
// This is an attempt to reorder or rename a container, which is not allowed.
351+
// Allow validation to catch this error.
352+
return new
328353
}
354+
oldCopyWithMergedResources[i].Resources = ctr.Resources
355+
oldCopyWithMergedResources[i].ResizePolicy = ctr.ResizePolicy
329356
}
330-
return pod
357+
358+
return oldCopyWithMergedResources
331359
}
332360

333361
func (podResizeStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
@@ -361,17 +389,6 @@ func (p podResizeStrategy) GetResetFieldsFilter() map[fieldpath.APIVersion]field
361389
}
362390
}
363391

364-
// dropPodUpdates drops any changes in the pod.
365-
func dropPodUpdates(newPod, oldPod *api.Pod) *api.Pod {
366-
pod := oldPod.DeepCopy()
367-
pod.Name = newPod.Name
368-
pod.Namespace = newPod.Namespace
369-
pod.ResourceVersion = newPod.ResourceVersion
370-
pod.UID = newPod.UID
371-
372-
return pod
373-
}
374-
375392
// GetAttrs returns labels and fields of a given object for filtering purposes.
376393
func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) {
377394
pod, ok := obj.(*api.Pod)

‎pkg/registry/core/pod/strategy_test.go‎

Lines changed: 41 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2643,15 +2643,25 @@ func TestPodResizePrepareForUpdate(t *testing.T) {
26432643
),
26442644
expected: podtest.MakePod("test-pod",
26452645
podtest.SetResourceVersion("2"),
2646-
podtest.SetContainers(podtest.MakeContainer("container1",
2647-
podtest.SetContainerResources(api.ResourceRequirements{
2648-
Requests: api.ResourceList{
2649-
api.ResourceCPU: resource.MustParse("100m"),
2650-
api.ResourceMemory: resource.MustParse("1Gi"),
2651-
},
2652-
}),
2653-
)),
2654-
podtest.SetGeneration(1),
2646+
podtest.SetContainers(
2647+
podtest.MakeContainer("container1",
2648+
podtest.SetContainerResources(api.ResourceRequirements{
2649+
Requests: api.ResourceList{
2650+
api.ResourceCPU: resource.MustParse("100m"),
2651+
api.ResourceMemory: resource.MustParse("1Gi"),
2652+
},
2653+
}),
2654+
),
2655+
podtest.MakeContainer("container2",
2656+
podtest.SetContainerResources(api.ResourceRequirements{
2657+
Requests: api.ResourceList{
2658+
api.ResourceCPU: resource.MustParse("100m"),
2659+
api.ResourceMemory: resource.MustParse("1Gi"),
2660+
},
2661+
}),
2662+
),
2663+
),
2664+
podtest.SetGeneration(2),
26552665
podtest.SetStatus(podtest.MakePodStatus(
26562666
podtest.SetContainerStatuses(podtest.MakeContainerStatus("container1",
26572667
api.ResourceList{
@@ -2713,17 +2723,26 @@ func TestPodResizePrepareForUpdate(t *testing.T) {
27132723
),
27142724
expected: podtest.MakePod("test-pod",
27152725
podtest.SetResourceVersion("2"),
2716-
podtest.SetContainers(podtest.MakeContainer("container1",
2717-
podtest.SetContainerResources(api.ResourceRequirements{
2718-
Requests: api.ResourceList{
2719-
api.ResourceCPU: resource.MustParse("100m"),
2720-
api.ResourceMemory: resource.MustParse("2Gi"), // Updated resource
2721-
},
2722-
}),
2723-
)),
2726+
podtest.SetContainers(
2727+
podtest.MakeContainer("container1",
2728+
podtest.SetContainerResources(api.ResourceRequirements{
2729+
Requests: api.ResourceList{
2730+
api.ResourceCPU: resource.MustParse("100m"),
2731+
api.ResourceMemory: resource.MustParse("2Gi"), // Updated resource
2732+
},
2733+
}),
2734+
),
2735+
podtest.MakeContainer("container2",
2736+
podtest.SetContainerResources(api.ResourceRequirements{
2737+
Requests: api.ResourceList{
2738+
api.ResourceCPU: resource.MustParse("100m"),
2739+
api.ResourceMemory: resource.MustParse("1Gi"),
2740+
},
2741+
}),
2742+
),
2743+
),
27242744
podtest.SetGeneration(2),
27252745
podtest.SetStatus(podtest.MakePodStatus(
2726-
podtest.SetResizeStatus(api.PodResizeStatusProposed), // Resize status set
27272746
podtest.SetContainerStatuses(podtest.MakeContainerStatus("container1",
27282747
api.ResourceList{
27292748
api.ResourceCPU: resource.MustParse("100m"),
@@ -2809,26 +2828,25 @@ func TestPodResizePrepareForUpdate(t *testing.T) {
28092828
expected: podtest.MakePod("test-pod",
28102829
podtest.SetResourceVersion("2"),
28112830
podtest.SetContainers(
2812-
podtest.MakeContainer("container1",
2831+
podtest.MakeContainer("container2", // Order changed
28132832
podtest.SetContainerResources(api.ResourceRequirements{
28142833
Requests: api.ResourceList{
28152834
api.ResourceCPU: resource.MustParse("200m"), // Updated resource
2816-
api.ResourceMemory: resource.MustParse("4Gi"), // Updated resource
2835+
api.ResourceMemory: resource.MustParse("2Gi"), // Updated resource
28172836
},
28182837
}),
28192838
),
2820-
podtest.MakeContainer("container2",
2839+
podtest.MakeContainer("container1", // Order changed
28212840
podtest.SetContainerResources(api.ResourceRequirements{
28222841
Requests: api.ResourceList{
28232842
api.ResourceCPU: resource.MustParse("200m"), // Updated resource
2824-
api.ResourceMemory: resource.MustParse("2Gi"), // Updated resource
2843+
api.ResourceMemory: resource.MustParse("4Gi"), // Updated resource
28252844
},
28262845
}),
28272846
),
28282847
),
28292848
podtest.SetGeneration(2),
28302849
podtest.SetStatus(podtest.MakePodStatus(
2831-
podtest.SetResizeStatus(api.PodResizeStatusProposed), // Resize status set
28322850
podtest.SetContainerStatuses(
28332851
podtest.MakeContainerStatus("container1",
28342852
api.ResourceList{

0 commit comments

Comments
 (0)