From 926e9fc8b2627b351e7713d4db8afd27c8903f00 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Fri, 6 Mar 2026 20:11:29 +0000 Subject: [PATCH] validate container restart policy against container resize policy --- pkg/api/pod/util.go | 23 +++- pkg/api/pod/util_test.go | 101 ++++++++++++++ pkg/apis/core/validation/validation.go | 13 +- pkg/apis/core/validation/validation_test.go | 140 +++++++++++++++----- 4 files changed, 238 insertions(+), 39 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 9f20394f051..a55fd32fcc5 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -430,8 +430,9 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po AllowContainerRestartPolicyRules: utilfeature.DefaultFeatureGate.Enabled(features.ContainerRestartRules), AllowUserNamespacesWithVolumeDevices: false, // This also allows restart rules on sidecar containers. - AllowRestartAllContainers: utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits), - AllowImageVolumeWithDigest: utilfeature.DefaultFeatureGate.Enabled(features.ImageVolumeWithDigest), + AllowRestartAllContainers: utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits), + AllowImageVolumeWithDigest: utilfeature.DefaultFeatureGate.Enabled(features.ImageVolumeWithDigest), + AllowExistingRestartContainerForNonSidecarInitContainer: hasRestartContainerForNonSidecarInitContainer(oldPodSpec), } // If old spec uses relaxed validation or enabled the RelaxedEnvironmentVariableValidation feature gate, @@ -2006,3 +2007,21 @@ func dropImageVolumeWithDigest(podStatus *api.PodStatus) { } } } + +// hasRestartContainerForNonSidecarInitContainer returns true if any non-sidecar init container +// has a RestartContainer resize policy. +func hasRestartContainerForNonSidecarInitContainer(spec *api.PodSpec) bool { + if spec == nil { + return false + } + for _, c := range spec.InitContainers { + if !IsRestartableInitContainer(&c) { + for _, p := range c.ResizePolicy { + if p.RestartPolicy == api.RestartContainer { + return true + } + } + } + } + return false +} diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index f2c1e1af718..6f546b2972f 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -7367,3 +7367,104 @@ func TestDropDisabledPodStatusFields_ResourceHealthStatusMessage(t *testing.T) { }) } } + +func TestHasRestartContainerForNonSidecarInitContainer(t *testing.T) { + tests := []struct { + name string + podSpec *api.PodSpec + expected bool + }{ + { + name: "nil pod spec", + podSpec: nil, + expected: false, + }, + { + name: "no init containers", + podSpec: &api.PodSpec{InitContainers: []api.Container{}}, + expected: false, + }, + { + name: "regular init container without resize policy", + podSpec: &api.PodSpec{ + InitContainers: []api.Container{ + {Name: "init-1"}, + }, + }, + expected: false, + }, + { + name: "sidecar (restartable) init container with RestartContainer policy", + podSpec: &api.PodSpec{ + InitContainers: []api.Container{ + { + Name: "sidecar", + RestartPolicy: ptr.To(api.ContainerRestartPolicyAlways), + ResizePolicy: []api.ContainerResizePolicy{ + {ResourceName: api.ResourceCPU, RestartPolicy: api.RestartContainer}, + }, + }, + }, + }, + expected: false, // Should be false because it's a sidecar + }, + { + name: "non-sidecar init container with NotRequired policy", + podSpec: &api.PodSpec{ + InitContainers: []api.Container{ + { + Name: "init-1", + ResizePolicy: []api.ContainerResizePolicy{ + {ResourceName: api.ResourceCPU, RestartPolicy: api.NotRequired}, + }, + }, + }, + }, + expected: false, + }, + { + name: "non-sidecar init container with RestartContainer policy", + podSpec: &api.PodSpec{ + InitContainers: []api.Container{ + { + Name: "init-1", + ResizePolicy: []api.ContainerResizePolicy{ + {ResourceName: api.ResourceMemory, RestartPolicy: api.RestartContainer}, + }, + }, + }, + }, + expected: true, + }, + { + name: "mix of sidecar and non-sidecar with RestartContainer policy", + podSpec: &api.PodSpec{ + InitContainers: []api.Container{ + { + Name: "sidecar", + RestartPolicy: ptr.To(api.ContainerRestartPolicyAlways), + ResizePolicy: []api.ContainerResizePolicy{ + {ResourceName: api.ResourceCPU, RestartPolicy: api.RestartContainer}, + }, + }, + { + Name: "init-2", + ResizePolicy: []api.ContainerResizePolicy{ + {ResourceName: api.ResourceCPU, RestartPolicy: api.RestartContainer}, + }, + }, + }, + }, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := hasRestartContainerForNonSidecarInitContainer(tt.podSpec) + if result != tt.expected { + t.Errorf("expected %v, got %v", tt.expected, result) + } + }) + } +} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 083e488e01a..c632b1c2e1b 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3653,9 +3653,10 @@ func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *fiel } if *podRestartPolicy == core.RestartPolicyNever && p.RestartPolicy != core.NotRequired { - allErrors = append(allErrors, field.Invalid(fldPath, p.RestartPolicy, "must be 'NotRequired' when `restartPolicy` is 'Never'")) + allErrors = append(allErrors, field.Invalid(fldPath, p.RestartPolicy, "must be 'NotRequired' when pod `restartPolicy` is 'Never'")) } } + return allErrors } @@ -3866,6 +3867,14 @@ func validateInitContainers(containers []core.Container, os *core.PodOS, regular if !opts.AllowSidecarResizePolicy && len(ctr.ResizePolicy) > 0 { allErrs = append(allErrs, field.Invalid(idxPath.Child("resizePolicy"), ctr.ResizePolicy, "must not be set for init containers")) } + + if !restartAlways && !opts.AllowExistingRestartContainerForNonSidecarInitContainer { + for j, p := range ctr.ResizePolicy { + if p.RestartPolicy == core.RestartContainer { + allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("resizePolicy").Index(j).Child("restartPolicy"), p.RestartPolicy, "must not be set to 'RestartContainer' for non-sidecar initContainers")) + } + } + } } return allErrs @@ -4485,6 +4494,8 @@ type PodValidationOptions struct { AllowEnvFilesValidation bool // Allows containers have restart policy and restart policy rules. AllowContainerRestartPolicyRules bool + // Allow existing RestartContainer resize policy for non-sidecar init containers for backward compatibility + AllowExistingRestartContainerForNonSidecarInitContainer bool // Allow user namespaces with volume devices, even though they will not function properly (should only be tolerated in updates of objects which already have this invalid configuration). AllowUserNamespacesWithVolumeDevices bool // Allow taint toleration comparison operators (Lt, Gt) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index fb73b6f37e1..d1b8c872ff2 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -8035,7 +8035,7 @@ func TestValidateResizePolicy(t *testing.T) { {ResourceName: "memory", RestartPolicy: "RestartContainer"}, }, ExpectError: true, - Errors: field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "must be 'NotRequired' when `restartPolicy` is 'Never'")}, + Errors: field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "must be 'NotRequired' when pod `restartPolicy` is 'Never'")}, PodRestartPolicy: "Never", }, "InvalidMemoryPolicyWithPodRestartPolicy": { @@ -8044,7 +8044,7 @@ func TestValidateResizePolicy(t *testing.T) { {ResourceName: "memory", RestartPolicy: "NotRequired"}, }, ExpectError: true, - Errors: field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "must be 'NotRequired' when `restartPolicy` is 'Never'")}, + Errors: field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "must be 'NotRequired' when pod `restartPolicy` is 'Never'")}, PodRestartPolicy: "Never", }, "InvalidMemoryCPUPolicyWithPodRestartPolicy": { @@ -8053,7 +8053,7 @@ func TestValidateResizePolicy(t *testing.T) { {ResourceName: "memory", RestartPolicy: "RestartContainer"}, }, ExpectError: true, - Errors: field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "must be 'NotRequired' when `restartPolicy` is 'Never'"), field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "must be 'NotRequired' when `restartPolicy` is 'Never'")}, + Errors: field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "must be 'NotRequired' when pod `restartPolicy` is 'Never'"), field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "must be 'NotRequired' when pod `restartPolicy` is 'Never'")}, PodRestartPolicy: "Never", }, "ValidMemoryCPUPolicyWithPodRestartPolicy": { @@ -8067,19 +8067,21 @@ func TestValidateResizePolicy(t *testing.T) { }, } for k, v := range testCases { - errs := validateResizePolicy(v.PolicyList, field.NewPath("field"), &v.PodRestartPolicy) - if !v.ExpectError && len(errs) > 0 { - t.Errorf("Testcase %s - expected success, got error: %+v", k, errs) - } - if v.ExpectError { - if len(errs) == 0 { - t.Errorf("Testcase %s - expected error, got success", k) + t.Run(k, func(t *testing.T) { + errs := validateResizePolicy(v.PolicyList, field.NewPath("field"), &v.PodRestartPolicy) + if !v.ExpectError && len(errs) > 0 { + t.Errorf("Testcase %s - expected success, got error: %+v", k, errs) } - delta := cmp.Diff(errs, v.Errors) - if delta != "" { - t.Errorf("Testcase %s - expected errors '%v', got '%v', diff: '%v'", k, v.Errors, errs, delta) + if v.ExpectError { + if len(errs) == 0 { + t.Errorf("Testcase %s - expected error, got success", k) + } + delta := cmp.Diff(errs, v.Errors) + if delta != "" { + t.Errorf("Testcase %s - expected errors '%v', got '%v', diff: '%v'", k, v.Errors, errs, delta) + } } - } + }) } } @@ -9467,8 +9469,7 @@ func TestValidateInitContainers(t *testing.T) { Image: "nginx", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", - }, - } + }} successCase := []core.Container{{ Name: "container-1-same-host-port-different-protocol", @@ -9537,10 +9538,27 @@ func TestValidateInitContainers(t *testing.T) { ResizePolicy: []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "NotRequired"}, }, - }, - } - var PodRestartPolicy core.RestartPolicy = "Never" - if errs := validateInitContainers(successCase, podOS, containers, volumeDevices, nil, defaultGracePeriod, field.NewPath("field"), PodValidationOptions{AllowSidecarResizePolicy: true}, &PodRestartPolicy, noUserNamespace); len(errs) != 0 { + }, { + Name: "restartcontainer-policy-allowed-if-already-exists", + Image: "nginx", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + ResizePolicy: []core.ContainerResizePolicy{ + {ResourceName: core.ResourceCPU, RestartPolicy: core.RestartContainer}, + }, + }, { + Name: "restartcontainer-policy-allowed-if-sidecar", + Image: "nginx", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + ResizePolicy: []core.ContainerResizePolicy{ + {ResourceName: core.ResourceCPU, RestartPolicy: core.RestartContainer}, + }, + RestartPolicy: ptr.To(core.ContainerRestartPolicyAlways), + }} + + var PodRestartPolicy core.RestartPolicy = "Always" + if errs := validateInitContainers(successCase, podOS, containers, volumeDevices, nil, defaultGracePeriod, field.NewPath("field"), PodValidationOptions{AllowSidecarResizePolicy: true, AllowExistingRestartContainerForNonSidecarInitContainer: true}, &PodRestartPolicy, noUserNamespace); len(errs) != 0 { t.Errorf("expected success: %v", errs) } @@ -9915,22 +9933,41 @@ func TestValidateInitContainers(t *testing.T) { }, }}, field.ErrorList{{Type: field.ErrorTypeRequired, Field: "initContainers[0].lifecycle.preStop", BadValue: ""}}, - }, - { - "Not supported ResizePolicy: invalid", - line(), - []core.Container{{ - Name: "init", - Image: "image", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - ResizePolicy: []core.ContainerResizePolicy{ - {ResourceName: "cpu", RestartPolicy: "NotRequired"}, - }, - }}, - field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "initContainers[0].resizePolicy", BadValue: []core.ContainerResizePolicy{{ResourceName: "cpu", RestartPolicy: "NotRequired"}}}}, - }, - } + }, { + "Not supported ResizePolicy: invalid", + line(), + []core.Container{{ + Name: "init", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + ResizePolicy: []core.ContainerResizePolicy{ + {ResourceName: "cpu", RestartPolicy: "NotRequired"}, + }, + }}, + field.ErrorList{{Type: field.ErrorTypeInvalid, Field: "initContainers[0].resizePolicy", BadValue: []core.ContainerResizePolicy{{ResourceName: "cpu", RestartPolicy: "NotRequired"}}}}, + }, { + "RestartContainer policy forbidden for non-sidecar init container", + line(), + []core.Container{{ + Name: "standard-init-no-restart", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + ResizePolicy: []core.ContainerResizePolicy{ + {ResourceName: core.ResourceCPU, RestartPolicy: core.RestartContainer}, + }, + }}, + field.ErrorList{{ + Type: field.ErrorTypeInvalid, + Field: "initContainers[0].resizePolicy", + BadValue: []core.ContainerResizePolicy{{ResourceName: core.ResourceCPU, RestartPolicy: core.RestartContainer}}, + }, { + Type: field.ErrorTypeInvalid, + Field: "initContainers[0].resizePolicy[0].restartPolicy", + BadValue: core.RestartContainer, + }}, + }} for _, tc := range errorCases { t.Run(tc.title+"__@L"+tc.line, func(t *testing.T) { @@ -13718,6 +13755,37 @@ func TestValidatePodUpdate(t *testing.T) { ), err: "Forbidden: pod updates may not change fields other than `spec.containers[*].image", test: "pod OS changing from Linux to Windows, IdentifyPodOS featuregate set", + }, { + new: *podtest.MakePod("foo", + podtest.SetInitContainers(core.Container{ + Name: "ictr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", + ResizePolicy: []core.ContainerResizePolicy{{ResourceName: core.ResourceCPU, RestartPolicy: core.RestartContainer}}, + }), + ), + old: *podtest.MakePod("foo", + podtest.SetInitContainers(core.Container{ + Name: "ictr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", + }), + ), + err: "spec.initContainers[0].resizePolicy[0].restartPolicy: Invalid value: \"RestartContainer\": must not be set to 'RestartContainer' for non-sidecar initContainers", + test: "adding RestartContainer resize policy to non-sidecar init container should fail", + opts: PodValidationOptions{AllowSidecarResizePolicy: true, AllowExistingRestartContainerForNonSidecarInitContainer: false}, + }, { + new: *podtest.MakePod("foo", + podtest.SetInitContainers(core.Container{ + Name: "ictr", Image: "image:v2", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", + ResizePolicy: []core.ContainerResizePolicy{{ResourceName: core.ResourceCPU, RestartPolicy: core.RestartContainer}}, + }), + ), + old: *podtest.MakePod("foo", + podtest.SetInitContainers(core.Container{ + Name: "ictr", Image: "image:v1", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", + ResizePolicy: []core.ContainerResizePolicy{{ResourceName: core.ResourceCPU, RestartPolicy: core.RestartContainer}}, + }), + ), + err: "", + test: "existing RestartContainer resize policy on non-sidecar init container should be allowed on update", + opts: PodValidationOptions{AllowSidecarResizePolicy: true, AllowExistingRestartContainerForNonSidecarInitContainer: true}, }, { new: *podtest.MakePod("foo", podtest.SetOS(core.Windows),