diff --git a/pkg/kubelet/container/testing/fake_runtime.go b/pkg/kubelet/container/testing/fake_runtime.go index 3b89f5c6560..bf82205303c 100644 --- a/pkg/kubelet/container/testing/fake_runtime.go +++ b/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 diff --git a/pkg/kubelet/images/image_manager.go b/pkg/kubelet/images/image_manager.go index 8910f64097f..f7f08133b3a 100644 --- a/pkg/kubelet/images/image_manager.go +++ b/pkg/kubelet/images/image_manager.go @@ -19,9 +19,9 @@ package images import ( "context" "fmt" + "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" @@ -32,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 { @@ -189,19 +190,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 } diff --git a/pkg/kubelet/images/image_manager_test.go b/pkg/kubelet/images/image_manager_test.go index 0e04c39e907..28152df89fc 100644 --- a/pkg/kubelet/images/image_manager_test.go +++ b/pkg/kubelet/images/image_manager_test.go @@ -162,6 +162,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}, + }}, } } @@ -190,7 +223,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, @@ -201,7 +234,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"}} @@ -229,7 +262,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 @@ -261,7 +294,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 @@ -287,6 +320,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) @@ -323,7 +358,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) @@ -373,7 +408,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 diff --git a/pkg/kubelet/kuberuntime/kuberuntime_image_test.go b/pkg/kubelet/kuberuntime/kuberuntime_image_test.go index 9ec10cb30de..53e964885bb 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_image_test.go +++ b/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() diff --git a/pkg/util/parsers/parsers.go b/pkg/util/parsers/parsers.go index ef869cd768e..75130a8628c 100644 --- a/pkg/util/parsers/parsers.go +++ b/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() diff --git a/pkg/util/parsers/parsers_test.go b/pkg/util/parsers/parsers_test.go index c3e004fbd96..42a0c6d299f 100644 --- a/pkg/util/parsers/parsers_test.go +++ b/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) }