image name must not have leading or trailing whitespace #47491
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/S
|
@eparis @smarterclayton -- in debugging events on our cluster, i observed 3 pods had an image name set to " ", and this resulted in ImageFailed events happening about 47k times for this pod over ~7d. |
| "name > 63 characters": {{Name: strings.Repeat("a", 64), Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, | ||
| "name not a DNS label": {{Name: "a.b.c", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, | ||
| "zero-length name": {{Name: "", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, | ||
| "image leading and trailing whitespace": {{Name: "", Image: " image ", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, |
There was a problem hiding this comment.
How about add another case of enabling image = " " like what you did in the test?
derekwaynecarr
force-pushed
the
fix-image-name-validation
branch
from
6d588bf to
73fdbbc
Compare
|
@gyliu513 - done |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve This can't break anyone (docker daemon won't pull), it's confusing and accidental, and it causes issues for the rest of the cluster |
k8s-github-robot
added
the
approved
marun
added
the
sig/api-machinery
pkg/api/validation/validation.go
Outdated
| @@ -1975,6 +1975,9 @@ func validateContainers(containers []api.Container, volumes sets.String, fldPath | |||
| if len(ctr.Image) == 0 { | |||
| allErrs = append(allErrs, field.Required(idxPath.Child("image"), "")) | |||
| } | |||
| if len(strings.TrimSpace(ctr.Image)) != len(ctr.Image) { | |||
There was a problem hiding this comment.
is this used to validate all objects with pod specs, or just pods?
There was a problem hiding this comment.
Yeah, can only be final pods.
There was a problem hiding this comment.
right, anyone that calls ValidatePodSpec, so this is picked up by all the job/rc/...
There was a problem hiding this comment.
i don't see how image is special. we validate every other aspect of the pod spec when doing a validate pod template (volumes, affinity, etc.)
smarterclayton
removed
the
lgtm
|
My concern is that for everything that uses a pod spec template today, |
|
@smarterclayton - opened openshift/origin#14659 i will update this pr and reference that issue for rationale until we transitioned. |
derekwaynecarr
force-pushed
the
fix-image-name-validation
branch
from
73fdbbc to
59b1bac
Compare
k8s-github-robot
added
size/M
|
@smarterclayton @liggitt - updated per request. |
|
/lgtm with the restricted scope |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, eparis, smarterclayton Associated issue: 47490 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue |
What this PR does / why we need it:
verifies that an image name can not have leading or trailing whitespace
Which issue this PR fixes
fixes #47490
Special notes for your reviewer:
i was surprised we had not caught this, so if there is an image spec that says leading and trailing whitespace is a good thing, i am open to correction.
i was made aware of downstream users of validate pod template spec that used " " as a special token. as a result, i only do the validation of image name " " in the
Podonly.Release note: