From a609beb6b195c7a2f7b72dec641a76b54bcce640 Mon Sep 17 00:00:00 2001 From: twelcon Date: Tue, 20 Jun 2023 18:48:47 +0530 Subject: [PATCH 1/4] Decline on resizePolicy if the restartPolicy is Never Signed-off-by: twelcon --- pkg/apis/core/validation/validation.go | 28 ++++++++++++--------- pkg/apis/core/validation/validation_test.go | 27 ++++++++++++++------ 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 6502d9c210a..bfe898b4615 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3044,7 +3044,7 @@ func validatePullPolicy(policy core.PullPolicy, fldPath *field.Path) field.Error var supportedResizeResources = sets.NewString(string(core.ResourceCPU), string(core.ResourceMemory)) var supportedResizePolicies = sets.NewString(string(core.NotRequired), string(core.RestartContainer)) -func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *field.Path) field.ErrorList { +func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *field.Path, restartPolicy *core.RestartPolicy) field.ErrorList { allErrors := field.ErrorList{} // validate that resource name is not repeated, supported resource names and policy values are specified @@ -3068,13 +3068,17 @@ func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *fiel default: allErrors = append(allErrors, field.NotSupported(fldPath, p.RestartPolicy, supportedResizePolicies.List())) } + + if *restartPolicy == core.RestartPolicyNever && p.RestartPolicy != core.NotRequired { + allErrors = append(allErrors, field.NotSupported(fldPath, p.RestartPolicy, []string{string(core.NotRequired)})) + } } return allErrors } // validateEphemeralContainers is called by pod spec and template validation to validate the list of ephemeral containers. // Note that this is called for pod template even though ephemeral containers aren't allowed in pod templates. -func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, containers, initContainers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { +func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, containers, initContainers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions, restartPolicy *core.RestartPolicy) field.ErrorList { var allErrs field.ErrorList if len(ephemeralContainers) == 0 { @@ -3095,7 +3099,7 @@ func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, idxPath := fldPath.Index(i) c := (*core.Container)(&ec.EphemeralContainerCommon) - allErrs = append(allErrs, validateContainerCommon(c, volumes, podClaimNames, idxPath, opts)...) + allErrs = append(allErrs, validateContainerCommon(c, volumes, podClaimNames, idxPath, opts, restartPolicy)...) // Ephemeral containers don't need looser constraints for pod templates, so it's convenient to apply both validations // here where we've already converted EphemeralContainerCommon to Container. allErrs = append(allErrs, validateContainerOnlyForPod(c, idxPath)...) @@ -3157,7 +3161,7 @@ func validateFieldAllowList(value interface{}, allowedFields map[string]bool, er } // validateInitContainers is called by pod spec and template validation to validate the list of init containers -func validateInitContainers(containers []core.Container, regularContainers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { +func validateInitContainers(containers []core.Container, regularContainers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions, restartPolicy *core.RestartPolicy) field.ErrorList { var allErrs field.ErrorList allNames := sets.String{} @@ -3168,7 +3172,7 @@ func validateInitContainers(containers []core.Container, regularContainers []cor idxPath := fldPath.Index(i) // Apply the validation common to all container types - allErrs = append(allErrs, validateContainerCommon(&ctr, volumes, podClaimNames, idxPath, opts)...) + allErrs = append(allErrs, validateContainerCommon(&ctr, volumes, podClaimNames, idxPath, opts, restartPolicy)...) // Names must be unique within regular and init containers. Collisions with ephemeral containers // will be detected by validateEphemeralContainers(). @@ -3204,7 +3208,7 @@ func validateInitContainers(containers []core.Container, regularContainers []cor // validateContainerCommon applies validation common to all container types. It's called by regular, init, and ephemeral // container list validation to require a properly formatted name, image, etc. -func validateContainerCommon(ctr *core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, path *field.Path, opts PodValidationOptions) field.ErrorList { +func validateContainerCommon(ctr *core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, path *field.Path, opts PodValidationOptions, restartPolicy *core.RestartPolicy) field.ErrorList { var allErrs field.ErrorList namePath := path.Child("name") @@ -3242,7 +3246,7 @@ func validateContainerCommon(ctr *core.Container, volumes map[string]core.Volume allErrs = append(allErrs, ValidateVolumeDevices(ctr.VolumeDevices, volMounts, volumes, path.Child("volumeDevices"))...) allErrs = append(allErrs, validatePullPolicy(ctr.ImagePullPolicy, path.Child("imagePullPolicy"))...) allErrs = append(allErrs, ValidateResourceRequirements(&ctr.Resources, podClaimNames, path.Child("resources"), opts)...) - allErrs = append(allErrs, validateResizePolicy(ctr.ResizePolicy, path.Child("resizePolicy"))...) + allErrs = append(allErrs, validateResizePolicy(ctr.ResizePolicy, path.Child("resizePolicy"), restartPolicy)...) allErrs = append(allErrs, ValidateSecurityContext(ctr.SecurityContext, path.Child("securityContext"))...) return allErrs } @@ -3295,7 +3299,7 @@ func validateHostUsers(spec *core.PodSpec, fldPath *field.Path) field.ErrorList } // validateContainers is called by pod spec and template validation to validate the list of regular containers. -func validateContainers(containers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { +func validateContainers(containers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions, restartPolicy *core.RestartPolicy) field.ErrorList { allErrs := field.ErrorList{} if len(containers) == 0 { @@ -3307,7 +3311,7 @@ func validateContainers(containers []core.Container, volumes map[string]core.Vol path := fldPath.Index(i) // Apply validation common to all containers - allErrs = append(allErrs, validateContainerCommon(&ctr, volumes, podClaimNames, path, opts)...) + allErrs = append(allErrs, validateContainerCommon(&ctr, volumes, podClaimNames, path, opts, restartPolicy)...) // Container names must be unique within the list of regular containers. // Collisions with init or ephemeral container names will be detected by the init or ephemeral @@ -3801,9 +3805,9 @@ func ValidatePodSpec(spec *core.PodSpec, podMeta *metav1.ObjectMeta, fldPath *fi allErrs = append(allErrs, vErrs...) podClaimNames := gatherPodResourceClaimNames(spec.ResourceClaims) allErrs = append(allErrs, validatePodResourceClaims(podMeta, spec.ResourceClaims, fldPath.Child("resourceClaims"))...) - allErrs = append(allErrs, validateContainers(spec.Containers, vols, podClaimNames, fldPath.Child("containers"), opts)...) - allErrs = append(allErrs, validateInitContainers(spec.InitContainers, spec.Containers, vols, podClaimNames, fldPath.Child("initContainers"), opts)...) - allErrs = append(allErrs, validateEphemeralContainers(spec.EphemeralContainers, spec.Containers, spec.InitContainers, vols, podClaimNames, fldPath.Child("ephemeralContainers"), opts)...) + allErrs = append(allErrs, validateContainers(spec.Containers, vols, podClaimNames, fldPath.Child("containers"), opts, &spec.RestartPolicy)...) + allErrs = append(allErrs, validateInitContainers(spec.InitContainers, spec.Containers, vols, podClaimNames, fldPath.Child("initContainers"), opts, &spec.RestartPolicy)...) + allErrs = append(allErrs, validateEphemeralContainers(spec.EphemeralContainers, spec.Containers, spec.InitContainers, vols, podClaimNames, fldPath.Child("ephemeralContainers"), opts, &spec.RestartPolicy)...) allErrs = append(allErrs, validatePodHostNetworkDeps(spec, fldPath, opts)...) allErrs = append(allErrs, validateRestartPolicy(&spec.RestartPolicy, fldPath.Child("restartPolicy"))...) allErrs = append(allErrs, validateDNSPolicy(&spec.DNSPolicy, fldPath.Child("dnsPolicy"))...) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 7474473f816..e5462f715d0 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -6821,8 +6821,10 @@ func TestValidateResizePolicy(t *testing.T) { field.ErrorList{field.Duplicate(field.NewPath("field").Index(2), core.ResourceCPU)}, }, } + var PodRestartPolicy core.RestartPolicy + PodRestartPolicy = "Never" for k, v := range testCases { - errs := validateResizePolicy(v.PolicyList, field.NewPath("field")) + errs := validateResizePolicy(v.PolicyList, field.NewPath("field"), &PodRestartPolicy) if !v.ExpectError && len(errs) > 0 { t.Errorf("Testcase %s - expected success, got error: %+v", k, errs) } @@ -6918,7 +6920,9 @@ func TestValidateEphemeralContainers(t *testing.T) { }, }}, } { - if errs := validateEphemeralContainers(ephemeralContainers, containers, initContainers, vols, nil, field.NewPath("ephemeralContainers"), PodValidationOptions{}); len(errs) != 0 { + var PodRestartPolicy core.RestartPolicy + PodRestartPolicy = "Never" + if errs := validateEphemeralContainers(ephemeralContainers, containers, initContainers, vols, nil, field.NewPath("ephemeralContainers"), PodValidationOptions{}, &PodRestartPolicy); len(errs) != 0 { t.Errorf("expected success for '%s' but got errors: %v", title, errs) } } @@ -7172,9 +7176,11 @@ func TestValidateEphemeralContainers(t *testing.T) { }, } + var PodRestartPolicy core.RestartPolicy + PodRestartPolicy = "Never" for _, tc := range tcs { t.Run(tc.title+"__@L"+tc.line, func(t *testing.T) { - errs := validateEphemeralContainers(tc.ephemeralContainers, containers, initContainers, vols, nil, field.NewPath("ephemeralContainers"), PodValidationOptions{}) + errs := validateEphemeralContainers(tc.ephemeralContainers, containers, initContainers, vols, nil, field.NewPath("ephemeralContainers"), PodValidationOptions{}, &PodRestartPolicy) if len(errs) == 0 { t.Fatal("expected error but received none") } @@ -7472,7 +7478,10 @@ func TestValidateContainers(t *testing.T) { }, }, } - if errs := validateContainers(successCase, volumeDevices, nil, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { + + var PodRestartPolicy core.RestartPolicy + PodRestartPolicy = "Never" + if errs := validateContainers(successCase, volumeDevices, nil, field.NewPath("field"), PodValidationOptions{}, &PodRestartPolicy); len(errs) != 0 { t.Errorf("expected success: %v", errs) } @@ -8028,9 +8037,10 @@ func TestValidateContainers(t *testing.T) { field.ErrorList{{Type: field.ErrorTypeNotSupported, Field: "containers[0].resizePolicy"}}, }, } + for _, tc := range errorCases { t.Run(tc.title+"__@L"+tc.line, func(t *testing.T) { - errs := validateContainers(tc.containers, volumeDevices, nil, field.NewPath("containers"), PodValidationOptions{}) + errs := validateContainers(tc.containers, volumeDevices, nil, field.NewPath("containers"), PodValidationOptions{}, &PodRestartPolicy) if len(errs) == 0 { t.Fatal("expected error but received none") } @@ -8077,7 +8087,9 @@ func TestValidateInitContainers(t *testing.T) { TerminationMessagePolicy: "File", }, } - if errs := validateInitContainers(successCase, containers, volumeDevices, nil, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { + var PodRestartPolicy core.RestartPolicy + PodRestartPolicy = "Never" + if errs := validateInitContainers(successCase, containers, volumeDevices, nil, field.NewPath("field"), PodValidationOptions{}, &PodRestartPolicy); len(errs) != 0 { t.Errorf("expected success: %v", errs) } @@ -8233,9 +8245,10 @@ func TestValidateInitContainers(t *testing.T) { field.ErrorList{{Type: field.ErrorTypeForbidden, Field: "initContainers[0].startupProbe", BadValue: ""}}, }, } + for _, tc := range errorCases { t.Run(tc.title+"__@L"+tc.line, func(t *testing.T) { - errs := validateInitContainers(tc.initContainers, containers, volumeDevices, nil, field.NewPath("initContainers"), PodValidationOptions{}) + errs := validateInitContainers(tc.initContainers, containers, volumeDevices, nil, field.NewPath("initContainers"), PodValidationOptions{}, &PodRestartPolicy) if len(errs) == 0 { t.Fatal("expected error but received none") } From 01c2c4f35f315eb79f1b9138691e45aba19d65ed Mon Sep 17 00:00:00 2001 From: twelcon Date: Tue, 20 Jun 2023 19:42:17 +0530 Subject: [PATCH 2/4] Error test cases added Signed-off-by: twelcon --- pkg/apis/core/validation/validation_test.go | 33 ++++++++++++++++----- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index e5462f715d0..9a0892112e3 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -6749,10 +6749,13 @@ func TestValidateResizePolicy(t *testing.T) { tSupportedResizeResources := sets.NewString(string(core.ResourceCPU), string(core.ResourceMemory)) tSupportedResizePolicies := sets.NewString(string(core.NotRequired), string(core.RestartContainer)) type T struct { - PolicyList []core.ContainerResizePolicy - ExpectError bool - Errors field.ErrorList + PolicyList []core.ContainerResizePolicy + ExpectError bool + Errors field.ErrorList + PodRestartPolicy core.RestartPolicy } + // var PodRestartPolicy core.RestartPolicy + // PodRestartPolicy = "Never" testCases := map[string]T{ "ValidCPUandMemoryPolicies": { []core.ContainerResizePolicy{ @@ -6761,6 +6764,7 @@ func TestValidateResizePolicy(t *testing.T) { }, false, nil, + "Always", }, "ValidCPUPolicy": { []core.ContainerResizePolicy{ @@ -6768,6 +6772,7 @@ func TestValidateResizePolicy(t *testing.T) { }, false, nil, + "Always", }, "ValidMemoryPolicy": { []core.ContainerResizePolicy{ @@ -6775,11 +6780,13 @@ func TestValidateResizePolicy(t *testing.T) { }, false, nil, + "Always", }, "NoPolicy": { []core.ContainerResizePolicy{}, false, nil, + "Always", }, "ValidCPUandInvalidMemoryPolicy": { []core.ContainerResizePolicy{ @@ -6788,6 +6795,7 @@ func TestValidateResizePolicy(t *testing.T) { }, true, field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceResizeRestartPolicy("Restarrrt"), tSupportedResizePolicies.List())}, + "Always", }, "ValidMemoryandInvalidCPUPolicy": { []core.ContainerResizePolicy{ @@ -6796,6 +6804,7 @@ func TestValidateResizePolicy(t *testing.T) { }, true, field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartNotRequirrred"), tSupportedResizePolicies.List())}, + "Always", }, "InvalidResourceNameValidPolicy": { []core.ContainerResizePolicy{ @@ -6803,6 +6812,7 @@ func TestValidateResizePolicy(t *testing.T) { }, true, field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceName("cpuuu"), tSupportedResizeResources.List())}, + "Always", }, "ValidResourceNameMissingPolicy": { []core.ContainerResizePolicy{ @@ -6810,6 +6820,7 @@ func TestValidateResizePolicy(t *testing.T) { }, true, field.ErrorList{field.Required(field.NewPath("field"), "")}, + "Always", }, "RepeatedPolicies": { []core.ContainerResizePolicy{ @@ -6819,12 +6830,20 @@ func TestValidateResizePolicy(t *testing.T) { }, true, field.ErrorList{field.Duplicate(field.NewPath("field").Index(2), core.ResourceCPU)}, + "Always", + }, + "InvalidPolicyWithPodRestartPolicy": { + []core.ContainerResizePolicy{ + {ResourceName: "cpu", RestartPolicy: "NotRequired"}, + {ResourceName: "memory", RestartPolicy: "RestartContainer"}, + }, + true, + field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), []string{string(core.NotRequired)})}, + "Never", }, } - var PodRestartPolicy core.RestartPolicy - PodRestartPolicy = "Never" for k, v := range testCases { - errs := validateResizePolicy(v.PolicyList, field.NewPath("field"), &PodRestartPolicy) + 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) } @@ -7480,7 +7499,7 @@ func TestValidateContainers(t *testing.T) { } var PodRestartPolicy core.RestartPolicy - PodRestartPolicy = "Never" + PodRestartPolicy = "Always" if errs := validateContainers(successCase, volumeDevices, nil, field.NewPath("field"), PodValidationOptions{}, &PodRestartPolicy); len(errs) != 0 { t.Errorf("expected success: %v", errs) } From 9d4b489107ec6b432091065e77af4c34c13f1444 Mon Sep 17 00:00:00 2001 From: twelcon Date: Wed, 21 Jun 2023 12:33:14 +0530 Subject: [PATCH 3/4] Renaming restartPolicy to containerRestartPolicy for better calrity Signed-off-by: twelcon --- pkg/apis/core/validation/validation.go | 22 +++++++-------- pkg/apis/core/validation/validation_test.go | 31 +++++++++++++++++++-- 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index bfe898b4615..0ed59df24ef 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3044,7 +3044,7 @@ func validatePullPolicy(policy core.PullPolicy, fldPath *field.Path) field.Error var supportedResizeResources = sets.NewString(string(core.ResourceCPU), string(core.ResourceMemory)) var supportedResizePolicies = sets.NewString(string(core.NotRequired), string(core.RestartContainer)) -func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *field.Path, restartPolicy *core.RestartPolicy) field.ErrorList { +func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *field.Path, podRestartPolicy *core.RestartPolicy) field.ErrorList { allErrors := field.ErrorList{} // validate that resource name is not repeated, supported resource names and policy values are specified @@ -3069,8 +3069,8 @@ func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *fiel allErrors = append(allErrors, field.NotSupported(fldPath, p.RestartPolicy, supportedResizePolicies.List())) } - if *restartPolicy == core.RestartPolicyNever && p.RestartPolicy != core.NotRequired { - allErrors = append(allErrors, field.NotSupported(fldPath, p.RestartPolicy, []string{string(core.NotRequired)})) + if *podRestartPolicy == core.RestartPolicyNever && p.RestartPolicy != core.NotRequired { + allErrors = append(allErrors, field.Invalid(fldPath, p.RestartPolicy, "Only NotRequired is allowed, if pod restartPolicy is Never")) } } return allErrors @@ -3078,7 +3078,7 @@ func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *fiel // validateEphemeralContainers is called by pod spec and template validation to validate the list of ephemeral containers. // Note that this is called for pod template even though ephemeral containers aren't allowed in pod templates. -func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, containers, initContainers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions, restartPolicy *core.RestartPolicy) field.ErrorList { +func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, containers, initContainers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions, podRestartPolicy *core.RestartPolicy) field.ErrorList { var allErrs field.ErrorList if len(ephemeralContainers) == 0 { @@ -3099,7 +3099,7 @@ func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, idxPath := fldPath.Index(i) c := (*core.Container)(&ec.EphemeralContainerCommon) - allErrs = append(allErrs, validateContainerCommon(c, volumes, podClaimNames, idxPath, opts, restartPolicy)...) + allErrs = append(allErrs, validateContainerCommon(c, volumes, podClaimNames, idxPath, opts, podRestartPolicy)...) // Ephemeral containers don't need looser constraints for pod templates, so it's convenient to apply both validations // here where we've already converted EphemeralContainerCommon to Container. allErrs = append(allErrs, validateContainerOnlyForPod(c, idxPath)...) @@ -3161,7 +3161,7 @@ func validateFieldAllowList(value interface{}, allowedFields map[string]bool, er } // validateInitContainers is called by pod spec and template validation to validate the list of init containers -func validateInitContainers(containers []core.Container, regularContainers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions, restartPolicy *core.RestartPolicy) field.ErrorList { +func validateInitContainers(containers []core.Container, regularContainers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions, podRestartPolicy *core.RestartPolicy) field.ErrorList { var allErrs field.ErrorList allNames := sets.String{} @@ -3172,7 +3172,7 @@ func validateInitContainers(containers []core.Container, regularContainers []cor idxPath := fldPath.Index(i) // Apply the validation common to all container types - allErrs = append(allErrs, validateContainerCommon(&ctr, volumes, podClaimNames, idxPath, opts, restartPolicy)...) + allErrs = append(allErrs, validateContainerCommon(&ctr, volumes, podClaimNames, idxPath, opts, podRestartPolicy)...) // Names must be unique within regular and init containers. Collisions with ephemeral containers // will be detected by validateEphemeralContainers(). @@ -3208,7 +3208,7 @@ func validateInitContainers(containers []core.Container, regularContainers []cor // validateContainerCommon applies validation common to all container types. It's called by regular, init, and ephemeral // container list validation to require a properly formatted name, image, etc. -func validateContainerCommon(ctr *core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, path *field.Path, opts PodValidationOptions, restartPolicy *core.RestartPolicy) field.ErrorList { +func validateContainerCommon(ctr *core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, path *field.Path, opts PodValidationOptions, podRestartPolicy *core.RestartPolicy) field.ErrorList { var allErrs field.ErrorList namePath := path.Child("name") @@ -3246,7 +3246,7 @@ func validateContainerCommon(ctr *core.Container, volumes map[string]core.Volume allErrs = append(allErrs, ValidateVolumeDevices(ctr.VolumeDevices, volMounts, volumes, path.Child("volumeDevices"))...) allErrs = append(allErrs, validatePullPolicy(ctr.ImagePullPolicy, path.Child("imagePullPolicy"))...) allErrs = append(allErrs, ValidateResourceRequirements(&ctr.Resources, podClaimNames, path.Child("resources"), opts)...) - allErrs = append(allErrs, validateResizePolicy(ctr.ResizePolicy, path.Child("resizePolicy"), restartPolicy)...) + allErrs = append(allErrs, validateResizePolicy(ctr.ResizePolicy, path.Child("resizePolicy"), podRestartPolicy)...) allErrs = append(allErrs, ValidateSecurityContext(ctr.SecurityContext, path.Child("securityContext"))...) return allErrs } @@ -3299,7 +3299,7 @@ func validateHostUsers(spec *core.PodSpec, fldPath *field.Path) field.ErrorList } // validateContainers is called by pod spec and template validation to validate the list of regular containers. -func validateContainers(containers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions, restartPolicy *core.RestartPolicy) field.ErrorList { +func validateContainers(containers []core.Container, volumes map[string]core.VolumeSource, podClaimNames sets.String, fldPath *field.Path, opts PodValidationOptions, podRestartPolicy *core.RestartPolicy) field.ErrorList { allErrs := field.ErrorList{} if len(containers) == 0 { @@ -3311,7 +3311,7 @@ func validateContainers(containers []core.Container, volumes map[string]core.Vol path := fldPath.Index(i) // Apply validation common to all containers - allErrs = append(allErrs, validateContainerCommon(&ctr, volumes, podClaimNames, path, opts, restartPolicy)...) + allErrs = append(allErrs, validateContainerCommon(&ctr, volumes, podClaimNames, path, opts, podRestartPolicy)...) // Container names must be unique within the list of regular containers. // Collisions with init or ephemeral container names will be detected by the init or ephemeral diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 9a0892112e3..55749d594b5 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -6832,13 +6832,40 @@ func TestValidateResizePolicy(t *testing.T) { field.ErrorList{field.Duplicate(field.NewPath("field").Index(2), core.ResourceCPU)}, "Always", }, - "InvalidPolicyWithPodRestartPolicy": { + "InvalidCPUPolicyWithPodRestartPolicy": { []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "NotRequired"}, {ResourceName: "memory", RestartPolicy: "RestartContainer"}, }, true, - field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), []string{string(core.NotRequired)})}, + field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "Only NotRequired is allowed, if pod restartPolicy is Never")}, + "Never", + }, + "InvalidMemoryPolicyWithPodRestartPolicy": { + []core.ContainerResizePolicy{ + {ResourceName: "cpu", RestartPolicy: "RestartContainer"}, + {ResourceName: "memory", RestartPolicy: "NotRequired"}, + }, + true, + field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "Only NotRequired is allowed, if pod restartPolicy is Never")}, + "Never", + }, + "InvalidMemoryCPUPolicyWithPodRestartPolicy": { + []core.ContainerResizePolicy{ + {ResourceName: "cpu", RestartPolicy: "RestartContainer"}, + {ResourceName: "memory", RestartPolicy: "RestartContainer"}, + }, + true, + field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "Only NotRequired is allowed, if pod restartPolicy is Never"), field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "Only NotRequired is allowed, if pod restartPolicy is Never")}, + "Never", + }, + "ValidMemoryCPUPolicyWithPodRestartPolicy": { + []core.ContainerResizePolicy{ + {ResourceName: "cpu", RestartPolicy: "NotRequired"}, + {ResourceName: "memory", RestartPolicy: "NotRequired"}, + }, + false, + nil, "Never", }, } From 70f979c8da350f95442902cff591091d0b7d439f Mon Sep 17 00:00:00 2001 From: twelcon Date: Mon, 10 Jul 2023 17:13:35 +0530 Subject: [PATCH 4/4] Alert message improved according to standards Signed-off-by: twelcon --- pkg/apis/core/validation/validation.go | 2 +- pkg/apis/core/validation/validation_test.go | 133 ++++++++++++-------- 2 files changed, 79 insertions(+), 56 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 0ed59df24ef..6145bf56aa7 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3070,7 +3070,7 @@ func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *fiel } if *podRestartPolicy == core.RestartPolicyNever && p.RestartPolicy != core.NotRequired { - allErrors = append(allErrors, field.Invalid(fldPath, p.RestartPolicy, "Only NotRequired is allowed, if pod restartPolicy is Never")) + allErrors = append(allErrors, field.Invalid(fldPath, p.RestartPolicy, "must be 'NotRequired' when `restartPolicy` is 'Never'")) } } return allErrors diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 55749d594b5..8f376c9ea8c 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -6754,119 +6754,118 @@ func TestValidateResizePolicy(t *testing.T) { Errors field.ErrorList PodRestartPolicy core.RestartPolicy } - // var PodRestartPolicy core.RestartPolicy - // PodRestartPolicy = "Never" + testCases := map[string]T{ "ValidCPUandMemoryPolicies": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "NotRequired"}, {ResourceName: "memory", RestartPolicy: "RestartContainer"}, }, - false, - nil, - "Always", + ExpectError: false, + Errors: nil, + PodRestartPolicy: "Always", }, "ValidCPUPolicy": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "RestartContainer"}, }, - false, - nil, - "Always", + ExpectError: false, + Errors: nil, + PodRestartPolicy: "Always", }, "ValidMemoryPolicy": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "memory", RestartPolicy: "NotRequired"}, }, - false, - nil, - "Always", + ExpectError: false, + Errors: nil, + PodRestartPolicy: "Always", }, "NoPolicy": { - []core.ContainerResizePolicy{}, - false, - nil, - "Always", + PolicyList: []core.ContainerResizePolicy{}, + ExpectError: false, + Errors: nil, + PodRestartPolicy: "Always", }, "ValidCPUandInvalidMemoryPolicy": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "NotRequired"}, {ResourceName: "memory", RestartPolicy: "Restarrrt"}, }, - true, - field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceResizeRestartPolicy("Restarrrt"), tSupportedResizePolicies.List())}, - "Always", + ExpectError: true, + Errors: field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceResizeRestartPolicy("Restarrrt"), tSupportedResizePolicies.List())}, + PodRestartPolicy: "Always", }, "ValidMemoryandInvalidCPUPolicy": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "RestartNotRequirrred"}, {ResourceName: "memory", RestartPolicy: "RestartContainer"}, }, - true, - field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartNotRequirrred"), tSupportedResizePolicies.List())}, - "Always", + ExpectError: true, + Errors: field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartNotRequirrred"), tSupportedResizePolicies.List())}, + PodRestartPolicy: "Always", }, "InvalidResourceNameValidPolicy": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "cpuuu", RestartPolicy: "NotRequired"}, }, - true, - field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceName("cpuuu"), tSupportedResizeResources.List())}, - "Always", + ExpectError: true, + Errors: field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceName("cpuuu"), tSupportedResizeResources.List())}, + PodRestartPolicy: "Always", }, "ValidResourceNameMissingPolicy": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "memory", RestartPolicy: ""}, }, - true, - field.ErrorList{field.Required(field.NewPath("field"), "")}, - "Always", + ExpectError: true, + Errors: field.ErrorList{field.Required(field.NewPath("field"), "")}, + PodRestartPolicy: "Always", }, "RepeatedPolicies": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "NotRequired"}, {ResourceName: "memory", RestartPolicy: "RestartContainer"}, {ResourceName: "cpu", RestartPolicy: "RestartContainer"}, }, - true, - field.ErrorList{field.Duplicate(field.NewPath("field").Index(2), core.ResourceCPU)}, - "Always", + ExpectError: true, + Errors: field.ErrorList{field.Duplicate(field.NewPath("field").Index(2), core.ResourceCPU)}, + PodRestartPolicy: "Always", }, "InvalidCPUPolicyWithPodRestartPolicy": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "NotRequired"}, {ResourceName: "memory", RestartPolicy: "RestartContainer"}, }, - true, - field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "Only NotRequired is allowed, if pod restartPolicy is Never")}, - "Never", + ExpectError: true, + Errors: field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "must be 'NotRequired' when `restartPolicy` is 'Never'")}, + PodRestartPolicy: "Never", }, "InvalidMemoryPolicyWithPodRestartPolicy": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "RestartContainer"}, {ResourceName: "memory", RestartPolicy: "NotRequired"}, }, - true, - field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "Only NotRequired is allowed, if pod restartPolicy is Never")}, - "Never", + ExpectError: true, + Errors: field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "must be 'NotRequired' when `restartPolicy` is 'Never'")}, + PodRestartPolicy: "Never", }, "InvalidMemoryCPUPolicyWithPodRestartPolicy": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "RestartContainer"}, {ResourceName: "memory", RestartPolicy: "RestartContainer"}, }, - true, - field.ErrorList{field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "Only NotRequired is allowed, if pod restartPolicy is Never"), field.Invalid(field.NewPath("field"), core.ResourceResizeRestartPolicy("RestartContainer"), "Only NotRequired is allowed, if pod restartPolicy is Never")}, - "Never", + 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'")}, + PodRestartPolicy: "Never", }, "ValidMemoryCPUPolicyWithPodRestartPolicy": { - []core.ContainerResizePolicy{ + PolicyList: []core.ContainerResizePolicy{ {ResourceName: "cpu", RestartPolicy: "NotRequired"}, {ResourceName: "memory", RestartPolicy: "NotRequired"}, }, - false, - nil, - "Never", + ExpectError: false, + Errors: nil, + PodRestartPolicy: "Never", }, } for k, v := range testCases { @@ -6971,6 +6970,16 @@ func TestValidateEphemeralContainers(t *testing.T) { if errs := validateEphemeralContainers(ephemeralContainers, containers, initContainers, vols, nil, field.NewPath("ephemeralContainers"), PodValidationOptions{}, &PodRestartPolicy); len(errs) != 0 { t.Errorf("expected success for '%s' but got errors: %v", title, errs) } + + PodRestartPolicy = "Always" + if errs := validateEphemeralContainers(ephemeralContainers, containers, initContainers, vols, nil, field.NewPath("ephemeralContainers"), PodValidationOptions{}, &PodRestartPolicy); len(errs) != 0 { + t.Errorf("expected success for '%s' but got errors: %v", title, errs) + } + + PodRestartPolicy = "OnFailure" + if errs := validateEphemeralContainers(ephemeralContainers, containers, initContainers, vols, nil, field.NewPath("ephemeralContainers"), PodValidationOptions{}, &PodRestartPolicy); len(errs) != 0 { + t.Errorf("expected success for '%s' but got errors: %v", title, errs) + } } // Failure Cases @@ -7223,14 +7232,28 @@ func TestValidateEphemeralContainers(t *testing.T) { } var PodRestartPolicy core.RestartPolicy - PodRestartPolicy = "Never" + for _, tc := range tcs { t.Run(tc.title+"__@L"+tc.line, func(t *testing.T) { + + PodRestartPolicy = "Never" errs := validateEphemeralContainers(tc.ephemeralContainers, containers, initContainers, vols, nil, field.NewPath("ephemeralContainers"), PodValidationOptions{}, &PodRestartPolicy) if len(errs) == 0 { t.Fatal("expected error but received none") } + PodRestartPolicy = "Always" + errs = validateEphemeralContainers(tc.ephemeralContainers, containers, initContainers, vols, nil, field.NewPath("ephemeralContainers"), PodValidationOptions{}, &PodRestartPolicy) + if len(errs) == 0 { + t.Fatal("expected error but received none") + } + + PodRestartPolicy = "OnFailure" + errs = validateEphemeralContainers(tc.ephemeralContainers, containers, initContainers, vols, nil, field.NewPath("ephemeralContainers"), PodValidationOptions{}, &PodRestartPolicy) + if len(errs) == 0 { + t.Fatal("expected error but received none") + } + if diff := cmp.Diff(tc.expectedErrors, errs, cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail")); diff != "" { t.Errorf("unexpected diff in errors (-want, +got):\n%s", diff) t.Errorf("INFO: all errors:\n%s", prettyErrorList(errs))