Merge pull request #2092 from buildpacks/bugfix/jjbustamante/issue-2078

Adding a nop-op when trying to check access for run-image against the daemon

internal/builder/image_fetcher_wrapper.go

@@ -13,6 +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)
+
+	// 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 pull policy, 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.
+	CheckReadAccess(repo string, options image.FetchOptions) bool
 }
 
 type ImageFetcherWrapper struct {
@@ -32,3 +40,7 @@ func (w *ImageFetcherWrapper) Fetch(
 ) (Inspectable, error) {
 	return w.fetcher.Fetch(ctx, name, options)
 }
+
+func (w *ImageFetcherWrapper) CheckReadAccessValidator(repo string, options image.FetchOptions) bool {
+	return w.fetcher.CheckReadAccess(repo, options)
+}

internal/fakes/fake_image_fetcher.go

@@ -55,6 +55,10 @@ func (f *FakeImageFetcher) Fetch(ctx context.Context, name string, options image
 	return ri, nil
 }
 
+func (f *FakeImageFetcher) CheckReadAccess(_ string, _ image.FetchOptions) bool {
+	return true
+}
+
 func shouldPull(localFound, remoteFound bool, policy image.PullPolicy) bool {
 	if remoteFound && !localFound && policy == image.PullIfNotPresent {
 		return true

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)
+	CheckReadAccess(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, c.accessChecker)
-
 	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/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,

pkg/client/client.go

@@ -52,6 +52,14 @@ 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)
+
+	// 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 pull policy, 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.
+	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
@@ -80,13 +88,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) 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 +98,6 @@ type Client struct {
 	keychain            authn.Keychain
 	imageFactory        ImageFactory
 	imageFetcher        ImageFetcher
-	accessChecker       AccessChecker
 	downloader          BlobDownloader
 	lifecycleExecutor   LifecycleExecutor
 	buildpackDownloader BuildpackDownloader
@@ -133,14 +133,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 +233,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,

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

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, options image.FetchOptions) string {
 	if runImage != "" {
 		c.logger.Debugf("Using provided run-image %s", style.Symbol(runImage))
 		return runImage
@@ -44,7 +45,8 @@ func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, run
 		runImageMetadata.Image,
 		runImageMetadata.Mirrors,
 		additionalMirrors[runImageMetadata.Image],
-		accessChecker,
+		c.imageFetcher,
+		options,
 	)
 
 	switch {
@@ -108,8 +110,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, 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 {
@@ -127,11 +129,11 @@ func getBestRunMirror(registry string, runImage string, mirrors []string, prefer
 	return runImage
 }
 
-func filterImageList(imageList []string, accessChecker AccessChecker) []string {
+func filterImageList(imageList []string, fetcher ImageFetcher, options image.FetchOptions) []string {
 	var accessibleImages []string
 
 	for i, img := range imageList {
-		if accessChecker.Check(img) {
+		if fetcher.CheckReadAccess(img, options) {
 			accessibleImages = append(accessibleImages, imageList[i])
 		}
 	}