Merge pull request #116231 from kannon92/kubelet-image-cleanup

Using parsers in applyDefaultImageTag and adding error test cases.

pkg/kubelet/container/testing/fake_runtime.go

@@ -115,38 +115,6 @@ func (f *FakeRuntimeCache) ForceUpdateIfOlder(context.Context, time.Time) error
 	return nil
 }
 
-// ClearCalls resets the FakeRuntime to the initial state.
-func (f *FakeRuntime) ClearCalls() {
-	f.Lock()
-	defer f.Unlock()
-
-	f.CalledFunctions = []string{}
-	f.PodList = []*FakePod{}
-	f.AllPodList = []*FakePod{}
-	f.APIPodStatus = v1.PodStatus{}
-	f.StartedPods = []string{}
-	f.KilledPods = []string{}
-	f.StartedContainers = []string{}
-	f.KilledContainers = []string{}
-	f.RuntimeStatus = nil
-	f.VersionInfo = ""
-	f.RuntimeType = ""
-	f.Err = nil
-	f.InspectErr = nil
-	f.StatusErr = nil
-	f.BlockImagePulls = false
-	if f.imagePullTokenBucket != nil {
-		for {
-			select {
-			case f.imagePullTokenBucket <- true:
-			default:
-				f.imagePullTokenBucket = nil
-				return
-			}
-		}
-	}
-}
-
 // UpdatePodCIDR fulfills the cri interface.
 func (f *FakeRuntime) UpdatePodCIDR(_ context.Context, c string) error {
 	return nil

pkg/kubelet/images/image_manager.go

@@ -22,7 +22,6 @@ import (
 	"strings"
 	"time"
 
-	dockerref "github.com/docker/distribution/reference"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
@@ -33,6 +32,7 @@ import (
 	crierrors "k8s.io/cri-api/pkg/errors"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
 	"k8s.io/kubernetes/pkg/kubelet/events"
+	"k8s.io/kubernetes/pkg/util/parsers"
 )
 
 type ImagePodPullingTimeRecorder interface {
@@ -204,19 +204,18 @@ func evalCRIPullErr(container *v1.Container, err error) (errMsg string, errRes e
 // 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.ParseNormalizedNamed(image)
+	_, tag, digest, err := parsers.ParseImageName(image)
 	if err != nil {
-		return "", fmt.Errorf("couldn't parse image reference %q: %v", image, err)
+		return "", err
 	}
-	_, isTagged := named.(dockerref.Tagged)
-	_, isDigested := named.(dockerref.Digested)
-	if !isTagged && !isDigested {
+	// we just concatenate the image name with the default tag here instead
+	if len(digest) == 0 && len(tag) > 0 && !strings.HasSuffix(image, ":"+tag) {
 		// we just concatenate the image name with the default tag here instead
 		// of using dockerref.WithTag(named, ...) because that would cause the
 		// image to be fully qualified as docker.io/$name if it's a short name
 		// (e.g. just busybox). We don't want that to happen to keep the CRI
 		// agnostic wrt image names and default hostnames.
-		image = image + ":latest"
+		image = image + ":" + tag
 	}
 	return image, nil
 }

pkg/kubelet/images/image_manager_test.go

@@ -163,6 +163,39 @@ func pullerTestCases() []pullerTestCase {
 				{[]string{"GetImageRef"}, ErrImagePull, true, false},
 				{[]string{"GetImageRef"}, ErrImagePullBackOff, false, false},
 			}},
+		// error case if image name fails validation due to invalid reference format
+		{containerImage: "FAILED_IMAGE",
+			testName:   "invalid image name, no pull",
+			policy:     v1.PullAlways,
+			inspectErr: nil,
+			pullerErr:  nil,
+			qps:        0.0,
+			burst:      0,
+			expected: []pullerExpects{
+				{[]string(nil), ErrInvalidImageName, false, false},
+			}},
+		// error case if image name contains http
+		{containerImage: "http://url",
+			testName:   "invalid image name with http, no pull",
+			policy:     v1.PullAlways,
+			inspectErr: nil,
+			pullerErr:  nil,
+			qps:        0.0,
+			burst:      0,
+			expected: []pullerExpects{
+				{[]string(nil), ErrInvalidImageName, false, false},
+			}},
+		// error case if image name contains sha256
+		{containerImage: "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad",
+			testName:   "invalid image name with sha256, no pull",
+			policy:     v1.PullAlways,
+			inspectErr: nil,
+			pullerErr:  nil,
+			qps:        0.0,
+			burst:      0,
+			expected: []pullerExpects{
+				{[]string(nil), ErrInvalidImageName, false, false},
+			}},
 	}
 }
 
@@ -191,7 +224,7 @@ func (m *mockPodPullingTimeRecorder) reset() {
 	m.finishedPullingRecorded = false
 }
 
-func pullerTestEnv(c pullerTestCase, serialized bool, maxParallelImagePulls *int32) (puller ImageManager, fakeClock *testingclock.FakeClock, fakeRuntime *ctest.FakeRuntime, container *v1.Container, fakePodPullingTimeRecorder *mockPodPullingTimeRecorder) {
+func pullerTestEnv(t *testing.T, c pullerTestCase, serialized bool, maxParallelImagePulls *int32) (puller ImageManager, fakeClock *testingclock.FakeClock, fakeRuntime *ctest.FakeRuntime, container *v1.Container, fakePodPullingTimeRecorder *mockPodPullingTimeRecorder) {
 	container = &v1.Container{
 		Name:            "container_name",
 		Image:           c.containerImage,
@@ -202,7 +235,7 @@ func pullerTestEnv(c pullerTestCase, serialized bool, maxParallelImagePulls *int
 	fakeClock = testingclock.NewFakeClock(time.Now())
 	backOff.Clock = fakeClock
 
-	fakeRuntime = &ctest.FakeRuntime{}
+	fakeRuntime = &ctest.FakeRuntime{T: t}
 	fakeRecorder := &record.FakeRecorder{}
 
 	fakeRuntime.ImageList = []Image{{ID: "present_image:latest"}}
@@ -230,7 +263,7 @@ func TestParallelPuller(t *testing.T) {
 	for _, c := range cases {
 		t.Run(c.testName, func(t *testing.T) {
 			ctx := context.Background()
-			puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder := pullerTestEnv(c, useSerializedEnv, nil)
+			puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder := pullerTestEnv(t, c, useSerializedEnv, nil)
 
 			for _, expected := range c.expected {
 				fakeRuntime.CalledFunctions = nil
@@ -262,7 +295,7 @@ func TestSerializedPuller(t *testing.T) {
 	for _, c := range cases {
 		t.Run(c.testName, func(t *testing.T) {
 			ctx := context.Background()
-			puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder := pullerTestEnv(c, useSerializedEnv, nil)
+			puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder := pullerTestEnv(t, c, useSerializedEnv, nil)
 
 			for _, expected := range c.expected {
 				fakeRuntime.CalledFunctions = nil
@@ -288,6 +321,8 @@ func TestApplyDefaultImageTag(t *testing.T) {
 		{testName: "root", Input: "root", Output: "root:latest"},
 		{testName: "root:tag", Input: "root:tag", Output: "root:tag"},
 		{testName: "root@sha", Input: "root@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Output: "root@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"},
+		{testName: "root:latest@sha", Input: "root:latest@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Output: "root:latest@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"},
+		{testName: "root:latest", Input: "root:latest", Output: "root:latest"},
 	} {
 		t.Run(testCase.testName, func(t *testing.T) {
 			image, err := applyDefaultImageTag(testCase.Input)
@@ -324,7 +359,7 @@ func TestPullAndListImageWithPodAnnotations(t *testing.T) {
 	useSerializedEnv := true
 	t.Run(c.testName, func(t *testing.T) {
 		ctx := context.Background()
-		puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder := pullerTestEnv(c, useSerializedEnv, nil)
+		puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder := pullerTestEnv(t, c, useSerializedEnv, nil)
 		fakeRuntime.CalledFunctions = nil
 		fakeRuntime.ImageList = []Image{}
 		fakeClock.Step(time.Second)
@@ -374,7 +409,7 @@ func TestMaxParallelImagePullsLimit(t *testing.T) {
 	maxParallelImagePulls := 5
 	var wg sync.WaitGroup
 
-	puller, fakeClock, fakeRuntime, container, _ := pullerTestEnv(*testCase, useSerializedEnv, utilpointer.Int32Ptr(int32(maxParallelImagePulls)))
+	puller, fakeClock, fakeRuntime, container, _ := pullerTestEnv(t, *testCase, useSerializedEnv, utilpointer.Int32Ptr(int32(maxParallelImagePulls)))
 	fakeRuntime.BlockImagePulls = true
 	fakeRuntime.CalledFunctions = nil
 	fakeRuntime.T = t

pkg/kubelet/kuberuntime/kuberuntime_image_test.go

@@ -67,6 +67,21 @@ func TestPullImageWithError(t *testing.T) {
 	assert.Equal(t, 0, len(images))
 }
 
+func TestPullImageWithInvalidImageName(t *testing.T) {
+	_, fakeImageService, fakeManager, err := createTestRuntimeManager()
+	assert.NoError(t, err)
+
+	imageList := []string{"FAIL", "http://fail", "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad"}
+	fakeImageService.SetFakeImages(imageList)
+	for _, val := range imageList {
+		ctx := context.Background()
+		imageRef, err := fakeManager.PullImage(ctx, kubecontainer.ImageSpec{Image: val}, nil, nil)
+		assert.Error(t, err)
+		assert.Equal(t, "", imageRef)
+
+	}
+}
+
 func TestListImages(t *testing.T) {
 	ctx := context.Background()
 	_, fakeImageService, fakeManager, err := createTestRuntimeManager()

pkg/util/parsers/parsers.go

@@ -31,7 +31,7 @@ import (
 func ParseImageName(image string) (string, string, string, error) {
 	named, err := dockerref.ParseNormalizedNamed(image)
 	if err != nil {
-		return "", "", "", fmt.Errorf("couldn't parse image name: %v", err)
+		return "", "", "", fmt.Errorf("couldn't parse image name %q: %v", image, err)
 	}
 
 	repoToPull := named.Name()

pkg/util/parsers/parsers_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package parsers
 
 import (
+	"strings"
 	"testing"
 )
 
@@ -24,10 +25,11 @@ import (
 // https://github.com/docker/docker/commit/4352da7803d182a6013a5238ce20a7c749db979a
 func TestParseImageName(t *testing.T) {
 	testCases := []struct {
-		Input  string
-		Repo   string
-		Tag    string
-		Digest string
+		Input         string
+		Repo          string
+		Tag           string
+		Digest        string
+		expectedError string
 	}{
 		{Input: "root", Repo: "docker.io/library/root", Tag: "latest"},
 		{Input: "root:tag", Repo: "docker.io/library/root", Tag: "tag"},
@@ -38,12 +40,19 @@ func TestParseImageName(t *testing.T) {
 		{Input: "url:5000/repo", Repo: "url:5000/repo", Tag: "latest"},
 		{Input: "url:5000/repo:tag", Repo: "url:5000/repo", Tag: "tag"},
 		{Input: "url:5000/repo@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Repo: "url:5000/repo", Digest: "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"},
+		{Input: "url:5000/repo:latest@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Tag: "latest", Repo: "url:5000/repo", Digest: "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"},
+		{Input: "ROOT", expectedError: "repository name must be lowercase"},
+		{Input: "http://root", expectedError: "invalid reference format"},
+		{Input: "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", expectedError: "cannot specify 64-byte hexadecimal strings"},
 	}
 	for _, testCase := range testCases {
 		repo, tag, digest, err := ParseImageName(testCase.Input)
-		if err != nil {
+		switch {
+		case testCase.expectedError != "" && !strings.Contains(err.Error(), testCase.expectedError):
+			t.Errorf("ParseImageName(%s) expects error %v but did not get one", testCase.Input, err)
+		case testCase.expectedError == "" && err != nil:
 			t.Errorf("ParseImageName(%s) failed: %v", testCase.Input, err)
-		} else if repo != testCase.Repo || tag != testCase.Tag || digest != testCase.Digest {
+		case repo != testCase.Repo || tag != testCase.Tag || digest != testCase.Digest:
 			t.Errorf("Expected repo: %q, tag: %q and digest: %q, got %q, %q and %q", testCase.Repo, testCase.Tag, testCase.Digest,
 				repo, tag, digest)
 		}