diff --git a/internal/builder/image_fetcher_wrapper.go b/internal/builder/image_fetcher_wrapper.go index 613a9647d..61ec4d40b 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 e95ce824b..e2c21722b 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 0857f2473..68bc32310 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 a3a31b2f8..60f29d40d 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 97fa306fa..8d0272d2e 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 d23cb2e95..8ca77edd0 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 09719f619..baa4f0b64 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 246302f59..12a1b65f1 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 c5fb7d946..3d7e6532d 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 e9958653c..2e941e3ce 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 e12c859f9..f07598a48 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 11177db22..02bb71504 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.