Merge pull request #33233 from yujuhong/default_image_tag

Automatic merge from submit-queue

Apply default image tags for all runtimes

Move the docker-specific logic up to the ImageManager to allow code sharing
among different implementations.

Part of #31459

/cc @kubernetes/sig-node

pkg/kubelet/dockershim/docker_image.go

@@ -65,7 +65,6 @@ func (ds *dockerService) ImageStatus(image *runtimeApi.ImageSpec) (*runtimeApi.I
 
 // PullImage pulls an image with authentication config.
 func (ds *dockerService) PullImage(image *runtimeApi.ImageSpec, auth *runtimeApi.AuthConfig) error {
-	// TODO: add default tags for images or should this be done by kubelet?
 	return ds.client.PullImage(image.GetImage(),
 		dockertypes.AuthConfig{
 			Username:      auth.GetUsername(),

pkg/kubelet/dockertools/docker.go

@@ -38,7 +38,6 @@ import (
 	"k8s.io/kubernetes/pkg/kubelet/leaky"
 	"k8s.io/kubernetes/pkg/types"
 	utilerrors "k8s.io/kubernetes/pkg/util/errors"
-	"k8s.io/kubernetes/pkg/util/parsers"
 )
 
 const (
@@ -194,32 +193,7 @@ func matchImageTagOrSHA(inspected dockertypes.ImageInspect, image string) bool {
 	return false
 }
 
-// applyDefaultImageTag parses a docker image string, if it doesn't contain any tag or digest,
-// a default tag will be applied.
-func applyDefaultImageTag(image string) (string, error) {
-	named, err := dockerref.ParseNamed(image)
-	if err != nil {
-		return "", fmt.Errorf("couldn't parse image reference %q: %v", image, err)
-	}
-	_, isTagged := named.(dockerref.Tagged)
-	_, isDigested := named.(dockerref.Digested)
-	if !isTagged && !isDigested {
-		named, err := dockerref.WithTag(named, parsers.DefaultImageTag)
-		if err != nil {
-			return "", fmt.Errorf("failed to apply default image tag %q: %v", image, err)
-		}
-		image = named.String()
-	}
-	return image, nil
-}
-
 func (p dockerPuller) Pull(image string, secrets []api.Secret) error {
-	// If the image contains no tag or digest, a default tag should be applied.
-	image, err := applyDefaultImageTag(image)
-	if err != nil {
-		return err
-	}
-
 	keyring, err := credentialprovider.MakeDockerKeyring(secrets, p.keyring)
 	if err != nil {
 		return err

pkg/kubelet/dockertools/docker_manager_test.go

@@ -110,6 +110,16 @@ func (f *fakeRuntimeHelper) GetExtraSupplementalGroupsForPod(pod *api.Pod) []int
 	return nil
 }
 
+type fakeImageManager struct{}
+
+func newFakeImageManager() images.ImageManager {
+	return &fakeImageManager{}
+}
+
+func (m *fakeImageManager) EnsureImageExists(pod *api.Pod, container *api.Container, pullSecrets []api.Secret) (error, string) {
+	return nil, ""
+}
+
 func createTestDockerManager(fakeHTTPClient *fakeHTTP, fakeDocker *FakeDockerClient) (*DockerManager, *FakeDockerClient) {
 	if fakeHTTPClient == nil {
 		fakeHTTPClient = &fakeHTTP{}
@@ -144,17 +154,26 @@ func createTestDockerManager(fakeHTTPClient *fakeHTTP, fakeDocker *FakeDockerCli
 	return dockerManager, fakeDocker
 }
 
+func createTestDockerManagerWithFakeImageManager(fakeHTTPClient *fakeHTTP, fakeDocker *FakeDockerClient) (*DockerManager, *FakeDockerClient) {
+	dm, fd := createTestDockerManager(fakeHTTPClient, fakeDocker)
+	dm.imagePuller = newFakeImageManager()
+	return dm, fd
+}
+
+func newTestDockerManagerWithRealImageManager() (*DockerManager, *FakeDockerClient) {
+	return createTestDockerManager(nil, nil)
+}
 func newTestDockerManagerWithHTTPClient(fakeHTTPClient *fakeHTTP) (*DockerManager, *FakeDockerClient) {
-	return createTestDockerManager(fakeHTTPClient, nil)
+	return createTestDockerManagerWithFakeImageManager(fakeHTTPClient, nil)
 }
 
 func newTestDockerManagerWithVersion(version, apiVersion string) (*DockerManager, *FakeDockerClient) {
 	fakeDocker := NewFakeDockerClientWithVersion(version, apiVersion)
-	return createTestDockerManager(nil, fakeDocker)
+	return createTestDockerManagerWithFakeImageManager(nil, fakeDocker)
 }
 
 func newTestDockerManager() (*DockerManager, *FakeDockerClient) {
-	return createTestDockerManager(nil, nil)
+	return createTestDockerManagerWithFakeImageManager(nil, nil)
 }
 
 func matchString(t *testing.T, pattern, str string) bool {
@@ -617,14 +636,14 @@ func TestSyncPodCreateNetAndContainer(t *testing.T) {
 }
 
 func TestSyncPodCreatesNetAndContainerPullsImage(t *testing.T) {
-	dm, fakeDocker := newTestDockerManager()
-	dm.podInfraContainerImage = "pod_infra_image"
+	dm, fakeDocker := newTestDockerManagerWithRealImageManager()
+	dm.podInfraContainerImage = "foo/infra_image:v1"
 	puller := dm.dockerPuller.(*FakeDockerPuller)
 	puller.HasImages = []string{}
-	dm.podInfraContainerImage = "pod_infra_image"
+	dm.podInfraContainerImage = "foo/infra_image:v1"
 	pod := makePod("foo", &api.PodSpec{
 		Containers: []api.Container{
-			{Name: "bar", Image: "something", ImagePullPolicy: "IfNotPresent"},
+			{Name: "bar", Image: "foo/something:v0", ImagePullPolicy: "IfNotPresent"},
 		},
 	})
 
@@ -639,7 +658,7 @@ func TestSyncPodCreatesNetAndContainerPullsImage(t *testing.T) {
 
 	fakeDocker.Lock()
 
-	if !reflect.DeepEqual(puller.ImagesPulled, []string{"pod_infra_image", "something"}) {
+	if !reflect.DeepEqual(puller.ImagesPulled, []string{"foo/infra_image:v1", "foo/something:v0"}) {
 		t.Errorf("unexpected pulled containers: %v", puller.ImagesPulled)
 	}
 
@@ -1478,18 +1497,18 @@ func TestGetIPCMode(t *testing.T) {
 }
 
 func TestSyncPodWithPullPolicy(t *testing.T) {
-	dm, fakeDocker := newTestDockerManager()
+	dm, fakeDocker := newTestDockerManagerWithRealImageManager()
 	puller := dm.dockerPuller.(*FakeDockerPuller)
-	puller.HasImages = []string{"existing_one", "want:latest"}
-	dm.podInfraContainerImage = "pod_infra_image"
+	puller.HasImages = []string{"foo/existing_one:v1", "foo/want:latest"}
+	dm.podInfraContainerImage = "foo/infra_image:v1"
 
 	pod := makePod("foo", &api.PodSpec{
 		Containers: []api.Container{
-			{Name: "bar", Image: "pull_always_image", ImagePullPolicy: api.PullAlways},
-			{Name: "bar2", Image: "pull_if_not_present_image", ImagePullPolicy: api.PullIfNotPresent},
-			{Name: "bar3", Image: "existing_one", ImagePullPolicy: api.PullIfNotPresent},
-			{Name: "bar4", Image: "want:latest", ImagePullPolicy: api.PullIfNotPresent},
-			{Name: "bar5", Image: "pull_never_image", ImagePullPolicy: api.PullNever},
+			{Name: "bar", Image: "foo/pull_always_image:v1", ImagePullPolicy: api.PullAlways},
+			{Name: "bar2", Image: "foo/pull_if_not_present_image:v1", ImagePullPolicy: api.PullIfNotPresent},
+			{Name: "bar3", Image: "foo/existing_one:v1", ImagePullPolicy: api.PullIfNotPresent},
+			{Name: "bar4", Image: "foo/want:latest", ImagePullPolicy: api.PullIfNotPresent},
+			{Name: "bar5", Image: "foo/pull_never_image:v1", ImagePullPolicy: api.PullNever},
 		},
 	})
 
@@ -1503,7 +1522,7 @@ func TestSyncPodWithPullPolicy(t *testing.T) {
 		{kubecontainer.StartContainer, "bar3", nil, ""},
 		{kubecontainer.StartContainer, "bar4", nil, ""},
 		{kubecontainer.StartContainer, "bar5", images.ErrImageNeverPull,
-			"Container image \"pull_never_image\" is not present with pull policy of Never"},
+			"Container image \"foo/pull_never_image:v1\" is not present with pull policy of Never"},
 	}
 
 	result := runSyncPod(t, dm, fakeDocker, pod, nil, true)
@@ -1514,7 +1533,7 @@ func TestSyncPodWithPullPolicy(t *testing.T) {
 
 	pulledImageSorted := puller.ImagesPulled[:]
 	sort.Strings(pulledImageSorted)
-	assert.Equal(t, []string{"pod_infra_image", "pull_always_image", "pull_if_not_present_image"}, pulledImageSorted)
+	assert.Equal(t, []string{"foo/infra_image:v1", "foo/pull_always_image:v1", "foo/pull_if_not_present_image:v1"}, pulledImageSorted)
 
 	if len(fakeDocker.Created) != 5 {
 		t.Errorf("unexpected containers created %v", fakeDocker.Created)
@@ -1533,19 +1552,19 @@ func TestSyncPodWithFailure(t *testing.T) {
 		expected    []*kubecontainer.SyncResult
 	}{
 		"PullImageFailure": {
-			api.Container{Name: "bar", Image: "realImage", ImagePullPolicy: api.PullAlways},
+			api.Container{Name: "bar", Image: "foo/real_image:v1", ImagePullPolicy: api.PullAlways},
 			map[string]error{},
 			[]error{fmt.Errorf("can't pull image")},
 			[]*kubecontainer.SyncResult{{kubecontainer.StartContainer, "bar", images.ErrImagePull, "can't pull image"}},
 		},
 		"CreateContainerFailure": {
-			api.Container{Name: "bar", Image: "alreadyPresent"},
+			api.Container{Name: "bar", Image: "foo/already_present:v2"},
 			map[string]error{"create": fmt.Errorf("can't create container")},
 			[]error{},
 			[]*kubecontainer.SyncResult{{kubecontainer.StartContainer, "bar", kubecontainer.ErrRunContainer, "can't create container"}},
 		},
 		"StartContainerFailure": {
-			api.Container{Name: "bar", Image: "alreadyPresent"},
+			api.Container{Name: "bar", Image: "foo/already_present:v2"},
 			map[string]error{"start": fmt.Errorf("can't start container")},
 			[]error{},
 			[]*kubecontainer.SyncResult{{kubecontainer.StartContainer, "bar", kubecontainer.ErrRunContainer, "can't start container"}},
@@ -1553,7 +1572,7 @@ func TestSyncPodWithFailure(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		dm, fakeDocker := newTestDockerManager()
+		dm, fakeDocker := newTestDockerManagerWithRealImageManager()
 		puller := dm.dockerPuller.(*FakeDockerPuller)
 		puller.HasImages = []string{test.container.Image}
 		// Pretend that the pod infra container has already been created, so that

pkg/kubelet/dockertools/docker_test.go

@@ -315,34 +315,16 @@ func TestMatchImageTagOrSHA(t *testing.T) {
 	}
 }
 
-func TestApplyDefaultImageTag(t *testing.T) {
-	for _, testCase := range []struct {
-		Input  string
-		Output string
-	}{
-		{Input: "root", Output: "root:latest"},
-		{Input: "root:tag", Output: "root:tag"},
-		{Input: "root@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Output: "root@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"},
-	} {
-		image, err := applyDefaultImageTag(testCase.Input)
-		if err != nil {
-			t.Errorf("applyDefaultTag(%s) failed: %v", testCase.Input, err)
-		} else if image != testCase.Output {
-			t.Errorf("Expected image reference: %q, got %q", testCase.Output, image)
-		}
-	}
-}
-
 func TestPullWithNoSecrets(t *testing.T) {
 	tests := []struct {
 		imageName     string
 		expectedImage string
 	}{
-		{"ubuntu", "ubuntu:latest using {}"},
+		{"ubuntu", "ubuntu using {}"},
 		{"ubuntu:2342", "ubuntu:2342 using {}"},
 		{"ubuntu:latest", "ubuntu:latest using {}"},
 		{"foo/bar:445566", "foo/bar:445566 using {}"},
-		{"registry.example.com:5000/foobar", "registry.example.com:5000/foobar:latest using {}"},
+		{"registry.example.com:5000/foobar", "registry.example.com:5000/foobar using {}"},
 		{"registry.example.com:5000/foobar:5342", "registry.example.com:5000/foobar:5342 using {}"},
 		{"registry.example.com:5000/foobar:latest", "registry.example.com:5000/foobar:latest using {}"},
 	}
@@ -430,7 +412,7 @@ func TestPullWithSecrets(t *testing.T) {
 			"ubuntu",
 			[]api.Secret{},
 			credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{}),
-			[]string{"ubuntu:latest using {}"},
+			[]string{"ubuntu using {}"},
 		},
 		"default keyring secrets": {
 			"ubuntu",
@@ -438,7 +420,7 @@ func TestPullWithSecrets(t *testing.T) {
 			credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{
 				"index.docker.io/v1/": {Username: "built-in", Password: "password", Email: "email", Provider: nil},
 			}),
-			[]string{`ubuntu:latest using {"username":"built-in","password":"password","email":"email"}`},
+			[]string{`ubuntu using {"username":"built-in","password":"password","email":"email"}`},
 		},
 		"default keyring secrets unused": {
 			"ubuntu",
@@ -446,7 +428,7 @@ func TestPullWithSecrets(t *testing.T) {
 			credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{
 				"extraneous": {Username: "built-in", Password: "password", Email: "email", Provider: nil},
 			}),
-			[]string{`ubuntu:latest using {}`},
+			[]string{`ubuntu using {}`},
 		},
 		"builtin keyring secrets, but use passed": {
 			"ubuntu",
@@ -454,7 +436,7 @@ func TestPullWithSecrets(t *testing.T) {
 			credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{
 				"index.docker.io/v1/": {Username: "built-in", Password: "password", Email: "email", Provider: nil},
 			}),
-			[]string{`ubuntu:latest using {"username":"passed-user","password":"passed-password","email":"passed-email"}`},
+			[]string{`ubuntu using {"username":"passed-user","password":"passed-password","email":"passed-email"}`},
 		},
 		"builtin keyring secrets, but use passed with new docker config": {
 			"ubuntu",
@@ -462,7 +444,7 @@ func TestPullWithSecrets(t *testing.T) {
 			credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{
 				"index.docker.io/v1/": {Username: "built-in", Password: "password", Email: "email", Provider: nil},
 			}),
-			[]string{`ubuntu:latest using {"username":"passed-user","password":"passed-password","email":"passed-email"}`},
+			[]string{`ubuntu using {"username":"passed-user","password":"passed-password","email":"passed-email"}`},
 		},
 	}
 	for i, test := range tests {

pkg/kubelet/images/image_manager.go

@@ -19,12 +19,14 @@ package images
 import (
 	"fmt"
 
+	dockerref "github.com/docker/distribution/reference"
 	"github.com/golang/glog"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/client/record"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
 	"k8s.io/kubernetes/pkg/kubelet/events"
 	"k8s.io/kubernetes/pkg/util/flowcontrol"
+	"k8s.io/kubernetes/pkg/util/parsers"
 )
 
 // imageManager provides the functionalities for image pulling.
@@ -87,7 +89,15 @@ func (m *imageManager) EnsureImageExists(pod *api.Pod, container *api.Container,
 		glog.Errorf("Couldn't make a ref to pod %v, container %v: '%v'", pod.Name, container.Name, err)
 	}
 
-	spec := kubecontainer.ImageSpec{Image: container.Image}
+	// If the image contains no tag or digest, a default tag should be applied.
+	image, err := applyDefaultImageTag(container.Image)
+	if err != nil {
+		msg := fmt.Sprintf("Failed to apply default image tag %q: %v", container.Image, err)
+		m.logIt(ref, api.EventTypeWarning, events.FailedToInspectImage, logPrefix, msg, glog.Warning)
+		return ErrInvalidImageName, msg
+	}
+
+	spec := kubecontainer.ImageSpec{Image: image}
 	present, err := m.imageService.IsImagePresent(spec)
 	if err != nil {
 		msg := fmt.Sprintf("Failed to inspect image %q: %v", container.Image, err)
@@ -130,3 +140,22 @@ func (m *imageManager) EnsureImageExists(pod *api.Pod, container *api.Container,
 	m.backOff.GC()
 	return nil, ""
 }
+
+// applyDefaultImageTag parses a docker image string, if it doesn't contain any tag or digest,
+// a default tag will be applied.
+func applyDefaultImageTag(image string) (string, error) {
+	named, err := dockerref.ParseNamed(image)
+	if err != nil {
+		return "", fmt.Errorf("couldn't parse image reference %q: %v", image, err)
+	}
+	_, isTagged := named.(dockerref.Tagged)
+	_, isDigested := named.(dockerref.Digested)
+	if !isTagged && !isDigested {
+		named, err := dockerref.WithTag(named, parsers.DefaultImageTag)
+		if err != nil {
+			return "", fmt.Errorf("failed to apply default image tag %q: %v", image, err)
+		}
+		image = named.String()
+	}
+	return image, nil
+}

pkg/kubelet/images/image_manager_test.go

@@ -211,3 +211,21 @@ func TestSerializedPuller(t *testing.T) {
 		}
 	}
 }
+
+func TestApplyDefaultImageTag(t *testing.T) {
+	for _, testCase := range []struct {
+		Input  string
+		Output string
+	}{
+		{Input: "root", Output: "root:latest"},
+		{Input: "root:tag", Output: "root:tag"},
+		{Input: "root@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Output: "root@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"},
+	} {
+		image, err := applyDefaultImageTag(testCase.Input)
+		if err != nil {
+			t.Errorf("applyDefaultImageTag(%s) failed: %v", testCase.Input, err)
+		} else if image != testCase.Output {
+			t.Errorf("Expected image reference: %q, got %q", testCase.Output, image)
+		}
+	}
+}

pkg/kubelet/images/types.go

@@ -37,6 +37,9 @@ var (
 
 	// Get http error when pulling image from registry
 	RegistryUnavailable = errors.New("RegistryUnavailable")
+
+	// Unable to parse the image name.
+	ErrInvalidImageName = errors.New("InvalidImageName")
 )
 
 // ImageManager provides an interface to manage the lifecycle of images.