Adding Pull Policy into the logic to detect read access

Signed-off-by: Juan Bustamante <jbustamante@vmware.com>

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)
 }

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 {

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 {

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 {

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

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])
 		}
 	}

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
 }

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
 }

pkg/client/rebase.go

@@ -98,24 +98,28 @@ 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(),
 		"",
 		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
 	}

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
 	}
 }