renaming method interface to CheckReadAccess and adding some tests cases

Signed-off-by: Juan Bustamante <[email protected]>

internal/builder/image_fetcher_wrapper.go

@@ -14,13 +14,13 @@ type ImageFetcher interface {
 	// attempt to pull a remote image first.
 	Fetch(ctx context.Context, name string, options image.FetchOptions) (imgutil.Image, error)
 
-	// CheckReadAccessValidator verifies if an image accessible with read permissions
+	// CheckReadAccess verifies if an image accessible with read permissions
 	// When FetchOptions.Daemon is true and the image doesn't exist in the daemon,
 	// the behavior is dictated by the pullPolicy, which can have the following behavior
 	//   - PullNever: returns false
 	//   - PullAlways Or PullIfNotPResent: it will check read access for the remote image.
 	// When FetchOptions.Daemon is false it will check read access for the remote image.
-	CheckReadAccessValidator(repo string, options image.FetchOptions) bool
+	CheckReadAccess(repo string, options image.FetchOptions) bool
 }
 
 type ImageFetcherWrapper struct {
@@ -42,5 +42,5 @@ func (w *ImageFetcherWrapper) Fetch(
 }
 
 func (w *ImageFetcherWrapper) CheckReadAccessValidator(repo string, options image.FetchOptions) bool {
-	return w.fetcher.CheckReadAccessValidator(repo, options)
+	return w.fetcher.CheckReadAccess(repo, options)
 }

internal/fakes/fake_image_fetcher.go

@@ -55,7 +55,7 @@ func (f *FakeImageFetcher) Fetch(ctx context.Context, name string, options image
 	return ri, nil
 }
 
-func (f *FakeImageFetcher) CheckReadAccessValidator(_ string, _ image.FetchOptions) bool {
+func (f *FakeImageFetcher) CheckReadAccess(_ string, _ image.FetchOptions) bool {
 	return true
 }
 

pkg/buildpack/downloader.go

@@ -29,7 +29,7 @@ type Logger interface {
 
 type ImageFetcher interface {
 	Fetch(ctx context.Context, name string, options image.FetchOptions) (imgutil.Image, error)
-	CheckReadAccessValidator(repo string, options image.FetchOptions) bool
+	CheckReadAccess(repo string, options image.FetchOptions) bool
 }
 
 type Downloader interface {

pkg/client/client.go

@@ -53,13 +53,13 @@ type ImageFetcher interface {
 	// There is a single invalid configuration, PullNever and daemon = false, this will always fail.
 	Fetch(ctx context.Context, name string, options image.FetchOptions) (imgutil.Image, error)
 
-	// CheckReadAccessValidator verifies if an image is accessible with read permissions
+	// CheckReadAccess verifies if an image is accessible with read permissions
 	// When FetchOptions.Daemon is true and the image doesn't exist in the daemon,
 	// the behavior is dictated by the pullPolicy, which can have the following behavior
 	//   - PullNever: returns false
 	//   - PullAlways Or PullIfNotPResent: it will check read access for the remote image.
 	// When FetchOptions.Daemon is false it will check read access for the remote image.
-	CheckReadAccessValidator(repo string, options image.FetchOptions) bool
+	CheckReadAccess(repo string, options image.FetchOptions) bool
 }
 
 //go:generate mockgen -package testmocks -destination ../testmocks/mock_blob_downloader.go github.com/buildpacks/pack/pkg/client BlobDownloader

pkg/client/common.go

@@ -133,7 +133,7 @@ func filterImageList(imageList []string, fetcher ImageFetcher, options image.Fet
 	var accessibleImages []string
 
 	for i, img := range imageList {
-		if fetcher.CheckReadAccessValidator(img, options) {
+		if fetcher.CheckReadAccess(img, options) {
 			accessibleImages = append(accessibleImages, imageList[i])
 		}
 	}

pkg/client/example_fetcher_test.go

@@ -54,6 +54,6 @@ func (f *fetcher) Fetch(_ context.Context, imageName string, _ image.FetchOption
 	return nil, errors.New("not implemented")
 }
 
-func (f *fetcher) CheckReadAccessValidator(_ string, _ FetchOptions) bool {
+func (f *fetcher) CheckReadAccess(_ string, _ FetchOptions) bool {
 	return true
 }

pkg/image/fetcher.go

@@ -123,18 +123,22 @@ func (f *Fetcher) Fetch(ctx context.Context, name string, options FetchOptions)
 	return f.fetchDaemonImage(name)
 }
 
-func (f *Fetcher) CheckReadAccessValidator(repo string, options FetchOptions) bool {
-	if !options.Daemon {
+func (f *Fetcher) CheckReadAccess(repo string, options FetchOptions) bool {
+	if !options.Daemon || options.PullPolicy == PullAlways {
 		return f.checkRemoteReadAccess(repo)
 	}
 	if _, err := f.fetchDaemonImage(repo); err != nil {
-		// Image doesn't exist in the daemon
-		// 	Pull Never: should failed
-		// 	Pull Always or Pull If Not Present: need to check the registry
-		if options.PullPolicy == PullNever {
-			return false
+		if errors.Is(err, ErrNotFound) {
+			// Image doesn't exist in the daemon
+			// 	Pull Never: should failed
+			// 	Pull If Not Present: need to check the registry
+			if options.PullPolicy == PullNever {
+				return false
+			}
+			return f.checkRemoteReadAccess(repo)
 		}
-		return f.checkRemoteReadAccess(repo)
+		f.logger.Debugf("failed reading image from the daemon %s, error: %s", repo, err.Error())
+		return false
 	}
 	// no-op
 	return true
@@ -143,6 +147,7 @@ func (f *Fetcher) CheckReadAccessValidator(repo string, options FetchOptions) bo
 func (f *Fetcher) checkRemoteReadAccess(repo string) bool {
 	img, err := remote.NewImage(repo, f.keychain)
 	if err != nil {
+		f.logger.Debugf("failed accessing remote image %s, error: %s", repo, err.Error())
 		return false
 	}
 	if ok, err := img.CheckReadAccess(); ok {

pkg/image/fetcher_test.go

@@ -12,14 +12,18 @@ import (
 	"github.com/buildpacks/imgutil"
 	"github.com/buildpacks/imgutil/local"
 	"github.com/buildpacks/imgutil/remote"
+	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/client"
+	"github.com/golang/mock/gomock"
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/heroku/color"
+	"github.com/pkg/errors"
 	"github.com/sclevine/spec"
 	"github.com/sclevine/spec/report"
 
 	"github.com/buildpacks/pack/pkg/image"
 	"github.com/buildpacks/pack/pkg/logging"
+	"github.com/buildpacks/pack/pkg/testmocks"
 	h "github.com/buildpacks/pack/testhelpers"
 )
 
@@ -404,35 +408,57 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) {
 		})
 	})
 
-	when("#CheckReadAccessValidator", func() {
+	when("#CheckReadAccess", func() {
 		var daemon bool
 
 		when("Daemon is true", func() {
 			it.Before(func() {
 				daemon = true
 			})
 
+			when("an error is thrown by the daemon", func() {
+				it.Before(func() {
+					mockController := gomock.NewController(t)
+					mockDockerClient := testmocks.NewMockCommonAPIClient(mockController)
+					mockDockerClient.EXPECT().ServerVersion(gomock.Any()).Return(types.Version{}, errors.New("something wrong happened"))
+					imageFetcher = image.NewFetcher(logging.NewLogWithWriters(&outBuf, &outBuf, logging.WithVerbose()), mockDockerClient)
+				})
+				when("PullNever", func() {
+					it("read access must be false", func() {
+						h.AssertFalse(t, imageFetcher.CheckReadAccess("pack.test/dummy", image.FetchOptions{Daemon: daemon, PullPolicy: image.PullNever}))
+						h.AssertContains(t, outBuf.String(), "failed reading image from the daemon")
+					})
+				})
+
+				when("PullIfNotPresent", func() {
+					it("read access must be false", func() {
+						h.AssertFalse(t, imageFetcher.CheckReadAccess("pack.test/dummy", image.FetchOptions{Daemon: daemon, PullPolicy: image.PullIfNotPresent}))
+						h.AssertContains(t, outBuf.String(), "failed reading image from the daemon")
+					})
+				})
+			})
+
 			when("image exists only in the daemon", func() {
 				it.Before(func() {
 					img, err := local.NewImage("pack.test/dummy", docker)
 					h.AssertNil(t, err)
 					h.AssertNil(t, img.Save())
 				})
 				when("PullAlways", func() {
-					it("read access must be true", func() {
-						h.AssertTrue(t, imageFetcher.CheckReadAccessValidator("pack.test/dummy", image.FetchOptions{Daemon: daemon, PullPolicy: image.PullAlways}))
+					it("read access must be false", func() {
+						h.AssertFalse(t, imageFetcher.CheckReadAccess("pack.test/dummy", image.FetchOptions{Daemon: daemon, PullPolicy: image.PullAlways}))
 					})
 				})
 
 				when("PullNever", func() {
 					it("read access must be true", func() {
-						h.AssertTrue(t, imageFetcher.CheckReadAccessValidator("pack.test/dummy", image.FetchOptions{Daemon: daemon, PullPolicy: image.PullNever}))
+						h.AssertTrue(t, imageFetcher.CheckReadAccess("pack.test/dummy", image.FetchOptions{Daemon: daemon, PullPolicy: image.PullNever}))
 					})
 				})
 
 				when("PullIfNotPresent", func() {
 					it("read access must be true", func() {
-						h.AssertTrue(t, imageFetcher.CheckReadAccessValidator("pack.test/dummy", image.FetchOptions{Daemon: daemon, PullPolicy: image.PullIfNotPresent}))
+						h.AssertTrue(t, imageFetcher.CheckReadAccess("pack.test/dummy", image.FetchOptions{Daemon: daemon, PullPolicy: image.PullIfNotPresent}))
 					})
 				})
 			})
@@ -445,19 +471,19 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) {
 				})
 				when("PullAlways", func() {
 					it("read access must be true", func() {
-						h.AssertTrue(t, imageFetcher.CheckReadAccessValidator(repoName, image.FetchOptions{Daemon: daemon, PullPolicy: image.PullAlways}))
+						h.AssertTrue(t, imageFetcher.CheckReadAccess(repoName, image.FetchOptions{Daemon: daemon, PullPolicy: image.PullAlways}))
 					})
 				})
 
 				when("PullNever", func() {
 					it("read access must be false", func() {
-						h.AssertFalse(t, imageFetcher.CheckReadAccessValidator(repoName, image.FetchOptions{Daemon: daemon, PullPolicy: image.PullNever}))
+						h.AssertFalse(t, imageFetcher.CheckReadAccess(repoName, image.FetchOptions{Daemon: daemon, PullPolicy: image.PullNever}))
 					})
 				})
 
 				when("PullIfNotPresent", func() {
 					it("read access must be true", func() {
-						h.AssertTrue(t, imageFetcher.CheckReadAccessValidator(repoName, image.FetchOptions{Daemon: daemon, PullPolicy: image.PullIfNotPresent}))
+						h.AssertTrue(t, imageFetcher.CheckReadAccess(repoName, image.FetchOptions{Daemon: daemon, PullPolicy: image.PullIfNotPresent}))
 					})
 				})
 			})
@@ -470,7 +496,7 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) {
 
 			when("remote image doesn't exists", func() {
 				it("fails when checking dummy image", func() {
-					h.AssertFalse(t, imageFetcher.CheckReadAccessValidator("pack.test/dummy", image.FetchOptions{Daemon: daemon}))
+					h.AssertFalse(t, imageFetcher.CheckReadAccess("pack.test/dummy", image.FetchOptions{Daemon: daemon}))
 					h.AssertContains(t, outBuf.String(), "CheckReadAccess failed for the run image pack.test/dummy")
 				})
 			})
@@ -483,7 +509,7 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) {
 				})
 
 				it("read access is valid", func() {
-					h.AssertTrue(t, imageFetcher.CheckReadAccessValidator(repoName, image.FetchOptions{Daemon: daemon}))
+					h.AssertTrue(t, imageFetcher.CheckReadAccess(repoName, image.FetchOptions{Daemon: daemon}))
 					h.AssertContains(t, outBuf.String(), fmt.Sprintf("CheckReadAccess succeeded for the run image %s", repoName))
 				})
 			})