Merge pull request #42149 from Random-Liu/check-infra-container-image-existence

Automatic merge from submit-queue (batch tested with PRs 42216, 42136, 42183, 42149, 36828)

Check infra container image existence before pulling.

Fixes https://github.com/kubernetes/kubernetes/issues/42040.

This PR:
* Fixes https://github.com/kubernetes/kubernetes/issues/42040 by checking image existence before pulling.
* Add unit test for it.
* Fix a potential panic at https://github.com/kubernetes/kubernetes/compare/master...Random-Liu:check-infra-container-image-existence?expand=1#diff-e2eefa11d78ba95197ce406772c18c30R421.

@yujuhong

pkg/kubelet/dockershim/docker_sandbox.go

@@ -61,8 +61,9 @@ func (ds *dockerService) RunPodSandbox(config *runtimeapi.PodSandboxConfig) (str
 
 	// NOTE: To use a custom sandbox image in a private repository, users need to configure the nodes with credentials properly.
 	// see: http://kubernetes.io/docs/user-guide/images/#configuring-nodes-to-authenticate-to-a-private-repository
-	if err := ds.client.PullImage(image, dockertypes.AuthConfig{}, dockertypes.ImagePullOptions{}); err != nil {
-		return "", fmt.Errorf("unable to pull image for the sandbox container: %v", err)
+	// Only pull sandbox image when it's not present - v1.PullIfNotPresent.
+	if err := ensureSandboxImageExists(ds.client, image); err != nil {
+		return "", err
 	}
 
 	// Step 2: Create the sandbox container.

pkg/kubelet/dockershim/helpers.go

@@ -320,3 +320,19 @@ func getSecurityOptSeparator(v *semver.Version) rune {
 		return dockertools.SecurityOptSeparatorNew
 	}
 }
+
+// ensureSandboxImageExists pulls the sandbox image when it's not present.
+func ensureSandboxImageExists(client dockertools.DockerInterface, image string) error {
+	_, err := client.InspectImageByRef(image)
+	if err == nil {
+		return nil
+	}
+	if !dockertools.IsImageNotFoundError(err) {
+		return fmt.Errorf("failed to inspect sandbox image %q: %v", image, err)
+	}
+	err = client.PullImage(image, dockertypes.AuthConfig{}, dockertypes.ImagePullOptions{})
+	if err != nil {
+		return fmt.Errorf("unable to pull sandbox image %q: %v", image, err)
+	}
+	return nil
+}

pkg/kubelet/dockershim/helpers_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package dockershim
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/blang/semver"
@@ -25,6 +26,7 @@ import (
 
 	"k8s.io/kubernetes/pkg/api/v1"
 	runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
+	"k8s.io/kubernetes/pkg/kubelet/dockertools"
 	"k8s.io/kubernetes/pkg/security/apparmor"
 )
 
@@ -261,3 +263,32 @@ func TestGetSecurityOptSeparator(t *testing.T) {
 		assert.Equal(t, test.expected, actual, c)
 	}
 }
+
+func TestEnsureSandboxImageExists(t *testing.T) {
+	for desc, test := range map[string]struct {
+		inject error
+		calls  []string
+		err    bool
+	}{
+		"should not pull image when it already exists": {
+			inject: nil,
+			calls:  []string{"inspect_image"},
+		},
+		"should pull image when it doesn't exist": {
+			inject: dockertools.ImageNotFoundError{ID: "image_id"},
+			calls:  []string{"inspect_image", "pull"},
+		},
+		"should return error when inspect image fails": {
+			inject: fmt.Errorf("arbitrary error"),
+			calls:  []string{"inspect_image"},
+			err:    true,
+		},
+	} {
+		t.Logf("TestCase: %q", desc)
+		_, fakeDocker, _ := newTestDockerService()
+		fakeDocker.InjectError("inspect_image", test.inject)
+		err := ensureSandboxImageExists(fakeDocker, "gcr.io/test/image")
+		assert.NoError(t, fakeDocker.AssertCalls(test.calls))
+		assert.Equal(t, test.err, err != nil)
+	}
+}

