Skip to content

Commit a1595d9

Browse files
tallclairtallclair
committed
Don't allow memory limit decrease unless resize policy is RestartContainer
1 parent 9f26291 commit a1595d9

File tree

3 files changed

+163
-59
lines changed

3 files changed

+163
-59
lines changed

‎pkg/api/pod/testing/make.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,3 +335,20 @@ func SetResizeStatus(resizeStatus api.PodResizeStatus) TweakPodStatus {
335335
podstatus.Resize = resizeStatus
336336
}
337337
}
338+
339+
// TweakContainers applies the container tweaks to all containers in the pod with a masked type.
340+
// Note: this should typically be added to pod tweaks after all containers have been added.
341+
func TweakContainers(tweaks ...TweakContainer) Tweak {
342+
return func(pod *api.Pod) {
343+
for i := range pod.Spec.InitContainers {
344+
for _, tweak := range tweaks {
345+
tweak(&pod.Spec.InitContainers[i])
346+
}
347+
}
348+
for i := range pod.Spec.Containers {
349+
for _, tweak := range tweaks {
350+
tweak(&pod.Spec.Containers[i])
351+
}
352+
}
353+
}
354+
}

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

Lines changed: 54 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5622,21 +5622,19 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel
56225622
if !isRestartableInitContainer(&ctr) {
56235623
continue
56245624
}
5625-
if resourcesRemoved(newPod.Spec.InitContainers[ix].Resources.Requests, ctr.Resources.Requests) {
5626-
allErrs = append(allErrs, field.Forbidden(specPath.Child("initContainers").Index(ix).Child("resources").Child("requests"), "resource requests cannot be removed"))
5627-
}
5628-
if resourcesRemoved(newPod.Spec.InitContainers[ix].Resources.Limits, ctr.Resources.Limits) {
5629-
allErrs = append(allErrs, field.Forbidden(specPath.Child("initContainers").Index(ix).Child("resources").Child("limits"), "resource limits cannot be removed"))
5630-
}
5625+
allErrs = append(allErrs, validateContainerResize(
5626+
&newPod.Spec.InitContainers[ix].Resources,
5627+
&oldPod.Spec.InitContainers[ix].Resources,
5628+
newPod.Spec.InitContainers[ix].ResizePolicy,
5629+
specPath.Child("initContainers").Index(ix).Child("resources"))...)
56315630
}
56325631
}
5633-
for ix, ctr := range oldPod.Spec.Containers {
5634-
if resourcesRemoved(newPod.Spec.Containers[ix].Resources.Requests, ctr.Resources.Requests) {
5635-
allErrs = append(allErrs, field.Forbidden(specPath.Child("containers").Index(ix).Child("resources").Child("requests"), "resource requests cannot be removed"))
5636-
}
5637-
if resourcesRemoved(newPod.Spec.Containers[ix].Resources.Limits, ctr.Resources.Limits) {
5638-
allErrs = append(allErrs, field.Forbidden(specPath.Child("containers").Index(ix).Child("resources").Child("limits"), "resource limits cannot be removed"))
5639-
}
5632+
for ix := range oldPod.Spec.Containers {
5633+
allErrs = append(allErrs, validateContainerResize(
5634+
&newPod.Spec.Containers[ix].Resources,
5635+
&oldPod.Spec.Containers[ix].Resources,
5636+
newPod.Spec.Containers[ix].ResizePolicy,
5637+
specPath.Child("containers").Index(ix).Child("resources"))...)
56405638
}
56415639

56425640
// Ensure that only CPU and memory resources are mutable for regular containers.
@@ -5732,6 +5730,49 @@ func isPodResizeRequestSupported(pod core.Pod) bool {
57325730
return true
57335731
}
57345732

5733+
// validateContainerResize validates the changes to the container's resource requirements for a pod resize request.
5734+
// newRequriements and oldRequirements must be non-nil.
5735+
func validateContainerResize(newRequirements, oldRequirements *core.ResourceRequirements, resizePolicies []core.ContainerResizePolicy, fldPath *field.Path) field.ErrorList {
5736+
var allErrs field.ErrorList
5737+
5738+
// Removing resource requirements is not supported.
5739+
if resourcesRemoved(newRequirements.Requests, oldRequirements.Requests) {
5740+
allErrs = append(allErrs, field.Forbidden(fldPath.Child("requests"), "resource requests cannot be removed"))
5741+
}
5742+
if resourcesRemoved(newRequirements.Limits, oldRequirements.Limits) {
5743+
allErrs = append(allErrs, field.Forbidden(fldPath.Child("limits"), "resource limits cannot be removed"))
5744+
}
5745+
5746+
// Special case: memory limits may not be decreased if resize policy is NotRequired.
5747+
var memRestartPolicy core.ResourceResizeRestartPolicy
5748+
for _, policy := range resizePolicies {
5749+
if policy.ResourceName == core.ResourceMemory {
5750+
memRestartPolicy = policy.RestartPolicy
5751+
break
5752+
}
5753+
}
5754+
if memRestartPolicy == core.NotRequired || memRestartPolicy == "" {
5755+
newLimit, hasNewLimit := newRequirements.Limits[core.ResourceMemory]
5756+
oldLimit, hasOldLimit := oldRequirements.Limits[core.ResourceMemory]
5757+
if hasNewLimit && hasOldLimit {
5758+
if newLimit.Cmp(oldLimit) < 0 {
5759+
allErrs = append(allErrs, field.Forbidden(
5760+
fldPath.Child("limits").Key(core.ResourceMemory.String()),
5761+
fmt.Sprintf("memory limits cannot be decreased unless resizePolicy is %s", core.RestartContainer)))
5762+
}
5763+
} else if hasNewLimit && !hasOldLimit {
5764+
// Adding a memory limit is implicitly decreasing the memory limit (from 'max')
5765+
allErrs = append(allErrs, field.Forbidden(
5766+
fldPath.Child("limits").Key(core.ResourceMemory.String()),
5767+
fmt.Sprintf("memory limits cannot be added unless resizePolicy is %s", core.RestartContainer)))
5768+
}
5769+
}
5770+
5771+
// TODO(tallclair): Move resizable resource checks here.
5772+
5773+
return allErrs
5774+
}
5775+
57355776
func resourcesRemoved(resourceList, oldResourceList core.ResourceList) bool {
57365777
if len(oldResourceList) > len(resourceList) {
57375778
return true

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

Lines changed: 92 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -25607,7 +25607,7 @@ func TestValidateSELinuxChangePolicy(t *testing.T) {
2560725607

2560825608
func TestValidatePodResize(t *testing.T) {
2560925609
mkPod := func(req, lim core.ResourceList, tweaks ...podtest.Tweak) *core.Pod {
25610-
return podtest.MakePod("pod", append(tweaks,
25610+
allTweaks := []podtest.Tweak{
2561125611
podtest.SetContainers(
2561225612
podtest.MakeContainer(
2561325613
"container",
@@ -25619,11 +25619,14 @@ func TestValidatePodResize(t *testing.T) {
2561925619
),
2562025620
),
2562125621
),
25622-
)...)
25622+
}
25623+
// Prepend the SetContainers call so TweakContainers can be used.
25624+
allTweaks = append(allTweaks, tweaks...)
25625+
return podtest.MakePod("pod", allTweaks...)
2562325626
}
2562425627

2562525628
mkPodWithInitContainers := func(req, lim core.ResourceList, restartPolicy core.ContainerRestartPolicy, tweaks ...podtest.Tweak) *core.Pod {
25626-
return podtest.MakePod("pod", append(tweaks,
25629+
allTweaks := []podtest.Tweak{
2562725630
podtest.SetInitContainers(
2562825631
podtest.MakeContainer(
2562925632
"container",
@@ -25636,7 +25639,18 @@ func TestValidatePodResize(t *testing.T) {
2563625639
podtest.SetContainerRestartPolicy(restartPolicy),
2563725640
),
2563825641
),
25639-
)...)
25642+
}
25643+
// Prepend the SetInitContainers call so TweakContainers can be used.
25644+
allTweaks = append(allTweaks, tweaks...)
25645+
return podtest.MakePod("pod", allTweaks...)
25646+
}
25647+
25648+
resizePolicy := func(resource core.ResourceName, policy core.ResourceResizeRestartPolicy) podtest.Tweak {
25649+
return podtest.TweakContainers(podtest.SetContainerResizePolicy(
25650+
core.ContainerResizePolicy{
25651+
ResourceName: resource,
25652+
RestartPolicy: policy,
25653+
}))
2564025654
}
2564125655

2564225656
tests := []struct {
@@ -25667,14 +25681,14 @@ func TestValidatePodResize(t *testing.T) {
2566725681
new: podtest.MakePod("pod",
2566825682
podtest.SetContainers(podtest.MakeContainer("container",
2566925683
podtest.SetContainerResources(core.ResourceRequirements{
25670-
Limits: getResources("100m", "100Mi", "", ""),
25684+
Limits: getResources("100m", "200Mi", "", ""),
2567125685
}))),
2567225686
podtest.SetPodResources(&core.ResourceRequirements{Limits: getResources("100m", "200Mi", "", "")}),
2567325687
),
2567425688
old: podtest.MakePod("pod",
2567525689
podtest.SetContainers(podtest.MakeContainer("container",
2567625690
podtest.SetContainerResources(core.ResourceRequirements{
25677-
Limits: getResources("100m", "200Mi", "", ""),
25691+
Limits: getResources("100m", "100Mi", "", ""),
2567825692
}))),
2567925693
podtest.SetPodResources(&core.ResourceRequirements{Limits: getResources("100m", "200Mi", "", "")}),
2568025694
),
@@ -25793,9 +25807,24 @@ func TestValidatePodResize(t *testing.T) {
2579325807
new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")),
2579425808
err: "",
2579525809
}, {
25796-
test: "memory limit change",
25810+
test: "memory limit increase",
25811+
old: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")),
25812+
new: mkPod(core.ResourceList{}, getResources("100m", "200Mi", "", "")),
25813+
err: "",
25814+
}, {
25815+
test: "no restart policy: memory limit decrease",
2579725816
old: mkPod(core.ResourceList{}, getResources("100m", "200Mi", "", "")),
2579825817
new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")),
25818+
err: "memory limits cannot be decreased",
25819+
}, {
25820+
test: "restart NotRequired: memory limit decrease",
25821+
old: mkPod(core.ResourceList{}, getResources("100m", "200Mi", "", ""), resizePolicy("memory", core.NotRequired)),
25822+
new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", ""), resizePolicy("memory", core.NotRequired)),
25823+
err: "memory limits cannot be decreased",
25824+
}, {
25825+
test: "RestartContainer: memory limit decrease",
25826+
old: mkPod(core.ResourceList{}, getResources("100m", "200Mi", "", ""), resizePolicy("memory", core.RestartContainer)),
25827+
new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", ""), resizePolicy("memory", core.RestartContainer)),
2579925828
err: "",
2580025829
}, {
2580125830
test: "storage limit change",
@@ -25824,13 +25853,13 @@ func TestValidatePodResize(t *testing.T) {
2582425853
err: "",
2582525854
}, {
2582625855
test: "Pod QoS unchanged, burstable -> burstable",
25827-
old: mkPod(getResources("200m", "200Mi", "1Gi", ""), getResources("400m", "400Mi", "2Gi", "")),
25828-
new: mkPod(getResources("100m", "100Mi", "1Gi", ""), getResources("200m", "200Mi", "2Gi", "")),
25856+
old: mkPod(getResources("200m", "100Mi", "1Gi", ""), getResources("400m", "200Mi", "2Gi", "")),
25857+
new: mkPod(getResources("100m", "200Mi", "1Gi", ""), getResources("200m", "400Mi", "2Gi", "")),
2582925858
err: "",
2583025859
}, {
2583125860
test: "Pod QoS unchanged, burstable -> burstable, add limits",
2583225861
old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}),
25833-
new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")),
25862+
new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "", "", "")),
2583425863
err: "",
2583525864
}, {
2583625865
test: "Pod QoS unchanged, burstable -> burstable, add requests",
@@ -26005,9 +26034,24 @@ func TestValidatePodResize(t *testing.T) {
2600526034
new: mkPodWithInitContainers(core.ResourceList{}, getResources("200m", "0", "1Gi", ""), core.ContainerRestartPolicyAlways),
2600626035
err: "",
2600726036
}, {
26008-
test: "memory limit change for sidecar containers",
26037+
test: "memory limit increase for sidecar containers",
26038+
old: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "", ""), core.ContainerRestartPolicyAlways),
26039+
new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "200Mi", "", ""), core.ContainerRestartPolicyAlways),
26040+
err: "",
26041+
}, {
26042+
test: "memory limit decrease for sidecar containers, no resize policy",
2600926043
old: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "200Mi", "", ""), core.ContainerRestartPolicyAlways),
2601026044
new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "", ""), core.ContainerRestartPolicyAlways),
26045+
err: "memory limits cannot be decreased",
26046+
}, {
26047+
test: "memory limit decrease for sidecar containers, resize policy NotRequired",
26048+
old: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "200Mi", "", ""), core.ContainerRestartPolicyAlways, resizePolicy(core.ResourceMemory, core.NotRequired)),
26049+
new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "", ""), core.ContainerRestartPolicyAlways, resizePolicy(core.ResourceMemory, core.NotRequired)),
26050+
err: "memory limits cannot be decreased",
26051+
}, {
26052+
test: "memory limit decrease for sidecar containers, resize policy RestartContainer",
26053+
old: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "200Mi", "", ""), core.ContainerRestartPolicyAlways, resizePolicy(core.ResourceMemory, core.RestartContainer)),
26054+
new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "", ""), core.ContainerRestartPolicyAlways, resizePolicy(core.ResourceMemory, core.RestartContainer)),
2601126055
err: "",
2601226056
}, {
2601326057
test: "storage limit change for sidecar containers",
@@ -26033,42 +26077,44 @@ func TestValidatePodResize(t *testing.T) {
2603326077
}
2603426078

2603526079
for _, test := range tests {
26036-
test.new.ObjectMeta.ResourceVersion = "1"
26037-
test.old.ObjectMeta.ResourceVersion = "1"
26038-
26039-
// set required fields if old and new match and have no opinion on the value
26040-
if test.new.Name == "" && test.old.Name == "" {
26041-
test.new.Name = "name"
26042-
test.old.Name = "name"
26043-
}
26044-
if test.new.Namespace == "" && test.old.Namespace == "" {
26045-
test.new.Namespace = "namespace"
26046-
test.old.Namespace = "namespace"
26047-
}
26048-
if test.new.Spec.Containers == nil && test.old.Spec.Containers == nil {
26049-
test.new.Spec.Containers = []core.Container{{Name: "autoadded", Image: "image", TerminationMessagePolicy: "File", ImagePullPolicy: "Always"}}
26050-
test.old.Spec.Containers = []core.Container{{Name: "autoadded", Image: "image", TerminationMessagePolicy: "File", ImagePullPolicy: "Always"}}
26051-
}
26052-
if len(test.new.Spec.DNSPolicy) == 0 && len(test.old.Spec.DNSPolicy) == 0 {
26053-
test.new.Spec.DNSPolicy = core.DNSClusterFirst
26054-
test.old.Spec.DNSPolicy = core.DNSClusterFirst
26055-
}
26056-
if len(test.new.Spec.RestartPolicy) == 0 && len(test.old.Spec.RestartPolicy) == 0 {
26057-
test.new.Spec.RestartPolicy = "Always"
26058-
test.old.Spec.RestartPolicy = "Always"
26059-
}
26060-
26061-
errs := ValidatePodResize(test.new, test.old, PodValidationOptions{})
26062-
if test.err == "" {
26063-
if len(errs) != 0 {
26064-
t.Errorf("unexpected invalid: %s (%+v)\nA: %+v\nB: %+v", test.test, errs, test.new, test.old)
26080+
t.Run(test.test, func(t *testing.T) {
26081+
test.new.ObjectMeta.ResourceVersion = "1"
26082+
test.old.ObjectMeta.ResourceVersion = "1"
26083+
26084+
// set required fields if old and new match and have no opinion on the value
26085+
if test.new.Name == "" && test.old.Name == "" {
26086+
test.new.Name = "name"
26087+
test.old.Name = "name"
2606526088
}
26066-
} else {
26067-
if len(errs) == 0 {
26068-
t.Errorf("unexpected valid: %s\nA: %+v\nB: %+v", test.test, test.new, test.old)
26069-
} else if actualErr := errs.ToAggregate().Error(); !strings.Contains(actualErr, test.err) {
26070-
t.Errorf("unexpected error message: %s\nExpected error: %s\nActual error: %s", test.test, test.err, actualErr)
26089+
if test.new.Namespace == "" && test.old.Namespace == "" {
26090+
test.new.Namespace = "namespace"
26091+
test.old.Namespace = "namespace"
2607126092
}
26072-
}
26093+
if test.new.Spec.Containers == nil && test.old.Spec.Containers == nil {
26094+
test.new.Spec.Containers = []core.Container{{Name: "autoadded", Image: "image", TerminationMessagePolicy: "File", ImagePullPolicy: "Always"}}
26095+
test.old.Spec.Containers = []core.Container{{Name: "autoadded", Image: "image", TerminationMessagePolicy: "File", ImagePullPolicy: "Always"}}
26096+
}
26097+
if len(test.new.Spec.DNSPolicy) == 0 && len(test.old.Spec.DNSPolicy) == 0 {
26098+
test.new.Spec.DNSPolicy = core.DNSClusterFirst
26099+
test.old.Spec.DNSPolicy = core.DNSClusterFirst
26100+
}
26101+
if len(test.new.Spec.RestartPolicy) == 0 && len(test.old.Spec.RestartPolicy) == 0 {
26102+
test.new.Spec.RestartPolicy = "Always"
26103+
test.old.Spec.RestartPolicy = "Always"
26104+
}
26105+
26106+
errs := ValidatePodResize(test.new, test.old, PodValidationOptions{AllowSidecarResizePolicy: true})
26107+
if test.err == "" {
26108+
if len(errs) != 0 {
26109+
t.Errorf("unexpected invalid: (%+v)\nA: %+v\nB: %+v", errs, test.new, test.old)
26110+
}
26111+
} else {
26112+
if len(errs) == 0 {
26113+
t.Errorf("unexpected valid:\nA: %+v\nB: %+v", test.new, test.old)
26114+
} else if actualErr := errs.ToAggregate().Error(); !strings.Contains(actualErr, test.err) {
26115+
t.Errorf("unexpected error message:\nExpected error: %s\nActual error: %s", test.err, actualErr)
26116+
}
26117+
}
26118+
})
2607326119
}
2607426120
}

0 commit comments

Comments
 (0)