From 1dc040082c3bead76c8731c7f94eba4a0265ca10 Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Sun, 24 Jul 2022 15:52:03 +0200 Subject: [PATCH] Refactor container validation Refactor common validation into methods that validate a single container and call these methods when iterating the three types of container lists. Move initContainer-specific validation from validateContainers to validateInitContainers. This resolves issues where init and ephemeral containers would return duplicate or incorrectly formatted errors for problems detected by validateContainers. --- pkg/apis/core/validation/validation.go | 166 ++++++++++---------- pkg/apis/core/validation/validation_test.go | 47 +++--- 2 files changed, 109 insertions(+), 104 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index b0ad047073f..ffb91a497b0 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -2930,18 +2930,14 @@ func validateEphemeralContainers(ephemeralContainers []core.EphemeralContainer, } if ec.Name == "" { - allErrs = append(allErrs, field.Required(idxPath, "ephemeralContainer requires a name")) + allErrs = append(allErrs, field.Required(idxPath.Child("name"), "ephemeralContainer requires a name")) continue } - // Using validateContainers() here isn't ideal because it adds an index to the error message that - // doesn't really exist for EphemeralContainers (i.e. ephemeralContainers[0].spec[0].name instead - // of ephemeralContainers[0].spec.name) - // TODO(verb): factor a validateContainer() out of validateContainers() to be used here - c := core.Container(ec.EphemeralContainerCommon) - allErrs = append(allErrs, validateContainers([]core.Container{c}, false, volumes, idxPath, opts)...) + c := (*core.Container)(&ec.EphemeralContainerCommon) + allErrs = append(allErrs, validateContainer(c, volumes, idxPath, opts)...) // EphemeralContainers don't require the backwards-compatibility distinction between pod/podTemplate validation - allErrs = append(allErrs, validateContainersOnlyForPod([]core.Container{c}, idxPath)...) + allErrs = append(allErrs, validateContainerOnlyForPod(c, idxPath)...) if allNames.Has(ec.Name) { allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ec.Name)) @@ -2993,11 +2989,8 @@ func validateFieldAllowList(value interface{}, allowedFields map[string]bool, er return allErrs } -func validateInitContainers(containers []core.Container, otherContainers []core.Container, deviceVolumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { +func validateInitContainers(containers []core.Container, otherContainers []core.Container, volumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { var allErrs field.ErrorList - if len(containers) > 0 { - allErrs = append(allErrs, validateContainers(containers, true, deviceVolumes, fldPath, opts)...) - } allNames := sets.String{} for _, ctr := range otherContainers { @@ -3007,10 +3000,15 @@ func validateInitContainers(containers []core.Container, otherContainers []core. idxPath := fldPath.Index(i) if allNames.Has(ctr.Name) { allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ctr.Name)) - } - if len(ctr.Name) > 0 { + } else if len(ctr.Name) > 0 { allNames.Insert(ctr.Name) } + + allErrs = append(allErrs, validateContainer(&ctr, volumes, idxPath, opts)...) + + // Check for port conflicts in init containers individually since init containers run one-by-one. + allErrs = append(allErrs, checkHostPortConflicts([]core.Container{ctr}, fldPath)...) + if ctr.Lifecycle != nil { allErrs = append(allErrs, field.Invalid(idxPath.Child("lifecycle"), ctr.Lifecycle, "must not be set for init containers")) } @@ -3024,10 +3022,70 @@ func validateInitContainers(containers []core.Container, otherContainers []core. allErrs = append(allErrs, field.Invalid(idxPath.Child("startupProbe"), ctr.StartupProbe, "must not be set for init containers")) } } + return allErrs } -func validateContainers(containers []core.Container, isInitContainers bool, volumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { +func validateContainer(ctr *core.Container, volumes map[string]core.VolumeSource, path *field.Path, opts PodValidationOptions) field.ErrorList { + allErrs := field.ErrorList{} + + namePath := path.Child("name") + if len(ctr.Name) == 0 { + allErrs = append(allErrs, field.Required(namePath, "")) + } else { + allErrs = append(allErrs, ValidateDNS1123Label(ctr.Name, namePath)...) + } + + // TODO: do not validate leading and trailing whitespace to preserve backward compatibility. + // for example: https://github.com/openshift/origin/issues/14659 image = " " is special token in pod template + // others may have done similar + if len(ctr.Image) == 0 { + allErrs = append(allErrs, field.Required(path.Child("image"), "")) + } + if ctr.Lifecycle != nil { + allErrs = append(allErrs, validateLifecycle(ctr.Lifecycle, path.Child("lifecycle"))...) + } + + allErrs = append(allErrs, validateProbe(ctr.LivenessProbe, path.Child("livenessProbe"))...) + // Readiness-specific validation + if ctr.ReadinessProbe != nil && ctr.ReadinessProbe.TerminationGracePeriodSeconds != nil { + allErrs = append(allErrs, field.Invalid(path.Child("readinessProbe", "terminationGracePeriodSeconds"), ctr.ReadinessProbe.TerminationGracePeriodSeconds, "must not be set for readinessProbes")) + } + allErrs = append(allErrs, validateProbe(ctr.StartupProbe, path.Child("startupProbe"))...) + // Liveness-specific validation + if ctr.LivenessProbe != nil && ctr.LivenessProbe.SuccessThreshold != 1 { + allErrs = append(allErrs, field.Invalid(path.Child("livenessProbe", "successThreshold"), ctr.LivenessProbe.SuccessThreshold, "must be 1")) + } + allErrs = append(allErrs, validateProbe(ctr.StartupProbe, path.Child("startupProbe"))...) + // Startup-specific validation + if ctr.StartupProbe != nil && ctr.StartupProbe.SuccessThreshold != 1 { + allErrs = append(allErrs, field.Invalid(path.Child("startupProbe", "successThreshold"), ctr.StartupProbe.SuccessThreshold, "must be 1")) + } + + switch ctr.TerminationMessagePolicy { + case core.TerminationMessageReadFile, core.TerminationMessageFallbackToLogsOnError: + case "": + allErrs = append(allErrs, field.Required(path.Child("terminationMessagePolicy"), "must be 'File' or 'FallbackToLogsOnError'")) + default: + allErrs = append(allErrs, field.Invalid(path.Child("terminationMessagePolicy"), ctr.TerminationMessagePolicy, "must be 'File' or 'FallbackToLogsOnError'")) + } + + volMounts := GetVolumeMountMap(ctr.VolumeMounts) + volDevices := GetVolumeDeviceMap(ctr.VolumeDevices) + allErrs = append(allErrs, validateProbe(ctr.ReadinessProbe, path.Child("readinessProbe"))...) + allErrs = append(allErrs, validateContainerPorts(ctr.Ports, path.Child("ports"))...) + allErrs = append(allErrs, ValidateEnv(ctr.Env, path.Child("env"), opts)...) + allErrs = append(allErrs, ValidateEnvFrom(ctr.EnvFrom, path.Child("envFrom"))...) + allErrs = append(allErrs, ValidateVolumeMounts(ctr.VolumeMounts, volDevices, volumes, ctr, path.Child("volumeMounts"))...) + 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, path.Child("resources"), opts)...) + allErrs = append(allErrs, ValidateSecurityContext(ctr.SecurityContext, path.Child("securityContext"))...) + + return allErrs +} + +func validateContainers(containers []core.Container, volumes map[string]core.VolumeSource, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} if len(containers) == 0 { @@ -3037,73 +3095,18 @@ func validateContainers(containers []core.Container, isInitContainers bool, volu allNames := sets.String{} for i, ctr := range containers { idxPath := fldPath.Index(i) - namePath := idxPath.Child("name") - volMounts := GetVolumeMountMap(ctr.VolumeMounts) - volDevices := GetVolumeDeviceMap(ctr.VolumeDevices) - if len(ctr.Name) == 0 { - allErrs = append(allErrs, field.Required(namePath, "")) - } else { - allErrs = append(allErrs, ValidateDNS1123Label(ctr.Name, namePath)...) - } if allNames.Has(ctr.Name) { - allErrs = append(allErrs, field.Duplicate(namePath, ctr.Name)) + allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ctr.Name)) } else { allNames.Insert(ctr.Name) } - // TODO: do not validate leading and trailing whitespace to preserve backward compatibility. - // for example: https://github.com/openshift/origin/issues/14659 image = " " is special token in pod template - // others may have done similar - if len(ctr.Image) == 0 { - allErrs = append(allErrs, field.Required(idxPath.Child("image"), "")) - } - if ctr.Lifecycle != nil { - allErrs = append(allErrs, validateLifecycle(ctr.Lifecycle, idxPath.Child("lifecycle"))...) - } - allErrs = append(allErrs, validateProbe(ctr.LivenessProbe, idxPath.Child("livenessProbe"))...) - // Readiness-specific validation - if ctr.ReadinessProbe != nil && ctr.ReadinessProbe.TerminationGracePeriodSeconds != nil { - allErrs = append(allErrs, field.Invalid(idxPath.Child("readinessProbe", "terminationGracePeriodSeconds"), ctr.ReadinessProbe.TerminationGracePeriodSeconds, "must not be set for readinessProbes")) - } - allErrs = append(allErrs, validateProbe(ctr.StartupProbe, idxPath.Child("startupProbe"))...) - // Liveness-specific validation - if ctr.LivenessProbe != nil && ctr.LivenessProbe.SuccessThreshold != 1 { - allErrs = append(allErrs, field.Invalid(idxPath.Child("livenessProbe", "successThreshold"), ctr.LivenessProbe.SuccessThreshold, "must be 1")) - } - allErrs = append(allErrs, validateProbe(ctr.StartupProbe, idxPath.Child("startupProbe"))...) - // Startup-specific validation - if ctr.StartupProbe != nil && ctr.StartupProbe.SuccessThreshold != 1 { - allErrs = append(allErrs, field.Invalid(idxPath.Child("startupProbe", "successThreshold"), ctr.StartupProbe.SuccessThreshold, "must be 1")) - } - switch ctr.TerminationMessagePolicy { - case core.TerminationMessageReadFile, core.TerminationMessageFallbackToLogsOnError: - case "": - allErrs = append(allErrs, field.Required(idxPath.Child("terminationMessagePolicy"), "must be 'File' or 'FallbackToLogsOnError'")) - default: - allErrs = append(allErrs, field.Invalid(idxPath.Child("terminationMessagePolicy"), ctr.TerminationMessagePolicy, "must be 'File' or 'FallbackToLogsOnError'")) - } - - allErrs = append(allErrs, validateProbe(ctr.ReadinessProbe, idxPath.Child("readinessProbe"))...) - allErrs = append(allErrs, validateContainerPorts(ctr.Ports, idxPath.Child("ports"))...) - allErrs = append(allErrs, ValidateEnv(ctr.Env, idxPath.Child("env"), opts)...) - allErrs = append(allErrs, ValidateEnvFrom(ctr.EnvFrom, idxPath.Child("envFrom"))...) - allErrs = append(allErrs, ValidateVolumeMounts(ctr.VolumeMounts, volDevices, volumes, &ctr, idxPath.Child("volumeMounts"))...) - allErrs = append(allErrs, ValidateVolumeDevices(ctr.VolumeDevices, volMounts, volumes, idxPath.Child("volumeDevices"))...) - allErrs = append(allErrs, validatePullPolicy(ctr.ImagePullPolicy, idxPath.Child("imagePullPolicy"))...) - allErrs = append(allErrs, ValidateResourceRequirements(&ctr.Resources, idxPath.Child("resources"), opts)...) - allErrs = append(allErrs, ValidateSecurityContext(ctr.SecurityContext, idxPath.Child("securityContext"))...) + allErrs = append(allErrs, validateContainer(&ctr, volumes, idxPath, opts)...) } - if isInitContainers { - // check initContainers one by one since they are running in sequential order. - for _, initContainer := range containers { - allErrs = append(allErrs, checkHostPortConflicts([]core.Container{initContainer}, fldPath)...) - } - } else { - // Check for colliding ports across all containers. - allErrs = append(allErrs, checkHostPortConflicts(containers, fldPath)...) - } + // Check for colliding ports across all containers. + allErrs = append(allErrs, checkHostPortConflicts(containers, fldPath)...) return allErrs } @@ -3393,10 +3396,15 @@ func ValidateTolerations(tolerations []core.Toleration, fldPath *field.Path) fie func validateContainersOnlyForPod(containers []core.Container, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for i, ctr := range containers { - idxPath := fldPath.Index(i) - if len(ctr.Image) != len(strings.TrimSpace(ctr.Image)) { - allErrs = append(allErrs, field.Invalid(idxPath.Child("image"), ctr.Image, "must not have leading or trailing whitespace")) - } + allErrs = append(allErrs, validateContainerOnlyForPod(&ctr, fldPath.Index(i))...) + } + return allErrs +} + +func validateContainerOnlyForPod(ctr *core.Container, path *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if len(ctr.Image) != len(strings.TrimSpace(ctr.Image)) { + allErrs = append(allErrs, field.Invalid(path.Child("image"), ctr.Image, "must not have leading or trailing whitespace")) } return allErrs } @@ -3503,7 +3511,7 @@ func ValidatePodSpec(spec *core.PodSpec, podMeta *metav1.ObjectMeta, fldPath *fi vols, vErrs := ValidateVolumes(spec.Volumes, podMeta, fldPath.Child("volumes"), opts) allErrs = append(allErrs, vErrs...) - allErrs = append(allErrs, validateContainers(spec.Containers, false, vols, fldPath.Child("containers"), opts)...) + allErrs = append(allErrs, validateContainers(spec.Containers, vols, fldPath.Child("containers"), opts)...) allErrs = append(allErrs, validateInitContainers(spec.InitContainers, spec.Containers, vols, fldPath.Child("initContainers"), opts)...) allErrs = append(allErrs, validateEphemeralContainers(spec.EphemeralContainers, spec.Containers, spec.InitContainers, vols, fldPath.Child("ephemeralContainers"), opts)...) allErrs = append(allErrs, validateRestartPolicy(&spec.RestartPolicy, fldPath.Child("restartPolicy"))...) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 9504a15caba..00ea5fbcbb3 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -6747,28 +6747,28 @@ func TestValidateEphemeralContainers(t *testing.T) { []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{}}, }, - field.Error{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0]"}, + field.Error{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].name"}, }, { "empty Container Name", []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0]"}, + field.Error{Type: field.ErrorTypeRequired, Field: "ephemeralContainers[0].name"}, }, { "whitespace padded image name", []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: " image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeInvalid, Field: "ephemeralContainers[0][0].image"}, + field.Error{Type: field.ErrorTypeInvalid, Field: "ephemeralContainers[0].image"}, }, { "invalid image pull policy", []core.EphemeralContainer{ {EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: "image", ImagePullPolicy: "PullThreeTimes", TerminationMessagePolicy: "File"}}, }, - field.Error{Type: field.ErrorTypeNotSupported, Field: "ephemeralContainers[0][0].imagePullPolicy"}, + field.Error{Type: field.ErrorTypeNotSupported, Field: "ephemeralContainers[0].imagePullPolicy"}, }, { "TargetContainerName doesn't exist", @@ -7218,7 +7218,7 @@ func TestValidateContainers(t *testing.T) { TerminationMessagePolicy: "File", }, } - if errs := validateContainers(successCase, false, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { + if errs := validateContainers(successCase, volumeDevices, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { t.Errorf("expected success: %v", errs) } @@ -7704,7 +7704,7 @@ func TestValidateContainers(t *testing.T) { } for _, tc := range errorCases { t.Run(tc.title, func(t *testing.T) { - errs := validateContainers(tc.containers, false, volumeDevices, field.NewPath("containers"), PodValidationOptions{}) + errs := validateContainers(tc.containers, volumeDevices, field.NewPath("containers"), PodValidationOptions{}) if len(errs) == 0 { t.Fatalf("expected error but received none") } @@ -7798,27 +7798,24 @@ func TestValidateInitContainers(t *testing.T) { }, field.Error{Type: field.ErrorTypeInvalid, Field: "initContainers[0].terminationMessagePolicy"}, }, - /* - TODO: Validation incorrectly returns duplicate errors for duplicate names. + { + "duplicate names", + []core.Container{ { - "duplicate names", - []core.Container{ - { - Name: "init", - Image: "image", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - { - Name: "init", - Image: "image", - ImagePullPolicy: "IfNotPresent", - TerminationMessagePolicy: "File", - }, - }, - field.Error{Type: field.ErrorTypeDuplicate, Field: "initContainers[1].name"}, + Name: "init", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", }, - */ + { + Name: "init", + Image: "image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePolicy: "File", + }, + }, + field.Error{Type: field.ErrorTypeDuplicate, Field: "initContainers[1].name"}, + }, { "duplicate ports", []core.Container{