pkg/kubelet/dockertools/docker.go

@@ -290,7 +290,7 @@ func (p dockerPuller) GetImageRef(image string) (string, error) {
 		}
 		return imageRef, nil
 	}
-	if _, ok := err.(imageNotFoundError); ok {
+	if IsImageNotFoundError(err) {
 		return "", nil
 	}
 	return "", err

pkg/kubelet/dockertools/docker_manager.go

@@ -405,6 +405,7 @@ func (dm *DockerManager) inspectContainer(id string, podName, podNamespace strin
 
 	// default to the image ID, but try and inspect for the RepoDigests
 	imageID := DockerPrefix + iResult.Image
+	imageName := iResult.Config.Image
 	imgInspectResult, err := dm.client.InspectImageByID(iResult.Image)
 	if err != nil {
 		utilruntime.HandleError(fmt.Errorf("unable to inspect docker image %q while inspecting docker container %q: %v", iResult.Image, containerName, err))
@@ -416,12 +417,12 @@ func (dm *DockerManager) inspectContainer(id string, podName, podNamespace strin
 		if len(imgInspectResult.RepoDigests) > 0 {
 			imageID = DockerPullablePrefix + imgInspectResult.RepoDigests[0]
 		}
+
+		if len(imgInspectResult.RepoTags) > 0 {
+			imageName = imgInspectResult.RepoTags[0]
+		}
 	}
 
-	imageName := iResult.Config.Image
-	if len(imgInspectResult.RepoTags) > 0 {
-		imageName = imgInspectResult.RepoTags[0]
-	}
 	status := kubecontainer.ContainerStatus{
 		Name:         containerName,
 		RestartCount: containerInfo.RestartCount,

pkg/kubelet/dockertools/kube_docker_client.go

@@ -187,7 +187,7 @@ func (d *kubeDockerClient) inspectImageRaw(ref string) (*dockertypes.ImageInspec
 	}
 	if err != nil {
 		if dockerapi.IsErrImageNotFound(err) {
-			err = imageNotFoundError{ID: ref}
+			err = ImageNotFoundError{ID: ref}
 		}
 		return nil, err
 	}
@@ -202,7 +202,7 @@ func (d *kubeDockerClient) InspectImageByID(imageID string) (*dockertypes.ImageI
 	}
 
 	if !matchImageIDOnly(*resp, imageID) {
-		return nil, imageNotFoundError{ID: imageID}
+		return nil, ImageNotFoundError{ID: imageID}
 	}
 	return resp, nil
 }
@@ -214,7 +214,7 @@ func (d *kubeDockerClient) InspectImageByRef(imageRef string) (*dockertypes.Imag
 	}
 
 	if !matchImageTagOrSHA(*resp, imageRef) {
-		return nil, imageNotFoundError{ID: imageRef}
+		return nil, ImageNotFoundError{ID: imageRef}
 	}
 	return resp, nil
 }
@@ -613,18 +613,19 @@ func IsContainerNotFoundError(err error) bool {
 	return containerNotFoundErrorRegx.MatchString(err.Error())
 }
 
-// imageNotFoundError is the error returned by InspectImage when image not found.
-type imageNotFoundError struct {
+// ImageNotFoundError is the error returned by InspectImage when image not found.
+// Expose this to inject error in dockershim for testing.
+type ImageNotFoundError struct {
 	ID string
 }
 
-func (e imageNotFoundError) Error() string {
+func (e ImageNotFoundError) Error() string {
 	return fmt.Sprintf("no such image: %q", e.ID)
 }
 
 // IsImageNotFoundError checks whether the error is image not found error. This is exposed
 // to share with dockershim.
 func IsImageNotFoundError(err error) bool {
-	_, ok := err.(imageNotFoundError)
+	_, ok := err.(ImageNotFoundError)
 	return ok
 }