Merge pull request #51870 from feiskyer/sandbox-creds

Automatic merge from submit-queue (batch tested with PRs 52264, 51870)

Use credentials from providers for docker sandbox image

**What this PR does / why we need it**:

Sandbox image lookup uses creds from docker config only; other credential providers are ignored. This is a regression introduced in dockershim.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #51293

**Special notes for your reviewer**:

Should also cherry-pick this to release-1.6 and release-1.7.

**Release note**:

```release-note
Fix credentials providers for docker sandbox image.
```

pkg/kubelet/dockershim/BUILD

@@ -57,6 +57,7 @@ go_library(
         "//pkg/kubelet/util/ioutils:go_default_library",
         "//pkg/security/apparmor:go_default_library",
         "//pkg/util/hash:go_default_library",
+        "//pkg/util/parsers:go_default_library",
         "//pkg/util/term:go_default_library",
         "//vendor/github.com/blang/semver:go_default_library",
         "//vendor/github.com/docker/docker/api/types:go_default_library",

pkg/kubelet/dockershim/helpers.go

@@ -18,8 +18,6 @@ package dockershim
 
 import (
 	"fmt"
-	"os"
-	"path/filepath"
 	"regexp"
 	"strconv"
 	"strings"
@@ -31,12 +29,13 @@ import (
 	dockernat "github.com/docker/go-connections/nat"
 	"github.com/golang/glog"
 
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/kubernetes/pkg/credentialprovider"
 	runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime"
+	"k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker"
 	"k8s.io/kubernetes/pkg/kubelet/types"
 	"k8s.io/kubernetes/pkg/security/apparmor"
-
+	"k8s.io/kubernetes/pkg/util/parsers"
-	"k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker"
 )
 
 const (
@@ -347,11 +346,6 @@ func getSecurityOptSeparator(v *semver.Version) rune {
 
 // ensureSandboxImageExists pulls the sandbox image when it's not present.
 func ensureSandboxImageExists(client libdocker.Interface, image string) error {
-	dockerCfgSearchPath := []string{"/.docker", filepath.Join(os.Getenv("HOME"), ".docker")}
-	return ensureSandboxImageExistsDockerCfg(client, image, dockerCfgSearchPath)
-}
-
-func ensureSandboxImageExistsDockerCfg(client libdocker.Interface, image string, dockerCfgSearchPath []string) error {
 	_, err := client.InspectImageByRef(image)
 	if err == nil {
 		return nil
@@ -360,34 +354,37 @@ func ensureSandboxImageExistsDockerCfg(client libdocker.Interface, image string,
 		return fmt.Errorf("failed to inspect sandbox image %q: %v", image, err)
 	}
 
-	// To support images in private registries, try to read docker config
+	repoToPull, _, _, err := parsers.ParseImageName(image)
-	authConfig := dockertypes.AuthConfig{}
+	if err != nil {
-	keyring := &credentialprovider.BasicDockerKeyring{}
+		return err
-	var cfgLoadErr error
-	if cfg, err := credentialprovider.ReadDockerConfigJSONFile(dockerCfgSearchPath); err == nil {
-		keyring.Add(cfg)
-	} else if cfg, err := credentialprovider.ReadDockercfgFile(dockerCfgSearchPath); err == nil {
-		keyring.Add(cfg)
-	} else {
-		cfgLoadErr = err
-	}
-	if creds, withCredentials := keyring.Lookup(image); withCredentials {
-		// Use the first one that matched our image
-		for _, cred := range creds {
-			authConfig.Username = cred.Username
-			authConfig.Password = cred.Password
-			break
-		}
 	}
 
-	err = client.PullImage(image, authConfig, dockertypes.ImagePullOptions{})
+	keyring := credentialprovider.NewDockerKeyring()
-	if err != nil {
+	creds, withCredentials := keyring.Lookup(repoToPull)
-		if cfgLoadErr != nil {
+	if !withCredentials {
-			glog.Warningf("Couldn't load Docker cofig. If sandbox image %q is in a private registry, this will cause further errors. Error: %v", image, cfgLoadErr)
+		glog.V(3).Infof("Pulling image %q without credentials", image)
+
+		err := client.PullImage(image, dockertypes.AuthConfig{}, dockertypes.ImagePullOptions{})
+		if err != nil {
+			return fmt.Errorf("failed pulling image %q: %v", image, err)
 		}
-		return fmt.Errorf("unable to pull sandbox image %q: %v", image, err)
+
+		return nil
 	}
-	return nil
+
+	var pullErrs []error
+	for _, currentCreds := range creds {
+		authConfig := credentialprovider.LazyProvide(currentCreds)
+		err := client.PullImage(image, authConfig, dockertypes.ImagePullOptions{})
+		// If there was no error, return success
+		if err == nil {
+			return nil
+		}
+
+		pullErrs = append(pullErrs, err)
+	}
+
+	return utilerrors.NewAggregate(pullErrs)
 }
 
 func getAppArmorOpts(profile string) ([]dockerOpt, error) {

pkg/kubelet/dockershim/helpers_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package dockershim
 
 import (
-	"encoding/base64"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -31,9 +30,8 @@ import (
 	"github.com/stretchr/testify/require"
 
 	runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime"
-	"k8s.io/kubernetes/pkg/security/apparmor"
-
 	"k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker"
+	"k8s.io/kubernetes/pkg/security/apparmor"
 )
 
 func TestLabelsAndAnnotationsRoundTrip(t *testing.T) {
@@ -171,10 +169,7 @@ func writeDockerConfig(cfg string) (string, error) {
 
 func TestEnsureSandboxImageExists(t *testing.T) {
 	sandboxImage := "gcr.io/test/image"
-	registryHost := "https://gcr.io/"
 	authConfig := dockertypes.AuthConfig{Username: "user", Password: "pass"}
-	authB64 := base64.StdEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", authConfig.Username, authConfig.Password)))
-	authJSON := fmt.Sprintf("{\"auths\": {\"%s\": {\"auth\": \"%s\"} } }", registryHost, authB64)
 	for desc, test := range map[string]struct {
 		injectImage  bool
 		imgNeedsAuth bool
@@ -206,14 +201,6 @@ func TestEnsureSandboxImageExists(t *testing.T) {
 			calls:        []string{"inspect_image", "pull"},
 			err:          true,
 		},
-		"should pull private image using dockerauth if image doesn't exist": {
-			injectImage:  true,
-			imgNeedsAuth: true,
-			injectErr:    libdocker.ImageNotFoundError{ID: "image_id"},
-			calls:        []string{"inspect_image", "pull"},
-			configJSON:   authJSON,
-			err:          false,
-		},
 	} {
 		t.Logf("TestCase: %q", desc)
 		_, fakeDocker, _ := newTestDockerService()
@@ -226,15 +213,7 @@ func TestEnsureSandboxImageExists(t *testing.T) {
 		}
 		fakeDocker.InjectError("inspect_image", test.injectErr)
 
-		var dockerCfgSearchPath []string
+		err := ensureSandboxImageExists(fakeDocker, sandboxImage)
-		if test.configJSON != "" {
-			tmpdir, err := writeDockerConfig(test.configJSON)
-			require.NoError(t, err, "could not create a temp docker config file")
-			dockerCfgSearchPath = append(dockerCfgSearchPath, filepath.Join(tmpdir, ".docker"))
-			defer os.RemoveAll(tmpdir)
-		}
-
-		err := ensureSandboxImageExistsDockerCfg(fakeDocker, sandboxImage, dockerCfgSearchPath)
 		assert.NoError(t, fakeDocker.AssertCalls(test.calls))
 		assert.Equal(t, test.err, err != nil)
 	}