diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 031f4494f51..7d1e492a6a8 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5135,16 +5135,6 @@ var updatablePodSpecFields = []string{ "`spec.activeDeadlineSeconds`", "`spec.tolerations` (only additions to existing tolerations)", "`spec.terminationGracePeriodSeconds` (allow it to be set to 1 if it was previously negative)", - "`spec.containers[*].resources` (for CPU/memory only)", -} - -// TODO(vinaykul,InPlacePodVerticalScaling): Drop this var once InPlacePodVerticalScaling goes GA and featuregate is gone. -var updatablePodSpecFieldsNoResources = []string{ - "`spec.containers[*].image`", - "`spec.initContainers[*].image`", - "`spec.activeDeadlineSeconds`", - "`spec.tolerations` (only additions to existing tolerations)", - "`spec.terminationGracePeriodSeconds` (allow it to be set to 1 if it was previously negative)", } // ValidatePodUpdate tests to see if the update is legal for an end user to make. newPod is updated with fields @@ -5206,45 +5196,12 @@ func ValidatePodUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel return allErrs } - if qos.GetPodQOS(oldPod) != qos.ComputePodQOS(newPod) { - allErrs = append(allErrs, field.Invalid(fldPath, newPod.Status.QOSClass, "Pod QoS is immutable")) - } - // handle updateable fields by munging those fields prior to deep equal comparison. mungedPodSpec := *newPod.Spec.DeepCopy() // munge spec.containers[*].image var newContainers []core.Container for ix, container := range mungedPodSpec.Containers { container.Image = oldPod.Spec.Containers[ix].Image // +k8s:verify-mutation:reason=clone - // When the feature-gate is turned off, any new requests attempting to update CPU or memory - // resource values will result in validation failure. - if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - // Resources are mutable for CPU & memory only - // - user can now modify Resources to express new desired Resources - mungeCpuMemResources := func(resourceList, oldResourceList core.ResourceList) core.ResourceList { - if oldResourceList == nil { - return nil - } - var mungedResourceList core.ResourceList - if resourceList == nil { - mungedResourceList = make(core.ResourceList) - } else { - mungedResourceList = resourceList.DeepCopy() - } - delete(mungedResourceList, core.ResourceCPU) - delete(mungedResourceList, core.ResourceMemory) - if cpu, found := oldResourceList[core.ResourceCPU]; found { - mungedResourceList[core.ResourceCPU] = cpu - } - if mem, found := oldResourceList[core.ResourceMemory]; found { - mungedResourceList[core.ResourceMemory] = mem - } - return mungedResourceList - } - lim := mungeCpuMemResources(container.Resources.Limits, oldPod.Spec.Containers[ix].Resources.Limits) - req := mungeCpuMemResources(container.Resources.Requests, oldPod.Spec.Containers[ix].Resources.Requests) - container.Resources = core.ResourceRequirements{Limits: lim, Requests: req} - } newContainers = append(newContainers, container) } mungedPodSpec.Containers = newContainers @@ -5321,10 +5278,7 @@ func ValidatePodUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel // This diff isn't perfect, but it's a helluva lot better an "I'm not going to tell you what the difference is". // TODO: Pinpoint the specific field that causes the invalid error after we have strategic merge diff specDiff := cmp.Diff(oldPod.Spec, mungedPodSpec) - errs := field.Forbidden(specPath, fmt.Sprintf("pod updates may not change fields other than %s\n%v", strings.Join(updatablePodSpecFieldsNoResources, ","), specDiff)) - if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - errs = field.Forbidden(specPath, fmt.Sprintf("pod updates may not change fields other than %s\n%v", strings.Join(updatablePodSpecFields, ","), specDiff)) - } + errs := field.Forbidden(specPath, fmt.Sprintf("pod updates may not change fields other than %s\n%v", strings.Join(updatablePodSpecFields, ","), specDiff)) allErrs = append(allErrs, errs) } return allErrs @@ -5531,6 +5485,65 @@ func ValidatePodEphemeralContainersUpdate(newPod, oldPod *core.Pod, opts PodVali return allErrs } +func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) field.ErrorList { + // 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)...) + + // static pods cannot be resized. + if _, ok := oldPod.Annotations[core.MirrorPodAnnotationKey]; ok { + return field.ErrorList{field.Forbidden(field.NewPath(""), "static pods cannot be resized")} + } + + // Part 2: Validate that the changes between oldPod.Spec.Containers[].Resources and + // newPod.Spec.Containers[].Resources are allowed. + specPath := field.NewPath("spec") + if qos.GetPodQOS(oldPod) != qos.ComputePodQOS(newPod) { + allErrs = append(allErrs, field.Invalid(specPath, newPod.Status.QOSClass, "Pod QoS is immutable")) + } + + // Ensure that only CPU and memory resources are mutable. + mungedPodSpec := *newPod.Spec.DeepCopy() + var newContainers []core.Container + for ix, container := range mungedPodSpec.Containers { + mungeCPUMemResources := func(resourceList, oldResourceList core.ResourceList) core.ResourceList { + if oldResourceList == nil { + return nil + } + var mungedResourceList core.ResourceList + if resourceList == nil { + mungedResourceList = make(core.ResourceList) + } else { + mungedResourceList = resourceList.DeepCopy() + } + delete(mungedResourceList, core.ResourceCPU) + delete(mungedResourceList, core.ResourceMemory) + if cpu, found := oldResourceList[core.ResourceCPU]; found { + mungedResourceList[core.ResourceCPU] = cpu + } + if mem, found := oldResourceList[core.ResourceMemory]; found { + mungedResourceList[core.ResourceMemory] = mem + } + return mungedResourceList + } + lim := mungeCPUMemResources(container.Resources.Limits, oldPod.Spec.Containers[ix].Resources.Limits) + req := mungeCPUMemResources(container.Resources.Requests, oldPod.Spec.Containers[ix].Resources.Requests) + container.Resources = core.ResourceRequirements{Limits: lim, Requests: req} + container.ResizePolicy = oldPod.Spec.Containers[ix].ResizePolicy // +k8s:verify-mutation:reason=clone + newContainers = append(newContainers, container) + } + mungedPodSpec.Containers = newContainers + if !apiequality.Semantic.DeepEqual(mungedPodSpec, oldPod.Spec) { + // This likely means that the user has made changes to CPU and Memory resources. + specDiff := cmp.Diff(oldPod.Spec, mungedPodSpec) + errs := field.Forbidden(specPath, fmt.Sprintf("pod resize may not change fields other than cpu and memory\n%v", specDiff)) + allErrs = append(allErrs, errs) + } + return allErrs +} + // ValidatePodBinding tests if required fields in the pod binding are legal. func ValidatePodBinding(binding *core.Binding) field.ErrorList { allErrs := field.ErrorList{} diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 6adb7d1892b..956d0b7dbee 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -12495,7 +12495,7 @@ func TestValidatePodUpdate(t *testing.T) { Limits: getResources("100m", "0", "1Gi"), }))), ), - err: "", + err: "spec: Forbidden: pod updates may not change fields", test: "cpu limit change", }, { new: *podtest.MakePod("pod", @@ -12510,7 +12510,7 @@ func TestValidatePodUpdate(t *testing.T) { Limits: getResourceLimits("100m", "200Mi"), }))), ), - err: "", + err: "spec: Forbidden: pod updates may not change fields", test: "memory limit change", }, { new: *podtest.MakePod("pod", @@ -12540,7 +12540,7 @@ func TestValidatePodUpdate(t *testing.T) { Requests: getResourceLimits("200m", "0"), }))), ), - err: "", + err: "spec: Forbidden: pod updates may not change fields", test: "cpu request change", }, { new: *podtest.MakePod("pod", @@ -12555,7 +12555,7 @@ func TestValidatePodUpdate(t *testing.T) { Requests: getResourceLimits("0", "100Mi"), }))), ), - err: "", + err: "spec: Forbidden: pod updates may not change fields", test: "memory request change", }, { new: *podtest.MakePod("pod", @@ -12587,7 +12587,7 @@ func TestValidatePodUpdate(t *testing.T) { Requests: getResources("100m", "100Mi", "1Gi"), }))), ), - err: "", + err: "spec: Forbidden: pod updates may not change fields", test: "Pod QoS unchanged, guaranteed -> guaranteed", }, { new: *podtest.MakePod("pod", @@ -12604,7 +12604,7 @@ func TestValidatePodUpdate(t *testing.T) { Requests: getResources("200m", "200Mi", "1Gi"), }))), ), - err: "", + err: "spec: Forbidden: pod updates may not change fields", test: "Pod QoS unchanged, burstable -> burstable", }, { new: *podtest.MakePod("pod", @@ -12620,7 +12620,7 @@ func TestValidatePodUpdate(t *testing.T) { Requests: getResourceLimits("100m", "100Mi"), }))), ), - err: "", + err: "spec: Forbidden: pod updates may not change fields", test: "Pod QoS unchanged, burstable -> burstable, add limits", }, { new: *podtest.MakePod("pod", @@ -12636,7 +12636,7 @@ func TestValidatePodUpdate(t *testing.T) { Requests: getResourceLimits("100m", "100Mi"), }))), ), - err: "", + err: "spec: Forbidden: pod updates may not change fields", test: "Pod QoS unchanged, burstable -> burstable, remove limits", }, { new: *podtest.MakePod("pod", @@ -12652,7 +12652,7 @@ func TestValidatePodUpdate(t *testing.T) { Limits: getResources("200m", "500Mi", "1Gi"), }))), ), - err: "", + err: "spec: Forbidden: pod updates may not change fields", test: "Pod QoS unchanged, burstable -> burstable, add requests", }, { new: *podtest.MakePod("pod", @@ -12668,7 +12668,7 @@ func TestValidatePodUpdate(t *testing.T) { Requests: getResourceLimits("100m", "200Mi"), }))), ), - err: "", + err: "spec: Forbidden: pod updates may not change fields", test: "Pod QoS unchanged, burstable -> burstable, remove requests", }, { new: *podtest.MakePod("pod", @@ -12685,7 +12685,7 @@ func TestValidatePodUpdate(t *testing.T) { Requests: getResourceLimits("100m", "100Mi"), }))), ), - err: "Pod QoS is immutable", + err: "spec: Forbidden: pod updates may not change fields", test: "Pod QoS change, guaranteed -> burstable", }, { new: *podtest.MakePod("pod", @@ -12701,7 +12701,7 @@ func TestValidatePodUpdate(t *testing.T) { Requests: getResourceLimits("100m", "100Mi"), }))), ), - err: "Pod QoS is immutable", + err: "spec: Forbidden: pod updates may not change fields", test: "Pod QoS change, burstable -> guaranteed", }, { new: *podtest.MakePod("pod", @@ -12712,7 +12712,7 @@ func TestValidatePodUpdate(t *testing.T) { }))), ), old: *podtest.MakePod("pod"), - err: "Pod QoS is immutable", + err: "spec: Forbidden: pod updates may not change fields", test: "Pod QoS change, besteffort -> burstable", }, { new: *podtest.MakePod("pod"), @@ -12723,7 +12723,7 @@ func TestValidatePodUpdate(t *testing.T) { Requests: getResourceLimits("100m", "100Mi"), }))), ), - err: "Pod QoS is immutable", + err: "spec: Forbidden: pod updates may not change fields", test: "Pod QoS change, burstable -> besteffort", }, { new: *podtest.MakePod("pod", @@ -25104,3 +25104,297 @@ func TestValidateSELinuxChangePolicy(t *testing.T) { }) } } + +func TestValidatePodResize(t *testing.T) { + tests := []struct { + new core.Pod + old core.Pod + err string + test string + }{ + { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("200m", "0", "1Gi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("100m", "0", "1Gi"), + }))), + ), + err: "", + test: "cpu limit change", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResourceLimits("100m", "100Mi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResourceLimits("100m", "200Mi"), + }))), + ), + err: "", + test: "memory limit change", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("100m", "100Mi", "1Gi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("100m", "100Mi", "2Gi"), + }))), + ), + err: "spec: Forbidden: pod resize may not change fields other than cpu and memory", + test: "storage limit change", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Requests: getResourceLimits("100m", "0"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Requests: getResourceLimits("200m", "0"), + }))), + ), + err: "", + test: "cpu request change", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Requests: getResourceLimits("0", "200Mi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Requests: getResourceLimits("0", "100Mi"), + }))), + ), + err: "", + test: "memory request change", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Requests: getResources("100m", "0", "2Gi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Requests: getResources("100m", "0", "1Gi"), + }))), + ), + err: "spec: Forbidden: pod resize may not change fields other than cpu and memory", + test: "storage request change", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("200m", "400Mi", "1Gi"), + Requests: getResources("200m", "400Mi", "1Gi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("100m", "100Mi", "1Gi"), + Requests: getResources("100m", "100Mi", "1Gi"), + }))), + ), + err: "", + test: "Pod QoS unchanged, guaranteed -> guaranteed", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("200m", "200Mi", "2Gi"), + Requests: getResources("100m", "100Mi", "1Gi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("400m", "400Mi", "2Gi"), + Requests: getResources("200m", "200Mi", "1Gi"), + }))), + ), + err: "", + test: "Pod QoS unchanged, burstable -> burstable", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResourceLimits("200m", "200Mi"), + Requests: getResourceLimits("100m", "100Mi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Requests: getResourceLimits("100m", "100Mi"), + }))), + ), + err: "", + test: "Pod QoS unchanged, burstable -> burstable, add limits", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Requests: getResourceLimits("100m", "100Mi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResourceLimits("200m", "200Mi"), + Requests: getResourceLimits("100m", "100Mi"), + }))), + ), + err: "", + test: "Pod QoS unchanged, burstable -> burstable, remove limits", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("400m", "", "1Gi"), + Requests: getResources("300m", "", "1Gi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("200m", "500Mi", "1Gi"), + }))), + ), + err: "", + test: "Pod QoS unchanged, burstable -> burstable, add requests", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("400m", "500Mi", "2Gi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResources("200m", "300Mi", "2Gi"), + Requests: getResourceLimits("100m", "200Mi"), + }))), + ), + err: "", + test: "Pod QoS unchanged, burstable -> burstable, remove requests", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResourceLimits("200m", "200Mi"), + Requests: getResourceLimits("100m", "100Mi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResourceLimits("100m", "100Mi"), + Requests: getResourceLimits("100m", "100Mi"), + }))), + ), + err: "Pod QoS is immutable", + test: "Pod QoS change, guaranteed -> burstable", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResourceLimits("100m", "100Mi"), + Requests: getResourceLimits("100m", "100Mi"), + }))), + ), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Requests: getResourceLimits("100m", "100Mi"), + }))), + ), + err: "Pod QoS is immutable", + test: "Pod QoS change, burstable -> guaranteed", + }, { + new: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResourceLimits("200m", "200Mi"), + Requests: getResourceLimits("100m", "100Mi"), + }))), + ), + old: *podtest.MakePod("pod"), + err: "Pod QoS is immutable", + test: "Pod QoS change, besteffort -> burstable", + }, { + new: *podtest.MakePod("pod"), + old: *podtest.MakePod("pod", + podtest.SetContainers(podtest.MakeContainer("container", + podtest.SetContainerResources(core.ResourceRequirements{ + Limits: getResourceLimits("200m", "200Mi"), + Requests: getResourceLimits("100m", "100Mi"), + }))), + ), + err: "Pod QoS is immutable", + test: "Pod QoS change, burstable -> besteffort", + }, + } + + for _, test := range tests { + test.new.ObjectMeta.ResourceVersion = "1" + test.old.ObjectMeta.ResourceVersion = "1" + + // set required fields if old and new match and have no opinion on the value + if test.new.Name == "" && test.old.Name == "" { + test.new.Name = "name" + test.old.Name = "name" + } + if test.new.Namespace == "" && test.old.Namespace == "" { + test.new.Namespace = "namespace" + test.old.Namespace = "namespace" + } + if test.new.Spec.Containers == nil && test.old.Spec.Containers == nil { + test.new.Spec.Containers = []core.Container{{Name: "autoadded", Image: "image", TerminationMessagePolicy: "File", ImagePullPolicy: "Always"}} + test.old.Spec.Containers = []core.Container{{Name: "autoadded", Image: "image", TerminationMessagePolicy: "File", ImagePullPolicy: "Always"}} + } + if len(test.new.Spec.DNSPolicy) == 0 && len(test.old.Spec.DNSPolicy) == 0 { + test.new.Spec.DNSPolicy = core.DNSClusterFirst + test.old.Spec.DNSPolicy = core.DNSClusterFirst + } + if len(test.new.Spec.RestartPolicy) == 0 && len(test.old.Spec.RestartPolicy) == 0 { + test.new.Spec.RestartPolicy = "Always" + test.old.Spec.RestartPolicy = "Always" + } + + errs := ValidatePodResize(&test.new, &test.old, PodValidationOptions{}) + if test.err == "" { + if len(errs) != 0 { + t.Errorf("unexpected invalid: %s (%+v)\nA: %+v\nB: %+v", test.test, errs, test.new, test.old) + } + } 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) + } + } + } +} diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index 0586f3994ba..2022873d4ee 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -321,7 +321,7 @@ func (podResizeStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Ob oldPod := old.(*api.Pod) opts := podutil.GetValidationOptionsFromPodSpecAndMeta(&newPod.Spec, &oldPod.Spec, &newPod.ObjectMeta, &oldPod.ObjectMeta) opts.ResourceIsPod = true - return nil + return corevalidation.ValidatePodResize(newPod, oldPod, opts) } // WarningsOnUpdate returns warnings for the given update.