From 69c5a50c05b0d57bc4ef4f5da852841c65d4c4ab Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Mon, 15 Apr 2024 15:02:49 -0500 Subject: [PATCH] renaming method interface to CheckReadAccess and adding some tests cases Signed-off-by: Juan Bustamante --- internal/builder/image_fetcher_wrapper.go | 6 +-- internal/fakes/fake_image_fetcher.go | 2 +- pkg/buildpack/downloader.go | 2 +- pkg/client/client.go | 4 +- pkg/client/common.go | 2 +- pkg/image/fetcher.go | 21 +++++++---- pkg/image/fetcher_test.go | 46 ++++++++++++++++++----- pkg/testmocks/mock_image_fetcher.go | 6 +-- 8 files changed, 60 insertions(+), 29 deletions(-) diff --git a/internal/builder/image_fetcher_wrapper.go b/internal/builder/image_fetcher_wrapper.go index 61ec4d40b..71fbebf54 100644 --- a/internal/builder/image_fetcher_wrapper.go +++ b/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) } diff --git a/internal/fakes/fake_image_fetcher.go b/internal/fakes/fake_image_fetcher.go index e2c21722b..9e09ecfde 100644 --- a/internal/fakes/fake_image_fetcher.go +++ b/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 } diff --git a/pkg/buildpack/downloader.go b/pkg/buildpack/downloader.go index 68bc32310..9fab38321 100644 --- a/pkg/buildpack/downloader.go +++ b/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 { diff --git a/pkg/client/client.go b/pkg/client/client.go index 8d0272d2e..fe8ebbd8a 100644 --- a/pkg/client/client.go +++ b/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 diff --git a/pkg/client/common.go b/pkg/client/common.go index 8ca77edd0..e8e7f07ab 100644 --- a/pkg/client/common.go +++ b/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]) } } diff --git a/pkg/image/fetcher.go b/pkg/image/fetcher.go index 2e941e3ce..aa6a1b720 100644 --- a/pkg/image/fetcher.go +++ b/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 { diff --git a/pkg/image/fetcher_test.go b/pkg/image/fetcher_test.go index f07598a48..a120ac053 100644 --- a/pkg/image/fetcher_test.go +++ b/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,7 +408,7 @@ 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() { @@ -412,6 +416,28 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) { 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) @@ -419,20 +445,20 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) { 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)) }) }) diff --git a/pkg/testmocks/mock_image_fetcher.go b/pkg/testmocks/mock_image_fetcher.go index 02bb71504..6fd890db2 100644 --- a/pkg/testmocks/mock_image_fetcher.go +++ b/pkg/testmocks/mock_image_fetcher.go @@ -38,9 +38,9 @@ func (m *MockImageFetcher) EXPECT() *MockImageFetcherMockRecorder { } // CheckReadAccessValidator mocks base method. -func (m *MockImageFetcher) CheckReadAccessValidator(arg0 string, arg1 image.FetchOptions) bool { +func (m *MockImageFetcher) CheckReadAccess(arg0 string, arg1 image.FetchOptions) bool { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CheckReadAccessValidator", arg0, arg1) + ret := m.ctrl.Call(m, "CheckReadAccess", arg0, arg1) ret0, _ := ret[0].(bool) return ret0 } @@ -48,7 +48,7 @@ func (m *MockImageFetcher) CheckReadAccessValidator(arg0 string, arg1 image.Fetc // CheckReadAccessValidator indicates an expected call of CheckReadAccessValidator. func (mr *MockImageFetcherMockRecorder) CheckReadAccessValidator(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckReadAccessValidator", reflect.TypeOf((*MockImageFetcher)(nil).CheckReadAccessValidator), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckReadAccess", reflect.TypeOf((*MockImageFetcher)(nil).CheckReadAccess), arg0, arg1) } // Fetch mocks base method.