From 81d9698496b4da9eb65c49f14e38a9b1d3bebf03 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Fri, 8 Mar 2024 12:56:34 -0500 Subject: [PATCH 1/6] Adding a nop-op when trying to check access for run-image against the daemon Signed-off-by: Juan Bustamante --- internal/fakes/fake_access_checker.go | 5 +++- pkg/client/client.go | 2 +- pkg/client/common.go | 9 ++++--- pkg/client/common_test.go | 27 ++++++++++++++++--- pkg/image/access_checker.go | 7 ++++- pkg/image/access_checker_test.go | 37 ++++++++++++++++++++++----- 6 files changed, 70 insertions(+), 17 deletions(-) diff --git a/internal/fakes/fake_access_checker.go b/internal/fakes/fake_access_checker.go index 9912df97dc..c4d731774f 100644 --- a/internal/fakes/fake_access_checker.go +++ b/internal/fakes/fake_access_checker.go @@ -8,7 +8,10 @@ func NewFakeAccessChecker() *FakeAccessChecker { return &FakeAccessChecker{} } -func (f *FakeAccessChecker) Check(repo string) bool { +func (f *FakeAccessChecker) Check(repo string, publish bool) bool { + if !publish { + return true + } for _, toFail := range f.RegistriesToFail { if toFail == repo { return false diff --git a/pkg/client/client.go b/pkg/client/client.go index d034851fc6..d46ccb2d2b 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -84,7 +84,7 @@ type BuildpackDownloader interface { // AccessChecker is an interface for checking remote images for read access type AccessChecker interface { - Check(repo string) bool + Check(repo string, publish bool) bool } // Client is an orchestration object, it contains all parameters needed to diff --git a/pkg/client/common.go b/pkg/client/common.go index d3c67d882b..ff824c5d3e 100644 --- a/pkg/client/common.go +++ b/pkg/client/common.go @@ -44,6 +44,7 @@ func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, run runImageMetadata.Image, runImageMetadata.Mirrors, additionalMirrors[runImageMetadata.Image], + publish, accessChecker, ) @@ -108,8 +109,8 @@ func contains(slc []string, v string) bool { return false } -func getBestRunMirror(registry string, runImage string, mirrors []string, preferredMirrors []string, accessChecker AccessChecker) string { - runImageList := filterImageList(append(append(append([]string{}, preferredMirrors...), runImage), mirrors...), accessChecker) +func getBestRunMirror(registry string, runImage string, mirrors []string, preferredMirrors []string, publish bool, accessChecker AccessChecker) string { + runImageList := filterImageList(append(append(append([]string{}, preferredMirrors...), runImage), mirrors...), publish, accessChecker) for _, img := range runImageList { ref, err := name.ParseReference(img, name.WeakValidation) if err != nil { @@ -127,11 +128,11 @@ func getBestRunMirror(registry string, runImage string, mirrors []string, prefer return runImage } -func filterImageList(imageList []string, accessChecker AccessChecker) []string { +func filterImageList(imageList []string, publish bool, accessChecker AccessChecker) []string { var accessibleImages []string for i, img := range imageList { - if accessChecker.Check(img) { + if accessChecker.Check(img, publish) { accessibleImages = append(accessibleImages, imageList[i]) } } diff --git a/pkg/client/common_test.go b/pkg/client/common_test.go index 76138b5678..e610020760 100644 --- a/pkg/client/common_test.go +++ b/pkg/client/common_test.go @@ -131,9 +131,30 @@ func testCommon(t *testing.T, when spec.G, it spec.S) { accessChecker.RegistriesToFail = nil }) - it("selects the first accessible run-image", func() { - runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, true, accessChecker) - assert.Equal(runImageName, defaultMirror) + when("publish is true", func() { + it("selects the first accessible run-image", func() { + runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, true, accessChecker) + assert.Equal(runImageName, defaultMirror) + }) + }) + + when("publish is false", func() { + it.Before(func() { + stackInfo = builder.StackMetadata{ + RunImage: builder.RunImageMetadata{ + Image: "stack/run-image", + }, + } + accessChecker.RegistriesToFail = []string{ + stackInfo.RunImage.Image, + } + }) + + it("selects the given run-image", func() { + // issue: https://github.com/buildpacks/pack/issues/2078 + runImageName := subject.resolveRunImage("", "", "", stackInfo.RunImage, nil, false, accessChecker) + assert.Equal(runImageName, "stack/run-image") + }) }) }) }) diff --git a/pkg/image/access_checker.go b/pkg/image/access_checker.go index bc9ae55cd0..50401a104f 100644 --- a/pkg/image/access_checker.go +++ b/pkg/image/access_checker.go @@ -25,7 +25,12 @@ func NewAccessChecker(logger logging.Logger, keychain authn.Keychain) *Checker { return checker } -func (c *Checker) Check(repo string) bool { +func (c *Checker) Check(repo string, publish bool) bool { + if !publish { + // nop checker, we are running against the daemon + return true + } + img, err := remote.NewImage(repo, c.keychain) if err != nil { return false diff --git a/pkg/image/access_checker_test.go b/pkg/image/access_checker_test.go index 9b1bf3ebd6..d16de8b22a 100644 --- a/pkg/image/access_checker_test.go +++ b/pkg/image/access_checker_test.go @@ -18,17 +18,40 @@ func TestChecker(t *testing.T) { } func testChecker(t *testing.T, when spec.G, it spec.S) { + var publish bool + when("#Check", func() { - it("fails when checking dummy image", func() { - buf := &bytes.Buffer{} + when("publish is false", func() { + it.Before(func() { + publish = false + }) + + // issue: https://github.com/buildpacks/pack/issues/2078 + it("returns true", func() { + buf := &bytes.Buffer{} + keychain, err := auth.DefaultKeychain("pack-test/dummy") + h.AssertNil(t, err) + + ic := image.NewAccessChecker(logging.NewSimpleLogger(buf), keychain) + h.AssertTrue(t, ic.Check("pack.test/dummy", publish)) + }) + }) + + when("publish is true", func() { + it.Before(func() { + publish = true + }) - keychain, err := auth.DefaultKeychain("pack.test/dummy") - h.AssertNil(t, err) + it("fails when checking dummy image", func() { + buf := &bytes.Buffer{} + keychain, err := auth.DefaultKeychain("pack.test/dummy") + h.AssertNil(t, err) - ic := image.NewAccessChecker(logging.NewSimpleLogger(buf), keychain) + ic := image.NewAccessChecker(logging.NewSimpleLogger(buf), keychain) - h.AssertFalse(t, ic.Check("pack.test/dummy")) - h.AssertContains(t, buf.String(), "DEBUG: CheckReadAccess failed for the run image pack.test/dummy") + h.AssertFalse(t, ic.Check("pack.test/dummy", publish)) + h.AssertContains(t, buf.String(), "DEBUG: CheckReadAccess failed for the run image pack.test/dummy") + }) }) }) } From b53508619bf31c282de4a73158fe190e733b3ac2 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Wed, 3 Apr 2024 17:37:46 -0500 Subject: [PATCH 2/6] refactoring the logic, I moved the check read access to a method in the fetcher Signed-off-by: Juan Bustamante --- internal/builder/image_fetcher_wrapper.go | 5 + internal/fakes/fake_access_checker.go | 22 ---- internal/fakes/fake_image_fetcher.go | 6 + pkg/buildpack/downloader.go | 1 + pkg/client/build.go | 2 +- pkg/client/build_test.go | 3 - pkg/client/client.go | 22 +--- pkg/client/client_test.go | 10 -- pkg/client/common.go | 16 +-- pkg/client/common_test.go | 135 +++++++++++++--------- pkg/client/example_fetcher_test.go | 6 + pkg/client/rebase.go | 1 - pkg/client/rebase_test.go | 12 +- pkg/image/access_checker.go | 46 -------- pkg/image/access_checker_test.go | 57 --------- pkg/image/fetcher.go | 24 ++++ pkg/image/fetcher_test.go | 48 +++++++- pkg/testmocks/mock_image_fetcher.go | 14 +++ 18 files changed, 202 insertions(+), 228 deletions(-) delete mode 100644 internal/fakes/fake_access_checker.go delete mode 100644 pkg/image/access_checker.go delete mode 100644 pkg/image/access_checker_test.go diff --git a/internal/builder/image_fetcher_wrapper.go b/internal/builder/image_fetcher_wrapper.go index dfb0de4a83..613a9647d4 100644 --- a/internal/builder/image_fetcher_wrapper.go +++ b/internal/builder/image_fetcher_wrapper.go @@ -13,6 +13,7 @@ type ImageFetcher interface { // If daemon is true, it will look return a `local.Image`. Pull, applicable only when daemon is true, will // attempt to pull a remote image first. Fetch(ctx context.Context, name string, options image.FetchOptions) (imgutil.Image, error) + CheckReadAccessValidator(options image.FetchOptions) image.CheckReadAccess } type ImageFetcherWrapper struct { @@ -32,3 +33,7 @@ func (w *ImageFetcherWrapper) Fetch( ) (Inspectable, error) { return w.fetcher.Fetch(ctx, name, options) } + +func (w *ImageFetcherWrapper) CheckReadAccessValidator(options image.FetchOptions) image.CheckReadAccess { + return w.fetcher.CheckReadAccessValidator(options) +} diff --git a/internal/fakes/fake_access_checker.go b/internal/fakes/fake_access_checker.go deleted file mode 100644 index c4d731774f..0000000000 --- a/internal/fakes/fake_access_checker.go +++ /dev/null @@ -1,22 +0,0 @@ -package fakes - -type FakeAccessChecker struct { - RegistriesToFail []string -} - -func NewFakeAccessChecker() *FakeAccessChecker { - return &FakeAccessChecker{} -} - -func (f *FakeAccessChecker) Check(repo string, publish bool) bool { - if !publish { - return true - } - for _, toFail := range f.RegistriesToFail { - if toFail == repo { - return false - } - } - - return true -} diff --git a/internal/fakes/fake_image_fetcher.go b/internal/fakes/fake_image_fetcher.go index 3ae30f9610..e95ce824bc 100644 --- a/internal/fakes/fake_image_fetcher.go +++ b/internal/fakes/fake_image_fetcher.go @@ -55,6 +55,12 @@ func (f *FakeImageFetcher) Fetch(ctx context.Context, name string, options image return ri, nil } +func (f *FakeImageFetcher) CheckReadAccessValidator(_ image.FetchOptions) image.CheckReadAccess { + return func(_ string) bool { + return true + } +} + func shouldPull(localFound, remoteFound bool, policy image.PullPolicy) bool { if remoteFound && !localFound && policy == image.PullIfNotPresent { return true diff --git a/pkg/buildpack/downloader.go b/pkg/buildpack/downloader.go index d3a5eaea2c..45930342ed 100644 --- a/pkg/buildpack/downloader.go +++ b/pkg/buildpack/downloader.go @@ -29,6 +29,7 @@ type Logger interface { type ImageFetcher interface { Fetch(ctx context.Context, name string, options image.FetchOptions) (imgutil.Image, error) + CheckReadAccessValidator(options image.FetchOptions) image.CheckReadAccess } type Downloader interface { diff --git a/pkg/client/build.go b/pkg/client/build.go index 670f53f2c8..da58f580fe 100644 --- a/pkg/client/build.go +++ b/pkg/client/build.go @@ -340,7 +340,7 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error { return errors.Wrapf(err, "invalid builder %s", style.Symbol(opts.Builder)) } - runImageName := c.resolveRunImage(opts.RunImage, imgRegistry, builderRef.Context().RegistryStr(), bldr.DefaultRunImage(), opts.AdditionalMirrors, opts.Publish, c.accessChecker) + runImageName := c.resolveRunImage(opts.RunImage, imgRegistry, builderRef.Context().RegistryStr(), bldr.DefaultRunImage(), opts.AdditionalMirrors, opts.Publish) fetchOptions := image.FetchOptions{ Daemon: !opts.Publish, diff --git a/pkg/client/build_test.go b/pkg/client/build_test.go index 27cd02d236..b01cb9b2be 100644 --- a/pkg/client/build_test.go +++ b/pkg/client/build_test.go @@ -56,7 +56,6 @@ func testBuild(t *testing.T, when spec.G, it spec.S) { subject *Client fakeImageFetcher *ifakes.FakeImageFetcher fakeLifecycle *ifakes.FakeLifecycle - fakeAccessChecker *ifakes.FakeAccessChecker defaultBuilderStackID = "some.stack.id" defaultWindowsBuilderStackID = "some.windows.stack.id" defaultBuilderImage *fakes.Image @@ -81,7 +80,6 @@ func testBuild(t *testing.T, when spec.G, it spec.S) { var err error fakeImageFetcher = ifakes.NewFakeImageFetcher() - fakeAccessChecker = ifakes.NewFakeAccessChecker() fakeLifecycle = &ifakes.FakeLifecycle{} tmpDir, err = os.MkdirTemp("", "build-test") @@ -138,7 +136,6 @@ func testBuild(t *testing.T, when spec.G, it spec.S) { logger: logger, imageFetcher: fakeImageFetcher, downloader: blobDownloader, - accessChecker: fakeAccessChecker, lifecycleExecutor: fakeLifecycle, docker: docker, buildpackDownloader: buildpackDownloader, diff --git a/pkg/client/client.go b/pkg/client/client.go index d46ccb2d2b..97fa306fa3 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -52,6 +52,8 @@ type ImageFetcher interface { // PullIfNotPresent and daemon = false, gives us the same behavior as PullAlways. // 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(options image.FetchOptions) image.CheckReadAccess } //go:generate mockgen -package testmocks -destination ../testmocks/mock_blob_downloader.go github.com/buildpacks/pack/pkg/client BlobDownloader @@ -80,13 +82,6 @@ type BuildpackDownloader interface { Download(ctx context.Context, buildpackURI string, opts buildpack.DownloadOptions) (buildpack.BuildModule, []buildpack.BuildModule, error) } -//go:generate mockgen -package testmocks -destination ../testmocks/mock_access_checker.go github.com/buildpacks/pack/pkg/client AccessChecker - -// AccessChecker is an interface for checking remote images for read access -type AccessChecker interface { - Check(repo string, publish bool) bool -} - // Client is an orchestration object, it contains all parameters needed to // build an app image using Cloud Native Buildpacks. // All settings on this object should be changed through ClientOption functions. @@ -97,7 +92,6 @@ type Client struct { keychain authn.Keychain imageFactory ImageFactory imageFetcher ImageFetcher - accessChecker AccessChecker downloader BlobDownloader lifecycleExecutor LifecycleExecutor buildpackDownloader BuildpackDownloader @@ -133,14 +127,6 @@ func WithFetcher(f ImageFetcher) Option { } } -// WithAccessChecker supply your own AccessChecker. -// A AccessChecker returns true if an image is accessible for reading. -func WithAccessChecker(f AccessChecker) Option { - return func(c *Client) { - c.accessChecker = f - } -} - // WithDownloader supply your own downloader. // A Downloader is used to gather buildpacks from both remote urls, or local sources. func WithDownloader(d BlobDownloader) Option { @@ -241,10 +227,6 @@ func NewClient(opts ...Option) (*Client, error) { } } - if client.accessChecker == nil { - client.accessChecker = image.NewAccessChecker(client.logger, client.keychain) - } - if client.buildpackDownloader == nil { client.buildpackDownloader = buildpack.NewDownloader( client.logger, diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 2ee6d8b2fe..0b40ec0a8f 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -10,7 +10,6 @@ import ( "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" @@ -123,13 +122,4 @@ func testClient(t *testing.T, when spec.G, it spec.S) { h.AssertEq(t, cl.registryMirrors, registryMirrors) }) }) - - when("#WithAccessChecker", func() { - it("uses AccessChecker provided", func() { - ac := &image.Checker{} - cl, err := NewClient(WithAccessChecker(ac)) - h.AssertNil(t, err) - h.AssertSameInstance(t, cl.accessChecker, ac) - }) - }) } diff --git a/pkg/client/common.go b/pkg/client/common.go index ff824c5d3e..d23cb2e957 100644 --- a/pkg/client/common.go +++ b/pkg/client/common.go @@ -10,6 +10,7 @@ import ( "github.com/buildpacks/pack/internal/config" "github.com/buildpacks/pack/internal/registry" "github.com/buildpacks/pack/internal/style" + "github.com/buildpacks/pack/pkg/image" "github.com/buildpacks/pack/pkg/logging" ) @@ -28,7 +29,7 @@ func (c *Client) parseTagReference(imageName string) (name.Reference, error) { return ref, nil } -func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, runImageMetadata builder.RunImageMetadata, additionalMirrors map[string][]string, publish bool, accessChecker AccessChecker) string { +func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, runImageMetadata builder.RunImageMetadata, additionalMirrors map[string][]string, publish bool) string { if runImage != "" { c.logger.Debugf("Using provided run-image %s", style.Symbol(runImage)) return runImage @@ -44,8 +45,9 @@ func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, run runImageMetadata.Image, runImageMetadata.Mirrors, additionalMirrors[runImageMetadata.Image], - publish, - accessChecker, + c.imageFetcher.CheckReadAccessValidator(image.FetchOptions{ + Daemon: !publish, + }), ) switch { @@ -109,8 +111,8 @@ func contains(slc []string, v string) bool { return false } -func getBestRunMirror(registry string, runImage string, mirrors []string, preferredMirrors []string, publish bool, accessChecker AccessChecker) string { - runImageList := filterImageList(append(append(append([]string{}, preferredMirrors...), runImage), mirrors...), publish, accessChecker) +func getBestRunMirror(registry string, runImage string, mirrors []string, preferredMirrors []string, accessChecker image.CheckReadAccess) string { + runImageList := filterImageList(append(append(append([]string{}, preferredMirrors...), runImage), mirrors...), accessChecker) for _, img := range runImageList { ref, err := name.ParseReference(img, name.WeakValidation) if err != nil { @@ -128,11 +130,11 @@ func getBestRunMirror(registry string, runImage string, mirrors []string, prefer return runImage } -func filterImageList(imageList []string, publish bool, accessChecker AccessChecker) []string { +func filterImageList(imageList []string, accessChecker image.CheckReadAccess) []string { var accessibleImages []string for i, img := range imageList { - if accessChecker.Check(img, publish) { + if accessChecker(img) { accessibleImages = append(accessibleImages, imageList[i]) } } diff --git a/pkg/client/common_test.go b/pkg/client/common_test.go index e610020760..09719f6199 100644 --- a/pkg/client/common_test.go +++ b/pkg/client/common_test.go @@ -4,13 +4,17 @@ import ( "bytes" "testing" + "github.com/buildpacks/lifecycle/auth" + "github.com/golang/mock/gomock" + "github.com/google/go-containerregistry/pkg/authn" "github.com/heroku/color" "github.com/sclevine/spec" "github.com/sclevine/spec/report" "github.com/buildpacks/pack/internal/builder" - ifakes "github.com/buildpacks/pack/internal/fakes" + "github.com/buildpacks/pack/pkg/image" "github.com/buildpacks/pack/pkg/logging" + "github.com/buildpacks/pack/pkg/testmocks" h "github.com/buildpacks/pack/testhelpers" ) @@ -26,21 +30,25 @@ func testCommon(t *testing.T, when spec.G, it spec.S) { subject *Client outBuf bytes.Buffer logger logging.Logger + keychain authn.Keychain runImageName string defaultRegistry string defaultMirror string gcrRegistry string gcrRunMirror string stackInfo builder.StackMetadata - accessChecker *ifakes.FakeAccessChecker assert = h.NewAssertionManager(t) + publish bool + err error ) it.Before(func() { logger = logging.NewLogWithWriters(&outBuf, &outBuf) - var err error - subject, err = NewClient(WithLogger(logger)) + keychain, err = auth.DefaultKeychain("pack-test/dummy") + h.AssertNil(t, err) + + subject, err = NewClient(WithLogger(logger), WithKeychain(keychain)) assert.Nil(err) defaultRegistry = "default.registry.io" @@ -56,46 +64,83 @@ func testCommon(t *testing.T, when spec.G, it spec.S) { }, }, } - accessChecker = ifakes.NewFakeAccessChecker() }) when("passed specific run image", func() { + it.Before(func() { + publish = false + }) + it("selects that run image", func() { runImgFlag := "flag/passed-run-image" - runImageName := subject.resolveRunImage(runImgFlag, defaultRegistry, "", stackInfo.RunImage, nil, false, accessChecker) + runImageName = subject.resolveRunImage(runImgFlag, defaultRegistry, "", stackInfo.RunImage, nil, publish) assert.Equal(runImageName, runImgFlag) }) }) when("publish is true", func() { - it("defaults to run-image in registry publishing to", func() { - runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, true, accessChecker) - assert.Equal(runImageName, gcrRunMirror) - }) + when("desirable run-image are accessible", func() { + it.Before(func() { + publish = true + mockFetcher := fetcherWithCheckReadAccess(t, publish, func(repo string) bool { + return true + }) + subject, err = NewClient(WithLogger(logger), WithKeychain(keychain), WithFetcher(mockFetcher)) + h.AssertNil(t, err) + }) - it("prefers config defined run image mirror to stack defined run image mirror", func() { - configMirrors := map[string][]string{ - runImageName: {defaultRegistry + "/unique-run-img"}, - } - runImageName := subject.resolveRunImage("", defaultRegistry, "", stackInfo.RunImage, configMirrors, true, accessChecker) - assert.NotEqual(runImageName, defaultMirror) - assert.Equal(runImageName, defaultRegistry+"/unique-run-img") + it("defaults to run-image in registry publishing to", func() { + runImageName = subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, publish) + assert.Equal(runImageName, gcrRunMirror) + }) + + it("prefers config defined run image mirror to stack defined run image mirror", func() { + configMirrors := map[string][]string{ + runImageName: {defaultRegistry + "/unique-run-img"}, + } + runImageName = subject.resolveRunImage("", defaultRegistry, "", stackInfo.RunImage, configMirrors, publish) + assert.NotEqual(runImageName, defaultMirror) + assert.Equal(runImageName, defaultRegistry+"/unique-run-img") + }) + + it("returns a config mirror if no match to target registry", func() { + configMirrors := map[string][]string{ + runImageName: {defaultRegistry + "/unique-run-img"}, + } + runImageName = subject.resolveRunImage("", "test.registry.io", "", stackInfo.RunImage, configMirrors, publish) + assert.NotEqual(runImageName, defaultMirror) + assert.Equal(runImageName, defaultRegistry+"/unique-run-img") + }) }) - it("returns a config mirror if no match to target registry", func() { - configMirrors := map[string][]string{ - runImageName: {defaultRegistry + "/unique-run-img"}, - } - runImageName := subject.resolveRunImage("", "test.registry.io", "", stackInfo.RunImage, configMirrors, true, accessChecker) - assert.NotEqual(runImageName, defaultMirror) - assert.Equal(runImageName, defaultRegistry+"/unique-run-img") + when("desirable run-images are not accessible", func() { + it.Before(func() { + publish = true + mockFetcher := fetcherWithCheckReadAccess(t, publish, func(repo string) bool { + if repo == gcrRunMirror || repo == stackInfo.RunImage.Image { + return false + } + return true + }) + subject, err = NewClient(WithLogger(logger), WithKeychain(keychain), WithFetcher(mockFetcher)) + h.AssertNil(t, err) + }) + + it("selects the first accessible run-image", func() { + runImageName = subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, publish) + assert.Equal(runImageName, defaultMirror) + }) }) }) // If publish is false, we are using the local daemon, and want to match to the builder registry when("publish is false", func() { + it.Before(func() { + publish = false + }) + it("defaults to run-image in registry publishing to", func() { - runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, false, accessChecker) + runImageName = subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, publish) assert.Equal(runImageName, defaultMirror) assert.NotEqual(runImageName, gcrRunMirror) }) @@ -104,7 +149,7 @@ func testCommon(t *testing.T, when spec.G, it spec.S) { configMirrors := map[string][]string{ runImageName: {defaultRegistry + "/unique-run-img"}, } - runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, configMirrors, false, accessChecker) + runImageName = subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, configMirrors, publish) assert.NotEqual(runImageName, defaultMirror) assert.Equal(runImageName, defaultRegistry+"/unique-run-img") }) @@ -113,49 +158,33 @@ func testCommon(t *testing.T, when spec.G, it spec.S) { configMirrors := map[string][]string{ runImageName: {defaultRegistry + "/unique-run-img"}, } - runImageName := subject.resolveRunImage("", defaultRegistry, "test.registry.io", stackInfo.RunImage, configMirrors, false, accessChecker) + runImageName = subject.resolveRunImage("", defaultRegistry, "test.registry.io", stackInfo.RunImage, configMirrors, publish) assert.NotEqual(runImageName, defaultMirror) assert.Equal(runImageName, defaultRegistry+"/unique-run-img") }) - }) - when("desirable run-image is not accessible", func() { - it.Before(func() { - accessChecker.RegistriesToFail = []string{ - gcrRunMirror, - stackInfo.RunImage.Image, - } - }) - - it.After(func() { - accessChecker.RegistriesToFail = nil - }) - - when("publish is true", func() { - it("selects the first accessible run-image", func() { - runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, true, accessChecker) - assert.Equal(runImageName, defaultMirror) - }) - }) - - when("publish is false", func() { + when("desirable run-image are empty", func() { it.Before(func() { stackInfo = builder.StackMetadata{ RunImage: builder.RunImageMetadata{ Image: "stack/run-image", }, } - accessChecker.RegistriesToFail = []string{ - stackInfo.RunImage.Image, - } }) - it("selects the given run-image", func() { + it("selects the builder run-image", func() { // issue: https://github.com/buildpacks/pack/issues/2078 - runImageName := subject.resolveRunImage("", "", "", stackInfo.RunImage, nil, false, accessChecker) + runImageName = subject.resolveRunImage("", "", "", stackInfo.RunImage, nil, publish) assert.Equal(runImageName, "stack/run-image") }) }) }) }) } + +func fetcherWithCheckReadAccess(t *testing.T, publish bool, checker image.CheckReadAccess) *testmocks.MockImageFetcher { + mockController := gomock.NewController(t) + mockFetcher := testmocks.NewMockImageFetcher(mockController) + mockFetcher.EXPECT().CheckReadAccessValidator(image.FetchOptions{Daemon: !publish}).Return(checker) + return mockFetcher +} diff --git a/pkg/client/example_fetcher_test.go b/pkg/client/example_fetcher_test.go index e8cf56ce65..246302f596 100644 --- a/pkg/client/example_fetcher_test.go +++ b/pkg/client/example_fetcher_test.go @@ -53,3 +53,9 @@ func (f *fetcher) Fetch(_ context.Context, imageName string, _ image.FetchOption fmt.Println("custom fetcher called") return nil, errors.New("not implemented") } + +func (f *fetcher) CheckReadAccessValidator(options image.FetchOptions) image.CheckReadAccess { + return func(_ string) bool { + return true + } +} diff --git a/pkg/client/rebase.go b/pkg/client/rebase.go index f493f6184a..c5fb7d946b 100644 --- a/pkg/client/rebase.go +++ b/pkg/client/rebase.go @@ -105,7 +105,6 @@ func (c *Client) Rebase(ctx context.Context, opts RebaseOptions) error { runImageMD, opts.AdditionalMirrors, opts.Publish, - c.accessChecker, ) if runImageName == "" { diff --git a/pkg/client/rebase_test.go b/pkg/client/rebase_test.go index ba6a9df86b..7ceff08df2 100644 --- a/pkg/client/rebase_test.go +++ b/pkg/client/rebase_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/buildpacks/imgutil/fakes" + "github.com/buildpacks/lifecycle/auth" "github.com/heroku/color" "github.com/sclevine/spec" "github.com/sclevine/spec/report" @@ -28,7 +29,6 @@ func testRebase(t *testing.T, when spec.G, it spec.S) { when("#Rebase", func() { var ( fakeImageFetcher *ifakes.FakeImageFetcher - fakeAccessChecker *ifakes.FakeAccessChecker subject *Client fakeAppImage *fakes.Image fakeRunImage *fakes.Image @@ -38,7 +38,6 @@ func testRebase(t *testing.T, when spec.G, it spec.S) { it.Before(func() { fakeImageFetcher = ifakes.NewFakeImageFetcher() - fakeAccessChecker = ifakes.NewFakeAccessChecker() fakeAppImage = fakes.NewImage("some/app", "", &fakeIdentifier{name: "app-image"}) h.AssertNil(t, fakeAppImage.SetLabel("io.buildpacks.lifecycle.metadata", @@ -54,11 +53,14 @@ func testRebase(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, fakeRunImageMirror.SetLabel("io.buildpacks.stack.id", "io.buildpacks.stacks.jammy")) fakeImageFetcher.LocalImages["example.com/some/run"] = fakeRunImageMirror + keychain, err := auth.DefaultKeychain("pack-test/dummy") + h.AssertNil(t, err) + fakeLogger := logging.NewLogWithWriters(&out, &out) subject = &Client{ - logger: fakeLogger, - imageFetcher: fakeImageFetcher, - accessChecker: fakeAccessChecker, + logger: fakeLogger, + imageFetcher: fakeImageFetcher, + keychain: keychain, } }) diff --git a/pkg/image/access_checker.go b/pkg/image/access_checker.go deleted file mode 100644 index 50401a104f..0000000000 --- a/pkg/image/access_checker.go +++ /dev/null @@ -1,46 +0,0 @@ -package image - -import ( - "github.com/buildpacks/imgutil/remote" - "github.com/google/go-containerregistry/pkg/authn" - - "github.com/buildpacks/pack/pkg/logging" -) - -type Checker struct { - logger logging.Logger - keychain authn.Keychain -} - -func NewAccessChecker(logger logging.Logger, keychain authn.Keychain) *Checker { - checker := &Checker{ - logger: logger, - keychain: keychain, - } - - if checker.keychain == nil { - checker.keychain = authn.DefaultKeychain - } - - return checker -} - -func (c *Checker) Check(repo string, publish bool) bool { - if !publish { - // nop checker, we are running against the daemon - return true - } - - img, err := remote.NewImage(repo, c.keychain) - if err != nil { - return false - } - - if ok, err := img.CheckReadAccess(); ok { - c.logger.Debugf("CheckReadAccess succeeded for the run image %s", repo) - return true - } else { - c.logger.Debugf("CheckReadAccess failed for the run image %s, error: %s", repo, err.Error()) - return false - } -} diff --git a/pkg/image/access_checker_test.go b/pkg/image/access_checker_test.go deleted file mode 100644 index d16de8b22a..0000000000 --- a/pkg/image/access_checker_test.go +++ /dev/null @@ -1,57 +0,0 @@ -package image_test - -import ( - "bytes" - "testing" - - "github.com/buildpacks/lifecycle/auth" - "github.com/sclevine/spec" - "github.com/sclevine/spec/report" - - "github.com/buildpacks/pack/pkg/image" - "github.com/buildpacks/pack/pkg/logging" - h "github.com/buildpacks/pack/testhelpers" -) - -func TestChecker(t *testing.T) { - spec.Run(t, "Checker", testChecker, spec.Report(report.Terminal{})) -} - -func testChecker(t *testing.T, when spec.G, it spec.S) { - var publish bool - - when("#Check", func() { - when("publish is false", func() { - it.Before(func() { - publish = false - }) - - // issue: https://github.com/buildpacks/pack/issues/2078 - it("returns true", func() { - buf := &bytes.Buffer{} - keychain, err := auth.DefaultKeychain("pack-test/dummy") - h.AssertNil(t, err) - - ic := image.NewAccessChecker(logging.NewSimpleLogger(buf), keychain) - h.AssertTrue(t, ic.Check("pack.test/dummy", publish)) - }) - }) - - when("publish is true", func() { - it.Before(func() { - publish = true - }) - - it("fails when checking dummy image", func() { - buf := &bytes.Buffer{} - keychain, err := auth.DefaultKeychain("pack.test/dummy") - h.AssertNil(t, err) - - ic := image.NewAccessChecker(logging.NewSimpleLogger(buf), keychain) - - h.AssertFalse(t, ic.Check("pack.test/dummy", publish)) - h.AssertContains(t, buf.String(), "DEBUG: CheckReadAccess failed for the run image pack.test/dummy") - }) - }) - }) -} diff --git a/pkg/image/fetcher.go b/pkg/image/fetcher.go index 60548fcc1d..e9958653c6 100644 --- a/pkg/image/fetcher.go +++ b/pkg/image/fetcher.go @@ -53,6 +53,9 @@ type DockerClient interface { ImagePull(ctx context.Context, ref string, options types.ImagePullOptions) (io.ReadCloser, error) } +// CheckReadAccess is a method for checking remote images for read access +type CheckReadAccess func(repo string) bool + type Fetcher struct { docker DockerClient logger logging.Logger @@ -123,6 +126,27 @@ func (f *Fetcher) Fetch(ctx context.Context, name string, options FetchOptions) return f.fetchDaemonImage(name) } +func (f *Fetcher) CheckReadAccessValidator(options FetchOptions) CheckReadAccess { + return func(repo string) bool { + if !options.Daemon && (options.LayoutOption == LayoutOption{}) { + // remote access checker + img, err := remote.NewImage(repo, f.keychain) + if err != nil { + return false + } + if ok, err := img.CheckReadAccess(); ok { + f.logger.Debugf("CheckReadAccess succeeded for the run image %s", repo) + return true + } else { + f.logger.Debugf("CheckReadAccess failed for the run image %s, error: %s", repo, err.Error()) + return false + } + } + // no-op + return true + } +} + func (f *Fetcher) fetchDaemonImage(name string) (imgutil.Image, error) { image, err := local.NewImage(name, f.docker, local.FromBaseImage(name)) if err != nil { diff --git a/pkg/image/fetcher_test.go b/pkg/image/fetcher_test.go index 3b119e15f3..e12c859f92 100644 --- a/pkg/image/fetcher_test.go +++ b/pkg/image/fetcher_test.go @@ -10,7 +10,6 @@ import ( "testing" "github.com/buildpacks/imgutil" - "github.com/buildpacks/imgutil/local" "github.com/buildpacks/imgutil/remote" "github.com/docker/docker/client" @@ -42,7 +41,7 @@ func TestFetcher(t *testing.T) { var err error docker, err = client.NewClientWithOpts(client.FromEnv, client.WithVersion("1.38")) h.AssertNil(t, err) - spec.Run(t, "Fetcher", testFetcher, spec.Report(report.Terminal{})) + spec.Run(t, "Fetcher", testFetcher, spec.Parallel(), spec.Report(report.Terminal{})) } func testFetcher(t *testing.T, when spec.G, it spec.S) { @@ -57,7 +56,7 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) { it.Before(func() { repo = "some-org/" + h.RandString(10) repoName = registryConfig.RepoName(repo) - imageFetcher = image.NewFetcher(logging.NewLogWithWriters(&outBuf, &outBuf), docker) + imageFetcher = image.NewFetcher(logging.NewLogWithWriters(&outBuf, &outBuf, logging.WithVerbose()), docker) info, err := docker.Info(context.TODO()) h.AssertNil(t, err) @@ -404,4 +403,47 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) { }) }) }) + + when("#CheckReadAccessValidator", func() { + var daemon bool + + when("Daemon is true", func() { + it.Before(func() { + daemon = true + }) + + it("read access is always valid", func() { + accessChecker := imageFetcher.CheckReadAccessValidator(image.FetchOptions{Daemon: daemon}) + h.AssertTrue(t, accessChecker("pack.test/dummy")) + }) + }) + + when("Daemon is false", func() { + it.Before(func() { + daemon = false + }) + + when("remote image doesn't exists", func() { + it("fails when checking dummy image", func() { + accessChecker := imageFetcher.CheckReadAccessValidator(image.FetchOptions{Daemon: daemon}) + h.AssertFalse(t, accessChecker("pack.test/dummy")) + h.AssertContains(t, outBuf.String(), "CheckReadAccess failed for the run image pack.test/dummy") + }) + }) + + when("remote image exists", func() { + it.Before(func() { + img, err := remote.NewImage(repoName, authn.DefaultKeychain) + h.AssertNil(t, err) + h.AssertNil(t, img.Save()) + }) + + it("read access is valid", func() { + accessChecker := imageFetcher.CheckReadAccessValidator(image.FetchOptions{Daemon: daemon}) + h.AssertTrue(t, accessChecker(repoName)) + 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 281f28d04d..11177db224 100644 --- a/pkg/testmocks/mock_image_fetcher.go +++ b/pkg/testmocks/mock_image_fetcher.go @@ -37,6 +37,20 @@ func (m *MockImageFetcher) EXPECT() *MockImageFetcherMockRecorder { return m.recorder } +// CheckReadAccessValidator mocks base method. +func (m *MockImageFetcher) CheckReadAccessValidator(arg0 image.FetchOptions) image.CheckReadAccess { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CheckReadAccessValidator", arg0) + ret0, _ := ret[0].(image.CheckReadAccess) + return ret0 +} + +// CheckReadAccessValidator indicates an expected call of CheckReadAccessValidator. +func (mr *MockImageFetcherMockRecorder) CheckReadAccessValidator(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckReadAccessValidator", reflect.TypeOf((*MockImageFetcher)(nil).CheckReadAccessValidator), arg0) +} + // Fetch mocks base method. func (m *MockImageFetcher) Fetch(arg0 context.Context, arg1 string, arg2 image.FetchOptions) (imgutil.Image, error) { m.ctrl.T.Helper() From 56bcbce94cc180a4b2a867151b362b1dadc15697 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Mon, 8 Apr 2024 14:54:07 -0500 Subject: [PATCH 3/6] Adding Pull Policy into the logic to detect read access Signed-off-by: Juan Bustamante --- internal/builder/image_fetcher_wrapper.go | 13 ++- internal/fakes/fake_image_fetcher.go | 6 +- pkg/buildpack/downloader.go | 2 +- pkg/client/build.go | 4 +- pkg/client/client.go | 8 +- pkg/client/common.go | 15 ++- pkg/client/common_test.go | 129 ++++++++-------------- pkg/client/example_fetcher_test.go | 6 +- pkg/client/rebase.go | 14 ++- pkg/image/fetcher.go | 45 ++++---- pkg/image/fetcher_test.go | 57 ++++++++-- pkg/testmocks/mock_image_fetcher.go | 10 +- 12 files changed, 170 insertions(+), 139 deletions(-) diff --git a/internal/builder/image_fetcher_wrapper.go b/internal/builder/image_fetcher_wrapper.go index 613a9647d4..61ec4d40b5 100644 --- a/internal/builder/image_fetcher_wrapper.go +++ b/internal/builder/image_fetcher_wrapper.go @@ -13,7 +13,14 @@ type ImageFetcher interface { // If daemon is true, it will look return a `local.Image`. Pull, applicable only when daemon is true, will // attempt to pull a remote image first. Fetch(ctx context.Context, name string, options image.FetchOptions) (imgutil.Image, error) - CheckReadAccessValidator(options image.FetchOptions) image.CheckReadAccess + + // CheckReadAccessValidator 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 } type ImageFetcherWrapper struct { @@ -34,6 +41,6 @@ func (w *ImageFetcherWrapper) Fetch( return w.fetcher.Fetch(ctx, name, options) } -func (w *ImageFetcherWrapper) CheckReadAccessValidator(options image.FetchOptions) image.CheckReadAccess { - return w.fetcher.CheckReadAccessValidator(options) +func (w *ImageFetcherWrapper) CheckReadAccessValidator(repo string, options image.FetchOptions) bool { + return w.fetcher.CheckReadAccessValidator(repo, options) } diff --git a/internal/fakes/fake_image_fetcher.go b/internal/fakes/fake_image_fetcher.go index e95ce824bc..e2c21722bf 100644 --- a/internal/fakes/fake_image_fetcher.go +++ b/internal/fakes/fake_image_fetcher.go @@ -55,10 +55,8 @@ func (f *FakeImageFetcher) Fetch(ctx context.Context, name string, options image return ri, nil } -func (f *FakeImageFetcher) CheckReadAccessValidator(_ image.FetchOptions) image.CheckReadAccess { - return func(_ string) bool { - return true - } +func (f *FakeImageFetcher) CheckReadAccessValidator(_ string, _ image.FetchOptions) bool { + return true } func shouldPull(localFound, remoteFound bool, policy image.PullPolicy) bool { diff --git a/pkg/buildpack/downloader.go b/pkg/buildpack/downloader.go index 45930342ed..91cd597815 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(options image.FetchOptions) image.CheckReadAccess + CheckReadAccessValidator(repo string, options image.FetchOptions) bool } type Downloader interface { diff --git a/pkg/client/build.go b/pkg/client/build.go index da58f580fe..43c82e0196 100644 --- a/pkg/client/build.go +++ b/pkg/client/build.go @@ -340,13 +340,13 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error { return errors.Wrapf(err, "invalid builder %s", style.Symbol(opts.Builder)) } - runImageName := c.resolveRunImage(opts.RunImage, imgRegistry, builderRef.Context().RegistryStr(), bldr.DefaultRunImage(), opts.AdditionalMirrors, opts.Publish) - fetchOptions := image.FetchOptions{ Daemon: !opts.Publish, PullPolicy: opts.PullPolicy, Platform: fmt.Sprintf("%s/%s", builderOS, builderArch), } + runImageName := c.resolveRunImage(opts.RunImage, imgRegistry, builderRef.Context().RegistryStr(), bldr.DefaultRunImage(), opts.AdditionalMirrors, opts.Publish, fetchOptions) + if opts.Layout() { targetRunImagePath, err := layout.ParseRefToPath(runImageName) if err != nil { diff --git a/pkg/client/client.go b/pkg/client/client.go index 97fa306fa3..8d0272d2ef 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -53,7 +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(options image.FetchOptions) image.CheckReadAccess + // CheckReadAccessValidator 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 } //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 d23cb2e957..8ca77edd0e 100644 --- a/pkg/client/common.go +++ b/pkg/client/common.go @@ -29,7 +29,7 @@ func (c *Client) parseTagReference(imageName string) (name.Reference, error) { return ref, nil } -func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, runImageMetadata builder.RunImageMetadata, additionalMirrors map[string][]string, publish bool) string { +func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, runImageMetadata builder.RunImageMetadata, additionalMirrors map[string][]string, publish bool, options image.FetchOptions) string { if runImage != "" { c.logger.Debugf("Using provided run-image %s", style.Symbol(runImage)) return runImage @@ -45,9 +45,8 @@ func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, run runImageMetadata.Image, runImageMetadata.Mirrors, additionalMirrors[runImageMetadata.Image], - c.imageFetcher.CheckReadAccessValidator(image.FetchOptions{ - Daemon: !publish, - }), + c.imageFetcher, + options, ) switch { @@ -111,8 +110,8 @@ func contains(slc []string, v string) bool { return false } -func getBestRunMirror(registry string, runImage string, mirrors []string, preferredMirrors []string, accessChecker image.CheckReadAccess) string { - runImageList := filterImageList(append(append(append([]string{}, preferredMirrors...), runImage), mirrors...), accessChecker) +func getBestRunMirror(registry string, runImage string, mirrors []string, preferredMirrors []string, fetcher ImageFetcher, options image.FetchOptions) string { + runImageList := filterImageList(append(append(append([]string{}, preferredMirrors...), runImage), mirrors...), fetcher, options) for _, img := range runImageList { ref, err := name.ParseReference(img, name.WeakValidation) if err != nil { @@ -130,11 +129,11 @@ func getBestRunMirror(registry string, runImage string, mirrors []string, prefer return runImage } -func filterImageList(imageList []string, accessChecker image.CheckReadAccess) []string { +func filterImageList(imageList []string, fetcher ImageFetcher, options image.FetchOptions) []string { var accessibleImages []string for i, img := range imageList { - if accessChecker(img) { + if fetcher.CheckReadAccessValidator(img, options) { accessibleImages = append(accessibleImages, imageList[i]) } } diff --git a/pkg/client/common_test.go b/pkg/client/common_test.go index 09719f6199..baa4f0b645 100644 --- a/pkg/client/common_test.go +++ b/pkg/client/common_test.go @@ -73,83 +73,31 @@ func testCommon(t *testing.T, when spec.G, it spec.S) { it("selects that run image", func() { runImgFlag := "flag/passed-run-image" - runImageName = subject.resolveRunImage(runImgFlag, defaultRegistry, "", stackInfo.RunImage, nil, publish) + runImageName = subject.resolveRunImage(runImgFlag, defaultRegistry, "", stackInfo.RunImage, nil, publish, image.FetchOptions{Daemon: !publish, PullPolicy: image.PullAlways}) assert.Equal(runImageName, runImgFlag) }) }) - when("publish is true", func() { - when("desirable run-image are accessible", func() { - it.Before(func() { - publish = true - mockFetcher := fetcherWithCheckReadAccess(t, publish, func(repo string) bool { - return true - }) - subject, err = NewClient(WithLogger(logger), WithKeychain(keychain), WithFetcher(mockFetcher)) - h.AssertNil(t, err) - }) - - it("defaults to run-image in registry publishing to", func() { - runImageName = subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, publish) - assert.Equal(runImageName, gcrRunMirror) - }) - - it("prefers config defined run image mirror to stack defined run image mirror", func() { - configMirrors := map[string][]string{ - runImageName: {defaultRegistry + "/unique-run-img"}, - } - runImageName = subject.resolveRunImage("", defaultRegistry, "", stackInfo.RunImage, configMirrors, publish) - assert.NotEqual(runImageName, defaultMirror) - assert.Equal(runImageName, defaultRegistry+"/unique-run-img") - }) - - it("returns a config mirror if no match to target registry", func() { - configMirrors := map[string][]string{ - runImageName: {defaultRegistry + "/unique-run-img"}, - } - runImageName = subject.resolveRunImage("", "test.registry.io", "", stackInfo.RunImage, configMirrors, publish) - assert.NotEqual(runImageName, defaultMirror) - assert.Equal(runImageName, defaultRegistry+"/unique-run-img") - }) - }) - - when("desirable run-images are not accessible", func() { - it.Before(func() { - publish = true - mockFetcher := fetcherWithCheckReadAccess(t, publish, func(repo string) bool { - if repo == gcrRunMirror || repo == stackInfo.RunImage.Image { - return false - } - return true - }) - subject, err = NewClient(WithLogger(logger), WithKeychain(keychain), WithFetcher(mockFetcher)) - h.AssertNil(t, err) - }) - - it("selects the first accessible run-image", func() { - runImageName = subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, publish) - assert.Equal(runImageName, defaultMirror) - }) - }) - }) - - // If publish is false, we are using the local daemon, and want to match to the builder registry - when("publish is false", func() { + when("desirable run-image are accessible", func() { it.Before(func() { - publish = false + publish = true + mockController := gomock.NewController(t) + mockFetcher := testmocks.NewMockImageFetcher(mockController) + mockFetcher.EXPECT().CheckReadAccessValidator(gomock.Any(), gomock.Any()).Return(true).AnyTimes() + subject, err = NewClient(WithLogger(logger), WithKeychain(keychain), WithFetcher(mockFetcher)) + h.AssertNil(t, err) }) it("defaults to run-image in registry publishing to", func() { - runImageName = subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, publish) - assert.Equal(runImageName, defaultMirror) - assert.NotEqual(runImageName, gcrRunMirror) + runImageName = subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, publish, image.FetchOptions{}) + assert.Equal(runImageName, gcrRunMirror) }) it("prefers config defined run image mirror to stack defined run image mirror", func() { configMirrors := map[string][]string{ runImageName: {defaultRegistry + "/unique-run-img"}, } - runImageName = subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, configMirrors, publish) + runImageName = subject.resolveRunImage("", defaultRegistry, "", stackInfo.RunImage, configMirrors, publish, image.FetchOptions{}) assert.NotEqual(runImageName, defaultMirror) assert.Equal(runImageName, defaultRegistry+"/unique-run-img") }) @@ -158,33 +106,54 @@ func testCommon(t *testing.T, when spec.G, it spec.S) { configMirrors := map[string][]string{ runImageName: {defaultRegistry + "/unique-run-img"}, } - runImageName = subject.resolveRunImage("", defaultRegistry, "test.registry.io", stackInfo.RunImage, configMirrors, publish) + runImageName = subject.resolveRunImage("", "test.registry.io", "", stackInfo.RunImage, configMirrors, publish, image.FetchOptions{}) assert.NotEqual(runImageName, defaultMirror) assert.Equal(runImageName, defaultRegistry+"/unique-run-img") }) + }) + + when("desirable run-images are not accessible", func() { + it.Before(func() { + publish = true + + mockController := gomock.NewController(t) + mockFetcher := testmocks.NewMockImageFetcher(mockController) + mockFetcher.EXPECT().CheckReadAccessValidator(gcrRunMirror, gomock.Any()).Return(false) + mockFetcher.EXPECT().CheckReadAccessValidator(stackInfo.RunImage.Image, gomock.Any()).Return(false) + mockFetcher.EXPECT().CheckReadAccessValidator(defaultMirror, gomock.Any()).Return(true) + + subject, err = NewClient(WithLogger(logger), WithKeychain(keychain), WithFetcher(mockFetcher)) + h.AssertNil(t, err) + }) + + it("selects the first accessible run-image", func() { + runImageName = subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, publish, image.FetchOptions{}) + assert.Equal(runImageName, defaultMirror) + }) + }) + + when("desirable run-image are empty", func() { + it.Before(func() { + publish = false + stackInfo = builder.StackMetadata{ + RunImage: builder.RunImageMetadata{ + Image: "stack/run-image", + }, + } + }) - when("desirable run-image are empty", func() { - it.Before(func() { - stackInfo = builder.StackMetadata{ - RunImage: builder.RunImageMetadata{ - Image: "stack/run-image", - }, - } - }) - - it("selects the builder run-image", func() { - // issue: https://github.com/buildpacks/pack/issues/2078 - runImageName = subject.resolveRunImage("", "", "", stackInfo.RunImage, nil, publish) - assert.Equal(runImageName, "stack/run-image") - }) + it("selects the builder run-image", func() { + // issue: https://github.com/buildpacks/pack/issues/2078 + runImageName = subject.resolveRunImage("", "", "", stackInfo.RunImage, nil, publish, image.FetchOptions{}) + assert.Equal(runImageName, "stack/run-image") }) }) }) } -func fetcherWithCheckReadAccess(t *testing.T, publish bool, checker image.CheckReadAccess) *testmocks.MockImageFetcher { +func fetcherWithCheckReadAccess(t *testing.T, publish bool, value bool) *testmocks.MockImageFetcher { mockController := gomock.NewController(t) mockFetcher := testmocks.NewMockImageFetcher(mockController) - mockFetcher.EXPECT().CheckReadAccessValidator(image.FetchOptions{Daemon: !publish}).Return(checker) + mockFetcher.EXPECT().CheckReadAccessValidator(gomock.Any(), image.FetchOptions{Daemon: !publish}).Return(value).AnyTimes() return mockFetcher } diff --git a/pkg/client/example_fetcher_test.go b/pkg/client/example_fetcher_test.go index 246302f596..12a1b65f10 100644 --- a/pkg/client/example_fetcher_test.go +++ b/pkg/client/example_fetcher_test.go @@ -54,8 +54,6 @@ func (f *fetcher) Fetch(_ context.Context, imageName string, _ image.FetchOption return nil, errors.New("not implemented") } -func (f *fetcher) CheckReadAccessValidator(options image.FetchOptions) image.CheckReadAccess { - return func(_ string) bool { - return true - } +func (f *fetcher) CheckReadAccessValidator(_ string, _ FetchOptions) bool { + return true } diff --git a/pkg/client/rebase.go b/pkg/client/rebase.go index c5fb7d946b..3d7e6532da 100644 --- a/pkg/client/rebase.go +++ b/pkg/client/rebase.go @@ -98,6 +98,13 @@ func (c *Client) Rebase(ctx context.Context, opts RebaseOptions) error { Mirrors: md.Stack.RunImage.Mirrors, } } + + fetchOptions := image.FetchOptions{ + Daemon: !opts.Publish, + PullPolicy: opts.PullPolicy, + Platform: fmt.Sprintf("%s/%s", appOS, appArch), + } + runImageName := c.resolveRunImage( opts.RunImage, imageRef.Context().RegistryStr(), @@ -105,17 +112,14 @@ func (c *Client) Rebase(ctx context.Context, opts RebaseOptions) error { runImageMD, opts.AdditionalMirrors, opts.Publish, + fetchOptions, ) if runImageName == "" { return errors.New("run image must be specified") } - baseImage, err := c.imageFetcher.Fetch(ctx, runImageName, image.FetchOptions{ - Daemon: !opts.Publish, - PullPolicy: opts.PullPolicy, - Platform: fmt.Sprintf("%s/%s", appOS, appArch), - }) + baseImage, err := c.imageFetcher.Fetch(ctx, runImageName, fetchOptions) if err != nil { return err } diff --git a/pkg/image/fetcher.go b/pkg/image/fetcher.go index e9958653c6..2e941e3ce1 100644 --- a/pkg/image/fetcher.go +++ b/pkg/image/fetcher.go @@ -53,9 +53,6 @@ type DockerClient interface { ImagePull(ctx context.Context, ref string, options types.ImagePullOptions) (io.ReadCloser, error) } -// CheckReadAccess is a method for checking remote images for read access -type CheckReadAccess func(repo string) bool - type Fetcher struct { docker DockerClient logger logging.Logger @@ -126,24 +123,34 @@ func (f *Fetcher) Fetch(ctx context.Context, name string, options FetchOptions) return f.fetchDaemonImage(name) } -func (f *Fetcher) CheckReadAccessValidator(options FetchOptions) CheckReadAccess { - return func(repo string) bool { - if !options.Daemon && (options.LayoutOption == LayoutOption{}) { - // remote access checker - img, err := remote.NewImage(repo, f.keychain) - if err != nil { - return false - } - if ok, err := img.CheckReadAccess(); ok { - f.logger.Debugf("CheckReadAccess succeeded for the run image %s", repo) - return true - } else { - f.logger.Debugf("CheckReadAccess failed for the run image %s, error: %s", repo, err.Error()) - return false - } +func (f *Fetcher) CheckReadAccessValidator(repo string, options FetchOptions) bool { + if !options.Daemon { + 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 } - // no-op + return f.checkRemoteReadAccess(repo) + } + // no-op + return true +} + +func (f *Fetcher) checkRemoteReadAccess(repo string) bool { + img, err := remote.NewImage(repo, f.keychain) + if err != nil { + return false + } + if ok, err := img.CheckReadAccess(); ok { + f.logger.Debugf("CheckReadAccess succeeded for the run image %s", repo) return true + } else { + f.logger.Debugf("CheckReadAccess failed for the run image %s, error: %s", repo, err.Error()) + return false } } diff --git a/pkg/image/fetcher_test.go b/pkg/image/fetcher_test.go index e12c859f92..f07598a489 100644 --- a/pkg/image/fetcher_test.go +++ b/pkg/image/fetcher_test.go @@ -412,9 +412,54 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) { daemon = true }) - it("read access is always valid", func() { - accessChecker := imageFetcher.CheckReadAccessValidator(image.FetchOptions{Daemon: daemon}) - h.AssertTrue(t, accessChecker("pack.test/dummy")) + 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})) + }) + }) + + 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})) + }) + }) + + 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})) + }) + }) + }) + + when("image doesn't exist in the daemon but in remote", func() { + it.Before(func() { + img, err := remote.NewImage(repoName, authn.DefaultKeychain) + h.AssertNil(t, err) + h.AssertNil(t, img.Save()) + }) + when("PullAlways", func() { + it("read access must be true", func() { + h.AssertTrue(t, imageFetcher.CheckReadAccessValidator(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})) + }) + }) + + when("PullIfNotPresent", func() { + it("read access must be true", func() { + h.AssertTrue(t, imageFetcher.CheckReadAccessValidator(repoName, image.FetchOptions{Daemon: daemon, PullPolicy: image.PullIfNotPresent})) + }) + }) }) }) @@ -425,8 +470,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() { - accessChecker := imageFetcher.CheckReadAccessValidator(image.FetchOptions{Daemon: daemon}) - h.AssertFalse(t, accessChecker("pack.test/dummy")) + h.AssertFalse(t, imageFetcher.CheckReadAccessValidator("pack.test/dummy", image.FetchOptions{Daemon: daemon})) h.AssertContains(t, outBuf.String(), "CheckReadAccess failed for the run image pack.test/dummy") }) }) @@ -439,8 +483,7 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) { }) it("read access is valid", func() { - accessChecker := imageFetcher.CheckReadAccessValidator(image.FetchOptions{Daemon: daemon}) - h.AssertTrue(t, accessChecker(repoName)) + h.AssertTrue(t, imageFetcher.CheckReadAccessValidator(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 11177db224..02bb715041 100644 --- a/pkg/testmocks/mock_image_fetcher.go +++ b/pkg/testmocks/mock_image_fetcher.go @@ -38,17 +38,17 @@ func (m *MockImageFetcher) EXPECT() *MockImageFetcherMockRecorder { } // CheckReadAccessValidator mocks base method. -func (m *MockImageFetcher) CheckReadAccessValidator(arg0 image.FetchOptions) image.CheckReadAccess { +func (m *MockImageFetcher) CheckReadAccessValidator(arg0 string, arg1 image.FetchOptions) bool { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CheckReadAccessValidator", arg0) - ret0, _ := ret[0].(image.CheckReadAccess) + ret := m.ctrl.Call(m, "CheckReadAccessValidator", arg0, arg1) + ret0, _ := ret[0].(bool) return ret0 } // CheckReadAccessValidator indicates an expected call of CheckReadAccessValidator. -func (mr *MockImageFetcherMockRecorder) CheckReadAccessValidator(arg0 interface{}) *gomock.Call { +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) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckReadAccessValidator", reflect.TypeOf((*MockImageFetcher)(nil).CheckReadAccessValidator), arg0, arg1) } // Fetch mocks base method. From bcc4a5fc2a933301cb6879d8f680dabd48017b52 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Tue, 9 Apr 2024 09:57:15 -0500 Subject: [PATCH 4/6] Removing unused method Signed-off-by: Juan Bustamante --- pkg/client/common_test.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/pkg/client/common_test.go b/pkg/client/common_test.go index baa4f0b645..9a14ed0c24 100644 --- a/pkg/client/common_test.go +++ b/pkg/client/common_test.go @@ -150,10 +150,3 @@ func testCommon(t *testing.T, when spec.G, it spec.S) { }) }) } - -func fetcherWithCheckReadAccess(t *testing.T, publish bool, value bool) *testmocks.MockImageFetcher { - mockController := gomock.NewController(t) - mockFetcher := testmocks.NewMockImageFetcher(mockController) - mockFetcher.EXPECT().CheckReadAccessValidator(gomock.Any(), image.FetchOptions{Daemon: !publish}).Return(value).AnyTimes() - return mockFetcher -} From a9bb1ca905c40ae1e71e5d0753b8a47991e08884 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Mon, 15 Apr 2024 15:02:49 -0500 Subject: [PATCH 5/6] 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/client/example_fetcher_test.go | 2 +- pkg/image/fetcher.go | 21 +++++++---- pkg/image/fetcher_test.go | 46 ++++++++++++++++++----- pkg/testmocks/mock_image_fetcher.go | 6 +-- 9 files changed, 61 insertions(+), 30 deletions(-) diff --git a/internal/builder/image_fetcher_wrapper.go b/internal/builder/image_fetcher_wrapper.go index 61ec4d40b5..71fbebf54d 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 e2c21722bf..9e09ecfde2 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 91cd597815..0a8a0d64cf 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 8d0272d2ef..fe8ebbd8a9 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 8ca77edd0e..e8e7f07ab8 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/client/example_fetcher_test.go b/pkg/client/example_fetcher_test.go index 12a1b65f10..d649e842d7 100644 --- a/pkg/client/example_fetcher_test.go +++ b/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 } diff --git a/pkg/image/fetcher.go b/pkg/image/fetcher.go index 2e941e3ce1..aa6a1b7200 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 f07598a489..a120ac053b 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 02bb715041..6fd890db2b 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. From e85ddee1f5fb2b41262a6ca51a1100f999e100e0 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Fri, 26 Apr 2024 17:00:19 -0500 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Natalie Arellano Co-authored-by: Ralf Pannemans Signed-off-by: Juan Bustamante Signed-off-by: Juan Bustamante --- internal/builder/image_fetcher_wrapper.go | 6 +++--- pkg/client/client.go | 4 ++-- pkg/image/fetcher.go | 5 ++--- pkg/image/fetcher_test.go | 4 ++-- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/internal/builder/image_fetcher_wrapper.go b/internal/builder/image_fetcher_wrapper.go index 71fbebf54d..16bf39b2a7 100644 --- a/internal/builder/image_fetcher_wrapper.go +++ b/internal/builder/image_fetcher_wrapper.go @@ -14,11 +14,11 @@ type ImageFetcher interface { // attempt to pull a remote image first. Fetch(ctx context.Context, name string, options image.FetchOptions) (imgutil.Image, error) - // CheckReadAccess verifies if an image 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 + // the behavior is dictated by the pull policy, which can have the following behavior // - PullNever: returns false - // - PullAlways Or PullIfNotPResent: it will check read access for the remote image. + // - 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. CheckReadAccess(repo string, options image.FetchOptions) bool } diff --git a/pkg/client/client.go b/pkg/client/client.go index fe8ebbd8a9..7dd899b20a 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -55,9 +55,9 @@ type ImageFetcher interface { // 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 + // the behavior is dictated by the pull policy, which can have the following behavior // - PullNever: returns false - // - PullAlways Or PullIfNotPResent: it will check read access for the remote image. + // - 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. CheckReadAccess(repo string, options image.FetchOptions) bool } diff --git a/pkg/image/fetcher.go b/pkg/image/fetcher.go index aa6a1b7200..9b8d0886b2 100644 --- a/pkg/image/fetcher.go +++ b/pkg/image/fetcher.go @@ -130,17 +130,16 @@ func (f *Fetcher) CheckReadAccess(repo string, options FetchOptions) bool { if _, err := f.fetchDaemonImage(repo); err != nil { if errors.Is(err, ErrNotFound) { // Image doesn't exist in the daemon - // Pull Never: should failed + // Pull Never: should fail // Pull If Not Present: need to check the registry if options.PullPolicy == PullNever { return false } return f.checkRemoteReadAccess(repo) } - f.logger.Debugf("failed reading image from the daemon %s, error: %s", repo, err.Error()) + f.logger.Debugf("failed reading image '%s' from the daemon, error: %s", repo, err.Error()) return false } - // no-op return true } diff --git a/pkg/image/fetcher_test.go b/pkg/image/fetcher_test.go index a120ac053b..b9491b24c2 100644 --- a/pkg/image/fetcher_test.go +++ b/pkg/image/fetcher_test.go @@ -426,14 +426,14 @@ func testFetcher(t *testing.T, when spec.G, it spec.S) { 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") + h.AssertContains(t, outBuf.String(), "failed reading image 'pack.test/dummy' 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") + h.AssertContains(t, outBuf.String(), "failed reading image 'pack.test/dummy' from the daemon") }) }) })