Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg: kubelet: do not assume anything about images names #58955

Merged
merged 1 commit into from Jan 29, 2018

Conversation

@runcom
Copy link
Member

runcom commented Jan 29, 2018

This patch fixes a regression introduced by
#51751 in the CRI
interface.
That commit actually changed a unit test where we were previously not
assuming anything about an image name.
Before that commit, if you send the image "busybox" through the CRI,
the container runtime receives "busybox". After that patch the
container runtime gets "docker.io/library/busybox".
While that may be correct for the internal kube dockershim, in the CRI
we must not assume anything about image names. The ImageSpec is not
providing any spec around the image so the container runtime should
just get the raw image name from the pod spec. Every container runtime
can handle image names the way it wants. The "docker.io" namespace is
not at all "standard", CRI-O is not following what the docker UI say
since that's the docker UI. We should not focus the CRI on wrong UI
design, especially around a default namespace.
Image name normalization is a Docker implementation detail around short images names, not the CRI.

ImageSpec is not standardized yet:
#46255 and
#7203

This is something which should land in 1.9 as well since the regression
is from 1.8.

Signed-off-by: Antonio Murdaca runcom@redhat.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Fix regression in the CRI: do not add a default hostname on short image names
@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Jan 29, 2018

@mrunalp

This comment has been minimized.

Copy link
Contributor

mrunalp commented Jan 29, 2018

👍

@Random-Liu

This comment has been minimized.

Copy link
Member

Random-Liu commented Jan 29, 2018

While that may be correct for the internal kube dockershim, in the CRI
we must not assume anything about image names. The ImageSpec is not
providing any spec around the image so the container runtime should
just get the raw image name from the pod spec. Every container runtime
can handle image names the way it wants.

Before image distribution is standardized by OCI, docker distribution is the only thing we have. We are using dockerref anyway today.

But I think it's fine to remove the docker.io from default image ref.

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Jan 29, 2018

The behavior change was inadvertent as part of the cAdvisor dep update.

The kubelet should not assume anything about image names and should pass directly what the user provided.

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Jan 29, 2018

Before image distribution is standardized by OCI, docker distribution is the only thing we have. We are using dockerref anyway today.

but naming and the default docker.io namespace is still yet to be decided and unlikely short image names get a docker.io hostname.

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Jan 29, 2018

with this change, we most likely could also revert #53161

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Jan 29, 2018

/approve
/lgtm

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Jan 29, 2018

/kind bug

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jan 29, 2018

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@derekwaynecarr @runcom

Action required: This pull request requires label changes. If the required changes are not made within 2 days, the pull request will be moved out of the v1.9 milestone.

priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.
sig owner: Must specify at least one label prefixed with sig/.

Help
return "", fmt.Errorf("failed to apply default image tag %q: %v", image, err)
}
image = named.String()
image = image + ":" + parsers.DefaultImageTag

This comment has been minimized.

@derekwaynecarr

derekwaynecarr Jan 29, 2018

Member

if you have time before this merges, please leave a note explaining why you are not using named in this context so its not updated in a future pr inadvertently.

pkg: kubelet: do not assume anything about images names
This patch fixes a regression introduced by
#51751 in the CRI
interface.
That commit actually changed a unit test where we were previously *not*
assuming anything about an image name.
Before that commit, if you send the image "busybox" through the CRI,
the container runtime receives "busybox". After that patch the
container runtime gets "docker.io/library/busybox".
While that may be correct for the internal kube dockershim, in the CRI
we must not assume anything about image names. The ImageSpec is not
providing any spec around the image so the container runtime should
just get the raw image name from the pod spec. Every container runtime
can handle image names the way it wants. The "docker.io" namespace is
not at all "standard", CRI-O is not following what the docker UI say
since that's the docker UI. We should not focus the CRI on wrong UI
design, especially around a default namespace.

ImageSpec is not standardized yet:
#46255 and
#7203

This is something which should land in 1.9 as well since the regression
is from 1.8.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

@runcom runcom force-pushed the runcom:fix-cri-image-spec branch from fceb3a5 to 520b99c Jan 29, 2018

@k8s-github-robot k8s-github-robot removed the lgtm label Jan 29, 2018

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Jan 29, 2018

@runcom thanks for the note.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 29, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 29, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, runcom

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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jan 29, 2018

Automatic merge from submit-queue (batch tested with PRs 58955, 58968, 58971, 58963, 58298). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit da601bc into kubernetes:master Jan 29, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation runcom authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@runcom runcom deleted the runcom:fix-cri-image-spec branch Jan 29, 2018

openshift-merge-robot added a commit to openshift/origin that referenced this pull request Jan 31, 2018

Merge pull request #18340 from sjenning/pick-58955
Automatic merge from submit-queue (batch tested with PRs 18225, 18351, 18331, 18340, 18326).

UPSTREAM: 58955: pkg: kubelet: do not assume anything about images names

kubernetes/kubernetes#58955

removes the need for #18020

see history:
kubernetes/kubernetes#51751
kubernetes/kubernetes#52110
kubernetes/kubernetes#53161

@liggitt @derekwaynecarr @frobware @mrunalp @runcom

openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Jan 31, 2018

Merge pull request kubernetes#18340 from sjenning/pick-58955
Automatic merge from submit-queue (batch tested with PRs 18225, 18351, 18331, 18340, 18326).

UPSTREAM: 58955: pkg: kubelet: do not assume anything about images names

kubernetes#58955

removes the need for openshift/origin#18020

see history:
kubernetes#51751
kubernetes#52110
kubernetes#53161

@liggitt @derekwaynecarr @frobware @mrunalp @runcom

Origin-commit: d91de347457d7f6f3d1859c671c7355cc193d038

openshift-merge-robot added a commit to openshift/origin that referenced this pull request Feb 15, 2018

Merge pull request #18605 from sjenning/revert-18020
Automatic merge from submit-queue (batch tested with PRs 18624, 18605).

Revert "Admission plugin: openshift.io/ImageQualify"

This reverts commit 8bc3e07.  #18020

Render unneeded by upstream kubernetes/kubernetes#58955 and Origin pick #18340

Will live on forever in git history if we find a use case for it later.

openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Feb 27, 2018

Merge pull request kubernetes#18340 from sjenning/pick-58955
Automatic merge from submit-queue (batch tested with PRs 18225, 18351, 18331, 18340, 18326).

UPSTREAM: 58955: pkg: kubelet: do not assume anything about images names

kubernetes#58955

removes the need for openshift/origin#18020

see history:
kubernetes#51751
kubernetes#52110
kubernetes#53161

@liggitt @derekwaynecarr @frobware @mrunalp @runcom

Origin-commit: d91de347457d7f6f3d1859c671c7355cc193d038
@k8s-cherrypick-bot

This comment has been minimized.

Copy link

k8s-cherrypick-bot commented Mar 5, 2018

Commit found in the "release-1.9" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Mar 5, 2018

This change is Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.